[PATCH v6 0/7] dm-multipath: push back requests instead of queueing

[PATCH v6 0/7] dm-multipath: push back requests instead of queueing

[Mike Snitzer]

Hi Hannes,

I went over all of your patches and made a few tweaks, comments, and
improvements along the way.  Patch headers were modified very
slightly.

I wasn't seeing the need for (m->queue_io << 1) + m->queue_if_no_path
in v5's "[PATCH 3/6] dm-multipath: push back requests instead of
queueing" so I simplified that and documented the change in the
header.  If there is a reason for what you had please explain.

Patch 7 offers and incremental cleanup that builds on your v5
patchset.

I can rebase/modify any aspect of these patches so please just let me
know if there is something that needs changing.

Jun'ichi, I didn't add any tag for your contributions to this patchset
(either Acked-by or Reviewed-by) but I'd like you to reply to this
v6 patchset with those if you're OK with these changes.

Thanks,
Mike

Hannes Reinecke (5):
  dm mpath: do not call pg_init when it is already running
  dm mpath: push back requests instead of queueing
  dm mpath: remove process_queued_ios()
  dm mpath: reduce memory pressure when requeuing
  dm mpath: remove map_io()

Mike Snitzer (2):
  dm table: add dm_table_run_md_queue_async
  dm mpath: remove extra nesting in map function when path exists

 drivers/md/dm-mpath.c         | 208 ++++++++++++++----------------------------
 drivers/md/dm-table.c         |  19 ++++
 drivers/md/dm.c               |   5 +
 drivers/md/dm.h               |   1 +
 include/linux/device-mapper.h |   5 +
 5 files changed, 100 insertions(+), 138 deletions(-)

-- 
1.8.1.4

[PATCH 1/7] dm mpath: do not call pg_init when it is already running

[Mike Snitzer]

From: Hannes Reinecke <hare@suse.de>

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>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-mpath.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 6eb9dc9..304ee5c 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)
@@ -501,8 +504,7 @@ static void process_queued_ios(struct work_struct *work)
 	    (!pgpath && !m->queue_if_no_path))
 		must_queue = 0;
 
-	if (m->pg_init_required && !m->pg_init_in_progress && pgpath &&
-	    !m->pg_init_disabled)
+	if (pgpath && m->pg_init_required)
 		__pg_init_all_paths(m);
 
 	spin_unlock_irqrestore(&m->lock, flags);
-- 
1.8.1.4

[PATCH 2/7] dm table: add dm_table_run_md_queue_async

[Mike Snitzer]

Introduce dm_table_run_md_queue_async() to run the request_queue of the
mapped_device associated with a request-based DM table.

Also add dm_md_get_queue() wrapper to extract the request_queue from a
mapped_device.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/md/dm-table.c         | 19 +++++++++++++++++++
 drivers/md/dm.c               |  5 +++++
 drivers/md/dm.h               |  1 +
 include/linux/device-mapper.h |  5 +++++
 4 files changed, 30 insertions(+)

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 6a7f2b8..8df63ed 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1618,6 +1618,25 @@ 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;
+	struct request_queue *queue;
+	unsigned long flags;
+
+	if (!dm_table_request_based(t))
+		return;
+
+	md = dm_table_get_md(t);
+	queue = dm_get_md_queue(md);
+	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 8c53b09..341991f 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,
diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
index ed419c6..250225b 100644
--- a/include/linux/device-mapper.h
+++ b/include/linux/device-mapper.h
@@ -466,6 +466,11 @@ struct mapped_device *dm_table_get_md(struct dm_table *t);
 void dm_table_event(struct dm_table *t);
 
 /*
+ * Run the queue for request-based targets.
+ */
+void dm_table_run_md_queue_async(struct dm_table *t);
+
+/*
  * The device must be suspended before calling this method.
  * Returns the previous table, which the caller must destroy.
  */
-- 
1.8.1.4

[PATCH 3/7] dm mpath: push back requests instead of queueing

[Mike Snitzer]

From: Hannes Reinecke <hare@suse.de>

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.

