* [PATCH 1/3] virtio-blk: Call del_gendisk() before disable guest kick
2012-05-25 2:34 [PATCH 0/3] Fix hot-unplug race in virtio-blk Asias He
@ 2012-05-25 2:34 ` Asias He
2012-05-25 7:07 ` Michael S. Tsirkin
2012-05-25 2:34 ` [PATCH 2/3] virtio-blk: Reset device after blk_cleanup_queue() Asias He
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Asias He @ 2012-05-25 2:34 UTC (permalink / raw)
To: virtualization, Rusty Russell, Michael S. Tsirkin; +Cc: kvm, Asias He
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@rustcorp.com.au>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: virtualization@lists.linux-foundation.org
Cc: kvm@vger.kernel.org
Signed-off-by: Asias He <asias@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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 1/3] virtio-blk: Call del_gendisk() before disable guest kick
2012-05-25 2:34 ` [PATCH 1/3] virtio-blk: Call del_gendisk() before disable guest kick Asias He
@ 2012-05-25 7:07 ` Michael S. Tsirkin
0 siblings, 0 replies; 15+ messages in thread
From: Michael S. Tsirkin @ 2012-05-25 7:07 UTC (permalink / raw)
To: Asias He; +Cc: kvm, virtualization
On Fri, May 25, 2012 at 10:34:47AM +0800, Asias He wrote:
> 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@rustcorp.com.au>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: virtualization@lists.linux-foundation.org
> Cc: kvm@vger.kernel.org
> Signed-off-by: Asias He <asias@redhat.com>
Acked-by: Michael S. Tsirkin <mst@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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/3] virtio-blk: Reset device after blk_cleanup_queue()
2012-05-25 2:34 [PATCH 0/3] Fix hot-unplug race in virtio-blk Asias He
2012-05-25 2:34 ` [PATCH 1/3] virtio-blk: Call del_gendisk() before disable guest kick Asias He
@ 2012-05-25 2:34 ` Asias He
2012-05-25 6:52 ` Michael S. Tsirkin
2012-05-25 7:08 ` Michael S. Tsirkin
2012-05-25 2:34 ` [PATCH 3/3] virtio-blk: Use block layer provided spinlock Asias He
2012-06-04 0:41 ` [PATCH 0/3] Fix hot-unplug race in virtio-blk Rusty Russell
3 siblings, 2 replies; 15+ messages in thread
From: Asias He @ 2012-05-25 2:34 UTC (permalink / raw)
To: virtualization, Rusty Russell, Michael S. Tsirkin; +Cc: kvm, Asias He
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@rustcorp.com.au>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: virtualization@lists.linux-foundation.org
Cc: kvm@vger.kernel.org
Signed-off-by: Asias He <asias@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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 2/3] virtio-blk: Reset device after blk_cleanup_queue()
2012-05-25 2:34 ` [PATCH 2/3] virtio-blk: Reset device after blk_cleanup_queue() Asias He
@ 2012-05-25 6:52 ` Michael S. Tsirkin
2012-05-25 7:03 ` Asias He
2012-05-25 7:08 ` Michael S. Tsirkin
1 sibling, 1 reply; 15+ messages in thread
From: Michael S. Tsirkin @ 2012-05-25 6:52 UTC (permalink / raw)
To: Asias He; +Cc: kvm, virtualization
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@rustcorp.com.au>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: virtualization@lists.linux-foundation.org
> Cc: kvm@vger.kernel.org
> Signed-off-by: Asias He <asias@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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 2/3] virtio-blk: Reset device after blk_cleanup_queue()
2012-05-25 6:52 ` Michael S. Tsirkin
@ 2012-05-25 7:03 ` Asias He
2012-05-25 7:06 ` Michael S. Tsirkin
0 siblings, 1 reply; 15+ messages in thread
From: Asias He @ 2012-05-25 7:03 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, virtualization
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@rustcorp.com.au>
>> Cc: "Michael S. Tsirkin"<mst@redhat.com>
>> Cc: virtualization@lists.linux-foundation.org
>> Cc: kvm@vger.kernel.org
>> Signed-off-by: Asias He<asias@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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 2/3] virtio-blk: Reset device after blk_cleanup_queue()
2012-05-25 7:03 ` Asias He
@ 2012-05-25 7:06 ` Michael S. Tsirkin
0 siblings, 0 replies; 15+ messages in thread
From: Michael S. Tsirkin @ 2012-05-25 7:06 UTC (permalink / raw)
To: Asias He; +Cc: virtualization, Rusty Russell, kvm
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@rustcorp.com.au>
> >>Cc: "Michael S. Tsirkin"<mst@redhat.com>
> >>Cc: virtualization@lists.linux-foundation.org
> >>Cc: kvm@vger.kernel.org
> >>Signed-off-by: Asias He<asias@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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] virtio-blk: Reset device after blk_cleanup_queue()
2012-05-25 2:34 ` [PATCH 2/3] virtio-blk: Reset device after blk_cleanup_queue() Asias He
2012-05-25 6:52 ` Michael S. Tsirkin
@ 2012-05-25 7:08 ` Michael S. Tsirkin
1 sibling, 0 replies; 15+ messages in thread
From: Michael S. Tsirkin @ 2012-05-25 7:08 UTC (permalink / raw)
To: Asias He; +Cc: kvm, virtualization
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@rustcorp.com.au>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: virtualization@lists.linux-foundation.org
> Cc: kvm@vger.kernel.org
> Signed-off-by: Asias He <asias@redhat.com>
Acked-by: Michael S. Tsirkin <mst@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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/3] virtio-blk: Use block layer provided spinlock
2012-05-25 2:34 [PATCH 0/3] Fix hot-unplug race in virtio-blk Asias He
2012-05-25 2:34 ` [PATCH 1/3] virtio-blk: Call del_gendisk() before disable guest kick Asias He
2012-05-25 2:34 ` [PATCH 2/3] virtio-blk: Reset device after blk_cleanup_queue() Asias He
@ 2012-05-25 2:34 ` Asias He
2012-05-25 7:02 ` Michael S. Tsirkin
2012-06-04 0:41 ` [PATCH 0/3] Fix hot-unplug race in virtio-blk Rusty Russell
3 siblings, 1 reply; 15+ messages in thread
From: Asias He @ 2012-05-25 2:34 UTC (permalink / raw)
To: virtualization, Rusty Russell, Michael S. Tsirkin; +Cc: kvm, Asias He
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@rustcorp.com.au>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: virtualization@lists.linux-foundation.org
Cc: kvm@vger.kernel.org
Signed-off-by: Asias He <asias@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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 3/3] virtio-blk: Use block layer provided spinlock
2012-05-25 2:34 ` [PATCH 3/3] virtio-blk: Use block layer provided spinlock Asias He
@ 2012-05-25 7:02 ` Michael S. Tsirkin
2012-05-25 7:23 ` Asias He
2012-05-25 8:03 ` [PATCH v2 " Asias He
0 siblings, 2 replies; 15+ messages in thread
From: Michael S. Tsirkin @ 2012-05-25 7:02 UTC (permalink / raw)
To: Asias He; +Cc: kvm, virtualization
On Fri, May 25, 2012 at 10:34:49AM +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
>
> Other drivers use block layer provided spinlock as well, e.g. SCSI. I
> do not see any reason why we shouldn't,
OK, but the commit log is all wrong then, it should look like this:
virtio uses an internal lock while block layer provides
its own spinlock. Switching to the common lock saves
a bit of memory and does not seem to have any disadvantages:
this does not increase lock contention because .....
Performance tests show no real difference: before ... after ...
> even the lock unblance issue can
> be fixed by block layer.
s/even/even if/ ?
The lock unblance issue wasn't yet discussed upstream, was it?
Looking at it from the other side, even if virtio can
work around the issue, block layer should be fixed if
it's buggy. Or maybe it's not buggy and this is just masking
some other real issue?
Does this mean it's inherently unsafe to use an internal spinlock?
Aren't there other drivers doing this?
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: virtualization@lists.linux-foundation.org
> Cc: kvm@vger.kernel.org
> Signed-off-by: Asias He <asias@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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/3] virtio-blk: Use block layer provided spinlock
2012-05-25 7:02 ` Michael S. Tsirkin
@ 2012-05-25 7:23 ` Asias He
2012-05-25 8:03 ` [PATCH v2 " Asias He
1 sibling, 0 replies; 15+ messages in thread
From: Asias He @ 2012-05-25 7:23 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, virtualization
On 05/25/2012 03:02 PM, Michael S. Tsirkin wrote:
> On Fri, May 25, 2012 at 10:34:49AM +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
>>
>> Other drivers use block layer provided spinlock as well, e.g. SCSI. I
>> do not see any reason why we shouldn't,
>
> OK, but the commit log is all wrong then, it should look like this:
>
> virtio uses an internal lock while block layer provides
> its own spinlock. Switching to the common lock saves
> a bit of memory and does not seem to have any disadvantages:
> this does not increase lock contention because .....
> Performance tests show no real difference: before ... after ...
Hmm. Why using the internal lock will have impact on the performance?
Anyway I will update the commit log.
>
>> even the lock unblance issue can
>> be fixed by block layer.
>
> s/even/even if/ ?
yes ;-)
> The lock unblance issue wasn't yet discussed upstream, was it?
See the patch I sent this morning.
[PATCH] block: Fix lock unbalance caused by lock disconnect
> Looking at it from the other side, even if virtio can
> work around the issue, block layer should be fixed if
> it's buggy. Or maybe it's not buggy and this is just masking
> some other real issue?
Yes. I got your point. I am trying to fix block layer as well.
> Does this mean it's inherently unsafe to use an internal spinlock?
> Aren't there other drivers doing this?
I think so.
>> Cc: Rusty Russell<rusty@rustcorp.com.au>
>> Cc: "Michael S. Tsirkin"<mst@redhat.com>
>> Cc: virtualization@lists.linux-foundation.org
>> Cc: kvm@vger.kernel.org
>> Signed-off-by: Asias He<asias@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
--
Asias
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH v2 3/3] virtio-blk: Use block layer provided spinlock
2012-05-25 7:02 ` Michael S. Tsirkin
2012-05-25 7:23 ` Asias He
@ 2012-05-25 8:03 ` Asias He
2012-05-25 13:10 ` Michael S. Tsirkin
2012-06-01 8:49 ` Michael S. Tsirkin
1 sibling, 2 replies; 15+ messages in thread
From: Asias He @ 2012-05-25 8:03 UTC (permalink / raw)
To: virtualization, Rusty Russell, Michael S. Tsirkin; +Cc: kvm
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.
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@rustcorp.com.au>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: virtualization@lists.linux-foundation.org
Cc: kvm@vger.kernel.org
Signed-off-by: Asias He <asias@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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v2 3/3] virtio-blk: Use block layer provided spinlock
2012-05-25 8:03 ` [PATCH v2 " Asias He
@ 2012-05-25 13:10 ` Michael S. Tsirkin
2012-06-01 8:49 ` Michael S. Tsirkin
1 sibling, 0 replies; 15+ messages in thread
From: Michael S. Tsirkin @ 2012-05-25 13:10 UTC (permalink / raw)
To: Asias He; +Cc: kvm, virtualization
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@rustcorp.com.au>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: virtualization@lists.linux-foundation.org
> Cc: kvm@vger.kernel.org
> Signed-off-by: Asias He <asias@redhat.com>
Well why not.
Acked-by: Michael S. Tsirkin <mst@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@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 3/3] virtio-blk: Use block layer provided spinlock
2012-05-25 8:03 ` [PATCH v2 " Asias He
2012-05-25 13:10 ` Michael S. Tsirkin
@ 2012-06-01 8:49 ` Michael S. Tsirkin
1 sibling, 0 replies; 15+ messages in thread
From: Michael S. Tsirkin @ 2012-06-01 8:49 UTC (permalink / raw)
To: Asias He; +Cc: kvm, virtualization
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
>
> 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@rustcorp.com.au>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: virtualization@lists.linux-foundation.org
> Cc: kvm@vger.kernel.org
> Signed-off-by: Asias He <asias@redhat.com>
It may also be worth pointing out that external lock
is inherently broken - it's kept around for historical reasons
only.
See also this discussion
https://lkml.org/lkml/2012/5/28/72
Note: a bugfix so 3.5 material I think.
Acked-by: Michael S. Tsirkin <mst@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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] Fix hot-unplug race in virtio-blk
2012-05-25 2:34 [PATCH 0/3] Fix hot-unplug race in virtio-blk Asias He
` (2 preceding siblings ...)
2012-05-25 2:34 ` [PATCH 3/3] virtio-blk: Use block layer provided spinlock Asias He
@ 2012-06-04 0:41 ` Rusty Russell
3 siblings, 0 replies; 15+ messages in thread
From: Rusty Russell @ 2012-06-04 0:41 UTC (permalink / raw)
To: Asias He, virtualization, Michael S. Tsirkin; +Cc: kvm
On Fri, 25 May 2012 10:34:46 +0800, Asias He <asias@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.
^ permalink raw reply [flat|nested] 15+ messages in thread