On 05/22/2012 04:59 AM, Michael S. Tsirkin wrote:
On Mon, May 21, 2012 at 05:08:33PM +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.

Are you saying any driver with its own spinlock is
broken if hotunplugged under stress?

Hi, Michael

I can not say that. It is very hard to find real hardware device to try this. I tried on qemu with LSI Logic / Symbios Logic 53c895a scsi disk with hot-unplug. It is completely broken. And IDE does not support hotplug at all.

Do you see any downside of using 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

Cc: Rusty Russell<ru...@rustcorp.com.au>
Cc: "Michael S. Tsirkin"<m...@redhat.com>
Cc: virtualizat...@lists.linux-foundation.org
Cc: kvm@vger.kernel.org
Signed-off-by: Asias He<as...@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 ba35509..0c2f0e8 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,
@@ -456,7 +454,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);
@@ -481,7 +478,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.1


--
Asias
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to