Since mpath no longer does internal queuing of I/O the table info no
longer emits the internal queue_size.  Instead it displays 1 if queuing
is being used or 0 if it is not.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Cc: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-mpath.c | 116 +++++++++++++++++---------------------------------
 1 file changed, 38 insertions(+), 78 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 304ee5c..89c0997 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 =
@@ -509,7 +469,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_md_queue_async(m->ti->table);
 }
 
 /*
@@ -987,7 +947,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);
 
@@ -1056,7 +1016,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)) {
@@ -1435,7 +1395,7 @@ 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, m->pg_init_count);
 	else {
 		DMEMIT("%u ", m->queue_if_no_path +
 			      (m->pg_init_retries > 0) * 2 +
@@ -1683,7 +1643,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;
 	}
@@ -1736,7 +1696,7 @@ out:
  *---------------------------------------------------------------*/
 static struct target_type multipath_target = {
 	.name = "multipath",
-	.version = {1, 6, 0},
+	.version = {1, 7, 0},
 	.module = THIS_MODULE,
 	.ctr = multipath_ctr,
 	.dtr = multipath_dtr,
-- 
1.8.1.4

[PATCH 4/7] dm mpath: remove process_queued_ios()

[Mike Snitzer]

From: Hannes Reinecke <hare@suse.de>

process_queued_ios() has served 3 functions:
  1) select pg and pgpath if none is selected
  2) start pg_init if requested
  3) dispatch queued IOs when pg is ready

Basically, a call to queue_work(process_queued_ios) can be replaced by
dm_table_run_md_queue_async(), which runs request queue and ends up
calling map_io(), which does 1), 2) and 3).

Exception is when !pg_ready() (which means either pg_init is running or
requested), then multipath_busy() prevents map_io() being called from
request_fn.

If pg_init is running, it should be ok as long as pg_init_done() does
the right thing when pg_init is completed, I.e.: restart pg_init if
!pg_ready() or call dm_table_run_md_queue_async() to kick map_io().

If pg_init is requested, we have to make sure the request is detected
and pg_init will be started.  pg_init is requested in 3 places:
  a) __choose_pgpath() in map_io()
  b) __choose_pgpath() in multipath_ioctl()
  c) pg_init retry in pg_init_done()
a) is ok because map_io() calls __pg_init_all_paths(), which does 2).
b) needs a call to __pg_init_all_paths(), which does 2).
c) needs a call to __pg_init_all_paths(), which does 2).

So this patch removes process_queued_ios() and ensures that
__pg_init_all_paths() is called at the appropriate locations.

We only need to take care to add a small delay when calling
__pg_init_all_paths() to move processing off to a workqueue; otherwise
pg_init_done() might end up calling scsi_dh_activate() directly, which
might use non-atomic memory allocations or issue I/O.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Cc: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-mpath.c | 63 ++++++++++++++++++---------------------------------
 1 file changed, 22 insertions(+), 41 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 89c0997..b99cff0 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);
@@ -254,10 +250,10 @@ static void clear_mapinfo(struct multipath *m, union map_info *info)
  * Path selection
  *-----------------------------------------------*/
 
