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

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

[Asias He]

When user hot-unplug a disk which is busy serving I/O, __blk_run_queue
might be unable to drain all the requests. As a result, the
blk_drain_queue() would loop forever and blk_cleanup_queue would not
return. So hot-unplug will fail.

This patch adds a callback in blk_drain_queue() for low lever driver to
abort requests.

Currently, this is useful for virtio-blk to do cleanup in hot-unplug.

Cc: Jens Axboe <axboe@kernel.dk>
Cc: Tejun Heo <tj@kernel.org>
Cc: linux-fsdevel@vger.kernel.org
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>
---
 block/blk-core.c       |    3 +++
 block/blk-settings.c   |   12 ++++++++++++
 include/linux/blkdev.h |    3 +++
 3 files changed, 18 insertions(+)

diff --git a/block/blk-core.c b/block/blk-core.c
index 1f61b74..ca42fd7 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -369,6 +369,9 @@ void blk_drain_queue(struct request_queue *q, bool drain_all)
 		if (drain_all)
 			blk_throtl_drain(q);
 
+		if (q->abort_queue_fn)
+			q->abort_queue_fn(q);
+
 		/*
 		 * This function might be called on a queue which failed
 		 * driver init after queue creation.  Some drivers
diff --git a/block/blk-settings.c b/block/blk-settings.c
index d3234fc..83ccb48 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -100,6 +100,18 @@ void blk_queue_lld_busy(struct request_queue *q, lld_busy_fn *fn)
 EXPORT_SYMBOL_GPL(blk_queue_lld_busy);
 
 /**
+ * blk_queue_abort_queue - set driver specific abort function
+ * @q:		queue
+ * @mbfn:	abort_queue_fn
+ */
+void blk_queue_abort_queue(struct request_queue *q, abort_queue_fn *afn)
+{
+	q->abort_queue_fn = afn;
+}
+EXPORT_SYMBOL(blk_queue_abort_queue);
+
+
+/**
  * blk_set_default_limits - reset limits to default values
  * @lim:  the queue_limits structure to reset
  *
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 2aa2466..e2d58bd 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -200,6 +200,7 @@ struct request_pm_state
 
 typedef void (request_fn_proc) (struct request_queue *q);
 typedef void (make_request_fn) (struct request_queue *q, struct bio *bio);
+typedef void (abort_queue_fn) (struct request_queue *q);
 typedef int (prep_rq_fn) (struct request_queue *, struct request *);
 typedef void (unprep_rq_fn) (struct request_queue *, struct request *);
 
@@ -282,6 +283,7 @@ struct request_queue {
 
 	request_fn_proc		*request_fn;
 	make_request_fn		*make_request_fn;
+	abort_queue_fn		*abort_queue_fn;
 	prep_rq_fn		*prep_rq_fn;
 	unprep_rq_fn		*unprep_rq_fn;
 	merge_bvec_fn		*merge_bvec_fn;
@@ -821,6 +823,7 @@ extern struct request_queue *blk_init_allocated_queue(struct request_queue *,
 						      request_fn_proc *, spinlock_t *);
 extern void blk_cleanup_queue(struct request_queue *);
 extern void blk_queue_make_request(struct request_queue *, make_request_fn *);
+extern void blk_queue_abort_queue(struct request_queue *, abort_queue_fn *);
 extern void blk_queue_bounce_limit(struct request_queue *, u64);
 extern void blk_limits_max_hw_sectors(struct queue_limits *, unsigned int);
 extern void blk_queue_max_hw_sectors(struct request_queue *, unsigned int);
-- 
1.7.10.1

[RFC PATCH 2/5] block: Do not stop draining if waitqueue is not empty.

[Asias He]

If there are processes still in the wait queue, keep draining,
otherwise these processes would be in D state forever.

I noticed this situation:
q->rq.count[0] == 0, q->rq.count[1] == 0, however wait queue
q->rq.wait[0].task_list and q->rq.wait[1].task_list are not empty.

Cc: Jens Axboe <axboe@kernel.dk>
Cc: Tejun Heo <tj@kernel.org>
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Asias He <asias@redhat.com>
---
 block/blk-core.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/block/blk-core.c b/block/blk-core.c
index ca42fd7..2c2b585 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -394,6 +394,7 @@ void blk_drain_queue(struct request_queue *q, bool drain_all)
 				drain |= q->rq.count[i];
 				drain |= q->in_flight[i];
 				drain |= !list_empty(&q->flush_queue[i]);
+				drain |= waitqueue_active(&q->rq.wait[i]);
 			}
 		}
 
-- 
1.7.10.1

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

[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 |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 693187d..7d5f5b0 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -584,12 +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);
-- 
1.7.10.1

[RFC PATCH 4/5] virtio-blk: Use q->abort_queue_fn() to abort requests

[Asias He]

virtblk_abort_queue() will be called by the block layer when cleans up
the queue. blk_cleanup_queue -> blk_drain_queue() -> q->abort_queue_fn(q)

virtblk_abort_queue()
1) Abort requests in block which is not dispatched to driver
2) Abort requests already dispatched to driver
3) Wake up processes which is sleeping on get_request_wait()

This makes hot-unplug a disk which is busy serving I/O success.

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
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Tejun Heo <tj@kernel.org>
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Asias He <asias@redhat.com>
---
 drivers/block/virtio_blk.c |   38 +++++++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 11 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 7d5f5b0..ba35509 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -182,6 +182,31 @@ static bool do_req(struct request_queue *q, struct virtio_blk *vblk,
 	return true;
 }
 
+void virtblk_abort_queue(struct request_queue *q)
+{
+	struct virtio_blk *vblk = q->queuedata;
+	struct virtblk_req *vbr;
+	int i;
+
+	/* Abort requests in block layer. */
+	elv_abort_queue(q);
+
+	/* Abort requests dispatched to driver. */
+	while ((vbr = virtqueue_detach_unused_buf(vblk->vq))) {
+		vbr->req->cmd_flags |= REQ_QUIET;
+		__blk_end_request_all(vbr->req, -EIO);
+		mempool_free(vbr, vblk->pool);
+	}
+
+	/* Wake up threads sleeping on get_request_wait() */
+	for (i = 0; i < ARRAY_SIZE(q->rq.wait); i++) {
+		if (waitqueue_active(&q->rq.wait[i]))
+			wake_up_all(&q->rq.wait[i]);
+	}
+
+	return;
+}
+
 static void do_virtblk_request(struct request_queue *q)
 {
 	struct virtio_blk *vblk = q->queuedata;
@@ -462,6 +487,8 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
 		goto out_put_disk;
 	}
 
