Subject: [nfsv4] Initial review of draft-ietf-nfsv4-rfc5666bis-00

From: David Noveck <[email-masked]>
Date: Sat, 5 Dec 2015 10:34:36 -0500

*General Comments*

*General Evaluation*
This document is in very good shape for a -00.  Most of what I've found so
far is basically editorial.   I'll try to complete my initial review within
the next few weeks.

*The Meaning of "deprecated"*
This word appears in eight places in the draft and I propose a replacement
in each specific instance.  In this section I discuss the meaning of the
word in general and explain why I don't think it is appropriate for this
document

In the active voice "deprecate" generally means to say something negative
about someone or something.

The passive "deprecated" has a specialized meaning to indicate that a
feature, API, command,etc., while usable and implemented has problems that
are worthy of note and that makes use of that entity, in some way,
problematic.   For example, it may be inefficient, confusing,
soon-to-be-deleted, not thread-safe, etc.

Generally, deprecation happens before something is removed and is necessary
because immediate removal is too disruptive/difficult.  It is not needed in
this document because the items "deprecated" have never been implemented or
used, or, in some cases, never really specified.

*Comments by Section (Through Section 4.6.3)*

*Abstract*

In the first sentence, suggest replacing "providing" by "supporting the use
of"

In the second sentence, suggest replacing "conveys' by "provides".

As the second sentence is now written, it seems to incorrectly suggest you
can have both (at the same time) efficient bulk data transport and minimal
change to rpc applications, suggest replacing the portion of the second
sentence following the comma by the following:

while also allowing use with minimal changes to RPC applications.  In
either case, there is no required revision of the application RPC protocol,
or the RPC protocol itself.

*1. Introduction*

Suggest replacing the first sentence by:

This document obsoletes RFC 5666 and is based on the behavior of existing
interoperable implementations of the RPC-over-RDMA Version One protocol.

Suggest replacing the second sentence by:

It is published to clarify ambiguous text that is subject to multiple
interpretations and to eliminate support for unimplemented RPC-over-RDMA
Version One protocol elements.  It also introduces conventions to allow
bi-directional RPC-over-RDMA support to be provided as an optional feature.

*1.2. RPC Over RDMA Transports*

In the first sentence, suggest replacing "over high-speed transports by "as
transport speeds increase"

In the second sentence.suggest replacing the material after the comma by
the following:

the benefits of faster transfers and reduced host overhead are obtained

In the last sentence of the second paragraph, suggest replacing "unique" by
"specific".

In the first sentence of the third paragraph, suggest replacing "unlike" by
"different from".

In the last sentence of the third paragraph, suggest replacing "naturally"
by "appropriately".

Suggest replacing the second and third sentences of the fourth paragraph by
the following:

In this context, the Network File System (NFS) protocols as described in
[RFC1094], [RFC1813]. [RFC7530], [RFC5661], and future NFSv4 minor versions
are obvious beneficiaries of RDMA. A complete problem statement is
discussed in [RFC5532], and NFSv4-related issues are discussed in [RFC5661].

Suggest replacing the last paragraph by the following:

Although the RDMA-based transport described here can be used to provide
relatively transparent support for any RPC application, this document also
describes mechanisms that can be used to optimize the use of RDMA given
more active participation by the RPC application.

BTW, if "transparent" support is to be provided, you need some sort of
"default ULB" to accommodate such use.  I'll deal with this issue in more
detail later when I review Section 6

*2. Changes Since RFC 5666*Suggest replacing the first bullet by the
following:

   -  Section 2 has been expanded to introduce and explain key RPC, XDR,
   and RDMA terminology. These terms are now used consistently throughout the
   specification.

This change was necessary because implementers familiar with RDMA are often
not familiar with the mechanics of RPC, and vice versa.

*2.2. Changes To The Protocol*
Suggest replacing the introductory material by the following:

While the protocol described herein interoperates with existing
implementations of [RFC5666], the following changes have been made relative
to the protocol described in [RFC5666].

In the first bullet, suggest replacing "deprecated" by "eliminated".