-static void __pg_init_all_paths(struct multipath *m)
+static void __pg_init_all_paths(struct multipath *m, unsigned long min_delay)
 {
 	struct pgpath *pgpath;
-	unsigned long pg_init_delay = 0;
+	unsigned long pg_init_delay = min_delay;
 
 	if (m->pg_init_in_progress || m->pg_init_disabled)
 		return;
@@ -406,7 +402,7 @@ static int map_io(struct multipath *m, struct request *clone,
 							      &pgpath->path,
 							      nr_bytes);
 		} else {
-			__pg_init_all_paths(m);
+			__pg_init_all_paths(m, 0);
 			r = DM_MAPIO_REQUEUE;
 		}
 	} else {
@@ -438,40 +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_md_queue_async(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 (pgpath && m->pg_init_required)
-		__pg_init_all_paths(m);
-
-	spin_unlock_irqrestore(&m->lock, flags);
-	if (!must_queue)
-		dm_table_run_md_queue_async(m->ti->table);
-}
-
 /*
  * An event is triggered whenever a path is taken out of use.
  * Includes path failure and PG bypass.
@@ -1018,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_md_queue_async(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++;
@@ -1216,9 +1185,12 @@ 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 (m->current_pg) {
+		m->pg_init_delay_retry = delay_retry;
+		/* Use a small delay to force the use of workqueue context */
+		__pg_init_all_paths(m, 50/HZ);
+		goto out;
+	}
 
 	/*
 	 * Wake up any thread waiting to suspend.
@@ -1591,8 +1563,17 @@ 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_pg) {
+			/* Path status changed, redo selection */
+			__choose_pgpath(m, 0);
+		}
+		if (m->current_pg && m->pg_init_required)
+			__pg_init_all_paths(m, 0);
+		spin_unlock_irqrestore(&m->lock, flags);
+		dm_table_run_md_queue_async(m->ti->table);
+	}
 
 	return r ? : __blkdev_driver_ioctl(bdev, mode, cmd, arg);
 }
-- 
1.8.1.4

[PATCH 5/7] dm mpath: reduce memory pressure when requeuing

[Mike Snitzer]

From: Hannes Reinecke <hare@suse.de>

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.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Cc: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-mpath.c | 43 ++++++++++++++++++-------------------------
 1 file changed, 18 insertions(+), 25 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index b99cff0..07a00a2 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,32 @@ 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)
+				/* ENOMEM, requeue */
+				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, 0);
-			r = DM_MAPIO_REQUEUE;
+			r = DM_MAPIO_REMAPPED;
+			goto out_unlock;
 		}
-	} else {
-		/* No path */
-		if (__must_push_back(m))
-			r = DM_MAPIO_REQUEUE;
-		else
-			r = -EIO;	/* Failed */
-	}
+		__pg_init_all_paths(m, 0);
+	} else if (!__must_push_back(m))
+		r = -EIO;	/* Failed */
 
+out_unlock:
 	spin_unlock_irqrestore(&m->lock, flags);
 
 	return r;
@@ -908,19 +911,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.8.1.4

[PATCH 6/7] dm mpath: remove map_io()

[Mike Snitzer]

From: Hannes Reinecke <hare@suse.de>

multipath_map() is now just a wrapper around map_io(), so we
can rename map_io() to multipath_map().

Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 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 07a00a2..971fbec 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;
@@ -906,17 +910,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.8.1.4

[PATCH 7/7] dm mpath: remove extra nesting in map function when path exists

[Mike Snitzer]

Return early for case when no path exists, this eliminates the need for
extra nesting for the case when a path exists (the common case).

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-mpath.c | 51 +++++++++++++++++++++++++++------------------------
 1 file changed, 27 insertions(+), 24 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 971fbec..272e7d4 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -392,32 +392,35 @@ static int multipath_map(struct dm_target *ti, struct request *clone,
 
 	pgpath = m->current_pgpath;
 
-	if (pgpath) {
-		if (__pgpath_busy(pgpath))
-			goto out_unlock;
+	if (!pgpath) {
+		if (!__must_push_back(m))
+			r = -EIO;	/* Failed */
+		goto out_unlock;
+	}
+
+	if (__pgpath_busy(pgpath))
+		goto out_unlock;
 
-		if (pg_ready(m)) {
-			if (set_mapinfo(m, map_context) < 0)
-				/* ENOMEM, requeue */
-				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);
-			r = DM_MAPIO_REMAPPED;
+	if (pg_ready(m)) {
+		if (set_mapinfo(m, map_context) < 0)
+			/* ENOMEM, requeue */
 			goto out_unlock;
-		}
-		__pg_init_all_paths(m, 0);
-	} else if (!__must_push_back(m))
-		r = -EIO;	/* Failed */
+
+		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);
+		r = DM_MAPIO_REMAPPED;
+		goto out_unlock;
+	}
+	__pg_init_all_paths(m, 0);
 
 out_unlock:
 	spin_unlock_irqrestore(&m->lock, flags);
-- 
1.8.1.4

