[PATCH 0/4] dm-mpath: bug fixes around pg-init handling

[PATCH 0/4] dm-mpath: bug fixes around pg-init handling

[Kiyoshi Ueda]

Hi Alasdair and multipath developers,

I've started to work on removing the multipath internal I/O queue,
since request-based dm-core now has a generic queue and multipath
can use it instead of having its own queue.

As a first step, I'm separating pg-init handling from queued I/O
handling.  During code inspection for that work, I found some bugs
in the current pg-init handling codes.
This patch-set fixes those bugs (and implies preparations for
the work of removing the multipath internal queue).

I don't have any active-passive storage, so I've just done compile
and boot tests.  Please review, and test on your storages.

This patch-set was made on top of 2.6.33-rc6 + Alasdair's patches below:
  dm-mpath-pass-struct-pgpath-to-pg-init-done.patch
  dm-mpath-skip-activate_path-for-failed-paths.patch
  dm-mpath-Remove-suspended-flag-from-struct-multipath.patch

Summary of the patch-set:
  1/4: dm-mpath: don't clear m->queue_io until all activations complete
  2/4: dm-mpath: must wait for pg-init completion in postsuspend
  3/4: dm-mpath: separate pg-init handling from process_queued_ios()
  4/4: dm-mpath: move initial pg-init kick into __switch_pg()

 drivers/md/dm-mpath.c |  131 +++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 97 insertions(+), 34 deletions(-)

Thanks,
Kiyoshi Ueda

[PATCH 1/4] dm-mpath: don't clear m->queue_io until all activations complete

[Kiyoshi Ueda]

m->queue_io is set to block processing I/Os, and it needs to be kept
while pg-init, which issues multiple path activations, is in progress.
But m->queue is cleared when a path activation completes without error
in pg_init_done(), even while other path activations are in progress.
That may cause undesired -EIO on paths which are not complete activation.

This patch fixes that by not clearing m->queue_io until all path
activations complete.

Signed-off-by: Kiyoshi Ueda <k-ueda@ct.jp.nec.com>
Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Cc: Alasdair G Kergon <agk@redhat.com>
---
 drivers/md/dm-mpath.c |   17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

Index: 2.6.33-rc6/drivers/md/dm-mpath.c
===================================================================
--- 2.6.33-rc6.orig/drivers/md/dm-mpath.c
+++ 2.6.33-rc6/drivers/md/dm-mpath.c
@@ -1181,14 +1181,19 @@ static void pg_init_done(void *data, int
 			m->current_pgpath = NULL;
 			m->current_pg = NULL;
 		}
-	} else if (!m->pg_init_required) {
-		m->queue_io = 0;
+	} else if (!m->pg_init_required)
 		pg->bypassed = 0;
-	}
 
-	m->pg_init_in_progress--;
-	if (!m->pg_init_in_progress)
-		queue_work(kmultipathd, &m->process_queued_ios);
+	if (--m->pg_init_in_progress)
+		/* Activations of other paths are still on going */
+		goto out;
+
+	if (!m->pg_init_required)
+		m->queue_io = 0;
+
+	queue_work(kmultipathd, &m->process_queued_ios);
+
+out:
 	spin_unlock_irqrestore(&m->lock, flags);
 }
 

[PATCH 2/4] dm-mpath: must wait for pg-init completion in postsuspend

[Kiyoshi Ueda]

Target must be quiet if it is suspended.  So multipath_postsusped()
flushes all workqueues to become quiet.
But pg-init may be still in progress even after flushing the workqueue
for kmpath_handlerd.

This patch waits for pg-init completion correctly in
multipath_postsuspend().

Signed-off-by: Kiyoshi Ueda <k-ueda@ct.jp.nec.com>
Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Cc: Alasdair G Kergon <agk@redhat.com>
---
 drivers/md/dm-mpath.c |   38 +++++++++++++++++++++++++++++++++++---
 1 file changed, 35 insertions(+), 3 deletions(-)

