Hannes Reinecke wrote:
> Mike Christie wrote:
>> On 07/22/2009 12:00 PM, Mike Christie wrote:
>>>> No, wrong. The check in queuecommand is by no means relevant
>>>> to the actual window.
>>>> We're checking the target window at the time queuecommand is run,
>>>> but we're _generating_ the CmdSN only much later after we've
>>>> dequeued the command.
>>> The check in queuecommand is for scsi cmd pdus and checks the
>>> session->queued_cmdsn against the MaxCmdSn. The queued_cmdsn is just a
>>> preallocation of CmdSn values. So if the scsi layer sends us X commands,
>>> the initiator checks if the window has X spaces open. If it does, then
>>> we add the task to the cmdqueue, and increase queued_cmdsn. This is
>>> supposed to prevent the scsi layer from sending us too many commands and
>>> them sitting in the driver cmdqueue and timing out.
>>>
>>> The cmdsn is then allocated when we put the cmd on the wire. So the
>> Maybe this is causing some confusion. We do not increment the cmdsn 
>> value for immediate commands (how we send a tmf or nop) and for data-out 
>> pdus. We only increment it for scsi cmd pdus, so the only place I check 
>> for the window having space is in queuecommand. If you are worried about 
>> a nop or data out increasing the cmdsn value in one thread and a scsi 
>> cmd pdu increasing it in another, then it will not happen. We only this 
>> one path to worry about.
>>
> If it were so easy.
> 
> I have this patch:
> 
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 716cc34..90905f2 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -1212,6 +1212,21 @@ static int iscsi_xmit_task(struct iscsi_conn *conn)
>         struct iscsi_task *task = conn->task;
>         int rc;
>  
> +       /* Check for target CmdSN window */
> +       if (conn->session->state == ISCSI_STATE_LOGGED_IN &&
> +           iscsi_sna_lt(conn->session->max_cmdsn,
> +                        conn->session->cmdsn)) {
> +               iscsi_conn_printk(KERN_INFO, conn,
> +                                 "target window closed cmd %u max %u",
> +                                 conn->session->cmdsn,
> +                                 conn->session->max_cmdsn);
> +
> +#if 0
> +               /* Window closed, wait for CmdSN update */
> +               return -EPROTO;
> +#endif
> +       }
> +
>         __iscsi_get_task(task);
>         spin_unlock_bh(&conn->session->lock);
>         rc = conn->session->tt->xmit_task(task);
> 
> against the current linux-2.6-iscsi tree.
> When running my test harness (10 concurrent bonnie runs over 10
> iscsi LUNs), I'm seeing these messages:
> 
>  connection2:0: target window closed cmd 61225 max 61224
>  connection2:0: target window closed cmd 61258 max 61257
>  connection2:0: target window closed cmd 61291 max 61290
>  connection2:0: target window closed cmd 61324 max 61323
>  connection2:0: target window closed cmd 61357 max 61356
>  connection2:0: target window closed cmd 61413 max 61412
>  connection1:0: target window closed cmd 61321 max 61320
>  connection1:0: target window closed cmd 61361 max 61360
>  connection1:0: target window closed cmd 61401 max 61400
>  connection1:0: target window closed cmd 61449 max 61448
>  connection1:0: target window closed cmd 61482 max 61481
>  connection1:0: target window closed cmd 61531 max 61530
>  connection1:0: target window closed cmd 61579 max 61578
>  connection1:0: target window closed cmd 61626 max 61625
>  connection1:0: target window closed cmd 61674 max 61673
>  connection1:0: target window closed cmd 61717 max 61716
>  connection1:0: target window closed cmd 61754 max 61753
>  connection1:0: target window closed cmd 61802 max 61801
>  connection1:0: target window closed cmd 61837 max 61836
>  connection1:0: target window closed cmd 61874 max 61873
>  connection2:0: target window closed cmd 62205 max 62204
>  connection2:0: target window closed cmd 62326 max 62325
>  connection2:0: target window closed cmd 62390 max 62389
>  connection2:0: target window closed cmd 62486 max 62485
>  connection2:0: target window closed cmd 62522 max 62521
>  connection2:0: target window closed cmd 62686 max 62685
>  connection2:0: target window closed cmd 62813 max 62812
>  connection2:0: target window closed cmd 62881 max 62880
>  connection2:0: target window closed cmd 62914 max 62913
> 
> So there _is_ a race window. It might be that my 

This is expected like I said in the other mail, because before 
iscsi_xmit_task is called the session->cmdsn is incremented by 
iscsi_prep_scsi_cmd_pdu when sending scsi cmd pdus. For data-outs and 
nops, then it will probably be higher too, but like I said in the other 
mail that is ok.


iscsi_data_xmit->iscsi_prep_scsi_cmd_pdu

iscsi_prep_scsi_cmd_pdu()
{
hdr->cmdsn = session->cmdsn;
session->cmdsn++;
}

Then we call iscsi_xmit_task() and your check is spitting out and 
testing the new/updated session->cmdsn. The scsi cmd pdu that gets sent 
out is going to have a cmdsn that is less than or equal to maxcmdsn.

Try printing out the task->cmdsn for scsi cmd pdus.


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