Re: [PATCH 4/7] dm mpath: remove process_queued_ios()

[Junichi Nomura]

On 02/04/14 05:28, Mike Snitzer wrote:
> @@ -1216,9 +1185,12 @@ 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 (m->current_pg) {
> +		m->pg_init_delay_retry = delay_retry;
> +		/* Use a small delay to force the use of workqueue context */
> +		__pg_init_all_paths(m, 50/HZ);
> +		goto out;
> +	}

I think the patch is still broken.

When "m->pg_init_required && !m->current_pg",
it ends up with !pg_ready() and no pg_init running.
So map_io() will not be called and IO will stall.

What do you think about my suggestion here:
https://www.redhat.com/archives/dm-devel/2014-February/msg00013.html

Also it's not yet clear why the second parameter of __pg_init_all_paths()
is needed. (At least, "50/HZ" is typo or something.)

> @@ -1591,8 +1563,17 @@ static int multipath_ioctl(struct dm_target *ti, unsigned int cmd,
...
> +		if (m->current_pg && m->pg_init_required)
> +			__pg_init_all_paths(m, 0);
> +		spin_unlock_irqrestore(&m->lock, flags);
> +		dm_table_run_md_queue_async(m->ti->table);
> +	}

What happens if "!m->current_pg && m->pg_init_required"?

-- 
Jun'ichi Nomura, NEC Corporation

Re: [PATCH 3/7] dm mpath: push back requests instead of queueing

[Junichi Nomura]

On 02/04/14 05:28, Mike Snitzer wrote:
> From: Hannes Reinecke <hare@suse.de>
> 
> 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.
> 
> Since mpath no longer does internal queuing of I/O the table info no
> longer emits the internal queue_size.  Instead it displays 1 if queuing
> is being used or 0 if it is not.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Cc: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
> Signed-off-by: Mike Snitzer <snitzer@redhat.com>

> @@ -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;

Sorry I forgot to comment on this.
The above 2 lines should be removed from this patch.
I suggested to add them because the early version of this patch was
going to remove multipath_busy(). It's no longer true.

Removing them will make this patch easier to understand.

Other than that:

Reviewed-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>


> +		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);
>  

-- 
Jun'ichi Nomura, NEC Corporation

Re: [PATCH 1/7] dm mpath: do not call pg_init when it is already running

[Junichi Nomura]

On 02/04/14 05:28, Mike Snitzer wrote:
> From: Hannes Reinecke <hare@suse.de>
> 
> 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>
> Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> ---
>  drivers/md/dm-mpath.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> index 6eb9dc9..304ee5c 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)
> @@ -501,8 +504,7 @@ static void process_queued_ios(struct work_struct *work)
>  	    (!pgpath && !m->queue_if_no_path))
>  		must_queue = 0;
>  
> -	if (m->pg_init_required && !m->pg_init_in_progress && pgpath &&
> -	    !m->pg_init_disabled)
> +	if (pgpath && m->pg_init_required)
>  		__pg_init_all_paths(m);
>  
>  	spin_unlock_irqrestore(&m->lock, flags);
> 

I think the patch is ok but the description seems not
matching what it does.

This patch moves condition checks as a preparation of following
patches and has no effect on behaviour.
process_queued_ios() is the only caller of __pg_init_all_paths()
and 2 condition checks are moved from outside to inside without
side effects.

Reviewed-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>

-- 
Jun'ichi Nomura, NEC Corporation

Re: [PATCH 7/7] dm mpath: remove extra nesting in map function when path exists

[Junichi Nomura]

