Subject: [nfsv4] Review of draft-ietf-nfsv4-rfc5666bis-04

From: David Noveck <[email-masked]>
Date: Sun, 6 Mar 2016 08:04:01 -0500

*General Comments*

*Overall Evaluation*

I think this document is in pretty good shape.  I don't see anything that
would keep it from being ready for WGLC quite soon, although a decision on
that point should probably be made during/after the 3/15 conference call.

*Nature of this review*

Given the state of this document I haven't gone through the entire document
as part of the review.  This time,  I basically did the review based on the
diff from the -03.  I'll do a complete review based on reading the document
all the way through once we start WGLC.  I'm not sure whether it will be
necessary to publish a -05 before that starts.

If anyone has a problem with my use of gmail formatting in this review, let
me know and I'll send you a pdf.

*Terminology Issues:*

There are a small set of terminology issues that affect multiple sections,
although the issues are each discussed in the review comments for one
particular section:

   - Description of data items as beIng "reduced", discussed in *4.4.2.
   DDP-eligibility.*
   - The use of the term "procedure" to denote an RPC/RDMA message header
   type, Discussed in *9.1. Changes To RPC-Over-RDMA Header XDR*

*Larger issues to Address/Discuss*

I think there needs to be discussion leading to agreement on two issues.
This does not imply that there will be any controversy about these:

   - Issues regarding connection sharing and  transport parameter
   recommendations.  I've tried to capture what i believe the original intent
   was in* 8.3. Additional Considerations*
   - In the document as written the extension model is not clear. In some
   places it seems to say that XDR extension in general is OK but that only
   addition of new header types is allowed in Version One,.implying that a
   Version Two could change that without updating the Version One RFC. In
   others, it is implied that the restriction of extensibility to new header
   types is in effect until changed by a document updating the Version One
   RFC.

I don't have a problem with either approach, but the document needs to be
clarified to pick one. I mention these possibilities in review comments for
subsections of Section 9, where the two choices are referred to as *MTO-V1*
(message-types-only-for-version-one) and *MTO-UMR *
(message-types-only-until-major-revision).

*Comments by Section*

*1. Introduction*

With regard to the last paragraph, I don't believe that the phrase
"introduces conventions" is an accurate description of what the document
does in regard to the items cited.

Specifically, with regard to bi-directional operation:

   - This standards-track document provides support for bi-directional
   operation.
   - That support is optional, so that if the implementation has no need
   for bidirectional operation, it does not need to be supported.
   - However, there is no other way to provide bidirectional operation and,
   if there were, there would be no need to provide it here.
   - Any new, additional way to support bidirectional operation would have
   to be specified in a standards-track document,

As a result, the provisions made for bi-directional operation are not one
among a set of possible "conventions".  They are an addition to the
protocol, necessary to support NFSv4.1 over RPC/RDMA which I think is a
good thing to provide.

With regard to RPCSEC_GSS, the text in question (appropriately) contains
RFC2119 terms.

I would rewrite the last paragraph as follows:

In addition, this document provides support for bi-directional
operation of RPC-over-RDMA, enabling operation of NFSv4.1 [RFC5661] on
RDMA transports, and describes the use of RPCSEC_GSS [RFC5403] on RDMA
transports.

*2.1. Changes to the Specification*

As explained elsewhere, I don't think the seventh bullet is appropriate and
I feel it should be deleted.

In the second sentence of the eighth bullet suggest replacing "specifying
conventions for using" by "describing the use of".

*2.3. Changes to the Protocol*

I don't think it was correct to delete the seventh bullet that was in the
corresponding section on -03. I believe it should be reinstated.

*4.1. Transfer Models*

I have a couple of wording suggestions regarding the last paragraph:

   - In the third sentence suggest replacing "by" by "within".
   - In the last sentence suggest replacing "a future version" by "future
   versions".

*4.2. Message Framing*

Regarding "transport headers stream", the term actually defined and used in
the document is "transport stream"

*4.4. XDR Encoding with Chunks*

Although I agree that it is appropriate to make this more generic by
shifting from "RDMA" to "a direct data placement facility", the resulting
paragraph is unclear since it is written with the assumption that the only
direct data placement facility is RDMA. Need to either shift back or
complete the change to a more generic statement.

The following is a possible replacement

When a direct data placement capability is available, it can be
determined whether an XDR data item is large enough that it might be
more efficient for the transport to provide direct placement of  the
data item contents, rather than having the data contents as a part of
the message XDR stream.

*4.4.2. DDP-eligibility*

In previous sections the document refers to the XDR stream as being
"reduced" by the removal of one or more XDR items, which is correct. Here,
it starts referring to the data items as having been "reduced", which is
not correct. It would be correct to say they are "removed", "subtracted",
"excised" or "eliminated" but the only thing that is "reduced" is the XDR
stream. My preference is "excised" since the data is still present
somewhere and transferred somehow. There's a bunch of such issues in
following sections. I haven't noted them all.

*4.5.2. Chunked Messages*

In the first paragraph, the phrase "to enable use of" is less correct than
the "and' that used to be there before.

I suggest the following as a possible replacement:

If DDP-eligible data items are present in a Payload stream, a sender MAY
reduce the Payload stream by removing the DDP-eligible items. The sender is
then able to use RDMA Read or Write operations to transfer the removed data
items. The Transport stream with the reduced Payload stream immediately
following is transferred using a single RDMA Send operation

In the second paragraph there are two instances of the phrase "reduced data
items".

*5. RPC-Over-RDMA In Operation*

The second sentence of the last paragraph may run into issues with
RDMA_ERROR and possible extensions. I propose the following replacement.

