thr3ads.net - Virtualization - [PATCH 0/3] Fix hot-unplug race in virtio-blk [May 2012]

If this information is useful, please help other people find it:
Share via:

Asias He

2012-May-25 02:34 UTC

[PATCH 0/3] Fix hot-unplug race in virtio-blk

This patch set fixes the race when hot-unplug stressed disk.

Asias He (3):
  virtio-blk: Call del_gendisk() before disable guest kick
  virtio-blk: Reset device after blk_cleanup_queue()
  virtio-blk: Use block layer provided spinlock

 drivers/block/virtio_blk.c |   25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

-- 
1.7.10.2

Asias He

2012-May-25 02:34 UTC

head link

[PATCH 1/3] virtio-blk: Call del_gendisk() before disable guest kick

del_gendisk() might not return due to failing to remove the
/sys/block/vda/serial sysfs entry when another thread (udev) is
trying to read it.

virtblk_remove()
  vdev->config->reset() : guest will not kick us through interrupt
    del_gendisk()
      device_del()
        kobject_del(): got stuck, sysfs entry ref count non zero

sysfs_open_file(): user space process read /sys/block/vda/serial
   sysfs_get_active() : got sysfs entry ref count
      dev_attr_show()
        virtblk_serial_show()
           blk_execute_rq() : got stuck, interrupt is disabled
                              request cannot be finished

This patch fixes it by calling del_gendisk() before we disable guest's
interrupt so that the request sent in virtblk_serial_show() will be
finished and del_gendisk() will success.

This fixes another race in hot-unplug process.

It is save to call del_gendisk(vblk->disk) before
flush_work(&vblk->config_work) which might access vblk->disk, because
vblk->disk is not freed until put_disk(vblk->disk).

Cc: Rusty Russell <rusty at rustcorp.com.au>
Cc: "Michael S. Tsirkin" <mst at redhat.com>
Cc: virtualization at lists.linux-foundation.org
Cc: kvm at vger.kernel.org
Signed-off-by: Asias He <asias at redhat.com>
---
 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 693187d..1bed517 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -584,13 +584,13 @@ static void __devexit virtblk_remove(struct virtio_device
*vdev)
 	vblk->config_enable = false;
 	mutex_unlock(&vblk->config_lock);
 
+	del_gendisk(vblk->disk);
+
 	/* Stop all the virtqueues. */
 	vdev->config->reset(vdev);
 
 	flush_work(&vblk->config_work);
 
-	del_gendisk(vblk->disk);
-
 	/* Abort requests dispatched to driver. */
 	spin_lock_irqsave(&vblk->lock, flags);
 	while ((vbr = virtqueue_detach_unused_buf(vblk->vq))) {
-- 
1.7.10.2

Asias He

2012-May-25 02:34 UTC

head link

[PATCH 2/3] virtio-blk: Reset device after blk_cleanup_queue()

blk_cleanup_queue() will call blk_drian_queue() to drain all the
requests before queue DEAD marking. If we reset the device before
blk_cleanup_queue() the drain would fail.

1) if the queue is stopped in do_virtblk_request() because device is
full, the q->request_fn() will not be called.

blk_drain_queue() {
   while(true) {
      ...
      if (!list_empty(&q->queue_head))
        __blk_run_queue(q) {
	    if (queue is not stoped)
		q->request_fn()
	}
      ...
   }
}

Do no reset the device before blk_cleanup_queue() gives the chance to
start the queue in interrupt handler blk_done().

2) In commit b79d866c8b7014a51f611a64c40546109beaf24a, We abort requests
dispatched to driver before blk_cleanup_queue(). There is a race if
requests are dispatched to driver after the abort and before the queue
DEAD mark. To fix this, instead of aborting the requests explicitly, we
can just reset the device after after blk_cleanup_queue so that the
device can complete all the requests before queue DEAD marking in the
drain process.

