[PATCHv9 0/8] dm-multipath: push back requests instead of queueing

[PATCHv9 0/8] dm-multipath: push back requests instead of queueing

[Hannes Reinecke]

Hi Mike,

here's version 9 of the patchset which might just be the
final one. I've included the remaining issue Jun'ichi pointed
out, and added the respective 'reviewed-by' tags where missing.

Hannes Reinecke (6):
  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()
  dm-mpath: do not activate failed paths

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

 drivers/md/dm-mpath.c         | 213 +++++++++++++++---------------------------
 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, 105 insertions(+), 138 deletions(-)

-- 
1.7.12.4

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

[Hannes Reinecke]

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.

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>
---
 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 e8431fb..bb2f6b2 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.7.12.4

[PATCH 2/8] dm table: add dm_table_run_md_queue_async

[Hannes Reinecke]

From: Mike Snitzer <snitzer@redhat.com>

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>
---
 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.7.12.4

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

[Hannes Reinecke]

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>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
Reviewed-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
---
 drivers/md/dm-mpath.c | 114 ++++++++++++++++----------------------------------
 1 file changed, 36 insertions(+), 78 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index bb2f6b2..1c6ca5c 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,28 @@ 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 (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 +435,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 +443,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 +467,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 +945,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 +1014,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 +1393,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 +
@@ -1688,7 +1646,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;
 	}
@@ -1741,7 +1699,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.7.12.4

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

[Hannes Reinecke]

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.

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>
---
 drivers/md/dm-mpath.c | 67 +++++++++++++++++++++------------------------------
 1 file changed, 27 insertions(+), 40 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 1c6ca5c..a26980b 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,16 +250,21 @@ static void clear_mapinfo(struct multipath *m, union map_info *info)
  * Path selection
  *-----------------------------------------------*/
 
-static void __pg_init_all_paths(struct multipath *m)
+static int __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;
+		return 0;
 
 	m->pg_init_count++;
 	m->pg_init_required = 0;
+
+	/* Check here to reset pg_init_required */
+	if (!m->current_pg)
+		return 0;
+
 	if (m->pg_init_delay_retry)
 		pg_init_delay = msecs_to_jiffies(m->pg_init_delay_msecs != DM_PG_INIT_DELAY_DEFAULT ?
 						 m->pg_init_delay_msecs : DM_PG_INIT_DELAY_MSECS);
@@ -275,6 +276,7 @@ static void __pg_init_all_paths(struct multipath *m)
 				       pg_init_delay))
 			m->pg_init_in_progress++;
 	}
+	return m->pg_init_in_progress;
 }
 
 static void __switch_pg(struct multipath *m, struct pgpath *pgpath)
@@ -436,40 +438,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.
@@ -1016,7 +991,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++;
@@ -1214,9 +1189,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;
+		if (__pg_init_all_paths(m))
+			goto out;
+		m->queue_io = 0;
+	}
 
 	/*
 	 * Wake up any thread waiting to suspend.
@@ -1594,8 +1572,17 @@ static int multipath_ioctl(struct dm_target *ti, unsigned int cmd,
 			r = err;
 	}
 
-	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->pg_init_required)
+			__pg_init_all_paths(m);
+		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.7.12.4

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

[Hannes Reinecke]

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>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
Reviewed-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
---
 drivers/md/dm-mpath.c | 38 +++++++++++++++-----------------------
 1 file changed, 15 insertions(+), 23 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index a26980b..13a855b 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -378,12 +378,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);
 
@@ -396,27 +396,29 @@ static int map_io(struct multipath *m, struct request *clone,
 
 	if (pgpath) {
 		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);
-			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);
+	} else if (!__must_push_back(m))
+		r = -EIO;	/* Failed */
 
+out_unlock:
 	spin_unlock_irqrestore(&m->lock, flags);
 
 	return r;
@@ -912,19 +914,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

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

[Hannes Reinecke]

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>
---
 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 13a855b..5e92d51 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -375,9 +375,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;
@@ -909,17 +913,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

[PATCH 7/8] dm mpath: remove extra nesting in map function

[Hannes Reinecke]

From: Mike Snitzer <snitzer@redhat.com>