Suggest replacing the second bullet by the following:

   - Support for the RDMA_MSGP message type has been severely restricted
   because of the lack of any clear definition of how it would be used in
   [RFC5666] and because the few existing implementations which use it do not
   actually depend on its use,

As originally conceived, it has no benefit for RPC programs that place bulk
payload items at positions other than at the end of their argument or
result lists, as is commonly done with NFSv4 COMPOUND RPCs [RFC7530].
Similarly, it is not beneficial when the inline threshold is significantly
smaller than the system page size.

I think that the third bullet belongs in section 2.1 since, there is no
change to the protocol.  Some XDR zealots might not agree but these are
precisely the people who would object to the change in any case.  Telling
them you are changing the protocol makes it harder to make this desirable
editorial change.

In the sixth bullet, suggest adding the following at the end of the second
sentence:

when both parties support the new functionality.

In the last paragraph, suggest replacing "is not" by "has not been".

*3.1.2. Requesters And Responders*
In the first sentence of the third paragraph*, *suggest replacing "work" by
"that work be done".

*3.1.3.1. XDR Opaque Data*
Suggest replacing the first sentence of the second paragraph by the
following:

Upper Layer Protocols or applications provide any needed data translation
in this case.

*3.2. Remote Direct Memory Access*
In the first sentence, suggest replacing "An RPC requester" by "RPC
requesters and responders"

*3.2.1. Direct Data Placement*

In the second sentence of  the first paragraph, suggest replacing "can" by
"will".

Suggest replacing the second sentence of  the third paragraph by the
following:

Although it is not efficient,  the sender may still copy data into an
intermediate buffer before an RDMA transfer.

In the first sentence of the last paragraph, suggest replacing "again" by
"to another location".

*3.2.2. RDMA Transport Requirements*
First some general comments about this section:

   - The term "tagged RDMA operations" is first used in this section but
   not defined previously.
   - The term "buffer" is often used in descriptions of RDMA targets in
   ways that are inconsistent with the idea of DDP, which is that data should
   go from its normal home location to its destination location, without a
   buffer being needed.
   - There is a mix of participant designations including sender/receiver
   and requester/responder

It seems to me that the last sentence in the paragraph headed "RDMA
Receive" is incorrect or a least incomplete.  Note that:

   - Section 4.3 describes a flow control mechanism with RPC/RDMA.
   - I can't see that NFSv3 has such a mechanism.
   - This is inconsistent with the idea previously stated that other RPC
   protocols can use RPC/RDMA without change to the protocol.

The first sentence in the paragraph headed "Registered Memory"" is
incorrect in that it does not apply to RDMA Read.  Suggest the following
revision:

All data moved via tagged RDMA operations is resident in registered memory
on the target node (i.e, source  for RDMA Reads and destination for RDMA
Writes).

With regard to the last sentence of the last paragraph, I wonder if we need
a reference to ROCE.

*4.1. Transfer Models*

Suggest replacing the second and third sentence of the last paragraph by
the following:

All current RPC-over-RDMA Version One implementations use only the
Read-Write Transfer Model. Use of the Read-
Read Transfer Model by RPC-over-RDMA Version One implementations is no
longer supported.

*4.2. RPC-over-RDMA Framing*
In the last sentence of the last paragraph, suggest adding "uderlying"
before "transport".

*4.3. Flow Control*
While flow control for the requester is simpler/trivial, it should at least
be mentioned.

I suggest deleting the strawman (i.e. second) paragraph and replacing the
first sentence of the third paragraph by the following:

Flow control for RDMA Send operations directed to the responder is
implemented as a simple request/grant protocol in the RPC-over-RDMA header
associated with each RPC message.

While the restriction regarding zero in the fourth paragraph is valid,
there needs to be clarification so that credit changes are not effected in
a way that can cause problems.  Suggest the follow new material after the
existing fourth paragraph:

When the credit value sent is different from what would result from using
the previous credit value minus the number of requests received, the
responder needs to ensure:

   - That a credit increase is effected (i.e. receives are posted) before
   the reply is sent.
   - That a credit decrease is not given effect until the responder
   receivers a request sent after the response indicating the new credit limit
   was receive by the requester.

