[nfsv4] Re: Genart last call review of draft-ietf-nfsv4-layrec-01

[Thomas Haynes <[e-mail]>]

Hi Dale.

Thanks for the review, I’m sorry I lost it in a storm of reviews over 3 documents.

I believe I have addressed all of your points, even if I did not directly reply to your points. I.e., an earlier rewrite might impact the scope of your comments.

I will push the changes per your review to be draft-ietf-nfsv4-layrec-02.

Thanks,
Tom


Comments are inline.

> On Aug 22, 2024, at 6:29 PM, Dale Worley via Datatracker <[e-mail]> wrote:
> 
> Reviewer: Dale Worley
> Review result: Ready with Issues
> 
> I am the assigned Gen-ART reviewer for this draft. The General Area
> Review Team (Gen-ART) reviews all IETF documents being processed
> by the IESG for the IETF Chair.  Please treat these comments just
> like any other last call comments.
> 
> For more information, please see the FAQ at
> 
> <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.
> 
> Document:  draft-ietf-nfsv4-layrec-01
> Reviewer:  Dale R. Worley
> Review Date:  2024-08-22
> IETF LC End Date:  2024-08-27
> IESG Telechat date:  [not known]
> 
> Summary:
> 
>    This draft is on the right track but has open issues, described in
>    the review.
> 
> Major issues:
> 
> There seems to be no discussion of upward-compatibility.
> Particularly, what happens if the metadata server supports this
> extension but the client does not, and what happens if the client
> supports this extension but the metadata server does not.  I expect
> that following the details of the text, the servers will work things
> out correctly, but it's best if there is a holistic description of the
> compatibility situations so the reader can verify that they've been
> handled correctly, and that the implementer knows what to look for in
> version-mismatch situations.

I added a section for version mismatching. 


> 
> The text is written for people who have the entirety of the previously
> defined protocol in their heads, and know all of the processing
> paths.  

I will not argue this point.



> That is, it's a very densely-written sent of amendments, with
> no clear indexing of exactly what execution paths are affected by what
> extensions/requirements.  It would be better if the items were broken
> apart, the text expanded, and keyed to the definitions of the
> procedures which are being amended.
> 
> Minor issues:
> 
> A section of the text is written in the subjunctive mood, despite that
> it seems to be the core of the extension.  It should be written
> normally, with as much use of normative keywords as possible.
> 
> Nits/editorial comments:
> 
> 1.1.  Definitions
> 
>   See Section 1.1 of [RFC8435] for a more complete set of definitions.
> 
> However, the definitions that are given here duplicate RFC 8435.
> Perhaps better to make that clear, e.g.
> 
>   The following definitions are from Section 1.1 of [RFC8435].  See it
>   for a more complete set of definitions.


As per Shuping Peng’s review, I now just point the reader to RFC8435.

> 
> 2.  Layout State Recovery
> 
>   The LAYOUTRETURN needs
>   a layout stateid to proceed and there is no way for the client to
>   recover layout state.
> 
> Does this mean "the previous layout stateid known by the client has
> been rendered invalid by the MDS restart and there is no way for the
> client to obtain a valid stateid"?  Or is there more to "recover
> layout state" than just obtaining a valid a stateid?


Expounded on this as to why it can’t use the layout it has and why it can’t get a new one during recovery.



> 
>   If the server were to allow the client to use the anonymous stateid
>   of all zeros (see Section 8.2.3 of [RFC8881]) for lrf_stateid in
>   LAYOUTRETURN (see Section 18.44.1 of [RFC8881]), then the client
>   could inform the metadata server of errors encountered.
> 
> This paragraph and the following three paragraphs are written in the
> subjunctive mood.  This leaves it unclear to the reader whether this
> the text is prescribing the extension mentioned in the Abstract, or
> just presenting a proposal which will be discussed further.
> 
>   That in turn
>   would allow the metadata server to accurately resilver the file by
>   picking the correct mirror(s).
> 
> It seems desirable to state explicitly here how the metadata server
> "picks the correct mirror(s)", given that the LAYOUTRETURN seems to be
> used to inform the MDS of the copies that are corrupt rather than the
> ones that are not corrupt.  It's possible that this process is implied
> by previously defined parts of NFSv4.1, but if so it might be useful
> to point the reader to that description.

Fixed this inadvertently in doing the change for above.


