[nfsv4] AD review: draft-ietf-nfsv4-minorversion1-24

[ext Lars Eggert <[e-mail]>]

This was an extremely readable and clear document - it's obvious that  
the WG has spent a lot of time honing the text. Thanks!

I'm under no illusions about being able to review the specific  
technical details of the NFSv4.1 protocol itself. This will be up to  
the expert reviewers assigned by the IESG. What I can say after  
reading the document is that it does give a very readable overview of  
the different components of NFS, which did allow a non-expert such as  
myself understand much more of the architecture and mechanisms in the  
later sections than I had hoped. Nice job. I believe this is very  
close to being ready for publication.

I have a few comments on various sections and a list of editorial  
nits. The most important comments are likely those related to the IANA  
considerations section - I do believe we need another quick revision  
that rephrases the desired IANA actions in a way that follows the  
guidelines in RFC5226. Doing this now will remove ambiguities and the  
potential for misunderstandings that could draw out approval of the  
document at a later stage, when IANA would need to ask for  
clarifications.

Note to the chairs: I still haven't received the publication request  
(incl. document writeup) for this document and the dot-x draft.

--------------------------------------------------------------------------
COMMENTS
--------------------------------------------------------------------------


Section 1.6.3.2., paragraph 1:
 > The NFSv4.1 protocol has a rich and extensible attribute structure,
 > which is divided into REQUIRED, RECOMMENDED, and named attributes.

   The rest of this section talks about RECOMMENDED and named  
attributes,
   but not REQUIRED ones. Should a sentence be added about what these
   are?


Section 2.2.1.1.1.1., paragraph 3:
 > NFSv4.1 client and servers MUST support RPCSEC_GSS's integrity and
 > authentication service.  NFSv4.1 servers MUST support RPCSEC_GSS's
 > privacy service.

   So there is no requirement or at least a recommendation for clients  
to
   implement the privacy service? We should discuss this with the
   security folks - my impression was that privacy needs to be a
   MUST-implement-need-not-use feature. (But I may be wrong.)


Section 2.7., paragraph 2:
 > The base assumption with respect to minor versioning is that any
 > future accepted minor version must follow the IETF process and be
 > documented in a standards track RFC.  Therefore, each minor version
 > number will correspond to an RFC.

   This procedure means that the NFSv4 WG will continue to produce
   "monolithic" RFCs that specify an entire minor version in one
   document. I'd like to at least allow (maybe even recommend) that an
   NFS minor version may be described in a set of interrelated RFCs.


Section 2.9.1., paragraph 7:
 > It is permissible for a connectionless transport to be used under
 > NFSv4.1, however reliable and in-order delivery of data by the
 > connectionless transport is REQUIRED.

   I'd like to require (or at the very least recommend) that congestion
   control be used with connectionless transports, too.


Section 2.10.3.1., paragraph 0:
 > 2.10.3.1.  Association of Connections, Channels, and Sessions

   Although its implicitly clear that one channel can be associated with
   more than one transport connection of the same type, the section  
never
   explicitly says so. Suggest to add a short sentence on that.


Section 2.10.4., paragraph 1:
 > Trunking is the use of multiple connections between a client and
 > server in order to increase the speed of data transfer.  NFSv4.1
 > supports two types of trunking: session trunking and client ID
 > trunking.  NFSv4.1 servers MUST support trunking.

   Must support both types of trunking?


Section 2.10.6.2., paragraph 1:
 > RDMA connections are managed within
 > these limits as described in section 3.3 ("Flow Control"[[Comment.2:
 > RFC Editor: please verify section and title of the RPCRDMA
 > document]])

   The RFC Editor isn't able to locate the correct reference; you need  
to
   do that.


Section 3.3.9., paragraph 0:
 > 3.3.9.  netaddr4

   Does anything in this section change now that we have
   draft-ietf-nfsv4-rpc-netid? Should this new document be cited
   somewhere?


Section 3.3.9., paragraph 2:
 > The netaddr4 data type is used to identify TCP/IP based endpoints.
 > The r_netid and r_addr fields are specified in RFC1833 [26], but they
 > are underspecified in RFC1833 [26] as far as what they should look
 > like for specific protocols.  The next section clarifies this.

   This sounds like this document should update RFC1833?


Section 3.3.9.1., paragraph 3:
 > For example, if a host, in big-endian order, has an address
 > of 0x0A010307 and there is a service listening on, in big endian
 > order, port 0x020F (decimal 527), then the complete universal address
 > is "10.1.3.7.2.15".

   You should use an IP address ouf of the RFC3330 "example" address
   range here. Also, please use a private port number (>49152) for this
   example.