Suggest revising the last two sentences of the current fifth paragraph as
follows:

When an error does not occur, the responder MAY handle the excess
request(s) normally or return an RPC error to the requester.

In the first sentence of the current sixth paragraph replace 'an" by 'a".

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

Because such restrictions concern the implementation off required RDMA
transport primitives, they are not dealt with by this protocol.  Instead,
from the viewpoint of this protocol, the request to provide the RDMA
transfer is considered made when required by this protocol and any deferral
to accommodate such restrictions is considered the responsibility of the
implementation.

*4.3.1. Initial Connection State*
The last paragraph isn't clear in that it says implementations "MAY" use
values above 1K but leaves it unclear how.   My expectation is that it is
not prepared to make concrete proposals but if that is the case, it should
say so more clearly.  Suggest the following replacement text:

Implementations MUST support an inline threshold of 1024 bytes,  They MAY
support larger inline thresholds but MUST assume the receiver’s inline
threshold is 1024 bytes unless there is an appropriate exchange of buffer
size information.  Since Version One of  Connection Configuration Protocol
described in [RFC5666] has been removed from this specification and
[RFC5666] will be obsoleted by this specification,  Additional means of
exchanging such information need to be developed.

Some possibilities that should be considered:

   - Development of a Version Two of the Connection Configuration Protocol.
   - Development of an in-line log-in sequence to be used as part of
   configuration setup, as an optional feature within RPC/RDMA Version One.
   - With suitable restrictions using feature of the ULP designed to assist
   in conveying this information.   An extension to NFSv4.2 or a new minor
   version of NFSv4 are possibilities.

*4.4.1. DDP-Eligibility*

I have problems with "Upper Layer Protocol MUST determine".  Normally
"MUST"s are directed to implementations.  In this case the MUST is directed
at previously published Standards-track RFCs making my head hurt.   I
propose replacing third and fourth sentence of the first paragraph by the
following:

For each Upper Layer Protocol, a  determination needs to be made as to
which items in its XDR definition are allowed to use
Direct Data Placement. Information about such determinations needs to be
included in an additional specification is
needed that describes how an Upper Layer Protocol enables Direct Data
Placement.

*4.6.3. Long Messages*
The definition in the first paragraph does not cover the case in which
DDP-eligible data is excised and the message is still above the inline
threshhold.  I think this case should be covered.

---

From: Chuck Lever <[email-masked]>
Date: Sat, 5 Dec 2015 15:57:59 -0500

[ Replaced Tom’s address with the address listed in rfc5666bis ]

Thanks as always for the careful review! I’ll have a close look
at the individual items this coming week.

One area for immediate discussion below.

