[PATCH 0/5] dm-rq: improve sequential I/O performance

[PATCH 0/5] dm-rq: improve sequential I/O performance

[Ming Lei]

Hi,

This 1st one patch removes one log message which can be triggered
very easily.

The 2nd patch removes the workaround of blk_mq_delay_run_hw_queue()
in case of requeue, this way isn't necessary, and more worse, it
makes BLK_MQ_S_SCHED_RESTART not working, and degarde I/O performance.

The 3rd patch return DM_MAPIO_REQUEUE to dm-rq if underlying request
allocation fails, then we can return BLK_STS_RESOURCE from dm-rq
to blk-mq, so that blk-mq can hold the requests to be dequeued.

The 4th patch is a pre-patch for the 5th one, becasue even though
underlying request allocation succeeds, its queue may be
busy and we can get this feedback from blk_insert_cloned_request()
now. This patch trys to cache the allocated request so that
it may be reused in next dispatch to underlying queue.

The 5th patch improves sequential I/O performance by returning
STS_RESOURCE if underlying queue is busy.

In the commit log of the 5th patch, I/O IOPS data is provided and
we can see sequential I/O performance is improved a lot with this
patchset.

This patchset depends on the following two patchset:

[1] [PATCH V5 0/7] blk-mq-sched: improve sequential I/O performance(part 1)
    
	https://marc.info/?l=linux-block&m=150676854821077&w=2
    
[2] [PATCH V5 0/8] blk-mq: improve bio merge for none scheduler
    
	https://marc.info/?l=linux-block&m=150677085521416&w=2

Any comments are welcome! 

Thanks,
Ming

Ming Lei (5):
  dm-mpath: remove annoying message of 'blk_get_request() returned -11'
  dm-mpath: don't call blk_mq_delay_run_hw_queue() in case of
    BLK_STS_RESOURCE
  dm-mpath: return DM_MAPIO_REQUEUE in case of rq allocation failure
  dm-mpath: cache ti->clone during requeue
  dm-rq: improve I/O merge by dealing with underlying STS_RESOURCE

 block/blk-mq.c        | 17 +---------------
 drivers/md/dm-mpath.c | 51 ++++++++++++++++++++++++++++++++++------------
 drivers/md/dm-rq.c    | 56 +++++++++++++++++++++++++++++++++++++--------------
 3 files changed, 80 insertions(+), 44 deletions(-)

-- 
2.9.5

[PATCH 1/5] dm-mpath: remove annoying message of 'blk_get_request() returned -11'

[Ming Lei]

It is very normal to see allocation failure, so not necessary
to dump it and annoy people.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/md/dm-mpath.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 11f273d2f018..e8094d8fbe0d 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -499,8 +499,6 @@ static int multipath_clone_and_map(struct dm_target *ti, struct request *rq,
 	if (IS_ERR(clone)) {
 		/* EBUSY, ENODEV or EWOULDBLOCK: requeue */
 		bool queue_dying = blk_queue_dying(q);
-		DMERR_LIMIT("blk_get_request() returned %ld%s - requeuing",
-			    PTR_ERR(clone), queue_dying ? " (path offline)" : "");
 		if (queue_dying) {
 			atomic_inc(&m->pg_init_in_progress);
 			activate_or_offline_path(pgpath);
-- 
2.9.5

[PATCH 2/5] dm-mpath: don't call blk_mq_delay_run_hw_queue() in case of BLK_STS_RESOURCE

[Ming Lei]

If .queue_rq() returns BLK_STS_RESOURCE, blk-mq will rerun
the queue in the three situations:

1) if BLK_MQ_S_SCHED_RESTART is set
- queue is rerun after one rq is completed, see blk_mq_sched_restart()
which is run from blk_mq_free_request()

2) BLK_MQ_S_TAG_WAITING is set
- queue is rerun after one tag is freed

3) otherwise
- queue is run immediately in blk_mq_dispatch_rq_list()

This random dealy of running hw queue is introduced by commit
6077c2d706097c0(dm rq: Avoid that request processing stalls sporadically),
which claimed one request processing stalling is fixed,
but never explained the behind idea, and it is a workaound at most.
Even the question isn't explained by anyone in recent discussion.

