* [PATCH 1/5] dm-mpath: remove annoying message of 'blk_get_request() returned -11'
2017-09-30 11:46 [PATCH 0/5] dm-rq: improve sequential I/O performance Ming Lei
@ 2017-09-30 11:46 ` Ming Lei
2017-09-30 11:46 ` [PATCH 2/5] dm-mpath: don't call blk_mq_delay_run_hw_queue() in case of BLK_STS_RESOURCE Ming Lei
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Ming Lei @ 2017-09-30 11:46 UTC (permalink / raw)
To: Jens Axboe, linux-block, Christoph Hellwig, Mike Snitzer, dm-devel
Cc: Bart Van Assche, Laurence Oberman, linux-kernel, Omar Sandoval, 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
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 2/5] dm-mpath: don't call blk_mq_delay_run_hw_queue() in case of BLK_STS_RESOURCE
2017-09-30 11:46 [PATCH 0/5] dm-rq: improve sequential I/O performance Ming Lei
2017-09-30 11:46 ` [PATCH 1/5] dm-mpath: remove annoying message of 'blk_get_request() returned -11' Ming Lei
@ 2017-09-30 11:46 ` Ming Lei
2017-09-30 11:46 ` [PATCH 3/5] dm-mpath: return DM_MAPIO_REQUEUE in case of rq allocation failure Ming Lei
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Ming Lei @ 2017-09-30 11:46 UTC (permalink / raw)
To: Jens Axboe, linux-block, Christoph Hellwig, Mike Snitzer, dm-devel
Cc: Bart Van Assche, Laurence Oberman, linux-kernel, Omar Sandoval, 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
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 3/5] dm-mpath: return DM_MAPIO_REQUEUE in case of rq allocation failure
2017-09-30 11:46 [PATCH 0/5] dm-rq: improve sequential I/O performance Ming Lei
2017-09-30 11:46 ` [PATCH 1/5] dm-mpath: remove annoying message of 'blk_get_request() returned -11' Ming Lei
2017-09-30 11:46 ` [PATCH 2/5] dm-mpath: don't call blk_mq_delay_run_hw_queue() in case of BLK_STS_RESOURCE Ming Lei
@ 2017-09-30 11:46 ` Ming Lei
2017-09-30 11:46 ` [PATCH 4/5] dm-mpath: cache ti->clone during requeue Ming Lei
2017-09-30 11:46 ` [PATCH 5/5] dm-rq: improve I/O merge by dealing with underlying STS_RESOURCE Ming Lei
4 siblings, 0 replies; 6+ messages in thread
From: Ming Lei @ 2017-09-30 11:46 UTC (permalink / raw)
To: Jens Axboe, linux-block, Christoph Hellwig, Mike Snitzer, dm-devel
Cc: Bart Van Assche, Laurence Oberman, linux-kernel, Omar Sandoval, 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
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 4/5] dm-mpath: cache ti->clone during requeue
2017-09-30 11:46 [PATCH 0/5] dm-rq: improve sequential I/O performance Ming Lei
` (2 preceding siblings ...)
2017-09-30 11:46 ` [PATCH 3/5] dm-mpath: return DM_MAPIO_REQUEUE in case of rq allocation failure Ming Lei
@ 2017-09-30 11:46 ` Ming Lei
2017-09-30 11:46 ` [PATCH 5/5] dm-rq: improve I/O merge by dealing with underlying STS_RESOURCE Ming Lei
4 siblings, 0 replies; 6+ messages in thread
From: Ming Lei @ 2017-09-30 11:46 UTC (permalink / raw)
To: Jens Axboe, linux-block, Christoph Hellwig, Mike Snitzer, dm-devel
Cc: Bart Van Assche, Laurence Oberman, linux-kernel, Omar Sandoval, 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
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 5/5] dm-rq: improve I/O merge by dealing with underlying STS_RESOURCE
2017-09-30 11:46 [PATCH 0/5] dm-rq: improve sequential I/O performance Ming Lei
` (3 preceding siblings ...)
2017-09-30 11:46 ` [PATCH 4/5] dm-mpath: cache ti->clone during requeue Ming Lei
@ 2017-09-30 11:46 ` Ming Lei
4 siblings, 0 replies; 6+ messages in thread
From: Ming Lei @ 2017-09-30 11:46 UTC (permalink / raw)
To: Jens Axboe, linux-block, Christoph Hellwig, Mike Snitzer, dm-devel
Cc: Bart Van Assche, Laurence Oberman, linux-kernel, Omar Sandoval, 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
^ permalink raw reply [flat|nested] 6+ messages in thread