iSCSI : Rev 05 Review Comments

[Santosh Rao <santoshr@cup.hp.com>]

Julian & All,

Enclosed is some review comments on rev 05 of the iSCSI draft.

Regards,
Santosh


Comments on iSCSI Rev 05 :
==========================

1) Section 2.10.7, pg 52. Login
-------------------------------
"Is significant only if TSID is zero and indicates the starting 
   Command Sequence Number for this session; it SHOULD be zero for all 
   other instances."

The above SHOULD to be replaced by MUST.

2) Section 2.11, pg 55. Login Response
--------------------------------------
Section 2.11.5 refers to the use of a 0 value for TSID to indicate Login
Reject. This can be removed since a status of 0206 indicates the same.

Also, are targets expected to populate all the fields of a login response 
when rejecting a login (i.e. non-0 status class) ? Needs to be called out
explicitly one way or the other.

3) Section 2.12.4, pg 58. PingMaxReplyLength
--------------------------------------------
"Ping data is reflected in the Ping Response. Note that the length of 
   the reflected data is limited by a negotiated parameter and the 
   initiator SHOULD avoid sending more than the negotiated limit. "

Replace SHOULD by MUST.

4) Section 2.6, pg 41. SCSI Task Mgmt Response
----------------------------------------------
The Response Codes in the SCSI Task Mgmt Response are insufficient.
The following additional response codes should be considered for addition to
the existing list :

a) Command Not Supported (ex : target does not support LUN Reset)
b) Invalid LUN (ex : LUN Reset sent to a non-existent LUN)
c) Busy	(ex : Task Mgmt command cannot be executed since the task manager is
	 busy.)

5) Section 2.12, pg 57. NOP-OUT
--------------------------------
When does ExpStatSN can take a value of 0. Needs to be called out in the spec.

6) Section 2.13, pg 59. NOP-IN requires iSCSI to be LUN aware.
---------------------------------------------------------------
In the NOP-IN PDU, when the P bit is 1, the spec requires the LUN field to
contain a valid LUN (should not be 0 ?). 
This requires LUN knowledge at the iSCSI layer, since NOP-IN is generated 
at the iSCSI layer which is LUN-list unaware.

The LUN field is not required for the NOP-IN PDU and should be removed. (As a
side note, the figure states LUN or reserved, while the description states the
field MUST contain a valid LUN.)

7) Section 2.12, pg 60. StatSN not required in NOP-IN.
-------------------------------------------------------
"Whenever the NOP-In is not issued in response to a NOP-Out the StatSN 
   field will contain as usual the next StatSN but StatSN for this 
   connection is not advanced."

Why is StatSN required in the NOP-IN PDU ? The above is adding extra code paths
in StatSN assignment in the performance path.(conditional StatSN assignment
based on opcode.). It is un-necessary complexity.

8) Section 2.16, pg 64. SACK should be included in "connection allegiance"
---------------------------------------------------------------------------
The "connection allegiance" definition should be expanded to include the SACK
in order to avoid spanning I/O state information across connections.(Needs to
be explicitly stated in the spec.)

9) Section 2.16. Initiator Task Tag in SACK PDU.
-------------------------------------------------
The spec needs to document when I.T.T can be reserved in a SACK. (A general
comment is that all fields in all PDUs need describing and should be described
in the order of the PDU layout.)

10) The layout of the Additional Runs for a SACK should be described in Section
2.16.

11) Per the spec, "The SACK also implicitly acknowledges data or status PDUs."
How is this ? The SACK is meant as a request to re-transmit a missing PDU. How
is it an implicit acknowledgement (?)

12) Section 2.17, pg 66, 67. Protocol rule regarding Target Transfer Tag.
-------------------------------------------------------------------------
Section 2.17 contradicts itself by stating that 
"there is no protocol rule about Target Transfer Tag"
 and then, stating :
 "All outstanding R2Ts should have different Target Transfer Tags".
 The usage of the Target Transfer Tags is device dependent and the draft need
 not elaborate on this.

13) Section 2.18.2., pg 69 Asynch Message SCSI Event.
-----------------------------------------------------
The draft refers to a non-existent "Length parameter" as :
"The Length Parameter is set to the length of the sense data."

14) Section 2.20. pg 71. Reject PDU
-----------------------------------
The draft must state that a reject PDU is not to be transmitted when the target
detects any of the reasons to reject on a NOP-OUT PDU with the Target Transfer
Tag set. (i.e.  a NOP-OUT from initiator in response to the target having sent 
a NOP-IN.)

15) Section 6.1. pg 80. Format Errors
-------------------------------------
"When an initiator receives an iSCSI PDU with a format error, for 
   which it has an outstanding task, it MUST abort the target task and 
   report the error through an appropriate service response"

How can an initiator know which task to abort when it may not be able to 
extract the Initiator Task Tag from the PDU due to the format error ?

Suggest re-wording the above to indicate that the initiator discard a PDU with
a format error and recover the I/O through the timeout path. It should specify
that on an I/O timeout, the initiator MUST abort the I/O. (As discussed during
rev 03 review, a section on Abort handling would be useful.)

16) Section 6.3. pg 81. Sequence Errors
---------------------------------------
"As a side effect of receiving the missing responses, the initiator 
   may discover missing data PDUs. The initiator MUST NOT acknowledge 
   (either explicitly through ExpStatRN or implicitly through a status 
   SACK) the received responses until it has completed receiving all the 
   data PDUs of a SCSI command. "

The above is not clear. Does this imply it should not acknowledge the status
until the I/O is "retried" and all the data PDUs are received ?

17) Section 6.6. pg 82. Session Errors.
---------------------------------------
This section is not clear on the recovery action being prescribed. Can the
initiator perform a logout with a reason of "closes the session" as session
recovery ? 

18) Section 6.7.1.1. pg 84. Recovery within a connection.
---------------------------------------------------------
"(1)Status/Response not acknowledged for a long time. The target 
      MAY issue a NOP-IN, with the P bit set to 1 or 0, which 
      indicates in the StatSN field the next status number it is 
      going to issue.  This helps the initiator detect missing StatSN 
      and issue a SACK-status. "

StatSN is not required to be sent in NOP-IN. When the target sends a NOP-IN
with P bit set, the initiator sends back ExpStatSN thereby acknowledging
Status.

19) Section 1.2.2.1, pg 12
--------------------------
"If immediate delivery is used with task management commands, these 
commands may reach the SCSI target task management before the tasks 
they are supposed to act upon.  Whenever those effects are 
undesirable, connection allegiance or ordered delivery MAY be used."

The term "connection allegiance" is used above before its definition in Section
1.2.5. Recommend adding a list of acronyms and definitions to the document.

20) Section 2.3.2, pg 33
------------------------
The use of "SCSI Execute Command" must be additionally qualified with [SAM2]
reference or better still, a brief explanation of the SAM2 CRN to iSCSI CmdRN
mapping added to this section.


-- 
#################################
Santosh Rao
Software Design Engineer,
HP, Cupertino.
email : santoshr@cup.hp.com
Phone : 408-447-3751
#################################