[art] Re: Artart last call review of draft-ietf-nfsv4-layout wcc-04
Thomas Haynes <[email protected]> Wed, 20 Nov 2024 10:49:05 -0800
| Newsgroups | gmane.ietf.apps-discuss,gmane.ietf.nfsv4 |
|---|---|
| Message-ID | <[email protected]> |
--===============3893820663543056819== Content-Type: multipart/alternative; boundary="Apple-Mail=_736F2665-5612-4D0A-8B76-D3200F415A8D" --Apple-Mail=_736F2665-5612-4D0A-8B76-D3200F415A8D Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 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=E2=80=AFAM, Carsten Bormann via Datatracker = <[email protected]> wrote: >=20 > Reviewer: Carsten Bormann > Review result: Ready with Issues >=20 > (Insert ARTART review boilerplate here.) >=20 > I have been using NFS and XDR since the early 1980s, but I am not an > expert on NFSv4.2 or pNFS. >=20 > 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.) [email protected]:loghyr/layout_wcc.git I=E2=80=99ve been using GitHub a long time before many WGs adopted it, = so this probably appears rather simplistic to what is now standard. >=20 > Any page numbers below are those in the plaintext form of the I-D. >=20 > ## Major >=20 > There is an editors' note that appear to be unanswered questions: > =C2=A72 (p5): > // Can it go into LAYOUTRETURN? > Please answer. >=20 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. > =C2=A73.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. >=20 > 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. =E2=80=A6. >=20 > (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.) >=20 It is the intended meaning. Changed =E2=80=9Cshould=E2=80=9D to =E2=80=9Ccan=E2=80=9D. > ## Minor >=20 > The definitions (1.1) should probably mention any other sources of > definitions that this document relies on (e.g., for "layout"). Lol - I=E2=80=99ve 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 =20 See Section 1.1 of [RFC8435] for a fuller set of definitions. >=20 > 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. >=20 > 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. >=20 > One would expect Section 4 to be boilerplate now; does this differ in > any way from similar sections in previous documents? >=20 No, just the name of the final file are changed. > ## Nits >=20 > Nits are given as a pair of old and new lines, if possible. >=20 > 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"? >=20 =E2=80=9CTo" > =C2=A71.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.) >=20 More than willing to do that, if you let me know how. > =C2=A72 (p4): Typo > Because there is no contol protocol (see [RFC8434]) possible with = all > Because there is no control protocol (see [RFC8434]) possible with = all >=20 Ack > =C2=A72 (p5): consistency > Is it "flexible files layout" (plural, used here) or "Flexible File > Layout" (singular, as in abstract/introduction)? >=20 Singular. Fixed up in the places I could find > =C2=A72 (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 >=20 Ack > =C2=A73.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, >=20 Ack > =C2=A73.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 >=20 Ack > =C2=A73.4.2 (p7): Table 1 could use a caption Ack >=20 > =C2=A73.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]? >=20 Ack > =C2=A74 (p10)): Typo > shell script to produce the machine readable XDR description of the > shell script to produce the machine-readable XDR description of the >=20 Ack > =C2=A74 (p11): Typo > code snippets belong in their respective areas of the that XDR. > code snippets belong in their respective areas of that XDR. >=20 Ack > =C2=A77.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! >=20 > ## Sorry, could not resist >=20 > =C2=A74 (p10): (would be even easier with sed -E) > grep '^ *///' $* | sed 's?^ */// ??' | sed 's?^ *///$??' > sed -n 's,^ *///,,p' "$@" | sed 's/^ //=E2=80=98 >=20 This has been in place for 20 years, loathe to change it at all. :-) >=20 >=20 --Apple-Mail=_736F2665-5612-4D0A-8B76-D3200F415A8D Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 <html><head><meta http-equiv=3D"content-type" content=3D"text/html; = charset=3Dutf-8"></head><body style=3D"overflow-wrap: break-word; = -webkit-nbsp-mode: space; line-break: after-white-space;">Hi = Carsten,<div><br></div><div>Thanks for the review, comments = inline.</div><div><br></div><div>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.</div><div><div><br><blockquote = type=3D"cite"><div>On Nov 19, 2024, at 12:24=E2=80=AFAM, Carsten Bormann = via Datatracker <[email protected]> wrote:</div><br = class=3D"Apple-interchange-newline"><div><div>Reviewer: Carsten = Bormann<br>Review result: Ready with Issues<br><br>(Insert ARTART review = boilerplate here.)<br><br>I have been using NFS and XDR since the early = 1980s, but I am not an<br>expert on NFSv4.2 or pNFS.<br><br>I did not = find a github repository for the draft, so I'll provide<br>comments in a = more traditional/tedious form. (Please use the<br>venue/"about = this document" facilities of document generation to make<br>the repo = easy to find for = reviewers.)<br></div></div></blockquote><div><br></div><div><br></div><div= >[email protected]:loghyr/layout_wcc.git</div><div><br></div><div>I=E2=80=99v= e been using GitHub a long time before many WGs adopted it, so this = probably appears rather simplistic to what is now = standard.</div><div><br></div><br><blockquote = type=3D"cite"><div><div><br>Any page numbers below are those in the = plaintext form of the I-D.<br><br>## Major<br><br>There is an editors' = note that appear to be unanswered questions:<br>=C2=A72 (p5):<br> = // Can it go into LAYOUTRETURN?<br>Please = answer.<br><br></div></div></blockquote><div><br></div><div>The answer = is no - not without a lot of effort.</div><div><br></div><div>Not sure = how I forgot to remove this - I might have added it late, not realizing = we were at a LC.</div><div><br></div><div>Removed the = AI.</div><div><br></div><br><blockquote type=3D"cite"><div><div>=C2=A73.4.= 2 says: The reason to<br> provide these two attributes is in = case of NFS4ERR_ACCESS, the<br> metadata server can compare = what it expects the values of the uid and<br> gid of the = data file to be versus the actual values. It can then<br> = repair the permissions as needed or modify the expected = values it has<br> cached.<br>To someone not familiar with = the underlying protocols, this appears to<br>be very weak advice on what = the metadata server should be doing here.<br>In particular, is such a = "repair" already covered by the Security<br>Considerations in [RFC7862] = (which is all the Security Considerations<br>in this document amount = to)?<br></div></div></blockquote><div><br></div><div>No, not RFC7862, = but RFC8435, see Section 2.2.</div><div><br></div><div>The security = model of RFC8435 is set the mode bits to be S_IRUSR | S_IWUSR | = S_IRGRP</div><div><br></div><div>The user can read and write. The group = can read.</div><div><br></div><div>Then the uid and gid are set to = specific values. These values are sent to the client in the LAYOUTGET = reply.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>I can be more explicit in the text if = needed to be.</div><div><br></div><div><br></div><br><blockquote = type=3D"cite"><div><div><br>Also, it says:<br> The mapping = of NFSv3 to NFSv4 attributes shown in Table 1 also<br> = details which attributes the LAYOUT_WCC SHOULD be providing = to the<br> metadata server, [...]<br>Does it? I cannot = find that information.<br>Or is the intended meaning that *all* the = attributes shown in Table 1<br>SHOULD be = provided?<br></div></div></blockquote><div><br></div><div>*all*</div><div>= <br></div><br><blockquote type=3D"cite"><div><div>What is the limit of = the SHOULD (i.e., under what specific<br>circumstances can that be = overridden)?<br></div></div></blockquote><div><br></div><div>New text = is:</div><div><br></div><div><div>The NFSv3 attributes returned in the = WCC of WRITE, READ, and COMMIT are a smaller subset</div><div> of = what can be transmitted as a NFSv4 attribute. The mapping of NFSv3 to = NFSv4 attributes</div><div>is shown in Table 1. The = LAYOUT_WCC MUST provide all of these attributes to the = metadata server.</div><div>Both the uid and gid are stringified into = their respective attributes of owner and = owner_group.</div><div>=E2=80=A6.</div><div><br></div><div><br></div></div= ><div><br></div><br><blockquote type=3D"cite"><div><div><br>(I note that = there is a lower-case "should" in Section 3.4.1 (p6);<br>please check = that this is the intended [non-2119] = meaning.)<br><br></div></div></blockquote><div><br></div><div><br></div><d= iv>It is the intended meaning.</div><div><br></div><div>Changed = =E2=80=9Cshould=E2=80=9D to = =E2=80=9Ccan=E2=80=9D.</div><div><br></div><br><blockquote = type=3D"cite"><div><div>## Minor<br><br>The definitions (1.1) should = probably mention any other sources of<br>definitions that this document = relies on (e.g., for = "layout").<br></div></div></blockquote><div><br></div><div><br></div><div>= Lol - I=E2=80=99ve been getting requests to shorten these sections by = pointing the reader to earlier documents.</div><div><br></div><div>I.e., = in layout_rec, I state:</div><div><br></div><div><div> = See Section 1.1 of [RFC8435] for a set of = definitions.</div></div><div><br></div><div>See <a = href=3D"https://datatracker.ietf.org/doc/draft-ietf-nfsv4-layrec/">https:/= /datatracker.ietf.org/doc/draft-ietf-nfsv4-layrec/</a></div><div><br></div= ><div>If anything, I would remove all of the definitions here except WCC = and point the reader to RFC8435.</div><div><br></div><div>But I = currently have:</div><div><br></div><div><div>1.1. = Definitions</div><div> </div><div> See = Section 1.1 of [RFC8435] for a fuller set of = definitions.</div></div><div><br></div><br><blockquote = type=3D"cite"><div><div><br>Section 3.6 could hint at how the = optionality is handled (e.g., does<br>the client stop sending the = operation when it gets a specific error<br>from the server?).<br>(The = answer may be obvious with the level of knowledge about = NFSv4.2<br>actually required by this document, please ignore this = observation<br>then.)<br></div></div></blockquote><div><br></div><div>It = would move to REQUIRED with NFSv4.3 along with all of the OPTIONAL ops = of RFC8435.</div><div><br></div><br><blockquote = type=3D"cite"><div><div><br>Section 3.7:<br> But the = positional correspondence between the elements is not<br> = sufficient to determine the attributes to update.<br> = [...]<br> In either case, the combination of = ffdsw_deviceid, ffdsw_stateid, and<br> ffdsw_fh_vers will = uniquely identify the attributes to be updated.<br>Is this really about = the specific attributes to update/to be updated<br>or is it actually = about identifying the information object (mirror?)<br>in which the = attributes are to be = updated?<br></div></div></blockquote><div><br></div><div><br></div><div>It= is about identifying the information object, which is a = mirror.</div><div><br></div><br><blockquote = type=3D"cite"><div><div><br>One would expect Section 4 to be boilerplate = now; does this differ in<br>any way from similar sections in previous = documents?<br><br></div></div></blockquote><div><br></div><div>No, just = the name of the final file are = changed.</div><div><br></div><br><blockquote type=3D"cite"><div><div>## = Nits<br><br>Nits are given as a pair of old and new lines, if = possible.<br><br>Abstract (p1)<br> It does not provide a = mechanism for the data server to update the<br> metadata = server of changes to the data part of the file. The = client<br>Abstract(p1)<br> allow the client to update the = metadata server to changes on the data<br>Which one should it be -- = update "of" or update = "to"?<br><br></div></div></blockquote><div><br></div><div>=E2=80=9CTo"</di= v><div><br></div><div><br></div><div><br></div><div><br></div><br><blockqu= ote type=3D"cite"><div><div>=C2=A71.1 (p3): ease of use<br> = Section 2.1 of [RFC8434]<br> (enable directly = linking the section reference on this citation)<br>(This is a recurring = comment to a couple dozen other places in the<br>document as = well.)<br><br></div></div></blockquote><div><br></div><div><br></div><div>= More than willing to do that, if you let me know = how.</div><div><br></div><br><blockquote type=3D"cite"><div><div>=C2=A72 = (p4): Typo<br> Because there is no contol protocol (see = [RFC8434]) possible with all<br> Because there is no control = protocol (see [RFC8434]) possible with = all<br><br></div></div></blockquote><div><br></div><div>Ack</div><div><br>= </div><br><blockquote type=3D"cite"><div><div>=C2=A72 (p5): = consistency<br>Is it "flexible files layout" (plural, used here) or = "Flexible File<br>Layout" (singular, as in = abstract/introduction)?<br><br></div></div></blockquote><div><br></div><di= v><br></div><div>Singular. Fixed up in the places I could = find</div><div><br></div><br><blockquote type=3D"cite"><div><div>=C2=A72 = (p5): typo<br> model, the metatdate server MAY make such = calls anyway in order to<br> model, the metadata server MAY = make such calls anyway in order = to<br><br></div></div></blockquote><div><br></div><div>Ack</div><br><block= quote type=3D"cite"><div><div>=C2=A73.4.1 (p6): typo<br> = data files. While The client can send a LAYOUT_WCC at = any time,<br> data files. While the client can send a = LAYOUT_WCC at any = time,<br><br></div></div></blockquote><div><br></div><div>Ack</div><div><b= r></div><br><blockquote type=3D"cite"><div><div>=C2=A73.4.2 (p7): = typo<br> metadata server, Both the uid and gid are = stringified into their<br> metadata server. Both the uid and = gid are stringified into = their<br><br></div></div></blockquote><div><br></div><div>Ack</div><div><b= r></div><br><blockquote type=3D"cite"><div><div>=C2=A73.4.2 (p7): Table = 1 could use a = caption<br></div></div></blockquote><div><br></div>Ack</div><div><br></div= ><div><br></div><div><blockquote type=3D"cite"><div><div><br>=C2=A73.5 = (p9): Table 2 could use a caption, maybe moving the stray text line<br> = Valid Error Returns for LAYOUT_WCC<br>Is there anything = gained from presenting this list in an essentially<br>single-cell = table?<br>Maybe it could be formatted to resemble Table 12 (Valid Error = Returns<br>for Each Protocol Operation) in Section 15.2 of = [RFC8881]?<br><br></div></div></blockquote><div><br></div><div>Ack</div><d= iv><br></div><br><blockquote type=3D"cite"><div><div>=C2=A74 (p10)): = Typo<br> shell script to produce the machine readable XDR = description of the<br> shell script to produce the = machine-readable XDR description of = the<br><br></div></div></blockquote><div><br></div><div>Ack</div><br><bloc= kquote type=3D"cite"><div><div>=C2=A74 (p11): Typo<br> code = snippets belong in their respective areas of the that XDR.<br> = code snippets belong in their respective areas of that = XDR.<br><br></div></div></blockquote><div><br></div><div>Ack</div><br><blo= ckquote type=3D"cite"><div><div>=C2=A77.1 (p11): missing link<br>This = reference seems to be hand-built and is providing an unusual citation = of<br>the draft name (which is also missing a link): [delstid] Haynes, = T. and T.<br>Myklebust, "Extending the Opening of Files in = NFSv4.2",<br>draft-ietf-nfsv4-delstid-02.xml (Work In Progress), = February 2023. Please use<br>the references from bib.ietf.org = instead.<br></div></div></blockquote><div><br></div><div><br></div><div>Th= at draft is not finished going through the EDIT = cycle.</div><div><br></div><div><br></div><div>Wow, followed your advice = and that is nice. Thanks!</div><div><br></div><br><blockquote = type=3D"cite"><div><div><br>## Sorry, could not resist<br><br>=C2=A74 = (p10): (would be even easier with sed -E)<br> grep '^ *///' = $* | sed 's?^ */// ??' | sed 's?^ *///$??'<br> sed -n 's,^ = *///,,p' "$@" | sed 's/^ = //=E2=80=98<br><br></div></div></blockquote><div><br></div><div>This has = been in place for 20 years, loathe to change it at all. = :-)</div><div><br></div><br><blockquote = type=3D"cite"><div><div><br><br></div></div></blockquote></div><br></div><= div><br></div></body></html>= --Apple-Mail=_736F2665-5612-4D0A-8B76-D3200F415A8D-- --===============3893820663543056819== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KYXJ0IG1haWxp bmcgbGlzdCAtLSBhcnRAaWV0Zi5vcmcKVG8gdW5zdWJzY3JpYmUgc2VuZCBhbiBlbWFpbCB0byBh cnQtbGVhdmVAaWV0Zi5vcmcK --===============3893820663543056819==--