Re: iSCSI-07: recovery

["Mallikarjun C." <cbm@rose.hp.com>]

Sandeep,

Thank you for the detailed review, and apologies for the
delay in responding, I was travelling to London and elsewhere.

>Comments/questions on new Appendix F and related Section 7.x
>These are arranged sequentially down the appendix.
>
>-Sandeep
>
>1) Page 173: What is the usage of nextCmdSN in the Connection 
>   structure ?
>
>   If it is being used to check gaps, something like this 
>   seems more appropriate to the Session and not Connection.

You are right, it is incorrectly placed in Connection.  I will
move this to the Session datastructure.

>
>2) Page 174: Within-command recovery
>   Initiator algo for Recover-Data-If-Possible
>
>   The step of sending an abort here could be skipped if 
>   the initiator knew that task has already completed at the 
>   target.   This avoids an extra task cmd & response 
>   (Function-complete)
>
>   The initiator knows the task has completed if this routine 
>   is invoked on an expDataSN mismatch in a SCSI Response 
>   (middle of page 175)

I see that you are suggesting an optimization.  These algorithms 
are not really tuned to be optimal - they strive to convey the 
recovery semantics in the simplest manner.

Anyway, I will add an additional check to convey the point you
make.

>
>3) Top of Page 175: Within-command recovery 
>   Initiator algo for Receive-a-In-PDU
>
>   When SoFarInOrder is False, and current DataSN is expected 
>   (its the next) then the action seems to be "discard; return".
>   Is this correct ?  So when does one *accept* the next 
>   DataSN (say 9) in the presence of outstanding missing data 
>   PDUs (say 4-6)

If there were outstanding missing data PDUs 4-6, the TCB.ExpectedDataSN
would stay stuck at 4 per the algorithm.  So, a subsequent DataSN=9
would result in send-data-SNACK being TRUE, and it may generate a
new Data SNACK on the wire (if deemed appropriate in the procedure 
Build-And-Send-DSnack - which is not described).

>
>4) Page 175-176: Within-command recovery 
>   Initiator algo for Receive-a-In-PDU
>
>   The psuedo-code under "Connection.SoFarInOrder is TRUE"
>   could be moved to Within-Connection recovery since the logic 
>   applies to all other PDUs (not just SCSI response) sent from 
>   target bearing statSNs.

This is something I debated myself while putting this together -
you are right, the code is fairly generic.  The only reason I finally
put this under Within-command is to have all SCSI response handling
in one place.  Let me split this into a separate procedure callable
from both Within-command and Within-connection algorithms.

>
>5) Page 177 : Within-command recovery
>   Target algo for Receive-a-In-PDU
>  
>   Please confirm, can the target use R2T for digest errors on 
>   unsolicited bursts ?   If yes, can we add an explicit statement 
>   to this effect.  Neither Section 7.4.(a) nor this section 
>   seem to imply otherwise but just want to confirm.

Yes, I certainly think so (of course if DataSequenceOrder was negotiated 
as "no").  Unsolicited bursts are technically a lot similar to solicited
in target handling - the only difference being that the TargetTransferTag
is spec-defined as 0xffffffff.  This is the running assumption in the
pseudo-code.

I will add a statement to this effect to section 7.4, if Julian agrees :-)

>
>6) Page 177 : Within-command recovery
>   Target algo for Receive-a-In-PDU
>
>   Bottom of the case for "if (CurrentPDU.type=DATA)"
>   we have "if (TCB.activeR2Ts=0) Build-and-send-Status()"
>
>   This needs an extra predicate as follows..
>     "If TCB.Response=DeliveryFailure && TCB.ActiveR2Ts=0"
>
>   This is correct in Transfer-Context-Timeout-Handler.

The code is for the case of successful reception of all data PDUs, so
it doesn't really need the extra predicate you suggest.  The case in
Transfer-Context-Timeout-Handler is different where the target is 
declaring a DeliveryFailure since it never received the last data PDU
for the transfer context in question.  

However, I did realize now in reviewing the code that it doesn't 
contain the subsequent changes to the spec - the "Not enough unsolicited
data" case - I will add what's missing now.

>
>7) Page 179 : Within-connection recovery
>   Initiator algo for Recover-Status-If-Possible
>
>   Typo error..  "note the missing statSNs in TCB"
>   should be     "note the missing statSNs in Connection"
>
>   statSN list is per-connection state.

You are right, I will fix it.

>
>8) Page 180: Within-Connection recovery
>   Initiator algo for Command-Acknowledge-Timeout-Handler
>   
>   This function seems like a better fit for the previous 
>   section on "within-command-recovery" and could be moved 
>   over there.

This functionality is actually part of the definition of Within-connection 
recovery, 7.11.2.

>
>9) Page 184 : Within-Session recovery
>   Target algo for Receive-A-In-PDU
>
>   It does not cover the case of receiving a login-restart 
>   (implicit logout).  This would have the same action as 
>   "logout.reason=recoveryRemove" 

The implicit logout may mean either recovery/close of the connection
based on the negotiated recovery capabilities (CommandFailoverSupport
= yes/no).  It is not clearly called out in the current spec.  Please
wait for my write-up on London error recovery proposals in the next
two days, which I hope would make it much more straightforward to grasp
this.

Thanks again.
--
Mallikarjun 


Mallikarjun Chadalapaka
Networked Storage Architecture
Network Storage Solutions Organization
MS 5668	Hewlett-Packard, Roseville.
cbm@rose.hp.com