[nfsv4] Re: Artart last call review of draft-ietf-nfsv4-delstid-06

[Thomas Haynes <[e-mail]>]

Hi Henry,

Thanks for the review, sorry for the delay in response, I have had work project deadlines.

> On Aug 26, 2024, at 2:40 AM, Henry Thompson via Datatracker <[e-mail]> wrote:
> 
> Reviewer: Henry Thompson
> Review result: Ready with Issues
> 
> Document: draft-ietf-nfsv4-delstid-06
> Intended RFC status: Proposed Standard
> Review type: artart - Last Call review
> Reviewer: Henry S. Thompson
> Review Date: 2024-08-26
> Result: Ready with minor issues
> 
> *Summary*
> 
> *Substantive points*
> 
> *Minor points*
> 
> Section 2.1: Probably worth mentioning that the 'CODE' shown here is
> defined per RFC 4506.


Section 1 states:

   Using the process detailed in [RFC8178], the revisions in this
   document become an extension of NFSv4.2 [RFC7862].  They are built on
   top of the external data representation (XDR) [RFC4506] generated
   from [RFC7863].

Is that sufficient or do you still want a call out?

> 
> Section 4: "The open stateid field, OPEN4resok.stateid ..., will MUST
>            be set to the special all zero"
> 
> There's a typo here, and in any case I _think_ it should be expanded
> slightly for clarity:
> 
>  "The open stateid field, OPEN4resok.stateid ..., MUST be set
>   to the special all zero in this case."

Both fixed.


> 
> Section 4.1: I'm not familiar with the implementation details of NFS,
> but I find this discussion difficult to follow.  Perhaps it would help
> if it were expanded to show what the two compounds sequences look like
> for the two different values of
> OPEN_ARGS_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION.
> 

I’ve added two “figures”:

   Consider a small workload of creating a file with content.  That
   takes 3 synchronous and 1 asynchronous operations with existing
   implementations.  The first synchronous one has to OPEN the file, the
   second synchronous one performs the WRITE to the file, the third
   synchronous one has to CLOSE the file, and the fourth asynchronous
   one uses DELEGRETURN (see Section 18.6 of [RFC8881]) to return the
   delegation stateid.
   
   <CODE BEGINS> 
         SEQ PUTFH OPEN GETFH GETATTR
         SEQ PUTFH WRITE GETATTR
         SEQ PUTFH CLOSE
         ...
         SEQ PUTFH DELEGRETURN
   <CODE ENDS>
   
   With the proposed approach of setting the
   OPEN_ARGS_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION flag during the OPEN,
   the number of operations is always 3.  The first two compounds are
   still synchronous, but the last is asynchronous.  I.e., since the
   client no longer has to send a CLOSE operation, it can delay the
   DELEGRETURN until either the server requests it back via delegation
   recall or garbage collection causes the client to return the stateid.
   
   <CODE BEGINS>
         SEQ PUTFH OPEN(OPEN_ARGS_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION)
             GETFH GETATTR
         SEQ PUTFH WRITE GETATTR
         ...
         SEQ PUTFH DELEGRETURN
   <CODE ENDS>


Please let me know if that suffices.



> Section 5: As far as the protocol itself is concerned, this section
> looks OK to me, but the mixture of BCP14 keywords and ordinary
> language to describe the correct _use_ of the protocol with respect to
> the various times involved is quite confusing, and would benefit from
> a more structure, in the form of an analysis by cases.
> 

I find this quite vague.

Are you asking me to invert the order of the text to be BCP14 keyword statements and the supporting text or what?

I.e., I don’t know what you want to happen here and I’m confused as to why a major rewrite would be considered a “Minor point”.

> *Nits*
> 
> Section 3: "Note that as these flags MUST only change from OPTIONAL
> to REQUIRED when the NFSv4 minor version is incremented" --- something
> wrong with the grammar here, possibly just delete "as".


Done

> 
> Section 6: I think "the that" should be just "that", but you may
> possibly have meant just "the".

Removed ’the"


> 
> ht
> -- 
> 
> 
> 
> 
> _______________________________________________
> nfsv4 mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]