> 
>   There are two error scenarios that can occur:
> 
>   During the grace period:  If the client were to send any lrf_stateid
>      in the LAYOUTRETURN other than the anonymous stateid of all zeros,
>      then the metadata server would respond with an error of
>      NFS4ERR_GRACE (see Section of 15.1.9.2 [RFC8881]).
> 
>   After the grace period:  If the client were to send any lrf_stateid
>      in the LAYOUTRETURN with the anonymous stateid of all zeros, then
>      the metadata server would respond with an error of
>      NFS4ERR_NO_GRACE (see Section 15.1.9.3 of [RFC8881]).
> 
> This text is unclear what of this error processing is added in the
> extension and what of it is part of the base that this document
> extends.

Added MUST now to try to clear that up.

>  Also, the text says "the metadata server would respond"
> rather than "the metadata server MUST respond" or other normative
> language that makes it clear how strict the requirements are.
> 
>   Also, when the metadata server builds the reply to the LAYOUTRETURN,
>   it MUST NOT bump the seqid of the lorr_stateid.
> 
> My suspicion is that "it MUST NOT bump the seqid of the lorr_stateid"
> only applies when the stateid of the request is all zeros, but the
> text doesn't state that, inviting implementation errors.

Thanks, subtle point.


> 
>   The metadata
>   server MUST NOT have been resilvering the file such that it has a
>   different layout (set of mirror instances) than the client before the
>   restart of the metadata server.
> 
> Presumably the metadata server might be restarted at any instant,
> regardless of what tasks it was executing.  That seems to make it
> impossible to conform to this requirement.  I suspect that the meaning
> is
> 
>   If the metadata server at the point of restarting was resilvering
>   the file such that the MDS has a different layout (set of mirror
>   instances) than the client, then upon restart, the MDS MUST NOT allow
>   <the procedures described in this document> [<-- replace those
>   words with the correct term].


Your point is valid, but the result is off.

Say the mds restarts and client A has a RW layout.

The mds cannot resilver until the client either recovers the file or the grace period expires.

If the client sends an error, then the resilvering can proceed. If the client sends no error, but does reclaim the file, then there is no need to resilver.

If the client does not respond before the grace period ends, the server would:

a) Fence the layout (change the uid/gid on the instances such that the client has no access with the old layout)
b) Record the file needs resilvering
c) Delete the write intent record (the record that allows it to know that the file needs recovering)
d) Start resilvering the file

If the mds restarts before c), then it just starts the process over when it comes back up.

If it restarts after d), then it knows that the client does not have a valid RW layout and can just start the resilvering.

You have raised a great question here, let me take the reply above and reword the text.


> 
> Also this assumes that the metadata server has some way of knowing
> what layout the client has, despite that the metadata server has
> restarted.  Presumably that is previously defined in NFSv4.1, but it
> might be helpful to point to that.

Agreed


> 
>   The metadata server MUST NOT resilver a file if there are clients
>   with outstanding layouts with iomode of LAYOUTIOMODE4_RW.  Whether
>   the metadata server prevents all I/O to the file until the
>   resilvering is done or [...]
> 
> Are these two sentences related?  The first talks of when the MDS must
> not resilver, but the second talks of what is to happen when
> resilvering is being done.  It seems that there should be a paragraph
> break here and then some text setting the context in which "The
> metadata server prevents all I/O ... or ...".
> 
>   Whether
>   the metadata server prevents all I/O to the file until the
>   resilvering is done or forces all I/O to go through the metadata
>   server or allows a proxy server to update the new data file as it is
>   being reslivered is all an implementation choice.
> 
> This sentence interacts in complex ways with a lot of specifics of how
> the metadata server behaves.  I assume that the WG has verified that
> this description is sufficient to clearly specify what behaviors are
> allowed by the client and servers.
> 


Yes 


>   Finally, the metadata server MUST determine that any files which are
>   neither explicitly recovered with a CLAIM_PREVIOUS nor have a
>   reported error via a LAYOUTRETURN, have to be resilvered.
> 
> would be simpler as
> 
>   Finally, the metadata server MUST resilver any files which are
>   neither explicitly recovered with a CLAIM_PREVIOUS nor have a
>   reported error via a LAYOUTRETURN.


Agreed


> 
> 4.  IANA Considerations
> 
>   IANA should use the current document (RFC-TBD) as the reference for
>   the new entries.
> 

Yes, will be fixed.


> This text speaks of new entries but does not specify what the new
> entries are.  As far as I can tell, there are no new entries in the
> IANA databases.  The text should be adjusted appropriately.
> 
> [END]
> 
> 
> 
> _______________________________________________
> nfsv4 mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]