Section 3.3.9.1., paragraph 4:
 > For TCP over IPv4 the value of r_netid is the string "tcp".  For UDP
 > over IPv4 the value of r_netid is the string "udp".

   What about SCTP?


Section 5.8.2.15., paragraph 1:
 > True, if the file is considered hidden with respect to the Windows
 > API.

   Is this really a Windows-only attribute? No other OS has this? If so,
   does it make sense to REQUIRE it? (Same comment aplies to "system".)


Section 8.7., paragraph 2:
 > To take propagation delay into account, the client should subtract it
 > from lease times (e.g. if the client estimates the one-way
 > propagation delay as 200 milliseconds, then it can assume that the
 > lease is already 200 milliseconds old when it gets it).  In addition,
 > it will take another 200 milliseconds to get a response back to the
 > server.  So the client must send a lease renewal or write data back
 > to the server at least 400 milliseconds before the lease would
 > expire.

   I'd encourage an even more conservative approach. For example, with
   mobile hosts, the network delay when the lease is granted may be
   significantly different (longer) when a lease is up for renewal.
   (Think mobile clients that acquired a lease while on the corporate
   network that then move to a WIFI or 3G VPN connection.)


Section 9., paragraph 1:
 > To support Win32 share reservations it is necessary to provide
 > operations which atomically open or create files.  Having a separate
 > share/unshare operation would not allow correct implementation of the
 > Win32 OpenFile API.  In order to correctly implement share semantics,
 > the previous NFS protocol mechanisms used when a file is opened or
 > created (LOOKUP, CREATE, ACCESS) need to be replaced.  The NFSv4.1
 > protocol defines an OPEN operation which is capable of atomically
 > looking up, creating, and locking a file on the server.

   Is this really only Windows? I thought Unix (at least BSD) had  
similar
   flags for open(), i.e., O_CREAT|O_EXCL|O_EXLOCK.


Section 12.9., paragraph 4:
 > Where communication with storage devices is subject to the same
 > threats as client to metadata server communication, the protocols
 > used for that communication need to provide security mechanisms as
 > strong as or no weaker than those available via RPSEC_GSS for
 > NFSv4.1.

   Where would these security mechanisms for the various storage
   protocols be specified? Is the intent here to require that there'd be
   a new RFC that describes how a new storage protocol integrates into
   pNFS? (Also, nit: s/RPSEC_GSS/RPCSEC_GSS/)


Section 22., paragraph 0:
 > 22.  IANA Considerations

   This section requires some work. Keep in mind that IANA will (mostly)
   only look at the subsections here to create new registries, i.e., all
   information needed by IANA to do so needs to be included here.
   Currently, that's not the case at least for some registries.


Section 22.1., paragraph 0:
 > 22.1.  Named Attribute Definitions

   I don't think this section has all the information IANA needs to
   create, populate and maintain this registry. Please see RFC5226, esp.
   Section 4. Specifically, it's not clear what fields this registry
   would have, exactly what the initial allocations are, and it would be
   good to use one of the well-known allocations policies from RFC5226
   (if suitable).


Section 22.2., paragraph 8:
 > Note: the '"' marks are used for delimiting the strings for this
 > document and are not part of the Network Identifier string.

   I think you can just drop the quotes and this note, or do you think
   there is a danger that folks will include the whitespace in the
   strings?


Section 22.2., paragraph 11:
 > As mentioned, the registration of new Network Identifiers will
 > require the publication of an RFC with similar detail as listed above
 > for the Network Identifier itself and corresponding Universal
 > Address.

   This subsection is more in line with what RFC5226 suggests for a
   registry definition. I'd still recommend to use one of the well-known
   allocation policies though.


Section 22.3., paragraph 0:
 > 22.3.  Defining New Notifications

   It's unclear what the IANA considerations are - is a new registry
   being defined here?


Section 22.4., paragraph 2:
 > The author of a new pNFS layout specification must follow these steps
 > to obtain acceptance of the layout type as a standard:

   Steps 3-5 are basically the "Standards Action" policy as defined by
   RFC5226.


Section 22.5.1., paragraph 2:
 > For the variable names ${ietf.org:CPU_ARCH} and ${ietf.org:OS_TYPE},
 > IANA will have to create a registry of values to be used for that
 > variable.  Applications for such values must contain the variable
 > name, the proposed value of that variable, and a brief (one or two
 > paragraphs) explanation of what is indicated by that specific value.
 > Such requests should be reviewed by [e-mail] and a Designated
 > Expert.

   What exactly are the fields of this registry and what is its name?
   What are the initial entries?