Return early for case when no path exists, and when the
pathgroup isn't ready. This eliminates the need for
extra nesting for the the common case.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/md/dm-mpath.c | 46 ++++++++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 5e92d51..eaf4855 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -398,29 +398,31 @@ static int multipath_map(struct dm_target *ti, struct request *clone,
 
 	pgpath = m->current_pgpath;
 
-	if (pgpath) {
-		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;
-			goto out_unlock;
-		}
+	if (!pgpath) {
+		if (!__must_push_back(m))
+			r = -EIO;	/* Failed */
+		goto out_unlock;
+	}
+	if (!pg_ready(m)) {
 		__pg_init_all_paths(m);
-	} else if (!__must_push_back(m))
-		r = -EIO;	/* Failed */
+		goto out_unlock;
+	}
+	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;
 
 out_unlock:
 	spin_unlock_irqrestore(&m->lock, flags);
-- 
1.7.12.4

[PATCH 8/8] dm-mpath: do not activate failed paths

[Hannes Reinecke]

activate_path() is run without a lock, so the path might be
set to failed before activate_path() had a chance to run.
This patch add a check for ->active in activate_path() to
avoid unnecessary overhead by calling functions which are known
to be failing.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
---
 drivers/md/dm-mpath.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index eaf4855..e750095 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -1197,8 +1197,11 @@ static void activate_path(struct work_struct *work)
 	struct pgpath *pgpath =
 		container_of(work, struct pgpath, activate_path.work);
 
-	scsi_dh_activate(bdev_get_queue(pgpath->path.dev->bdev),
-				pg_init_done, pgpath);
+	if (pgpath->is_active)
+		scsi_dh_activate(bdev_get_queue(pgpath->path.dev->bdev),
+				 pg_init_done, pgpath);
+	else
+		pg_init_done(pgpath, SCSI_DH_DEV_OFFLINED);
 }
 
 static int noretry_error(int error)
-- 
1.7.12.4

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

[Junichi Nomura]

On 02/28/14 23:33, Hannes Reinecke wrote:
> @@ -1214,9 +1189,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;
> +		if (__pg_init_all_paths(m))
> +			goto out;
> +		m->queue_io = 0;
> +	}

Hi Hannes,

If m->pg_init_required && !m->current_pg,
doesn't m->queue_io remain 1 while nobody kicks pg_init?

That's why I suggested this:

	if (m->pg_init_required) {
		m->pg_init_delay_retry = delay_retry;
		if (__pg_init_all_paths(m))
			goto out;
	}
	m->queue_io = 0;

-- 
Jun'ichi Nomura, NEC Corporation

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

[Hannes Reinecke]

On 03/03/2014 12:12 AM, Junichi Nomura wrote:
> On 02/28/14 23:33, Hannes Reinecke wrote:
>> @@ -1214,9 +1189,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;
>> +		if (__pg_init_all_paths(m))
>> +			goto out;
>> +		m->queue_io = 0;
>> +	}
> 
> Hi Hannes,
> 
> If m->pg_init_required && !m->current_pg,
> doesn't m->queue_io remain 1 while nobody kicks pg_init?
> 
> That's why I suggested this:
> 
> 	if (m->pg_init_required) {
> 		m->pg_init_delay_retry = delay_retry;
> 		if (__pg_init_all_paths(m))
> 			goto out;
> 	}
> 	m->queue_io = 0;
> 
Yeah, sorry. You are right.

Mike, can you fix it up or do you need an additional round of patches?

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/8] dm mpath: remove process_queued_ios()

[Mike Snitzer]

On Wed, Mar 05 2014 at  3:59am -0500,
Hannes Reinecke <hare@suse.de> wrote:

> On 03/03/2014 12:12 AM, Junichi Nomura wrote:
> > On 02/28/14 23:33, Hannes Reinecke wrote:
> >> @@ -1214,9 +1189,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;
> >> +		if (__pg_init_all_paths(m))
> >> +			goto out;
> >> +		m->queue_io = 0;
> >> +	}
> > 
> > Hi Hannes,
> > 
> > If m->pg_init_required && !m->current_pg,
> > doesn't m->queue_io remain 1 while nobody kicks pg_init?
> > 
> > That's why I suggested this:
> > 
> > 	if (m->pg_init_required) {
> > 		m->pg_init_delay_retry = delay_retry;
> > 		if (__pg_init_all_paths(m))
> > 			goto out;
> > 	}
> > 	m->queue_io = 0;
> > 
> Yeah, sorry. You are right.
> 
> Mike, can you fix it up or do you need an additional round of patches?