On 02/04/14 05:28, Mike Snitzer wrote:
> Return early for case when no path exists, this eliminates the need for
> extra nesting for the case when a path exists (the common case).
...
> -	if (pgpath) {
> -		if (__pgpath_busy(pgpath))
> -			goto out_unlock;
> +	if (!pgpath) {

This should be "unlikely(!pgpath)".

> +	if (__pgpath_busy(pgpath))
> +		goto out_unlock;

The above 2 lines should be removed.
(Actually it is a comment for PATCH 1/7 but just in case.)

Other than that:

Reviewed-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>

-- 
Jun'ichi Nomura, NEC Corporation

Re: [PATCH 2/7] dm table: add dm_table_run_md_queue_async

[Junichi Nomura]

On 02/04/14 05:28, Mike Snitzer wrote:
> Introduce dm_table_run_md_queue_async() to run the request_queue of the
> mapped_device associated with a request-based DM table.
> 
> Also add dm_md_get_queue() wrapper to extract the request_queue from a
> mapped_device.
> 
> Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> Signed-off-by: Hannes Reinecke <hare@suse.de>

Reviewed-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>

-- 
Jun'ichi Nomura, NEC Corporation

Re: [PATCH 5/7] dm mpath: reduce memory pressure when requeuing

[Junichi Nomura]

On 02/04/14 05:28, Mike Snitzer wrote:
> From: Hannes Reinecke <hare@suse.de>
> 
> 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.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Cc: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
> Signed-off-by: Mike Snitzer <snitzer@redhat.com>

Reviewed-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>

-- 
Jun'ichi Nomura, NEC Corporation

Re: [PATCH 6/7] dm mpath: remove map_io()

[Junichi Nomura]

On 02/04/14 05:28, Mike Snitzer wrote:
> From: Hannes Reinecke <hare@suse.de>
> 
> multipath_map() is now just a wrapper around map_io(), so we
> can rename map_io() to multipath_map().
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Mike Snitzer <snitzer@redhat.com>

Reviewed-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>

-- 
Jun'ichi Nomura, NEC Corporation

Re: [PATCH v6 0/7] dm-multipath: push back requests instead of queueing

[Hannes Reinecke]

Hi Mike,

On 02/03/2014 09:28 PM, Mike Snitzer wrote:
> Hi Hannes,
> 
> I went over all of your patches and made a few tweaks, comments, and
> improvements along the way.  Patch headers were modified very
> slightly.
> 
> I wasn't seeing the need for (m->queue_io << 1) + m->queue_if_no_path
> in v5's "[PATCH 3/6] dm-multipath: push back requests instead of
> queueing" so I simplified that and documented the change in the
> header.  If there is a reason for what you had please explain.
> 
'queue_io' is just used as a marker that I/O should be queued as
pg_init is running. The intention of the stacked status was so that
one could figure out by 'dmsetup status' whether I/O will be blocked
or not. And for that you'd need both, 'queue_io' and 'queue_if_no_path'.

But thanks for the rework.

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)

Re: [PATCH 4/7] dm mpath: remove process_queued_ios()

[Hannes Reinecke]

On 02/04/2014 04:24 AM, Junichi Nomura wrote:
> On 02/04/14 05:28, Mike Snitzer wrote:
>> @@ -1216,9 +1185,12 @@ 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 (m->current_pg) {
>> +		m->pg_init_delay_retry = delay_retry;
>> +		/* Use a small delay to force the use of workqueue context */
>> +		__pg_init_all_paths(m, 50/HZ);
>> +		goto out;
>> +	}
> 
> I think the patch is still broken.
> 
> When "m->pg_init_required && !m->current_pg",
> it ends up with !pg_ready() and no pg_init running.
> So map_io() will not be called and IO will stall.
> 
> What do you think about my suggestion here:
> https://www.redhat.com/archives/dm-devel/2014-February/msg00013.html
> 
Sigh. pg_init_required again...

> Also it's not yet clear why the second parameter of __pg_init_all_paths()
> is needed. (At least, "50/HZ" is typo or something.)
> 
>> @@ -1591,8 +1563,17 @@ static int multipath_ioctl(struct dm_target *ti, unsigned int cmd,
> ...
>> +		if (m->current_pg && m->pg_init_required)
>> +			__pg_init_all_paths(m, 0);
>> +		spin_unlock_irqrestore(&m->lock, flags);
>> +		dm_table_run_md_queue_async(m->ti->table);
>> +	}
> 
> What happens if "!m->current_pg && m->pg_init_required"?
> 
>From the current logic it means that no valid pg was found, so
calling pg_init would be pointless.
We're calling __choose_pgpath() before that, so if that returns
with current_pg == NULL all paths are down, and calling
pg_init would be pointless.

But I think I see to have pg_init_required handling cleared up;
I'll be doing a patch to unset it at the start of __choose_pgpath(),
this we we can be sure that it'll be set correctly.

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

Re: [PATCH 7/7] dm mpath: remove extra nesting in map function when path exists

[Hannes Reinecke]

On 02/03/2014 09:28 PM, Mike Snitzer wrote:
> Return early for case when no path exists, this eliminates the need for
> extra nesting for the case when a path exists (the common case).
> 
> Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> ---
>  drivers/md/dm-mpath.c | 51 +++++++++++++++++++++++++++------------------------
>  1 file changed, 27 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> index 971fbec..272e7d4 100644
> --- a/drivers/md/dm-mpath.c
> +++ b/drivers/md/dm-mpath.c
> @@ -392,32 +392,35 @@ static int multipath_map(struct dm_target *ti, struct request *clone,
>  
>  	pgpath = m->current_pgpath;
>  
> -	if (pgpath) {
> -		if (__pgpath_busy(pgpath))
> -			goto out_unlock;
> +	if (!pgpath) {
> +		if (!__must_push_back(m))
> +			r = -EIO;	/* Failed */
> +		goto out_unlock;
> +	}
> +
> +	if (__pgpath_busy(pgpath))
> +		goto out_unlock;
>  
> -		if (pg_ready(m)) {
> -			if (set_mapinfo(m, map_context) < 0)
> -				/* ENOMEM, requeue */
> -				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);
> -			r = DM_MAPIO_REMAPPED;
> +	if (pg_ready(m)) {
> +		if (set_mapinfo(m, map_context) < 0)
> +			/* ENOMEM, requeue */
>  			goto out_unlock;
> -		}
> -		__pg_init_all_paths(m, 0);
> -	} else if (!__must_push_back(m))
> -		r = -EIO;	/* Failed */
> +
> +		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);
> +		r = DM_MAPIO_REMAPPED;
> +		goto out_unlock;
> +	}
> +	__pg_init_all_paths(m, 0);
>  
>  out_unlock:
>  	spin_unlock_irqrestore(&m->lock, flags);
> 
But then you should be going full circle and move the now somewhat
lonely statement '__pg_init_all_paths' to the top, inverting the
if (pg_ready(m))
statement.

