Re: [storm] WG Last Call - iSER - through December 12 - comments

["Hemal Shah" <[e-mail]>]

Thanks David! I agree with your points #1 and #2 below.

Hemal

From: [e-mail] [mailto:[e-mail]]
Sent: Wednesday, December 21, 2011 2:18 PM
To: Hemal Shah; [e-mail]
Subject: RE: WG Last Call - iSER - through December 12 - comments

Hemal and Mike,

This looks like progress.  The text changes mostly look ok, but I have a couple of comments:

(1) Please always introduce InfiniBand as an example - prefixing "e.g.," or "for example" usually suffices.  I'd like to avoid having an IB spec as a normative reference.  This also affects some of Mike's existing text.  As part of this, I'd rewrite the last two sentences of Hemal's new text for item 11 as:

But, for an iSER-assisted iSCSI connection over some RCaP implementations (e.g., InfiniBand), there is no underlying TCP connection. For the specific example of an InfiniBand [IB] based RCaP implementation, an iSER-assisted iSCSI connection is directly mapped onto InfiniBand RC channel.

(2) It looks like the new iWARP-specific text to restore the requirements for SendSE and SendInvSE requirements is the major work item in this.


Thanks,
--David

From: Hemal Shah [mailto:[e-mail]]
Sent: Wednesday, December 21, 2011 1:57 PM
To: Black, David; [e-mail]
Subject: RE: WG Last Call - iSER - through December 12 - comments

David,

See my reply below!

Hemal

From: [e-mail] [mailto:[e-mail]]
Sent: Wednesday, December 14, 2011 6:44 AM
To: Hemal Shah; [e-mail]
Subject: RE: WG Last Call - iSER - through December 12 - comments

Hemal,

Thank you for the detailed review.

The minor comments appear to all be editorial, so I'll leave them to Mike to resolve.

On the major comments:

Items 11 and 12 are the same issue, as I also found item 11, and the text quoted in item 12 was supposed to resolve it.  Obviously, that text is not sufficiently clear ;-), so please work with Mike to come up with a clearer paragraph.
[Hemal]
For Item 11, I suggest the addition of the following text to the definition of iSER-assisted iSCSI connection:

An iSCSI connection that is not iSER-assisted always maps on to a TCP connection at the transport level. But, an iSER-assisted iSCSI connection may not have an underlying TCP connection. For an iWARP based RCaP implementation, an iSER-assisted iSCSI connection has an underlying TCP connection. But, for an iSER-assisted iSCSI connection over an InfiniBand [IB] based RCaP implementation, there is no underlying TCP connection. For an InfiniBand [IB] based RCaP implementation, an iSER-assisted iSCSI connection is directly mapped onto InfiniBand RC channel.

[Hemal] For Item 12, I suggest the following changes to Section 4.1.

OLD
"For a transport layer that provides message delivery capability such as [IB], the RCaP implementation supports the use of the messaging capability by the iSCSI Layer directly for the Login phase after connection establishment before enabling iSER-assisted mode."

NEW
"For a transport layer that provides message delivery capability such as [IB], the RCaP implementation supports the use of the messaging capability by the iSCSI Layer directly for the Login phase after connection establishment before enabling iSER-assisted mode. For an InfiniBand [IB] based RCaP implementation, the iSCSI Layer uses IB messages to transfer iSCSI PDUs for the Login phase after connection establishment before enabling iSER-assisted mode."

Item 13 looks like a useful clarification - the non-TCP connection may have to be torn down in order to establish a TCP connection, and IB is an example where there is necessary.
[Hemal] I suggest the following modification to the text in Section 5.1.

OLD
"If the RDMAExtensions key is not negotiated to Yes, then for some RCaP implementation (such as [IB]), the connection may need to be re-established in TCP capable mode.  (For InfiniBand this will require an [IPoIB] type connection.) "

NEW
"If the RDMAExtensions key is not negotiated to Yes, then for some RCaP implementation (such as [IB]), the existing connection may need to be torn down and a new connection may need to be established in TCP capable mode.  (For InfiniBand, this will require an [IPoIB] type connection.) "


Items 14 and 15 represent a deliberate change because the implementations did something different from what's in RFC 5046, as noted in Appendix A (Section 14).

   8. Added two 64-bit fields in iSER header in section 9.2 for the Read
     Base Offset and the Write Base Offset to accommodate a non-zero
     Base Offset.  This allows one implementation such as the OFED
     stack to be used in both the Infiniband and the iWARP environment.
     Changes were made in the definition of Base Offset, Advertisement,
     and Tagged Buffer.  Changes were also made in sections 2.4.1, 2.5,
     2.6, 7.3.1, 7.3.3, 7.3.5, 7.3.6, 9.1, 9.3, 9.4, 9.5.1, and 9.5.2.
     This change is not backward compatible with RFC 5046.

I would suggest changing the last sentence to:

     This change is not backward compatible with RFC 5046, but is part
     of all known implementations of iSER at the time this document was
     developed.

If you know of an implementation that did *NOT* do this, please speak up now!
[Hemal] I like your suggested text above.

Item 16 - I would prefer not to add the iSER InfiniBand message formats to this draft, but instead leave them to the IBTA to document.  OTOH, if there's no good documentation of these for InfiniBand, they could be added.  What's the state of IB documentation for iSER?
[Hemal] I'm fine with it. I suggest that we add a reference to look at InfiniBand documents for iSER InfiniBand message formats. Mike should be able to answer your question.

Item 17 - It looks like this is covered by item 9 in Annex A:

   9. Remove iWARP specific behavior.  Changes were made in the
     definition section on RDMA Operation and Send Message Type.
     Changes affecting Send with Invalidate were made in sections
     2.4.1, 2.5, 2.6, 4.1, and 7.3.2.  Changes affecting Terminate were
     made in sections 10.1.2.1 and 10.1.2.2.  Changes were made in
     section 15 to remove iWARP headers.

The notion of Solicited Event (SE) is iWARP-specific; the generalization above seems useful.  OTOH, RFC 5046 contained requirements for use of SendSE and SendInvSE, and those requirements appear to have been removed.  I believe that they need to be restored, possibly in a new section that is specifically about iSER over iWARP.
[Hemal] I agree that these requirements need to be restored and added as a new section.

Thanks,
--David

From: Hemal Shah [mailto:[e-mail]]
Sent: Monday, December 12, 2011 8:06 PM
To: Black, David; [e-mail]
Subject: RE: WG Last Call - iSER - through December 12 - comments

David and Mike,

Below are my comments on the iSER draft.

Minor


 1.  Section 1.1: iSCSI data-type PDU definition should clearly say at what layer the data transfer is happening. For example, the following sentence "An iSCSI data-type PDU is defined as an iSCSI PDU that causes data transfer, transparent to the remote iSCSI Layer, to take place between the peer iSCSI nodes on a full feature phase iSCSI connection." should be rephrased (added text in red) as "An iSCSI data-type PDU is defined as an iSCSI PDU that causes data transfer via RDMA operations at the iSER layer, transparent to the remote iSCSI Layer, to take place between the peer iSCSI nodes on a full feature phase iSCSI connection. I believe that the definition is already being revised based on David's comments.
 2.  Section 1.1: The definition of iSCSI/iSER session needs to have further description. The current description is insufficient. Suggest to add the following to the definition "All connections of an iSCSI/iSER session are iSCSI/iSER connections."
 3.  SNIC term is not defined but used in this draft. This needs to be addressed.
 4.  Sections 2.4.3 and 2.4.4 use terms "sending side", "receiving side", "remote node", etc. It is confusing to use multiple terms (some of them are not defined in the draft) in these two sections. Suggest to use "Local Peer/Remote Peer" or "Data Source/Data Sink" terms consistently in these two sections.
 5.  The use of term input qualifiers in Section 3 is misleading as we are actually referring to the input parameters for the operational primitives. Suggest to replace term "input qualifier" with "input parameter" in Section 3.
 6.  Section 5.1.3: In the following sentence the second "iSER-ORD" should be replaced with "ORD". "Upon receiving the iSER Hello Message, the iSER Layer at the target MUST set the iSER-ORD value to the minimum of the iSER-ORD value at the target and the iSER-IRD value declared by the initiator."
 7.  Section 9.2: Do WSV and RSV flags indicate the validity of WBO and RBO fields? The description of these flags should state it.
 8.  Section 15 should be titled as "iWARP Message Formats for iSER".
 9.  Sections 15.3-15.5 should be formatted as iWARP messages.
 10. I do not understand the reason behind removing the iWARP Message Format for RDMA Read Request, iWARP Message Format for Solicited SCSI Write Data, and iWARP Message Format for SCSI Response PDU in Section 15. They should be kept in Section 15.

