gmane.linux.kernel.virtualization

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

Michael S. Tsirkin — 2012-05-25T13:10:47

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?



Well why not.

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

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

Asias He — 2012-05-25T08:03:27

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< 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
< at >< at > -21,8 +21,6 < at >< at > struct workqueue_struct *virtblk_wq;
 
 struct virtio_blk
 {
-spinlock_t lock;
-
 struct virtio_device *vdev;
 struct virtqueue *vq;
 
< at >< at > -65,7 +63,7 < at >< at > 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;
 
< at >< at > -99,7 +97,7 < at >< at > 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,
< at >< at > -431,7 +429,6 < at >< at > 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);
< at >< at > -456,7 +453,7 < at >< at > 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;

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

Asias He — 2012-05-25T07:23:12

Hmm. Why using the internal lock will have impact on the performance? 
Anyway I will update the commit log.


yes ;-)


See the patch I sent this morning.

[PATCH] block: Fix lock unbalance caused by lock disconnect



Yes. I got your point. I am trying to fix block layer as well.


I think so.

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

Michael S. Tsirkin — 2012-05-25T07:08:02


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

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

Michael S. Tsirkin — 2012-05-25T07:07:48


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

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

Michael S. Tsirkin — 2012-05-25T07:06:01

I see, thanks for the clarification.

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

Michael S. Tsirkin — 2012-05-25T07:02:45

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 ...



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?

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

Asias He — 2012-05-25T07:03:16

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.

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

Michael S. Tsirkin — 2012-05-25T06:52:29

Device is not reset here yet, so it will process
some requests in parallel with blk_cleanup_queue.
Is this a problem? Why not?

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

Asias He — 2012-05-25T02:34:49

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
< at >< at > -21,8 +21,6 < at >< at > struct workqueue_struct *virtblk_wq;
 
 struct virtio_blk
 {
-spinlock_t lock;
-
 struct virtio_device *vdev;
 struct virtqueue *vq;
 
< at >< at > -65,7 +63,7 < at >< at > 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;
 
< at >< at > -99,7 +97,7 < at >< at > 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,
< at >< at > -431,7 +429,6 < at >< at > 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);
< at >< at > -456,7 +453,7 < at >< at > 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;

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

Asias He — 2012-05-25T02:34:48

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
< at >< at > -576,8 +576,6 < at >< at > 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);
< at >< at > -585,21 +583,13 < at >< at > 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);

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

Asias He — 2012-05-25T02:34:47

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
< at >< at > -584,13 +584,13 < at >< at > 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))) {

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

Asias He — 2012-05-25T02:34:46

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(-)

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

Asias He — 2012-05-23T15:04:18



Well. I think I can do the cleanup in virtio driver without introducing 
a new API now. We can reset the device after blk_cleanup_queue so that 
the device can complete all the requests before queue DEAD marking. This 
would be much simpler.

Thanks for pointing out, Tejun ;-)

Re: [PATCH RESENT] xen: do not disable netfront in dom0

David Miller — 2012-05-22T20:50:47

From: Marek Marczykowski <marmarek< at >invisiblethingslab.com>
Date: Sun, 20 May 2012 13:45:10 +0200


Applied, thanks.

[PATCH RESENT] xen: do not disable netfront in dom0

Marek Marczykowski — 2012-05-20T11:45:10

Netfront driver can be also useful in dom0, eg when all NICs are assigned to
some domU (aka driver domain). Then using netback in domU and netfront in dom0
is the only way to get network access in dom0.

Signed-off-by: Marek Marczykowski <marmarek< at >invisiblethingslab.com>
Acked-by: Ian Campbell <ian.campbell< at >citrix.com>
Acked-by: Konrad Rzeszutek Wilk <konrad.wilk< at >oracle.com>
---
 drivers/net/xen-netfront.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 0ebbb19..2027afe 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
< at >< at > -1962,9 +1962,6 < at >< at > static int __init netif_init(void)
 if (!xen_domain())
 return -ENODEV;
 
-if (xen_initial_domain())
-return 0;
-
 if (xen_hvm_domain() && !xen_platform_pci_unplug)
 return -ENODEV;
 
< at >< at > -1977,9 +1974,6 < at >< at > module_init(netif_init);
 
 static void __exit netif_exit(void)
 {
-if (xen_initial_domain())
-return;
-
 xenbus_unregister_driver(&netfront_driver);
 }
 module_exit(netif_exit);

Re: [Xen-devel] [PATCH] xen: do not disable netfront in dom0

Konrad Rzeszutek Wilk — 2012-05-22T19:43:19

Oh no! Marek, can you repin it please (along with all the Ack's on it).

Re: [Xen-devel] [PATCH] xen: do not disable netfront in dom0

David Miller — 2012-05-22T19:38:47

From: Ian Campbell <Ian.Campbell< at >citrix.com>
Date: Tue, 22 May 2012 20:30:28 +0100


Ok, but this patch doesn't appply cleanly at all to Linus's
current tree nor my 'net' tree (which are equal right now).

Re: [Xen-devel] [PATCH] xen: do not disable netfront in dom0

Ian Campbell — 2012-05-22T19:30:28

Konrad is "Xen folks" and has acked it already but FWIW:

Acked-by: Ian Campbell <ian.campbell< at >citrix.com>

Ian.

Re: [PATCH] xen: do not disable netfront in dom0

David Miller — 2012-05-22T19:13:54

From: Marek Marczykowski <marmarek< at >invisiblethingslab.com>
Date: Sun, 20 May 2012 13:45:10 +0200


Someone please review this and I can merge it in via the 'net' tree if
it looks OK to XEN folks.

Re: [PATCH] xen: do not disable netfront in dom0

Konrad Rzeszutek Wilk — 2012-05-22T18:34:33

Acked-by: Konrad Rzeszutek Wilk <konrad.wilk< at >oracle.com>