[nfsv4] Re: Artart last call review of draft-ietf-nfsv4-layoutwcc-04

[Thomas Haynes <[e-mail]>]

Hi Carsten,

Thanks for the review, comments inline.

I really appreciate the reference comment, it will make my drafts better. And I am sure I will like learning how to link the sections.

> On Nov 19, 2024, at 12:24 AM, Carsten Bormann via Datatracker <[e-mail]> wrote:
> 
> Reviewer: Carsten Bormann
> Review result: Ready with Issues
> 
> (Insert ARTART review boilerplate here.)
> 
> I have been using NFS and XDR since the early 1980s, but I am not an
> expert on NFSv4.2 or pNFS.
> 
> I did not find a github repository for the draft, so I'll provide
> comments in a more traditional/tedious form.  (Please use the
> venue/"about this document" facilities of document generation to make
> the repo easy to find for reviewers.)


[e-mail]:loghyr/layout_wcc.git

I’ve been using GitHub a long time before many WGs adopted it, so this probably appears rather simplistic to what is now standard.


> 
> Any page numbers below are those in the plaintext form of the I-D.
> 
> ## Major
> 
> There is an editors' note that appear to be unanswered questions:
> §2 (p5):
>   // Can it go into LAYOUTRETURN?
> Please answer.
> 

The answer is no - not without a lot of effort.

Not sure how I forgot to remove this - I might have added it late, not realizing we were at a LC.

Removed the AI.


> §3.4.2 says: The reason to
>   provide these two attributes is in case of NFS4ERR_ACCESS, the
>   metadata server can compare what it expects the values of the uid and
>   gid of the data file to be versus the actual values.  It can then
>   repair the permissions as needed or modify the expected values it has
>   cached.
> To someone not familiar with the underlying protocols, this appears to
> be very weak advice on what the metadata server should be doing here.
> In particular, is such a "repair" already covered by the Security
> Considerations in [RFC7862] (which is all the Security Considerations
> in this document amount to)?

No, not RFC7862, but RFC8435, see Section 2.2.

The security model of RFC8435 is set the mode bits to be S_IRUSR | S_IWUSR | S_IRGRP

The user can read and write. The group can read.

Then the uid and gid are set to specific values. These values are sent to the client in the LAYOUTGET reply.

If the client requests a LAYOUTIOMODE4_READ layout, then the uid is mangled in the reply such that the client can not WRITE with those credentials.

The quoted text in Section 3.4.2. is stating that if the uid and gid reported in the LAYOUT_WCC do not match those stored for the data file, then the server either needs to use a SETATTR to adjust the data file or it needs to update the stored values.

I can be more explicit in the text if needed to be.



> 
> Also, it says:
>   The mapping of NFSv3 to NFSv4 attributes shown in Table 1 also
>   details which attributes the LAYOUT_WCC SHOULD be providing to the
>   metadata server, [...]
> Does it?  I cannot find that information.
> Or is the intended meaning that *all* the attributes shown in Table 1
> SHOULD be provided?

*all*


> What is the limit of the SHOULD (i.e., under what specific
> circumstances can that be overridden)?

New text is:

The NFSv3 attributes returned in the WCC of WRITE, READ, and COMMIT are a smaller subset
 of what can be transmitted as a NFSv4 attribute. The mapping of NFSv3 to NFSv4 attributes
is shown in Table 1. The LAYOUT_WCC MUST provide all of these attributes to the metadata server.
Both the uid and gid are stringified into their respective attributes of owner and owner_group.
….




> 
> (I note that there is a lower-case "should" in Section 3.4.1 (p6);
> please check that this is the intended [non-2119] meaning.)
> 


It is the intended meaning.

Changed “should” to “can”.


> ## Minor
> 
> The definitions (1.1) should probably mention any other sources of
> definitions that this document relies on (e.g., for "layout").


Lol - I’ve been getting requests to shorten these sections by pointing the reader to earlier documents.

I.e., in layout_rec, I state:

      See Section 1.1 of [RFC8435] for a set of definitions.

See https://datatracker.ietf.org/doc/draft-ietf-nfsv4-layrec/

If anything, I would remove all of the definitions here except WCC and point the reader to RFC8435.

But I currently have:

