Re: [nfsv4] Review of draft-ietf-nfsv4-rfc5667bis-00

[Chuck Lever <[e-mail]>]

Thanks, Dave. I generally agree with your assessment. A few
scattered comments and responses are inline.


> On Jun 15, 2016, at 6:29 AM, David Noveck <[e-mail]> wrote:
> 
> General Comments
> 
> Overall Evaluation
> 
> This draft does not yet take sufficient account of the changes that were made in going from RFC5666 to rfc5666bis.
> 
> In particular, since it only addresses changes in things known to be wrong in RFC5667, it does not take proper account of
> 	• The definition of what is supposed to be in a ULB.
> 	• The fact that DDP-eligibility violations are supposed to be checked for.
> 	• The fact that DDP-eligibility is now a generic concept and is not, in general, tied to the use of explicit RDMA operations.
> 	• It repeats a  lot of material that is in rfc5666bis, which is in conflict with the fact that there will be multiple RPC-over-RDMA versions and this ULB is supposed to cover all of them.
> 	• A lot of stuff that was deleted in rfc5666bis, is still referred to in rfc5667bis.
> 
> Specific Classes of Issues
> 
> A very large part of this document seems to inappropriately duplicate what is in rfc5666bis.
> 
> There are many instances in which the responder is told to ignore chunk lists.  I believe that these statements basically contradict rfc5666bis in that the chunks in the lists to be ignored will inevitably involve DDP-eligiblility violations and the ULB has no business telling people to ignore these.
> 
> The rfc5666bis requirement to specify response size limits seems to be ignored.  In many cases (see below), there appears to be confusion in that there is inappropriate discussion of the maximum size of buffers (which will be removed), which might be taken into account as we formulate text regarding the maximum size of responses.  In any case, this gap needs to be filled.

The ULB is supposed to provide mechanisms that ensure Requesters
can properly estimate the maximum size of RPC replies. I think
that's what you're referring to here? Yes, I agree that should
be discussed for each NFS version.


> Comments by Section
> 
> Title
> 
> Suggest changing the title to be "Network File System (NFS) RDMA Upper Layer Binding"
> 
> Abstract
> 
> The fact that this assumes that all DDP will be done by explicit RDMA operations makes it Version-One-only.
> 
> In addition, given that rfc566bis defines the concept of a ULB, it is appropriate to reference that. in the abstract.
> 
> Suggest the following rewrite:
> This document provides the ULB needed for various version of the Network File System (NFS)  to use the RPC-over-RDMA transport.  It describes the DDP-eligibility of XDR items within NFS versions 2, 3, 4, and 4.1, as well as any other material defined as necessary for an upper Layer Bindiing in [rfc5666bis].  This binding may be extended to cover the additions of new DDP-eligible XDR items as defined in new versions of the NFSv4 protocol. This document obsoletes RFC5667.
> 1.  Introduction
> 
> I think this is far too specific, given that this document is supposed to be a ULB which applies to all RPC-over-RDMA versions.

I wonder about that scope. -All- RPC-over-RDMA Versions?
I think we should wander in that direction, but be very
careful about whether to state that explicitly.


> Also, the phrase "specific argument and results" is way out of date given that, in the context of NFSv4, every RPC has only a single argument and result, although each is likely to be a complicated one. 
> 
> Also, I think better advantage can be taken of the framework established by rfc5666bis.
> 
> Suggest the following rewrite:
> The Remote Direct Memory Access (RDMA) Transport for Remote Procedure Call (RPC) [RFC5666] allows an RPC client to provide for the direct placement of data to be sent to and received from the RPC server The RDMA transport header provides information about the desired placement of XDR items subject to direct data placement.  In order to assure interoperability, the client and the server must agree about which XDR items are eligible for Direct Data Placement. This document details the DDP-eligible data items for each of:
> 	• NFS version 2 [RFC1094]
> 	• NFS version 3 [RFC1813]
> 	• NFS version 4.0 [RFC7530]
> 	• NFS version 4.1 [RFC5661]

I have in my notes from the design phone call in March that
we are also interested in providing a binding for NFSv4.2 in
this document; in particular, for READ_PLUS. Is that also your
recollection?


> This listing of eligible items may be extended to cover the addition of new DDP-eligible XDR items as defined by new versions of the NFSv4 protocol.
> It also contains additional material required of Upper layer bindings, as specified in [rfc5666bis].
> 1.2.  Planned Changes To This Document
> In the second sentence of the first bullet, suggest replacing "relative to" by "made necessary by".
> Although this could arguably be within the first bullet, suggest adding a new bullet "Will add material required for ULBs by [rfc5666bis], that was not present in [RFC5667]". 
> Suggest adding a new bullet: "Material that duplicates what is in [rfc5666bis] will be deleted"
> In the fourth bullet, suggest replacing 'threshold by thresholds"
> In the sixth bullet, suggest adding ", replacing the previous treatment of callback operations." at the end of the sentence.
> In the seventh bullet, suggest replace "is required by IDNITS" by "will be added".
> Suggest deleting the eighth bullet since all of the code fragments should be gone.  See 3.  Transfers from NFS Server to NFS Client 
> 2.  Transfers from NFS Client to NFS Server
> I don't see anything in this section that is appropriate to a ULB.  It basically just repeats a lot of material that belongs in rfc5666bis.
> I suggest deleting the section.
> 3.  Transfers from NFS Server to NFS Client
> Has the same issue as 2.  Transfers from NFS Client to NFS Server
> Also suggest deleting this section.

