Re: AD review of draft-ietf-nfsv4-rpcrdma-cm-pvt-data

> On Nov 4, 2019, at 11:16 AM, Tom Talpey <[email protected]> wrote:
> 
> Magnus, one quick response on your first question:
>  
> Private Data is supported by multiple RDMA protocols, including iWARP aka RDDP. The document takes care to define a payload which is supported by all.
>  
> The Infiniband-style private data exchange, used by RoCE and InfiniBand, is implemented in the IB Connection Manager, and is out of scope for IETF. However the iWARP one is supported by MPA in RFC5044 and RFC6581. But this MPA support isn’t mentioned until the Security Considerations section, so I’d agree that adding discussion of all these mappings earlier in the document is a good idea.

Thanks, Tom. My quick search earlier today did not turn up these two RFC numbers. I can include these two and a reference to the iSER RFC, 7145.

I'll also do a closer comparison of the text Magnus called out with the two paragraphs below from RFC 6581.


> RFC6581 has a similar approach btw. It adds private data payloads which are exchanged by the lower RDMA layers. And it provides some possibly useful text:
>  
>    As currently defined, DDP connection establishment requires the ULP
>    to encode the RDMA configuration in the application-specific Private
>    Data.  This results in undesirable duplication of logic to cover RDMA
>    characteristics of both InfiniBand and RDDP for each ULP, and to
>    specify for InfiniBand and RDDP the extraction of the RDMA
>    characteristics for each ULP.
>  
>    Both RDDP and InfiniBand support an initial Private Data exchange;
>    therefore, a standard definition of the RDMA characteristics within
>    the Private Data section would enable common connection establishment
>    APIs to format the RDMA characteristics based on the same API
>    information used when establishing either protocol to form the
>    connection.  The application would then only have to indicate that it
>    was using this standard format to enable common connection
>    establishment procedures to apply common code to properly parse these
>    fields and configure the RDMA endpoints accordingly.  Exchange of
>    parameters necessary to perform RDMA Read operations is a common
>    usage of the initial Private Data exchange.
>  
> In 6581, the text above provides justification (“undesirable duplication of logic”). But for this document, the motivation is to enhance the connection model without extending the rpcrdmav1 protocol. Similar goal, but may lead to subtly different text.
>  
> Tom.
>  
>  
> From: nfsv4 <[email protected]> On Behalf Of Magnus Westerlund
> Sent: Monday, November 4, 2019 9:57 AM
> To: [email protected]; [email protected]
> Subject: [nfsv4] AD review of draft-ietf-nfsv4-rpcrdma-cm-pvt-data
>  
> Hi,
>  
> I have started my AD review. It was such a short document that I thought I read through it to figure out what more I need to read. So I have not yet read like RFC 8166. So from a context of having little context understanding the below are my comments and questions. I think it may be faster if you help answering them then I read a lot of documents and still have question marks.
>  
>  
> So this private data field is only existing in Infinite Band Connection manger? So what this document is defining a structure to a field which currently don’t have any structure but are part of a protocol defined by another body? 
> Have this work been formally agreed with IBTA?
> Or is the relationship to the connection manager used to establish the RPC over RDMA different, if that is the case it needs to be better explained.
> Is it bit 8 or bit 15 of the flags field that are used. Section 5.1 and 4.1 do not agree. The later thinks it is bit 8.
> Section 5: Is there only going to be one object of private data forever in the relevant communication? If not, why isn’t this a well-specified TLV so that unknow type entries can be skipped to the nest object? 
> Even if there ever is going to be one entry, can you be more formal in description of what is a valid message using another format identifier, is that only the 32-bit field of the format Identifier, or also the version. 
> Isn’t the version field just unnecessary? If one needs version using another format identifier is more easy than the identifier + version construct.
> Section 6: The IANA consideration is confusing. If the specification for a CM private data format requires IESG approval, then why have the expert review policy. In that case one can simply use the IESG approval policy instead. However, requiring any type of IESG approval here appear to be a way to high bar. I think expert review is likely appropriate, but the guidance to the Experts needs to be clearer. For example what happens if IBTA want to use this to add some format? 
> Section 7: The security consideration. I am missing any discussion about the security requirements that the actual extension. It appears that integrity and source authenticity are required for safe operation of these two extensions. 
> IBARCH URL Is not valid, it results in a 404.
>  
> Cheers
>  
> Magnus Westerlund
> _______________________________________________
> nfsv4 mailing list
> [email protected]
> https://www.ietf.org/mailman/listinfo/nfsv4

--
Chuck Lever
[email protected]

_______________________________________________
nfsv4 mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/nfsv4