+	blk_queue_abort_queue(q, virtblk_abort_queue);
+
 	q->queuedata = vblk;
 
 	virtblk_name_format("vd", index, vblk->disk->disk_name, DISK_NAME_LEN);
@@ -576,8 +603,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);
@@ -591,15 +616,6 @@ static void __devexit virtblk_remove(struct virtio_device *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);
-- 
1.7.10.1

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

[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

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

Re: [RFC PATCH 2/5] block: Do not stop draining if waitqueue is not empty.

[Tejun Heo]

On Mon, May 21, 2012 at 05:08:30PM +0800, Asias He wrote:
> If there are processes still in the wait queue, keep draining,
> otherwise these processes would be in D state forever.
> 
> I noticed this situation:
> q->rq.count[0] == 0, q->rq.count[1] == 0, however wait queue
> q->rq.wait[0].task_list and q->rq.wait[1].task_list are not empty.
> 
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: linux-fsdevel@vger.kernel.org
> Signed-off-by: Asias He <asias@redhat.com>
> ---
>  block/blk-core.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index ca42fd7..2c2b585 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -394,6 +394,7 @@ void blk_drain_queue(struct request_queue *q, bool drain_all)
>  				drain |= q->rq.count[i];
>  				drain |= q->in_flight[i];
>  				drain |= !list_empty(&q->flush_queue[i]);
> +				drain |= waitqueue_active(&q->rq.wait[i]);

Hmm... how does that happen?  What do you mean "you noticed this
situation"?  Did that actually happen or you noticed such scenarios
would be possible?  If rl.wait[] isn't empty with zero rq count, the
queue would hang, so we should be fixing that situation instead of
working around it from cleanup_queue.

Thanks.

-- 
tejun

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

[Tejun Heo]

On Mon, May 21, 2012 at 05:08:29PM +0800, Asias He wrote:
> When user hot-unplug a disk which is busy serving I/O, __blk_run_queue
> might be unable to drain all the requests. As a result, the
> blk_drain_queue() would loop forever and blk_cleanup_queue would not
> return. So hot-unplug will fail.
> 
> This patch adds a callback in blk_drain_queue() for low lever driver to
> abort requests.
> 
> Currently, this is useful for virtio-blk to do cleanup in hot-unplug.

Why is this necessary?  virtio-blk should know that the device is gone
and fail in-flight / new commands.  That's what other drivers do.
What makes virtio-blk different?

Thanks.

-- 
tejun

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

[Michael S. Tsirkin]

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?

> =====================================
> [ 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 <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 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

Re: [RFC PATCH 2/5] block: Do not stop draining if waitqueue is not empty.

[Asias He]

On 05/21/2012 11:39 PM, Tejun Heo wrote:
> On Mon, May 21, 2012 at 05:08:30PM +0800, Asias He wrote:
>> If there are processes still in the wait queue, keep draining,
>> otherwise these processes would be in D state forever.
>>
>> I noticed this situation:
>> q->rq.count[0] == 0, q->rq.count[1] == 0, however wait queue
>> q->rq.wait[0].task_list and q->rq.wait[1].task_list are not empty.
>>
>> Cc: Jens Axboe<axboe@kernel.dk>
>> Cc: Tejun Heo<tj@kernel.org>
>> Cc: linux-fsdevel@vger.kernel.org
>> Signed-off-by: Asias He<asias@redhat.com>
>> ---
>>   block/blk-core.c |    1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index ca42fd7..2c2b585 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -394,6 +394,7 @@ void blk_drain_queue(struct request_queue *q, bool drain_all)
>>   				drain |= q->rq.count[i];
>>   				drain |= q->in_flight[i];
>>   				drain |= !list_empty(&q->flush_queue[i]);
>> +				drain |= waitqueue_active(&q->rq.wait[i]);
>
> Hmm... how does that happen?  What do you mean "you noticed this
> situation"?  Did that actually happen or you noticed such scenarios
> would be possible?  If rl.wait[] isn't empty with zero rq count, the
> queue would hang, so we should be fixing that situation instead of
> working around it from cleanup_queue.

Hi, Tejun

I actually saw this happened though it should not happen. I have no idea 
why this happens. Maybe unbalanced prepare_to_wait_exclusive() in 
get_request_wait() and wake_up() in __freed_request()?

With this happened, I saw some fio threads in D state which are sleeping 
on get_request_wait(). If I wake up the threads in the wait queue in 
q->abort_queue_fn() callback which i proposed in the 1/5 of this patch 
set, the queue cleanup and thus hot-unplug went pretty well. (Passed 
3000~ rounds of test, without this 2~ round of test would fail). See 
this patch [RFC PATCH 4/5] virtio-blk: Use q->abort_queue_fn() to abort.

-- 
Asias

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

[Asias He]

On 05/21/2012 11:42 PM, Tejun Heo wrote:
> On Mon, May 21, 2012 at 05:08:29PM +0800, Asias He wrote:
>> When user hot-unplug a disk which is busy serving I/O, __blk_run_queue
>> might be unable to drain all the requests. As a result, the
>> blk_drain_queue() would loop forever and blk_cleanup_queue would not
>> return. So hot-unplug will fail.
>>
>> This patch adds a callback in blk_drain_queue() for low lever driver to
>> abort requests.
>>
>> Currently, this is useful for virtio-blk to do cleanup in hot-unplug.
>
> Why is this necessary?  virtio-blk should know that the device is gone
> and fail in-flight / new commands.  That's what other drivers do.
> What makes virtio-blk different?

blk_cleanup_queue() relies on __blk_run_queue() to finish all the 
requests before DEAD marking, right?

There are two problems:

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);
       ...
    }
}

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.

