Re: [RFC PATCH 1/5] block: Introduce q->abort_queue_fn()

[Tejun Heo <tj@kernel.org>]

Hello,

On Tue, May 22, 2012 at 03:30:37PM +0800, Asias He wrote:
> On 05/21/2012 11:42 PM, Tejun Heo wrote:
> 1) if the queue is stopped, q->request_fn() will never call called.
> we will be stuck in the loop forever. This can happen if the remove
> method is called after the q->request_fn() calls blk_stop_queue() to
> stop the queue when the device is full, and before the device
> interrupt handler to start the queue. This can be fixed by calling
> blk_start_queue() before __blk_run_queue(q).
> 
> blk_drain_queue() {
>    while(true) {
>       ...
>       if (!list_empty(&q->queue_head))
>         __blk_run_queue(q);
>       ...
>    }
> }

Wouldn't that be properly fixed by making queue cleanup override
stopped state?

> 2) Since the device is gonna be removed, is it safe to rely on the
> device to finish the request before the DEAD marking? E.g, In
> vritio-blk, We reset the device and thus disable the interrupt
> before we call blk_cleanup_queue(). I also suspect that the real
> hardware can finish the pending requests when being hot-unplugged.

Yes, it should be safe (otherwise it's a driver bug).  Device driver
already knows the state of the device it is driving.  If the device
can't service requests for whatever reason, the device driver should
abort any in-flight and future requests.  That's how other block
drivers behave and I don't see why virtio should be any different.

Also, blk_drain_queue() is used for other purposes too - elevator
switch and blkcg policy changes.  You definitely don't want to be
aborting requests across those events.

So, NACK.

Thanks.

-- 
tejun