Index: 2.6.33-rc6/drivers/md/dm-mpath.c
===================================================================
--- 2.6.33-rc6.orig/drivers/md/dm-mpath.c
+++ 2.6.33-rc6/drivers/md/dm-mpath.c
@@ -69,6 +69,7 @@ struct multipath {
 	struct list_head priority_groups;
 	unsigned pg_init_required;	/* pg_init needs calling? */
 	unsigned pg_init_in_progress;	/* Only one pg_init allowed at once */
+	wait_queue_head_t wait;		/* Wait for pg_init completion */
 
 	unsigned nr_valid_paths;	/* Total number of usable paths */
 	struct pgpath *current_pgpath;
@@ -200,6 +201,7 @@ static struct multipath *alloc_multipath
 		m->queue_io = 1;
 		INIT_WORK(&m->process_queued_ios, process_queued_ios);
 		INIT_WORK(&m->trigger_event, trigger_event);
+		init_waitqueue_head(&m->wait);
 		mutex_init(&m->work_mutex);
 		m->mpio_pool = mempool_create_slab_pool(MIN_IOS, _mpio_cache);
 		if (!m->mpio_pool) {
@@ -891,9 +893,34 @@ static int multipath_ctr(struct dm_targe
 	return r;
 }
 
-static void flush_multipath_work(void)
+static void multipath_wait_for_pg_init_completion(struct multipath *m)
+{
+	DECLARE_WAITQUEUE(wait, current);
+	unsigned long flags;
+
+	add_wait_queue(&m->wait, &wait);
+
+	while (1) {
+		set_current_state(TASK_UNINTERRUPTIBLE);
+
+		spin_lock_irqsave(&m->lock, flags);
+		if (!m->pg_init_in_progress) {
+			spin_unlock_irqrestore(&m->lock, flags);
+			break;
+		}
+		spin_unlock_irqrestore(&m->lock, flags);
+
+		io_schedule();
+	}
+	set_current_state(TASK_RUNNING);
+
+	remove_wait_queue(&m->wait, &wait);
+}
+
+static void flush_multipath_work(struct multipath *m)
 {
 	flush_workqueue(kmpath_handlerd);
+	multipath_wait_for_pg_init_completion(m);
 	flush_workqueue(kmultipathd);
 	flush_scheduled_work();
 }
@@ -902,7 +929,7 @@ static void multipath_dtr(struct dm_targ
 {
 	struct multipath *m = ti->private;
 
-	flush_multipath_work();
+	flush_multipath_work(m);
 	free_multipath(m);
 }
 
@@ -1193,6 +1220,11 @@ static void pg_init_done(void *data, int
 
 	queue_work(kmultipathd, &m->process_queued_ios);
 
+	/*
+	 * Must not do anything related to pg-init after waking up the waiter
+	 */
+	wake_up(&m->wait);
+
 out:
 	spin_unlock_irqrestore(&m->lock, flags);
 }
@@ -1281,7 +1313,7 @@ static void multipath_postsuspend(struct
 	struct multipath *m = ti->private;
 
 	mutex_lock(&m->work_mutex);
-	flush_multipath_work();
+	flush_multipath_work(m);
 	mutex_unlock(&m->work_mutex);
 }
 

[PATCH 3/4] dm-mpath: separate pg-init handling from process_queued_ios()

[Kiyoshi Ueda]

This patch is a preparation of the next patch, which fixes the issue
that ioctl isn't processed until any I/O is issued.  (And also it is
a preparation of another patch-set to remove multipath internal queue.)
No functional change.

This patch changes pg-init kickers to use activate_path() directly
instead of using process_queued_ios().

process_queued_ios() has been used for pg-init handling, which needs
a kthread context.
However, recent development introduced a special work, activate_path(),
for that purpose.  So there is no need to use process_queued_ios().

Signed-off-by: Kiyoshi Ueda <k-ueda@ct.jp.nec.com>
Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Cc: Alasdair G Kergon <agk@redhat.com>
---
 drivers/md/dm-mpath.c |   55 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 39 insertions(+), 16 deletions(-)

Index: 2.6.33-rc6/drivers/md/dm-mpath.c
===================================================================
--- 2.6.33-rc6.orig/drivers/md/dm-mpath.c
+++ 2.6.33-rc6/drivers/md/dm-mpath.c
@@ -235,6 +235,21 @@ static void free_multipath(struct multip
  * Path selection
  *-----------------------------------------------*/
 
+static void __pg_init(struct multipath *m)
+{
+	struct pgpath *pgpath;
+
+	m->pg_init_count++;
+	m->pg_init_required = 0;
+	list_for_each_entry(pgpath, &m->current_pg->pgpaths, list) {
+		/* Skip failed paths */
+		if (!pgpath->is_active)
+			continue;
+		if (queue_work(kmpath_handlerd, &pgpath->activate_path))
+			m->pg_init_in_progress++;
+	}
+}
+
 static void __switch_pg(struct multipath *m, struct pgpath *pgpath)
 {
 	m->current_pg = pgpath->pg;
@@ -350,8 +365,9 @@ static int map_io(struct multipath *m, s
 		/* Queue for the daemon to resubmit */
 		list_add_tail(&clone->queuelist, &m->queued_ios);
 		m->queue_size++;
-		if ((m->pg_init_required && !m->pg_init_in_progress) ||
-		    !m->queue_io)
+		if (m->pg_init_required && !m->pg_init_in_progress && pgpath)
+			__pg_init(m);
+		else if (!m->queue_io)
 			queue_work(kmultipathd, &m->process_queued_ios);
 		pgpath = NULL;
 		r = DM_MAPIO_SUBMITTED;
@@ -439,7 +455,7 @@ static void process_queued_ios(struct wo
 {
 	struct multipath *m =
 		container_of(work, struct multipath, process_queued_ios);
-	struct pgpath *pgpath = NULL, *tmp;
+	struct pgpath *pgpath = NULL;
 	unsigned must_queue = 1;
 	unsigned long flags;
 
@@ -457,17 +473,9 @@ static void process_queued_ios(struct wo
 	    (!pgpath && !m->queue_if_no_path))
 		must_queue = 0;
 
-	if (m->pg_init_required && !m->pg_init_in_progress && pgpath) {
-		m->pg_init_count++;
-		m->pg_init_required = 0;
-		list_for_each_entry(tmp, &pgpath->pg->pgpaths, list) {
-			/* Skip failed paths */
-			if (!tmp->is_active)
-				continue;
-			if (queue_work(kmpath_handlerd, &tmp->activate_path))
-				m->pg_init_in_progress++;
-		}
-	}
+	if (m->pg_init_required && !m->pg_init_in_progress && pgpath)
+		__pg_init(m);
+
 out:
 	spin_unlock_irqrestore(&m->lock, flags);
 	if (!must_queue)
@@ -1215,9 +1223,24 @@ static void pg_init_done(void *data, int
 		/* Activations of other paths are still on going */
 		goto out;
 
-	if (!m->pg_init_required)
-		m->queue_io = 0;
+	if (m->pg_init_required) {
+		/* Requested retry or a new pg-init */
+		if (likely(m->current_pgpath)) {
+			__pg_init(m);
+			goto out;
+		}
+
+		/*
+		 * The condition requiring pg-init has been changed by someone
+		 * after the pg-init had been requested.
+		 * Cancel m->pg_init_required here explicitly, and start over
+		 * from path selection.
+		 */
+		m->pg_init_required = 0;
+		m->current_pg = NULL;
+	}
 
+	m->queue_io = 0;
 	queue_work(kmultipathd, &m->process_queued_ios);
 
 	/*

Re: [PATCH 3/4] dm-mpath: separate pg-init handling from process_queued_ios()

[Alasdair G Kergon]

On Mon, Feb 01, 2010 at 01:22:23PM +0900, Kiyoshi Ueda wrote:
> This patch is a preparation of the next patch, which fixes the issue
> that ioctl isn't processed until any I/O is issued.  (And also it is
> a preparation of another patch-set to remove multipath internal queue.)
> No functional change.
 
I've split this into two, but it's very 'tight' code forming a state
machine here, and while I understand the principle of the patch, I can't
spot an easy way to tell that all the paths through the code are right.
Is it worth trying to extend the explanation, or will later patches be
replacing this code, making it better to wait for that new code before
reviewing?

  ftp://sources.redhat.com/pub/dm/patches/2.6-unstable/editing/patches/dm-mpath-refactor-pg_init-trigger.patch

Alasdair

Re: [PATCH 3/4] dm-mpath: separate pg-init handling from process_queued_ios()

[Kiyoshi Ueda]

Hi Alasdair,

Thanks for your review and comments.

On 02/03/2010 03:24 AM +0900, Alasdair G Kergon wrote:
> On Mon, Feb 01, 2010 at 01:22:23PM +0900, Kiyoshi Ueda wrote:
>> This patch is a preparation of the next patch, which fixes the issue
>> that ioctl isn't processed until any I/O is issued.  (And also it is
>> a preparation of another patch-set to remove multipath internal queue.)
>> No functional change.
>  
> I've split this into two, but it's very 'tight' code forming a state
> machine here, and while I understand the principle of the patch, I can't
> spot an easy way to tell that all the paths through the code are right.
> Is it worth trying to extend the explanation, or will later patches be
> replacing this code, making it better to wait for that new code before
> reviewing?
> 
>   ftp://sources.redhat.com/pub/dm/patches/2.6-unstable/editing/patches/dm-mpath-refactor-pg_init-trigger.patch

While trying to extend the explanation, I come to think there may be
better implementation for pg_init refactoring.
So please wait reviewing dm-mpath-refactor-pg_init-trigger.patch until
my next update.
(I think dm-mpath-refactor-pg_init.patch is no problem to apply even
 at this point.)

Thanks,
Kiyoshi Ueda

Re: [PATCH 3/4] dm-mpath: separate pg-init handling from process_queued_ios()

[Alasdair G Kergon]

On Thu, Feb 04, 2010 at 07:31:15PM +0900, Kiyoshi Ueda wrote:
> While trying to extend the explanation, I come to think there may be
> better implementation for pg_init refactoring.
> So please wait reviewing dm-mpath-refactor-pg_init-trigger.patch until
> my next update.
> (I think dm-mpath-refactor-pg_init.patch is no problem to apply even
>  at this point.)
 
OK.

Alasdair

[PATCH 4/4] dm-mpath: move initial pg-init kick into __switch_pg()

[Kiyoshi Ueda]

This patch moves initial pg-init kick into __switch_pg().
This has 2 meanings below:
  - Fix the issue that ioctl isn't processed until any I/O is issued.
    multipath_ioctl() doesn't kick pg-init even if it selects a path
    in a new pg, so it returns with -EAGAIN.  That state never be
    changed and ioctl is always returns with -EAGAIN until any I/O is
    issued and pg-init is kicked.
    By this patch, multipath_ioctl() kicks pg-init when it selects
    a path in a new pg, so following ioctl will be processed in
    the near future without any I/O.

  - By this patch, process_queued_ios() has no pg-init handling and
    it works just for queued I/Os in the multipath internal queue.
    So this patch makes removing multipath internal queue easy.

Signed-off-by: Kiyoshi Ueda <k-ueda@ct.jp.nec.com>
Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Cc: Alasdair G Kergon <agk@redhat.com>
---
 drivers/md/dm-mpath.c |   21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

Index: 2.6.33-rc6/drivers/md/dm-mpath.c
===================================================================
--- 2.6.33-rc6.orig/drivers/md/dm-mpath.c
+++ 2.6.33-rc6/drivers/md/dm-mpath.c
@@ -256,14 +256,22 @@ static void __switch_pg(struct multipath
 
 	/* Must we initialise the PG first, and queue I/O till it's ready? */
 	if (m->hw_handler_name) {
-		m->pg_init_required = 1;
 		m->queue_io = 1;
+
+		/* Reset pg_init_count in 0 anyway to start a new pg-init */
+		m->pg_init_count = 0;
+		if (!m->pg_init_in_progress)
+			__pg_init(m);
+		else
+			/*
+			 * Mark to start a pg-init again when the current one
+			 * completes.
+			 */
+			m->pg_init_required = 1;
 	} else {
 		m->pg_init_required = 0;
 		m->queue_io = 0;
 	}
-
-	m->pg_init_count = 0;
 }
 
 static int __choose_path_in_pg(struct multipath *m, struct priority_group *pg,
@@ -365,9 +373,7 @@ static int map_io(struct multipath *m, s
 		/* Queue for the daemon to resubmit */
 		list_add_tail(&clone->queuelist, &m->queued_ios);
 		m->queue_size++;
-		if (m->pg_init_required && !m->pg_init_in_progress && pgpath)
-			__pg_init(m);
-		else if (!m->queue_io)
+		if (!m->queue_io)
 			queue_work(kmultipathd, &m->process_queued_ios);
 		pgpath = NULL;
 		r = DM_MAPIO_SUBMITTED;
@@ -473,9 +479,6 @@ static void process_queued_ios(struct wo
 	    (!pgpath && !m->queue_if_no_path))
 		must_queue = 0;
 
-	if (m->pg_init_required && !m->pg_init_in_progress && pgpath)
-		__pg_init(m);
-
 out:
 	spin_unlock_irqrestore(&m->lock, flags);
 	if (!must_queue)