Section 22.5.2., paragraph 2:
 > Applications for the addition of variables to this registry should
 > contain the name of the variable and a brief (one or a few
 > paragraphs) explanation of the purpose of the variable.  No review of
 > these applications by IANA is necessary.

   i.e., RFC5226's "First Come First Served" policy


--------------------------------------------------------------------------
EDITORIAL NITS
--------------------------------------------------------------------------

   Note: Most comments marked as nits below have been automatically
   flagged by review scripts - there may be some false positives in
   there.


Section 1.5., paragraph 8:
 >    requests from connnections with the same client owner as coming

   Nit: s/connnections/connections/


Section 1.5., paragraph 15:
 >    each connection might be associatable with the same session.

   Nit: s/associatable/associable/


Section 2.3., paragraph 4:
 > which the server makes an RPC directed at the client.  Callback RPC's

   Nit: s/RPC's/RPCs/


Section 2.6.3.1.2., paragraph 3:
 > MUST NOT NFS4ERR_WRONGSEC in that situation.  Instead the server MUST

   Nit: s/MUST NOT NFS4ERR_WRONGSEC/MUST NOT return NFS4ERR_WRONGSEC/


Section 2.9.1., paragraph 1:
 > NFSv4.1 works over RDMA and non-RDMA_based transports with the

   Nit: s/non-RDMA_based/non-RDMA-based/


Section 2.10.2.1., paragraph 3:
 >     and the reply's structure is:

   Nit: s/reply's/reply/


Section 2.10.5.1., paragraph 7:
 >    retries of requests that are stll in progress.

   Nit: s/stll/still/


Section 2.10.5.1., paragraph 16:
 > and sequence ID.  Furhermore, since the XID is only 32 bits, it is

   Nit: s/Furhermore,/Furthermore,/


Section 4.2.3., paragraph 9:
 > require special care as regards handling of RENAMESs and REMOVEs.

   Nit: s/RENAMESs/RENAMEs/


Section 5.8.1.12., paragraph 1:
 > Error returned from getattr during readdir.

   Nit: s/getattr during readdir/GETATTR during READDIR/ (?)


Section 5.8.1.13., paragraph 1:
 > The filehandle of this object (primarily for readdir requests).

   Nit: s/readdir/READDIR/ (?)


Section 6.2.1.3.2., paragraph 5:
 > This allows servers to support something close to traditional unix-

   Nit: s/unix-/Unix-/


Section 6.4.1., paragraph 1:
 > error to be given is NFS4ERR_ATTRNOTSUP.

   Nit: s/NFS4ERR_ATTRNOTSUP./NFS4ERR_ATTRNOTSUPP./


Section 6.4.3.2., paragraph 7:
 > descendents.

   Nit: s/descendents./descendants./


Section 10.2., paragraph 17:
 > to conflicting requests or respond to them with NFSERR_DELAY.

   Nit: s/NFSERR_DELAY/NFS4ERR_DELAY/


Section 10.2.1., paragraph 16:
 > errors NFS4EER_EXPIRED, NFS4ERR_ADMIN_REVOKED, or

   Nit: s/NFS4EER_EXPIRED,/NFS4ERR_EXPIRED,/


Section 11.7.5.1., paragraph 2:
 > mapping all of the fsids for the descendent file systems to the

   Nit: s/descendent/descendant/


Section 12.9., paragraph 2:
 > device(s)) and/or a decision to forego use of pNFS (e.g., and fall

   Nit: s/forego/forgo/


Section 18.30.4., paragraph 2:
 > the end of the file.  Servers are free to implement this using
 > unallocate bytes (holes) or allocated data bytes set to zero.

   Nit: s/unallocate/unallocated/


Section 18.34.3., paragraph 2:
 > the connnection with the session specified in SEQUENCE.

   Nit: s/connnection/connection/


Section 18.43.4., paragraph 13:
 > the client gets results from LAYTOUTGET, then there is no longer a

   Nit: s/LAYTOUTGET,/LAYOUTGET,/


Section 18.46.3., paragraph 16:
 >    consequence of there being no working backchanel and the client

   Nit: s/backchanel/backchannel/


Section 18.49.3., paragraph 19:
 > that are not writeable.  This includes file objects of types: NF4BLK,

   Nit: s/writeable./writable./


Section 20.5.4., paragraph 2:
 > When a client returns a status other than NFS4_OK, NFSERR_DELAY, or

   Nit: s/NFSERR_DELAY/NFS4ERR_DELAY/



_______________________________________________
nfsv4 mailing list
[e-mail]
https://www.ietf.org/mailman/listinfo/nfsv4