So I proposed the q->abort_queue_fn() callback in blk_drain_queue() for 
the driver to abort the queue explicitly no mater how the device behaves.

BTW, do we have any infrastructure in block layer to track the requests 
already dispatched to driver. This might be useful for driver if it want 
to abort all of them. Otherwise the driver has to do it on their own.

-- 
Asias

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

[Asias He]

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

Re: [RFC PATCH 2/5] block: Do not stop draining if waitqueue is not empty.

[Tejun Heo]

Hello, Asias.

On Tue, May 22, 2012 at 02:48:55PM +0800, Asias He wrote:
> I actually saw this happened though it should not happen. I have no
> idea why this happens. Maybe unbalanced prepare_to_wait_exclusive()
> in get_request_wait() and wake_up() in __freed_request()?

Hmm.... unbalanced how?  I think bugs in this area are much more
likely to show up as live queue hang rather than issues during queue
shutdown.

> With this happened, I saw some fio threads in D state which are
> sleeping on get_request_wait(). If I wake up the threads in the wait
> queue in q->abort_queue_fn() callback which i proposed in the 1/5 of
> this patch set, the queue cleanup and thus hot-unplug went pretty
> well. (Passed 3000~ rounds of test, without this 2~ round of test
> would fail). See this patch [RFC PATCH 4/5] virtio-blk: Use
> q->abort_queue_fn() to abort.