Also calling blk_mq_delay_run_hw_queue() inside .queue_rq() is
a horrible hack because it makes BLK_MQ_S_SCHED_RESTART not
working, and will degrade I/O peformance a lot.

Finally this patch makes sure that dm-rq returns
BLK_STS_RESOURCE to blk-mq only when underlying queue is
out of resource, so we switch to return DM_MAPIO_DELAY_REQUEU
if either MPATHF_QUEUE_IO or MPATHF_PG_INIT_REQUIRED is set in
multipath_clone_and_map().

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/md/dm-mpath.c | 4 +---
 drivers/md/dm-rq.c    | 1 -
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index e8094d8fbe0d..97e4bd100fa1 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -484,9 +484,7 @@ static int multipath_clone_and_map(struct dm_target *ti, struct request *rq,
 		return DM_MAPIO_KILL;
 	} else if (test_bit(MPATHF_QUEUE_IO, &m->flags) ||
 		   test_bit(MPATHF_PG_INIT_REQUIRED, &m->flags)) {
-		if (pg_init_all_paths(m))
-			return DM_MAPIO_DELAY_REQUEUE;
-		return DM_MAPIO_REQUEUE;
+		return DM_MAPIO_DELAY_REQUEUE;
 	}
 
 	memset(mpio, 0, sizeof(*mpio));
diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
index f5e2b6967357..46f012185b43 100644
--- a/drivers/md/dm-rq.c
+++ b/drivers/md/dm-rq.c
@@ -758,7 +758,6 @@ static blk_status_t dm_mq_queue_rq(struct blk_mq_hw_ctx *hctx,
 		/* Undo dm_start_request() before requeuing */
 		rq_end_stats(md, rq);
 		rq_completed(md, rq_data_dir(rq), false);
-		blk_mq_delay_run_hw_queue(hctx, 100/*ms*/);
 		return BLK_STS_RESOURCE;
 	}
 
-- 
2.9.5

[PATCH 3/5] dm-mpath: return DM_MAPIO_REQUEUE in case of rq allocation failure

[Ming Lei]

blk-mq will rerun queue via RESTART after one request is completed,
so not necessary to wait random time for requeuing, we should trust
blk-mq to do it.

More importantly, we need return BLK_STS_RESOURCE to blk-mq
so that dequeue from I/O scheduler can be stopped, then
I/O merge gets improved.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/md/dm-mpath.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 97e4bd100fa1..9ee223170ee9 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -500,8 +500,20 @@ static int multipath_clone_and_map(struct dm_target *ti, struct request *rq,
 		if (queue_dying) {
 			atomic_inc(&m->pg_init_in_progress);
 			activate_or_offline_path(pgpath);
+			return DM_MAPIO_DELAY_REQUEUE;
 		}
-		return DM_MAPIO_DELAY_REQUEUE;
+
+		/*
+		 * blk-mq's SCHED_RESTART can cover this requeue, so
+		 * we needn't to deal with it by DELAY_REQUEUE. More
+		 * importantly, we have to return DM_MAPIO_REQUEUE
+		 * so that blk-mq can get the queue busy feedback,
+		 * otherwise I/O merge can be hurt.
+		 */
+		if (q->mq_ops)
+			return DM_MAPIO_REQUEUE;
+		else
+			return DM_MAPIO_DELAY_REQUEUE;
 	}
 	clone->bio = clone->biotail = NULL;
 	clone->rq_disk = bdev->bd_disk;
-- 
2.9.5

[PATCH 4/5] dm-mpath: cache ti->clone during requeue

[Ming Lei]

During requeue, block layer won't change the request any
more, such as no merge, so we can cache ti->clone and
let .clone_and_map_rq check if the cache can be hit.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/md/dm-mpath.c | 31 ++++++++++++++++++++++++-------
 drivers/md/dm-rq.c    | 41 +++++++++++++++++++++++++++++------------
 2 files changed, 53 insertions(+), 19 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 9ee223170ee9..52e4730541fd 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -457,6 +457,11 @@ do {									\
 		 dm_noflush_suspending((m)->ti));			\
 } while (0)
 