My feeling about Sections 2 and 3 is that this is introductory
material and is pertinent to how implementers approach binding
NFS to RPC-over-RDMA.

I think it would be appropriate to make these two Sections into
subsections of the Introduction, carefully extracting the
portions that do not contradict rfc5666bis.

References to using a Read list for conveying RPC replies will
be removed, of course.

I notice that there is a paragraph in Section 3 that discusses
returning data content that is shorter than a Write chunk, or
even zero length. We have to be sure that the language here
is removed/shortened in favor of similar discussion in
rfc5666bis. However, I'm still not sure that rfc5666bis
aligns perfectly with current implementations.


> 4.  NFS Versions 2 and 3 Mapping
> Suggest changing the section title to "Binding for NFS Versions 2 and 3"
> A large part of what is in this section needs to be deleted because it either:
> 	• Duplicates what is in rfc5666bis.
> 	• Contradicts what is in rfc5666bis.
> Rather than identify each item, I'll just state what I think is in here that is relevant.  
> 
> Note that there is still things that the ULB says are supposed to do, that aren't done here, but I'll address those below.
> 
> The big issue in that respect is a means of bounding response length.
> 
> In any case, I think this section boils down to:
> The only DDP-eligible response data items are the following:
> 	• The opaque file data return by a READ request.
> 	• The pathname returned by a READLINK request.
> The only DDP-eligible request items data items are the following:
> 	• The opaque file data provided in a WRITE request.
> 	• The pathname provided in a SYMLINK request.
> With regard to response size limits, the material in the existing sixth paragraph, which
> needs to be deleted,  should be taken into account.  In any case we need some response
> size limits and the ones specified there are better than nothing.
> 5.  NFS Version 4 Mapping
> Suggest changing the section title to "Binding for NFSv4 Versions"
> Except for the first and the last three paragraphs, which I'll deal with later, I think this boils down to:
> Except in the case of backward direction operation (for which see section 5.1), the only DDP-eligible data items are contained within the request or the response for the COMPOUND procedure.
> The only DDP-eligible response data items are the following:
> 	• The opaque file data returned by a READ operation.
> 	• The pathname returned by a READLINK operation.
> The only DDP-eligible request items data items are the following:
> 	• The opaque file data provided in a WRITE operation.
> 	• The pathname provided when the NF4LNK case is used in a CRATE operation.
> With regard to the first paragraph, I suggest the following replacement:
> This specification specifies DDP-eligibility and request and response limits size to the applicable to the first minor version of NFS version 4 (NFSv4.0) and the second (NFSv4.1).  New versions of NFSv4 which extend the NFSv4 XDR may designate some of these new XDR items as  DDP-eligible.  Subsequent minor versions of NFSv4 may override this specification as it applies to those new minor versions.
> 
> The last three paragraphs meander for a while and never addresses the ULB requirements.  Not sure yet
> exactly how to fix this but note that:
> 	• It may be that. like the material  4.  NFS Versions 2 and 3 Mapping, this texr is inappropriately focused on determining the inline buffer size.  If so, this material would need to be deleted as both RFC5666 and rfc5666bis have moved past that idea that buffers have to be as long as messages.  However, the ULB still requires response size limits so we need to extract anything relevant from the existing discussion.
> 	• It is not clear what "mechanism" is being described as  "unworkable" in the antepenultimate paragraph.
> 	• This is particularly perplexing given that NFSv4.1, which presumably has the same layering as NFSv4.0, does have the limits needed, as noted in the lat paragraph.
> 	• The penultimate paragraph, while it discusses "theory" and "practice" never gets down to what is "required" to assure interoperability.  As such, it does not meet the requirements for a ULB, given that there is no way specific to bound the size of a response.  Thus, a requester has no ULB-specified way to determine how large the reply chunk should be.  
> Regarding response size limits:
> 	• For NFSv4.1, the limits must need to reflect, the session parameters referred to in the final paragraph.
> 	• For NFSv4.0, we need to focus on existing interoperable implementations and simply adopt those limits.  Although there might be, in theory or in practice, NFSv4.0 implementations that  on't respect those limits, those implementations need to be told that they are not compatible, as of now, with RPC-over-RDMA.
> 	• Even though the configuration protocol is gone, we need to somehow leave open the door for future versions of RPC-over-RDMA or optional extensions within them to provide a way to support higher response limits.
> 5.1.  NFS Version 4 Callbacks
> Needs to be replaced by something consistent with draft-ietf-nfsv4-rpcrdma-bidirection.

Agree on Section 5 and 5.1; that will be the bulk of the
work.


> 9.  References
> The reference to rfc5666bis need to be made normative.

Correct. I had thought IDNITS would complain about making a
normative reference to an in-process I-D. It didn't just now,
so I will move the reference.


--
Chuck Lever