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

[David Noveck <[e-mail]>]

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