[PATCH 0/2] dm-mpath: mutex and suspended check for messages

[PATCH 0/2] dm-mpath: mutex and suspended check for messages

[Mike Anderson]

This patch series is applied on top of the patch previously posted by
Kiyoshi Ueda. Archive url provided below.

http://permalink.gmane.org/gmane.linux.kernel.device-mapper.devel/10504

This patch series adds a mutex and element to the multipath structure to
allow synchronization between possible work creators and callers flushing
work while providing indication of suspended state.

---

Mike Anderson (2):
      dm-mpath: Add mutex to synchronize adding and flushing work
      dm-mpath: Add element for suspended state.


 drivers/md/dm-mpath.c |   71 +++++++++++++++++++++++++++++++++++--------------
 1 files changed, 50 insertions(+), 21 deletions(-)

--
-andmike

Michael Anderson <andmike@linux.vnet.ibm.com>

[PATCH 1/2] dm-mpath: Add mutex to synchronize adding and flushing work

[Mike Anderson]

Add a mutex to allow possible creators of new work to synchronize with
flushing work queues.

Signed-off-by: Mike Anderson <andmike@linux.vnet.ibm.com>
---
 drivers/md/dm-mpath.c |   59 ++++++++++++++++++++++++++++++++-----------------
 1 files changed, 38 insertions(+), 21 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 6ffffad..b102959 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -93,6 +93,8 @@ struct multipath {
 	 * can resubmit bios on error.
 	 */
 	mempool_t *mpio_pool;
+
+	struct mutex work_mutex;
 };
 
 /*
@@ -198,6 +200,7 @@ static struct multipath *alloc_multipath(struct dm_target *ti)
 		m->queue_io = 1;
 		INIT_WORK(&m->process_queued_ios, process_queued_ios);
 		INIT_WORK(&m->trigger_event, trigger_event);
+		mutex_init(&m->work_mutex);
 		m->mpio_pool = mempool_create_slab_pool(MIN_IOS, _mpio_cache);
 		if (!m->mpio_pool) {
 			kfree(m);
@@ -1268,7 +1271,11 @@ static void multipath_presuspend(struct dm_target *ti)
 
 static void multipath_postsuspend(struct dm_target *ti)
 {
+	struct multipath *m = (struct multipath *) ti->private;
+
+	mutex_lock(&m->work_mutex);
 	flush_multipath_work();
+	mutex_unlock(&m->work_mutex);
 }
 
 /*
@@ -1407,51 +1414,61 @@ static int multipath_status(struct dm_target *ti, status_type_t type,
 
 static int multipath_message(struct dm_target *ti, unsigned argc, char **argv)
 {
-	int r;
+	int r = -EINVAL;
 	struct dm_dev *dev;
 	struct multipath *m = (struct multipath *) ti->private;
 	action_fn action;
 
+	mutex_lock(&m->work_mutex);
+
 	if (argc == 1) {
-		if (!strnicmp(argv[0], MESG_STR("queue_if_no_path")))
-			return queue_if_no_path(m, 1, 0);
-		else if (!strnicmp(argv[0], MESG_STR("fail_if_no_path")))
-			return queue_if_no_path(m, 0, 0);
+		if (!strnicmp(argv[0], MESG_STR("queue_if_no_path"))) {
+			r = queue_if_no_path(m, 1, 0);
+			goto out;
+		} else if (!strnicmp(argv[0], MESG_STR("fail_if_no_path"))) {
+			r = queue_if_no_path(m, 0, 0);
+			goto out;
+		}
 	}
 
-	if (argc != 2)
-		goto error;
+	if (argc != 2) {
+		DMWARN("Unrecognised multipath message received.");
+		goto out;
+	}
 
-	if (!strnicmp(argv[0], MESG_STR("disable_group")))
-		return bypass_pg_num(m, argv[1], 1);
-	else if (!strnicmp(argv[0], MESG_STR("enable_group")))
-		return bypass_pg_num(m, argv[1], 0);
-	else if (!strnicmp(argv[0], MESG_STR("switch_group")))
-		return switch_pg_num(m, argv[1]);
-	else if (!strnicmp(argv[0], MESG_STR("reinstate_path")))
+	if (!strnicmp(argv[0], MESG_STR("disable_group"))) {
+		r = bypass_pg_num(m, argv[1], 1);
+		goto out;
+	} else if (!strnicmp(argv[0], MESG_STR("enable_group"))) {
+		r = bypass_pg_num(m, argv[1], 0);
+		goto out;
+	} else if (!strnicmp(argv[0], MESG_STR("switch_group"))) {
+		r = switch_pg_num(m, argv[1]);
+		goto out;
+	} else if (!strnicmp(argv[0], MESG_STR("reinstate_path")))
 		action = reinstate_path;
 	else if (!strnicmp(argv[0], MESG_STR("fail_path")))
 		action = fail_path;
-	else
-		goto error;
+	else {
+		DMWARN("Unrecognised multipath message received.");
+		goto out;
+	}
 
 	r = dm_get_device(ti, argv[1], ti->begin, ti->len,
 			  dm_table_get_mode(ti->table), &dev);
 	if (r) {
 		DMWARN("message: error getting device %s",
 		       argv[1]);
-		return -EINVAL;
+		goto out;
 	}
 
 	r = action_dev(m, dev, action);
 
 	dm_put_device(ti, dev);
 
+out:
+	mutex_unlock(&m->work_mutex);
 	return r;
-
-error:
-	DMWARN("Unrecognised multipath message received.");
-	return -EINVAL;
 }
 
 static int multipath_ioctl(struct dm_target *ti, unsigned int cmd,

Re: [PATCH 1/2] dm-mpath: Add mutex to synchronize adding and flushing work

[Kiyoshi Ueda]

Hi Mike,

On 11/16/2009 04:38 PM +0900, Mike Anderson wrote:
> Add a mutex to allow possible creators of new work to synchronize with
> flushing work queues.
<snip>
> @@ -1268,7 +1271,11 @@ static void multipath_presuspend(struct dm_target *ti)
>  
>  static void multipath_postsuspend(struct dm_target *ti)
>  {
> +	struct multipath *m = (struct multipath *) ti->private;

Please drop the cast when it's from void.

For others,
Acked-by: Kiyoshi Ueda <k-ueda@ct.jp.nec.com>

Thanks,
Kiyoshi Ueda

[PATCH 2/2] dm-mpath: Add element for suspended state.

[Mike Anderson]

Add element to multipath structure for indication of suspended state.

Signed-off-by: Mike Anderson <andmike@linux.vnet.ibm.com>
---
 drivers/md/dm-mpath.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index b102959..1f598c9 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -95,6 +95,8 @@ struct multipath {
 	mempool_t *mpio_pool;
 
 	struct mutex work_mutex;
+
+	unsigned int  suspended;
 };
 
 /*
@@ -1274,6 +1276,7 @@ static void multipath_postsuspend(struct dm_target *ti)
 	struct multipath *m = (struct multipath *) ti->private;
 
 	mutex_lock(&m->work_mutex);
+	m->suspended = 1;
 	flush_multipath_work();
 	mutex_unlock(&m->work_mutex);
 }
@@ -1286,6 +1289,10 @@ static void multipath_resume(struct dm_target *ti)
 	struct multipath *m = (struct multipath *) ti->private;
 	unsigned long flags;
 
+	mutex_lock(&m->work_mutex);
+	m->suspended = 0;
+	mutex_unlock(&m->work_mutex);
+
 	spin_lock_irqsave(&m->lock, flags);
 	m->queue_if_no_path = m->saved_queue_if_no_path;
 	spin_unlock_irqrestore(&m->lock, flags);
@@ -1421,6 +1428,11 @@ static int multipath_message(struct dm_target *ti, unsigned argc, char **argv)
 
 	mutex_lock(&m->work_mutex);
 
+	if (m->suspended) {
+		r = -EBUSY;
+		goto out;
+	}
+
 	if (argc == 1) {
 		if (!strnicmp(argv[0], MESG_STR("queue_if_no_path"))) {
 			r = queue_if_no_path(m, 1, 0);

Re: [PATCH 2/2] dm-mpath: Add element for suspended state.

[Kiyoshi Ueda]

On 11/16/2009 04:38 PM +0900, Mike Anderson wrote:
> Add element to multipath structure for indication of suspended state.

Acked-by: Kiyoshi Ueda <k-ueda@ct.jp.nec.com>

Thanks,
Kiyoshi Ueda

Re: [PATCH 2/2] dm-mpath: Add element for suspended state.

[Alasdair G Kergon]

On Sun, Nov 15, 2009 at 11:38:29PM -0800, Mike Anderson wrote:
> Add element to multipath structure for indication of suspended state.
 
Is there a way to avoid this?
It seems redundant for a target to need to track whether or not
it is suspended.  Core dm should be capable of that.

dm_suspend has:
        dm_table_postsuspend_targets(map);

        set_bit(DMF_SUSPENDED, &md->flags);

Can we reorder those two?

What about dm_resume?
        clear_bit(DMF_SUSPENDED, &md->flags);
Can that move a little higher up the function?
- preresume, clear DMF_SUSPENDED, resume perhaps?

Alasdair

Re: [PATCH 2/2] dm-mpath: Add element for suspended state.

[Kiyoshi Ueda]

Hi Alasdair, Mike,

On 11/16/2009 10:54 PM +0900, Alasdair G Kergon wrote:
> On Sun, Nov 15, 2009 at 11:38:29PM -0800, Mike Anderson wrote:
>> Add element to multipath structure for indication of suspended state.
>  
> Is there a way to avoid this?
> It seems redundant for a target to need to track whether or not
> it is suspended.  Core dm should be capable of that.
> 
> dm_suspend has:
>         dm_table_postsuspend_targets(map);
> 
>         set_bit(DMF_SUSPENDED, &md->flags);
> 
> Can we reorder those two?

For multipath and dm-core, yes.
I'm not sure whether other targets care about the ordering.

But from semantics point of view, is it confusing if dm_suspended()
returns true but a target is doing something in postsuspend()?

If we take the approach instead of this patch, the patch-set is like:
    1/4: http://patchwork.kernel.org/patch/61588/
    2/4: http://patchwork.kernel.org/patch/61589/
    3/4: http://patchwork.kernel.org/patch/61594/
    4/4: http://patchwork.kernel.org/patch/61595/

Please review.


> What about dm_resume?
>         clear_bit(DMF_SUSPENDED, &md->flags);
> Can that move a little higher up the function?
> - preresume, clear DMF_SUSPENDED, resume perhaps?

I think we shouldn't move the clear_bit in resume.

It doesn't make sense to clear the flag before I/Os start flowing.
If targets want to do any preparation before I/Os flowing through,
it can do in the resume() callback.

Thanks,
Kiyoshi Ueda