[PATCH 0/2] Add suspended state check for target messages

[PATCH 0/2] Add suspended state check for target messages

[Mike Anderson]

This patch adds a accessor function that allows determining if a
mapped_device is in the suspended state and then adds this check to the
multipath targets multipath_message function.

As previously described in the email at the archive url provided below the
target_message ioctl can call into the target after suspend completes.
This can cause new work to be queued and actions to be taken by the target
that should be prevented in the suspended state.

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

---

Mike Anderson (2):
      dm: Add accessor dm_table_md_suspended
      dm: Add suspended check to multipath_message


 drivers/md/dm-mpath.c         |    3 +++
 drivers/md/dm-table.c         |    6 ++++++
 include/linux/device-mapper.h |    1 +
 3 files changed, 10 insertions(+), 0 deletions(-)

--
-andmike

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

[PATCH 1/2] dm: Add accessor dm_table_md_suspended

[Mike Anderson]

Add a dm_table accessor function to check if md is suspended.

Signed-off-by: Mike Anderson <andmike@linux.vnet.ibm.com>
---
 drivers/md/dm-table.c         |    6 ++++++
 include/linux/device-mapper.h |    1 +
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 1a6cb3c..6616598 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1245,6 +1245,11 @@ struct mapped_device *dm_table_get_md(struct dm_table *t)
 	return t->md;
 }
 
+int dm_table_md_suspended(struct dm_table *t)
+{
+	return dm_suspended(t->md);
+}
+
 EXPORT_SYMBOL(dm_vcalloc);
 EXPORT_SYMBOL(dm_get_device);
 EXPORT_SYMBOL(dm_put_device);
@@ -1255,3 +1260,4 @@ EXPORT_SYMBOL(dm_table_get_md);
 EXPORT_SYMBOL(dm_table_put);
 EXPORT_SYMBOL(dm_table_get);
 EXPORT_SYMBOL(dm_table_unplug_all);
+EXPORT_SYMBOL(dm_table_md_suspended);
diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
index df7607e..3440c54 100644
--- a/include/linux/device-mapper.h
+++ b/include/linux/device-mapper.h
@@ -287,6 +287,7 @@ sector_t dm_table_get_size(struct dm_table *t);
 unsigned int dm_table_get_num_targets(struct dm_table *t);
 fmode_t dm_table_get_mode(struct dm_table *t);
 struct mapped_device *dm_table_get_md(struct dm_table *t);
+int dm_table_md_suspended(struct dm_table *t);
 
 /*
  * Trigger an event.

[PATCH 2/2] dm: Add suspended check to multipath_message

[Mike Anderson]

Add suspended check to multipath_message

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

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 32d0b87..ee56ef7 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -1402,6 +1402,9 @@ static int multipath_message(struct dm_target *ti, unsigned argc, char **argv)
 	struct multipath *m = (struct multipath *) ti->private;
 	action_fn action;
 
+	if (dm_table_md_suspended(ti->table))
+		return -EBUSY;
+
 	if (argc == 1) {
 		if (!strnicmp(argv[0], MESG_STR("queue_if_no_path")))
 			return queue_if_no_path(m, 1, 0);

Re: [PATCH 0/2] Add suspended state check for target messages

[Kiyoshi Ueda]

Hi Mike,

On 11/13/2009 05:06 PM +0900, Mike Anderson wrote:
> This patch adds a accessor function that allows determining if a
> mapped_device is in the suspended state and then adds this check to the
> multipath targets multipath_message function.
> 
> As previously described in the email at the archive url provided below the
> target_message ioctl can call into the target after suspend completes.
> This can cause new work to be queued and actions to be taken by the target
> that should be prevented in the suspended state.
> 
> http://permalink.gmane.org/gmane.linux.kernel.device-mapper.devel/10486
> 
> ---
> 
> Mike Anderson (2):
>       dm: Add accessor dm_table_md_suspended
>       dm: Add suspended check to multipath_message

Thank you for the patch.
But this patch-set doesn't solve the race problem.

Multipath must start rejecting message ioctl *before* flushing
workqueues in postsuspend.
  (*) With my patch, all multipath works are flushed in postsuspend.
      http://patchwork.kernel.org/patch/59556/

However, your patch starts rejecting message ioctl *after* postsuspend.
So there is a small window below:
  dm_suspend()                        multipath_message()
  ------------------------------------------------------------------
  dm_table_post_suspend()
    -> flush_multipath_works()
                                      if (!test_bit(DMF_SUSPENDED))
                                          queue_work()
  set_bit(DMF_SUSPENDED)
  ...
  dm_swap_table()
                                      [de]activate_path()

Thanks,
Kiyoshi Ueda

Re: [PATCH 1/2] dm: Add accessor dm_table_md_suspended

[Mike Snitzer]

On Fri, Nov 13 2009 at  3:06am -0500,
Mike Anderson <andmike@linux.vnet.ibm.com> wrote:

> Add a dm_table accessor function to check if md is suspended.
> 
> Signed-off-by: Mike Anderson <andmike@linux.vnet.ibm.com>

I was thinking of doing then same because I have a need to test if a
table is suspended in the snapshot target.  I stopped short of doing so
but it is clear that it is better to put this generic DM export in place
rather than have each target add their own 'suspended' flag in
presuspend (or some equivalent).

Acked-by: Mike Snitzer <snitzer@redhat.com>

Re: [PATCH 0/2] Add suspended state check for target messages

[Alasdair G Kergon]

Would DMF_BLOCK_IO_FOR_SUSPEND work instead?

Alasdair

Re: [PATCH 0/2] Add suspended state check for target messages

[malahal]

Alasdair G Kergon [agk@redhat.com] wrote:
> Would DMF_BLOCK_IO_FOR_SUSPEND work instead?

Should, provided that multipath_message() and multipath_postsuspend()
use some lock to synchronize. Without the lock, multipath_postsuspend()
could be executed between multipath_message's bit flag check and
its action().

multipath_message()
{
	lock();
	if (DMF_BLOCK_IO_FOR_SUSPEND is set) {
		unlock and fail;
	} else { /* postsuspend has not been executed yet and won't be
	completed until we are done here */
		action();
	}
	unlock();
}

