Subject: [nfsv4] Review of draft-dnoveck-nfsv4-rpcrdma-xcharext-00

From: Chuck Lever <[email-masked]>
Date: Wed, 20 Jul 2016 18:03:48 +0200

This is a long-form full document review of draft-dnoveck-nfsv4-rpcrdma-xcharext-00. Please feel free to respond with clarifying questions or further discussion where needed, and no response is necessary where the author(s) agree to a suggestion or plan to ignore it.

Comments applying to the whole document:

Subsequent extensions might each define one or more new characteristics. Since the extension specified in this document is a (necessary) extensibility enabler, should we consider merging it into the base RPC-over-RDMA Version Two protocol?

Two of the three initially specified transport characteristics need further technical discussion outside the scope of the review of this document.

*** Abstract

Editorial:

IMO it's safe to assume in the abstract that RPC-over-RDMA Version Two will have a mechanism that permits extension of the base RPC-over-RDMA protocol. Explaining that in this document's abstract seems unnecessary, and the use of subjunctive mood and future tense dulls the impact of both the abstract and the introduction section. Also, the term RPC-over-RDMA is preferred over the term RPC/RDMA.

I suggest tightening the text as follows:

OLD
It is expected that the RPC-over-RDMA transport will, at some point, allow protocol extensions to be defined. This would provide for the specification of OPTIONAL features, allowing participants who implement the OPTIONAL features, to cooperate as specified by that    extension, while still interoperating with participants who do not support that extension.

A particular extension is described herein, whose purpose is to allow RPC/RDMA Endpoints to specify and manage their transport characteristics in order to allow the other participant to optimize message transfer in light of the characteristics communicated by the initial sender.

NEW
This document specifies an extension to RPC-over-RDMA Version Two and newer. The extension enables endpoints of an RPC-over-RDMA connection to exchange information which can optimize message transfer.

*** Section 1.1:

Editorial:

Likewise, the introduction can be written to assume that the extension mechanism is in the base protocol already. I suggest replacing the first two paragraphs of Section 1.1.

OLD
This document describes a potential extension to the RPC-over-RDMA protocol, which would allow participating implementations to communicate the transport characteristics of their implementation, to request changes in those of the other participant, and to effect changes and notify the other participant of the changes.

Although this document specifies the new OPTIONAL message header types to implement these functions, the precise means by which the presence of support for these OPTIONAL functions will be ascertained is not described here, as would be done more appropriately by the RFC defining a version of RPC-over-RDMA which supports protocol extension.

NEW
This document specifies an extension to RPC-over-RDMA Version Two and newer. It allows each participating endpoint on a single connection to communicate various characteristics of its implementation, to request changes in characteristics of the other endpoint, and to effect changes and notify the other endpoint of changes to these characteristics during operation.

The extension described herein specifies OPTIONAL message header types to implement this mechanism. The means by which the implementation support status of these OPTIONAL types is ascertained is described in [rpcrmdav2].

*** Section 2.1:

Editorial:

The first sentence can be stated more clearly.

OLD
Receiver and sender characteristics are specified using an extensible approach, that allows new characteristics to be defined, in addition to the small set of initial transport characteristics specified herein.

NEW
An initial set of receiver and sender characteristics are specified in this document. An extensible approach is used, allowing new characteristics to be defined in future standards track documents.

Opinion:

The term "char" has a longstanding and well known meaning to C coders. I found the overloaded use of "char" as an abbreviation of "characteristic" to be confusing. I find "attribute" or "capability" to be more in line with the intended usage and either of these seems to be preferred historically over "characteristic". There are plenty of examples, including within NFS itself, of the use of the term "attribute."

In addition, I found the use of the prefix "x" to be confusing. Here I presume that "x" means "transport", not "transmit" or "unknown value".

Since this is an extension of RPC-over-RDMA, I was surprised not to find "rpcrdma" in the data type names.

The variable, type, and operation names used in this document are often not particularly mnemonic. I suggest (somewhat) longer names to help the reader remember function. Even an additional vowel or two might be helpful.

Technical:

I suggest that RFC 2119 terms are appropriate in the following sentence:

OLD
The receiver of a message containing an xcharval needs to report an XDR error if the length of xchv_data is such that it extends beyond the bounds of the message transferred.

NEW
If the length of xchv_data is such that it extends beyond the bounds of the transferred message, the receiver MUST report an XDR error.

Likewise for the following sentence: "the receiver also needs to report" ought to be "the receiver MUST report".

When first reading this section, it was not clear to me what the xcharsubset type is for. To provide some context for this type definition, it would help to add a paragraph in Section 2.1 briefly summarizing the later discussion: 

	• The xcharsubset type is used for reporting completion of one or more changes of characteristics, and that
	• Completing a change request does not have to be synchronous

*** Table 1:

Technical:

The default values provided here are based on the current behavior of RPC-over-RDMA Version Two. Future versions of RPC-over-RDMA may prescribe different default behavior for each of these characteristics. I suggest it would be better to defer the presentation of default values until subsections 3.1, 3.2, and 3.3, with explicit association of these particular values with RPC-over-RDMA Version Two only. Consider a minor text rewrite to make the default values dependent on the RPC-over-RDMA Version being deployed.

BKRQSUP_UNKNOWN is in fact not the default behavior of RPC-over-RDMA Version Two. Version Two REQUIRES support for backwards direction operation. I believe the correct default value for Version Two would be BKRQSUP_SZLIM ? More on that later.

Should Table 1 include a column that indicates whether a characteristic is Requester-only, Responder-only, or Both? Should the names of the characteristics also reflect this?

Regarding the remark following Table 1, implementations can choose not to support updating some or all of these characteristics. I suggest it is appropriate to use an RFC 2119 term here to make it explicit that implementations have a choice, and that they must be prepared for a peer to reject a change request. Here is a sample additional paragraph:

Peers MAY reject a request to change a characteristic for any reason. There are at least two ways to reject a characteristic change request. One is to not implement the REQ_XCHAR, RESP_XCHAR, and UPD_XCHAR operations. The other is to reject change requests for individual characteristics. See Section 4.3 for details.

*** Section 3.1:

Editorial:

For clarity, I suggest capitalizing Request Buffer Size whenever referring to the characteristic itself.

Technical:

Please explicitly specify the units of the xchrbsiz value. Octets is assumed, but in the context of XDR, the value could be in XDR quads, for instance. The buffer size in octets is likely to always be a multiple for four.

The data types defined here are:

const uint32    XCHAR_RBSIZ = 1;
typedef uint32  xchrbsiz;

I find these choices less readable than, for example, XCHAR_RCVBUF_SIZE and xchr_rcvbufsz. These are not necessarily my most preferred names, they simply illustrate that I believe we can afford a few more characters in these names to help legibility. The initial subset of characteristics are setting a precedent for future naming choices.

The terms "Position-Zero Read chunk" and "Reply chunk" as defined in rfc5666bis are capitalized.

*** Section 3.2:

Editorial:

Rather than the term "remote invalidation requests," I recommend the more specific term "RDMA Send With Invalidate". Here and elsewhere in the document, the term "invalidate" is preferred over "deregister". (The term is "Remote Invalidation" not "Remote Deregistration," after all).

Technical:

The mention of the server's lack of use of memory registration is not technically correct. A server often does register memory regions that are used as RDMA Read sink buffers. (the Linux server registers these buffers using FRWR; Solaris does not register these buffers, relying on physical addressing and a smaller scatter/gather limit per Read WR). These registered sink buffers are not exposed to other network nodes.

Finally, although there are no DDP-eligible data items in currently defined backward-direction ULPs, an implementation can still choose to use Position-Zero Read chunks or Reply chunks for larger messages. I'm not sure what exactly to do about backward-direction use of Remote Invalidation, other than to have the responder also provide this characteristic via INIT_XCHAR, but always set to false for now, or possibly stating that this characteristic is for forward-direction requests only (ie, punting).

Therefore may I suggest replacing the first two paragraphs with the following:

