[PATCH v1] virtio_blk: fix race between start and stop queue

[PATCH v1] virtio_blk: fix race between start and stop queue

[Ming Lei]

When there isn't enough vring descriptor for adding to vq,
blk-mq will be put as stopped state until some of pending
descriptors are completed & freed.

Unfortunately, the vq's interrupt may come just before
blk-mq's BLK_MQ_S_STOPPED flag is set, so the blk-mq will
still be kept as stopped even though lots of descriptors
are completed and freed in the interrupt handler. The worst
case is that all pending descriptors are freed in the
interrupt handler, and the queue is kept as stopped forever.

This patch fixes the problem by starting/stopping blk-mq
with holding vq_lock.

Cc: Jens Axboe <axboe@kernel.dk>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
v1:
	- remove 'queue_stopped' flag

 drivers/block/virtio_blk.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 7a51f06..9f340fa 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -147,11 +147,11 @@ static void virtblk_done(struct virtqueue *vq)
 		if (unlikely(virtqueue_is_broken(vq)))
 			break;
 	} while (!virtqueue_enable_cb(vq));
-	spin_unlock_irqrestore(&vblk->vq_lock, flags);
 
 	/* In case queue is stopped waiting for more buffers. */
 	if (req_done)
 		blk_mq_start_stopped_hw_queues(vblk->disk->queue, true);
+	spin_unlock_irqrestore(&vblk->vq_lock, flags);
 }
 
 static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req)
@@ -205,8 +205,8 @@ static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req)
 	err = __virtblk_add_req(vblk->vq, vbr, vbr->sg, num);
 	if (err) {
 		virtqueue_kick(vblk->vq);
-		spin_unlock_irqrestore(&vblk->vq_lock, flags);
 		blk_mq_stop_hw_queue(hctx);
+		spin_unlock_irqrestore(&vblk->vq_lock, flags);
 		/* Out of mem doesn't actually happen, since we fall back
 		 * to direct descriptors */
 		if (err == -ENOMEM || err == -ENOSPC)
-- 
1.7.9.5

Re: [PATCH v1] virtio_blk: fix race between start and stop queue

[Jens Axboe]

On 2014-05-16 09:31, Ming Lei wrote:
> When there isn't enough vring descriptor for adding to vq,
> blk-mq will be put as stopped state until some of pending
> descriptors are completed & freed.
>
> Unfortunately, the vq's interrupt may come just before
> blk-mq's BLK_MQ_S_STOPPED flag is set, so the blk-mq will
> still be kept as stopped even though lots of descriptors
> are completed and freed in the interrupt handler. The worst
> case is that all pending descriptors are freed in the
> interrupt handler, and the queue is kept as stopped forever.
>
> This patch fixes the problem by starting/stopping blk-mq
> with holding vq_lock.

Thanks, this looks good, I'll apply it for 3.16 (with a stable marker, 
even if it is an unlikely event).

-- 
Jens Axboe

Re: [PATCH v1] virtio_blk: fix race between start and stop queue

[Ming Lei]

On Fri, May 16, 2014 at 11:32 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 2014-05-16 09:31, Ming Lei wrote:
>>
>> When there isn't enough vring descriptor for adding to vq,
>> blk-mq will be put as stopped state until some of pending
>> descriptors are completed & freed.
>>
>> Unfortunately, the vq's interrupt may come just before
>> blk-mq's BLK_MQ_S_STOPPED flag is set, so the blk-mq will
>> still be kept as stopped even though lots of descriptors
>> are completed and freed in the interrupt handler. The worst
>> case is that all pending descriptors are freed in the
>> interrupt handler, and the queue is kept as stopped forever.
>>
>> This patch fixes the problem by starting/stopping blk-mq
>> with holding vq_lock.
>
>
> Thanks, this looks good, I'll apply it for 3.16 (with a stable marker, even
> if it is an unlikely event).

Thanks.

It shouldn't be very difficult to happen in case of
non-indirect descriptor, and it is easy to reproduce
when module parameter of 'virtblk_queue_depth'
is bigger than vq->num_free for non-indirect case.


Thanks,
-- 
Ming Lei

Re: [PATCH v1] virtio_blk: fix race between start and stop queue

[Jens Axboe]

On 2014-05-16 09:43, Ming Lei wrote:
> On Fri, May 16, 2014 at 11:32 PM, Jens Axboe <axboe@kernel.dk> wrote:
>> On 2014-05-16 09:31, Ming Lei wrote:
>>>
>>> When there isn't enough vring descriptor for adding to vq,
>>> blk-mq will be put as stopped state until some of pending
>>> descriptors are completed & freed.
>>>
>>> Unfortunately, the vq's interrupt may come just before
>>> blk-mq's BLK_MQ_S_STOPPED flag is set, so the blk-mq will
>>> still be kept as stopped even though lots of descriptors
>>> are completed and freed in the interrupt handler. The worst
>>> case is that all pending descriptors are freed in the
>>> interrupt handler, and the queue is kept as stopped forever.
>>>
>>> This patch fixes the problem by starting/stopping blk-mq
>>> with holding vq_lock.
>>
>>
>> Thanks, this looks good, I'll apply it for 3.16 (with a stable marker, even
>> if it is an unlikely event).
>
> Thanks.
>
> It shouldn't be very difficult to happen in case of
> non-indirect descriptor, and it is easy to reproduce
> when module parameter of 'virtblk_queue_depth'
> is bigger than vq->num_free for non-indirect case.

I agree, it can definitely be setup so that it would not be hard to 
trigger. But I don't recall seeing any hang bugs since 3.13 was 
released, which would seem to indicate that it doesn't happen a lot in 
the wild with default settings.

-- 
Jens Axboe