Re: [nfsv4] Review draft-ietf-nfsv4-rpcrdma-cm-pvt-data-00

[Chuck Lever <[e-mail]>]

> On Oct 22, 2018, at 10:16 AM, David Noveck <[e-mail]> wrote:
> 
> General Comments
> 
> Overall evaluation
> 
> In terms of technical content this document is pretty much there.   It has the information necessary to implement this convention, and there exist implementations of it.
> 
> However there are some editorial matters that need to be attended to before the document will be ready to go forward, although none should pose any difficulty for the group in meeting the June 2019 target date for final submission.   Many of these are quite minor and easily attended to but there is a more siginificant editorial issue discussed below in Nature of this Document. 
> 
> Nature of this Document
> 
> Although this document is descrIbed as informational, it is written pretty much as a standard-track document addressing the same topic would be.   At one level, I have no problem with this and have always believed that such treatment of the topic was appropriate.  However, after working group discussion, a decision was reached that this document would be published as Informational.  While I have no objection to the working group revisiting that decision, I don't see any point in doing so.   I will provide suggestions below as to how the text of this document could be adjusted to be consistent with the desision to make it informational.  
> 
> Comments by Section
> 
> Abstract
> 
> Suggest replacing the last sentence by "The data format is designed to allow later extension".
> 3.1.  Inline Threshold Size.
> In the last paragraph suggest replacing "can agree" by "could agree".
> 4.  Private Data Message Format
> I don't understand the use of the word "SHOULD" in the last sentence of the first paragraph.   I'm not clear who is being told they "SHOULD" do something or exactly what they are being told to do.   Since use of this format is to be optional, it cannot be directed to RPC-over-RDMA implementers.   It appears that the target of the "SHOULD" is CM implementers, but this document could never be normative with regard to CM implementers.   Unless I have misunderstood the intent, suggest simply deleting the word "SHOULD".
> 4.1.  Fixed Mandatory Fields
> I don't see the point of the term "Mandatory' in the section title.  It is confusing since this entire feature is optional.
> The use of the term "MUST" in the first sentence is problematic for a number of reasons:
> 	• As this is an informational document, it is isn't really clear why it is using these normative terms.
> 	• Since this extension is optional, "MUST" isn't really appropriate since one can choose not to use this extension and some existing impemenations do not.
> 	• It basically invalidates the material in Section 5, which establishes a registry of valid formats.   If the "MUST" stands there would only ever be one valid fornat, and the registry would consist of one valid format and some number of invalid formats.
> Suggest replacing "MUST" by "is to" 
> Suggest replacing "Protocol number" by "Format Identifier".
> In the second sentence of the paragraph following "Protocol number" suggest replacing "MUST" by "is always". 
> In the second sentence of the paragraph following "Version", suggest replacing "defined by "specified".
> In  the paragraph following "Send size", the use of the phrase "will transmit" is confusing.   The size of messages an implementation will transmit is constrained by the peers inline buffer size, which, at the point this data is sent, has not been decided yet.  suggest replcing by "is prepared to transmit.
> Similary , in the paragraph following "Receive size", suggest replacing "can receive" by "is prepared to receive".
> 4.1.1.  Interoperability Considerations
> The first sentence use the phrase "are intended to interoperate"  which does accurately describe the intention, but I think it is more important to clearly state how that intention can be realized, i.e. by following the practices in the rest of the section.
> Suggest replacing this seentence by the following which will become the first paragraph of the section with the rest of the material in the first paragraph becoming the second paragraph:
> The extension described in this document is designed to allow implementations of it interoperate fully with RPC-over-RDMA version 1 implementations that do not participate in the exchange of CM Private Data.  Realizing this goal requires that implementations follow the practices described below in the rest of this section..
> After that, suggest replacing the two instances of "MUST" in this section by "needs to".
> 4.1.2.  Extending the Message Format
> The fact that this an informatinal document poses special difficulties for the approach that you take this section.   While a description of an existing protocol in an informational document is not supposed to tell implementers what to do, it can get around this by describing the existing protocol with the implication that thosewho need to inter-operate with existing implementation have to do the same.   In the case of extensions, you cannot constrain future extensions except to point to the potential interperation issues with the existing protocol descrbed in this document.
> Suggest renaming this section Possible extensions to the Message Format and replacin it by something like the following:
> Although the message format described in this document addresses the issues cited in SEction by provided the ability for the client and server to exchange infoormation about the transport properties decscribed in section 3, it is possible that there will be a need to exchange additional properties to communicate, making it necessary to exten or otherwuise modify the format described in this document.
> Any such modification will face the problem of interoperating priperly with implementations of RPC-over-RDMA version 1 that are unaware of this existence of this new formmat.   These include implementations that that do not recognize theexchange of CM Private Data as well as those that only recognize the format described in this document.
> Given the structure of the dat a firmat descrbed in this document, these interoperability constraints could be met by the following sorts of new message formats:
> 	• A format which uses a different value for the first four bytes of the format, as provided for  in the registry described in Section 5.
> 	•  A format which uses the same value for the Format Identifier field and a value other than one in the Vesion field.  Although it is possible to re-organize the last three of the eight bytes in the existing format, extended format are ubnlikely to do so, so that new formats would take the form of extensions of the format described in tis document, with added fields stating at byte eight ot the format and/or the defnition of previousluyy reserved flags.
> 4.1.3.  Feature Support Flags
> In the second sentence of the first paragraph, the clause "the bits in the Flags field have the following meaning" has the the problem 
> that "bits" is 'plural while "meaning" is singular.   Suggest replacing this clause by "the bits in the flags field are to be set as follows".
> In the last sentence of the section suggest replacing 'MUST be zero' by "always are zero".
> 4.1.4.  Inline Threshold Value
> The existing second paragraph is confusing because:
> 	• When the terms "requester" and "responder" are used it is not clear who is bein spoken of.  Unlike the roles of client and server which are fixed, the roles of requeser and responder change from message to messafge since bi-directional operation is to be supported.
> 	• The use of the phrases "requester-to-responderinline threshold" and "respnder-to-requester inline threshold" might lead reders to suppose that a client or server, each of which can have both roles, could have two inline thresholds when in fact it can only have one.
> Suggest the following replacement:
> The client and the server each set its ilnline threshold to the smaller of its own send size and its peer's reported receive size.  
> Thus the client's inline threshols is to be set to the smaller of the client's send size and the server's reported receive size while
> the server's inline threshold is to be set to the smaller of the server's send size and the client's reported receve size. 

Thanks, Dave.

The Internet Draft submission window closes today, for IETF 103. I plan
to submit a new revision of integrity-measurement and cm-pvt-data when
the window opens again in two weeks.


--
Chuck Lever