OLD
The requester remote invalidation characteristic indicates that the requester is prepared for the responder to issue remote invalidation requests, in order to unregister memory regions established to support RDMA Read and Write operations done by the responder into or out of the requester’s memory.

As RPC-over-RDMA is currently used, memory registration is not done on the server and explicit RDMA operations are not done to satisfy backward-direction requests. This makes it unlikely that servers will present non-default values of the XCHAR_RQREMINV characteristic or that clients will take note of the value presented by servers.

NEW
The Requester Remote Invalidation characteristic indicates that a forward-direction requester is prepared for a forward-direction responder to use RDMA Send With Invalidate when replying to an RPC-over-RDMA request containing non-empty chunk lists.

IMO, there is no need to include a discussion of the implementation of Remote Invalidation in this document. After <CODE ENDS> in this section, I suggest finishing the section with the following two replacement paragraphs:

When the Requester Remote Invalidate characteristic is set to false, a responder MUST use Send to convey RPC reply messages to the requester. When the Requester Remote Invalidate characteristic is set to true, a responder MAY use Send With Invalidate instead of Send to convey RPC replies to the requester.

The value of the Requester Remote Invalidate characteristic is not likely to change from the value reported by INIT_XCHAR (see Section 4.1).

After some research, I've found that full support for RI is done in other protocols by having the client mark the chunk in each request it wants the server to invalidate. This allows servers to clearly distinguish chunks that are persistently registered (where an attempted invalidation might be terminal to the connection) and ones that have a short-lived tag (where invalidation should work); and of course it also indicates when the client cannot tolerate any remove invalidation.

This is not a mechanism that can be provided with a transport characteristic, but would require an XDR change in the RPC-over-RDMA header. In particular, it would require changes to the chunk list data structures. For example, the set of three chunk lists in each Call could be followed by a field containing the tag that the client wishes to be invalidated. Once such a data structure change is in place, a Requester Remote Invalidation characteristic might be superfluous.

We should discuss this further outside the scope of this review to come to agreement on how to proceed.

*** Section 3.3:

Technical:

RPC-over-RDMA Version Two and newer implementations are required to have support for backward-direction requests. I don't see a use for BKRQSUP_UNKNOWN or BKRQSUP_NONE here; V1, where NONE might be appropriate, isn't extensible, and thus will never have transport characteristics.

The only unknown is whether the implementation can support the use of chunks for backward-direction requests. This should not be a problem if the ULP does not have any DDP-eligible data items, and has a mechanism for indicating request size limits and it keeps the size of backward direction requests under the inline threshold.

Note that Version Two's inline threshold is at least 4KB, and implementations are free to advertise an even larger threshold using the Receive Buffer Size characteristic. This size limit is actively advertised by the only ULP that uses backward-direction requests, NFSv4.x.

The rpcrdma-bidirection specification purposely does not prohibit chunks in the backward direction. When a backward direction ULP is proposed with DDP-eligible data items, support for backward direction chunks will be introduced in RPC-over-RDMA implementations where that ULP is supported.

Outside the scope of this review, let's propose some use cases that demonstrate how this characteristic can be used effectively, and then we can consider adjusting what is currently proposed.

*** Section 4:

Editorial:

I suggest OPT_XCHAR_ or ROPT_XCHAR_ as a prefix rather than using an _XCHAR suffix for the operation names.

It might be useful to have a separate table which states what the receiver's expected response is (if I understand correctly):

Sender:            response:
INIT_XCHAR    INIT_XCHAR
REQ_XCHAR   RESP_XCHAR or UPD_XCHAR
RESP_XCHAR none
UPD_XCHAR   none

*** Section 4.1:

Technical:

Paragraph ending:

can be received by partners who use the default minimum receive buffer size.

I suggest the word "minimum" be dropped from that sentence. Afterwards, add something to the effect of:

The connection's initial receive buffer size is typically 1KB, but it depends on the initial connection state of the RPC-over-RDMA version in use. See [rpcrmdav2] for details.

*** Section 4.2:

Editorial:

The first paragraph has a superfluous comma:

The REQ_XCHAR message type allows an RPC-over-RDMA participant, whether client or server, to request of its partner >>,<< that relevant transport characteristics be changed.

The second paragraph is missing its ending period.

*** Section 4.3:

Editorial:

OLD
future transmissions may be made based on that assumption.

NEW
future transmissions may be made based on the new value.

*** Section 4.4:

Editorial:

The first paragraph has a superfluous comma:

The UPD_XCHAR message type allows an RPC-over-RDMA participant >>,<< to notify the other participant that a change to the transport characteristics has occurred.

The following paragraph:

Note that there no way to tie a request changed to the specific request which asked for it.  In particular, the xid associated with this message is independent of that for an earlier REQ_XCHAR message.

Seems more important than just a "Note". I suggest dropping "Note that," and clarifying the phrase "there no way to tie a request changed".

Technical:

I suggest something like ROPT_XCHAR_NOTIFY rather than UPD_XCHAR.

I suggest that it be made clear that "xid" in this document means the value in the RPC-over-RDMA header's rdma_xid field. In addition, the call-response relationships of the new operations proposed in this document are perhaps important enough (and unidirectional, not like normal RPC) to move discussion into a separate subsection of Section 4, or perhaps earlier in the document.

This section finishes with the following text:

optuxc_pendclr, if true, indicates that a previous request to update the characteristic specified by optuxc_now.xchv_which is no longer to be considered pending. This may be set true even if the characteristic value is not changed from the previous value.

This seems to imply that, after receiving an UPD_XCHAR message, the receiver must query the sender to see if the change actually took effect. Did I misunderstand this? Would it be better if UPD_XCHAR reported whether a characteristic change took effect or was rejected? Under what circumstances would optucx_pendclr be set to false?

*** Section 6.1:

Editorial:

The title of this section is "Additional Operations" but the two paragraphs here discuss only transport characteristics. Perhaps this subsection could be deleted?

The second paragraph is missing its ending period.

*** Section 6.2:

Editorial:

I suggest breaking the first sentence into two or three sentences and restating.

*** Section 6.3:

Technical:

I suggest adding an explicit statement here that use of xcharid values in the experimental range is not guaranteed to interoperate among independent implementations.

*** Section 7:

Editorial:

I suggest moving the second paragraph to section 6.2.

Technical:

I suggest replacing the first paragraph with the following two:

Like other fields that appear in each RPC-over-RDMA header, characteristic information is sent in the clear on the fabric with no integrity protection, making it vulnerable to man-in-the-middle attacks.

For example, if a man-in-the-middle changes the value of the Receive buffer size or the Requester Remote Invalidation boolean, it can reduce connection performance or trigger loss of connection. Repeated connection loss can impact performance or even prevent a new connection from being established. Recourse is to deploy on a private network or use link-layer encryption.

--
Chuck Lever

---

From: David Noveck <[email-masked]>
Date: Fri, 22 Jul 2016 09:20:11 -0400

> This is a long-form full document review of draft-dnoveck-nfsv4-rpcrdma-
xcharext-00.

I just sent a response to this review.

Unfortunately, because of the 100K limit, this response has not yet gone to
the
working group list.

I assume that  this will be allowed onto the list in the next few days.  If
that doesn't
happen quickly enough for someone, you can send me some mail and I'll
forward
you a copy.

---

From: David Noveck <[email-masked]>
Date: Fri, 22 Jul 2016 05:04:56 -0400

xcharext-00.

Thanks for the review.

> Please feel free to respond with clarifying questions or further
discussion where needed, and no response is necessary
> where the author(s) agree to a suggestion or plan to ignore it.

To be clearer, I will

   - If I'm not following a suggestion I will say why rather than simply
   ignoring it.
   - In cases in which I simply agree to all the suggestions in a given
   section, I will just say "OK" to that section.

> Subsequent extensions might each define one or more new characteristics.
Since the extension specified in this document
> is a (necessary) extensibility enabler, should we consider merging it
into the base RPC-over-RDMA Version Two protocol?

