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

From: David Noveck <[email-masked]>
Date: Wed, 15 Jun 2016 06:29:59 -0400

*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.

*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.

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]

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.

*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.

*9.  References*

The reference to rfc5666bis need to be made normative.

---

From: Chuck Lever <[email-masked]>
Date: Wed, 15 Jun 2016 13:48:49 -0400

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 <[email-masked]> 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.
> 
> 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.
> 
> 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 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.
> 3.  Transfers from NFS Server to NFS Client
> Has the same issue as 2.  Transfers from NFS Client to NFS Server

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
> 	• 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 opaque file data return by a READ request.
> 	• The pathname returned by a READLINK request.
> 	• 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
> 5.  NFS Version 4 Mapping
> 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 opaque file data returned by a READ operation.
> 	• The pathname returned by a READLINK operation.
> 	• The opaque file data provided in a WRITE operation.
> 	• The pathname provided when the NF4LNK case is used in a CRATE operation.
> 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
> 	• 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.  
> 	• 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

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

---

From: David Noveck <[email-masked]>
Date: Wed, 15 Jun 2016 14:37:51 -0400

> that's what you're referring to here?

Yes.

> careful about whether to state that explicitly.

I'm not proposing that the document states that explicitly but I want to
avoid gratuitously restricting it to a single version.  I'm sure that at
some point someone will propose a new Version that would
invalidate existing ULB's, and while that shouldn't be prohibited,
I'd argue that that would probably be a bad things to do.

> this document; in particular, for READ_PLUS. Is that also your

Yes.

> portions that do not contradict rfc5666bis.

Up to you, That would require careful work with a scalpel, while I
prefer the chainsaw approach.

> normative reference to an in-process I-D. It didn't just now

Maybe that's allowed in leap years, or maybe it is years divisible
by four?  I'm not sure :-)

On Wed, Jun 15, 2016 at 1:48 PM, Chuck Lever <[email-masked]> wrote:

> > In particular, since it only addresses changes in things known to be
> wrong in RFC5667, it does not take proper account of
> >       • 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
> > 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.
>
> > 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.
> >
> 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
> items are eligible for Direct Data Placement. This document details the
> DDP-eligible data items for each of:
> > 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.
> > 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]".
> > In the sixth bullet, suggest adding ", replacing the previous treatment
> of callback operations." at the end of the sentence.
> should be gone.  See 3.  Transfers from NFS Server to NFS Client
> section 5.1), the only DDP-eligible data items are contained within the
> request or the response for the COMPOUND procedure.
> 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.
> >
> >       • 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
> >       • 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.
> >       • 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
> >       • 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.