Cc: Rusty Russell <rusty at rustcorp.com.au>
Cc: "Michael S. Tsirkin" <mst at redhat.com>
Cc: virtualization at lists.linux-foundation.org
Cc: kvm at vger.kernel.org
Signed-off-by: Asias He <asias at redhat.com>
---
 drivers/block/virtio_blk.c |   12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 1bed517..b4fa2d7 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -576,8 +576,6 @@ static void __devexit virtblk_remove(struct virtio_device
*vdev)
 {
 	struct virtio_blk *vblk = vdev->priv;
 	int index = vblk->index;
-	struct virtblk_req *vbr;
-	unsigned long flags;
 
 	/* Prevent config work handler from accessing the device. */
 	mutex_lock(&vblk->config_lock);
@@ -585,21 +583,13 @@ static void __devexit virtblk_remove(struct virtio_device
*vdev)
 	mutex_unlock(&vblk->config_lock);
 
 	del_gendisk(vblk->disk);
+	blk_cleanup_queue(vblk->disk->queue);
 
 	/* Stop all the virtqueues. */
 	vdev->config->reset(vdev);
 
 	flush_work(&vblk->config_work);
 
-	/* Abort requests dispatched to driver. */
-	spin_lock_irqsave(&vblk->lock, flags);
-	while ((vbr = virtqueue_detach_unused_buf(vblk->vq))) {
-		__blk_end_request_all(vbr->req, -EIO);
-		mempool_free(vbr, vblk->pool);
-	}
-	spin_unlock_irqrestore(&vblk->lock, flags);
-
-	blk_cleanup_queue(vblk->disk->queue);
 	put_disk(vblk->disk);
 	mempool_destroy(vblk->pool);
 	vdev->config->del_vqs(vdev);
-- 
1.7.10.2

Asias He

2012-May-25 02:34 UTC

head link

[PATCH 3/3] virtio-blk: Use block layer provided spinlock

Block layer will allocate a spinlock for the queue if the driver does
not provide one in blk_init_queue().

The reason to use the internal spinlock is that blk_cleanup_queue() will
switch to use the internal spinlock in the cleanup code path.
        if (q->queue_lock != &q->__queue_lock)
                q->queue_lock = &q->__queue_lock;

However, processes which are in D state might have taken the driver
provided spinlock, when the processes wake up , they would release the
block provided spinlock.

====================================[ BUG: bad unlock balance detected! ]
3.4.0-rc7+ #238 Not tainted
-------------------------------------
fio/3587 is trying to release lock (&(&q->__queue_lock)->rlock)
at:
[<ffffffff813274d2>] blk_queue_bio+0x2a2/0x380
but there are no more locks to release!

other info that might help us debug this:
1 lock held by fio/3587:
 #0:  (&(&vblk->lock)->rlock){......}, at:
[<ffffffff8132661a>] get_request_wait+0x19a/0x250

Other drivers use block layer provided spinlock as well, e.g. SCSI.  I
do not see any reason why we shouldn't, even the lock unblance issue can
be fixed by block layer.

Cc: Rusty Russell <rusty at rustcorp.com.au>
Cc: "Michael S. Tsirkin" <mst at redhat.com>
Cc: virtualization at lists.linux-foundation.org
Cc: kvm at vger.kernel.org
Signed-off-by: Asias He <asias at redhat.com>
---
 drivers/block/virtio_blk.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index b4fa2d7..774c31d 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -21,8 +21,6 @@ struct workqueue_struct *virtblk_wq;
 
 struct virtio_blk
 {
-	spinlock_t lock;
-
 	struct virtio_device *vdev;
 	struct virtqueue *vq;
 
@@ -65,7 +63,7 @@ static void blk_done(struct virtqueue *vq)
 	unsigned int len;
 	unsigned long flags;
 
-	spin_lock_irqsave(&vblk->lock, flags);
+	spin_lock_irqsave(vblk->disk->queue->queue_lock, flags);
 	while ((vbr = virtqueue_get_buf(vblk->vq, &len)) != NULL) {
 		int error;
 
@@ -99,7 +97,7 @@ static void blk_done(struct virtqueue *vq)
 	}
 	/* In case queue is stopped waiting for more buffers. */
 	blk_start_queue(vblk->disk->queue);
-	spin_unlock_irqrestore(&vblk->lock, flags);
+	spin_unlock_irqrestore(vblk->disk->queue->queue_lock, flags);
 }
 
 static bool do_req(struct request_queue *q, struct virtio_blk *vblk,
@@ -431,7 +429,6 @@ static int __devinit virtblk_probe(struct virtio_device
*vdev)
 		goto out_free_index;
 	}
 
-	spin_lock_init(&vblk->lock);
 	vblk->vdev = vdev;
 	vblk->sg_elems = sg_elems;
 	sg_init_table(vblk->sg, vblk->sg_elems);
@@ -456,7 +453,7 @@ static int __devinit virtblk_probe(struct virtio_device
*vdev)
 		goto out_mempool;
 	}
 
