* Re: [PATCH] xen: do not disable netfront in dom0
From: Konrad Rzeszutek Wilk @ 2012-05-22 18:34 UTC (permalink / raw)
To: Marek Marczykowski, davem
Cc: netdev, Jeremy Fitzhardinge, virtualization, linux-kernel,
xen-devel
In-Reply-To: <20120522130558.D828E6C7@duch.mimuw.edu.pl>
On Sun, May 20, 2012 at 01:45:10PM +0200, Marek Marczykowski wrote:
> 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@invisiblethingslab.com>
Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@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 698b905..e31ebff 100644
> --- a/drivers/net/xen-netfront.c
> +++ b/drivers/net/xen-netfront.c
> @@ -1953,9 +1953,6 @@ static int __init netif_init(void)
> if (!xen_domain())
> return -ENODEV;
>
> - if (xen_initial_domain())
> - return 0;
> -
> printk(KERN_INFO "Initialising Xen virtual ethernet driver.\n");
>
> return xenbus_register_frontend(&netfront_driver);
> @@ -1965,9 +1962,6 @@ module_init(netif_init);
>
> static void __exit netif_exit(void)
> {
> - if (xen_initial_domain())
> - return;
> -
> xenbus_unregister_driver(&netfront_driver);
> }
> module_exit(netif_exit);
> --
> 1.7.4.4
^ permalink raw reply
* Re: [PATCH] xen: do not disable netfront in dom0
From: David Miller @ 2012-05-22 19:13 UTC (permalink / raw)
To: marmarek
Cc: jeremy, konrad.wilk, netdev, linux-kernel, virtualization,
xen-devel
In-Reply-To: <20120522130558.D828E6C7@duch.mimuw.edu.pl>
From: Marek Marczykowski <marmarek@invisiblethingslab.com>
Date: Sun, 20 May 2012 13:45:10 +0200
> 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@invisiblethingslab.com>
Someone please review this and I can merge it in via the 'net' tree if
it looks OK to XEN folks.
^ permalink raw reply
* Re: [Xen-devel] [PATCH] xen: do not disable netfront in dom0
From: Ian Campbell @ 2012-05-22 19:30 UTC (permalink / raw)
To: David Miller
Cc: jeremy@goop.org, konrad.wilk@oracle.com, netdev@vger.kernel.org,
marmarek@invisiblethingslab.com,
virtualization@lists.linux-foundation.org,
xen-devel@lists.xen.org, linux-kernel@vger.kernel.org
In-Reply-To: <20120522.151354.2131168749992255398.davem@davemloft.net>
On Tue, 2012-05-22 at 20:13 +0100, David Miller wrote:
> From: Marek Marczykowski <marmarek@invisiblethingslab.com>
> Date: Sun, 20 May 2012 13:45:10 +0200
>
> > 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@invisiblethingslab.com>
>
> Someone please review this and I can merge it in via the 'net' tree if
> it looks OK to XEN folks.
Konrad is "Xen folks" and has acked it already but FWIW:
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Ian.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply
* Re: [Xen-devel] [PATCH] xen: do not disable netfront in dom0
From: David Miller @ 2012-05-22 19:38 UTC (permalink / raw)
To: Ian.Campbell
Cc: jeremy, konrad.wilk, netdev, marmarek, virtualization, xen-devel,
linux-kernel
In-Reply-To: <1337715028.3991.2.camel@dagon.hellion.org.uk>
From: Ian Campbell <Ian.Campbell@citrix.com>
Date: Tue, 22 May 2012 20:30:28 +0100
> On Tue, 2012-05-22 at 20:13 +0100, David Miller wrote:
>> From: Marek Marczykowski <marmarek@invisiblethingslab.com>
>> Date: Sun, 20 May 2012 13:45:10 +0200
>>
>> > 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@invisiblethingslab.com>
>>
>> Someone please review this and I can merge it in via the 'net' tree if
>> it looks OK to XEN folks.
>
> Konrad is "Xen folks" and has acked it already but FWIW:
>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
Ok, but this patch doesn't appply cleanly at all to Linus's
current tree nor my 'net' tree (which are equal right now).
^ permalink raw reply
* Re: [Xen-devel] [PATCH] xen: do not disable netfront in dom0
From: Konrad Rzeszutek Wilk @ 2012-05-22 19:43 UTC (permalink / raw)
To: David Miller
Cc: jeremy, Ian.Campbell, netdev, marmarek, virtualization, xen-devel,
linux-kernel
In-Reply-To: <20120522.153847.2107186222464601466.davem@davemloft.net>
On Tue, May 22, 2012 at 03:38:47PM -0400, David Miller wrote:
> From: Ian Campbell <Ian.Campbell@citrix.com>
> Date: Tue, 22 May 2012 20:30:28 +0100
>
> > On Tue, 2012-05-22 at 20:13 +0100, David Miller wrote:
> >> From: Marek Marczykowski <marmarek@invisiblethingslab.com>
> >> Date: Sun, 20 May 2012 13:45:10 +0200
> >>
> >> > 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@invisiblethingslab.com>
> >>
> >> Someone please review this and I can merge it in via the 'net' tree if
> >> it looks OK to XEN folks.
> >
> > Konrad is "Xen folks" and has acked it already but FWIW:
> >
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> Ok, but this patch doesn't appply cleanly at all to Linus's
> current tree nor my 'net' tree (which are equal right now).
Oh no! Marek, can you repin it please (along with all the Ack's on it).
^ permalink raw reply
* Re: [PATCH RESENT] xen: do not disable netfront in dom0
From: David Miller @ 2012-05-22 20:50 UTC (permalink / raw)
To: marmarek
Cc: jeremy, Ian.Campbell, konrad.wilk, netdev, linux-kernel,
virtualization, xen-devel
In-Reply-To: <20120522204742.29E8A626@duch.mimuw.edu.pl>
From: Marek Marczykowski <marmarek@invisiblethingslab.com>
Date: Sun, 20 May 2012 13:45:10 +0200
> 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@invisiblethingslab.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Applied, thanks.
^ permalink raw reply
* Re: [RFC PATCH 1/5] block: Introduce q->abort_queue_fn()
From: Asias He @ 2012-05-23 15:04 UTC (permalink / raw)
To: Tejun Heo
Cc: Jens Axboe, kvm, Michael S. Tsirkin, virtualization,
linux-fsdevel
In-Reply-To: <20120522151441.GB14339@google.com>
On 05/22/2012 11:14 PM, Tejun Heo wrote:
> Hello,
>
> On Tue, May 22, 2012 at 03:30:37PM +0800, Asias He wrote:
>> On 05/21/2012 11:42 PM, Tejun Heo wrote:
>> 1) if the queue is stopped, q->request_fn() will never call called.
>> we will be stuck in the loop forever. This can happen if the remove
>> method is called after the q->request_fn() calls blk_stop_queue() to
>> stop the queue when the device is full, and before the device
>> interrupt handler to start the queue. This can be fixed by calling
>> blk_start_queue() before __blk_run_queue(q).
>>
>> blk_drain_queue() {
>> while(true) {
>> ...
>> if (!list_empty(&q->queue_head))
>> __blk_run_queue(q);
>> ...
>> }
>> }
>
> Wouldn't that be properly fixed by making queue cleanup override
> stopped state?
>
>> 2) Since the device is gonna be removed, is it safe to rely on the
>> device to finish the request before the DEAD marking? E.g, In
>> vritio-blk, We reset the device and thus disable the interrupt
>> before we call blk_cleanup_queue(). I also suspect that the real
>> hardware can finish the pending requests when being hot-unplugged.
>
> Yes, it should be safe (otherwise it's a driver bug). Device driver
> already knows the state of the device it is driving. If the device
> can't service requests for whatever reason, the device driver should
> abort any in-flight and future requests. That's how other block
> drivers behave and I don't see why virtio should be any different.
> Also, blk_drain_queue() is used for other purposes too - elevator
> switch and blkcg policy changes. You definitely don't want to be
> aborting requests across those events.
> So, NACK.
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 ;-)
--
Asias
^ permalink raw reply
* [PATCH 0/3] Fix hot-unplug race in virtio-blk
From: Asias He @ 2012-05-25 2:34 UTC (permalink / raw)
To: virtualization, Rusty Russell, Michael S. Tsirkin; +Cc: kvm
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
^ permalink raw reply
* [PATCH 1/3] virtio-blk: Call del_gendisk() before disable guest kick
From: Asias He @ 2012-05-25 2:34 UTC (permalink / raw)
To: virtualization, Rusty Russell, Michael S. Tsirkin; +Cc: kvm
In-Reply-To: <1337913289-23486-1-git-send-email-asias@redhat.com>
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
* [PATCH 2/3] virtio-blk: Reset device after blk_cleanup_queue()
From: Asias He @ 2012-05-25 2:34 UTC (permalink / raw)
To: virtualization, Rusty Russell, Michael S. Tsirkin; +Cc: kvm
In-Reply-To: <1337913289-23486-1-git-send-email-asias@redhat.com>
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
* [PATCH 3/3] virtio-blk: Use block layer provided spinlock
From: Asias He @ 2012-05-25 2:34 UTC (permalink / raw)
To: virtualization, Rusty Russell, Michael S. Tsirkin; +Cc: kvm
In-Reply-To: <1337913289-23486-1-git-send-email-asias@redhat.com>
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
* Re: [PATCH 2/3] virtio-blk: Reset device after blk_cleanup_queue()
From: Michael S. Tsirkin @ 2012-05-25 6:52 UTC (permalink / raw)
To: Asias He; +Cc: kvm, virtualization
In-Reply-To: <1337913289-23486-3-git-send-email-asias@redhat.com>
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
* Re: [PATCH 3/3] virtio-blk: Use block layer provided spinlock
From: Michael S. Tsirkin @ 2012-05-25 7:02 UTC (permalink / raw)
To: Asias He; +Cc: kvm, virtualization
In-Reply-To: <1337913289-23486-4-git-send-email-asias@redhat.com>
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
* Re: [PATCH 2/3] virtio-blk: Reset device after blk_cleanup_queue()
From: Asias He @ 2012-05-25 7:03 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, virtualization
In-Reply-To: <20120525065229.GD15474@redhat.com>
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
* Re: [PATCH 2/3] virtio-blk: Reset device after blk_cleanup_queue()
From: Michael S. Tsirkin @ 2012-05-25 7:06 UTC (permalink / raw)
To: Asias He; +Cc: kvm, virtualization
In-Reply-To: <4FBF2EB4.3020702@redhat.com>
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
* Re: [PATCH 1/3] virtio-blk: Call del_gendisk() before disable guest kick
From: Michael S. Tsirkin @ 2012-05-25 7:07 UTC (permalink / raw)
To: Asias He; +Cc: kvm, virtualization
In-Reply-To: <1337913289-23486-2-git-send-email-asias@redhat.com>
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
* Re: [PATCH 2/3] virtio-blk: Reset device after blk_cleanup_queue()
From: Michael S. Tsirkin @ 2012-05-25 7:08 UTC (permalink / raw)
To: Asias He; +Cc: kvm, virtualization
In-Reply-To: <1337913289-23486-3-git-send-email-asias@redhat.com>
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
* Re: [PATCH 3/3] virtio-blk: Use block layer provided spinlock
From: Asias He @ 2012-05-25 7:23 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, virtualization
In-Reply-To: <20120525070245.GE15474@redhat.com>
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
* [PATCH v2 3/3] virtio-blk: Use block layer provided spinlock
From: Asias He @ 2012-05-25 8:03 UTC (permalink / raw)
To: virtualization, Rusty Russell, Michael S. Tsirkin; +Cc: kvm
In-Reply-To: <20120525070245.GE15474@redhat.com>
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
* Re: [PATCH v2 3/3] virtio-blk: Use block layer provided spinlock
From: Michael S. Tsirkin @ 2012-05-25 13:10 UTC (permalink / raw)
To: Asias He; +Cc: kvm, virtualization
In-Reply-To: <1337933007-30634-1-git-send-email-asias@redhat.com>
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
* CFP: 8th IEEE International Conference on eScience -- Chicago IL USA, October 8th-12 2012
From: Ioan Raicu @ 2012-05-27 23:56 UTC (permalink / raw)
To: virtualization
CALL FOR PAPERS
8th IEEE International Conference on eScience
http://www.ci.uchicago.edu/escience2012/
October 8-12, 2012
Chicago, IL, USA
Researchers in all disciplines are increasingly adopting digital tools,
techniques and practices, often in
communities and projects that span disciplines, laboratories,
organizations, and national boundaries.
The eScience 2012 conference is designed to bring together leading
international and interdisciplinary
research communities, developers, and users of eScience applications and
enabling IT technologies. The
conference serves as a forum to present the results of the latest
applications research and product/tool
developments and to highlight related activities from around the world.
Also, we are now entering the
second decade of eScience and the 2012 conference gives an opportunity
to take stock of what has
been achieved so far and look forward to the challenges and
opportunities the next decade will bring.
A special emphasis of the 2012 conference is on advances in the
application of technology in a
particular discipline. Accordingly, significant advances in applications
science and technology will be
considered as important as the development of new technologies
themselves. Further, we welcome
contributions in educational activities under any of these disciplines.
As a result, the conference will be structured around two e-Science tracks:
• eScience Algorithms and Applications
• eScience application areas, including:
• Physical sciences
• Biomedical sciences
• Social sciences and humanities
• Data-oriented approaches and applications
• Compute-oriented approaches and applications
• Extreme scale approaches and applications
• Cyberinfrastructure to support eScience
• Novel hardware
• Novel uses of production infrastructure
• Software and services
• Tools
The conference proceedings will be published by the IEEE Computer
Society Press, USA and will be made
available online through the IEEE Digital Library. Selected papers will
be invited to submit extended
versions to a special issue of the Future Generation Computer Systems
(FGCS)journal.
SUBMISSION PROCESS
Authors are invited to submit papers with unpublished, original work of
not more than 8 pages of
double column text using single spaced 10 point size on 8.5 x 11 inch
pages, as per IEEE 8.5 x 11
manuscript guidelines. (Up to 2 additional pages may be purchased for
US$150/page)
Templates are available
from
http://www.ieee.org/conferences_events/conferences/publishing/templates.html.
Authors should submit a PDF file that will print on a PostScript printer
to https://www.easychair.org/conferences/?conf=escience2012
(Note that paper submitters also must submit an abstract in advance of
the paper deadline. This should
be done through the same site where papers are submitted.)
It is a requirement that at least one author of each accepted paper
attend the conference.
IMPORTANT DATES
Abstract submission (required): 4 July 2012
Paper submission: 11 July 2012
Paper author notification: 22 August 2012
Camera-ready papers due: 10 September 2012
Conference: 8-12 October 2012
CONFERENCE ORGANIZATION
General Chair
Ian Foster, University of Chicago & Argonne National Laboratory, USA
Program Co-Chairs
Daniel S. Katz, University of Chicago & Argonne National Laboratory, USA
Heinz Stockinger, SIB Swiss Institute of Bioinformatics, Switzerland
Program Vice Co-Chairs
eScience Algorithms and Applications Track
David Abramson, Monash University, Australia
Gabrielle Allen, Louisiana State University, USA
Cyberinfrastructure to support eScience Track
Rosa M. Badia, Barcelona Supercomputing Center / CSIC, Spain
Geoffrey Fox, Indiana University, USA
Sponsorship Chair
Charlie Catlett, Argonne National Laboratory, USA
Conference Manager and Finance Chair
Julie Wulf-Knoerzer, University of Chicago & Argonne National
Laboratory, USA
Publicity Chairs
Kento Aida, National Institute of Informatics, Japan
Ioan Raicu, Illinois Institute of Technology, USA
David Wallom, Oxford e-Research Centre, UK
Local Organizing Committee
Ninfa Mayorga, University of Chicago, USA
Evelyn Rayburn, University of Chicago, USA
Lynn Valentini, Argonne National Laboratory, USA
Program Committee
eScience Algorithms and Applications Track
Srinivas Aluru, Iowa State University, USA
Ashiq Anjum, University of Derby, UK
David A. Bader, Georgia Institute of Technology, USA
Jon Blower, University of Reading, UK
Paul Bonnington, Monash University, Australia
Simon Cox, University of Southampton, UK
David De Roure, Oxford e-Research Centre, UK
George Djorgovski, California Institute of Technology, USA
Anshu Dubey, University of Chicago & Argonne National Laboratory, USA
Yuri Estrin, Monash University, Australia
Dan Fay, Microsoft, USA
Jeremy Frey, University of Southampton, UK
Wolfgang Gentzsch, HPC Consultant, Germany
Lutz Gross, The University of Queensland, Austrialia
Sverker Holmgren, Uppsala University, Sweden
Bill Howe, University of Washington, USA
Marina Jirotka, University of Oxford, UK
Timoleon Kipouros, University of Cambridge, UK
Kerstin Kleese van Dam, Pacific Northwest National Laboratory, USA
Arun S. Konagurthu, Monash University, Australia
Peter Kunszt, SystemsX.ch, Switzerland
Alexey Lastovetsky, University College Dublin, Ireland
Andrew Lewis, Griffith University, Australia
Sergio Maffioletti, University of Zurich, Switzerland
Amitava Majumdar, San Diego Supercomputer Center, University of
California at San Diego, USA
Rui Mao, Shenzhen University, China
Madhav V. Marathe, Virginia Tech, USA
Maryann Martone, University of California at San Diego, USA
Louis Moresi, Monash University, Australia
Riccardo Murri, University of Zurich, Switzerland
Silvia D. Olabarriaga, Academic Medical Center of the University of
Amsterdam, Netherlands
Enrique S. Quintana-Ortí, Universidad Jaume I, Spain
Abani Patra, University at Buffalo, USA
Rob Pennington, NSF, USA
Andrew Perry, Monash University, Australia
Beth Plale, Indiana University, USA
Michael Resch, University of Stuttgart, Germany
Adrian Sandu, Virginia Tech, USA
Mark Savill, Cranfield University, UK
Erik Schnetter, Perimeter Institute for Theoretical Physics, Canada
Edward Seidel, Louisiana State University, USA
Suzanne M. Shontz, The Pennsylvania State University, USA
David Skinner, Lawrence Berkeley National Laboratory, USA
Alan Sussman, University of Maryland, USA
Alex Szalay, Johns Hopkins University, USA
Domenico Talia, ICAR-CNR & University of Calabria, Italy
Jian Tao, Louisiana State University, USA
David Wallom, Oxford e-Research Centre, UK
Shaowen Wang, University of Illinois at Urbana-Champaign, USA
Michael Wilde, Argonne National Laboratory & University of Chicago, USA
Nancy Wilkins-Diehr, San Diego Supercomputer Center, University of
California at San Diego, USA
Wu Zhang, Shanghai University, China
Yunquan Zhang, Chinese Academy of Sciences, China
Cyberinfrastructure to support eScience Track
Deb Agarwal, Lawrence Berkeley National Laboratory, USA
Ilkay Altintas, San Diego Supercomputer Center, University of California
at San Diego, USA
Henri Bal, Vrije Universiteit, Netherlands
Roger Barga, Microsoft, USA
Martin Berzins, University of Utah, USA
John Brooke, University of Manchester, UK
Thomas Fahringer, University of Innsbruck, Austria
Gilles Fedak, INRIA, France
José A. B. Fortes, University of Florida, USA
Yolanda Gil, ISI/USC, USA
Madhusudhan Govindaraju, SUNY Binghamton, USA
Thomas Hacker, Purdue University, USA
Ken Hawick, Massey University, New Zealand
Marty Humphrey, University of Virginia, USA
Hai Jin, Huazhong University of Science and Technology, China
Thilo Kielmann, Vrije Universiteit, Netherlands
Scott Klasky, Oak Ridge National Laboratory, USA
Isao Kojima, AIST, Japan
Tevfik Kosar, University at Buffalo, USA
Dieter Kranzlmueller, LMU & LRZ Munich, Germany
Erwin Laure, KTH, Sweden
Jysoo Lee, KISTI, Korea
Li Xiaoming, Peking University, China
Bertram Ludäscher, University of California, Davis, USA
Andrew Lumsdaine, Indiana University, USA
Tanu Malik, University of Chicago, USA
Satoshi Matsuoka, Tokyo Institute of Technology, Japan
Reagan Moore, University of North Carolina at Chapel Hill, USA
Shirley Moore, University of Kentucky, USA
Steven Newhouse, EGI, Netherlands
Dhabaleswar K. (DK) Panda, The Ohio State University, USA
Manish Parashar, Rutgers University, USA
Ron Perrott, University of Oxford, UK
Depei Qian, Beihang University, China
Judy Qui, Indiana University, USA
Ioan Raicu, Illinois Institute of Technology, USA
Lavanya Ramakrishnan, Lawrence Berkeley National Laboratory, USA
Omer Rana, Cardiff University, UK
Paul Roe, Queensland University of Technology, Australia
Bruno Schulze, LNCC, Brazil
Marc Snir, Argonne National Laboratory & University of Illinois at
Urbana-Champaign, USA
Xian-He Sun, Illinois Institute of Technology, USA
Yoshio Tanaka, AIST, Japan
Michela Taufer, University of Delaware, USA
Kerry Taylor, CSIRO, Australia
Douglas Thain, University of Notre Dame, USA
Paul Watson, Newcastle University, UK
Jun Zhao, University of Oxford, UK
--
=================================================================
Ioan Raicu, Ph.D.
Assistant Professor, Illinois Institute of Technology (IIT)
Guest Research Faculty, Argonne National Laboratory (ANL)
=================================================================
Data-Intensive Distributed Systems Laboratory, CS/IIT
Distributed Systems Laboratory, MCS/ANL
=================================================================
Cel: 1-847-722-0876
Office: 1-312-567-5704
Email: iraicu@cs.iit.edu
Web: http://www.cs.iit.edu/~iraicu/
Web: http://datasys.cs.iit.edu/
=================================================================
=================================================================
^ permalink raw reply
* [PATCH 0/5] virtio: rng: fixes
From: Amit Shah @ 2012-05-28 6:48 UTC (permalink / raw)
To: Rusty Russell; +Cc: Amit Shah, Virtualization List
Hi Rusty,
These are a few fixes for the virtio-rng driver. These were tested
using the not-yet-upstream virtio-rng device patch to qemu:
http://thread.gmane.org/gmane.comp.emulators.qemu/152668
Please apply.
Amit Shah (5):
virtio ids: fix comment for virtio-rng
virtio: rng: allow tasks to be killed that are waiting for rng input
virtio: rng: don't wait on host when module is going away
virtio: rng: split out common code in probe / remove for s3/s4 ops
virtio: rng: s3/s4 support
drivers/char/hw_random/virtio-rng.c | 37 ++++++++++++++++++++++++++++++++--
include/linux/virtio_ids.h | 2 +-
2 files changed, 35 insertions(+), 4 deletions(-)
--
1.7.7.6
^ permalink raw reply
* [PATCH 1/5] virtio ids: fix comment for virtio-rng
From: Amit Shah @ 2012-05-28 6:48 UTC (permalink / raw)
To: Rusty Russell; +Cc: Amit Shah, Virtualization List
In-Reply-To: <cover.1338187342.git.amit.shah@redhat.com>
It's virtio-rng, not virtio-ring.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
include/linux/virtio_ids.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/linux/virtio_ids.h b/include/linux/virtio_ids.h
index 7529b85..270fb22 100644
--- a/include/linux/virtio_ids.h
+++ b/include/linux/virtio_ids.h
@@ -32,7 +32,7 @@
#define VIRTIO_ID_NET 1 /* virtio net */
#define VIRTIO_ID_BLOCK 2 /* virtio block */
#define VIRTIO_ID_CONSOLE 3 /* virtio console */
-#define VIRTIO_ID_RNG 4 /* virtio ring */
+#define VIRTIO_ID_RNG 4 /* virtio rng */
#define VIRTIO_ID_BALLOON 5 /* virtio balloon */
#define VIRTIO_ID_RPMSG 7 /* virtio remote processor messaging */
#define VIRTIO_ID_SCSI 8 /* virtio scsi */
--
1.7.7.6
^ permalink raw reply related
* [PATCH 2/5] virtio: rng: allow tasks to be killed that are waiting for rng input
From: Amit Shah @ 2012-05-28 6:48 UTC (permalink / raw)
To: Rusty Russell; +Cc: Amit Shah, Virtualization List
In-Reply-To: <cover.1338187342.git.amit.shah@redhat.com>
Use wait_for_completion_killable() instead of wait_for_completion() when
waiting for the host to send us entropy. Without this,
# cat /dev/hwrng
^C
just hangs.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/hw_random/virtio-rng.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index 723725b..c8a9350 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -55,6 +55,7 @@ static void register_buffer(u8 *buf, size_t size)
static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
{
+ int ret;
if (!busy) {
busy = true;
@@ -65,7 +66,9 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
if (!wait)
return 0;
- wait_for_completion(&have_data);
+ ret = wait_for_completion_killable(&have_data);
+ if (ret < 0)
+ return ret;
busy = false;
--
1.7.7.6
^ permalink raw reply related
* [PATCH 3/5] virtio: rng: don't wait on host when module is going away
From: Amit Shah @ 2012-05-28 6:48 UTC (permalink / raw)
To: Rusty Russell; +Cc: Amit Shah, Virtualization List
In-Reply-To: <cover.1338187342.git.amit.shah@redhat.com>
No use waiting for input from host when the module is being removed.
We're going to remove the vq in the next step anyway, so just perform
any other steps for cleanup (currently none).
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/hw_random/virtio-rng.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index c8a9350..2dc9ce1 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -109,6 +109,7 @@ static int virtrng_probe(struct virtio_device *vdev)
static void __devexit virtrng_remove(struct virtio_device *vdev)
{
vdev->config->reset(vdev);
+ busy = false;
hwrng_unregister(&virtio_hwrng);
vdev->config->del_vqs(vdev);
}
--
1.7.7.6
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox