Internet Small Computer System Interface (iSCSI) Extensions for the Remote Direct Memory Access (RDMA) Specification
draft-ietf-storm-iser-15

I have only reviewed the things in this document that have changed since 5046.

Overall comment: The writeup indicates that there is widespread interoperable deployment. The changes to this document from 5046 seem to document what's actually going on in the field, as against introducing new functionality. Is there a reason that you didn't submit this for full Internet Standard instead of recycling at Proposed Standard? Unless you see an update to this coming soon, it seems like a waste not to try to advance it unless there is a good reason not to.

Only a couple of specific comments:


2.5.1:

   The iSER layer at the initiator SHOULD invalidate the Advertised
   STag upon a normal completion of the associated task.  The Send with
   Invalidate Message, if supported by the RCaP layer (e.g., iWARP),
   can be used for automatic invalidation when it is used to carry the
   SCSI Response PDU.  There are two exceptions to this automatic
   invalidation - bidirectional commands, and abnormal completion of a
   command.  The iSER Layer at the initiator SHOULD explicitly
   invalidate the STag in these two cases.
   
   
The last paragraph of this section describes the downside of violating the SHOULDs in the above paragraph. Thanks for that; it's exactly what a document ought to provide whenever there is a SHOULD. But you don't explain the sorts of circumstances that would necessitate not following the SHOULDs: Why are these not MUSTs?

5.1.1:

   If the outcome of the iSCSI negotiation is to enable iSER-assisted
   mode, then on the initiator side, prior to sending the Login Request
   with the T (Transit) bit set to 1 and the NSG (Next Stage) field set
   to FullFeaturePhase, the iSCSI Layer SHOULD request the iSER Layer
   to allocate the connection resources necessary to support RCaP by
   invoking the Allocate_Connection_Resources Operational Primitive.

I don't understand why the MUST was changed to a SHOULD, but then again I don't understand why either one is used in the first place: Don't you just mean "the iSCSI Layer will request the iSER Layer to allocate" or "the iSCSI Layer can request the iSER Layer to allocate"? Maybe I'm not understanding the meaning of the requirement here. Is this part of what's being discussed in the new text in 5.1.3? If this is just about the timing, switching the MUST to a SHOULD in this paragraph doesn't make that clear.

   These may include, but not
   limited to, MaxOutstandingR2T, ErrorRecoveryLevel, etc.

5046 was correct here: "but *are* not limited to"

One more global comment, strictly editorial (and probably pedantic): You've (incorrectly) changed a bunch of "that"s to "which"s. The RFC Editor will probably deal with them, but I don't understand why you did.

Thank you for your help in resolving my DISCUSS.

In 2.1  Motivation

   Out-of-order TCP segments in the Traditional iSCSI model have to be
   stored and reassembled before the iSCSI protocol layer within an end
   node can place the data in the iSCSI buffers.  This reassembly is
   required because not every TCP segment is likely to contain an iSCSI
   header to enable its placement and TCP itself does not have a built-
   in mechanism for signaling ULP message boundaries to aid placement
   of out-of-order segments.  

Isn't the fundamental reason out-of-order TCP segments have to be reassembled, that TCP offers a connection-oriented service with no alternatives to in-order delivery to an application? Or are you talking about something else? I note that pretty much the same text is in RFC 5046, so, I'm probably just not understanding what's going on.

In 4.1  Interactions with the RCaP Layer

   The iSER protocol layer is layered on top of an RCaP layer (see
   Error! Reference source not found.)

I think you've already seen comments about problems with cross-references, but I'm including for completeness.

In 5.1  iSCSI/iSER Connection Setup

   iSER-assisted mode MUST be enabled only if it is negotiated on the
   leading connection during the LoginOperationalNegotiation Stage of
   the iSCSI Login Phase.  

I think I understand this, but it might be clearer to say

   iSER-assisted mode MUST NOT be enabled unless it is negotiated on the
   leading connection during the LoginOperationalNegotiation Stage of
   the iSCSI Login Phase.

In 5.2.3.2  Connection Termination Notification to the iSCSI Layer

   If the remote iSCSI/iSER node initiated the closing of the
   Connection (e.g., by sending a TCP FIN or TCP RST), the iSER Layer
   MUST notify the iSCSI Layer after the RCaP layer reports that the
   Connection is closed by invoking the Connection_Terminate_Notify
   Operational Primitive.

Do the iSER and iSCSI layers treat TCP FIN and TCP RST the same way? is there any action that's required for one, but not the other? I think (from reading the sections on recovery) that they're treated the same way, so I'm not asking for a text change here, just for some clue.

In 7.3.11 SNACK Request

   Since HeaderDigest and DataDigest must be negotiated to "None",
   there are no digest errors when the connection is in iSER-assisted
   mode.  Also since RCaP delivers all messages in the order they were
   sent, there are no sequence errors when the connection is in iSER-
   assisted mode.  Therefore the iSCSI Layer MUST NOT send SNACK
   Request PDUs.  A SNCAK Request PDU, if used, MUST be treated as an
                    ^
Is this a typo? 

   iSCSI protocol error.  The iSER Layer MAY reject such a PDU from the
   iSCSI Layer with an appropriate error code.  If a SNACK Request PDU
   is received by the iSCSI Layer at the target, it MUST respond with a
   Reject PDU with a reason code of "protocol error".

In 1.1, you seem to have a diction problem in Completion:

       Completion is defined
       as the process by the RDMA-Capable Protocol layer to inform

Do you mean "Completion is defined as the process by which the RDMA-Capable Protocol layer informs"?

I assume that iSCSI control-type PDU and iSCSI data-type PDU are terms of art, and that you can't change them, but these are horribly confusing terms to use.   If possible, it would be better to leave out the "-type" part of the name, and use a name that makes clear that these are messages, and not pieces of hardware.

       iSER-IRD - This variable represents the maximum number of incoming
       outstanding RDMA Read Requests that the iSER Layer at the
       initiator declares on a particular RCaP Stream.

It might be better to say "specifies for" rather than "declares on" here.   I found the way iSER-ORD was defined to be clearer, also—this definition doesn't say what effect the iSER-IRD has.

Phase Collapse is a really confusing term—it sounds like something that happens in an inductor in an AC circuit.   Can't you say something like "Phase aggregation" instead?

1.2 does not define the acronym "RCaP".  Personally I'd prefer to see fewer acronyms anyway, but it seems inconsistent to define so many acronyms in 1.2 and not define this one.

In 3.3:
   4.  The iSCSI Layer at the initiator MUST NOT issue SNACKs for PDUs.

Which type of SNACK do you mean here?  Given that the acronym is defined as having two meanings, I think you need to specify.