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

[David Noveck <[e-mail]>]

*General Comments*

*Overall Evaluation*

This document is in pretty good shape.  Unless other people find additional
issues, I expect that -07 will be ready for WGLC.

*Comments by Section*

*Abstract*

In the second sentence, suggest deleting "Version One" at the end.  As
written, this suggests that ULBs are not or might not be required for
future transport versions.

*1.  Introduction*

The current first paragraph is misplaced.  I think it derives from rfc5667
where this was essentially the only function of ULBs and there were no
explicit ULB requirements.  I suggest replacing this paragraph by the
following two paragraphs but putting this material after the end of the
current material in this section:

The RPC-over-RDMA Version One transport may employ direct data
placement to convey data payloads associated with RPC transactions. To
enable successful interoperation, RPC client and server
implementations must agree as to which XDR data items in what
particular RPC procedures are eligible for direct data placement
(DDP).  Specifying those data items is a major component of a
protocol's Upper Layer Binding.

In addition, Upper Layer Bindings are required to include additional
information to assure that adequate resources are allocated to receive
RPC replies, and for a number of other reasons.

Suggest replacing the first paragraph (currently the second) by the
following to make sure all ULBs in this document are mentioned:

This document contains material required of Upper Layer Bindings, as
specified in [I-D.ietf-nfsv4-rfc5666bis], for the NFS protocol
versions listed below.  In addition, bindings are provided, when
necessary, for auxiliary protocols used together with NFS versions 2
and 3.

*2.  Reply Size Estimation*

The first paragraph as written is clumsy.  Suggest rewriting it as follows:

When using an  RPC-over-RDMA Version One transport, a requester,
during the construction of each RPC Call message, is responsible for
allocating appropriate resources for receiving the corresponding Reply
message.

There are a number of problems with the second paragraph;

   - Other than because of an astonishingly poor implementation, I don't
   see how you can have "An overrun of these resources"  resulting in
   "corruption of the Reply message".
   - The second sentence as written, implies that these horrible
   consequences (corruption or disconnection) are the reason for reply size
   estimation.  In fact, the reason you need a big enough buffer for the reply
   is that if you didn't you couldn't get the generated reply back and that is
   sufficient reason to require reply size estimation.

Suggest rewriting the second paragraph as follows:

When insufficient resources are available, it may be impossible to
provide the reply corresponding to the call. Therefore. reliable reply
size estimation is necessary to ensure successful interoperation.
This is of particular concern, for example, when deciding whether to
allocate a reply Chunk and when deciding on the proper size for a
Reply chunk.

Suggest adding a new bullet, as follows, following the current second:

The client specifies a reply size limit for the particular reply or
operation, as it does by setting the count field of READDIR request.

*2.1. Short Reply Chunk Retry*

A problem with this section as written is that this problem can arise even
if no reply chunk has been allocated. For example if the estimate is 900
bytes, the inline threshold is 1K, and the reply is 1028 bytes.

I don't see the point of the fourth (unbulleted) paragraph. The previous
paragraph and the associated bullets have already given the client two
choices of which he MAY choose one. Given that this is a standards track
document, that is all that is required. The last paragraph in this section
specifies that this technique is limited in scope. I don't see the need for
the editorializing here. Suggest deleting this paragraph.

*3.  Upper Layer Binding for NFS Versions 2 and 3*

The penultimate paragraph in this section is new in this version and I
don't understand the need for a new version 2/3 restriction so late in
the game.  If this is not a new restriction, the intent would be
clearer if it was rewritten as follows:

The structure of the NFS version 2 and  3 protocols and limitations on
DDP-eligible data items given above are such that:


   - It is impossible for a Legacy  NFS client to validly send a
reduced Payload stream in a Long Call.
   - It is impossible for a Legacy  NFS client to validly enable a
Legacy NFS server to send a reduced Payload stream in a Long Reply.

*4.1.  MOUNT, NLM, and NSM Protocols*

The second sentence of the last paragraph reads as follows: "In most
implementations this data item is never larger than 1024 bytes, making
reliable reply size estimation straightforward using the criteria
outlined in Section 2".  It appears that  the qualifying phrase "In
most implementations" has been ignored., leading to an incorrect
conclusion. This section needs to be revised to reflect the fact that
the client has no way to know whether the server is one within the set
of "most implementations" or one outside it.  Even if all existing
implementations acted within this limit, there is no way to prevent
new ones from being generated

*5.2.1.  Reply Size Estimation for Minor Version 0*

With regard to the second sentence of the second paragraph the
introductory phrase  "As discussed in Section 2," is incorrect since
the referenced material does not appear in section 2.

With regard to the first sentence of the last paragraph, WRITE is not
actually non-idempotent.  Better examples would be RENAME or REMOVE.

*5.4.2.  Multiple DDP-eligible Data Items*

With regard to the last sentence of the second paragraph, it is not
clear what within the following material are "additional
restrictions".  I can't see how all of it can so characterized.  This
needs to be clarified.

*5.5.2.  NFS Version 4.1 Callback*

With regard to the second sentence of the first paragraph, it is not
clear which particular "mechanism" within the bidirection document is
being referred to.  Suggest replacing the word by  "mechanisms" or
"approach".

*5.7.1.  Congestion Avoidance*

Given that the section about RFC56661 is referencing a different
document and is about a different protocol, suggest replacing
"further" by "also".