I'll be redoing the patchset (yet again)

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)

Re: [PATCH 4/7] dm mpath: remove process_queued_ios()

[Junichi Nomura]

On 02/04/14 17:18, Hannes Reinecke wrote:
>>> @@ -1591,8 +1563,17 @@ static int multipath_ioctl(struct dm_target *ti, unsigned int cmd,
>> ...
>>> +		if (m->current_pg && m->pg_init_required)
>>> +			__pg_init_all_paths(m, 0);
>>> +		spin_unlock_irqrestore(&m->lock, flags);
>>> +		dm_table_run_md_queue_async(m->ti->table);
>>> +	}
>>
>> What happens if "!m->current_pg && m->pg_init_required"?
>>
>>From the current logic it means that no valid pg was found, so
> calling pg_init would be pointless.
> We're calling __choose_pgpath() before that, so if that returns
> with current_pg == NULL all paths are down, and calling
> pg_init would be pointless.
> 
> But I think I see to have pg_init_required handling cleared up;
> I'll be doing a patch to unset it at the start of __choose_pgpath(),
> this we we can be sure that it'll be set correctly.

I think it is possible that __choose_pgpath() being called twice
before pg_init_required is checked.

For example,

  multipath_ioctl()
    __choose_pgpath()
      clear pg_init_required
      select a new pg
      __switch_pg()
        set pg_init_required

  map_io()
    __choose_pgpath()
      clear pg_init_required
      select the same pg
      (pg_init_required is not set)
      ...

In this case, pg_init should be submitted to the pg but not.

-- 
Jun'ichi Nomura, NEC Corporation

Re: [PATCH 4/7] dm mpath: remove process_queued_ios()

