On 08/14/2009 07:01 AM, Hannes Reinecke wrote:
> Hi Mike,
> finally I've found the mysterious I/O stall I've been chasing
> for the last two months.
> Problem is iscsi_conn_queue_work(); we're just calling
> queue_work() without checking the return value.
> However, queue_work() will be effectively a no-op when
> there is some work already running, so we'll be missing
> quite a few invocations (on my NetApp target I've been
> counting up to 120000 missed invocations ...).

Huh, I think I am misunderstanding you or I must be misreading workqueue 
code or probably both :(

When you saying running do you mean the work fn is running or that the 
work is queued (WORK_STRUCT_PENDING bit is set but the work fn is not 
yet called)?

I thought this happens when queue_work returns 1:


so if we called queue_work while iscsi_data_xmit was running then 
queue_work's pending test:

         if (!test_and_set_bit(WORK_STRUCT_PENDING, work_data_bits(work))) {
                 __queue_work(wq_per_cpu(wq, cpu), work);

would actually queue the work to be run. So I thought we could actually 
be getting extra calls to our work function, because we could call 
queue_work right after the WORK_STRUCT_PENDING bit cleared but the work 
fn is not yet running.

If the queue_work returns 0, then I thought iscsi_data_xmit is not yet 
running, so when it does run it would handle the task added to the list 
by the caller of queue_work (we do list_add then queue_work so it should 
be on the list when the work function would run if the work is pending 
when we call queue_work).

Hey, for the list_add+queue_work sequence do I need to add some sort of 

> In addition to that, iscsi_check_cmdsn_window_closed()
> doesn't actually check if the cmdsn window has been closed
> (in the sense that we're not allowed to send any PDUs),
> but rather that the new PDU _to be queued_ will be rejected.
> There might still be PDUs in the queue, waiting to be
> transferred.
> So if we're hitting queuecommand hard enough we might be running into this
> scenario:
> - iscsi_data_xmit:
>     Transfers last Data-out PDU in requeue list; all queues are empty.
>     ->  xmit_task()
>        ->  unlock session
> - queuecommand()
>      being called repeatedly until iscsi_check_cmdsn triggers, giving
>      xmit thread no chance to grab the session lock
>      ->  iscsi_conn_queue_work() does nothing as work is already pending

Yeah, so I thought here because iscsi_data_xmit is running we hit this:


//WORK_STRUCT_PENDING is now cleared so queue_work will queue the work 
(queue_work returns 1).


> - iscsi_data_xmit:
>        ->  locks session
>      ->  returns from xmit_task()
>    ->  end of iscsi_data_xmit()
> ->  I/O stall.
> So we really should check if we've missed some queue_work calls,
> and restart iscsi_data_xmit() if so.
> Proposed patch is attached.

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 
For more options, visit this group at http://groups.google.com/group/open-iscsi

Reply via email to