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 <noreply@xxxxxxxx> 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 -- nfsv4@xxxxxxxx
> To unsubscribe send an email to nfsv4-leave@xxxxxxxx
--
last-call mailing list -- last-call@xxxxxxxx
To unsubscribe send an email to last-call-leave@xxxxxxxx