[Hannes Reinecke]

On 02/04/2014 09:55 AM, Junichi Nomura wrote:
> On 02/04/14 17:18, Hannes Reinecke wrote:
>>>> @@ -1591,8 +1563,17 @@ static int multipath_ioctl(struct dm_target *ti, unsigned int cmd,
>>> ...
>>>> +		if (m->current_pg && m->pg_init_required)
>>>> +			__pg_init_all_paths(m, 0);
>>>> +		spin_unlock_irqrestore(&m->lock, flags);
>>>> +		dm_table_run_md_queue_async(m->ti->table);
>>>> +	}
>>>
>>> What happens if "!m->current_pg && m->pg_init_required"?
>>>
>> >From the current logic it means that no valid pg was found, so
>> calling pg_init would be pointless.
>> We're calling __choose_pgpath() before that, so if that returns
>> with current_pg == NULL all paths are down, and calling
>> pg_init would be pointless.
>>
>> But I think I see to have pg_init_required handling cleared up;
>> I'll be doing a patch to unset it at the start of __choose_pgpath(),
>> this we we can be sure that it'll be set correctly.
> 
> I think it is possible that __choose_pgpath() being called twice
> before pg_init_required is checked.
> 
> For example,
> 
>   multipath_ioctl()
>     __choose_pgpath()
>       clear pg_init_required
>       select a new pg
>       __switch_pg()
>         set pg_init_required
> 
>   map_io()
>     __choose_pgpath()
>       clear pg_init_required
>       select the same pg
>       (pg_init_required is not set)
>       ...
> 
But why should 'map_io' calling __choose_pgpath()?