I'll take care of it.  I won't be looking at these patches until next
week though.

Mike

Re: [PATCHv9 0/8] dm-multipath: push back requests instead of queueing

[Mike Snitzer]

On Fri, Feb 28 2014 at  9:33am -0500,
Hannes Reinecke <hare@suse.de> wrote:

> Hi Mike,
> 
> here's version 9 of the patchset which might just be the
> final one. I've included the remaining issue Jun'ichi pointed
> out, and added the respective 'reviewed-by' tags where missing.

I've picked these patches up.  They are now queued for 3.15 and in
linux-dm.git's 'for-next' branch.

I'll likely rebase linux-dm.git to 3.14 once it is released but aside
from commit id churn there won't be any other changes to this series
(unless linux-next turns something up).

Thanks for sorting this area of mpath out and sticking with this
patchset for so many review iterations.

Mike

Re: [PATCHv9 0/8] dm-multipath: push back requests instead of queueing

[Hannes Reinecke]

On 03/12/2014 10:46 PM, Mike Snitzer wrote:
> On Fri, Feb 28 2014 at  9:33am -0500,
> Hannes Reinecke <hare@suse.de> wrote:
> 
>> Hi Mike,
>>
>> here's version 9 of the patchset which might just be the
>> final one. I've included the remaining issue Jun'ichi pointed
>> out, and added the respective 'reviewed-by' tags where missing.
> 
> I've picked these patches up.  They are now queued for 3.15 and in
> linux-dm.git's 'for-next' branch.
> 
> I'll likely rebase linux-dm.git to 3.14 once it is released but aside
> from commit id churn there won't be any other changes to this series
> (unless linux-next turns something up).
> 
> Thanks for sorting this area of mpath out and sticking with this
> patchset for so many review iterations.
> 
Yes! Finally!

Thanks for including it.

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 8/8] dm-mpath: do not activate failed paths

[Hannes Reinecke]

On 02/28/2014 04:02 PM, Mike Snitzer wrote:
> On Fri, Feb 28 2014 at  9:44am -0500,
> Hannes Reinecke <hare@suse.de> wrote:
> 
>> On 02/28/2014 03:22 PM, Mike Snitzer wrote:
>> [ .. ]
>>>
>>> FYI, I still intend to review/take your "accept failed paths" patch.
>>> Would be helpful if any rebase is needed for that patch that you do so
>>> and re-post.
>>>
>>> One thing I noticed is you're only converting MAJ:MIN paths to devices.
>>> I think we should factor a helper out of DM core that does the full path
>>> lookup check from dm_get_device() -- rather than you open coding an
>>> older version of the MAJ:MIN device path processing.
>>>
>>> But is there a reason for not using lookup_bdev()?  Because the device
>>> is failed it cannot be found using lookup_bdev()?
>>>
>> Yes, that's precisely it.
>> When setting dev_loss_tmo very aggressively (to, say, 10 or even 5
>> seconds) it might well be that multipathd won't be able to keep up
>> with the events in time.
>> So by the time multipathd tries to push a new table into the kernel
>> (which contains major:minor numbers only) those devices are already
>> gone. So lookup_bdev() won't be able to find them, either.
> 
> Been talking this over with Alasdair.  Need some things clarified.
> 
> Are you needing to handle non-existent devices during initial mpath
> table load?
> 
> Or is the failed path in question already part of the active mpath table
> -- so active table currently holds a reference to the device?
> 
> Or do you need to support both cases?  Sounds like you want to support
> both cases..
> 
Hmm. I wouldn't think I'd need it during initial mpath table load;
but then you never now, devices might fail faster than you'd think ...

So yeah, I'd need to support both cases.

The main reason why I've sent this patch is to get rid of these
_really_ annoying messages 'error getting device' during failover.
We really should and can do better than that.

That this patch allows to drop references to failed devices earlier
is a nice side effect :-)

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 8/8] dm-mpath: do not activate failed paths

[Mike Snitzer]

On Fri, Feb 28 2014 at  9:44am -0500,
Hannes Reinecke <hare@suse.de> wrote:

> On 02/28/2014 03:22 PM, Mike Snitzer wrote:
> [ .. ]
> > 
> > FYI, I still intend to review/take your "accept failed paths" patch.
> > Would be helpful if any rebase is needed for that patch that you do so
> > and re-post.
> > 
> > One thing I noticed is you're only converting MAJ:MIN paths to devices.
> > I think we should factor a helper out of DM core that does the full path
> > lookup check from dm_get_device() -- rather than you open coding an
> > older version of the MAJ:MIN device path processing.
> > 
> > But is there a reason for not using lookup_bdev()?  Because the device
> > is failed it cannot be found using lookup_bdev()?
> > 
> Yes, that's precisely it.
> When setting dev_loss_tmo very aggressively (to, say, 10 or even 5
> seconds) it might well be that multipathd won't be able to keep up
> with the events in time.
> So by the time multipathd tries to push a new table into the kernel
> (which contains major:minor numbers only) those devices are already
> gone. So lookup_bdev() won't be able to find them, either.

Been talking this over with Alasdair.  Need some things clarified.

Are you needing to handle non-existent devices during initial mpath
table load?

Or is the failed path in question already part of the active mpath table
-- so active table currently holds a reference to the device?

Or do you need to support both cases?  Sounds like you want to support
both cases..

> Not sure if factoring things out from dm_get_device() would help
> here ...
> 
> But I'll be sending an updated patch, which'll apply on top of the
> 'push back' patchset.

Thanks.

Re: [PATCH 8/8] dm-mpath: do not activate failed paths

[Hannes Reinecke]

On 02/28/2014 03:22 PM, Mike Snitzer wrote:
[ .. ]
> 
> FYI, I still intend to review/take your "accept failed paths" patch.
> Would be helpful if any rebase is needed for that patch that you do so
> and re-post.
> 
> One thing I noticed is you're only converting MAJ:MIN paths to devices.
> I think we should factor a helper out of DM core that does the full path
> lookup check from dm_get_device() -- rather than you open coding an
> older version of the MAJ:MIN device path processing.
> 
> But is there a reason for not using lookup_bdev()?  Because the device
> is failed it cannot be found using lookup_bdev()?
> 
Yes, that's precisely it.
When setting dev_loss_tmo very aggressively (to, say, 10 or even 5
seconds) it might well be that multipathd won't be able to keep up
with the events in time.
So by the time multipathd tries to push a new table into the kernel
(which contains major:minor numbers only) those devices are already
gone. So lookup_bdev() won't be able to find them, either.

Not sure if factoring things out from dm_get_device() would help
here ...

But I'll be sending an updated patch, which'll apply on top of the
'push back' patchset.

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 8/8] dm-mpath: do not activate failed paths

[Mike Snitzer]

On Fri, Feb 28 2014 at  4:52am -0500,
Hannes Reinecke <hare@suse.de> wrote:

> On 02/28/2014 10:37 AM, Junichi Nomura wrote:
> > On 02/28/14 18:32, Hannes Reinecke wrote:
> >> On 02/28/2014 09:49 AM, Junichi Nomura wrote:
> >>> On 02/27/14 16:30, Hannes Reinecke wrote:
> >>>> activate_path() is run without a lock, so the path might be
> >>>> set to failed before activate_path() had a chance to run.
> >>>>
> >>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
> >>>> ---
> >>>>  drivers/md/dm-mpath.c | 7 +++++--
> >>>>  1 file changed, 5 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> >>>> index 0a40fa9..22e7365 100644
> >>>> --- a/drivers/md/dm-mpath.c
> >>>> +++ b/drivers/md/dm-mpath.c
> >>>> @@ -1196,8 +1196,11 @@ static void activate_path(struct work_struct *work)
> >>>>  	struct pgpath *pgpath =
> >>>>  		container_of(work, struct pgpath, activate_path.work);
> >>>>  
> >>>> -	scsi_dh_activate(bdev_get_queue(pgpath->path.dev->bdev),
> >>>> -				pg_init_done, pgpath);
> >>>> +	if (pgpath->is_active)
> >>>> +		scsi_dh_activate(bdev_get_queue(pgpath->path.dev->bdev),
> >>>> +				 pg_init_done, pgpath);
> >>>> +	else
> >>>> +		pg_init_done(pgpath, SCSI_DH_DEV_OFFLINED);
> >>>
> >>> Hi Hannes,
> >>>
> >>> What problem are you going to fix with this?
> >>> It is still possible that the path is set to failed just after
> >>> "if (pgpath->is_active)" and before "scsi_dh_activate".
> >>>
> >> activate_path() is run from a workqueue, so there is a race window
> >> between the check ->is_active in __pg_init_all_paths() and the time
> >> the scsi_dh_activate() is executed. And as activate_path)() is run
> >> without any locks we don't have any synchronization with fail_path().
> >> So it's well possible that a path gets failed in between
> >> __pg_init_all_paths() and activate_paths().
> > 
> > What I pointed is that your patch makes the window very small
> > but doesn't close it.
> > If the race is a problem, this patch doesn't fix the problem.
> > 
> True. As said I just want to avoid unnecessary overhead by calling
> functions which are known to be failing.
> This patch actually makes more sense in combination with the 'accept
> failed paths' patch, where it's much more likely to trigger.