> On Dec 5, 2015, at 10:34 AM, David Noveck <[email-masked]> wrote:
> 
> General Comments
> 
> General Evaluation
> 
> This document is in very good shape for a -00.  Most of what I've found so far is basically editorial.   I'll try to complete my initial review within the next few weeks.
> 
> The Meaning of "deprecated"
> 
> This word appears in eight places in the draft and I propose a replacement in each specific instance.  In this section I discuss the meaning of the word in general and explain why I don't think it is appropriate for this document
> 
> In the active voice "deprecate" generally means to say something negative about someone or something.
> 
> The passive "deprecated" has a specialized meaning to indicate that a feature, API, command,etc., while usable and implemented has problems that are worthy of note and that makes use of that entity, in some way, problematic.   For example, it may be inefficient, confusing, soon-to-be-deleted, not thread-safe, etc. 
> 
> Generally, deprecation happens before something is removed and is necessary because immediate removal is too disruptive/difficult.  It is not needed in this document because the items "deprecated" have never been implemented or used, or, in some cases, never really specified.
> 
> Comments by Section (Through Section 4.6.3)
> 
> Abstract
> 
> In the first sentence, suggest replacing "providing" by "supporting the use of"
> 
> As the second sentence is now written, it seems to incorrectly suggest you can have both (at the same time) efficient bulk data transport and minimal change to rpc applications, suggest replacing the portion of the second sentence following the comma by the following:
> 
> while also allowing use with minimal changes to RPC applications.  In either case, there is no required revision of the application RPC protocol, or the RPC protocol itself.
> 
> 1. Introduction
> 
> This document obsoletes RFC 5666 and is based on the behavior of existing interoperable implementations of the RPC-over-RDMA Version One protocol.
> 
> It is published to clarify ambiguous text that is subject to multiple interpretations and to eliminate support for unimplemented RPC-over-RDMA Version One protocol elements.  It also introduces conventions to allow bi-directional RPC-over-RDMA support to be provided as an optional feature.
> 
> 1.2. RPC Over RDMA Transports
> 
> In the first sentence, suggest replacing "over high-speed transports by "as transport speeds increase"
> 
> In the second sentence.suggest replacing the material after the comma by the following:
> 
> In the last sentence of the second paragraph, suggest replacing "unique" by "specific". 
> 
> In the first sentence of the third paragraph, suggest replacing "unlike" by "different from". 
> 
> In the last sentence of the third paragraph, suggest replacing "naturally" by "appropriately". 
> 
> Suggest replacing the second and third sentences of the fourth paragraph by the following:
> 
> In this context, the Network File System (NFS) protocols as described in [RFC1094], [RFC1813]. [RFC7530], [RFC5661], and future NFSv4 minor versions are obvious beneficiaries of RDMA. A complete problem statement is discussed in [RFC5532], and NFSv4-related issues are discussed in [RFC5661].
> 
> Although the RDMA-based transport described here can be used to provide relatively transparent support for any RPC application, this document also describes mechanisms that can be used to optimize the use of RDMA given more active participation by the RPC application.
> 
> BTW, if "transparent" support is to be provided, you need some sort of "default ULB" to accommodate such use.  I'll deal with this issue in more detail later when I review Section 6
> 
> 2. Changes Since RFC 5666
> 
> Suggest replacing the first bullet by the following:
> 	•  Section 2 has been expanded to introduce and explain key RPC, XDR, and RDMA terminology. These terms are now used consistently throughout the specification.
> This change was necessary because implementers familiar with RDMA are often not familiar with the mechanics of RPC, and vice versa.
> 
> 2.2. Changes To The Protocol
> 
> While the protocol described herein interoperates with existing implementations of [RFC5666], the following changes have been made relative to the protocol described in [RFC5666].
> 
> 	• Support for the RDMA_MSGP message type has been severely restricted because of the lack of any clear definition of how it would be used in [RFC5666] and because the few existing implementations which use it do not actually depend on its use,
> As originally conceived, it has no benefit for RPC programs that place bulk payload items at positions other than at the end of their argument or result lists, as is commonly done with NFSv4 COMPOUND RPCs [RFC7530]. Similarly, it is not beneficial when the inline threshold is significantly smaller than the system page size.
> 
> I think that the third bullet belongs in section 2.1 since, there is no change to the protocol.  Some XDR zealots might not agree but these are precisely the people who would object to the change in any case.  Telling them you are changing the protocol makes it harder to make this desirable editorial change.
> 
> In the sixth bullet, suggest adding the following at the end of the second sentence:
> 
> 3.1.2. Requesters And Responders
> 
> In the first sentence of the third paragraph, suggest replacing "work" by "that work be done".
> 
> 3.1.3.1. XDR Opaque Data
> 
> Suggest replacing the first sentence of the second paragraph by the following:
> 
> Upper Layer Protocols or applications provide any needed data translation in this case.
> 
> 3.2. Remote Direct Memory Access
> 
> In the first sentence, suggest replacing "An RPC requester" by "RPC requesters and responders"
> 
> 3.2.1. Direct Data Placement
> 
> In the second sentence of  the first paragraph, suggest replacing "can" by "will".
> 
> Suggest replacing the second sentence of  the third paragraph by the following:
> 
> Although it is not efficient,  the sender may still copy data into an intermediate buffer before an RDMA transfer.
> 
> In the first sentence of the last paragraph, suggest replacing "again" by "to another location".
> 
> 3.2.2. RDMA Transport Requirements
> 
> 	• The term "tagged RDMA operations" is first used in this section but not defined previously.
> 	• The term "buffer" is often used in descriptions of RDMA targets in ways that are inconsistent with the idea of DDP, which is that data should go from its normal home location to its destination location, without a buffer being needed.
> 	• There is a mix of participant designations including sender/receiver and requester/responder
> It seems to me that the last sentence in the paragraph headed "RDMA Receive" is incorrect or a least incomplete.  Note that:
> 
> 	• Section 4.3 describes a flow control mechanism with RPC/RDMA.
> 	• I can't see that NFSv3 has such a mechanism.
> 	• This is inconsistent with the idea previously stated that other RPC protocols can use RPC/RDMA without change to the protocol.
> The first sentence in the paragraph headed "Registered Memory"" is incorrect in that it does not apply to RDMA Read.  Suggest the following revision:
> 
> All data moved via tagged RDMA operations is resident in registered memory on the target node (i.e, source  for RDMA Reads and destination for RDMA Writes). 
> 
> With regard to the last sentence of the last paragraph, I wonder if we need a reference to ROCE.
> 
> 4.1. Transfer Models
> 
> Suggest replacing the second and third sentence of the last paragraph by the following:
> 
> All current RPC-over-RDMA Version One implementations use only the Read-Write Transfer Model. Use of the Read-
> Read Transfer Model by RPC-over-RDMA Version One implementations is no longer supported.
> 
> 4.2. RPC-over-RDMA Framing
> 
> In the last sentence of the last paragraph, suggest adding "uderlying" before "transport".
> 
> 4.3. Flow Control
> 
> While flow control for the requester is simpler/trivial, it should at least be mentioned.
> 
> I suggest deleting the strawman (i.e. second) paragraph and replacing the first sentence of the third paragraph by the following:
> 
> Flow control for RDMA Send operations directed to the responder is implemented as a simple request/grant protocol in the RPC-over-RDMA header associated with each RPC message.
> 
> While the restriction regarding zero in the fourth paragraph is valid,  there needs to be clarification so that credit changes are not effected in a way that can cause problems.  Suggest the follow new material after the existing fourth paragraph:
> 
> When the credit value sent is different from what would result from using the previous credit value minus the number of requests received, the responder needs to ensure:
> 	• That a credit increase is effected (i.e. receives are posted) before the reply is sent.
> 	• That a credit decrease is not given effect until the responder receivers a request sent after the response indicating the new credit limit was receive by the requester.
> Suggest revising the last two sentences of the current fifth paragraph as follows:
> 
> When an error does not occur, the responder MAY handle the excess request(s) normally or return an RPC error to the requester.
> 
> Because such restrictions concern the implementation off required RDMA transport primitives, they are not dealt with by this protocol.  Instead, from the viewpoint of this protocol, the request to provide the RDMA transfer is considered made when required by this protocol and any deferral to accommodate such restrictions is considered the responsibility of the implementation.
> 
> 4.3.1. Initial Connection State
> 
> The last paragraph isn't clear in that it says implementations "MAY" use values above 1K but leaves it unclear how.   My expectation is that it is not prepared to make concrete proposals but if that is the case, it should say so more clearly.  Suggest the following replacement text:
> 
> Implementations MUST support an inline threshold of 1024 bytes,  They MAY support larger inline thresholds but MUST assume the receiver’s inline threshold is 1024 bytes unless there is an appropriate exchange of buffer size information.  Since Version One of  Connection Configuration Protocol described in [RFC5666] has been removed from this specification and [RFC5666] will be obsoleted by this specification,  Additional means of exchanging such information need to be developed.