+static void multipath_release_clone(struct request *clone)
+{
+	blk_put_request(clone);
+}
+
 /*
  * Map cloned requests (request-based multipath)
  */
@@ -470,7 +475,7 @@ static int multipath_clone_and_map(struct dm_target *ti, struct request *rq,
 	struct block_device *bdev;
 	struct dm_mpath_io *mpio = get_mpio(map_context);
 	struct request_queue *q;
-	struct request *clone;
+	struct request *clone = *__clone;
 
 	/* Do we need to select a new pgpath? */
 	pgpath = lockless_dereference(m->current_pgpath);
@@ -493,7 +498,23 @@ static int multipath_clone_and_map(struct dm_target *ti, struct request *rq,
 
 	bdev = pgpath->path.dev->bdev;
 	q = bdev_get_queue(bdev);
-	clone = blk_get_request(q, rq->cmd_flags | REQ_NOMERGE, GFP_ATOMIC);
+
+	/*
+	 * This request may be from requeue path, and its clone
+	 * may have been allocated before. We need to check
+	 * if the cached clone can be hit.
+	 */
+	if (clone) {
+		if (clone->q != q) {
+			blk_rq_unprep_clone(clone);
+			multipath_release_clone(clone);
+			clone = NULL;
+		} else
+			goto start_io;
+	}
+
+	if (!clone)
+		clone = blk_get_request(q, rq->cmd_flags | REQ_NOMERGE, GFP_ATOMIC);
 	if (IS_ERR(clone)) {
 		/* EBUSY, ENODEV or EWOULDBLOCK: requeue */
 		bool queue_dying = blk_queue_dying(q);
@@ -520,6 +541,7 @@ static int multipath_clone_and_map(struct dm_target *ti, struct request *rq,
 	clone->cmd_flags |= REQ_FAILFAST_TRANSPORT;
 	*__clone = clone;
 
+ start_io:
 	if (pgpath->pg->ps.type->start_io)
 		pgpath->pg->ps.type->start_io(&pgpath->pg->ps,
 					      &pgpath->path,
@@ -527,11 +549,6 @@ static int multipath_clone_and_map(struct dm_target *ti, struct request *rq,
 	return DM_MAPIO_REMAPPED;
 }
 
-static void multipath_release_clone(struct request *clone)
-{
-	blk_put_request(clone);
-}
-
 /*
  * Map cloned bios (bio-based multipath)
  */
diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
index 46f012185b43..2ef524bddd38 100644
--- a/drivers/md/dm-rq.c
+++ b/drivers/md/dm-rq.c
@@ -221,6 +221,12 @@ static void dm_end_request(struct request *clone, blk_status_t error)
 	blk_rq_unprep_clone(clone);
 	tio->ti->type->release_clone_rq(clone);
 
+	/*
+	 * We move the clearing from tio_init in .queue_rq to here because
+	 * tio->clone may be cached during requeue
+	 */
+	tio->clone = NULL;
+
 	rq_end_stats(md, rq);
 	if (!rq->q->mq_ops)
 		blk_end_request_all(rq, error);
@@ -267,11 +273,15 @@ static void dm_requeue_original_request(struct dm_rq_target_io *tio, bool delay_
 	int rw = rq_data_dir(rq);
 	unsigned long delay_ms = delay_requeue ? 100 : 0;
 
+	/*
+	 * This request won't be changed any more during requeue,
+	 * so we cache tio->clone and let .clone_and_map_rq decide
+	 * to use the cached clone or allocate a new clone, and
+	 * the cached clone has to be freed before allocating a
+	 * new one.
+	 */
+
 	rq_end_stats(md, rq);
-	if (tio->clone) {
-		blk_rq_unprep_clone(tio->clone);
-		tio->ti->type->release_clone_rq(tio->clone);
-	}
 
 	if (!rq->q->mq_ops)
 		dm_old_requeue_request(rq, delay_ms);
@@ -448,7 +458,6 @@ static void init_tio(struct dm_rq_target_io *tio, struct request *rq,
 {
 	tio->md = md;
 	tio->ti = NULL;
-	tio->clone = NULL;
 	tio->orig = rq;
 	tio->error = 0;
 	tio->completed = 0;
@@ -456,8 +465,12 @@ static void init_tio(struct dm_rq_target_io *tio, struct request *rq,
 	 * Avoid initializing info for blk-mq; it passes
 	 * target-specific data through info.ptr
 	 * (see: dm_mq_init_request)
+	 *
+	 * If tio->clone is cached during requeue, we don't
+	 * clear tio->info, and delay the initialization
+	 * to .clone_and_map_rq if the cache isn't hit.
 	 */
-	if (!md->init_tio_pdu)
+	if (!md->init_tio_pdu && !tio->clone)
 		memset(&tio->info, 0, sizeof(tio->info));
 	if (md->kworker_task)
 		kthread_init_work(&tio->work, map_tio_request);
@@ -475,7 +488,8 @@ static int map_request(struct dm_rq_target_io *tio)
 	struct dm_target *ti = tio->ti;
 	struct mapped_device *md = tio->md;
 	struct request *rq = tio->orig;
-	struct request *clone = NULL;
+	struct request *cache = tio->clone;
+	struct request *clone = cache;
 
 	r = ti->type->clone_and_map_rq(ti, rq, &tio->info, &clone);
 	switch (r) {
@@ -483,10 +497,13 @@ static int map_request(struct dm_rq_target_io *tio)
 		/* The target has taken the I/O to submit by itself later */
 		break;
 	case DM_MAPIO_REMAPPED:
-		if (setup_clone(clone, rq, tio, GFP_ATOMIC)) {
-			/* -ENOMEM */
-			ti->type->release_clone_rq(clone);
-			return DM_MAPIO_REQUEUE;
+		/* cache not hit or not cached */
+		if (!cache || clone != cache) {
+			if (setup_clone(clone, rq, tio, GFP_ATOMIC)) {
+				/* -ENOMEM */
+				ti->type->release_clone_rq(clone);
+				return DM_MAPIO_REQUEUE;
+			}
 		}
 
 		/* The target has remapped the I/O so dispatch it */
@@ -555,7 +572,7 @@ static int __dm_rq_init_rq(struct mapped_device *md, struct request *rq)
 	 * be available in dm_mq_queue_rq.
 	 */
 	tio->md = md;
-
+	tio->clone = NULL;
 	if (md->init_tio_pdu) {
 		/* target-specific per-io data is immediately after the tio */
 		tio->info.ptr = tio + 1;
-- 
2.9.5

[PATCH 5/5] dm-rq: improve I/O merge by dealing with underlying STS_RESOURCE

[Ming Lei]

If the underlying queue returns BLK_STS_RESOURCE, we let dm-rq
handle the requeue instead of blk-mq, then I/O merge can be
improved because underlying's out-of-resource can be perceived
and handled by dm-rq now.

Follows IOPS test of mpath on lpfc, fio(libaio, bs:4k, dio,
queue_depth:64, 8 jobs).

1) blk-mq none scheduler
-----------------------------------------------------
 IOPS(K)  |v4.14-rc2    |v4.14-rc2 with| v4.14-rc2 with
          |             |[1][2]        | [1] [2] [3]
-----------------------------------------------------
read      |       53.69 |       40.26  |       94.61
-----------------------------------------------------
randread  |       24.64 |       30.08  |       35.57
-----------------------------------------------------
write     |       39.55 |       41.51  |      216.84
-----------------------------------------------------
randwrite |       33.97 |       34.27  |       33.98
-----------------------------------------------------

2) blk-mq mq-deadline scheduler
-----------------------------------------------------
 IOPS(K)  |v4.14-rc2    |v4.14-rc2 with| v4.14-rc2 with
          |             |[1][2]        | [1] [2] [3]
-----------------------------------------------------
 IOPS(K)  |MQ-DEADLINE  |MQ-DEADLINE  |MQ-DEADLINE
-----------------------------------------------------
read      |       23.81 |       21.91 |       89.94
-----------------------------------------------------
randread  |       38.47 |       38.96 |       38.02
-----------------------------------------------------
write     |       39.52 |        40.2 |      225.75
-----------------------------------------------------
randwrite |        34.8 |       33.73 |       33.44
-----------------------------------------------------

[1] [PATCH V5 0/7] blk-mq-sched: improve sequential I/O performance(part 1)

	https://marc.info/?l=linux-block&m=150676854821077&w=2

[2] [PATCH V5 0/8] blk-mq: improve bio merge for none scheduler

	https://marc.info/?l=linux-block&m=150677085521416&w=2

[3] this patchset

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c     | 17 +----------------
 drivers/md/dm-rq.c | 14 ++++++++++++--
 2 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 9a3a561a63b5..58d2268f9733 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1467,17 +1467,6 @@ void __blk_mq_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
 	blk_mq_hctx_mark_pending(hctx, ctx);
 }
 
-static void blk_mq_request_direct_insert(struct blk_mq_hw_ctx *hctx,
-					 struct request *rq)
-{
-	spin_lock(&hctx->lock);
-	list_add_tail(&rq->queuelist, &hctx->dispatch);
-	set_bit(BLK_MQ_S_DISPATCH_BUSY, &hctx->state);
-	spin_unlock(&hctx->lock);
-
-	blk_mq_run_hw_queue(hctx, false);
-}
-
 /*
  * Should only be used carefully, when the caller knows we want to
  * bypass a potential IO scheduler on the target device.
@@ -1487,12 +1476,8 @@ blk_status_t blk_mq_request_bypass_insert(struct request *rq)
 	struct blk_mq_ctx *ctx = rq->mq_ctx;
 	struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(rq->q, ctx->cpu);
 	blk_qc_t cookie;
-	blk_status_t ret;
 
-	ret = blk_mq_try_issue_directly(hctx, rq, &cookie, true);
-	if (ret == BLK_STS_RESOURCE)
-		blk_mq_request_direct_insert(hctx, rq);
-	return ret;
+	return blk_mq_try_issue_directly(hctx, rq, &cookie, true);
 }
 
 void blk_mq_insert_requests(struct blk_mq_hw_ctx *hctx, struct blk_mq_ctx *ctx,
diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
index 2ef524bddd38..feb49c4d6fa2 100644
--- a/drivers/md/dm-rq.c
+++ b/drivers/md/dm-rq.c
@@ -405,7 +405,7 @@ static void end_clone_request(struct request *clone, blk_status_t error)
 	dm_complete_request(tio->orig, error);
 }
 
-static void dm_dispatch_clone_request(struct request *clone, struct request *rq)
+static blk_status_t dm_dispatch_clone_request(struct request *clone, struct request *rq)
 {
 	blk_status_t r;
 
@@ -417,6 +417,7 @@ static void dm_dispatch_clone_request(struct request *clone, struct request *rq)
 	if (r != BLK_STS_OK && r != BLK_STS_RESOURCE)
 		/* must complete clone in terms of original request */
 		dm_complete_request(rq, r);
+	return r;
 }
 
 static int dm_rq_bio_constructor(struct bio *bio, struct bio *bio_orig,
@@ -490,8 +491,10 @@ static int map_request(struct dm_rq_target_io *tio)
 	struct request *rq = tio->orig;
 	struct request *cache = tio->clone;
 	struct request *clone = cache;
+	blk_status_t ret;
 
 	r = ti->type->clone_and_map_rq(ti, rq, &tio->info, &clone);
+ again:
 	switch (r) {
 	case DM_MAPIO_SUBMITTED:
 		/* The target has taken the I/O to submit by itself later */
@@ -509,7 +512,14 @@ static int map_request(struct dm_rq_target_io *tio)
 		/* The target has remapped the I/O so dispatch it */
 		trace_block_rq_remap(clone->q, clone, disk_devt(dm_disk(md)),
 				     blk_rq_pos(rq));
-		dm_dispatch_clone_request(clone, rq);
+		ret = dm_dispatch_clone_request(clone, rq);
+		if (ret == BLK_STS_RESOURCE) {
+			if (!rq->q->mq_ops)
+				r = DM_MAPIO_DELAY_REQUEUE;
+			else
+				r = DM_MAPIO_REQUEUE;
+			goto again;
+		}
 		break;
 	case DM_MAPIO_REQUEUE:
 		/* The target wants to requeue the I/O */
-- 
2.9.5