FYI, I still intend to review/take your "accept failed paths" patch.
Would be helpful if any rebase is needed for that patch that you do so
and re-post.

One thing I noticed is you're only converting MAJ:MIN paths to devices.
I think we should factor a helper out of DM core that does the full path
lookup check from dm_get_device() -- rather than you open coding an
older version of the MAJ:MIN device path processing.

But is there a reason for not using lookup_bdev()?  Because the device
is failed it cannot be found using lookup_bdev()?

Re: [PATCH 8/8] dm-mpath: do not activate failed paths

[Junichi Nomura]

On 02/28/14 18:52, Hannes Reinecke wrote:
> On 02/28/2014 10:37 AM, Junichi Nomura wrote:
>> On 02/28/14 18:32, Hannes Reinecke wrote:
>>> On 02/28/2014 09:49 AM, Junichi Nomura wrote:
>>>> On 02/27/14 16:30, Hannes Reinecke wrote:
>>>>> activate_path() is run without a lock, so the path might be
>>>>> set to failed before activate_path() had a chance to run.
>>>>>
>>>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>>>> ---
>>>>>  drivers/md/dm-mpath.c | 7 +++++--
>>>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
>>>>> index 0a40fa9..22e7365 100644
>>>>> --- a/drivers/md/dm-mpath.c
>>>>> +++ b/drivers/md/dm-mpath.c
>>>>> @@ -1196,8 +1196,11 @@ static void activate_path(struct work_struct *work)
>>>>>  	struct pgpath *pgpath =
>>>>>  		container_of(work, struct pgpath, activate_path.work);
>>>>>  
>>>>> -	scsi_dh_activate(bdev_get_queue(pgpath->path.dev->bdev),
>>>>> -				pg_init_done, pgpath);
>>>>> +	if (pgpath->is_active)
>>>>> +		scsi_dh_activate(bdev_get_queue(pgpath->path.dev->bdev),
>>>>> +				 pg_init_done, pgpath);
>>>>> +	else
>>>>> +		pg_init_done(pgpath, SCSI_DH_DEV_OFFLINED);
>>>>
>>>> Hi Hannes,
>>>>
>>>> What problem are you going to fix with this?
>>>> It is still possible that the path is set to failed just after
>>>> "if (pgpath->is_active)" and before "scsi_dh_activate".
>>>>
>>> activate_path() is run from a workqueue, so there is a race window
>>> between the check ->is_active in __pg_init_all_paths() and the time
>>> the scsi_dh_activate() is executed. And as activate_path)() is run
>>> without any locks we don't have any synchronization with fail_path().
>>> So it's well possible that a path gets failed in between
>>> __pg_init_all_paths() and activate_paths().
>>
>> What I pointed is that your patch makes the window very small
>> but doesn't close it.
>> If the race is a problem, this patch doesn't fix the problem.
>>
> True. As said I just want to avoid unnecessary overhead by calling
> functions which are known to be failing.

"to avoid unnecessary overhead by calling functions which are known
 to be failing"

IMO, that should be in the patch description. :)

> This patch actually makes more sense in combination with the 'accept
> failed paths' patch, where it's much more likely to trigger.
> So if you insist I could move it over to there.

If the description is fixed, I have no problem with this patch.

