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

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