[PATCH 0/3] Add deleting state check for target messages

[PATCH 0/3] Add deleting state check for target messages

[Mike Anderson]

This patch adds a accessor function that allows determining if a
mapped_device is in the deleting state and then adds this check to the
target_message function. The result of this change would be that all
targets would not be allowed to process messages post a mapped device
entering the deleting state.

As previously described in the email at the archive url provided below the
target_message ioctl can result in new work / activities being started
post the mapped device entering states that new target work could cause
issues.

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

This patch depends on the previous patch that added dm_suspended_check due
to proximity of changes in common files.

---

Mike Anderson (3):
      dm: Add dm_deleting accesor function
      dm: Add accessor dm_table_md_deleting
      dm: Add deleting check to target_message


 drivers/md/dm-ioctl.c         |    6 ++++++
 drivers/md/dm-table.c         |    6 ++++++
 drivers/md/dm.c               |    5 +++++
 include/linux/device-mapper.h |    2 ++
 4 files changed, 19 insertions(+), 0 deletions(-)

--
-andmike

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

[PATCH 1/3] dm: Add dm_deleting accesor function

[Mike Anderson]

Add dm_deleting accesor function to allow checking of the DMF_DELETING bit.

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

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 724efc6..96127cc 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -2604,6 +2604,11 @@ int dm_suspended(struct mapped_device *md)
 	return test_bit(DMF_SUSPENDED, &md->flags);
 }
 
+int dm_deleting(struct mapped_device *md)
+{
+	return test_bit(DMF_DELETING, &md->flags);
+}
+
 int dm_noflush_suspending(struct dm_target *ti)
 {
 	struct mapped_device *md = dm_table_get_md(ti->table);
diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
index 3440c54..16cd291 100644
--- a/include/linux/device-mapper.h
+++ b/include/linux/device-mapper.h
@@ -236,6 +236,7 @@ const char *dm_device_name(struct mapped_device *md);
 int dm_copy_name_and_uuid(struct mapped_device *md, char *name, char *uuid);
 struct gendisk *dm_disk(struct mapped_device *md);
 int dm_suspended(struct mapped_device *md);
+int dm_deleting(struct mapped_device *md);
 int dm_noflush_suspending(struct dm_target *ti);
 union map_info *dm_get_mapinfo(struct bio *bio);
 union map_info *dm_get_rq_mapinfo(struct request *rq);

[PATCH 2/3] dm: Add accessor dm_table_md_deleting

[Mike Anderson]

Add a dm_table accessor function to check if md is being deleted.

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 6616598..fdf86fe 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1250,6 +1250,11 @@ int dm_table_md_suspended(struct dm_table *t)
 	return dm_suspended(t->md);
 }
 
+int dm_table_md_deleting(struct dm_table *t)
+{
+	return dm_deleting(t->md);
+}
+
 EXPORT_SYMBOL(dm_vcalloc);
 EXPORT_SYMBOL(dm_get_device);
 EXPORT_SYMBOL(dm_put_device);
@@ -1261,3 +1266,4 @@ EXPORT_SYMBOL(dm_table_put);
 EXPORT_SYMBOL(dm_table_get);
 EXPORT_SYMBOL(dm_table_unplug_all);
 EXPORT_SYMBOL(dm_table_md_suspended);
