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

Hi Magnus, thanks for your review and moving forward with the AD write-up.

> On Nov 4, 2019, at 9:56 AM, Magnus Westerlund <[email protected]> wrote:
> 
> 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. 
>  
>  
> 	1. 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? 
> 		a. Have this work been formally agreed with IBTA? 
> 		b. 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. 

The private data field is for use by Upper Layer Protocols. It's existence
is defined by the IBTA, but not its content. It's opaque to the CM, hence
the name "private".

Both iSER and iWARP itself (both IETF-defined) can place data in this field. That's why there
is a 32-bit format ID field: if that doesn't match the constant defined in
this document, that means some other ULP is using the private data field,
and its contents are then ignored by RPC-over-RDMA.


> 	2. 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 4.1 should say bit 15, thanks.


> 	3. 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?

As I understand it, the private data field is used by one ULP at a time.
I'm not aware of a TLV. There are already existing ULP implementations
that do not assume a TLV, so that might be a tough sell.


> 	4. 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.

I can cite the use of this field by other protocols, if that would help.


> 	5. Isn’t the version field just unnecessary? If one needs version using another format identifier is more easy than the identifier + version construct.

It's possibly superfluous. But:

- the two fields serve different purposes, even though they could be combined.
- there's already an implementation that uses both fields.


> 	6. 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?

I would be OK with Expert Review only, but:

1. I will need suggestions about expert guidance. Will look at other RFCs.
2. IMO WG consensus is needed on the replacement text of this section.


> 	7. 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.

Integrity is a property of RDMA Reliable Connection transports.

Not clear about the source authenticity requirement. Can you elaborate?


> 	8. IBARCH URL Is not valid, it results in a 404.

Well, that's unfortunate. That's the same URL that's cited in RFC 8166.

Could use this one instead: <http://www.infinibandta.org/specs>.



--
Chuck Lever



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