Aside from the problem of citing the RFC we are making
obsolete, does rfc5666bis need to discuss how the inline
threshold is set to a value other than 1KB? IMO, for
Version One, it is practically speaking an issue of
implementation quality. Can you elaborate on your
concern?

> Some possibilities that should be considered:

I don’t oppose careful consideration of any of these, but
I do feel such consideration is out of scope for rfc5666bis,
whose role is to document current implementation practice
to help resolve ambiguities in the language of RFC 5666.

> 	• Development of a Version Two of the Connection Configuration Protocol.
> 	• Development of an in-line log-in sequence to be used as part of configuration setup, as an optional feature within RPC/RDMA Version One.
> 	• With suitable restrictions using feature of the ULP designed to assist in conveying this information.   An extension to NFSv4.2 or a new minor version of NFSv4 are possibilities.

It’s not clear whether you’d like a ULP to report the current
inline threshold values, or to negotiate new values.

The current situation is that the peer inline thresholds are
not known before an RPC/RDMA connection is established, and
the inline threshold for an RPC/RDMA connection is fixed for
the life of that connection. All RPC programs on that
connection get the same inline threshold.

I’m not convinced that the inline threshold is or should be
known to or controlled by a ULP. Noted that RFC 5667 does
state normative requirements for the inline threshold to be
used with NFSv2 and NFSv3, but all current NFS/RDMA
implementations ignore those requirements. No NFSv3 server
I’m aware of would interoperate today with a client using
a 36KB inline threshold.