Each endpoint of a connection MAY send any RPC-over-RDMA message
header type used to transfer RPC messages (e.g., RDMA_MSG, RDMA_NOMSG)
at any time (subject to credit limits).

*5.5. Error handling*

In the last sentence, suggest replacing "cannot be decoded" by "it cannot
decode"

*8.3. Additional **Considerations*

The use of "Upper Layer Protocols" in the last paragraph is incorrect since
it is inconsistent with the idea that RPC/RDMA requires no change in upper
layer protocols.

Also it isn't clear what "the above" refers to. The normal interpretation
would be all the bullets, but in that case the paragraph basically makes
any recommendations in the first bullet useless.

I suggest the following replacement text as clearer. I believe it is in
line with the intent of the statement in -04, but I'm not clear how we will
win up eling with this issue. This text may be too restrictive of the
ability of Upper Layer Bindings to impose restrictions on connection
parameters.

Given the possibility of sharing connections between multiple Upper
Layer Protocols as described above, an Upper Layer Binding cannot be
written assuming that its recommendations regarding transport
parameters will necessarily be followed.  The upper layer Binding
needs to be designed and to allow correct interoperation without
regard to the transport parameters actually used so long as they obey
the restrictions specified in this document.  Furthermore,
implementations of Upper Layer Protocols must be designed to
interoperate correctly regardless of the connection parameters in
effect on a connection.

*9. Protocol Extensibility*

In the second sentence of the second paragraph, suggest replacing "with" by
"after".

In the second sentence of the third paragraph, suggest replacing "using
this approach" by "making such XDR changes".

*9.1. Changes To RPC-Over-RDMA Header XDR*

Because of the the XDR change made in -04, the second paragraph needs
to be rewritten.  In addition "arguments and results' is incorrect
since the RPC/RDMA transport, unlike RPC itself, is not a
request-response protocol.

In addition, the word "procedure" to denote a header message type is
inappropriate, and this issue needs to be looked at globally.

Here's some suggested replacement text:

Within this framework, RPC-over-RDMA may be extended by defining a new
constant value to denote a new message header type and defining an XDR
structure to represent the type-specific contents of the new header
type.  Such extensions would be specified by publishing a new
Standards track document after appropriate Working Group and IESG
review.  One important goal of such review is to ensure continued
interoperation with existing implementations.

The existing fifth paragraph is not vey clear. I suggest deleting it and
adding the following text to the fourth paragraph.

For a discussion of how the sender can deal with such issues see Section 9.x

I note that most of this section is written assuming *MTO-V1*. However, in
*9**.4. RPC-Over-RDMA Version One Extension Practices*, there is no
explicit statement regarding the restriction of extensions in Version One
to new message types only. Given that, it seems that the more expansive
definition of XDR extensibility is moot in this document and a change to
move away from that woul be a change in the extensibility model requiring a
document updating this RFC, as is the case in *MTO-UMR.*

*9.3. RPC-Over-RDMA Version Numbering*

The first paragraph as written is not correct.  A revision would start
as follows:

The use of RPC-over-RDMA version numbers enables both endpoints to
agree on a set of interoperable behaviors and REQUIRED features.
Senders are then able to determine which OPTIONAL features the
receiver supports.

The following continuation is appropriate if *MTO-V1 *is in effect:

Senders may do this for each OPTIONAL feature using the techniques defined
by version number being used. In the case of Version One, that technique,
only valid for features that consist only of new messages types, is
described n Section 9.4.x (*new section for which see below)*

An alternate continuation for the case in which *MTO-UMR *is in effect is
as follows:

Sender may do this for each OPTIONAL message type using the technique
described in Section 9.x (*to be derived from the existing section 9.4.2)*

In the first sentence of the third paragraph, suggest replacing "prevents"
by "might interfere with".

*9.4.  RPC-Over-RDMA Version One Extension Practices*

The problem here is the broad definition of extension in *9.1. Changes
To RPC-Over-RDMA Header XDR.*  That would call for some explicit
restriction on extension pactices in Version one to continue
consistently with the *MTO-V1 *approach.

*9.4.x Version One Extension Restrictions*

This new section will be needed to adopt  *MTO-V1 *approach.

The following section text is suggested.

Within Version One, and until modified in subsequent versions unless
explicitly modified in RFCs defining new versions, extensions may only
consists of features that create new message header types.  No other
form of XDR extension can be done.

Any other sort of XDR change, even if it is a compatible extension as
described in Section 9.1, requires a change in the version number.

*9.4.1.  Documentation Requirements*

if *MTO-UMR* is in effect, the first paragraph needs to be deleted as
this material would be in Section 9.4.x.

*9.4.2.  Detecting Support For Message Header Types*

This needs to be moved out of Section 9.4 and elsewhere I refer to it
as Section 9.x.

I believe this needs to be done independent of the choice between
*MTO-V1* and *MTO-UMR.*

I think the title should be *Determining Support for OPTIONAL Message
Header Type*

I propose the following minor textual changes to the section:

   - In the first sentence of the first paragraph, suggest replacing
"determines" by "can determine"
   - In the second sentence of the first paragraph, suggest replacing
"The receiver sends" by "The extension should defined such that the
receiver sends"
   - In the last sentence of the last paragraph, suggest replacing
"indication" by "an indication"

*10.2.1.  RPC-Over-RDMA Link-Level Protection*

Suggest replacing the last sentence of the last paragraph by the following:

There is no available alternative to the use of such mechanisms to
deal with cases in which integrity and/or privacy is needed and where
efficient operation is important.

*10.2.2.  RPCSEC_GSS On RPC-Over-RDMA Transports*

In the second sentence of the first paragraph, suggest replacing
"observe the conventions" by "obey the restrictions".

The last word of the section ("reduced") needs to be changed.