-- 
Jun'ichi Nomura, NEC Corporation

Re: [PATCH 8/8] dm-mpath: do not activate failed paths

[Hannes Reinecke]

On 02/28/2014 10:37 AM, Junichi Nomura wrote:
> On 02/28/14 18:32, Hannes Reinecke wrote:
>> On 02/28/2014 09:49 AM, Junichi Nomura wrote:
>>> On 02/27/14 16:30, Hannes Reinecke wrote:
>>>> activate_path() is run without a lock, so the path might be
>>>> set to failed before activate_path() had a chance to run.
>>>>
>>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>>> ---
>>>>  drivers/md/dm-mpath.c | 7 +++++--
>>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
>>>> index 0a40fa9..22e7365 100644
>>>> --- a/drivers/md/dm-mpath.c
>>>> +++ b/drivers/md/dm-mpath.c
>>>> @@ -1196,8 +1196,11 @@ static void activate_path(struct work_struct *work)
>>>>  	struct pgpath *pgpath =
>>>>  		container_of(work, struct pgpath, activate_path.work);
>>>>  
>>>> -	scsi_dh_activate(bdev_get_queue(pgpath->path.dev->bdev),
>>>> -				pg_init_done, pgpath);
>>>> +	if (pgpath->is_active)
>>>> +		scsi_dh_activate(bdev_get_queue(pgpath->path.dev->bdev),
>>>> +				 pg_init_done, pgpath);
>>>> +	else
>>>> +		pg_init_done(pgpath, SCSI_DH_DEV_OFFLINED);
>>>
>>> Hi Hannes,
>>>
>>> What problem are you going to fix with this?
>>> It is still possible that the path is set to failed just after
>>> "if (pgpath->is_active)" and before "scsi_dh_activate".
>>>
>> activate_path() is run from a workqueue, so there is a race window
>> between the check ->is_active in __pg_init_all_paths() and the time
>> the scsi_dh_activate() is executed. And as activate_path)() is run
>> without any locks we don't have any synchronization with fail_path().
>> So it's well possible that a path gets failed in between
>> __pg_init_all_paths() and activate_paths().
> 
> What I pointed is that your patch makes the window very small
> but doesn't close it.
> If the race is a problem, this patch doesn't fix the problem.
> 
True. As said I just want to avoid unnecessary overhead by calling
functions which are known to be failing.
This patch actually makes more sense in combination with the 'accept
failed paths' patch, where it's much more likely to trigger.
So if you insist I could move it over to there.

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 8/8] dm-mpath: do not activate failed paths

[Junichi Nomura]

On 02/28/14 18:32, Hannes Reinecke wrote:
> On 02/28/2014 09:49 AM, Junichi Nomura wrote:
>> On 02/27/14 16:30, Hannes Reinecke wrote:
>>> activate_path() is run without a lock, so the path might be
>>> set to failed before activate_path() had a chance to run.
>>>
>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>> ---
>>>  drivers/md/dm-mpath.c | 7 +++++--
>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
>>> index 0a40fa9..22e7365 100644
>>> --- a/drivers/md/dm-mpath.c
>>> +++ b/drivers/md/dm-mpath.c
>>> @@ -1196,8 +1196,11 @@ static void activate_path(struct work_struct *work)
>>>  	struct pgpath *pgpath =
>>>  		container_of(work, struct pgpath, activate_path.work);
>>>  
>>> -	scsi_dh_activate(bdev_get_queue(pgpath->path.dev->bdev),
>>> -				pg_init_done, pgpath);
>>> +	if (pgpath->is_active)
>>> +		scsi_dh_activate(bdev_get_queue(pgpath->path.dev->bdev),
>>> +				 pg_init_done, pgpath);
>>> +	else
>>> +		pg_init_done(pgpath, SCSI_DH_DEV_OFFLINED);
>>
>> Hi Hannes,
>>
>> What problem are you going to fix with this?
>> It is still possible that the path is set to failed just after
>> "if (pgpath->is_active)" and before "scsi_dh_activate".
>>
> activate_path() is run from a workqueue, so there is a race window
> between the check ->is_active in __pg_init_all_paths() and the time
> the scsi_dh_activate() is executed. And as activate_path)() is run
> without any locks we don't have any synchronization with fail_path().
> So it's well possible that a path gets failed in between
> __pg_init_all_paths() and activate_paths().

