[PATCH 0/4] block: blk-mq: support blk_cleanup_queue on mq

[PATCH 0/4] block: blk-mq: support blk_cleanup_queue on mq

[Ming Lei]

Hi,

The first 2 patches support drain/sync mq queue in blk_cleanup_queue.

The 3rd patch calls blk_cleanup_queue() in removing device path
to fix queue leak problem.

The 4th patch doesn't export blk_mq_free_queue() because the function
is called in release handler of queue kobject, and drivers needn't
to call it.

Thanks,
--
Ming Lei

[PATCH 1/4] block: blk-mq: support draining mq queue

[Ming Lei]

blk_mq_drain_queue() is introduced so that we can drain
mq queue during cleanup queue.

Also don't accept new requests any more if queue is marked
as dying.

Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 block/blk-core.c |   13 +++++++++----
 block/blk-mq.c   |   24 +++++++++++++++++++++---
 block/blk-mq.h   |    1 +
 3 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 5da8e90..eb13db0 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -38,6 +38,7 @@
 
 #include "blk.h"
 #include "blk-cgroup.h"
+#include "blk-mq.h"
 
 EXPORT_TRACEPOINT_SYMBOL_GPL(block_bio_remap);
 EXPORT_TRACEPOINT_SYMBOL_GPL(block_rq_remap);
@@ -497,10 +498,14 @@ void blk_cleanup_queue(struct request_queue *q)
 	 * Drain all requests queued before DYING marking. Set DEAD flag to
 	 * prevent that q->request_fn() gets invoked after draining finished.
 	 */
-	spin_lock_irq(lock);
-	__blk_drain_queue(q, true);
-	queue_flag_set(QUEUE_FLAG_DEAD, q);
-	spin_unlock_irq(lock);
+	if (q->mq_ops) {
+		blk_mq_drain_queue(q);
+	} else {
+		spin_lock_irq(lock);
+		__blk_drain_queue(q, true);
+		queue_flag_set(QUEUE_FLAG_DEAD, q);
+		spin_unlock_irq(lock);
+	}
 
 	/* @q won't process any more request, flush async actions */
 	del_timer_sync(&q->backing_dev_info.laptop_mode_wb_timer);
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 53dc9f7..fcfdf35 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -104,10 +104,13 @@ static int blk_mq_queue_enter(struct request_queue *q)
 
 	spin_lock_irq(q->queue_lock);
 	ret = wait_event_interruptible_lock_irq(q->mq_freeze_wq,
-		!blk_queue_bypass(q), *q->queue_lock);
+		!blk_queue_bypass(q) || blk_queue_dying(q),
+		*q->queue_lock);
 	/* inc usage with lock hold to avoid freeze_queue runs here */
-	if (!ret)
+	if (!ret && !blk_queue_dying(q))
 		__percpu_counter_add(&q->mq_usage_counter, 1, 1000000);
+	else if (blk_queue_dying(q))
+		ret = -ENODEV;
 	spin_unlock_irq(q->queue_lock);
 
 	return ret;
@@ -122,10 +125,14 @@ static void blk_mq_queue_exit(struct request_queue *q)
  * Guarantee no request is in use, so we can change any data structure of
  * the queue afterward.
  */
-static void blk_mq_freeze_queue(struct request_queue *q)
+static void __blk_mq_freeze_queue(struct request_queue *q,
+		bool force_drain)
 {
 	bool drain;
 
+	if (force_drain)
+		goto do_drain;
+
 	spin_lock_irq(q->queue_lock);
 	drain = !q->bypass_depth++;
 	queue_flag_set(QUEUE_FLAG_BYPASS, q);
@@ -134,6 +141,7 @@ static void blk_mq_freeze_queue(struct request_queue *q)
 	if (!drain)
 		return;
 
+ do_drain:
 	while (true) {
 		s64 count;
 
@@ -148,6 +156,16 @@ static void blk_mq_freeze_queue(struct request_queue *q)
 	}
 }
 
+static void blk_mq_freeze_queue(struct request_queue *q)
+{
+	__blk_mq_freeze_queue(q, false);
+}
+
+void blk_mq_drain_queue(struct request_queue *q)
+{
+	__blk_mq_freeze_queue(q, true);
+}
+
 static void blk_mq_unfreeze_queue(struct request_queue *q)
 {
 	bool wake = false;
diff --git a/block/blk-mq.h b/block/blk-mq.h
index 5761eed..35ff4f7 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -27,6 +27,7 @@ void blk_mq_complete_request(struct request *rq, int error);
 void blk_mq_run_request(struct request *rq, bool run_queue, bool async);
 void blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async);
 void blk_mq_init_flush(struct request_queue *q);
