Hannes Reinecke wrote:
> Mike Christie wrote:
>> 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.
>>
> Fsck. You are correct.
> 

But you still might be hitting a problem where the target does not like 
data-outs when it closed the window. Maybe they interpreted the RFC 
differently. You should ask the HP target guys for more info.

Also your patch might be working because I think it ends up throttling 
the connection, so IO does not timeout because pipes are backed up (the 
slow down from the throttling is one of the problems we hit with the 
patch I did before which was pretty much the same as you posted).


I think I can replicate this problem now too. It was by accident. I am 
using a EQL target remotely (I am in the middle of the US and the target 
is on the west coast so there is a good deal of space between us and the 
connection is slow) and I am seeing the problem where the network layer 
is just not taking any more data so eventually something times out (if I 
turn off nops then scsi command timer fires and if I also increase that 
to 10 minutes then the EQL target will actually send me a nop and I 
cannot send that because the network layer just keeps returning -AGAIN). 
Are you still seeing that problem? Basically sendpage/sendmsg just keeps 
returning -EGAIN. We even get woken up by iscsi_sw_tcp_write_space, but 
the sk_wmem_queued and sk_sndbuf values are basically stuck and so no 
space ever opens up for some reason (I attached the debug patch I am using).

I tried your patch hoping it might help, but it does not help for this 
problem here. Maybe it is different issues.


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

diff --git a/kernel/iscsi_tcp.c b/kernel/iscsi_tcp.c
index bce1594..ac9f4ab 100644
--- a/kernel/iscsi_tcp.c
+++ b/kernel/iscsi_tcp.c
@@ -161,12 +161,23 @@ static void iscsi_sw_tcp_state_change(struct sock *sk)
  **/
 static void iscsi_sw_tcp_write_space(struct sock *sk)
 {
+	struct socket *sock = sk->sk_socket;
 	struct iscsi_conn *conn = (struct iscsi_conn*)sk->sk_user_data;
 	struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
 	struct iscsi_sw_tcp_conn *tcp_sw_conn = tcp_conn->dd_data;
 
 	tcp_sw_conn->old_write_space(sk);
-	ISCSI_SW_TCP_DBG(conn, "iscsi_write_space\n");
+	spin_lock_bh(&conn->session->lock);
+	ISCSI_SW_TCP_DBG(conn, "iscsi_write_space has xmit %s has cmd %s "
+			"has imm %s sk_wmem_queued %d sk_sndbuf %d"
+			"sk_wmem_queued %d\n",
+			list_empty(&conn->requeue) ? "no" : "yes",
+			list_empty(&conn->cmdqueue) ? "no" : "yes",
+			list_empty(&conn->mgmtqueue) ? "no" : "yes",
+			sk->sk_wmem_queued >> 1,
+			sk->sk_sndbuf, sk->sk_wmem_queued);
+	spin_unlock_bh(&conn->session->lock);
+
 	iscsi_conn_queue_work(conn);
 }
 

Reply via email to