Major

 1.  According to the iSCSI spec, an iSCSI connection has an underlying TCP connection. Based on that, an iSER-assisted iSCSI connection requires an underlying TCP connection to be present. For iWARP, an iSER-assisted iSCSI connection has an underlying TCP connection. For iSER-assisted iSCSI connection over InfiniBand transports, there is no underlying TCP connection. In this case, what does an iSER-assisted iSCSI connection mean? What IB transport service types are used for the iSER-assisted iSCSI connection? The draft is not clear on what an iSER-assisted iSCSI connection mean for the InfiniBand transports. The draft needs to clearly define iSER-assisted iSCSI connection for InfiniBand transports and its mapping over underlying InfiniBand transport connections and transport service types.
 2.  Section 4.1: "For a transport layer that provides message delivery capability such as [IB], the RCaP implementation supports the use of the messaging capability by the iSCSI Layer directly for the Login phase after connection establishment before enabling iSER-assisted mode." What is the meaning of this sentence? What message delivery capability is being referred here? Does this sentence infer that for IB transports, IB messages are allowed prior to enabling the iSER-assisted mode? The spec is not clear on how the iSCSI PDUs are transferred over IB connections prior to enabling iSER-assisted mode. Are iSCSI PDUs transferred over iSCSI/TCP/IPoIB connection or iSCSI/IB Reliable Connection prior to enabling iSER-assisted mode (I think it is the later)?
 3.  Section 5.1 has the following text "If the RDMAExtensions key is not negotiated to Yes, then for some RCaP implementation (such as [IB]), the connection may need to be re-established in TCP capable mode.  (For InfiniBand this will require an [IPoIB] type connection.) " What is the implication of re-establishing connection in TCP capable mode for InfiniBand? Does it require tearing down the exisiting iSCSI/IB connection, establishing a new TCP/IPoIB connection, and performing iSCSI login without RDMAExtensions key on that TCP connection? The spec does not describe this behavior clearly.
 4.  Section 9.2: iSER header and iSER messages defined in Section 9.2 are not backward compatible with RFC 5046 defined iSER messages. I consider this as a significant issue. I suggest to add a single flag (or two flags) in iSCSI-control type PDU to indicate the presence of WBO and RBO by using one (or two) of the reserved bits. If this flag is set to 1, then the format is as defined in Figure 3 of this draft. If this flag is cleared, then the format of the iSER header for iSCSI control-type PDU is same as the one defined in RFC5046. I strongly suggest to keep the formats of iSER Hello and HelloReply Messages to be identical to the formats defined in RFC5046. These proposed modifications to the header formats in Section 9.2 make this draft backward compatible with RFC5046 when WBO and RBO are not used.
 5.  The headers in Sections 9.3 and 9.4 need to be reformatted if the change suggested in the previous bullet is made.
 6.  We should add an appendix for "InfiniBand Message Formats for iSER" because they are different from "iWARP Message Formats for iSER".
 7.  Solicited Event: RFC5046 describes the use of Send with Solicited Event (SendSE) and Send with Solicited Event and Invalidate (SendSEInv) Messages. This draft removes the use of SendSE and SendSEInv Messages. What was the rationale behind this removal? The draft should allow the use of SendSE and SendSEInv unless we have good reasons not to.

Regards,

Hemal

-----Original Message-----
From: [e-mail] [mailto:[e-mail]] On Behalf Of [e-mail]
Sent: Monday, November 21, 2011 11:32 AM
To: [e-mail]
Subject: [storm] WG Last Call - iSER - through December 12
Importance: High

This message is to announce the STORM working group Last Call on the following document:

                  iSCSI Extensions for RDMA Specification
                       draft-ietf-storm-iser-05.txt

http://datatracker.ietf.org/doc/draft-ietf-storm-iser/

Last Call comments are due by Monday, December 12 at midnight Eastern time.

Please send all technical comments to the storm mailing list: [e-mail]
Editorial comments may be sent directly to the draft authors: [e-mail]

I'll even start this Working Group Last Call with an initial comment:

The iSER draft currently references RFC 3720 for iSCSI - that reference will need to be updated to at least the new consolidated iSCSI draft, and a reference to the iSCSI new features (SAM) draft should probably be added.

There's no need for a new version of the draft now, but a new version will be needed before the RFC publication request can be submitted.

Thanks in advance for the WG's prompt review. :-)

--David (storm WG co-chair)
----------------------------------------------------
David L. Black, Distinguished Engineer
EMC Corporation, 176 South St., Hopkinton, MA  01748
[phone]             FAX: [phone]
[e-mail]        Mobile: [phone]
----------------------------------------------------

_______________________________________________
storm mailing list
[e-mail]
https://www.ietf.org/mailman/listinfo/storm