Re: [PATCH] block: Make blk_drain_queue() work for stopped queues

[Bart Van Assche <bvanassche@acm.org>]

On 03/19/12 07:26, Stanislaw Gruszka wrote:
> On Sun, Mar 18, 2012 at 07:47:47PM +0000, Bart Van Assche wrote:
>> On 03/18/12 15:57, Tejun Heo wrote:
>>> On Sun, Mar 18, 2012 at 01:18:21PM +0000, Bart Van Assche wrote:
>>>> All queued requests must be processed eventually. Hence make sure
>>>> that blk_drain_queue() drains the queue even if the queue is in the
>>>> stopped state. This patch makes it safe to invoke blk_cleanup_queue()
>>>> on a stopped queue.
>>> ...
>>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>>> index 3a78b00..bdcec86 100644
>>>> --- a/block/blk-core.c
>>>> +++ b/block/blk-core.c
>>>> @@ -300,10 +300,8 @@ EXPORT_SYMBOL(blk_sync_queue);
>>>>   */
>>>>  void __blk_run_queue(struct request_queue *q)
>>>>  {
>>>> -	if (unlikely(blk_queue_stopped(q)))
>>>> -		return;
>>>> -
>>>> -	q->request_fn(q);
>>>> +	if (!blk_queue_stopped(q) || blk_queue_dead(q))
>>>> +		q->request_fn(q);
> I'm not sure if that behaviour is correct, i.e. we can call q->request_fn(q)
> if someone stoped queue, but if it is why not just call q->request_fn(q)
> from blk_drain_queue() instead?

As far as I can see invoking q->request_fn(q) directly from
blk_drain_queue() would be a valid alternative.

>
>>> So, this allows calling request_fn for dead && stopped queue.  Have
>>> you seen something which requires this?
>> Not servicing queued SCSI requests can e.g. cause user space processes
>> to hang. See also http://lkml.org/lkml/2011/8/27/6 for an example. Hence
>> commit 3308511c93e6ad0d3c58984ecd6e5e57f96b12c8 which causes pending
>> SCSI commands to be killed just before blk_cleanup_queue() is invoked.
>> However, there is still a tiny race window left by that patch - new
>> requests can get queued after the SCSI request function has been invoked
>> by scsi_free_queue() and before blk_cleanup_queue() gets invoked. Hence
>> the proposal to change the block layer to make sure that all queued
>> requests get processed eventually.
> That behaviour I can confirm using this script [1] running with usb
> dongle. I applied this patch and second one:
> http://marc.info/?l=linux-scsi&m=133207725114386&w=2
> (BTW: second one patch is mangled). My impression is, that the script run
> much longer before it finally hung at infinite loop in blk_drain_queue().

I'm not an USB expert but I've had a quick look at
usb_stor_release_resources() in drivers/usb/storage/usb.c. As far as I
can see that function will only stop the usb_stor_control_thread() if
that thread has been scheduled after the last complete() call by the USB
queuecommand() function and before the complete() call in
usb_stor_release_resources() is executed. That looks like a race
condition to me.

Bart.