+EXPORT_SYMBOL(dm_table_md_deleting);
diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
index 16cd291..5a0fb6c 100644
--- a/include/linux/device-mapper.h
+++ b/include/linux/device-mapper.h
@@ -289,6 +289,7 @@ 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);
+int dm_table_md_deleting(struct dm_table *t);
 
 /*
  * Trigger an event.

[PATCH 3/3] dm: Add deleting check to target_message

[Mike Anderson]

Add dm_table_md_deleting check to target_message

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

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index a679429..04e252c 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1292,6 +1292,11 @@ static int target_message(struct dm_ioctl *param, size_t param_size)
 	if (!table)
 		goto out_argv;
 
+	if (dm_table_md_deleting(table)) {
+		r = -ENXIO;
+		goto out_table;
+	}
+
 	ti = dm_table_find_target(table, tmsg->sector);
 	if (!dm_target_is_valid(ti)) {
 		DMWARN("Target message sector outside device.");
@@ -1303,6 +1308,7 @@ static int target_message(struct dm_ioctl *param, size_t param_size)
 		r = -EINVAL;
 	}
 
+ out_table:
 	dm_table_put(table);
  out_argv:
 	kfree(argv);

Re: [PATCH 3/3] dm: Add deleting check to target_message

[malahal]

Mike Anderson [andmike@linux.vnet.ibm.com] wrote:
> Add dm_table_md_deleting check to target_message
> 
> Signed-off-by: Mike Anderson <andmike@linux.vnet.ibm.com>
> ---
>  drivers/md/dm-ioctl.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> index a679429..04e252c 100644
> --- a/drivers/md/dm-ioctl.c
> +++ b/drivers/md/dm-ioctl.c
> @@ -1292,6 +1292,11 @@ static int target_message(struct dm_ioctl *param, size_t param_size)
>  	if (!table)
>  		goto out_argv;
> 
> +	if (dm_table_md_deleting(table)) {
> +		r = -ENXIO;
> +		goto out_table;
> +	}

Is there some reason that DELETING bit won't be set while executing the
rest of the following code. If not, what is the point of checking the
bit above?

> 
>  	ti = dm_table_find_target(table, tmsg->sector);
>  	if (!dm_target_is_valid(ti)) {
>  		DMWARN("Target message sector outside device.");
> @@ -1303,6 +1308,7 @@ static int target_message(struct dm_ioctl *param, size_t param_size)
>  		r = -EINVAL;
>  	}
> 
> + out_table:
>  	dm_table_put(table);
>   out_argv:
>  	kfree(argv);
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 0/3] Add deleting state check for target messages

[Kiyoshi Ueda]

Hi Mike,

On 11/13/2009 05:04 PM +0900, Mike Anderson wrote:
> This patch adds a accessor function that allows determining if a
> mapped_device is in the deleting state and then adds this check to the
> target_message function. The result of this change would be that all
> targets would not be allowed to process messages post a mapped device
> entering the deleting state.
> 
> As previously described in the email at the archive url provided below the
> target_message ioctl can result in new work / activities being started
> post the mapped device entering states that new target work could cause
> issues.
> 
> http://permalink.gmane.org/gmane.linux.kernel.device-mapper.devel/10486

Thank you for the patch-set.
Although I'm not sure this patch-set is really needed after the fix
of message ioctl during suspend will have been done, I understand
why you want this patch-set (since issuing message ioctl to a deleting
device is a bit pointless).

But there is a small window in this patch-set as Malahal described.
Also, I have some comments below:
  o Please use drivers/md/dm.h instead of include/linux/device-mapper.h
    and don't export symbols, because all accessors you are adding are
    used in only dm-core, no target uses them.

  o target_message() can use dm_deleting(), because target_message()
    has a reference to the md.
    We don't need the table function, dm_table_md_deleting(), at all.

Thanks,
Kiyoshi Ueda

Re: [PATCH 0/3] Add deleting state check for target messages

[Mike Anderson]

Kiyoshi Ueda <k-ueda@ct.jp.nec.com> wrote:
> Hi Mike,
> 
> On 11/13/2009 05:04 PM +0900, Mike Anderson wrote:
> > This patch adds a accessor function that allows determining if a
> > mapped_device is in the deleting state and then adds this check to the
> > target_message function. The result of this change would be that all
> > targets would not be allowed to process messages post a mapped device
> > entering the deleting state.
> > 
> > As previously described in the email at the archive url provided below the
> > target_message ioctl can result in new work / activities being started
> > post the mapped device entering states that new target work could cause
> > issues.
> > 
> > http://permalink.gmane.org/gmane.linux.kernel.device-mapper.devel/10486
> 
> Thank you for the patch-set.
> Although I'm not sure this patch-set is really needed after the fix
> of message ioctl during suspend will have been done, I understand
> why you want this patch-set (since issuing message ioctl to a deleting
> device is a bit pointless).
> 

Yes this patch set should not be needed post the fix to the message ioctl.
The reduction was aimed at future callers using dm_get_md prior to
DMF_FREEING being set as the dm_table_destroy holders wait would not
proceed until current message operations had completed, but the reduction
of this case is not that important with the other message ioctl changes in
place.

This patch set can be dropped.

> But there is a small window in this patch-set as Malahal described.
> Also, I have some comments below:
>   o Please use drivers/md/dm.h instead of include/linux/device-mapper.h
>     and don't export symbols, because all accessors you are adding are
>     used in only dm-core, no target uses them.
> 
>   o target_message() can use dm_deleting(), because target_message()
>     has a reference to the md.
>     We don't need the table function, dm_table_md_deleting(), at all.
>

Thanks for the review tip.


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

Re: [PATCH 0/3] Add deleting state check for target messages

[Alasdair G Kergon]

On Sun, Nov 15, 2009 at 11:38:48PM -0800, Mike Anderson wrote:
> Yes this patch set should not be needed post the fix to the message ioctl.
> The reduction was aimed at future callers using dm_get_md prior to
> DMF_FREEING being set as the dm_table_destroy holders wait would not
> proceed until current message operations had completed, but the reduction
> of this case is not that important with the other message ioctl changes in
> place.
 
I've left it in my tree for now (with the suggested simplifications - private
to dm.h), to enforce the situation that the dm core will not forward messages
to table targets after they begin to be deleted.

Alasdair