Either __choose_path() from ioctl was able to set ->current_pg
(in which case __choose_pgpath() wouldn't be called in map_io)
or it was not, in which case pg_init_required would need to be reset
during __choose_pgpath() when called from map_io().

Am I missing something?

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

Re: [PATCH 4/7] dm mpath: remove process_queued_ios()

[Junichi Nomura]

On 02/04/14 18:08, Hannes Reinecke wrote:
> On 02/04/2014 09:55 AM, Junichi Nomura wrote:
>> On 02/04/14 17:18, Hannes Reinecke wrote:
>>>>> @@ -1591,8 +1563,17 @@ static int multipath_ioctl(struct dm_target *ti, unsigned int cmd,
>>>> ...
>>>>> +		if (m->current_pg && m->pg_init_required)
>>>>> +			__pg_init_all_paths(m, 0);
>>>>> +		spin_unlock_irqrestore(&m->lock, flags);
>>>>> +		dm_table_run_md_queue_async(m->ti->table);
>>>>> +	}
>>>>
>>>> What happens if "!m->current_pg && m->pg_init_required"?
>>>>
>>> >From the current logic it means that no valid pg was found, so
>>> calling pg_init would be pointless.
>>> We're calling __choose_pgpath() before that, so if that returns
>>> with current_pg == NULL all paths are down, and calling
>>> pg_init would be pointless.
>>>
>>> But I think I see to have pg_init_required handling cleared up;
>>> I'll be doing a patch to unset it at the start of __choose_pgpath(),
>>> this we we can be sure that it'll be set correctly.
>>
>> I think it is possible that __choose_pgpath() being called twice
>> before pg_init_required is checked.
>>
>> For example,
>>
>>   multipath_ioctl()
>>     __choose_pgpath()
>>       clear pg_init_required
>>       select a new pg
>>       __switch_pg()
>>         set pg_init_required
>>
>>   map_io()
>>     __choose_pgpath()
>>       clear pg_init_required
>>       select the same pg
>>       (pg_init_required is not set)
>>       ...
>>
> But why should 'map_io' calling __choose_pgpath()?
> 
> Either __choose_path() from ioctl was able to set ->current_pg
> (in which case __choose_pgpath() wouldn't be called in map_io)
> or it was not, in which case pg_init_required would need to be reset
> during __choose_pgpath() when called from map_io().

__choose_pgpath() is a function to select "path".
Even if current_pg is set, map_io() may call the function
to select a path. (Please look at the repeat_count check)

So, I suggested the followings:
  Call __pg_init_all_paths() regardless of current_pg.
  __pg_init_all_paths() returns whether pg_init has started.
  If !current_pg, it returns 0.
  (https://www.redhat.com/archives/dm-devel/2014-February/msg00013.html)

The semantics is clear:
  - if pg_init_required, call __pg_init_all_paths()
  - __pg_init_all_paths() might fail to start pg_init (= returns 0).
    Then caller should take some action or give up.

-- 
Jun'ichi Nomura, NEC Corporation

Re: [PATCH 4/7] dm mpath: remove process_queued_ios()

[Hannes Reinecke]

On 02/04/2014 10:27 AM, Junichi Nomura wrote:
> On 02/04/14 18:08, Hannes Reinecke wrote:
>> On 02/04/2014 09:55 AM, Junichi Nomura wrote:
>>> On 02/04/14 17:18, Hannes Reinecke wrote:
>>>>>> @@ -1591,8 +1563,17 @@ static int multipath_ioctl(struct dm_target *ti, unsigned int cmd,
>>>>> ...
>>>>>> +		if (m->current_pg && m->pg_init_required)
>>>>>> +			__pg_init_all_paths(m, 0);
>>>>>> +		spin_unlock_irqrestore(&m->lock, flags);
>>>>>> +		dm_table_run_md_queue_async(m->ti->table);
>>>>>> +	}
>>>>>
>>>>> What happens if "!m->current_pg && m->pg_init_required"?
>>>>>
>>>> >From the current logic it means that no valid pg was found, so
>>>> calling pg_init would be pointless.
>>>> We're calling __choose_pgpath() before that, so if that returns
>>>> with current_pg == NULL all paths are down, and calling
>>>> pg_init would be pointless.
>>>>
>>>> But I think I see to have pg_init_required handling cleared up;
>>>> I'll be doing a patch to unset it at the start of __choose_pgpath(),
>>>> this we we can be sure that it'll be set correctly.
>>>
>>> I think it is possible that __choose_pgpath() being called twice
>>> before pg_init_required is checked.
>>>
>>> For example,
>>>
>>>   multipath_ioctl()
>>>     __choose_pgpath()
>>>       clear pg_init_required
>>>       select a new pg
>>>       __switch_pg()
>>>         set pg_init_required
>>>
>>>   map_io()
>>>     __choose_pgpath()
>>>       clear pg_init_required
>>>       select the same pg
>>>       (pg_init_required is not set)
>>>       ...
>>>
>> But why should 'map_io' calling __choose_pgpath()?
>>
>> Either __choose_path() from ioctl was able to set ->current_pg
>> (in which case __choose_pgpath() wouldn't be called in map_io)
>> or it was not, in which case pg_init_required would need to be reset
>> during __choose_pgpath() when called from map_io().
> 
> __choose_pgpath() is a function to select "path".
> Even if current_pg is set, map_io() may call the function
> to select a path. (Please look at the repeat_count check)
> 
... But only if 'queue_io' is not set, ie no pg_init is running.
At which point 'pg_init_required' should be set to '0' anyway.

What I'm arguing here is an inconsistency in __choose_pg():
__choose_pg() might or might not set 'current_pg', but if
'current_pg' is not set the status of 'pg_init_required'
is undefined upon return.

This makes it (in my view) unnecessarily complicated to
check if we need to initialize the paths, as we have to
check for both, current_pg _and_ pg_init_required.
If it were _that_ easy we wouldn't have this discussion :-)

> So, I suggested the followings:
>   Call __pg_init_all_paths() regardless of current_pg.
>   __pg_init_all_paths() returns whether pg_init has started.
>   If !current_pg, it returns 0.
>   (https://www.redhat.com/archives/dm-devel/2014-February/msg00013.html)
> 
> The semantics is clear:
>   - if pg_init_required, call __pg_init_all_paths()
>   - __pg_init_all_paths() might fail to start pg_init (= returns 0).
>     Then caller should take some action or give up.
> 
All right, will be modifying the patch.

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