-	q = vblk->disk->queue = blk_init_queue(do_virtblk_request,
&vblk->lock);
+	q = vblk->disk->queue = blk_init_queue(do_virtblk_request, NULL);
 	if (!q) {
 		err = -ENOMEM;
 		goto out_put_disk;
-- 
1.7.10.2

Michael S. Tsirkin

2012-May-25 06:52 UTC

head link

[PATCH 2/3] virtio-blk: Reset device after blk_cleanup_queue()

On Fri, May 25, 2012 at 10:34:48AM +0800, Asias He
wrote:> blk_cleanup_queue() will call blk_drian_queue() to drain all the
> requests before queue DEAD marking. If we reset the device before
> blk_cleanup_queue() the drain would fail.
> 
> 1) if the queue is stopped in do_virtblk_request() because device is
> full, the q->request_fn() will not be called.
> 
> blk_drain_queue() {
>    while(true) {
>       ...
>       if (!list_empty(&q->queue_head))
>         __blk_run_queue(q) {
> 	    if (queue is not stoped)
> 		q->request_fn()
> 	}
>       ...
>    }
> }
> 
> Do no reset the device before blk_cleanup_queue() gives the chance to
> start the queue in interrupt handler blk_done().
> 
> 2) In commit b79d866c8b7014a51f611a64c40546109beaf24a, We abort requests
> dispatched to driver before blk_cleanup_queue(). There is a race if
> requests are dispatched to driver after the abort and before the queue
> DEAD mark. To fix this, instead of aborting the requests explicitly, we
> can just reset the device after after blk_cleanup_queue so that the
> device can complete all the requests before queue DEAD marking in the
> drain process.
> 
> Cc: Rusty Russell <rusty at rustcorp.com.au>
> Cc: "Michael S. Tsirkin" <mst at redhat.com>
> Cc: virtualization at lists.linux-foundation.org
> Cc: kvm at vger.kernel.org
> Signed-off-by: Asias He <asias at redhat.com>
> ---
>  drivers/block/virtio_blk.c |   12 +-----------
>  1 file changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 1bed517..b4fa2d7 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -576,8 +576,6 @@ static void __devexit virtblk_remove(struct
virtio_device *vdev)
>  {
>  	struct virtio_blk *vblk = vdev->priv;
>  	int index = vblk->index;
> -	struct virtblk_req *vbr;
> -	unsigned long flags;
>  
>  	/* Prevent config work handler from accessing the device. */
>  	mutex_lock(&vblk->config_lock);
> @@ -585,21 +583,13 @@ static void __devexit virtblk_remove(struct
virtio_device *vdev)
>  	mutex_unlock(&vblk->config_lock);
>  
>  	del_gendisk(vblk->disk);
> +	blk_cleanup_queue(vblk->disk->queue);
Device is not reset here yet, so it will process
some requests in parallel with blk_cleanup_queue.
Is this a problem? Why not?

>  
>  	/* Stop all the virtqueues. */
>  	vdev->config->reset(vdev);
>  
>  	flush_work(&vblk->config_work);
>  
> -	/* Abort requests dispatched to driver. */
> -	spin_lock_irqsave(&vblk->lock, flags);
> -	while ((vbr = virtqueue_detach_unused_buf(vblk->vq))) {
> -		__blk_end_request_all(vbr->req, -EIO);
> -		mempool_free(vbr, vblk->pool);
> -	}
> -	spin_unlock_irqrestore(&vblk->lock, flags);
> -
> -	blk_cleanup_queue(vblk->disk->queue);
>  	put_disk(vblk->disk);
>  	mempool_destroy(vblk->pool);
>  	vdev->config->del_vqs(vdev);
> -- 
> 1.7.10.2

Asias He

2012-May-25 07:03 UTC

head link

[PATCH 2/3] virtio-blk: Reset device after blk_cleanup_queue()

On 05/25/2012 02:52 PM, Michael S. Tsirkin wrote:> On Fri, May 25, 2012 at 10:34:48AM +0800, Asias He wrote:
>> blk_cleanup_queue() will call blk_drian_queue() to drain all the
>> requests before queue DEAD marking. If we reset the device before
>> blk_cleanup_queue() the drain would fail.
>>
>> 1) if the queue is stopped in do_virtblk_request() because device is
>> full, the q->request_fn() will not be called.
>>
>> blk_drain_queue() {
>>     while(true) {
>>        ...
>>        if (!list_empty(&q->queue_head))
>>          __blk_run_queue(q) {
>> 	    if (queue is not stoped)
>> 		q->request_fn()
>> 	}
>>        ...
>>     }
>> }
>>
>> Do no reset the device before blk_cleanup_queue() gives the chance to
>> start the queue in interrupt handler blk_done().
>>
>> 2) In commit b79d866c8b7014a51f611a64c40546109beaf24a, We abort
requests
>> dispatched to driver before blk_cleanup_queue(). There is a race if
>> requests are dispatched to driver after the abort and before the queue
>> DEAD mark. To fix this, instead of aborting the requests explicitly, we
>> can just reset the device after after blk_cleanup_queue so that the
>> device can complete all the requests before queue DEAD marking in the
>> drain process.
>>
>> Cc: Rusty Russell<rusty at rustcorp.com.au>
>> Cc: "Michael S. Tsirkin"<mst at redhat.com>
>> Cc: virtualization at lists.linux-foundation.org
>> Cc: kvm at vger.kernel.org
>> Signed-off-by: Asias He<asias at redhat.com>
>> ---
>>   drivers/block/virtio_blk.c |   12 +-----------
>>   1 file changed, 1 insertion(+), 11 deletions(-)
>>
>> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
>> index 1bed517..b4fa2d7 100644
>> --- a/drivers/block/virtio_blk.c
>> +++ b/drivers/block/virtio_blk.c
>> @@ -576,8 +576,6 @@ static void __devexit virtblk_remove(struct
virtio_device *vdev)
>>   {
>>   	struct virtio_blk *vblk = vdev->priv;
>>   	int index = vblk->index;
>> -	struct virtblk_req *vbr;
>> -	unsigned long flags;
>>
>>   	/* Prevent config work handler from accessing the device. */
>>   	mutex_lock(&vblk->config_lock);
>> @@ -585,21 +583,13 @@ static void __devexit virtblk_remove(struct
virtio_device *vdev)
>>   	mutex_unlock(&vblk->config_lock);
>>
>>   	del_gendisk(vblk->disk);
>> +	blk_cleanup_queue(vblk->disk->queue);
>
> Device is not reset here yet, so it will process
> some requests in parallel with blk_cleanup_queue.
> Is this a problem? Why not?
No. This is not a problem. Actually, blk_cleanup_queue assumes the 
device can process the requests before queue DEAD marking. Otherwise the 
drain in the blk_cleanup_queue would never success.
>
>>
>>   	/* Stop all the virtqueues. */
>>   	vdev->config->reset(vdev);
>>
>>   	flush_work(&vblk->config_work);
>>
>> -	/* Abort requests dispatched to driver. */
>> -	spin_lock_irqsave(&vblk->lock, flags);
>> -	while ((vbr = virtqueue_detach_unused_buf(vblk->vq))) {
>> -		__blk_end_request_all(vbr->req, -EIO);
>> -		mempool_free(vbr, vblk->pool);
>> -	}
>> -	spin_unlock_irqrestore(&vblk->lock, flags);
>> -
>> -	blk_cleanup_queue(vblk->disk->queue);
>>   	put_disk(vblk->disk);
>>   	mempool_destroy(vblk->pool);
>>   	vdev->config->del_vqs(vdev);
>> --
>> 1.7.10.2

-- 
Asias

Michael S. Tsirkin

2012-May-25 07:06 UTC

head link

[PATCH 2/3] virtio-blk: Reset device after blk_cleanup_queue()

On Fri, May 25, 2012 at 03:03:16PM +0800, Asias He
wrote:> On 05/25/2012 02:52 PM, Michael S. Tsirkin wrote:
> >On Fri, May 25, 2012 at 10:34:48AM +0800, Asias He wrote:
> >>blk_cleanup_queue() will call blk_drian_queue() to drain all the
> >>requests before queue DEAD marking. If we reset the device before
> >>blk_cleanup_queue() the drain would fail.
> >>
> >>1) if the queue is stopped in do_virtblk_request() because device
is
> >>full, the q->request_fn() will not be called.
> >>
> >>blk_drain_queue() {
> >>    while(true) {
> >>       ...
> >>       if (!list_empty(&q->queue_head))
> >>         __blk_run_queue(q) {
> >>	    if (queue is not stoped)
> >>		q->request_fn()
> >>	}
> >>       ...
> >>    }
> >>}
> >>
> >>Do no reset the device before blk_cleanup_queue() gives the chance
to
> >>start the queue in interrupt handler blk_done().
> >>
> >>2) In commit b79d866c8b7014a51f611a64c40546109beaf24a, We abort
requests
> >>dispatched to driver before blk_cleanup_queue(). There is a race if
> >>requests are dispatched to driver after the abort and before the
queue
> >>DEAD mark. To fix this, instead of aborting the requests
explicitly, we
> >>can just reset the device after after blk_cleanup_queue so that the
> >>device can complete all the requests before queue DEAD marking in
the
> >>drain process.
> >>
> >>Cc: Rusty Russell<rusty at rustcorp.com.au>
> >>Cc: "Michael S. Tsirkin"<mst at redhat.com>
> >>Cc: virtualization at lists.linux-foundation.org
> >>Cc: kvm at vger.kernel.org
> >>Signed-off-by: Asias He<asias at redhat.com>
> >>---
> >>  drivers/block/virtio_blk.c |   12 +-----------
> >>  1 file changed, 1 insertion(+), 11 deletions(-)
> >>
> >>diff --git a/drivers/block/virtio_blk.c
b/drivers/block/virtio_blk.c
> >>index 1bed517..b4fa2d7 100644
> >>--- a/drivers/block/virtio_blk.c
> >>+++ b/drivers/block/virtio_blk.c
> >>@@ -576,8 +576,6 @@ static void __devexit virtblk_remove(struct
virtio_device *vdev)
> >>  {
> >>  	struct virtio_blk *vblk = vdev->priv;
> >>  	int index = vblk->index;
> >>-	struct virtblk_req *vbr;
> >>-	unsigned long flags;
> >>
> >>  	/* Prevent config work handler from accessing the device. */
> >>  	mutex_lock(&vblk->config_lock);
> >>@@ -585,21 +583,13 @@ static void __devexit virtblk_remove(struct
virtio_device *vdev)
> >>  	mutex_unlock(&vblk->config_lock);
> >>
> >>  	del_gendisk(vblk->disk);
> >>+	blk_cleanup_queue(vblk->disk->queue);
> >
> >Device is not reset here yet, so it will process
> >some requests in parallel with blk_cleanup_queue.
> >Is this a problem? Why not?
> 
> No. This is not a problem. Actually, blk_cleanup_queue assumes the
> device can process the requests before queue DEAD marking. Otherwise
> the drain in the blk_cleanup_queue would never success.
I see, thanks for the clarification.
> >
> >>
> >>  	/* Stop all the virtqueues. */
> >>  	vdev->config->reset(vdev);
> >>
> >>  	flush_work(&vblk->config_work);
> >>
> >>-	/* Abort requests dispatched to driver. */
> >>-	spin_lock_irqsave(&vblk->lock, flags);
> >>-	while ((vbr = virtqueue_detach_unused_buf(vblk->vq))) {
> >>-		__blk_end_request_all(vbr->req, -EIO);
> >>-		mempool_free(vbr, vblk->pool);
> >>-	}
> >>-	spin_unlock_irqrestore(&vblk->lock, flags);
> >>-
> >>-	blk_cleanup_queue(vblk->disk->queue);
> >>  	put_disk(vblk->disk);
> >>  	mempool_destroy(vblk->pool);
> >>  	vdev->config->del_vqs(vdev);
> >>--
> >>1.7.10.2
> 
> 
> -- 
> Asias

Michael S. Tsirkin

2012-May-25 07:08 UTC

head link

[PATCH 2/3] virtio-blk: Reset device after blk_cleanup_queue()

On Fri, May 25, 2012 at 10:34:48AM +0800, Asias He
wrote:> blk_cleanup_queue() will call blk_drian_queue() to drain all the
> requests before queue DEAD marking. If we reset the device before
> blk_cleanup_queue() the drain would fail.
> 
> 1) if the queue is stopped in do_virtblk_request() because device is
> full, the q->request_fn() will not be called.
> 
> blk_drain_queue() {
>    while(true) {
>       ...
>       if (!list_empty(&q->queue_head))
>         __blk_run_queue(q) {
> 	    if (queue is not stoped)
> 		q->request_fn()
> 	}
>       ...
>    }
> }
> 
> Do no reset the device before blk_cleanup_queue() gives the chance to
> start the queue in interrupt handler blk_done().
> 
> 2) In commit b79d866c8b7014a51f611a64c40546109beaf24a, We abort requests
> dispatched to driver before blk_cleanup_queue(). There is a race if
> requests are dispatched to driver after the abort and before the queue
> DEAD mark. To fix this, instead of aborting the requests explicitly, we
> can just reset the device after after blk_cleanup_queue so that the
> device can complete all the requests before queue DEAD marking in the
> drain process.
> 
> Cc: Rusty Russell <rusty at rustcorp.com.au>
> Cc: "Michael S. Tsirkin" <mst at redhat.com>
> Cc: virtualization at lists.linux-foundation.org
> Cc: kvm at vger.kernel.org
> Signed-off-by: Asias He <asias at redhat.com>

