[PATCH 0/5] blk-mq fixes and improvements

[PATCH 0/5] blk-mq fixes and improvements

[Christoph Hellwig]

The first two patches make blk-mq work fine again for me in a KVM VM,
the others make sure that consumers don't have to care if the underlying queue
uses blk-mq or not and remove some code at the same time.

[PATCH 1/5] [PATCH 1/5] percpu_counter: fix irq restore in __percpu_counter_add

[Christoph Hellwig]

[-- Attachment #1: 0001-percpu_counter-fix-irq-restore-in-__percpu_counter_a.patch --]
[-- Type: text/plain, Size: 871 bytes --]

The current version of "percpu_counter: make APIs irq safe" in the blk-mq
tree doesn't properly restore irqs, thus causing the boot of the blk-multique
tree to fail with various irqs_disabled() BUGs and related issues.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 lib/percpu_counter.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
index 1ae43a7..7473ee3 100644
--- a/lib/percpu_counter.c
+++ b/lib/percpu_counter.c
@@ -82,7 +82,7 @@ void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch)
 		unsigned long flags;
 		raw_spin_lock_irqsave(&fbc->lock, flags);
 		fbc->count += count;
-		raw_spin_unlock(&fbc->lock);
+		raw_spin_unlock_irqrestore(&fbc->lock, flags);
 		__this_cpu_write(*fbc->counters, 0);
 	} else {
 		__this_cpu_write(*fbc->counters, count);
-- 
1.7.10.4

[PATCH 2/5] [PATCH 2/5] revert: "blk-mq: blk-mq should free bios in pass through case"

[Christoph Hellwig]

[-- Attachment #1: 0002-revert-blk-mq-blk-mq-should-free-bios-in-pass-throug.patch --]
[-- Type: text/plain, Size: 2970 bytes --]

This patch causes boot failures when using REQ_FLUSH requests.  Also the
following statement in the commit log:

    For non mq calls, the block layer will free the bios when
    blk_finish_request is called.

    For mq calls, the blk mq code wants the caller to do this.

Seems incorrect as far as I can follow the code as blk_finish_request only
calls __blk_put_request which then completes the bios if req->end_io is
not set.  This matches the blk-mq behaviour before this patch, so reverting
it makes the code more similar to the legacy case in addition to fixing the
boot failure.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-flush.c |    2 +-
 block/blk-mq.c    |   14 ++++++++++----
 block/blk-mq.h    |    1 +
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/block/blk-flush.c b/block/blk-flush.c
index 3e4cc9c..c56c37d 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -231,7 +231,7 @@ static void flush_end_io(struct request *flush_rq, int error)
 	unsigned long flags = 0;
 
 	if (q->mq_ops) {
-		blk_mq_free_request(flush_rq);
+		blk_mq_finish_request(flush_rq, error);
 		spin_lock_irqsave(&q->mq_flush_lock, flags);
 	}
 	running = &q->flush_queue[q->flush_running_idx];
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 93563e0..423089d 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -270,7 +270,7 @@ void blk_mq_free_request(struct request *rq)
 }
 EXPORT_SYMBOL(blk_mq_free_request);
 
-static void blk_mq_finish_request(struct request *rq, int error)
+void blk_mq_finish_request(struct request *rq, int error)
 {
 	struct bio *bio = rq->bio;
 	unsigned int bytes = 0;
@@ -286,17 +286,22 @@ static void blk_mq_finish_request(struct request *rq, int error)
 
 	blk_account_io_completion(rq, bytes);
 	blk_account_io_done(rq);
+	blk_mq_free_request(rq);
 }
 
 void blk_mq_complete_request(struct request *rq, int error)
 {
 	trace_block_rq_complete(rq->q, rq);
-	blk_mq_finish_request(rq, error);
 
+	/*
+	 * If ->end_io is set, it's responsible for doing the rest of the
+	 * completion.
+	 */
 	if (rq->end_io)
 		rq->end_io(rq, error);
 	else
-		blk_mq_free_request(rq);
+		blk_mq_finish_request(rq, error);
+
 }
 
 void __blk_mq_end_io(struct request *rq, int error)
@@ -984,7 +989,8 @@ int blk_mq_execute_rq(struct request_queue *q, struct request *rq)
 	if (rq->errors)
 		err = -EIO;
 
-	blk_mq_free_request(rq);
+	blk_mq_finish_request(rq, rq->errors);
+
 	return err;
 }
 EXPORT_SYMBOL(blk_mq_execute_rq);
