Amazingly the RFC says "The target MUST silently ignore any non-immediate 
command 
outside of this range(...)", but it does not say something like the "Initiator 
SHOULD NOT send ... any command out of range..."

Regards,
Ulrich

On 21 Jul 2009 at 15:21, Mike Christie wrote:

> On 07/21/2009 11:35 AM, Boaz Harrosh wrote:
> > On 07/21/2009 03:33 PM, Hannes Reinecke wrote:
> >> When sending a PDU we're just increase the CmdSN number
> >> without any check for MaxCmdSN. This results in unexpected
> >> ping timeouts and even connection stalls.
> >>
> >> So we should make sure to check CmdSN against MaxCmdSN
> >> before transferring a PDU, and just retry until MaxCmdSN
> >> has been updated.
> >>
> >> Signed-off-by: Hannes Reinecke<h...@suse.de>
> >> ---
> >>   drivers/scsi/libiscsi.c |   13 +++++++++++++
> >>   1 files changed, 13 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> >> index 21ed45f..ffb1338 100644
> >> --- a/drivers/scsi/libiscsi.c
> >> +++ b/drivers/scsi/libiscsi.c
> >> @@ -1273,6 +1273,19 @@ static int iscsi_data_xmit(struct iscsi_conn *conn)
> >>                    goto again;
> >>    }
> >>
> >> +  if (conn->session->state == ISCSI_STATE_LOGGED_IN&&
> >> +      !iscsi_sna_lte(conn->session->cmdsn,
> >> +                     conn->session->max_cmdsn)) {
> >> +          /* Window closed, wait for MaxCmdSN update */
> >> +          ISCSI_DBG_SESSION(conn->session,
> >> +                            "target window closed, "
> >> +                            "cmd %u max %u\n",
> >> +                            conn->session->cmdsn,
> >> +                            conn->session->max_cmdsn);
> >> +          spin_unlock_bh(&conn->session->lock);
> >> +          return -ENODATA;
> >> +  }
> 
> 
> I am not sure if we should be needing this if the target is operating 
> within the RFC (there is one exception but I am not sure if you are 
> hitting it).
> 
> In 3.2.2.1, I saw this:
> 
> An iSCSI target MUST be able to handle at least one immediate task 
> management command and one immediate non-task-management iSCSI command 
> per connection at any time.
> 
> 3.2.2.1 also has:
> 
> The target MUST silently ignore any non-immediate command outside of 
> this range or non-immediate duplicates within the range. The CmdSN 
> carried by immediate commands may lie outside the ExpCmdSN to MaxCmdSN 
> range.
> 
> 
> I took this to mean even when the window is closed we can send a nop as 
> immediate. What do you guys think?
> 
> The initiator will only send 1 TMF as immediate per session at a time 
> and it will only send one nop as a ping marked as immediate at a time. 
> The only exception to use sending more than one non tmf immediate cmd is 
> if the target sends us a nop-in we could have sent two nop-outs marked 
> as immediate (for the nop-out in response to the target's nop-in, 
> 10.18.1 says we have to set the I bit).
> 
> If we send too many nops marked as immediate we should be getting a 
> reject pdu right? If so then I think we just need the attached patch 
> which adds some code to handle rejected immediate pdus. The patch is 
> made over scsi-rc-fixes and is only compile tested.
> 
> 
> Are you only seeing this with the one target? Could we confirm with them 
> that they will accept one non tmf immediate command?
> 
> 
> If I am reading the RFC wrong, then for your patch, we want to move the 
> check to below the check_mgmt label because iscsi_data_xmit can send 
> multiple pdus. You probably just want to move it to 
> iscsi_prep_mgmt_task(). Also I think we want to dequeue a nop as a ping 
> so it does not timeout while the cmd window is closed (the problem would 
> be is if the window was closed and then the connection goes bad - we 
> would not be able to catch that).
> 
> 
> 
> >> +
> >
> > Looks good, From the time queuecommand did the check 
> > (iscsi_check_cmdsn_window_closed)
> > a management command came in without checking and stuffed an entry into the 
> > task queue.
> > Good catch.
> >
> > Just nit picking you could use iscsi_check_cmdsn_window_closed() above just 
> > for
> > style optimization. And also you might want to add the below cleanup.
> >
> > One question though. Do we want to return -ENODATA which means we get woken 
> > up
> > only at next command submission, or we want "goto again" to wake up after a 
> > while
> > for retry? I'd say the later since it might be the last command in a set and
> 
> We do the same thing for the goto if rc == -ENODATA and if you return 
> -ENODATA.
> 
> If the cmd window opens iscsi_update_cmdsn will call queue_work, so the 
> xmit thread is woken up. Or if mem/space opens up in the socket, 
> iscsi_tcp will call queue_work to wake it, or if a new command comes in 
> that will also kick the thread.
> 
> The only time we would retry immediately is if -EAGAIN is returned from 
> iscsi_data_xmit.
> 
> The goto again is probably better so it is consistent with the other code.
> 
> > then we get sleepy with queues full.
> >
> 
> 
> > 
> 



--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To post to this group, send email to open-iscsi@googlegroups.com
To unsubscribe from this group, send email to 
open-iscsi+unsubscr...@googlegroups.com
For more options, visit this group at http://groups.google.com/group/open-iscsi
-~----------~----~----~----~------~----~------~--~---

Reply via email to