* [PATCHv3 0/5] dm-multipath: push back requests instead of queueing
@ 2014-01-31 9:10 Hannes Reinecke
2014-01-31 9:10 ` [PATCH 1/5] dm-multipath: Do not call pg_init twice Hannes Reinecke
` (4 more replies)
0 siblings, 5 replies; 12+ messages in thread
From: Hannes Reinecke @ 2014-01-31 9:10 UTC (permalink / raw)
To: Alasdair Kergon; +Cc: Jun'ichi Nomura, dm-devel, Mike Snitzer
Hi all,
dm-multipath still carries around it's own queueing framework for
implementing 'queue_if_no_path'.
However, there is no real reason for this; we could as well
push back the requests onto the request_queue.
In doing so we can also reduce the memory pressure during
fail_if_no_path scenarios, as we don't have to allocate a
context for each request when it need to be requeued.
This patchset is the third version, including
the review by Jun'ichi Nomura and suggestions from Mike Snitzer.
Hannes Reinecke (5):
dm-multipath: Do not call pg_init twice
dm-multipath: push back requests instead of queueing
dm-multipath: remove process_queued_ios()
dm-multipath: reduce memory pressure during requeuing
dm-multipath: remove map_io()
drivers/md/dm-mpath.c | 214 +++++++++++++++---------------------------
drivers/md/dm.c | 13 +++
include/linux/device-mapper.h | 1 +
3 files changed, 88 insertions(+), 140 deletions(-)
--
1.7.12.4
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/5] dm-multipath: Do not call pg_init twice
2014-01-31 9:10 [PATCHv3 0/5] dm-multipath: push back requests instead of queueing Hannes Reinecke
@ 2014-01-31 9:10 ` Hannes Reinecke
2014-01-31 9:10 ` [PATCH 2/5] dm-multipath: push back requests instead of queueing Hannes Reinecke
` (3 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Hannes Reinecke @ 2014-01-31 9:10 UTC (permalink / raw)
To: Alasdair Kergon; +Cc: Jun'ichi Nomura, dm-devel, Mike Snitzer
When pg_init is running we shouldn't be calling the same
routine twice; we need to wait for the first pg_init to
complete.
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
drivers/md/dm-mpath.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 6eb9dc9..d45290a 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -261,6 +261,9 @@ static void __pg_init_all_paths(struct multipath *m)
struct pgpath *pgpath;
unsigned long pg_init_delay = 0;
+ if (m->pg_init_in_progress || m->pg_init_disabled)
+ return;
+
m->pg_init_count++;
m->pg_init_required = 0;
if (m->pg_init_delay_retry)
--
1.7.12.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/5] dm-multipath: push back requests instead of queueing
2014-01-31 9:10 [PATCHv3 0/5] dm-multipath: push back requests instead of queueing Hannes Reinecke
2014-01-31 9:10 ` [PATCH 1/5] dm-multipath: Do not call pg_init twice Hannes Reinecke
@ 2014-01-31 9:10 ` Hannes Reinecke
2014-01-31 15:40 ` Mike Snitzer
2014-01-31 9:10 ` [PATCH 3/5] dm-multipath: remove process_queued_ios() Hannes Reinecke
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Hannes Reinecke @ 2014-01-31 9:10 UTC (permalink / raw)
To: Alasdair Kergon; +Cc: Jun'ichi Nomura, dm-devel, Mike Snitzer
There is no reason why multipath needs to queue requests
internally for queue_if_no_path or pg_init; we should
rather push them back onto the request queue.
And while we're at it we can simplify the conditional
statement in map_io() to make it easier to read.
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
drivers/md/dm-mpath.c | 115 ++++++++++++++----------------------------
drivers/md/dm.c | 13 +++++
include/linux/device-mapper.h | 1 +
3 files changed, 52 insertions(+), 77 deletions(-)
diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index d45290a..39fd166 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -93,9 +93,7 @@ struct multipath {
unsigned pg_init_count; /* Number of times pg_init called */
unsigned pg_init_delay_msecs; /* Number of msecs before pg_init retry */
- unsigned queue_size;
struct work_struct process_queued_ios;
- struct list_head queued_ios;
struct work_struct trigger_event;
@@ -124,6 +122,7 @@ static struct workqueue_struct *kmultipathd, *kmpath_handlerd;
static void process_queued_ios(struct work_struct *work);
static void trigger_event(struct work_struct *work);
static void activate_path(struct work_struct *work);
+static int __pgpath_busy(struct pgpath *pgpath);
/*-----------------------------------------------
@@ -195,7 +194,6 @@ static struct multipath *alloc_multipath(struct dm_target *ti)
m = kzalloc(sizeof(*m), GFP_KERNEL);
if (m) {
INIT_LIST_HEAD(&m->priority_groups);
- INIT_LIST_HEAD(&m->queued_ios);
spin_lock_init(&m->lock);
m->queue_io = 1;
m->pg_init_delay_msecs = DM_PG_INIT_DELAY_DEFAULT;
@@ -368,12 +366,15 @@ failed:
*/
static int __must_push_back(struct multipath *m)
{
- return (m->queue_if_no_path != m->saved_queue_if_no_path &&
- dm_noflush_suspending(m->ti));
+ return (m->queue_if_no_path ||
+ (m->queue_if_no_path != m->saved_queue_if_no_path &&
+ dm_noflush_suspending(m->ti)));
}
+#define pg_ready(m) (!(m)->queue_io && !(m)->pg_init_required)
+
static int map_io(struct multipath *m, struct request *clone,
- union map_info *map_context, unsigned was_queued)
+ union map_info *map_context)
{
int r = DM_MAPIO_REMAPPED;
size_t nr_bytes = blk_rq_bytes(clone);
@@ -391,37 +392,30 @@ static int map_io(struct multipath *m, struct request *clone,
pgpath = m->current_pgpath;
- if (was_queued)
- m->queue_size--;
-
- if (m->pg_init_required) {
- if (!m->pg_init_in_progress)
- queue_work(kmultipathd, &m->process_queued_ios);
- r = DM_MAPIO_REQUEUE;
- } else if ((pgpath && m->queue_io) ||
- (!pgpath && m->queue_if_no_path)) {
- /* Queue for the daemon to resubmit */
- list_add_tail(&clone->queuelist, &m->queued_ios);
- m->queue_size++;
- if (!m->queue_io)
- queue_work(kmultipathd, &m->process_queued_ios);
- pgpath = NULL;
- r = DM_MAPIO_SUBMITTED;
- } else if (pgpath) {
- bdev = pgpath->path.dev->bdev;
- clone->q = bdev_get_queue(bdev);
- clone->rq_disk = bdev->bd_disk;
- } else if (__must_push_back(m))
- r = DM_MAPIO_REQUEUE;
- else
- r = -EIO; /* Failed */
-
- mpio->pgpath = pgpath;
- mpio->nr_bytes = nr_bytes;
-
- if (r == DM_MAPIO_REMAPPED && pgpath->pg->ps.type->start_io)
- pgpath->pg->ps.type->start_io(&pgpath->pg->ps, &pgpath->path,
- nr_bytes);
+ if (pgpath) {
+ if (__pgpath_busy(pgpath))
+ r = DM_MAPIO_REQUEUE;
+ else if (pg_ready(m)) {
+ bdev = pgpath->path.dev->bdev;
+ clone->q = bdev_get_queue(bdev);
+ clone->rq_disk = bdev->bd_disk;
+ mpio->pgpath = pgpath;
+ mpio->nr_bytes = nr_bytes;
+ if (pgpath->pg->ps.type->start_io)
+ pgpath->pg->ps.type->start_io(&pgpath->pg->ps,
+ &pgpath->path,
+ nr_bytes);
+ } else {
+ __pg_init_all_paths(m);
+ r = DM_MAPIO_REQUEUE;
+ }
+ } else {
+ /* No path */
+ if (__must_push_back(m))
+ r = DM_MAPIO_REQUEUE;
+ else
+ r = -EIO; /* Failed */
+ }
spin_unlock_irqrestore(&m->lock, flags);
@@ -443,7 +437,7 @@ static int queue_if_no_path(struct multipath *m, unsigned queue_if_no_path,
else
m->saved_queue_if_no_path = queue_if_no_path;
m->queue_if_no_path = queue_if_no_path;
- if (!m->queue_if_no_path && m->queue_size)
+ if (!m->queue_if_no_path)
queue_work(kmultipathd, &m->process_queued_ios);
spin_unlock_irqrestore(&m->lock, flags);
@@ -451,40 +445,6 @@ static int queue_if_no_path(struct multipath *m, unsigned queue_if_no_path,
return 0;
}
-/*-----------------------------------------------------------------
- * The multipath daemon is responsible for resubmitting queued ios.
- *---------------------------------------------------------------*/
-
-static void dispatch_queued_ios(struct multipath *m)
-{
- int r;
- unsigned long flags;
- union map_info *info;
- struct request *clone, *n;
- LIST_HEAD(cl);
-
- spin_lock_irqsave(&m->lock, flags);
- list_splice_init(&m->queued_ios, &cl);
- spin_unlock_irqrestore(&m->lock, flags);
-
- list_for_each_entry_safe(clone, n, &cl, queuelist) {
- list_del_init(&clone->queuelist);
-
- info = dm_get_rq_mapinfo(clone);
-
- r = map_io(m, clone, info, 1);
- if (r < 0) {
- clear_mapinfo(m, info);
- dm_kill_unmapped_request(clone, r);
- } else if (r == DM_MAPIO_REMAPPED)
- dm_dispatch_request(clone);
- else if (r == DM_MAPIO_REQUEUE) {
- clear_mapinfo(m, info);
- dm_requeue_unmapped_request(clone);
- }
- }
-}
-
static void process_queued_ios(struct work_struct *work)
{
struct multipath *m =
@@ -510,7 +470,7 @@ static void process_queued_ios(struct work_struct *work)
spin_unlock_irqrestore(&m->lock, flags);
if (!must_queue)
- dispatch_queued_ios(m);
+ dm_table_run_queue(m->ti->table);
}
/*
@@ -988,7 +948,7 @@ static int multipath_map(struct dm_target *ti, struct request *clone,
return DM_MAPIO_REQUEUE;
clone->cmd_flags |= REQ_FAILFAST_TRANSPORT;
- r = map_io(m, clone, map_context, 0);
+ r = map_io(m, clone, map_context);
if (r < 0 || r == DM_MAPIO_REQUEUE)
clear_mapinfo(m, map_context);
@@ -1057,7 +1017,7 @@ static int reinstate_path(struct pgpath *pgpath)
pgpath->is_active = 1;
- if (!m->nr_valid_paths++ && m->queue_size) {
+ if (!m->nr_valid_paths++) {
m->current_pgpath = NULL;
queue_work(kmultipathd, &m->process_queued_ios);
} else if (m->hw_handler_name && (m->current_pg == pgpath->pg)) {
@@ -1436,7 +1396,8 @@ static void multipath_status(struct dm_target *ti, status_type_t type,
/* Features */
if (type == STATUSTYPE_INFO)
- DMEMIT("2 %u %u ", m->queue_size, m->pg_init_count);
+ DMEMIT("2 %u %u ", (m->queue_io << 1) + m->queue_if_no_path,
+ m->pg_init_count);
else {
DMEMIT("%u ", m->queue_if_no_path +
(m->pg_init_retries > 0) * 2 +
@@ -1684,7 +1645,7 @@ static int multipath_busy(struct dm_target *ti)
spin_lock_irqsave(&m->lock, flags);
/* pg_init in progress, requeue until done */
- if (m->pg_init_in_progress) {
+ if (!pg_ready(m)) {
busy = 1;
goto out;
}
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 0704c52..291491b 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1912,6 +1912,19 @@ static int dm_any_congested(void *congested_data, int bdi_bits)
return r;
}
+void dm_table_run_queue(struct dm_table *t)
+{
+ struct mapped_device *md = dm_table_get_md(t);
+ unsigned long flags;
+
+ if (md->queue) {
+ spin_lock_irqsave(md->queue->queue_lock, flags);
+ blk_run_queue_async(md->queue);
+ spin_unlock_irqrestore(md->queue->queue_lock, flags);
+ }
+}
+EXPORT_SYMBOL_GPL(dm_table_run_queue);
+
/*-----------------------------------------------------------------
* An IDR is used to keep track of allocated minor numbers.
*---------------------------------------------------------------*/
diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
index ed419c6..a33653f 100644
--- a/include/linux/device-mapper.h
+++ b/include/linux/device-mapper.h
@@ -604,5 +604,6 @@ void dm_dispatch_request(struct request *rq);
void dm_requeue_unmapped_request(struct request *rq);
void dm_kill_unmapped_request(struct request *rq, int error);
int dm_underlying_device_busy(struct request_queue *q);
+void dm_table_run_queue(struct dm_table *t);
#endif /* _LINUX_DEVICE_MAPPER_H */
--
1.7.12.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/5] dm-multipath: remove process_queued_ios()
2014-01-31 9:10 [PATCHv3 0/5] dm-multipath: push back requests instead of queueing Hannes Reinecke
2014-01-31 9:10 ` [PATCH 1/5] dm-multipath: Do not call pg_init twice Hannes Reinecke
2014-01-31 9:10 ` [PATCH 2/5] dm-multipath: push back requests instead of queueing Hannes Reinecke
@ 2014-01-31 9:10 ` Hannes Reinecke
2014-01-31 9:43 ` Junichi Nomura
2014-01-31 9:10 ` [PATCH 4/5] dm-multipath: reduce memory pressure during requeuing Hannes Reinecke
2014-01-31 9:10 ` [PATCH 5/5] dm-multipath: remove map_io() Hannes Reinecke
4 siblings, 1 reply; 12+ messages in thread
From: Hannes Reinecke @ 2014-01-31 9:10 UTC (permalink / raw)
To: Alasdair Kergon; +Cc: Jun'ichi Nomura, dm-devel, Mike Snitzer
Doesn't serve any real purpose anymore; dm_table_run_queue()
will already move things to a workqueue, so we don't need
to do it ourselves.
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
drivers/md/dm-mpath.c | 67 ++++++++++++++++++++-------------------------------
1 file changed, 26 insertions(+), 41 deletions(-)
diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 39fd166..7122ac3 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -93,8 +93,6 @@ struct multipath {
unsigned pg_init_count; /* Number of times pg_init called */
unsigned pg_init_delay_msecs; /* Number of msecs before pg_init retry */
- struct work_struct process_queued_ios;
-
struct work_struct trigger_event;
/*
@@ -119,7 +117,6 @@ typedef int (*action_fn) (struct pgpath *pgpath);
static struct kmem_cache *_mpio_cache;
static struct workqueue_struct *kmultipathd, *kmpath_handlerd;
-static void process_queued_ios(struct work_struct *work);
static void trigger_event(struct work_struct *work);
static void activate_path(struct work_struct *work);
static int __pgpath_busy(struct pgpath *pgpath);
@@ -197,7 +194,6 @@ static struct multipath *alloc_multipath(struct dm_target *ti)
spin_lock_init(&m->lock);
m->queue_io = 1;
m->pg_init_delay_msecs = DM_PG_INIT_DELAY_DEFAULT;
- INIT_WORK(&m->process_queued_ios, process_queued_ios);
INIT_WORK(&m->trigger_event, trigger_event);
init_waitqueue_head(&m->pg_init_wait);
mutex_init(&m->work_mutex);
@@ -438,41 +434,13 @@ static int queue_if_no_path(struct multipath *m, unsigned queue_if_no_path,
m->saved_queue_if_no_path = queue_if_no_path;
m->queue_if_no_path = queue_if_no_path;
if (!m->queue_if_no_path)
- queue_work(kmultipathd, &m->process_queued_ios);
+ dm_table_run_queue(m->ti->table);
spin_unlock_irqrestore(&m->lock, flags);
return 0;
}
-static void process_queued_ios(struct work_struct *work)
-{
- struct multipath *m =
- container_of(work, struct multipath, process_queued_ios);
- struct pgpath *pgpath = NULL;
- unsigned must_queue = 1;
- unsigned long flags;
-
- spin_lock_irqsave(&m->lock, flags);
-
- if (!m->current_pgpath)
- __choose_pgpath(m, 0);
-
- pgpath = m->current_pgpath;
-
- if ((pgpath && !m->queue_io) ||
- (!pgpath && !m->queue_if_no_path))
- must_queue = 0;
-
- if (m->pg_init_required && !m->pg_init_in_progress && pgpath &&
- !m->pg_init_disabled)
- __pg_init_all_paths(m);
-
- spin_unlock_irqrestore(&m->lock, flags);
- if (!must_queue)
- dm_table_run_queue(m->ti->table);
-}
-
/*
* An event is triggered whenever a path is taken out of use.
* Includes path failure and PG bypass.
@@ -1019,7 +987,7 @@ static int reinstate_path(struct pgpath *pgpath)
if (!m->nr_valid_paths++) {
m->current_pgpath = NULL;
- queue_work(kmultipathd, &m->process_queued_ios);
+ dm_table_run_queue(m->ti->table);
} else if (m->hw_handler_name && (m->current_pg == pgpath->pg)) {
if (queue_work(kmpath_handlerd, &pgpath->activate_path.work))
m->pg_init_in_progress++;
@@ -1158,7 +1126,7 @@ static void pg_init_done(void *data, int errors)
struct priority_group *pg = pgpath->pg;
struct multipath *m = pg->m;
unsigned long flags;
- unsigned delay_retry = 0;
+ unsigned long pg_init_delay = 0;
/* device or driver problems */
switch (errors) {
@@ -1185,7 +1153,7 @@ static void pg_init_done(void *data, int errors)
break;
case SCSI_DH_RETRY:
/* Wait before retrying. */
- delay_retry = 1;
+ pg_init_delay = msecs_to_jiffies(m->pg_init_delay_msecs);
case SCSI_DH_IMM_RETRY:
case SCSI_DH_RES_TEMP_UNAVAIL:
if (pg_init_limit_reached(m, pgpath))
@@ -1217,9 +1185,21 @@ static void pg_init_done(void *data, int errors)
if (!m->pg_init_required)
m->queue_io = 0;
-
- m->pg_init_delay_retry = delay_retry;
- queue_work(kmultipathd, &m->process_queued_ios);
+ else {
+ if (!pg_init_delay) {
+ m->pg_init_delay_retry = 0;
+ /*
+ * Add a small delay to move the
+ * activation to a workqueue
+ */
+ pg_init_delay = HZ / 50;
+ } else
+ m->pg_init_delay_retry = 1;
+ if (queue_delayed_work(kmpath_handlerd, &pgpath->activate_path,
+ pg_init_delay))
+ m->pg_init_in_progress++;
+ goto out;
+ }
/*
* Wake up any thread waiting to suspend.
@@ -1593,8 +1573,13 @@ static int multipath_ioctl(struct dm_target *ti, unsigned int cmd,
if (!r && ti->len != i_size_read(bdev->bd_inode) >> SECTOR_SHIFT)
r = scsi_verify_blk_ioctl(NULL, cmd);
- if (r == -ENOTCONN && !fatal_signal_pending(current))
- queue_work(kmultipathd, &m->process_queued_ios);
+ if (r == -ENOTCONN && !fatal_signal_pending(current)) {
+ spin_lock_irqsave(&m->lock, flags);
+ if (m->current_pgpath && m->pg_init_required)
+ __pg_init_all_paths(m);
+ spin_unlock_irqrestore(&m->lock, flags);
+ dm_table_run_queue(m->ti->table);
+ }
return r ? : __blkdev_driver_ioctl(bdev, mode, cmd, arg);
}
--
1.7.12.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 4/5] dm-multipath: reduce memory pressure during requeuing
2014-01-31 9:10 [PATCHv3 0/5] dm-multipath: push back requests instead of queueing Hannes Reinecke
` (2 preceding siblings ...)
2014-01-31 9:10 ` [PATCH 3/5] dm-multipath: remove process_queued_ios() Hannes Reinecke
@ 2014-01-31 9:10 ` Hannes Reinecke
2014-01-31 9:10 ` [PATCH 5/5] dm-multipath: remove map_io() Hannes Reinecke
4 siblings, 0 replies; 12+ messages in thread
From: Hannes Reinecke @ 2014-01-31 9:10 UTC (permalink / raw)
To: Alasdair Kergon; +Cc: Jun'ichi Nomura, dm-devel, Mike Snitzer
When multipath needs to requeue I/O in the block layer
the per-request context shouldn't be allocated, as it will
be freed immediately afterwards anyway.
Avoiding this memory allocation will reduce memory
pressure during requeuing.
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
drivers/md/dm-mpath.c | 40 ++++++++++++++++------------------------
1 file changed, 16 insertions(+), 24 deletions(-)
diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 7122ac3..7146f7b 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -372,12 +372,12 @@ static int __must_push_back(struct multipath *m)
static int map_io(struct multipath *m, struct request *clone,
union map_info *map_context)
{
- int r = DM_MAPIO_REMAPPED;
+ int r = DM_MAPIO_REQUEUE;
size_t nr_bytes = blk_rq_bytes(clone);
unsigned long flags;
struct pgpath *pgpath;
struct block_device *bdev;
- struct dm_mpath_io *mpio = map_context->ptr;
+ struct dm_mpath_io *mpio;
spin_lock_irqsave(&m->lock, flags);
@@ -390,29 +390,31 @@ static int map_io(struct multipath *m, struct request *clone,
if (pgpath) {
if (__pgpath_busy(pgpath))
- r = DM_MAPIO_REQUEUE;
- else if (pg_ready(m)) {
+ goto out_unlock;
+
+ if (pg_ready(m)) {
+ if (set_mapinfo(m, map_context) < 0)
+ goto out_unlock;
+
bdev = pgpath->path.dev->bdev;
clone->q = bdev_get_queue(bdev);
clone->rq_disk = bdev->bd_disk;
+ clone->cmd_flags |= REQ_FAILFAST_TRANSPORT;
+ mpio = map_context->ptr;
mpio->pgpath = pgpath;
mpio->nr_bytes = nr_bytes;
if (pgpath->pg->ps.type->start_io)
pgpath->pg->ps.type->start_io(&pgpath->pg->ps,
&pgpath->path,
nr_bytes);
- } else {
- __pg_init_all_paths(m);
- r = DM_MAPIO_REQUEUE;
+ r = DM_MAPIO_REMAPPED;
+ goto out_unlock;
}
- } else {
- /* No path */
- if (__must_push_back(m))
- r = DM_MAPIO_REQUEUE;
- else
+ __pg_init_all_paths(m);
+ } else if (!__must_push_back(m))
r = -EIO; /* Failed */
- }
+out_unlock:
spin_unlock_irqrestore(&m->lock, flags);
return r;
@@ -908,19 +910,9 @@ static void multipath_dtr(struct dm_target *ti)
static int multipath_map(struct dm_target *ti, struct request *clone,
union map_info *map_context)
{
- int r;
struct multipath *m = (struct multipath *) ti->private;
- if (set_mapinfo(m, map_context) < 0)
- /* ENOMEM, requeue */
- return DM_MAPIO_REQUEUE;
-
- clone->cmd_flags |= REQ_FAILFAST_TRANSPORT;
- r = map_io(m, clone, map_context);
- if (r < 0 || r == DM_MAPIO_REQUEUE)
- clear_mapinfo(m, map_context);
-
- return r;
+ return map_io(m, clone, map_context);
}
/*
--
1.7.12.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 5/5] dm-multipath: remove map_io()
2014-01-31 9:10 [PATCHv3 0/5] dm-multipath: push back requests instead of queueing Hannes Reinecke
` (3 preceding siblings ...)
2014-01-31 9:10 ` [PATCH 4/5] dm-multipath: reduce memory pressure during requeuing Hannes Reinecke
@ 2014-01-31 9:10 ` Hannes Reinecke
4 siblings, 0 replies; 12+ messages in thread
From: Hannes Reinecke @ 2014-01-31 9:10 UTC (permalink / raw)
To: Alasdair Kergon; +Cc: Jun'ichi Nomura, dm-devel, Mike Snitzer
multipath_map() is now just a wrapper around map_io(), so we
can fold both into multipath_map().
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
drivers/md/dm-mpath.c | 19 ++++++-------------
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 7146f7b..4974aac 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -369,9 +369,13 @@ static int __must_push_back(struct multipath *m)
#define pg_ready(m) (!(m)->queue_io && !(m)->pg_init_required)
-static int map_io(struct multipath *m, struct request *clone,
- union map_info *map_context)
+/*
+ * Map cloned requests
+ */
+static int multipath_map(struct dm_target *ti, struct request *clone,
+ union map_info *map_context)
{
+ struct multipath *m = (struct multipath *) ti->private;
int r = DM_MAPIO_REQUEUE;
size_t nr_bytes = blk_rq_bytes(clone);
unsigned long flags;
@@ -905,17 +909,6 @@ static void multipath_dtr(struct dm_target *ti)
}
/*
- * Map cloned requests
- */
-static int multipath_map(struct dm_target *ti, struct request *clone,
- union map_info *map_context)
-{
- struct multipath *m = (struct multipath *) ti->private;
-
- return map_io(m, clone, map_context);
-}
-
-/*
* Take a path out of use.
*/
static int fail_path(struct pgpath *pgpath)
--
1.7.12.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/5] dm-multipath: remove process_queued_ios()
2014-01-31 9:10 ` [PATCH 3/5] dm-multipath: remove process_queued_ios() Hannes Reinecke
@ 2014-01-31 9:43 ` Junichi Nomura
2014-01-31 14:55 ` Hannes Reinecke
0 siblings, 1 reply; 12+ messages in thread
From: Junichi Nomura @ 2014-01-31 9:43 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: dm-devel, Mike Snitzer, Alasdair Kergon
On 01/31/14 18:10, Hannes Reinecke wrote:
> @@ -1217,9 +1185,21 @@ static void pg_init_done(void *data, int errors)
>
> if (!m->pg_init_required)
> m->queue_io = 0;
> -
> - m->pg_init_delay_retry = delay_retry;
> - queue_work(kmultipathd, &m->process_queued_ios);
> + else {
> + if (!pg_init_delay) {
> + m->pg_init_delay_retry = 0;
> + /*
> + * Add a small delay to move the
> + * activation to a workqueue
> + */
> + pg_init_delay = HZ / 50;
> + } else
> + m->pg_init_delay_retry = 1;
> + if (queue_delayed_work(kmpath_handlerd, &pgpath->activate_path,
> + pg_init_delay))
> + m->pg_init_in_progress++;
> + goto out;
> + }
This code is called when pg_init_progress becomes 0,
that means it is possible that multiple pgpaths have requested retries.
So you have to activate_path for all non-active pgpath in this pg,
like __pg_init_all_paths() does in the current code.
Also, you aren't clearing pg_init_required here.
I think it causes extra pg_init unnecessarily.
What do you think if we just call __pg_init_all_paths() here
instead of open coding it?
--
Jun'ichi Nomura, NEC Corporation
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/5] dm-multipath: remove process_queued_ios()
2014-01-31 9:43 ` Junichi Nomura
@ 2014-01-31 14:55 ` Hannes Reinecke
2014-02-03 10:26 ` Junichi Nomura
0 siblings, 1 reply; 12+ messages in thread
From: Hannes Reinecke @ 2014-01-31 14:55 UTC (permalink / raw)
To: Junichi Nomura; +Cc: dm-devel, Mike Snitzer, Alasdair Kergon
On 01/31/2014 10:43 AM, Junichi Nomura wrote:
> On 01/31/14 18:10, Hannes Reinecke wrote:
>> @@ -1217,9 +1185,21 @@ static void pg_init_done(void *data, int errors)
>>
>> if (!m->pg_init_required)
>> m->queue_io = 0;
>> -
>> - m->pg_init_delay_retry = delay_retry;
>> - queue_work(kmultipathd, &m->process_queued_ios);
>> + else {
>> + if (!pg_init_delay) {
>> + m->pg_init_delay_retry = 0;
>> + /*
>> + * Add a small delay to move the
>> + * activation to a workqueue
>> + */
>> + pg_init_delay = HZ / 50;
>> + } else
>> + m->pg_init_delay_retry = 1;
>> + if (queue_delayed_work(kmpath_handlerd, &pgpath->activate_path,
>> + pg_init_delay))
>> + m->pg_init_in_progress++;
>> + goto out;
>> + }
>
> This code is called when pg_init_progress becomes 0,
> that means it is possible that multiple pgpaths have requested retries.
> So you have to activate_path for all non-active pgpath in this pg,
> like __pg_init_all_paths() does in the current code.
>
Not necessarily. SCSI_DH_RETRY (initially) is per-path, so it's well
feasible that pg_init on the first path returns SCSI_DH_OK, and
pg_init on the other path returns SCSI_DH_RETRY.
> Also, you aren't clearing pg_init_required here.
> I think it causes extra pg_init unnecessarily.
>
Indeed. I'll need to fix this up.
> What do you think if we just call __pg_init_all_paths() here
> instead of open coding it?
>
That depends on the intended policy.
Problem here is that pg_init_done() is per path, so at face value
SCSI_DH_RETRY is per path, too.
So from that we should be retrying this path (and this path only).
Hence it would be correct to call queue_delayed_work directly.
However, typically any pg_init affects _every_ path in the multipath
device (active paths become passive and vice versa).
Which seems to be the intended usage, as we're checking for
pg_init_in_progress prior to invoking queue_delayed_work().
But _if_ we assume that, then we only need to send a _single_
pg_init, as this will switch all paths. So again, a call to
__pg_init_all_paths will not do the correct thing as it'll
send activations to _every_ active path.
(And, in fact, we're trying really hard in scsi_dh_rdac
and scsi_dh_alua to bunch together all the various pg_init
requests precisely for the cited reason).
So my inclination here would be to treat SCSI_DH_RETRY
as _per path_, and retry only this specific path.
IE removing the check to pg_init_in_progress and call
queue_delayed_work() directly.
IMHO this would impose the least restriction on
the internal workings of the various device handlers.
What do you think?
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/5] dm-multipath: push back requests instead of queueing
2014-01-31 9:10 ` [PATCH 2/5] dm-multipath: push back requests instead of queueing Hannes Reinecke
@ 2014-01-31 15:40 ` Mike Snitzer
2014-02-01 13:51 ` Hannes Reinecke
0 siblings, 1 reply; 12+ messages in thread
From: Mike Snitzer @ 2014-01-31 15:40 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: Jun'ichi Nomura, dm-devel, Alasdair Kergon
On Fri, Jan 31 2014 at 4:10am -0500,
Hannes Reinecke <hare@suse.de> wrote:
> There is no reason why multipath needs to queue requests
> internally for queue_if_no_path or pg_init; we should
> rather push them back onto the request queue.
>
> And while we're at it we can simplify the conditional
> statement in map_io() to make it easier to read.
>
> Cc: Mike Snitzer <snitzer@redhat.com>
> Cc: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
> drivers/md/dm-mpath.c | 115 ++++++++++++++----------------------------
> drivers/md/dm.c | 13 +++++
> include/linux/device-mapper.h | 1 +
> 3 files changed, 52 insertions(+), 77 deletions(-)
...
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 0704c52..291491b 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -1912,6 +1912,19 @@ static int dm_any_congested(void *congested_data, int bdi_bits)
> return r;
> }
>
> +void dm_table_run_queue(struct dm_table *t)
> +{
> + struct mapped_device *md = dm_table_get_md(t);
> + unsigned long flags;
> +
> + if (md->queue) {
> + spin_lock_irqsave(md->queue->queue_lock, flags);
> + blk_run_queue_async(md->queue);
> + spin_unlock_irqrestore(md->queue->queue_lock, flags);
> + }
> +}
> +EXPORT_SYMBOL_GPL(dm_table_run_queue);
> +
I think I agree with Junichi's previous point that this should live in
dm-table.c. You don't need the mapped_device, only the associated
request_queue, so... I know I implemented something like
dm_get_md_queue() before, but obviously it never went in for whatever
reason.
Anyway, I think something like this would be ideal (I renamed to
dm_table_run_md_queue_async). Please feel free to fold this in and
repost with v4 of your patchset (assuming you'll be fixing up patch 3
after you get Jun'ichi's feedback):
drivers/md/dm-table.c | 14 ++++++++++++++
drivers/md/dm.c | 5 +++++
drivers/md/dm.h | 1 +
3 files changed, 20 insertions(+), 0 deletions(-)
diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 6a7f2b8..72362ce 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1618,6 +1618,20 @@ struct mapped_device *dm_table_get_md(struct dm_table *t)
}
EXPORT_SYMBOL(dm_table_get_md);
+void dm_table_run_md_queue_async(struct dm_table *t)
+{
+ struct mapped_device *md = dm_table_get_md(t);
+ struct request_queue *queue = dm_get_md_queue(md);
+ unsigned long flags;
+
+ if (queue) {
+ spin_lock_irqsave(queue->queue_lock, flags);
+ blk_run_queue_async(queue);
+ spin_unlock_irqrestore(queue->queue_lock, flags);
+ }
+}
+EXPORT_SYMBOL(dm_table_run_md_queue_async);
+
static int device_discard_capable(struct dm_target *ti, struct dm_dev *dev,
sector_t start, sector_t len, void *data)
{
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index b49c762..10cae7f 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -475,6 +475,11 @@ sector_t dm_get_size(struct mapped_device *md)
return get_capacity(md->disk);
}
+struct request_queue *dm_get_md_queue(struct mapped_device *md)
+{
+ return md->queue;
+}
+
struct dm_stats *dm_get_stats(struct mapped_device *md)
{
return &md->stats;
diff --git a/drivers/md/dm.h b/drivers/md/dm.h
index c4569f0..1b2ca8a 100644
--- a/drivers/md/dm.h
+++ b/drivers/md/dm.h
@@ -189,6 +189,7 @@ int dm_lock_for_deletion(struct mapped_device *md, bool mark_deferred, bool only
int dm_cancel_deferred_remove(struct mapped_device *md);
int dm_request_based(struct mapped_device *md);
sector_t dm_get_size(struct mapped_device *md);
+struct request_queue *dm_get_md_queue(struct mapped_device *md);
struct dm_stats *dm_get_stats(struct mapped_device *md);
int dm_kobject_uevent(struct mapped_device *md, enum kobject_action action,
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/5] dm-multipath: push back requests instead of queueing
2014-01-31 15:40 ` Mike Snitzer
@ 2014-02-01 13:51 ` Hannes Reinecke
0 siblings, 0 replies; 12+ messages in thread
From: Hannes Reinecke @ 2014-02-01 13:51 UTC (permalink / raw)
To: Mike Snitzer; +Cc: Jun'ichi Nomura, dm-devel, Alasdair Kergon
On 01/31/2014 04:40 PM, Mike Snitzer wrote:
> On Fri, Jan 31 2014 at 4:10am -0500,
> Hannes Reinecke <hare@suse.de> wrote:
>
>> There is no reason why multipath needs to queue requests
>> internally for queue_if_no_path or pg_init; we should
>> rather push them back onto the request queue.
>>
>> And while we're at it we can simplify the conditional
>> statement in map_io() to make it easier to read.
>>
>> Cc: Mike Snitzer <snitzer@redhat.com>
>> Cc: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>> drivers/md/dm-mpath.c | 115 ++++++++++++++----------------------------
>> drivers/md/dm.c | 13 +++++
>> include/linux/device-mapper.h | 1 +
>> 3 files changed, 52 insertions(+), 77 deletions(-)
>
> ...
>
>> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
>> index 0704c52..291491b 100644
>> --- a/drivers/md/dm.c
>> +++ b/drivers/md/dm.c
>> @@ -1912,6 +1912,19 @@ static int dm_any_congested(void *congested_data, int bdi_bits)
>> return r;
>> }
>>
>> +void dm_table_run_queue(struct dm_table *t)
>> +{
>> + struct mapped_device *md = dm_table_get_md(t);
>> + unsigned long flags;
>> +
>> + if (md->queue) {
>> + spin_lock_irqsave(md->queue->queue_lock, flags);
>> + blk_run_queue_async(md->queue);
>> + spin_unlock_irqrestore(md->queue->queue_lock, flags);
>> + }
>> +}
>> +EXPORT_SYMBOL_GPL(dm_table_run_queue);
>> +
>
> I think I agree with Junichi's previous point that this should live in
> dm-table.c. You don't need the mapped_device, only the associated
> request_queue, so... I know I implemented something like
> dm_get_md_queue() before, but obviously it never went in for whatever
> reason.
>
> Anyway, I think something like this would be ideal (I renamed to
> dm_table_run_md_queue_async). Please feel free to fold this in and
> repost with v4 of your patchset (assuming you'll be fixing up patch 3
> after you get Jun'ichi's feedback):
>
As mentioned, I don't mind.
The layering conventions in the device-mapper code still confuses
me occasionally.
I'll be folding it in.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/5] dm-multipath: remove process_queued_ios()
2014-01-31 14:55 ` Hannes Reinecke
@ 2014-02-03 10:26 ` Junichi Nomura
2014-02-03 10:31 ` Hannes Reinecke
0 siblings, 1 reply; 12+ messages in thread
From: Junichi Nomura @ 2014-02-03 10:26 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: dm-devel, Mike Snitzer, Alasdair Kergon
On 01/31/14 23:55, Hannes Reinecke wrote:
> Problem here is that pg_init_done() is per path, so at face value
> SCSI_DH_RETRY is per path, too.
> So from that we should be retrying this path (and this path only).
> Hence it would be correct to call queue_delayed_work directly.
>
> However, typically any pg_init affects _every_ path in the multipath
> device (active paths become passive and vice versa).
> Which seems to be the intended usage, as we're checking for
> pg_init_in_progress prior to invoking queue_delayed_work().
>
> But _if_ we assume that, then we only need to send a _single_
> pg_init, as this will switch all paths. So again, a call to
> __pg_init_all_paths will not do the correct thing as it'll
> send activations to _every_ active path.
Sending activation for every paths was introduced by this:
commit e54f77ddda72781ec1c1696b21aabd6a30cbb7c6
Author: Chandra Seetharaman <sekharan@us.ibm.com>
Date: Mon Jun 22 10:12:12 2009 +0100
So if you make change in the logic, you have to check whether the
change does not break what the above solved.
> (And, in fact, we're trying really hard in scsi_dh_rdac
> and scsi_dh_alua to bunch together all the various pg_init
> requests precisely for the cited reason).
>
> So my inclination here would be to treat SCSI_DH_RETRY
> as _per path_, and retry only this specific path.
> IE removing the check to pg_init_in_progress and call
> queue_delayed_work() directly.
>
> IMHO this would impose the least restriction on
> the internal workings of the various device handlers.
>
> What do you think?
I don't have strong opinion either way. But if we do that, more code
has to be changed, e.g. the management of retry count.
Removing the unnecessary workqueue has already a benefit.
It would be nice to focus on that instead of folding in more changes.
--
Jun'ichi Nomura, NEC Corporation
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/5] dm-multipath: remove process_queued_ios()
2014-02-03 10:26 ` Junichi Nomura
@ 2014-02-03 10:31 ` Hannes Reinecke
0 siblings, 0 replies; 12+ messages in thread
From: Hannes Reinecke @ 2014-02-03 10:31 UTC (permalink / raw)
To: Junichi Nomura; +Cc: dm-devel, Mike Snitzer, Alasdair Kergon
On 02/03/2014 11:26 AM, Junichi Nomura wrote:
> On 01/31/14 23:55, Hannes Reinecke wrote:
>> Problem here is that pg_init_done() is per path, so at face value
>> SCSI_DH_RETRY is per path, too.
>> So from that we should be retrying this path (and this path only).
>> Hence it would be correct to call queue_delayed_work directly.
>>
>> However, typically any pg_init affects _every_ path in the multipath
>> device (active paths become passive and vice versa).
>> Which seems to be the intended usage, as we're checking for
>> pg_init_in_progress prior to invoking queue_delayed_work().
>>
>> But _if_ we assume that, then we only need to send a _single_
>> pg_init, as this will switch all paths. So again, a call to
>> __pg_init_all_paths will not do the correct thing as it'll
>> send activations to _every_ active path.
>
> Sending activation for every paths was introduced by this:
> commit e54f77ddda72781ec1c1696b21aabd6a30cbb7c6
> Author: Chandra Seetharaman <sekharan@us.ibm.com>
> Date: Mon Jun 22 10:12:12 2009 +0100
> So if you make change in the logic, you have to check whether the
> change does not break what the above solved.
>
>> (And, in fact, we're trying really hard in scsi_dh_rdac
>> and scsi_dh_alua to bunch together all the various pg_init
>> requests precisely for the cited reason).
>>
>> So my inclination here would be to treat SCSI_DH_RETRY
>> as _per path_, and retry only this specific path.
>> IE removing the check to pg_init_in_progress and call
>> queue_delayed_work() directly.
>>
>> IMHO this would impose the least restriction on
>> the internal workings of the various device handlers.
>>
>> What do you think?
>
> I don't have strong opinion either way. But if we do that, more code
> has to be changed, e.g. the management of retry count.
>
> Removing the unnecessary workqueue has already a benefit.
> It would be nice to focus on that instead of folding in more changes.
>
Yes, that's what I've figured, too.
So I've just included the changes you suggested without modifying
the current logic.
I've already sent a new patchset, please check if the changes there
are correct.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2014-02-03 10:31 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-01-31 9:10 [PATCHv3 0/5] dm-multipath: push back requests instead of queueing Hannes Reinecke
2014-01-31 9:10 ` [PATCH 1/5] dm-multipath: Do not call pg_init twice Hannes Reinecke
2014-01-31 9:10 ` [PATCH 2/5] dm-multipath: push back requests instead of queueing Hannes Reinecke
2014-01-31 15:40 ` Mike Snitzer
2014-02-01 13:51 ` Hannes Reinecke
2014-01-31 9:10 ` [PATCH 3/5] dm-multipath: remove process_queued_ios() Hannes Reinecke
2014-01-31 9:43 ` Junichi Nomura
2014-01-31 14:55 ` Hannes Reinecke
2014-02-03 10:26 ` Junichi Nomura
2014-02-03 10:31 ` Hannes Reinecke
2014-01-31 9:10 ` [PATCH 4/5] dm-multipath: reduce memory pressure during requeuing Hannes Reinecke
2014-01-31 9:10 ` [PATCH 5/5] dm-multipath: remove map_io() Hannes Reinecke
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.