diff --git a/block/blk-mq.h b/block/blk-mq.h
index 52bf1f9..42d0110 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_finish_request(struct request *rq, int error);
 
 /*
  * CPU hotplug helpers
-- 
1.7.10.4

Re: [PATCH 2/5] [PATCH 2/5] revert: "blk-mq: blk-mq should free bios in pass through case"

[Mike Christie]

On 10/4/13 8:49 AM, Christoph Hellwig wrote:
> This patch causes boot failures when using REQ_FLUSH requests.  Also the
> following statement in the commit log:
>
>      For non mq calls, the block layer will free the bios when
>      blk_finish_request is called.

Sorry, messed up function name. I meant blk_end_request*.

For blk_execute_rq_nowait/blk_execute_rq and normal request use, the 
lower levels free the bios as they are completed by one of the 
blk_finish_request* calls. The caller of of 
blk_execute_rq_nowait/blk_execute_rq does not have to worry about 
freeing bios. It just frees the request when it is done with it.

>
>      For mq calls, the blk mq code wants the caller to do this.

The problem was that Nick's scsi-mq code added a async
blk_mq_execute_rq support and it did not complete bios in the rq->end_io 
callback because he most likely expected it to work like the non-mq case 
above.

The goal of my patch was to make the underlying behavior and the 
function naming similar.

If we can get the the core block code to finish the bios as they are 
completed like is done in the non mq case like I was trying to do, then 
with your other patches in this set we do not need to have the rq end_io 
callbacks have this:

>   	if (q->mq_ops) {
> -		blk_mq_free_request(flush_rq);
> +		blk_mq_finish_request(flush_rq, error);
>   		spin_lock_irqsave(&q->mq_flush_lock, flags);


The block code would free the bios no matter who sent it and mq type, 
and the callers of blk_execute* would just call blk_put_request.

Re: [PATCH 2/5] [PATCH 2/5] revert: "blk-mq: blk-mq should free bios in pass through case"

[Christoph Hellwig]

On Fri, Oct 04, 2013 at 12:39:33PM -0500, Mike Christie wrote:
> Sorry, messed up function name. I meant blk_end_request*.
> 
> For blk_execute_rq_nowait/blk_execute_rq and normal request use, the
> lower levels free the bios as they are completed by one of the
> blk_finish_request* calls. The caller of of
> blk_execute_rq_nowait/blk_execute_rq does not have to worry about
> freeing bios. It just frees the request when it is done with it.

Are you talking about bios or requests?  All these functions deal with
requests, so the talk of bios really confuses me.

That beeing said the old ones all require the caller to free the
request, and complicate that with the useless refcounting that my patch
3 removes.  Take a look at the other patches how all the calling
conventions can be nicely unified.

Re: [PATCH 2/5] [PATCH 2/5] revert: "blk-mq: blk-mq should free bios in pass through case"

[Mike Christie]

On 10/05/2013 05:50 AM, Christoph Hellwig wrote:
> On Fri, Oct 04, 2013 at 12:39:33PM -0500, Mike Christie wrote:
>> Sorry, messed up function name. I meant blk_end_request*.
>>
>> For blk_execute_rq_nowait/blk_execute_rq and normal request use, the
>> lower levels free the bios as they are completed by one of the
>> blk_finish_request* calls. The caller of of
>> blk_execute_rq_nowait/blk_execute_rq does not have to worry about
>> freeing bios. It just frees the request when it is done with it.
> 
> Are you talking about bios or requests?  All these functions deal with
> requests, so the talk of bios really confuses me.

The functions take in requests as the argument, but they end up
operating on bios too. The scsi layer does
scsi_io_completion->scsi_end_request-> blk_end_request ->
blk_end_bidi_request -> blk_update_bidi_request -> blk_update_request.
That function will then complete bios on the request passed in. It does
not matter if the request is a REQ_TYPE_FS or REQ_TYPE_BLOCK_PC.

With my patch I was trying to make the block layer do the same for mq
REQ_TYPE_BLOCK_PC requests inserted into the queue with
blk_execute_rq_nowait (Nick's patch had support for something like that)
by having the block mq layer call blk_mq_finish_request instead of
making the code that calls blk_execute_rq_nowait do it.


> 
> That beeing said the old ones all require the caller to free the
> request, and complicate that with the useless refcounting that my patch
> 3 removes.  Take a look at the other patches how all the calling
> conventions can be nicely unified.

I agree. I like them. I am saying though it could be better because even
with your patches the rq->end_io functions need to have the mq_ops check
like the flush_end_io does. If my patch worked as intended we would have
your improvements and we would not need the rq->end_io functions to have
that check and call blk_mq_finish_request because the blk mq layer was
doing it for them.

That is all I am saying. I would like to be able to remove that check
and blk_mq_finish_request call from the rq->end_io callouts.

Re: [PATCH 2/5] [PATCH 2/5] revert: "blk-mq: blk-mq should free bios in pass through case"

[Christoph Hellwig]

On Sat, Oct 05, 2013 at 03:20:05PM -0500, Mike Christie wrote:
> The functions take in requests as the argument, but they end up
> operating on bios too. The scsi layer does
> scsi_io_completion->scsi_end_request-> blk_end_request ->
> blk_end_bidi_request -> blk_update_bidi_request -> blk_update_request.
> That function will then complete bios on the request passed in. It does
> not matter if the request is a REQ_TYPE_FS or REQ_TYPE_BLOCK_PC.
> 
> With my patch I was trying to make the block layer do the same for mq
> REQ_TYPE_BLOCK_PC requests inserted into the queue with
> blk_execute_rq_nowait (Nick's patch had support for something like that)
> by having the block mq layer call blk_mq_finish_request instead of
> making the code that calls blk_execute_rq_nowait do it.

Ok, I get the point now.  Didn't see that issue because I've only been
testing the non-bio REQ_TYPE_BLOCK_PC case as exposed by the virtio-blk
serial attribute so far.

> > That beeing said the old ones all require the caller to free the
> > request, and complicate that with the useless refcounting that my patch
> > 3 removes.  Take a look at the other patches how all the calling
> > conventions can be nicely unified.
> 
> I agree. I like them. I am saying though it could be better because even
> with your patches the rq->end_io functions need to have the mq_ops check
> like the flush_end_io does. If my patch worked as intended we would have
> your improvements and we would not need the rq->end_io functions to have
> that check and call blk_mq_finish_request because the blk mq layer was
> doing it for them.
> 
> That is all I am saying. I would like to be able to remove that check
> and blk_mq_finish_request call from the rq->end_io callouts.

I've found the bug that caused the regression with your patch, it's that
blk-mq incorrectly completes bios midway through a flush sequence, while
the old path didn't.

I'll send out a series soon that fixes this and re-reverts your patch,
although I split that re-revert into two patches in addition to my new
fix, given that I think your mixing up of the blk_mq_finish_request /
blk_mq_free_spit plus the confusing description made it really hard to
grasp, but I'll leave it to Jens how he wants to apply it to his tree
and make it look after the next rebase.

[PATCH 3/5] [PATCH 3/5] block: remove request ref_count

[Christoph Hellwig]

[-- Attachment #1: 0003-block-remove-request-ref_count.patch --]
[-- Type: text/plain, Size: 2145 bytes --]

This reference count has been around since before git history, but the only
place where it's used is in blk_execute_rq, and ther it is entirely useless
as it is incremented before submitting the request and decremented in the
end_io handler before waking up the submitter thread.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-core.c       |    3 ---
 block/blk-exec.c       |    7 -------
 include/linux/blkdev.h |    2 --
 3 files changed, 12 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index f80df88..5620e58 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -110,7 +110,6 @@ void blk_rq_init(struct request_queue *q, struct request *rq)
 	rq->cmd = rq->__cmd;
 	rq->cmd_len = BLK_MAX_CDB;
 	rq->tag = -1;
-	rq->ref_count = 1;
 	rq->start_time = jiffies;
 	set_start_time_ns(rq);
 	rq->part = NULL;
@@ -1249,8 +1248,6 @@ void __blk_put_request(struct request_queue *q, struct request *req)
 {
 	if (unlikely(!q))
 		return;
-	if (unlikely(--req->ref_count))
-		return;
 
 	blk_pm_put_request(req);
 
diff --git a/block/blk-exec.c b/block/blk-exec.c
index e706213..7972da7 100644
--- a/block/blk-exec.c
+++ b/block/blk-exec.c
@@ -24,7 +24,6 @@ static void blk_end_sync_rq(struct request *rq, int error)
 	struct completion *waiting = rq->end_io_data;
 
 	rq->end_io_data = NULL;
-	__blk_put_request(rq->q, rq);
 
 	/*
 	 * complete last, if this is a stack request the process (and thus
@@ -103,12 +102,6 @@ int blk_execute_rq(struct request_queue *q, struct gendisk *bd_disk,
 	int err = 0;
 	unsigned long hang_check;
 
-	/*
-	 * we need an extra reference to the request, so we can look at
-	 * it after io completion
-	 */
-	rq->ref_count++;
-
 	if (!rq->sense) {
 		memset(sense, 0, sizeof(sense));
 		rq->sense = sense;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 06df51d..8771c0b 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -168,8 +168,6 @@ struct request {
 
 	unsigned short ioprio;
 
-	int ref_count;
-
 	void *special;		/* opaque pointer available for LLD use */
 	char *buffer;		/* kaddr of the current segment if available */
 
-- 
1.7.10.4

[PATCH 4/5] [PATCH 4/5] block: make blk_get/put_request work for blk-mq drivers

[Christoph Hellwig]

[-- Attachment #1: 0004-block-make-blk_get-put_request-work-for-blk-mq-drive.patch --]
[-- Type: text/plain, Size: 3002 bytes --]

Consumers of the block layer shouldn't have to care if a driver uses
blk-mq or not, so make blk_get/put_request call the mq equivalents
underneath.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-core.c                  |   31 ++++++++++++++++++++-----------
 block/blk-mq.c                    |    1 -
 drivers/block/mtip32xx/mtip32xx.c |    2 +-
 3 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 5620e58..6d7fd79 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1080,7 +1080,8 @@ retry:
 	goto retry;
 }
 
-struct request *blk_get_request(struct request_queue *q, int rw, gfp_t gfp_mask)
+static struct request *blk_old_get_request(struct request_queue *q, int rw,
+		gfp_t gfp_mask)
 {
 	struct request *rq;
 
@@ -1097,6 +1098,14 @@ struct request *blk_get_request(struct request_queue *q, int rw, gfp_t gfp_mask)
 
 	return rq;
 }
+
+struct request *blk_get_request(struct request_queue *q, int rw, gfp_t gfp_mask)
+{
+	if (q->mq_ops)
+		return blk_mq_alloc_request(q, rw, gfp_mask);
+	else
+		return blk_old_get_request(q, rw, gfp_mask);
+}
 EXPORT_SYMBOL(blk_get_request);
 
 /**
@@ -1133,12 +1142,7 @@ EXPORT_SYMBOL(blk_get_request);
 struct request *blk_make_request(struct request_queue *q, struct bio *bio,
 				 gfp_t gfp_mask)
 {
-	struct request *rq;
-
-	if (q->mq_ops)
-		rq = blk_mq_alloc_request(q, bio_data_dir(bio), gfp_mask);
-	else
-		rq = blk_get_request(q, bio_data_dir(bio), gfp_mask);
+	struct request *rq = blk_get_request(q, bio_data_dir(bio), gfp_mask);
 
 	if (unlikely(!rq))
 		return ERR_PTR(-ENOMEM);
@@ -1276,12 +1280,17 @@ EXPORT_SYMBOL_GPL(__blk_put_request);
 
 void blk_put_request(struct request *req)
 {
-	unsigned long flags;
 	struct request_queue *q = req->q;
 
-	spin_lock_irqsave(q->queue_lock, flags);
-	__blk_put_request(q, req);
-	spin_unlock_irqrestore(q->queue_lock, flags);
+	if (q->mq_ops)
+		blk_mq_free_request(req);
+	else {
+		unsigned long flags;
+
+		spin_lock_irqsave(q->queue_lock, flags);
+		__blk_put_request(q, req);
+		spin_unlock_irqrestore(q->queue_lock, flags);
+	}
 }
 EXPORT_SYMBOL(blk_put_request);
 
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 423089d..709747f 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -268,7 +268,6 @@ void blk_mq_free_request(struct request *rq)
 	hctx = q->mq_ops->map_queue(q, ctx->cpu);
 	__blk_mq_free_request(hctx, ctx, rq);
 }
-EXPORT_SYMBOL(blk_mq_free_request);
 
 void blk_mq_finish_request(struct request *rq, int error)
 {
diff --git a/drivers/block/mtip32xx/mtip32xx.c b/drivers/block/mtip32xx/mtip32xx.c
index 4427178..3c4c668 100644
--- a/drivers/block/mtip32xx/mtip32xx.c
+++ b/drivers/block/mtip32xx/mtip32xx.c
@@ -158,7 +158,7 @@ static struct mtip_cmd *mtip_get_int_command(struct driver_data *dd)
 
 static void mtip_put_int_command(struct driver_data *dd, struct mtip_cmd *cmd)
 {
-	blk_mq_free_request(blk_mq_rq_from_pdu(cmd));
+	blk_put_request(blk_mq_rq_from_pdu(cmd));
 }
 
 /*
-- 
1.7.10.4

[PATCH 5/5] [PATCH 5/5] block: use blk-exec.c infrastructure for blk-mq

[Christoph Hellwig]

[-- Attachment #1: 0005-block-use-blk-exec.c-infrastructure-for-blk-mq.patch --]
[-- Type: text/plain, Size: 3679 bytes --]

There is no need to reinvent blk_execute_rq for blk-mq if we can easily
reuse the sync and async versions already present.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-exec.c           |    7 +++++++
 block/blk-mq.c             |   36 ------------------------------------
 drivers/block/virtio_blk.c |    5 ++++-
 include/linux/blk-mq.h     |    1 -
 4 files changed, 11 insertions(+), 38 deletions(-)

diff --git a/block/blk-exec.c b/block/blk-exec.c
index 7972da7..47aef02 100644
--- a/block/blk-exec.c
+++ b/block/blk-exec.c
@@ -5,6 +5,7 @@
 #include <linux/module.h>
 #include <linux/bio.h>
 #include <linux/blkdev.h>
+#include <linux/blk-mq.h>
 #include <linux/sched/sysctl.h>
 
 #include "blk.h"
@@ -58,6 +59,12 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,
 
 	rq->rq_disk = bd_disk;
 	rq->end_io = done;
+
+	if (q->mq_ops) {
+		blk_mq_insert_request(q, rq, true);
+		return;
+	}
+
 	/*
 	 * need to check this before __blk_run_queue(), because rq can
 	 * be freed before that returns.
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 709747f..dece2e2 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -958,42 +958,6 @@ run_queue:
 	blk_mq_run_hw_queue(hctx, !is_sync || is_flush_fua);
 }
 
-static void blk_mq_execute_end_io(struct request *rq, int error)
-{
-	struct completion *wait = rq->end_io_data;
-
-	complete(wait);
-}
-
-/*
- * Insert request, pass to device, and wait for it to finish.
- */
-int blk_mq_execute_rq(struct request_queue *q, struct request *rq)
-{
-	DECLARE_COMPLETION_ONSTACK(wait);
-	unsigned long hang_check;
-	int err = 0;
-
-	rq->end_io_data = &wait;
-	rq->end_io = blk_mq_execute_end_io;
-	blk_mq_insert_request(q, rq, true);
-
-	/* Prevent hang_check timer from firing at us during very long I/O */
-	hang_check = sysctl_hung_task_timeout_secs;
-	if (hang_check)
-		while (!wait_for_completion_io_timeout(&wait, hang_check * (HZ/2)));
-	else
-		wait_for_completion_io(&wait);
-
-	if (rq->errors)
-		err = -EIO;
-
-	blk_mq_finish_request(rq, rq->errors);
-
-	return err;
-}
-EXPORT_SYMBOL(blk_mq_execute_rq);
-
 /*
  * Default mapping to a software queue, since we use one per CPU.
  */
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index e0b1115..0352df1 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -410,6 +410,7 @@ static int virtblk_get_id(struct gendisk *disk, char *id_str)
 	struct virtio_blk *vblk = disk->private_data;
 	struct request *req;
 	struct bio *bio;
+	int err;
 
 	bio = bio_map_kern(vblk->disk->queue, id_str, VIRTIO_BLK_ID_BYTES,
 			   GFP_KERNEL);
@@ -423,7 +424,9 @@ static int virtblk_get_id(struct gendisk *disk, char *id_str)
 	}
 
 	req->cmd_type = REQ_TYPE_SPECIAL;
-	return blk_mq_execute_rq(vblk->disk->queue, req);
+	err = blk_execute_rq(vblk->disk->queue, vblk->disk, req, false);
+	blk_put_request(req);
+	return err;
 }
 
 static int virtblk_ioctl(struct block_device *bdev, fmode_t mode,
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 2fea261..4fddab2 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -121,7 +121,6 @@ void blk_mq_init_commands(struct request_queue *, void (*init)(void *data, struc
 void blk_mq_flush_plug(struct request_queue *, bool);
 void blk_mq_insert_request(struct request_queue *, struct request *, bool);
 void blk_mq_insert_requests(struct request_queue *, struct list_head *, bool, bool);
-int blk_mq_execute_rq(struct request_queue *, struct request *);
 void blk_mq_run_queues(struct request_queue *q, bool async);
 void blk_mq_free_request(struct request *rq);
 bool blk_mq_can_queue(struct blk_mq_hw_ctx *);
-- 
1.7.10.4

Re: [PATCH 0/5] blk-mq fixes and improvements

[Jens Axboe]

On 10/04/2013 07:49 AM, Christoph Hellwig wrote:
> The first two patches make blk-mq work fine again for me in a KVM VM,
> the others make sure that consumers don't have to care if the underlying queue
> uses blk-mq or not and remove some code at the same time.

Thanks Christoph, applied.

-- 
Jens Axboe