+void blk_mq_drain_queue(struct request_queue *q);
 
 /*
  * CPU hotplug helpers
-- 
1.7.9.5

[PATCH 2/4] block: blk-mq: make blk_sync_queue support mq

[Ming Lei]

This patch moves synchronization on mq->delay_work
from blk_mq_free_queue() to blk_sync_queue(), so that
blk_sync_queue can work on mq.

Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 block/blk-core.c |   11 ++++++++++-
 block/blk-mq.c   |    1 -
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index eb13db0..74ebde3 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -246,7 +246,16 @@ EXPORT_SYMBOL(blk_stop_queue);
 void blk_sync_queue(struct request_queue *q)
 {
 	del_timer_sync(&q->timeout);
-	cancel_delayed_work_sync(&q->delay_work);
+
+	if (q->mq_ops) {
+		struct blk_mq_hw_ctx *hctx;
+		int i;
+
+		queue_for_each_hw_ctx(q, hctx, i)
+			cancel_delayed_work_sync(&hctx->delayed_work);
+	} else {
+		cancel_delayed_work_sync(&q->delay_work);
+	}
 }
 EXPORT_SYMBOL(blk_sync_queue);
 
diff --git a/block/blk-mq.c b/block/blk-mq.c
index fcfdf35..16c046f 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1406,7 +1406,6 @@ void blk_mq_free_queue(struct request_queue *q)
 	int i;
 
 	queue_for_each_hw_ctx(q, hctx, i) {
-		cancel_delayed_work_sync(&hctx->delayed_work);
 		kfree(hctx->ctx_map);
 		kfree(hctx->ctxs);
 		blk_mq_free_rq_map(hctx);
-- 
1.7.9.5

[PATCH 3/4] block: null_blk: fix queue leak inside removing device

[Ming Lei]

When queue_mode is NULL_Q_MQ and null_blk is being removed,
blk_cleanup_queue() isn't called to cleanup queue, so the
queue allocated won't be freed.

This patch calls blk_cleanup_queue() for MQ to drain all
pending requests first and release the reference counter
of queue kobject, then blk_mq_free_queue() will be called
in queue kobject's release handler when queue kobject's
reference counter drops to zero.

Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 drivers/block/null_blk.c |   10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/block/null_blk.c b/drivers/block/null_blk.c
index a2e69d2..83a598e 100644
--- a/drivers/block/null_blk.c
+++ b/drivers/block/null_blk.c
@@ -425,10 +425,7 @@ static void null_del_dev(struct nullb *nullb)
 	list_del_init(&nullb->list);
 
 	del_gendisk(nullb->disk);
-	if (queue_mode == NULL_Q_MQ)
-		blk_mq_free_queue(nullb->q);
-	else
-		blk_cleanup_queue(nullb->q);
+	blk_cleanup_queue(nullb->q);
 	put_disk(nullb->disk);
 	kfree(nullb);
 }
@@ -578,10 +575,7 @@ static int null_add_dev(void)
 	disk = nullb->disk = alloc_disk_node(1, home_node);
 	if (!disk) {
 queue_fail:
-		if (queue_mode == NULL_Q_MQ)
-			blk_mq_free_queue(nullb->q);
-		else
-			blk_cleanup_queue(nullb->q);
+		blk_cleanup_queue(nullb->q);
 		cleanup_queues(nullb);
 err:
 		kfree(nullb);
-- 
1.7.9.5

[PATCH 4/4] block: blk-mq: don't export blk_mq_free_queue()

[Ming Lei]

blk_mq_free_queue() is called from release handler of
queue kobject, so it needn't be called from drivers.

Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 block/blk-mq.c         |    1 -
 block/blk-mq.h         |    1 +
 block/blk-sysfs.c      |    1 +
 include/linux/blk-mq.h |    1 -
 4 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 16c046f..a58f103 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1427,7 +1427,6 @@ void blk_mq_free_queue(struct request_queue *q)
 	list_del_init(&q->all_q_node);
 	mutex_unlock(&all_q_mutex);
 }
-EXPORT_SYMBOL(blk_mq_free_queue);
 
 /* Basically redo blk_mq_init_queue with queue frozen */
 static void blk_mq_queue_reinit(struct request_queue *q)
diff --git a/block/blk-mq.h b/block/blk-mq.h
index 35ff4f7..5c39179 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -28,6 +28,7 @@ void blk_mq_run_request(struct request *rq, bool run_queue, bool async);
 void blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async);
 void blk_mq_init_flush(struct request_queue *q);
 void blk_mq_drain_queue(struct request_queue *q);