If the problem is that easily reproducible (you mean that you can
reproduce it every other time, right?), it would be immensely helpful
if you can root cause the issue properly.  As it currently stands,
this series seems to work around the problem by adding extra API
without properly root-causing it.  Workarounds without proper
root-causing are already pretty bad and adding extra API for that is
rather silly, IMHO.

Thanks.

-- 
tejun

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

[Tejun Heo]

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.

Thanks.

-- 
tejun

Re: [RFC PATCH 2/5] block: Do not stop draining if waitqueue is not empty.

[Asias He]

On 05/22/2012 11:07 PM, Tejun Heo wrote:
> Hello, Asias.
>
> On Tue, May 22, 2012 at 02:48:55PM +0800, Asias He wrote:
>> I actually saw this happened though it should not happen. I have no
>> idea why this happens. Maybe unbalanced prepare_to_wait_exclusive()
>> in get_request_wait() and wake_up() in __freed_request()?
>
> Hmm.... unbalanced how?  I think bugs in this area are much more
> likely to show up as live queue hang rather than issues during queue
> shutdown.

I added some debug code to count the number of sleep and wakeup in 
get_request_wait() and __freed_request().

I found this after queue cleanup. rl->wait[] is not empty while 
rl->count[] == 0. There are exactly nr_sleep - nr_wakeup of process in D 
state. So missed wakeup happens? Any ideas to do more debug to find the 
root-cause?

[   52.917115] ---> nr_sleep=1046, nr_wakeup=873, delta=173

$ vmstat 1
  1 173  0 712640  24292  96172 0 0  0  0  419  757  0  0  0 100  0
  0 173  0 712764  24292  96180 0 0  0  0  472  725  0  1  0 97  2


>> With this happened, I saw some fio threads in D state which are
>> sleeping on get_request_wait(). If I wake up the threads in the wait
>> queue in q->abort_queue_fn() callback which i proposed in the 1/5 of
>> this patch set, the queue cleanup and thus hot-unplug went pretty
>> well. (Passed 3000~ rounds of test, without this 2~ round of test
>> would fail). See this patch [RFC PATCH 4/5] virtio-blk: Use
>> q->abort_queue_fn() to abort.
>
> If the problem is that easily reproducible (you mean that you can
> reproduce it every other time, right?), it would be immensely helpful
> if you can root cause the issue properly.  As it currently stands,
> this series seems to work around the problem by adding extra API
> without properly root-causing it.  Workarounds without proper
> root-causing are already pretty bad and adding extra API for that is
> rather silly, IMHO.

Yes. it is very easy to reproduce. /me Trying to figure the root cause out.

-- 
Asias

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

[Asias He]

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

Re: [RFC PATCH 2/5] block: Do not stop draining if waitqueue is not empty.

[Asias He]

Hi, Tejun and Jens