What I pointed is that your patch makes the window very small
but doesn't close it.
If the race is a problem, this patch doesn't fix the problem.

-- 
Jun'ichi Nomura, NEC Corporation

Re: [PATCH 8/8] dm-mpath: do not activate failed paths

[Hannes Reinecke]

On 02/28/2014 09:49 AM, Junichi Nomura wrote:
> On 02/27/14 16:30, Hannes Reinecke wrote:
>> activate_path() is run without a lock, so the path might be
>> set to failed before activate_path() had a chance to run.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>  drivers/md/dm-mpath.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
>> index 0a40fa9..22e7365 100644
>> --- a/drivers/md/dm-mpath.c
>> +++ b/drivers/md/dm-mpath.c
>> @@ -1196,8 +1196,11 @@ static void activate_path(struct work_struct *work)
>>  	struct pgpath *pgpath =
>>  		container_of(work, struct pgpath, activate_path.work);
>>  
>> -	scsi_dh_activate(bdev_get_queue(pgpath->path.dev->bdev),
>> -				pg_init_done, pgpath);
>> +	if (pgpath->is_active)
>> +		scsi_dh_activate(bdev_get_queue(pgpath->path.dev->bdev),
>> +				 pg_init_done, pgpath);
>> +	else
>> +		pg_init_done(pgpath, SCSI_DH_DEV_OFFLINED);
> 
> Hi Hannes,
> 
> What problem are you going to fix with this?
> It is still possible that the path is set to failed just after
> "if (pgpath->is_active)" and before "scsi_dh_activate".
> 
activate_path() is run from a workqueue, so there is a race window
between the check ->is_active in __pg_init_all_paths() and the time
the scsi_dh_activate() is executed. And as activate_path)() is run
without any locks we don't have any synchronization with fail_path().
So it's well possible that a path gets failed in between
__pg_init_all_paths() and activate_paths().
Granted, we could remove the check for ->is_active in
__pg_init_all_paths(), but I wanted to avoid scheduling a workqueue
item which is known to be failing.

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 8/8] dm-mpath: do not activate failed paths

[Junichi Nomura]

On 02/27/14 16:30, Hannes Reinecke wrote:
> activate_path() is run without a lock, so the path might be
> set to failed before activate_path() had a chance to run.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/md/dm-mpath.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> index 0a40fa9..22e7365 100644
> --- a/drivers/md/dm-mpath.c
> +++ b/drivers/md/dm-mpath.c
> @@ -1196,8 +1196,11 @@ static void activate_path(struct work_struct *work)
>  	struct pgpath *pgpath =
>  		container_of(work, struct pgpath, activate_path.work);
>  
> -	scsi_dh_activate(bdev_get_queue(pgpath->path.dev->bdev),
> -				pg_init_done, pgpath);
> +	if (pgpath->is_active)
> +		scsi_dh_activate(bdev_get_queue(pgpath->path.dev->bdev),
> +				 pg_init_done, pgpath);
> +	else
> +		pg_init_done(pgpath, SCSI_DH_DEV_OFFLINED);

Hi Hannes,

What problem are you going to fix with this?
It is still possible that the path is set to failed just after
"if (pgpath->is_active)" and before "scsi_dh_activate".

-- 
Jun'ichi Nomura, NEC Corporation

[PATCH 8/8] dm-mpath: do not activate failed paths

[Hannes Reinecke]

activate_path() is run without a lock, so the path might be
set to failed before activate_path() had a chance to run.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/md/dm-mpath.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 0a40fa9..22e7365 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -1196,8 +1196,11 @@ static void activate_path(struct work_struct *work)
 	struct pgpath *pgpath =
 		container_of(work, struct pgpath, activate_path.work);
 
-	scsi_dh_activate(bdev_get_queue(pgpath->path.dev->bdev),
-				pg_init_done, pgpath);
+	if (pgpath->is_active)
+		scsi_dh_activate(bdev_get_queue(pgpath->path.dev->bdev),
+				 pg_init_done, pgpath);
+	else
+		pg_init_done(pgpath, SCSI_DH_DEV_OFFLINED);
 }
 
 static int noretry_error(int error)
-- 
1.7.12.4