Acked-by: Michael S. Tsirkin <mst at redhat.com>

> ---
>  drivers/block/virtio_blk.c |   12 +-----------
>  1 file changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 1bed517..b4fa2d7 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -576,8 +576,6 @@ static void __devexit virtblk_remove(struct
virtio_device *vdev)
>  {
>  	struct virtio_blk *vblk = vdev->priv;
>  	int index = vblk->index;
> -	struct virtblk_req *vbr;
> -	unsigned long flags;
>  
>  	/* Prevent config work handler from accessing the device. */
>  	mutex_lock(&vblk->config_lock);
> @@ -585,21 +583,13 @@ static void __devexit virtblk_remove(struct
virtio_device *vdev)
>  	mutex_unlock(&vblk->config_lock);
>  
>  	del_gendisk(vblk->disk);
> +	blk_cleanup_queue(vblk->disk->queue);
>  
>  	/* Stop all the virtqueues. */
>  	vdev->config->reset(vdev);
>  
>  	flush_work(&vblk->config_work);
>  
> -	/* Abort requests dispatched to driver. */
> -	spin_lock_irqsave(&vblk->lock, flags);
> -	while ((vbr = virtqueue_detach_unused_buf(vblk->vq))) {
> -		__blk_end_request_all(vbr->req, -EIO);
> -		mempool_free(vbr, vblk->pool);
> -	}
> -	spin_unlock_irqrestore(&vblk->lock, flags);
> -
> -	blk_cleanup_queue(vblk->disk->queue);
>  	put_disk(vblk->disk);
>  	mempool_destroy(vblk->pool);
>  	vdev->config->del_vqs(vdev);
> -- 
> 1.7.10.2

Michael S. Tsirkin

2012-May-25 13:10 UTC

head link

[PATCH v2 3/3] virtio-blk: Use block layer provided spinlock

On Fri, May 25, 2012 at 04:03:27PM +0800, Asias He
wrote:> Block layer will allocate a spinlock for the queue if the driver does
> not provide one in blk_init_queue().
> 
> The reason to use the internal spinlock is that blk_cleanup_queue() will
> switch to use the internal spinlock in the cleanup code path.
> 
>         if (q->queue_lock != &q->__queue_lock)
>                 q->queue_lock = &q->__queue_lock;
> 
> However, processes which are in D state might have taken the driver
> provided spinlock, when the processes wake up, they would release the
> block provided spinlock.
> 
> ====================================> [ BUG: bad unlock balance
detected! ]
> 3.4.0-rc7+ #238 Not tainted
> -------------------------------------
> fio/3587 is trying to release lock
(&(&q->__queue_lock)->rlock) at:
> [<ffffffff813274d2>] blk_queue_bio+0x2a2/0x380
> but there are no more locks to release!
> 
> other info that might help us debug this:
> 1 lock held by fio/3587:
>  #0:  (&(&vblk->lock)->rlock){......}, at:
> [<ffffffff8132661a>] get_request_wait+0x19a/0x250
The above is fixed by your patch
block: Fix lock unbalance caused by lock disconnect
so it really seems beside the point here.
So I think the part of the commit log that
makes sense starts here?

> Other drivers use block layer provided spinlock as well, e.g. SCSI.
> 
> Switching to the block layer provided spinlock saves a bit of memory and
> does not increase lock contention. Performance test shows no real
> difference is observed before and after this patch.
> 
> Changes in v2: Improve commit log as Michael suggested.
> 
> Cc: Rusty Russell <rusty at rustcorp.com.au>
> Cc: "Michael S. Tsirkin" <mst at redhat.com>
> Cc: virtualization at lists.linux-foundation.org
> Cc: kvm at vger.kernel.org
> Signed-off-by: Asias He <asias at redhat.com>
Well why not.

Acked-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>  drivers/block/virtio_blk.c |    9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index b4fa2d7..774c31d 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -21,8 +21,6 @@ struct workqueue_struct *virtblk_wq;
>  
>  struct virtio_blk
>  {
> -	spinlock_t lock;
> -
>  	struct virtio_device *vdev;
>  	struct virtqueue *vq;
>  
> @@ -65,7 +63,7 @@ static void blk_done(struct virtqueue *vq)
>  	unsigned int len;
>  	unsigned long flags;
>  
> -	spin_lock_irqsave(&vblk->lock, flags);
> +	spin_lock_irqsave(vblk->disk->queue->queue_lock, flags);
>  	while ((vbr = virtqueue_get_buf(vblk->vq, &len)) != NULL) {
>  		int error;
>  
> @@ -99,7 +97,7 @@ static void blk_done(struct virtqueue *vq)
>  	}
>  	/* In case queue is stopped waiting for more buffers. */
>  	blk_start_queue(vblk->disk->queue);
> -	spin_unlock_irqrestore(&vblk->lock, flags);
> +	spin_unlock_irqrestore(vblk->disk->queue->queue_lock, flags);
>  }
>  
>  static bool do_req(struct request_queue *q, struct virtio_blk *vblk,
> @@ -431,7 +429,6 @@ static int __devinit virtblk_probe(struct virtio_device
*vdev)
>  		goto out_free_index;
>  	}
>  
> -	spin_lock_init(&vblk->lock);
>  	vblk->vdev = vdev;
>  	vblk->sg_elems = sg_elems;
>  	sg_init_table(vblk->sg, vblk->sg_elems);
> @@ -456,7 +453,7 @@ static int __devinit virtblk_probe(struct virtio_device
*vdev)
>  		goto out_mempool;
>  	}
>  
> -	q = vblk->disk->queue = blk_init_queue(do_virtblk_request,
&vblk->lock);
> +	q = vblk->disk->queue = blk_init_queue(do_virtblk_request, NULL);
>  	if (!q) {
>  		err = -ENOMEM;
>  		goto out_put_disk;
> -- 
> 1.7.10.2
> 
> _______________________________________________
> Virtualization mailing list
> Virtualization at lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Rusty Russell

2012-Jun-04 00:41 UTC

head link

[PATCH 0/3] Fix hot-unplug race in virtio-blk

On Fri, 25 May 2012 10:34:46 +0800, Asias He <asias at redhat.com>
wrote:> This patch set fixes the race when hot-unplug stressed disk.
> 
> Asias He (3):
>   virtio-blk: Call del_gendisk() before disable guest kick
>   virtio-blk: Reset device after blk_cleanup_queue()
>   virtio-blk: Use block layer provided spinlock
> 
>  drivers/block/virtio_blk.c |   25 ++++++-------------------
>  1 file changed, 6 insertions(+), 19 deletions(-)
I've applied these (the revised spinlock patch, thanks to mst for his
feedback).

I think a cc: stable is in order for these, so I've added it.

Thanks,
Rusty.

Possibly Parallel Threads

Search for more seemingly similar threads

Virtualization - May 2012 - [PATCH 0/3] Fix hot-unplug race in virtio-blk

[PATCH 0/3] Fix hot-unplug race in virtio-blk

[PATCH 1/3] virtio-blk: Call del_gendisk() before disable guest kick

[PATCH 2/3] virtio-blk: Reset device after blk_cleanup_queue()

[PATCH 3/3] virtio-blk: Use block layer provided spinlock

[PATCH 2/3] virtio-blk: Reset device after blk_cleanup_queue()

[PATCH 2/3] virtio-blk: Reset device after blk_cleanup_queue()

[PATCH 2/3] virtio-blk: Reset device after blk_cleanup_queue()

[PATCH 2/3] virtio-blk: Reset device after blk_cleanup_queue()

[PATCH v2 3/3] virtio-blk: Use block layer provided spinlock

[PATCH 0/3] Fix hot-unplug race in virtio-blk

Possibly Parallel Threads