On 05/23/2012 10:54 PM, Asias He wrote:
> On 05/22/2012 11:07 PM, Tejun Heo wrote:
>> Hello, Asias.
>>
>> On Tue, May 22, 2012 at 02:48:55PM +0800, Asias He wrote:
>>> I actually saw this happened though it should not happen. I have no
>>> idea why this happens. Maybe unbalanced prepare_to_wait_exclusive()
>>> in get_request_wait() and wake_up() in __freed_request()?
>>
>> Hmm.... unbalanced how? I think bugs in this area are much more
>> likely to show up as live queue hang rather than issues during queue
>> shutdown.
>
> I added some debug code to count the number of sleep and wakeup in
> get_request_wait() and __freed_request().
>
> I found this after queue cleanup. rl->wait[] is not empty while
> rl->count[] == 0. There are exactly nr_sleep - nr_wakeup of process in D
> state. So missed wakeup happens? Any ideas to do more debug to find the
> root-cause?

Ping.

> [ 52.917115] ---> nr_sleep=1046, nr_wakeup=873, delta=173
>
> $ vmstat 1
> 1 173 0 712640 24292 96172 0 0 0 0 419 757 0 0 0 100 0
> 0 173 0 712764 24292 96180 0 0 0 0 472 725 0 1 0 97 2
>
>
>>> With this happened, I saw some fio threads in D state which are
>>> sleeping on get_request_wait(). If I wake up the threads in the wait
>>> queue in q->abort_queue_fn() callback which i proposed in the 1/5 of
>>> this patch set, the queue cleanup and thus hot-unplug went pretty
>>> well. (Passed 3000~ rounds of test, without this 2~ round of test
>>> would fail). See this patch [RFC PATCH 4/5] virtio-blk: Use
>>> q->abort_queue_fn() to abort.
>>
>> If the problem is that easily reproducible (you mean that you can
>> reproduce it every other time, right?), it would be immensely helpful
>> if you can root cause the issue properly. As it currently stands,
>> this series seems to work around the problem by adding extra API
>> without properly root-causing it. Workarounds without proper
>> root-causing are already pretty bad and adding extra API for that is
>> rather silly, IMHO.
>
> Yes. it is very easy to reproduce. /me Trying to figure the root cause out.
>
-- 
Asias

Re: [RFC PATCH 2/5] block: Do not stop draining if waitqueue is not empty.

[Tejun Heo]

Hello, Asias.

On Fri, May 25, 2012 at 09:16:47AM +0800, Asias He wrote:
> >I found this after queue cleanup. rl->wait[] is not empty while
> >rl->count[] == 0. There are exactly nr_sleep - nr_wakeup of process in D
> >state. So missed wakeup happens? Any ideas to do more debug to find the
> >root-cause?
> 
> Ping.

Ah, okay, freed_request() wakes up single waiter with the assumption
that after the wakeup there will at least be one successful allocation
which in turn will continue the wakeup chain until the wait list is
empty - ie. waiter wakeup is dependent on successful request
allocation happening after each wakeup.  With queue marked dead, any
woken up waiter fails the allocation path, so the wakeup chaining is
lost and we're left with hung waiters.  What we need is wake_up_all()
after drain completion.  Can you please test that?

Thanks.

-- 
tejun

Re: [RFC PATCH 2/5] block: Do not stop draining if waitqueue is not empty.

[Asias He]

On 05/28/2012 08:30 AM, Tejun Heo wrote:
> Hello, Asias.
>
> On Fri, May 25, 2012 at 09:16:47AM +0800, Asias He wrote:
>>> I found this after queue cleanup. rl->wait[] is not empty while
>>> rl->count[] == 0. There are exactly nr_sleep - nr_wakeup of process in D
>>> state. So missed wakeup happens? Any ideas to do more debug to find the
>>> root-cause?
>>
>> Ping.
>
> Ah, okay, freed_request() wakes up single waiter with the assumption
> that after the wakeup there will at least be one successful allocation
> which in turn will continue the wakeup chain until the wait list is
> empty - ie. waiter wakeup is dependent on successful request
> allocation happening after each wakeup.  With queue marked dead, any
> woken up waiter fails the allocation path, so the wakeup chaining is
> lost and we're left with hung waiters.  What we need is wake_up_all()
> after drain completion.  Can you please test that?

Thanks, Tejun! Works for me. Will cook a patch for this.

-- 
Asias