+void blk_mq_free_queue(struct request_queue *q);
 
 /*
  * CPU hotplug helpers
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 9777952..8095c4a 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -11,6 +11,7 @@
 
 #include "blk.h"
 #include "blk-cgroup.h"
+#include "blk-mq.h"
 
 struct queue_sysfs_entry {
 	struct attribute attr;
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index ab0e9b2..851d34b 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -113,7 +113,6 @@ enum {
 };
 
 struct request_queue *blk_mq_init_queue(struct blk_mq_reg *, void *);
-void blk_mq_free_queue(struct request_queue *);
 int blk_mq_register_disk(struct gendisk *);
 void blk_mq_unregister_disk(struct gendisk *);
 void blk_mq_init_commands(struct request_queue *, void (*init)(void *data, struct blk_mq_hw_ctx *, struct request *, unsigned int), void *data);
-- 
1.7.9.5

Re: [PATCH 1/4] block: blk-mq: support draining mq queue

[Christoph Hellwig]

On Tue, Dec 24, 2013 at 12:18:56AM +0800, Ming Lei wrote:
> blk_mq_drain_queue() is introduced so that we can drain
> mq queue during cleanup queue.
> 
> Also don't accept new requests any more if queue is marked
> as dying.

blk_cleanup_queue is a call from the LLDD, there is no need to make
it handle the MQ case.  However given that there might be a lot of
code shared between blk_cleanup_queue and blk_mq_cleanup_queue an
internal helper might be useful.

> +	if (q->mq_ops) {
> +		blk_mq_drain_queue(q);
> +	} else {
> +		spin_lock_irq(lock);
> +		__blk_drain_queue(q, true);
> +		queue_flag_set(QUEUE_FLAG_DEAD, q);
> +		spin_unlock_irq(lock);
> +	}

Why doesn't the mq case set QUEUE_FLAG_DEAD here?

> -static void blk_mq_freeze_queue(struct request_queue *q)
> +static void __blk_mq_freeze_queue(struct request_queue *q,
> +		bool force_drain)
>  {
>  	bool drain;
>  
> +	if (force_drain)
> +		goto do_drain;
> +
>  	spin_lock_irq(q->queue_lock);
>  	drain = !q->bypass_depth++;
>  	queue_flag_set(QUEUE_FLAG_BYPASS, q);
> @@ -134,6 +141,7 @@ static void blk_mq_freeze_queue(struct request_queue *q)
>  	if (!drain)
>  		return;
>  
> + do_drain:
>  	while (true) {
>  		s64 count;

This begs to be split into two functions, one that forces the drain, and
once that wraps it.

Also blk_execute_rq_nowait now needs to check the dying case for MQ as
well.

Re: [PATCH 2/4] block: blk-mq: make blk_sync_queue support mq

[Christoph Hellwig]

On Tue, Dec 24, 2013 at 12:18:57AM +0800, Ming Lei wrote:
> This patch moves synchronization on mq->delay_work
> from blk_mq_free_queue() to blk_sync_queue(), so that
> blk_sync_queue can work on mq.

Again, this is a LLD call, should have a separate blk_mq_sync_queue.

Re: [PATCH 3/4] block: null_blk: fix queue leak inside removing device

[Christoph Hellwig]

On Tue, Dec 24, 2013 at 12:18:58AM +0800, Ming Lei wrote:
>  	del_gendisk(nullb->disk);
> -	if (queue_mode == NULL_Q_MQ)
> -		blk_mq_free_queue(nullb->q);
> -	else
> -		blk_cleanup_queue(nullb->q);
> +	blk_cleanup_queue(nullb->q);

All exported users of blk_mq_free_queue go away with this change, so
make sure it's not available anymore.  I have to say the one in
blk-sysfs.c also looks suspicious.

Re: [PATCH 4/4] block: blk-mq: don't export blk_mq_free_queue()

[Christoph Hellwig]

Ah sorry, should have read this one earlier.

Re: [PATCH 1/4] block: blk-mq: support draining mq queue

[Ming Lei]

Hi Christoph,

Thanks for your review.

On Tue, Dec 24, 2013 at 12:24 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Tue, Dec 24, 2013 at 12:18:56AM +0800, Ming Lei wrote:
>> blk_mq_drain_queue() is introduced so that we can drain
>> mq queue during cleanup queue.
>>
>> Also don't accept new requests any more if queue is marked
>> as dying.
>
> blk_cleanup_queue is a call from the LLDD, there is no need to make

Some drivers don't have LLDD, like null_blk, virtio-blk, ...

> it handle the MQ case.  However given that there might be a lot of
> code shared between blk_cleanup_queue and blk_mq_cleanup_queue an
> internal helper might be useful.

So there are two ways to implement block API for MQ: one is to try to
adapt current block APIs for blk mq driver, another one is to always
reinvent new blk_mq_* APIs for blk mq driver.

Looks you prefer to the 2nd one, and this patch takes the 1st way.

Actually the below block APIs
               blk_get_request()
               blk_put_request()
               blk_execute_rq_nowait()
               blk_register_queue()
               blk_unregister_queue()
               blk_insert_flush()
               ....

have been there to support MQ.

One problem about the 2nd way is that MQ drivers may become a bit
ugly if the driver still need to support traditional request/bio mechanism.

So I think blk_mq_drain_queue can be called for MQ too.

>
>> +     if (q->mq_ops) {
>> +             blk_mq_drain_queue(q);
>> +     } else {
>> +             spin_lock_irq(lock);
>> +             __blk_drain_queue(q, true);
>> +             queue_flag_set(QUEUE_FLAG_DEAD, q);
>> +             spin_unlock_irq(lock);
>> +     }
>
> Why doesn't the mq case set QUEUE_FLAG_DEAD here?

Looks DEAD flag is set to prevent request_fn from being called,
and there is no request_fn for MQ, so I don't set it and QUEUE_FLAG_DYING
has been checked in blk_mq_queue_enter() already.

But we can do it easily.

>
>> -static void blk_mq_freeze_queue(struct request_queue *q)
>> +static void __blk_mq_freeze_queue(struct request_queue *q,
>> +             bool force_drain)
>>  {
>>       bool drain;
>>
>> +     if (force_drain)
>> +             goto do_drain;
>> +
>>       spin_lock_irq(q->queue_lock);
>>       drain = !q->bypass_depth++;
>>       queue_flag_set(QUEUE_FLAG_BYPASS, q);
>> @@ -134,6 +141,7 @@ static void blk_mq_freeze_queue(struct request_queue *q)
>>       if (!drain)
>>               return;
>>
>> + do_drain:
>>       while (true) {
>>               s64 count;
>
> This begs to be split into two functions, one that forces the drain, and
> once that wraps it.

Sounds a good idea, and I will do it in v1.

>
> Also blk_execute_rq_nowait now needs to check the dying case for MQ as
> well.

In this patch the dying flag is only checked in blk_mq_queue_enter()
which is called in allocating request path, so once the request is
allocated, we just simply wait for its completion in blk_mq_drain_queue().

Even for non-MQ case, looks blk_queue_dying(q) is checked to avoid
being freed early before running queue, I am not sure if it is good
because the I/O might or can have been completed.  And it isn't a problem
for MQ because this request can't be reused when dying is set.


Thanks,
-- 
Ming Lei

Re: [PATCH 2/4] block: blk-mq: make blk_sync_queue support mq

[Ming Lei]

Hello Christoph,


On Tue, Dec 24, 2013 at 12:25 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Tue, Dec 24, 2013 at 12:18:57AM +0800, Ming Lei wrote:
>> This patch moves synchronization on mq->delay_work
>> from blk_mq_free_queue() to blk_sync_queue(), so that
>> blk_sync_queue can work on mq.
>
> Again, this is a LLD call, should have a separate blk_mq_sync_queue.
>

As I replied in previous mail, we have other LLD APIs which should be
called for MQ too, also making current APIs adapt to MQ can simplify
drivers.


Thanks,
-- 
Ming Lei

Re: [PATCH 1/4] block: blk-mq: support draining mq queue

[Christoph Hellwig]

On Tue, Dec 24, 2013 at 11:30:58AM +0800, Ming Lei wrote:
> Some drivers don't have LLDD, like null_blk, virtio-blk, ...

Well, they are the low-level driver in this case.

> Actually the below block APIs
>                blk_get_request()
>                blk_put_request()
>                blk_execute_rq_nowait()
>                blk_register_queue()
>                blk_unregister_queue()
>                blk_insert_flush()
>                ....
> 
> have been there to support MQ.

All these are called by higher level code and not the driver, so we need
a switch between MQ and non-MQ support somewhere.

> One problem about the 2nd way is that MQ drivers may become a bit
> ugly if the driver still need to support traditional request/bio mechanism.

On the other hand drivers have no legitimate case for long-term parallel
MQ/non-MQ code paths.  The only exception is null_blk as a benchmarking
vehicle, but we should not optimize for that.

> >> +             spin_unlock_irq(lock);
> >> +     }
> >
> > Why doesn't the mq case set QUEUE_FLAG_DEAD here?
> 
> Looks DEAD flag is set to prevent request_fn from being called,
> and there is no request_fn for MQ, so I don't set it and QUEUE_FLAG_DYING
> has been checked in blk_mq_queue_enter() already.
> 
> But we can do it easily.

Do we have a per-context flag to prevent calling into the driver
instead?  Sorry, it's been a while that I last looked at the code, but
anyone just looking over the difference will have the same question, so
the answer needs to go into a comment.

> > Also blk_execute_rq_nowait now needs to check the dying case for MQ as
> > well.
> 
> In this patch the dying flag is only checked in blk_mq_queue_enter()
> which is called in allocating request path, so once the request is
> allocated, we just simply wait for its completion in blk_mq_drain_queue().
> 
> Even for non-MQ case, looks blk_queue_dying(q) is checked to avoid
> being freed early before running queue, I am not sure if it is good
> because the I/O might or can have been completed.  And it isn't a problem
> for MQ because this request can't be reused when dying is set.

Good.  I think we'll want comments like that preferable in the code, but
at very least in the commit log.

Re: [PATCH 1/4] block: blk-mq: support draining mq queue

[Ming Lei]

Hi Christoph,

On Thu, Dec 26, 2013 at 5:45 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Tue, Dec 24, 2013 at 11:30:58AM +0800, Ming Lei wrote:
>> Some drivers don't have LLDD, like null_blk, virtio-blk, ...
>
> Well, they are the low-level driver in this case.
>
>> Actually the below block APIs
>>                blk_get_request()
>>                blk_put_request()
>>                blk_execute_rq_nowait()
>>                blk_register_queue()
>>                blk_unregister_queue()
>>                blk_insert_flush()
>>                ....
>>
>> have been there to support MQ.
>
> All these are called by higher level code and not the driver, so we need
> a switch between MQ and non-MQ support somewhere.

But sometimes there is no so-called high level code, and only one driver
for driving the block device, such as two drivers above, and we have lots
of such drivers, so looks not easy to apply the rule for all drivers suppose
there is.

IMO, if one block API can serve for both non-MQ and MQ cases, why do
we have to split it into blk_foo() and blk_mq_foo()?

>
>> One problem about the 2nd way is that MQ drivers may become a bit
>> ugly if the driver still need to support traditional request/bio mechanism.
>
> On the other hand drivers have no legitimate case for long-term parallel
> MQ/non-MQ code paths.  The only exception is null_blk as a benchmarking
> vehicle, but we should not optimize for that.

IMO it is very possible that parallel path might keep for a while, and at least
the current scsi-mq patches do so.

>
>> >> +             spin_unlock_irq(lock);
>> >> +     }
>> >
>> > Why doesn't the mq case set QUEUE_FLAG_DEAD here?
>>
>> Looks DEAD flag is set to prevent request_fn from being called,
>> and there is no request_fn for MQ, so I don't set it and QUEUE_FLAG_DYING
>> has been checked in blk_mq_queue_enter() already.
>>
>> But we can do it easily.
>
> Do we have a per-context flag to prevent calling into the driver
> instead?  Sorry, it's been a while that I last looked at the code, but
> anyone just looking over the difference will have the same question, so
> the answer needs to go into a comment.

It is OK to add comment or just keep the two cases consistent, and looks
I prefer to the later.

>
>> > Also blk_execute_rq_nowait now needs to check the dying case for MQ as
>> > well.
>>
>> In this patch the dying flag is only checked in blk_mq_queue_enter()
>> which is called in allocating request path, so once the request is
>> allocated, we just simply wait for its completion in blk_mq_drain_queue().
>>
>> Even for non-MQ case, looks blk_queue_dying(q) is checked to avoid
>> being freed early before running queue, I am not sure if it is good
>> because the I/O might or can have been completed.  And it isn't a problem
>> for MQ because this request can't be reused when dying is set.
>
> Good.  I think we'll want comments like that preferable in the code, but
> at very least in the commit log.

OK.


Thanks,
-- 
Ming Lei

Re: [PATCH 1/4] block: blk-mq: support draining mq queue

[Christoph Hellwig]

On Thu, Dec 26, 2013 at 06:12:30PM +0800, Ming Lei wrote:
> IMO, if one block API can serve for both non-MQ and MQ cases, why do
> we have to split it into blk_foo() and blk_mq_foo()?

To make the usage obvious and to allow killing the old code more
easily.  Looking back I have to see I'd actually prefer it the MQ
code simply used different data structures.

> 
> IMO it is very possible that parallel path might keep for a while, and at least
> the current scsi-mq patches do so.

That might be okay for the current prototype, but there's not point in
merging it if it can't replace the old legacy request code.  And yes,
this will require a lot more work.