[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

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

[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

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

[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

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

[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

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

[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

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

[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

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

[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 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 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 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)

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

[Hannes Reinecke]

Hi Mike,

here's now an updated patchset, which includes the comments/modifications
from Jun'ichi.

I've also updated your last patch to remove the last layer of indentation;
it's now far clearer to read.

And I've attached the 'Reviewed-By' tags from Jun'ichi where applicable.

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

 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, 101 insertions(+), 137 deletions(-)

-- 
1.7.12.4

[PATCH 4/7] 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.

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 | 71 +++++++++++++++++++++------------------------------
 1 file changed, 29 insertions(+), 42 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 0c15847..0355adc 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, 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;
+		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)
@@ -404,7 +406,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 {
@@ -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;
+		/* Use a small delay to force the use of workqueue context */
+		if (__pg_init_all_paths(m, HZ/100))
+			goto out;
+	}
 
 	/*
 	 * Wake up any thread waiting to suspend.
@@ -1589,8 +1567,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->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.7.12.4

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

[Junichi Nomura]

On 02/04/14 19:54, Hannes Reinecke wrote:
> 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.

Hi Hannes,

could you tell me how "might end up calling scsi_dh_active()" happens?

  queue_delayed_work()
    queue_delayed_work_on()
      __queue_delayed_work()
        if (!delay)
          __queue_work()
            get_work_pool()
            insert_work()
              set_work_pwq()
              get_pwq()
              if (__need_more_worker())
                wake_up_worker()

queue_work() doesn't execute the work itself.

What am I missing?

-- 
Jun'ichi Nomura, NEC Corporation

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

[Hannes Reinecke]

On 02/04/2014 12:26 PM, Junichi Nomura wrote:
> On 02/04/14 19:54, Hannes Reinecke wrote:
>> 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.
> 
> Hi Hannes,
> 
> could you tell me how "might end up calling scsi_dh_active()" happens?
> 
>   queue_delayed_work()
>     queue_delayed_work_on()
>       __queue_delayed_work()
>         if (!delay)
>           __queue_work()
>             get_work_pool()
>             insert_work()
>               set_work_pwq()
>               get_pwq()
>               if (__need_more_worker())
>                 wake_up_worker()
> 
> queue_work() doesn't execute the work itself.
> 
> What am I missing?
> 
As mentioned, I stumbled across the same issue when developing the
asynchronous SCSI aborts. I'll see to have it recreated with this
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)

--
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()

[Mike Snitzer]

On Tue, Feb 04 2014 at  6:31am -0500,
Hannes Reinecke <hare@suse.de> wrote:

> On 02/04/2014 12:26 PM, Junichi Nomura wrote:
> > On 02/04/14 19:54, Hannes Reinecke wrote:
> >> 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.
> > 
> > Hi Hannes,
> > 
> > could you tell me how "might end up calling scsi_dh_active()" happens?
> > 
> >   queue_delayed_work()
> >     queue_delayed_work_on()
> >       __queue_delayed_work()
> >         if (!delay)
> >           __queue_work()
> >             get_work_pool()
> >             insert_work()
> >               set_work_pwq()
> >               get_pwq()
> >               if (__need_more_worker())
> >                 wake_up_worker()
> > 
> > queue_work() doesn't execute the work itself.
> > 
> > What am I missing?
> > 
> As mentioned, I stumbled across the same issue when developing the
> asynchronous SCSI aborts. I'll see to have it recreated with this
> patchset.

Just to verify, this seems to be the only outstanding question for this
patchset?

What value are you using for HZ?  If this portion of the change does
turn out to be meaningul: Rather than tieing to HZ should we just use an
explicitly non-zero value for __pg_init_all_paths()'s @min_delay?

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

[Hannes Reinecke]

On 02/10/2014 02:30 PM, Mike Snitzer wrote:
> On Tue, Feb 04 2014 at  6:31am -0500,
> Hannes Reinecke <hare@suse.de> wrote:
> 
>> On 02/04/2014 12:26 PM, Junichi Nomura wrote:
>>> On 02/04/14 19:54, Hannes Reinecke wrote:
>>>> 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.
>>>
>>> Hi Hannes,
>>>
>>> could you tell me how "might end up calling scsi_dh_active()" happens?
>>>
>>>   queue_delayed_work()
>>>     queue_delayed_work_on()
>>>       __queue_delayed_work()
>>>         if (!delay)
>>>           __queue_work()
>>>             get_work_pool()
>>>             insert_work()
>>>               set_work_pwq()
>>>               get_pwq()
>>>               if (__need_more_worker())
>>>                 wake_up_worker()
>>>
>>> queue_work() doesn't execute the work itself.
>>>
>>> What am I missing?
>>>
>> As mentioned, I stumbled across the same issue when developing the
>> asynchronous SCSI aborts. I'll see to have it recreated with this
>> patchset.
> 
> Just to verify, this seems to be the only outstanding question for this
> patchset?
> 
> What value are you using for HZ?  If this portion of the change does
> turn out to be meaningul: Rather than tieing to HZ should we just use an
> explicitly non-zero value for __pg_init_all_paths()'s @min_delay?
> 
The actual amount here is irrelevant, as long as it's non-zero.
It's just there to force execution of the work item off the current
thread.

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()

[Mike Snitzer]

On Tue, Feb 11 2014 at  4:46am -0500,
Hannes Reinecke <hare@suse.de> wrote:

> On 02/10/2014 02:30 PM, Mike Snitzer wrote:
> > 
> > Just to verify, this seems to be the only outstanding question for this
> > patchset?
> > 
> > What value are you using for HZ?  If this portion of the change does
> > turn out to be meaningul: Rather than tieing to HZ should we just use an
> > explicitly non-zero value for __pg_init_all_paths()'s @min_delay?
> > 
> The actual amount here is irrelevant, as long as it's non-zero.
> It's just there to force execution of the work item off the current
> thread.

I'm aware we just need a non-zero value.  My concern, as originally
raised by Junichi in an earlier reply when you had it as HZ/50, is that
the value could be 0 if HZ is really small.  While unlikely I see no
point allowing the variable nature of HZ compromise passing a non-zero
value here.  Best to just be explicit by passing 1 or something.

All said, the question of why this is actually needed remains.  I trust
you're working on answering that via reproducer (by not forcing the use
of workqueue context)?

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

[Hannes Reinecke]

On 02/11/2014 04:55 PM, Mike Snitzer wrote:
> On Tue, Feb 11 2014 at  4:46am -0500,
> Hannes Reinecke <hare@suse.de> wrote:
>
>> On 02/10/2014 02:30 PM, Mike Snitzer wrote:
>>>
>>> Just to verify, this seems to be the only outstanding question for this
>>> patchset?
>>>
>>> What value are you using for HZ?  If this portion of the change does
>>> turn out to be meaningul: Rather than tieing to HZ should we just use an
>>> explicitly non-zero value for __pg_init_all_paths()'s @min_delay?
>>>
>> The actual amount here is irrelevant, as long as it's non-zero.
>> It's just there to force execution of the work item off the current
>> thread.
>
> I'm aware we just need a non-zero value.  My concern, as originally
> raised by Junichi in an earlier reply when you had it as HZ/50, is that
> the value could be 0 if HZ is really small.  While unlikely I see no
> point allowing the variable nature of HZ compromise passing a non-zero
> value here.  Best to just be explicit by passing 1 or something.
>
> All said, the question of why this is actually needed remains.  I trust
> you're working on answering that via reproducer (by not forcing the use
> of workqueue context)?
>
Precisely.

But as this is a bit hard to trigger it might take some time.
(you'll only be hitting this issue if you have to retry 
scsi_dh_activate, so you'll need to trigger this somehow).

I hope to get it done this week.
Is there any deadline which I might miss with that?

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()

[Mike Snitzer]

On Tue, Feb 11 2014 at  1:03pm -0500,
Hannes Reinecke <hare@suse.de> wrote:

> On 02/11/2014 04:55 PM, Mike Snitzer wrote:
> >On Tue, Feb 11 2014 at  4:46am -0500,
> >Hannes Reinecke <hare@suse.de> wrote:
> >
> >>On 02/10/2014 02:30 PM, Mike Snitzer wrote:
> >>>
> >>>Just to verify, this seems to be the only outstanding question for this
> >>>patchset?
> >>>
> >>>What value are you using for HZ?  If this portion of the change does
> >>>turn out to be meaningul: Rather than tieing to HZ should we just use an
> >>>explicitly non-zero value for __pg_init_all_paths()'s @min_delay?
> >>>
> >>The actual amount here is irrelevant, as long as it's non-zero.
> >>It's just there to force execution of the work item off the current
> >>thread.
> >
> >I'm aware we just need a non-zero value.  My concern, as originally
> >raised by Junichi in an earlier reply when you had it as HZ/50, is that
> >the value could be 0 if HZ is really small.  While unlikely I see no
> >point allowing the variable nature of HZ compromise passing a non-zero
> >value here.  Best to just be explicit by passing 1 or something.
> >
> >All said, the question of why this is actually needed remains.  I trust
> >you're working on answering that via reproducer (by not forcing the use
> >of workqueue context)?
> >
> Precisely.
> 
> But as this is a bit hard to trigger it might take some time.
> (you'll only be hitting this issue if you have to retry
> scsi_dh_activate, so you'll need to trigger this somehow).

OK.

> I hope to get it done this week.
> Is there any deadline which I might miss with that?

No, that'll be great.  We have some time until the 3.15 merge window
opens.

Thanks,
Mike

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

[Junichi Nomura]

On 02/10/14 22:30, Mike Snitzer wrote:
> On Tue, Feb 04 2014 at  6:31am -0500,
> Hannes Reinecke <hare@suse.de> wrote:
> 
>> On 02/04/2014 12:26 PM, Junichi Nomura wrote:
>>> On 02/04/14 19:54, Hannes Reinecke wrote:
>>>> 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.
>>>
>>> Hi Hannes,
>>>
>>> could you tell me how "might end up calling scsi_dh_active()" happens?
>>>
>>>   queue_delayed_work()
>>>     queue_delayed_work_on()
>>>       __queue_delayed_work()
>>>         if (!delay)
>>>           __queue_work()
>>>             get_work_pool()
>>>             insert_work()
>>>               set_work_pwq()
>>>               get_pwq()
>>>               if (__need_more_worker())
>>>                 wake_up_worker()
>>>
>>> queue_work() doesn't execute the work itself.
>>>
>>> What am I missing?
>>>
>> As mentioned, I stumbled across the same issue when developing the
>> asynchronous SCSI aborts. I'll see to have it recreated with this
>> patchset.
> 
> Just to verify, this seems to be the only outstanding question for this
> patchset?

Hi Mike, Hannes,

Yes, this is the only remaining question from me.
For other parts of this patch and patchset, issues I pointed were solved.

-- 
Jun'ichi Nomura, NEC Corporation