Yes. As discussed at the meeting, this makes architectural sense.

> Two of the three initially specified transport characteristics need
further technical discussion outside the scope of the review of this
document.

I'm not sure as to best ordering of:

   - Those discussions
   - Production of a -01
   - Incorporation of xcharext into a new version-two document.

For now, I'll make the changes I'm sure about into the existing xml.  We
can decide what to
do later about things that might need further discussion.  I'll be keeping
a to-do/to-decide list.
It may be that some of the more disruptive of those changes might be done
as part of effecting
the document amalgamation.

> NEW
> This document specifies an extension to RPC-over-RDMA Version Two and
newer.

I have problem with the phrase "and newer".  I'm not sure what you are
assuming Version Three will be like such that it is sufficiently different
from Version Two to require a new version number and still need this
extension.   Also, if this extension is an integral part of version Two,
then it will remain in subsequent versions until someone makes an explicit
decision to take it out.

> NEW
> This document specifies an extension to RPC-over-RDMA Version Two and
newer.

Again, I have a problem with "and newer".  No problem with the rest of it.

> The first sentence can be stated more clearly.

OK.

> The term "char" has a longstanding and well known meaning to C coders. I
found the overloaded use of "char" as an abbreviation of "characteristic"
to be confusing.

I can see that might be a problem, but I don't think I use the unadorned
abbreviation "char'.  It is always part of the string "_XCHAR".  I can
change this and probably will.

> I find "attribute" or "capability" to be more in line with the intended
usage and either of these seems to be preferred historically over
"characteristic".
> There are plenty of examples, including within NFS itself, of the use of
the term "attribute."

At the time, I was worried that the use of the same term might be
confusing.  "capability" seems like it is limited to binary choices.

I'm willing to change this but note that it would change the title and the
filename  document.  As a result i am not making this change now.

> In addition, I found the use of the prefix "x" to be confusing. Here I
presume that "x" means "transport", not "transmit" or "unknown value".

You presume correctly.

Also, note that if we change to "attribute", "XCHAR" would become "XATTR"
:-)

> Since this is an extension of RPC-over-RDMA, I was surprised not to find
"rpcrdma" in the data type names.

If we add that (or "rpcrdma2" or "rpcrpdma2plus") to everything then the
names start getting very long for np very goo reaspn, and I prefer shorter
names.

> The variable, type, and operation names used in this document are often
not particularly mnemonic.
> I suggest (somewhat) longer names to help the reader remember function.
Even an additional vowel or two might be helpful.

Adding a few characters to make it more mnemonic is OK with me, I have
problems if that is carried to an extreme.

Although I think we can come to agreement about this issue, I think it
makes most sense to address this as part the merger of the two documents.
Are you OK with that?

For now, this is on my to-do/to-decide list.

> I suggest that RFC 2119 terms are appropriate in the following sentence:

OK with these.

> OLD
> The receiver of a message containing an xcharval needs to report an XDR
error if the length of xchv_data is such that it extends beyond the bounds
of the message transferred.

> NEW
> If the length of xchv_data is such that it extends beyond the bounds of
the transferred message, the receiver MUST report an XDR error.

> Likewise for the following sentence: "the receiver also needs to report"
ought to be "the receiver MUST report".

I've used the MUSTs but I haven't adopted your specific rephrasings,

> When first reading this section, it was not clear to me what the
xcharsubset type is for. To provide some context for this type definition,
it would help to add a
> paragraph in Section 2.1 briefly summarizing the later discussion:

>        • The xcharsubset type is used for reporting completion of one or
more changes of characteristics, and that
>        • Completing a change request does not have to be synchronous

Will address.

> The default values provided here are based on the current behavior of
RPC-over-RDMA Version Two.

True.

> Future versions of RPC-over-RDMA may prescribe different default behavior
for each of these characteristics.

They might, but given the fact that we now have a mechanism to pick and
communicate the value you want, I don't expect that changing the default
values will be all that important.

> I suggest it would be better to defer the presentation of default values
until subsections 3.1, 3.2, and 3.3, with explicit association of these
particular values with RPC-over-RDMA Version
> Two only. Consider a minor text rewrite to make the default values
dependent on the RPC-over-RDMA Version being deployed.

I'm Ok with this but note that, if we merge the documents, the problem will
come close to solving itself.  If these appear in  a document defining
Version Two, then obviously the default values are the default values
within Version Two.

In a section describing further extensability one could state:

   - New versions can add characteristics
   - New versions should not change the structure of an existing
   characteristic.
   - New versions can change the default value of a characteristic

For now, I am putting this on the to-do list

> BKRQSUP_UNKNOWN is in fact not the default behavior of RPC-over-RDMA
Version Two. Version Two REQUIRES support for backwards direction
operation. I believe the correct default value > for Version Two would be
BKRQSUP_SZLIM ? More on that later.

Ok on the default.  Will discuss further implications later.

> Should Table 1 include a column that indicates whether a characteristic
is Requester-only, Responder-only, or Both? Should the names of the
characteristics also reflect this?

I don't think this is an appropriate way to categorize characteristics.  A
endpoint is known to be either a client or a server but whether it is
requester or responder depends on
the direction of the particular message being processed.   Particular
characteristics may be relevant to particular roles, but I don't see that
as a basis for categorization.  For example the receive buffer size applies
to the endpoint in its role as receiver but that doesn't make it a
receiver-only characteristic, since all endpoints are receivers.  Similarly
for characteristics that only apply to the endpoint in the role of
requester.  Given bi-directional operation, these will apply to the client
or the server, depending on the direction of transfer.

> Regarding the remark following Table 1, implementations can choose not to
support updating some or all of these characteristics.
> I suggest it is appropriate to use an RFC 2119 term here to make it
explicit that implementations have a choice, and that they must
> be prepared for a peer to reject a change request. Here is a sample
additional paragraph:

> Peers MAY reject a request to change a characteristic for any reason.

OK.

> There are at least two ways to reject a characteristic change request.
One is to not implement the REQ_XCHAR, RESP_XCHAR, and UPD_XCHAR
operations.

I agree about REQ_XCHAR.  Not implementing the others doesn't result in
request rejection.  That just just means you can't find out about the other
endpoint's rejection of your request.

> The other is to reject change requests for individual characteristics.
See Section 4.3 for details.

OK.

> ** Section 3.1:

> For clarity, I suggest capitalizing Request Buffer Size whenever
referring to the characteristic itself.

The characteristic is "receive buffer size".  I capitalized it in two
places.

> Please explicitly specify the units of the xchrbsiz value. Octets is
assumed, but in the context of XDR, the value could be in XDR quads, for
instance.
> The buffer size in octets is likely to always be a multiple for four.

OK.

> I find these choices less readable than, for example, XCHAR_RCVBUF_SIZE
and xchr_rcvbufsz.

I'm OK with these changes, but will hold off for a while in order to make
sure we make all the necessary changes consistently.  This is on my
to-do/to-decide list.

> These are not necessarily my most preferred names, they simply illustrate
that I believe we can afford a few more characters in these names to help
legibility.