1.1.  Definitions
   
   See Section 1.1 of [RFC8435] for a fuller set of definitions.


> 
> Section 3.6 could hint at how the optionality is handled (e.g., does
> the client stop sending the operation when it gets a specific error
> from the server?).
> (The answer may be obvious with the level of knowledge about NFSv4.2
> actually required by this document, please ignore this observation
> then.)

It would move to REQUIRED with NFSv4.3 along with all of the OPTIONAL ops of RFC8435.


> 
> Section 3.7:
>   But the positional correspondence between the elements is not
>   sufficient to determine the attributes to update.
>   [...]
>   In either case, the combination of ffdsw_deviceid, ffdsw_stateid, and
>   ffdsw_fh_vers will uniquely identify the attributes to be updated.
> Is this really about the specific attributes to update/to be updated
> or is it actually about identifying the information object (mirror?)
> in which the attributes are to be updated?


It is about identifying the information object, which is a mirror.


> 
> One would expect Section 4 to be boilerplate now; does this differ in
> any way from similar sections in previous documents?
> 

No, just the name of the final file are changed.


> ## Nits
> 
> Nits are given as a pair of old and new lines, if possible.
> 
> Abstract (p1)
>   It does not provide a mechanism for the data server to update the
>   metadata server of changes to the data part of the file.  The client
> Abstract(p1)
>   allow the client to update the metadata server to changes on the data
> Which one should it be -- update "of" or update "to"?
> 

“To"





> §1.1 (p3): ease of use
>   Section 2.1 of [RFC8434]
>   (enable directly linking the section reference on this citation)
> (This is a recurring comment to a couple dozen other places in the
> document as well.)
> 


More than willing to do that, if you let me know how.


> §2 (p4): Typo
>   Because there is no contol protocol (see [RFC8434]) possible with all
>   Because there is no control protocol (see [RFC8434]) possible with all
> 

Ack


> §2 (p5): consistency
> Is it "flexible files layout" (plural, used here) or "Flexible File
> Layout" (singular, as in abstract/introduction)?
> 


Singular. Fixed up in the places I could find


> §2 (p5): typo
>   model, the metatdate server MAY make such calls anyway in order to
>   model, the metadata server MAY make such calls anyway in order to
> 

Ack

> §3.4.1 (p6): typo
>   data files.  While The client can send a LAYOUT_WCC at any time,
>   data files.  While the client can send a LAYOUT_WCC at any time,
> 

Ack


> §3.4.2 (p7): typo
>   metadata server, Both the uid and gid are stringified into their
>   metadata server. Both the uid and gid are stringified into their
> 

Ack


> §3.4.2 (p7): Table 1 could use a caption

Ack


> 
> §3.5 (p9): Table 2 could use a caption, maybe moving the stray text line
>   Valid Error Returns for LAYOUT_WCC
> Is there anything gained from presenting this list in an essentially
> single-cell table?
> Maybe it could be formatted to resemble Table 12 (Valid Error Returns
> for Each Protocol Operation) in Section 15.2 of [RFC8881]?
> 

Ack


> §4 (p10)): Typo
>   shell script to produce the machine readable XDR description of the
>   shell script to produce the machine-readable XDR description of the
> 

Ack

> §4 (p11): Typo
>   code snippets belong in their respective areas of the that XDR.
>   code snippets belong in their respective areas of that XDR.
> 

Ack

> §7.1 (p11): missing link
> This reference seems to be hand-built and is providing an unusual citation of
> the draft name (which is also missing a link): [delstid] Haynes, T. and T.
> Myklebust, "Extending the Opening of Files in NFSv4.2",
> draft-ietf-nfsv4-delstid-02.xml (Work In Progress), February 2023. Please use
> the references from bib.ietf.org instead.


That draft is not finished going through the EDIT cycle.


Wow, followed your advice and that is nice. Thanks!


> 
> ## Sorry, could not resist
> 
> §4 (p10): (would be even easier with sed -E)
>   grep '^ *///' $* | sed 's?^ */// ??' | sed 's?^ *///$??'
>   sed -n 's,^ *///,,p' "$@" | sed 's/^ //‘
> 

This has been in place for 20 years, loathe to change it at all. :-)


> 
>