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

[PATCH] 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>
---
 drivers/block/virtio_blk.c |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 7a51f06..97f53ac 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -42,6 +42,9 @@ struct virtio_blk
 	/* enable config space updates */
 	bool config_enable;
 
+	/* if the request queue is stopped, protected by vq_lock */
+	bool queue_stopped;
+
 	/* What host tells us, plus 2 for header & tailer. */
 	unsigned int sg_elems;
 
@@ -147,11 +150,13 @@ 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)
+	if (req_done && vblk->queue_stopped) {
 		blk_mq_start_stopped_hw_queues(vblk->disk->queue, true);
+		vblk->queue_stopped = false;
+	}
+	spin_unlock_irqrestore(&vblk->vq_lock, flags);
 }
 
 static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req)
@@ -205,8 +210,9 @@ 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);
+		vblk->queue_stopped = true;
+		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)
@@ -598,6 +604,7 @@ static int virtblk_probe(struct virtio_device *vdev)
 	vblk->disk->fops = &virtblk_fops;
 	vblk->disk->driverfs_dev = &vdev->dev;
 	vblk->index = index;
+	vblk->queue_stopped = false;
 
 	/* configure queue flush support */
 	virtblk_update_cache_mode(vdev);
-- 
1.7.9.5

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

[Rusty Russell]

Ming Lei <tom.leiming@gmail.com> writes:
> 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.

OK, but why the flag?  Isn't moving the
        blk_mq_start_stopped_hw_queues(vblk->disk->queue, true);

inside the lock sufficient?

Cheers,
Rusty.



>
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
> ---
>  drivers/block/virtio_blk.c |   13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 7a51f06..97f53ac 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -42,6 +42,9 @@ struct virtio_blk
>  	/* enable config space updates */
>  	bool config_enable;
>  
> +	/* if the request queue is stopped, protected by vq_lock */
> +	bool queue_stopped;
> +
>  	/* What host tells us, plus 2 for header & tailer. */
>  	unsigned int sg_elems;
>  
> @@ -147,11 +150,13 @@ 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)
> +	if (req_done && vblk->queue_stopped) {
>  		blk_mq_start_stopped_hw_queues(vblk->disk->queue, true);
> +		vblk->queue_stopped = false;
> +	}
> +	spin_unlock_irqrestore(&vblk->vq_lock, flags);
>  }
>  
>  static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req)
> @@ -205,8 +210,9 @@ 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);
> +		vblk->queue_stopped = true;
> +		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)
> @@ -598,6 +604,7 @@ static int virtblk_probe(struct virtio_device *vdev)
>  	vblk->disk->fops = &virtblk_fops;
>  	vblk->disk->driverfs_dev = &vdev->dev;
>  	vblk->index = index;
> +	vblk->queue_stopped = false;
>  
>  	/* configure queue flush support */
>  	virtblk_update_cache_mode(vdev);
> -- 
> 1.7.9.5

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

[Ming Lei]

On Fri, May 16, 2014 at 9:59 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Ming Lei <tom.leiming@gmail.com> writes:
>> 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.
>
> OK, but why the flag?  Isn't moving the
>         blk_mq_start_stopped_hw_queues(vblk->disk->queue, true);
>
> inside the lock sufficient?

The flag has been removed in v1. I introduced the flag just for
avoiding unnecessary calling blk_mq_start_stopped_hw_queues()
at most of time, but it isn't a big deal.

Thanks,
-- 
Ming Lei

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

[Jens Axboe]

On 2014-05-15 06:33, 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.

Why not just use blk_mq_start_hw_queues()?

-- 
Jens Axboe

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

[Christoph Hellwig]

On Fri, May 16, 2014 at 08:53:22AM -0600, Jens Axboe wrote:
> >This patch fixes the problem by starting/stopping blk-mq
> >with holding vq_lock.
> 
> Why not just use blk_mq_start_hw_queues()?

That would kick off a work item on every I/O completion, something
you didn't particularly like in scsi-mq :)

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

[Jens Axboe]

On 2014-05-16 08:54, Christoph Hellwig wrote:
> On Fri, May 16, 2014 at 08:53:22AM -0600, Jens Axboe wrote:
>>> This patch fixes the problem by starting/stopping blk-mq
>>> with holding vq_lock.
>>
>> Why not just use blk_mq_start_hw_queues()?
>
> That would kick off a work item on every I/O completion, something
> you didn't particularly like in scsi-mq :)

It would suck, I sent a followup after that one :-)

And yes, it still needs improving in scsi-mq too.

-- 
Jens Axboe

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

[Jens Axboe]

[-- Attachment #1: Type: text/plain, Size: 1037 bytes --]

On 2014-05-16 08:53, Jens Axboe wrote:
> On 2014-05-15 06:33, 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.
>
> Why not just use blk_mq_start_hw_queues()?

Or, if you want to maintain current heuristics, just move the start and 
stop under the vq_lock. That should prevent the race, as far as I can 
tell. Not sure what that extra queue_stopped would buy you, seems a lot 
cleaner to just maintain this state exclusively in the queue.

-- 
Jens Axboe


[-- Attachment #2: virtio-blk-start.patch --]
[-- Type: text/x-patch, Size: 1112 bytes --]

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 7a51f065edcd..2e328231a795 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -147,11 +147,12 @@ 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 +206,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)

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

[Ming Lei]

On Fri, May 16, 2014 at 10:57 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 2014-05-16 08:53, Jens Axboe wrote:
>>
>> On 2014-05-15 06:33, 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.
>>
>>
>> Why not just use blk_mq_start_hw_queues()?
>
>
> Or, if you want to maintain current heuristics, just move the start and stop
> under the vq_lock. That should prevent the race, as far as I can tell. Not
> sure what that extra queue_stopped would buy you, seems a lot cleaner to
> just maintain this state exclusively in the queue.

Yes.

But the flag can avoid to call blk_mq_start_stopped_hw_queues()
unnecessarily, which needn't at most of times.  Considered that
the interrupt may happen with very high frequency, I suggest to
introduce the extra flag.


Thanks,
-- 
Ming Lei

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

[Jens Axboe]

On 2014-05-16 09:15, Ming Lei wrote:
> On Fri, May 16, 2014 at 10:57 PM, Jens Axboe <axboe@kernel.dk> wrote:
>> On 2014-05-16 08:53, Jens Axboe wrote:
>>>
>>> On 2014-05-15 06:33, 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.
>>>
>>>
>>> Why not just use blk_mq_start_hw_queues()?
>>
>>
>> Or, if you want to maintain current heuristics, just move the start and stop
>> under the vq_lock. That should prevent the race, as far as I can tell. Not
>> sure what that extra queue_stopped would buy you, seems a lot cleaner to
>> just maintain this state exclusively in the queue.
>
> Yes.
>
> But the flag can avoid to call blk_mq_start_stopped_hw_queues()
> unnecessarily, which needn't at most of times.  Considered that
> the interrupt may happen with very high frequency, I suggest to
> introduce the extra flag.

virtio-blk just has one queue, so the flag is at least pointless for 
now. And since the other code stops all of them anyway, I don't see any 
reason not to just rely on that.

-- 
Jens Axboe

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

[Ming Lei]

On Fri, May 16, 2014 at 11:17 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 2014-05-16 09:15, Ming Lei wrote:
>
>
> virtio-blk just has one queue, so the flag is at least pointless for now.
> And since the other code stops all of them anyway, I don't see any reason
> not to just rely on that.

OK, if you don't care the little loading of calling
blk_mq_start_stopped_hw_queues() for each
interrupt, I will remove the flag.

Thanks,
-- 
Ming Lei