Re: iscsi : Review Feedback on Rev 11-94.

["Julian Satran" <Julian_Satran@il.ibm.com>]

Santosh,

Thanks again. Now I am on to it. Comments in text.

Julo

Santosh Rao <santoshr@cup.hp.com> Sent by: owner-ips@ece.cmu.edu 04/11/2002 05:51 AM Please respond to Santosh Rao

To:        IPS Reflector <ips@ece.cmu.edu>         cc:                 Subject:        iscsi : Review Feedback on Rev 11-94.

Julian,

Please find enclosed some review comments on iscsi rev 11-94. I have
classified them as EDITORIAL, TECHNICAL and CLARIFICATION.

Regards,
Santosh

-- 
We have enough people who tell it like it is
Now we could use a few who tell it like it can be.
+++ No comment on that :-) +++
-  Robert Orben1) Section 2.2.3 [EDITORIAL]
----------------------------
a) "On any connection the login phase must immediately succeed tcp connection
establishment and a single login phase is allowed before tearing down a
connection".

The above sentence is somewhat confusing since phase is used to denote (login,
full feature) as well as (security negotiation phase, operational negotiation
phase).

+++  I was carefull to call one phase and the other stage. If I slipped please tell me where +++

Can we instead state that session re-instatement should be performed on a new 
tcp connection ?

+++ it says also to tear down an old one (required for security)+++

2) Section 2.2.4 pg 36 [EDITORIAL]
----------------------------------
a) "The maximum amount of unsolicited data that can be sent with a command is
negotiated at login as the." 

Above sentence needs to be completed.

b) "The initiator actions on receiving an R2T request that specifies data all
or part outside the command bounds is unspecified".

Above sentence needs to be corrected. Perhaps, something like :
"The initiator actions on receiving an R2T request that requests data, all or
part of which is outside the data buffer boundaries is unspecified".

+++???+++

[TECHNICAL]
-----------
On a seperate note, why is this un-specified ? Can we change this to read :

"The initiator MUST discard an R2T request for data that is outside the data
buffer boundaries for that command. Following this, the initiator MAY abort the
command."
+++ So the target missbehaves and the initiator has to act - a good way to to enable DOS. As it stands it may disconnect which is better +++

3) Section 2.2.7 [TECHNICAL]
----------------------------
"iscsi does not require any persistent state maintenance across sessions."

The above is somewhat misleading, since some iscsi state is maintained
persistent across power cycles of the initiator or target :

i) iscsi targets must maintain a persistent mapping of their 
(network portals <-> TPGT), in order to present persistent target port
name/identifiers to the initiators they speak to. For this purpose, persistent
state must be preserved across a target power cycle.

ii) Initiators will have to re-use their ISIDs across sessions for the same I-T
nexus being [re-]established. For this purpose, initiators will need to save
their (ISID <-> target port) mappings persistent across initiator power cycles.
+++ all this is stated underneath. The above statement says ths iSCSI does not require state +++
4) Section 2.4.2 [TECHNICAL]
----------------------------
initiator scsi port name=="node name""\0""0 - 3 pad bytes""6 byte ISID"
Similar definition for target scsi port name.

The above definition causes problems for implementations due to the NULL in the
middle. This will confuse string manipulation routines (strlen, strcpy, etc) to
think it is the end of the string, ignoring the rest of the string. It is also 
quite complex in its construction.

Can we just use :
"node name,[i|t],[isid|tpgt]"
to represent initiator/target port names, instead ?

+++ I have changed it to a "comma separator" - I hope the NDT team won't kill me :-) +++
5) Section 2.5.1.1 [EDITORIAL]
------------------------------
a) Suffix "SCSI Execute Command" reference with [SAM-2] to make the definition
clear. Alternatively, add "SCSI Execute Command" to definitions and reference
SAM-2.

+++ OK +++

b) "SCSI Command carries output parameters". "SCSI Response carries input
parameters."
The above is confusing, since, it is actually the reverse. (i.e. The SCSI
Execute Command IN args are meant for the scsi command and the OUT args are
populated from the scsi response and returned to the application client.

+++ will fix +++

6) Section 2.5.1.5 [EDITORIAL]
------------------------------
"For the target, convinience, outgoing solicited data also carries a target
transfer tag."

should read :
"For the target's convinience,....".

7) Section 4.1 [CLARIFICATION]
------------------------------
"Any iscsi target or initiator MUST support receiving at least 16384 bytes of
key=value data in a negotiation sequence....".

What is the basis for the above requirement ? Is this referring to a single
"key=value" , or the complete "key=value" payload ?

+++ no the whole sequence - it is a simple way to put a "bound" on the negotiation" +++

8) Section 4.3.5.1 [TECHNICAL]
------------------------------
Is the "I_T nexus loss" notification being advocated for iscsi targets or
initiators or both ? If it is being advocated for the initiator, then, such
notification is not required upon session closure (case b), since session
closure occurs upon the ULP's explicit request to close. Hence, the ULP is
aware of the loss of nexus.

Also, can we avoid clearing mode pages in case of session re-instatement ?
(case a). In case of ErrorRecoveryLevel=0 initiators, on an iscsi error,
the initiator will log out and re-login, causing a session re-instatement. 

In this case, it is un-necessary to clear the mode page settings, since it 
causes extra work in the initiator for no reason. 
(need to apply flow control, notify ULP, re-negotiate mode pages, resume I/Os).
In this case, the nexus has not really been lost. It has been re-instated.
Hence, in this case, negotiated mode pages should not be cleared.

+++ Mallikarjun clarified this in an earlier note - we will remove the SCSI part if T10 folls up +++