multipath_postsuspend()
{
	lock();
	flush work();
	unlock();
}

Re: [PATCH 2/2] dm: Add suspended check to multipath_message

[malahal]

Mike Anderson [andmike@linux.vnet.ibm.com] wrote:
> Add suspended check to multipath_message
> 
> Signed-off-by: Mike Anderson <andmike@linux.vnet.ibm.com>
> ---
>  drivers/md/dm-mpath.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> index 32d0b87..ee56ef7 100644
> --- a/drivers/md/dm-mpath.c
> +++ b/drivers/md/dm-mpath.c
> @@ -1402,6 +1402,9 @@ static int multipath_message(struct dm_target *ti, unsigned argc, char **argv)
>  	struct multipath *m = (struct multipath *) ti->private;
>  	action_fn action;
> 
> +	if (dm_table_md_suspended(ti->table))
> +		return -EBUSY;

Same question as DELETING. SUSPEND bit could be set just after the above
check. Is it OK if the following code executes after the SUSPEND bit is
set???

> +
>  	if (argc == 1) {
>  		if (!strnicmp(argv[0], MESG_STR("queue_if_no_path")))
>  			return queue_if_no_path(m, 1, 0);
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 0/2] Add suspended state check for target messages

[Mike Anderson]

Alasdair G Kergon <agk@redhat.com> wrote:
> Would DMF_BLOCK_IO_FOR_SUSPEND work instead?
> 

DMF_BLOCK_IO_FOR_SUSPEND appears to be a better choice, but since other
targets like dm-raid already have a internal suspend bit being used
unclear on the value of creating a generic accessor vs. just adding a
suspend atomic to the multipath target.

To close another window I believe we need a form of synchronization to
ensure a current multipath_message has completed prior to
multipath_postsuspend calling flush_multipath_work. Later
multipath_message calls would return without adding work if a bit check
indicated that we where suspended.

I will redo the patch(s) and resubmit.

-andmike
--
Michael Anderson
andmike@linux.vnet.ibm.com

Re: [PATCH 0/2] Add suspended state check for target messages

[Kiyoshi Ueda]

Hi Alasdair,

On 11/13/2009 11:19 PM +0900, Alasdair G Kergon wrote:
> Would DMF_BLOCK_IO_FOR_SUSPEND work instead?

Maybe.
But we must remember that using the flag means that multipath can't
use message during flushing I/Os, although I'm not sure how it hurts us.

If we'd like to keep the ability to use message during flushing I/Os,
we should use another flag for this purpose and set it after
dm_wait_for_completion().

Thanks,
Kiyoshi Ueda

Re: [PATCH 0/2] Add suspended state check for target messages

[Mike Anderson]

Kiyoshi Ueda <k-ueda@ct.jp.nec.com> wrote:
> Hi Alasdair,
> 
> On 11/13/2009 11:19 PM +0900, Alasdair G Kergon wrote:
> > Would DMF_BLOCK_IO_FOR_SUSPEND work instead?
> 
> Maybe.
> But we must remember that using the flag means that multipath can't
> use message during flushing I/Os, although I'm not sure how it hurts us.
> 
> If we'd like to keep the ability to use message during flushing I/Os,
> we should use another flag for this purpose and set it after
> dm_wait_for_completion().
> 

I just re-rolled / re-submitted a update series that use a multipath
internal flag for the suspended state plus a mutex. This would avoid a
extended meaning for DMF_BLOCK_IO_FOR_SUSPEND if we needed to avoid that
case.

-andmike
--
Michael Anderson
andmike@linux.vnet.ibm.com

Re: [PATCH 0/2] Add suspended state check for target messages

[Alasdair G Kergon]

On Mon, Nov 16, 2009 at 04:25:26PM +0900, Kiyoshi Ueda wrote:
> On 11/13/2009 11:19 PM +0900, Alasdair G Kergon wrote:
> > Would DMF_BLOCK_IO_FOR_SUSPEND work instead?
> If we'd like to keep the ability to use message during flushing I/Os,
> we should use another flag for this purpose and set it after
> dm_wait_for_completion().
 
We should rename the flag as it's original meaning has changed:-)
FOR_SUSPEND_OR_FLUSH

Alasdair