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;
> +     }
> +

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
then we get sleepy with queues full.

Thanks
Boaz

---
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 716cc34..fc81560 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1263,8 +1263,7 @@ static int iscsi_data_xmit(struct iscsi_conn *conn)
        spin_lock_bh(&conn->session->lock);
        if (unlikely(conn->suspend_tx)) {
                ISCSI_DBG_SESSION(conn->session, "Tx suspended!\n");
-               spin_unlock_bh(&conn->session->lock);
-               return -ENODATA;
+               goto enodata;
        }
 
        if (conn->task) {
@@ -1348,6 +1347,7 @@ check_mgmt:
                if (!list_empty(&conn->mgmtqueue))
                        goto check_mgmt;
        }
+enodata:
        spin_unlock_bh(&conn->session->lock);
        return -ENODATA;
 
  

>       /*
>        * process mgmt pdus like nops before commands since we should
>        * only have one nop-out as a ping from us and targets should not


--~--~---------~--~----~------------~-------~--~----~
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