On Nov 19, 2024, at 12:24 AM, Carsten Bormann via Datatracker <noreply@xxxxxxxx> 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.)
git@xxxxxxxxxx: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.
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