I make this observation because to continue to allow Binding
specifications to make requirements or recommendations about
the inline threshold probably necessitates some statement in
rfc5666bis Section 6.

Outright prohibition of such statements would mean rfc5666bis
obsoletes RFC 5667 as well, wouldn’t it?

> 4.4.1. DDP-Eligibility
> 
> I have problems with "Upper Layer Protocol MUST determine".  Normally "MUST"s are directed to implementations.  In this case the MUST is directed at previously published Standards-track RFCs making my head hurt.   I propose replacing third and fourth sentence of the first paragraph by the following:
> 
> For each Upper Layer Protocol, a  determination needs to be made as to which items in its XDR definition are allowed to use
> Direct Data Placement. Information about such determinations needs to be included in an additional specification is
> needed that describes how an Upper Layer Protocol enables Direct Data Placement.
> 
> 4.6.3. Long Messages
> 
> The definition in the first paragraph does not cover the case in which DDP-eligible data is excised and the message is still above the inline threshhold.  I think this case should be covered.

--
Chuck Lever

---

From: William Allen Simpson <[email-masked]>
Date: Sat, 5 Dec 2015 18:00:47 -0500

On 12/5/15 10:34 AM, David Noveck wrote:
> *
I was busily writing little section by section comments.  Easier to
handle threads than a big mass....

> *
> *The Meaning of "deprecated"
>
> *
Not relevant.

> The passive "deprecated" has a specialized meaning to indicate that a feature, API, command,etc., while usable and implemented has problems that are worthy of note and that makes use of that entity, in some way, problematic.   For example, it may be
> inefficient, confusing, soon-to-be-deleted, not thread-safe, etc.
>
In the IETF, deprecated means the symbol or value is no longer used,
but is retained as a placeholder to document its removal.

> Generally, deprecation happens before something is removed and is necessary because immediate removal is too disruptive/difficult.  It is not needed in this document because the items "deprecated"…

Everywhere I can see in this document, deprecated is used correctly.

Moreover, I'm having a hard time reading your comments, because you
don't quote the existing words inline.  Let's work on this section by
section and paragraph by paragraph.

---

From: William Allen Simpson <[email-masked]>
Date: Sat, 5 Dec 2015 18:14:28 -0500

>> The last paragraph isn't clear in that it says implementations "MAY" use values above 1K but leaves it unclear how.   My expectation is that it is not prepared to make concrete proposals but if th…
I'd already offered Chuck verbally to propose some different
wording here.  Rather than holding on to things and posting
them in the order they appear, I'll post something out of order
later tonight.

> implementation quality. Can you elaborate on your
I agree we shouldn't cite the old RFC.  This should
stand alone.

> to help resolve ambiguities in the language of RFC 5666.
I agree!  This is not a vehicle for documenting a Version Two.

> connection get the same inline threshold.
Agreed.

CCP is removed because it was not implemented by anybody.

RFC 2026:

    In cases in which one or more options or features
    have not been demonstrated in at least two interoperable
    implementations, the specification may advance ...
    only if those options or features are removed.

> I’m aware of would interoperate today with a client using
Certainly not mine.

> obsoletes RFC 5667 as well, wouldn’t it?
I'm fairly sure that has to be updated in parallel.

Let's get this one reviewed, and then start work on 5667bis.
They should be published together.