I would have a problem with RPCRDMA2_TRANSPORT_CHAR_RECEIVE_BUFFER_SIZE and
rpcrpma2_transport_char_receive_buffer_size :-(

> The initial subset of characteristics are setting a precedent for future
naming choices.

Good point.

> The terms "Position-Zero Read chunk" and "Reply chunk" as defined in
rfc5666bis are capitalized.

OK

> Rather than the term "remote invalidation requests," I recommend the more
specific term "RDMA Send With Invalidate". Here and elsewhere in the
document, > the term "invalidate" is preferred over "deregister". (The term
is "Remote Invalidation" not "Remote Deregistration," after all).

OK.

Technical:

> The mention of the server's lack of use of memory registration is not
technically correct.

Will address.

> Finally, although there are no DDP-eligible data items in currently
defined backward-direction ULPs, an implementation can still choose to use
Position-Zero Read chunks or
> Reply chunks for larger messages.

Good point.

> I'm not sure what exactly to do about backward-direction use of Remote
Invalidation, other than to have the responder also provide this
characteristic via INIT_XCHAR, but
> always set to false for now,

What I feel is consistent with that but I'd distinguish between what the
spec  says to do and what we expect to implementations to do in the near
future.

The spec should state that this chracteristic applies to both directions of
operation.

You and I know that server implementations, for the forseeable future are
either going to not report this characteristic or report it as false. also
clients are unlikely to look at it for a while.  The document may provide
this information as interesting background but that doesn't affect the
protocol, which allows that situation to change.

> possibly stating that this characteristic is for forward-direction
requests only (ie, punting).

I don't think punting is appropriate.  This is a
second-down-and-inches-to-go situation.

> The requester remote invalidation characteristic indicates that the
requester is prepared for the responder to issue remote invalidation
requests, in order to unregister memory > regions established to support
RDMA Read and Write operations done by the responder into or out of the
requester’s memory.
>
> As RPC-over-RDMA is currently used, memory registration is not done on
the server and explicit RDMA operations are not done to satisfy
backward-direction requests. This
> makes it unlikely that servers will present non-default values of the
XCHAR_RQREMINV characteristic or that clients will take note of the value
presented by servers.

> NEW
> The Requester Remote Invalidation characteristic indicates that a
forward-direction requester is prepared for a forward-direction responder
to use RDMA Send With Invalidate
> when replying to an RPC-over-RDMA request containing non-empty chunk
lists.

I don't think we want to foreclose the option of using this in the reverse
direction.

PROPOSED:

The Requester Remote Invalidation characteristic indicates that the current
endpoint, when in the role of a requester, is prepared for the responder to
use RDMA Send With Invalidate when replying to an RPC-over-RDMA request
containing non-empty chuck lists.

As RPC-over-RDMA is currently used, memory registration exposed to peers
are not done by the server and explicit RDMA operations are not done to
satisfy backward direction requests.  This makes it unlikely that servers
will present non-default values of the XCHAR_RQREQ characteristic or that
clients will take note of such values when presented by servers.

> IMO, there is no need to include a discussion of the implementation of
Remote Invalidation in this document. After <CODE ENDS> in this section, I
suggest finishing the section with the following
> two replacement paragraphs:

> When the Requester Remote Invalidate characteristic is set to false, a
responder MUST use Send to convey
> RPC reply messages to the requester. When the Requester Remote Invalidate
characteristic is set to true, a
> responder MAY use Send With Invalidate instead of Send to convey RPC
replies to the requester.

> The value of the Requester Remote Invalidate characteristic is not likely
to change from the value reported by INIT_XCHAR (see Section 4.1).

OK.

 -- stuff about possible need for XDR change deleted ---

> We should discuss this further outside the scope of this review to come
to agreement on how to proceed.

OK.  I'll note this on the to-do/to-decde list.

I also note that if there is a change in the chunk XDR, I will need to
update the XDR In draft-dnoveck-nfsv4-rpcrdma-rtrext to match.

> *** Section 3.3:
>
> Technical:
>
> RPC-over-RDMA Version Two and newer implementations are required to have
support for backward-direction requests.

I didn't know that.  Now that you mention it, I'm not sure it is such a
good idea.  For example, I don't think that you want to
prevent an NFSv3 implementation, which has no need to use backward
direction requests, from upgrading from V1 to V2.  That
issue can be discussed further. For now, I'll keep this on the
to-do/to-decide list.

> I don't see a use for BKRQSUP_UNKNOWN or BKRQSUP_NONE here; V1, where
> NONE might be appropriate, isn't extensible, and thus will never have
transport characteristics.

Depends on the decision on whether backward direction support is required
in V2.

>The only unknown is whether the implementation can support the use of
chunks for backward-direction requests. This should not be a problem if the
ULP does not have any DDP-eligible data items, and has a mechanism for
indicating request size limits and it keeps the size of backward direction
requests under the inline threshold.

> Note that Version Two's inline threshold is at least 4KB, and
implementations are free to advertise an even larger threshold using the
Receive Buffer Size characteristic.

> This size limit is actively advertised by the only ULP that uses
backward-direction requests, NFSv4.x.

> The rpcrdma-bidirection specification purposely does not prohibit chunks
in the backward direction.

Exactly.  In version Two, this is optional behavior. The characteristic in
question provides a means to determine whether the peer supports transfer
of such chunks.

> When a backward direction ULP is proposed with DDP-eligible data items,
support for backward direction chunks will be introduced
> in RPC-over-RDMA implementations where that ULP is supported.

Yes, but my hope is that we will be in a situation in which the
implementation support can be introduced but no protocol change is
required.  If we don't provide the support before it is needed, it is
likely that it will be needed before it is available, which would be a real
drag.

> Outside the scope of this review, let's propose some use cases that
demonstrate how this characteristic can be used effectively, and then we
can consider adjusting what is currently proposed.

It has to be understood that any such use case will necessarily be
hypothetical, in that they have to assume that there will be a ULP that
wants this support, that Version Two, has, so far, been careful not to
prevent.

> I suggest OPT_XCHAR_ or ROPT_XCHAR_ as a prefix rather than using an
_XCHAR suffix for the operation names.

I'm OK with this but am going to hold off a bit since a merger with Version
Two may supersede this.  This is now on my to-decide list.

After all, if this is merged fully into Version Two, we might have
RDMA_INIT_XCHAR, etc.

> It might be useful to have a separate table which states what the
receiver's expected response is (if I understand correctly):

Not sure a table is useful.  Nevertheless the discussion of what would be
in it would be helpful.

> INIT_XCHAR    INIT_XCHAR

As stated, this would result infinite response-to-response loop :-(

I think of each of these as independently offered by each endpoint.

> REQ_XCHAR   RESP_XCHAR or UPD_XCHAR

RESP_XCHAR is the response to REQ_XCHAR.  In addition, one or more
UPD_XCHARs might be generate but these are
asynchronous and not readily connected with the REQ_XCHAR That caused them
to be generated.

> UPD_XCHAR   none

Yes.

> The connection's initial receive buffer size is typically 1KB, but it
depends on the initial connection state of the RPC-over-RDMA version in
use. See [rpcrmdav2] for details.

OK.

*** Section 4.2:

OK..

> *** Section 4.3:

OK

> The UPD_XCHAR message type allows an RPC-over-RDMA participant >>,<< to
notify the other participant that a change to the transport characteristics
has occurred.

Will fix.

> Note that there no way to tie a request changed to the specfic request
which asked for it.  In particular, the xid associated with this message is
independent of that for an earlier REQ_XCHAR message.

> Seems more important than just a "Note".

It is not "just a 'Note'", even though the verb "Note" is used.

> I suggest dropping "Note that,"

I don't see that raises the prominence or perceived importance of this fact.

One possibility is to say:

It is important to note that ...

another possibility is to replace the verb "note" by its definition,
resulting in:

One should pay particular attention to the fact that there is no way

> and clarifying the phrase "there no way to tie a request changed".

I think that is in the following sentence.

> I suggest something like ROPT_XCHAR_NOTIFY rather than UPD_XCHAR.

"NOTIFY" seems too generic to me.  How about "ROPT_XCHAR_CHANGED"?  I
supposed that this would be batched with other similar name changes.

> I suggest that it be made clear that "xid" in this document means the
value in the RPC-over-RDMA header's rdma_xid field.

I've done that.

> In addition, the call-response relationships of the new operations
proposed in this document are perhaps important enough
> (and unidirectional, not like normal RPC) to move discussion into a
separate subsection of Section 4, or perhaps earlier in the document.

I will do that.

> optuxc_pendclr, if true, indicates that a previous request to update the
characteristic specified by optuxc_now.xchv_which
> is no longer to be considered pending. This may be set true even if the
characteristic value is not changed from the previous value.

> This seems to imply that, after receiving an UPD_XCHAR message, the
receiver must query the sender to see if the change actually took effect.
Did I misunderstand this?

It doesn't.  The receiver gets the new current value of the characteristic
in any case.

> Would it be better if UPD_XCHAR reported whether a characteristic change
took effect or was rejected?

rejection is reported by RESP_XCHAR.

> Under what circumstances would optucx_pendclr be set to false?

Two possibilities;

   - change not requested.  i.e. the sender is notifying you of a change
   you didn't request.
   - you requested a change but the request is still pending. this could
   happen if you requested that something be changed and instead of waiting
   untuil the change was effected, the sender decides to update you on partial
   completion, while continuing to keep the original request pending.

*** Section 6.1:

Editorial:

> The title of this section is "Additional Operations" but the two
paragraphs here discuss only transport characteristics. Perhaps this
subsection could be deleted?

I will delete it.

> The second paragraph is missing its ending period.

Will fix if I don't delete this.

*** Section 6.2:

> I suggest breaking the first sentence into two or three sentences and
restating.

Will do.

*** Section 6.3:

OK.

*** Section 7:

OK.

On Wed, Jul 20, 2016 at 12:03 PM, Chuck Lever <[email-masked]>
wrote:

> This is a long-form full document review of
> draft-dnoveck-nfsv4-rpcrdma-xcharext-00. Please feel free to respond with
> clarifying questions or further discussion where needed, and no response is
> necessary where the author(s) agree to a suggestion or plan to ignore it.
>
>
> Subsequent extensions might each define one or more new characteristics.
> Since the extension specified in this document is a (necessary)
> extensibility enabler, should we consider merging it into the base
> RPC-over-RDMA Version Two protocol?
>
> Two of the three initially specified transport characteristics need
> further technical discussion outside the scope of the review of this
> document.
>
>
> *** Abstract
>
> Editorial:
>
> IMO it's safe to assume in the abstract that RPC-over-RDMA Version Two
> will have a mechanism that permits extension of the base RPC-over-RDMA
> protocol. Explaining that in this document's abstract seems unnecessary,
> and the use of subjunctive mood and future tense dulls the impact of both
> the abstract and the introduction section. Also, the term RPC-over-RDMA is
> preferred over the term RPC/RDMA.
>
> It is expected that the RPC-over-RDMA transport will, at some point, allow
> protocol extensions to be defined. This would provide for the specification
> of OPTIONAL features, allowing participants who implement the OPTIONAL
> features, to cooperate as specified by that    extension, while still
> interoperating with participants who do not support that extension.
>
> A particular extension is described herein, whose purpose is to allow
> RPC/RDMA Endpoints to specify and manage their transport characteristics in
> order to allow the other participant to optimize message transfer in light
> of the characteristics communicated by the initial sender.
>
> NEW
> This document specifies an extension to RPC-over-RDMA Version Two and
> newer. The extension enables endpoints of an RPC-over-RDMA connection to
> exchange information which can optimize message transfer.
>
>
> *** Section 1.1:
>
> Editorial:
>
> Likewise, the introduction can be written to assume that the extension
> mechanism is in the base protocol already. I suggest replacing the first
> two paragraphs of Section 1.1.
>
> OLD
> This document describes a potential extension to the RPC-over-RDMA
> protocol, which would allow participating implementations to communicate
> the transport characteristics of their implementation, to request changes
> in those of the other participant, and to effect changes and notify the
> other participant of the changes.
>
> Although this document specifies the new OPTIONAL message header types to
> implement these functions, the precise means by which the presence of
> support for these OPTIONAL functions will be ascertained is not described
> here, as would be done more appropriately by the RFC defining a version of
> RPC-over-RDMA which supports protocol extension.
>
> NEW
> This document specifies an extension to RPC-over-RDMA Version Two and
> newer. It allows each participating endpoint on a single connection to
> communicate various characteristics of its implementation, to request
> changes in characteristics of the other endpoint, and to effect changes and
> notify the other endpoint of changes to these characteristics during
> operation.
>
> The extension described herein specifies OPTIONAL message header types to
> implement this mechanism. The means by which the implementation support
> status of these OPTIONAL types is ascertained is described in [rpcrmdav2].
>
>
> *** Section 2.1:
>
> Editorial:
>
> Receiver and sender characteristics are specified using an extensible
> approach, that allows new characteristics to be defined, in addition to the
> small set of initial transport characteristics specified herein.
>
> NEW
> An initial set of receiver and sender characteristics are specified in
> this document. An extensible approach is used, allowing new characteristics
> to be defined in future standards track documents.
>
>
> Opinion:
>
> The term "char" has a longstanding and well known meaning to C coders. I
> found the overloaded use of "char" as an abbreviation of "characteristic"
> to be confusing. I find "attribute" or "capability" to be more in line with
> the intended usage and either of these seems to be preferred historically
> over "characteristic". There are plenty of examples, including within NFS
> itself, of the use of the term "attribute."
>
> In addition, I found the use of the prefix "x" to be confusing. Here I
> presume that "x" means "transport", not "transmit" or "unknown value".
>
> The variable, type, and operation names used in this document are often
> not particularly mnemonic. I suggest (somewhat) longer names to help the
> reader remember function. Even an additional vowel or two might be helpful.
>
>
> Technical:
>
> The receiver of a message containing an xcharval needs to report an XDR
> error if the length of xchv_data is such that it extends beyond the bounds
> of the message transferred.
>
> NEW
> If the length of xchv_data is such that it extends beyond the bounds of
> the transferred message, the receiver MUST report an XDR error.
>
> When first reading this section, it was not clear to me what the
> xcharsubset type is for. To provide some context for this type definition,
> it would help to add a paragraph in Section 2.1 briefly summarizing the
> later discussion:
>
>         • The xcharsubset type is used for reporting completion of one or
> more changes of characteristics, and that
> The default values provided here are based on the current behavior of
> RPC-over-RDMA Version Two. Future versions of RPC-over-RDMA may prescribe
> different default behavior for each of these characteristics. I suggest it
> would be better to defer the presentation of default values until
> subsections 3.1, 3.2, and 3.3, with explicit association of these
> particular values with RPC-over-RDMA Version Two only. Consider a minor
> text rewrite to make the default values dependent on the RPC-over-RDMA
> Version being deployed.
>
> BKRQSUP_UNKNOWN is in fact not the default behavior of RPC-over-RDMA
> Version Two. Version Two REQUIRES support for backwards direction
> operation. I believe the correct default value for Version Two would be
> BKRQSUP_SZLIM ? More on that later.
>
> Should Table 1 include a column that indicates whether a characteristic is
> Requester-only, Responder-only, or Both? Should the names of the
> characteristics also reflect this?
>
> Regarding the remark following Table 1, implementations can choose not to
> support updating some or all of these characteristics. I suggest it is
> appropriate to use an RFC 2119 term here to make it explicit that
> implementations have a choice, and that they must be prepared for a peer to
> reject a change request. Here is a sample additional paragraph:
>
> Peers MAY reject a request to change a characteristic for any reason.
> There are at least two ways to reject a characteristic change request. One
> is to not implement the REQ_XCHAR, RESP_XCHAR, and UPD_XCHAR operations.
> The other is to reject change requests for individual characteristics. See
> Section 4.3 for details.
>
>
> *** Section 3.1:
>
> Editorial:
>
> Please explicitly specify the units of the xchrbsiz value. Octets is
> assumed, but in the context of XDR, the value could be in XDR quads, for
> instance. The buffer size in octets is likely to always be a multiple for
> four.
>
> I find these choices less readable than, for example, XCHAR_RCVBUF_SIZE
> and xchr_rcvbufsz. These are not necessarily my most preferred names, they
> simply illustrate that I believe we can afford a few more characters in
> these names to help legibility. The initial subset of characteristics are
> setting a precedent for future naming choices.
>
> Rather than the term "remote invalidation requests," I recommend the more
> specific term "RDMA Send With Invalidate". Here and elsewhere in the
> document, the term "invalidate" is preferred over "deregister". (The term
> is "Remote Invalidation" not "Remote Deregistration," after all).
>
>
> Technical:
>
> The mention of the server's lack of use of memory registration is not
> technically correct. A server often does register memory regions that are
> used as RDMA Read sink buffers. (the Linux server registers these buffers
> using FRWR; Solaris does not register these buffers, relying on physical
> addressing and a smaller scatter/gather limit per Read WR). These
> registered sink buffers are not exposed to other network nodes.
>
> Finally, although there are no DDP-eligible data items in currently
> defined backward-direction ULPs, an implementation can still choose to use
> Position-Zero Read chunks or Reply chunks for larger messages. I'm not sure
> what exactly to do about backward-direction use of Remote Invalidation,
> other than to have the responder also provide this characteristic via
> INIT_XCHAR, but always set to false for now, or possibly stating that this
> characteristic is for forward-direction requests only (ie, punting).
>
> The requester remote invalidation characteristic indicates that the
> requester is prepared for the responder to issue remote invalidation
> requests, in order to unregister memory regions established to support RDMA
> Read and Write operations done by the responder into or out of the
> requester’s memory.
>
> As RPC-over-RDMA is currently used, memory registration is not done on the
> server and explicit RDMA operations are not done to satisfy
> backward-direction requests. This makes it unlikely that servers will
> present non-default values of the XCHAR_RQREMINV characteristic or that
> clients will take note of the value presented by servers.
>
> NEW
> The Requester Remote Invalidation characteristic indicates that a
> forward-direction requester is prepared for a forward-direction responder
> to use RDMA Send With Invalidate when replying to an RPC-over-RDMA request
> containing non-empty chunk lists.
>
>
> IMO, there is no need to include a discussion of the implementation of
> Remote Invalidation in this document. After <CODE ENDS> in this section, I
> suggest finishing the section with the following two replacement paragraphs:
>
> When the Requester Remote Invalidate characteristic is set to false, a
> responder MUST use Send to convey RPC reply messages to the requester. When
> the Requester Remote Invalidate characteristic is set to true, a responder
> MAY use Send With Invalidate instead of Send to convey RPC replies to the
> requester.
>
> The value of the Requester Remote Invalidate characteristic is not likely
> to change from the value reported by INIT_XCHAR (see Section 4.1).
>
>
> After some research, I've found that full support for RI is done in other
> protocols by having the client mark the chunk in each request it wants the
> server to invalidate. This allows servers to clearly distinguish chunks
> that are persistently registered (where an attempted invalidation might be
> terminal to the connection) and ones that have a short-lived tag (where
> invalidation should work); and of course it also indicates when the client
> cannot tolerate any remove invalidation.
>
> This is not a mechanism that can be provided with a transport
> characteristic, but would require an XDR change in the RPC-over-RDMA
> header. In particular, it would require changes to the chunk list data
> structures. For example, the set of three chunk lists in each Call could be
> followed by a field containing the tag that the client wishes to be
> invalidated. Once such a data structure change is in place, a Requester
> Remote Invalidation characteristic might be superfluous.
>
> RPC-over-RDMA Version Two and newer implementations are required to have
> support for backward-direction requests. I don't see a use for
> BKRQSUP_UNKNOWN or BKRQSUP_NONE here; V1, where NONE might be appropriate,
> isn't extensible, and thus will never have transport characteristics.
>
> The only unknown is whether the implementation can support the use of
> chunks for backward-direction requests. This should not be a problem if the
> ULP does not have any DDP-eligible data items, and has a mechanism for
> indicating request size limits and it keeps the size of backward direction
> requests under the inline threshold.
>
> Note that Version Two's inline threshold is at least 4KB, and
> implementations are free to advertise an even larger threshold using the
> Receive Buffer Size characteristic. This size limit is actively advertised
> by the only ULP that uses backward-direction requests, NFSv4.x.
>
> The rpcrdma-bidirection specification purposely does not prohibit chunks
> in the backward direction. When a backward direction ULP is proposed with
> DDP-eligible data items, support for backward direction chunks will be
> introduced in RPC-over-RDMA implementations where that ULP is supported.
>
> Outside the scope of this review, let's propose some use cases that
> demonstrate how this characteristic can be used effectively, and then we
> can consider adjusting what is currently proposed.
>
>
> *** Section 4:
>
> Editorial:
>
> It might be useful to have a separate table which states what the
> receiver's expected response is (if I understand correctly):
>
> Sender:            response:
> INIT_XCHAR    INIT_XCHAR
> The connection's initial receive buffer size is typically 1KB, but it
> depends on the initial connection state of the RPC-over-RDMA version in
> use. See [rpcrmdav2] for details.
>
>
> *** Section 4.2:
>
> Editorial:
>
> The REQ_XCHAR message type allows an RPC-over-RDMA participant, whether
> client or server, to request of its partner >>,<< that relevant transport
> characteristics be changed.
>
> The UPD_XCHAR message type allows an RPC-over-RDMA participant >>,<< to
> notify the other participant that a change to the transport characteristics
> has occurred.
>
> The following paragraph:
>
> Note that there no way to tie a request changed to the specific request
> which asked for it.  In particular, the xid associated with this message is
> independent of that for an earlier REQ_XCHAR message.
>
> Seems more important than just a "Note". I suggest dropping "Note that,"
> and clarifying the phrase "there no way to tie a request changed".
>
>
> Technical:
>
> I suggest that it be made clear that "xid" in this document means the
> value in the RPC-over-RDMA header's rdma_xid field. In addition, the
> call-response relationships of the new operations proposed in this document
> are perhaps important enough (and unidirectional, not like normal RPC) to
> move discussion into a separate subsection of Section 4, or perhaps earlier
> in the document.
>
> optuxc_pendclr, if true, indicates that a previous request to update the
> characteristic specified by optuxc_now.xchv_which is no longer to be
> considered pending. This may be set true even if the characteristic value
> is not changed from the previous value.
>
>
> This seems to imply that, after receiving an UPD_XCHAR message, the
> receiver must query the sender to see if the change actually took effect.
> Did I misunderstand this? Would it be better if UPD_XCHAR reported whether
> a characteristic change took effect or was rejected? Under what
> circumstances would optucx_pendclr be set to false?
>
>
> *** Section 6.1:
>
> Editorial:
>
> The title of this section is "Additional Operations" but the two
> paragraphs here discuss only transport characteristics. Perhaps this
> subsection could be deleted?
>
> I suggest adding an explicit statement here that use of xcharid values in
> the experimental range is not guaranteed to interoperate among independent
> implementations.
>
>
> *** Section 7:
>
> Editorial:
>
> Like other fields that appear in each RPC-over-RDMA header, characteristic
> information is sent in the clear on the fabric with no integrity
> protection, making it vulnerable to man-in-the-middle attacks.
>
> For example, if a man-in-the-middle changes the value of the Receive
> buffer size or the Requester Remote Invalidation boolean, it can reduce
> connection performance or trigger loss of connection. Repeated connection
> loss can impact performance or even prevent a new connection from being
> established. Recourse is to deploy on a private network or use link-layer
> encryption.
>
> --
> Chuck Lever
>
>
>
> _______________________________________________
> nfsv4 mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/nfsv4
>

---

From: Chuck Lever <[email-masked]>
Date: Wed, 27 Jul 2016 14:21:24 -0400

> On Jul 22, 2016, at 5:04 AM, David Noveck <[email-masked]> wrote:
> 
> > This is a long-form full document review of draft-dnoveck-nfsv4-rpcrdma-xcharext-00. 
> 
> Thanks for the review.
> 
> > Please feel free to respond with clarifying questions or further discussion where needed, and no response is necessary 
> 	• If I'm not following a suggestion I will say why rather than simply ignoring it.
> 	• In cases in which I simply agree to all the suggestions in a given section, I will just say "OK" to that section.
> > Subsequent extensions might each define one or more new characteristics. Since the extension specified in this document 
> > is a (necessary) extensibility enabler, should we consider merging it into the base RPC-over-RDMA Version Two protocol?
> 
> 	• Those discussions
> 	• Production of a -01
> 	• Incorporation of xcharext into a new version-two document.
> For now, I'll make the changes I'm sure about into the existing xml.  We can decide what to 
> do later about things that might need further discussion.  I'll be keeping a to-do/to-decide list. 
> It may be that some of the more disruptive of those changes might be done as part of effecting

I'll wait until you feel it is ready.

Someone might point out that the sooner the better, just to
have the most diff history attached to the particular document
that is going to be moved forward. I don't have a strong
preference.

> > *** Abstract
> 
> > Editorial:
> 
> 
> > NEW
> > This document specifies an extension to RPC-over-RDMA Version Two and newer.
> 
> I have problem with the phrase "and newer".  I'm not sure what you are assuming Version Three will be like such that it is sufficiently different from Version Two to require a new version number and still need this extension.   Also, if this extension is an integral part of version Two, then it will remain in subsequent versions until someone makes an explicit decision to take it out.
> 
> > *** Section 1.1:
> 
> > NEW
> > This document specifies an extension to RPC-over-RDMA Version Two and newer. 
> 
> > The term "char" has a longstanding and well known meaning to C coders. I found the overloaded use of "char" as an abbreviation of "characteristic" to be confusing. 
> 
> I can see that might be a problem, but I don't think I use the unadorned abbreviation "char'.  It is always part of the string "_XCHAR".  I can change this and probably will.
> 
> > I find "attribute" or "capability" to be more in line with the intended usage and either of these seems to be preferred historically over "characteristic". 
> > There are plenty of examples, including within NFS itself, of the use of the term "attribute."
> 
> At the time, I was worried that the use of the same term might be confusing.  "capability" seems like it is limited to binary choices.
> 
> I'm willing to change this but note that it would change the title and the filename  document.  As a result i am not making this change now.
> 
> Also, note that if we change to "attribute", "XCHAR" would become "XATTR" :-)
> 
> If we add that (or "rpcrdma2" or "rpcrpdma2plus") to everything then the names start getting very long for np very goo reaspn, and I prefer shorter names.
> 
> > The variable, type, and operation names used in this document are often not particularly mnemonic. 
> > I suggest (somewhat) longer names to help the reader remember function. Even an additional vowel or two might be helpful.
> 
> Adding a few characters to make it more mnemonic is OK with me, I have problems if that is carried to an extreme.
> 
> Although I think we can come to agreement about this issue, I think it makes most sense to address this as part the merger of the two documents.  Are you OK with that?

That's fine.

> > When first reading this section, it was not clear to me what the xcharsubset type is for. To provide some context for this type definition, it would help to add a 

[ ... snipped ... ]

> > *** Section 3.2:
> 
> > Editorial:
> 
> > Rather than the term "remote invalidation requests," I recommend the more specific term "RDMA Send With Invalidate". Here and elsewhere in the document, > the term "invalidate" is preferred over "deregister". (The term is "Remote Invalidation" not "Remote Deregistration," after all).
> 
> OK.
> 
> Technical:
> 
> > The mention of the server's lack of use of memory registration is not technically correct. 
> 
> Will address.
> 
> > Finally, although there are no DDP-eligible data items in currently defined backward-direction ULPs, an implementation can still choose to use Position-Zero Read chunks or 
> > I'm not sure what exactly to do about backward-direction use of Remote Invalidation, other than to have the responder also provide this characteristic via INIT_XCHAR, but
> What I feel is consistent with that but I'd distinguish between what the spec  says to do and what we expect to implementations to do in the near future. 
> 
> The spec should state that this chracteristic applies to both directions of operation.
> 
> You and I know that server implementations, for the forseeable future are either going to not report this characteristic or report it as false. also clients are unlikely to look at it for a while.  The document may provide this information as interesting background but that doesn't affect the protocol, which allows that situation to change. 
> 
> > possibly stating that this characteristic is for forward-direction requests only (ie, punting).
> 
> I don't think punting is appropriate.  This is a second-down-and-inches-to-go situation.
> 
> > The requester remote invalidation characteristic indicates that the requester is prepared for the responder to issue remote invalidation requests, in order to unregister memory > regions established to support RDMA Read and Write operations done by the responder into or out of the requester’s memory.
> >
> > As RPC-over-RDMA is currently used, memory registration is not done on the server and explicit RDMA operations are not done to satisfy backward-direction requests. This 
> > makes it unlikely that servers will present non-default values of the XCHAR_RQREMINV characteristic or that clients will take note of the value presented by servers.
> 
> > NEW
> > The Requester Remote Invalidation characteristic indicates that a forward-direction requester is prepared for a forward-direction responder to use RDMA Send With Invalidate 
> > when replying to an RPC-over-RDMA request containing non-empty chunk lists.
> 
> I don't think we want to foreclose the option of using this in the reverse direction.
> 
> PROPOSED:
> The Requester Remote Invalidation characteristic indicates that the current endpoint, when in the role of a requester, is prepared for the responder to use RDMA Send With Invalidate when replying to an RPC-over-RDMA request containing non-empty chuck lists.
> 
> As RPC-over-RDMA is currently used, memory registration exposed to peers are not done by the server and explicit RDMA operations are not done to satisfy backward direction requests.  This makes it unlikely that servers will present non-default values of the XCHAR_RQREQ characteristic or that clients will take note of such values when presented by servers.

The first paragraph is OK. The second seems unnecessary to me.

> > When the Requester Remote Invalidate characteristic is set to false, a responder MUST use Send to convey 
> > RPC reply messages to the requester. When the Requester Remote Invalidate characteristic is set to true, a 
> > responder MAY use Send With Invalidate instead of Send to convey RPC replies to the requester.
> 

I'll wait for you to bring this up on the mailing list.

> I also note that if there is a change in the chunk XDR, I will need to update the XDR In draft-dnoveck-nfsv4-rpcrdma-rtrext to match. 
> 
> 
> > *** Section 3.3:
> > 
> > Technical:
> > 
> > RPC-over-RDMA Version Two and newer implementations are required to have support for backward-direction requests. 
> 
> I didn't know that.  Now that you mention it, I'm not sure it is such a good idea.  For example, I don't think that you want to 
> prevent an NFSv3 implementation, which has no need to use backward direction requests, from upgrading from V1 to V2.

Fair point. Also, I may have overstated the requirement. I'll
make a note to revisit that language.

> That
> issue can be discussed further. For now, I'll keep this on the to-do/to-decide list.

> > NONE might be appropriate, isn't extensible, and thus will never have transport characteristics.
> 
> Depends on the decision on whether backward direction support is required in V2.
> 
> > Note that Version Two's inline threshold is at least 4KB, and implementations are free to advertise an even larger threshold using the Receive Buffer Size characteristic. 
> 
> > This size limit is actively advertised by the only ULP that uses backward-direction requests, NFSv4.x.
> 
> 
> > The rpcrdma-bidirection specification purposely does not prohibit chunks in the backward direction. 
> 
> Exactly.  In version Two, this is optional behavior.

It is also an option in Version One.

> The characteristic in question provides a means to determine whether the peer supports transfer of such chunks.

That's not at all clear from the description and discussion
in the draft. But, OK, I'll go with that.

> > When a backward direction ULP is proposed with DDP-eligible data items, support for backward direction chunks will be introduced 
> Yes, but my hope is that we will be in a situation in which the implementation support can be introduced but no protocol change is required.  If we don't provide the support before it is needed, it is likely that it will be needed before it is available, which would be a real drag.

So then:

UNKNOWN - The upper layer handles reporting support levels.

NONE - No backwards-direction support at all.

SZLIM - Implementation does not support chunks.

GENL - implementation supports backward-direction chunks

I don't understand why UNKNOWN is necessary. The transport
layer either has the support or it doesn't, and it can report
that.

What should be reported when, say, NFSv4.0 and NFSv4.1 are
operating on the same connection? How does the transport
decide whether it should report UNKNOWN or SZLIM ?

I suggest that SZLIM and GENL might be more functionally named.
For example, INLINE and CHUNKS might be better. GENL is a bit
broad.

Instead of a discriminator, a bitmap might be more extensible.
That would allow an implementation that supported only
RDMA_NOMSG in the backward direction.

> It has to be understood that any such use case will necessarily be hypothetical, in that they have to assume that there will be a ULP that wants this support, that Version Two, has, so far, been careful not to prevent.

Still struggling to understand why this one needs to be an
initially defined characteristic. We don't have any ULP that
needs this functionality. The other two characteristics have
strong justifications.

Even if this characteristic might become necessary sometime
down the line, RPC-over-RDMA Version Two is extensible:
it can be introduced at any time. It seems like there ought to
be a real use case or two to understand how it needs to work.

If we look at TCP implementations:

- there's no way at the transport level for peer implementations
to report whether they support backward-direction operation at
all.

- clients that connect via NFSv3 quite likely use an RPC layer
that does support backward-direction operation. It is completely
interoperable with an implementation that has no backward-
direction support.

There is no danger of sending a backward direction request to
a client that doesn't support backward direction operation. The
request will just be silently discarded.

Without chunk support, the behavior is supposed to be similar.

Thus, what is the purpose of reporting a backward-direction
support level? The Upper Layer can report either "no backward
direction support" or can report the largest size backward-
direction operation it can receive, already; and it's more or
less required to do that when operating on other transports.

> I'm OK with this but am going to hold off a bit since a merger with Version Two may supersede this.  This is now on my to-decide list.
> 
> After all, if this is merged fully into Version Two, we might have RDMA_INIT_XCHAR, etc.

> Not sure a table is useful.  Nevertheless the discussion of what would be in it would be helpful.
> 
> > Sender:            response:
> > INIT_XCHAR    INIT_XCHAR
> 
> RESP_XCHAR is the response to REQ_XCHAR.  In addition, one or more UPD_XCHARs might be generate but these are 
> asynchronous and not readily connected with the REQ_XCHAR That caused them to be generated.
> 
> > RESP_XCHAR none
> > UPD_XCHAR   none
> 
> Yes.

Agreed, a table oversimplifies it. But the discussion here seems
useful to include.

> > Note that there no way to tie a request changed to the specfic request which asked for it.  In particular, the xid associated with this message is independent of that for an earlier REQ_XCHAR message.
> 
> another possibility is to replace the verb "note" by its definition, resulting in:
> 

The phrase I mentioned is not grammatical. Can it be cleaned up?

> "NOTIFY" seems too generic to me.  How about "ROPT_XCHAR_CHANGED"?  I supposed that this would be batched with other similar name changes.
> 
> > I suggest that it be made clear that "xid" in this document means the value in the RPC-over-RDMA header's rdma_xid field. 
> 
> I've done that.
> 
> > In addition, the call-response relationships of the new operations proposed in this document are perhaps important enough 
> > (and unidirectional, not like normal RPC) to move discussion into a separate subsection of Section 4, or perhaps earlier in the document.
> 
> I will do that.
> 
> > optuxc_pendclr, if true, indicates that a previous request to update the characteristic specified by optuxc_now.xchv_which 
> > is no longer to be considered pending. This may be set true even if the characteristic value is not changed from the previous value.
> 
> > This seems to imply that, after receiving an UPD_XCHAR message, the receiver must query the sender to see if the change actually took effect. Did I misunderstand this? 
> 
> It doesn't.  The receiver gets the new current value of the characteristic in any case.
> 
> > Would it be better if UPD_XCHAR reported whether a characteristic change took effect or was rejected? 
> 
> 	• change not requested.  i.e. the sender is notifying you of a change you didn't request.

The usual term is "unsolicited."

> 	• you requested a change but the request is still pending. this could happen if you requested that something be changed and instead of waiting untuil the change was effected, the sender decides to update you on partial completion, while continuing to keep the original request pending.

I'm still not clear on what "partial completion" means. Can you
give an example?

--
Chuck Lever

---

From: David Noveck <[email-masked]>
Date: Sat, 30 Jul 2016 17:23:38 -0400

>
> > > The characteristic in question provides a means to determine whether
> the peer supports transfer of such chunks.
>

Will clarify the language in -02.

> layer either has the support or it doesn't, and it can report

Good point.  If this characteristic is still present in -02, UNKNOWN will
be deleted.

> decide whether it should report UNKNOWN or SZLIM ?

I think you've answered your own question above: "The transport layer
either has the support or it doesn't, and it can report that".

> For example, INLINE and CHUNKS might be better.

I agree that INLINE is better.  Given the possibility of message
continuation, it is no longer the case that we are limited, in this case,
by the receive buffer limit.

As far as "CHUNKS", I may be getting ahead of myself here, but, with the
rtrext stuff, you can do RDMA without CHUNKS and DDP without RDMA.  I'd
like not to pick something that may not continue to mean the same thing, as
the protocol evolves.

> GENL is a bit broad.

It is intended to be broad.  The idea is that it supports all the
functionality normally part of the forward direction with no special
backward-direction restrictions.

> RDMA_NOMSG in the backward direction.

Should we?  Is it valid for an implementation only to support RDMA_NOMSG in
the forward direction?  If it were, we could have a receive support
characteristic defining the header types one was prepared to receive that
could apply to either backward or forward transfers.

> Still struggling to understand why this one needs to be an
initially defined characteristic.

When I drafted this initially, I didn't set a high bar.  I included it
because it was useful, without requiring that it be necessary.

> We don't have any ULP that needs this functionality.

True.

> The other two characteristics have strong justifications.

> it can be introduced at any time.

The problem is the latency to do so.

> It seems like there ought to

Depends what you mean by a "real" use case.   In my view, any actual
proposal, even if not detailed in a formal document should be reason enough
to work on including this.  Requiring an actual working group document to
start on this could add to latency.  Even worse would the "or two" case, in
which those who  might want this support could be dissuaded by the need to
build support for this extension.

Although this may not be necessary right now, it is optional and would not
add any complexity to implementations not needing it.

As I understand things, the goal of Version Two was to be symmetric with
regard to direction of operation.   As things turned out, it was not
symmetric in regard to use of chunks/rdma/ddp in the backward direction
because of a need to accommodate version one implementations that did  not
support these functions in the backward directions.

Given that we have decided to make this functionality optional in the
backward direction, I think makes sense to provide an easy way to find out
if one's peer provides the support that would be required.

-- stuff on TCP implementations deleted --

> support level?

I think it is to allow your peer to know, before asking you to do something
that you are not required to support, whether you in fact support a feature
which is OPTIONAL.

> The Upper Layer can report either "no backward

It isn't just operation size.  If there will be ULP extensions (more likely
than entirely new ULPs), they are likely to need DDP support, so that, when
used on a transport without DDP, they might not be usable at all.

> Agreed, a table oversimplifies it. But the discussion here
> seems useful to include.

I think that's been done in -01.

> The phrase I mentioned is not grammatical. Can it be cleaned up?

I think that's been done in -01.

> The usual term is "unsolicited."

I didn't use that term in -01.  Will do in -02.

> >       • you requested a change but the request is still pending. this
could happen if you requested that something be changed and instead of
waiting untuil the change was effected, the sender decides to update you on
partial completion, while continuing to keep the original request pending.

give an example?

Suppose you ask your peer to provide 64K receive buffers.

You could get the following response with pendclr set:

   - 4k --> rejection.
   - 8K --> He's given you the biggest he is prepared to.  Your request is
   partially satisfied.
   - 64K --> request satisfied.

On the other hand if you get 8K without pendclr, you can now assume 8K
receive buffers but you don't know the eventual disposition of your
request.  You might subsequently get a request with pendclr set and it
might tell you 8K was the best the peer could do, that 64K was done,
or that 16K was the best it could do.

Implementations would do well to treat message without pendclr as if they
were unsolicited even if, as in these cases, they were prompted by his
request.  The important thing is that the eventual disposition of the
request is not yet known.

I'll add a more complete explanation of the uses of UPD_XCHAR to the -02.