[PATCH v2] dm ioctl: allow change device target type to error

[PATCH v2] dm ioctl: allow change device target type to error

[Joe Jin]

commit a5664da "dm ioctl: make bio or request based device type immutable"
prevented "dmsetup wape_table" change the target type to "error".

-v2: setup md->queue even target type is "error".

Signed-off-by: Joe Jin <joe.jin@oracle.com>
---
 drivers/md/dm-ioctl.c |  4 ++++
 drivers/md/dm-table.c | 12 ++++++++++++
 drivers/md/dm.h       |  1 +
 3 files changed, 17 insertions(+)

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index f1b7586..2a9b63d 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1280,6 +1280,9 @@ static int table_load(struct dm_ioctl *param, size_t param_size)
 		goto out;
 	}
 
+	if (dm_is_error_target(t))
+		goto error_target;
+
 	/* Protect md->type and md->queue against concurrent table loads. */
 	dm_lock_md_type(md);
 	if (dm_get_md_type(md) == DM_TYPE_NONE)
@@ -1293,6 +1296,7 @@ static int table_load(struct dm_ioctl *param, size_t param_size)
 		goto out;
 	}
 
+error_target:
 	/* setup md->queue to reflect md's type (may block) */
 	r = dm_setup_md_queue(md);
 	if (r) {
diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index f221812..27be46a 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -184,6 +184,18 @@ static int alloc_targets(struct dm_table *t, unsigned int num)
 	return 0;
 }
 
+bool dm_is_error_target(struct dm_table *t)
+{
+	unsigned i;
+
+	for (i = 0; i < t->num_targets; i++) {
+		struct dm_target *tgt = t->targets + i;
+		if (strcmp(tgt->type->name, "error") == 0)
+			return true;
+	}
+	return false;
+}
+
 int dm_table_create(struct dm_table **result, fmode_t mode,
 		    unsigned num_targets, struct mapped_device *md)
 {
diff --git a/drivers/md/dm.h b/drivers/md/dm.h
index 45b97da..c7bceeb 100644
--- a/drivers/md/dm.h
+++ b/drivers/md/dm.h
@@ -69,6 +69,7 @@ unsigned dm_table_get_type(struct dm_table *t);
 struct target_type *dm_table_get_immutable_target_type(struct dm_table *t);
 bool dm_table_request_based(struct dm_table *t);
 bool dm_table_supports_discards(struct dm_table *t);
+bool dm_is_error_target(struct dm_table *t);
 int dm_table_alloc_md_mempools(struct dm_table *t);
 void dm_table_free_md_mempools(struct dm_table *t);
 struct dm_md_mempools *dm_table_get_md_mempools(struct dm_table *t);
-- 
1.8.3.1

Re: [dm-devel] [PATCH v2] dm ioctl: allow change device target type to error

[Mikulas Patocka]

On Wed, 21 Aug 2013, Joe Jin wrote:

> commit a5664da "dm ioctl: make bio or request based device type immutable"
> prevented "dmsetup wape_table" change the target type to "error".

That commit a5664da is there for a reason (it is not possible to change 
bio-based device to request-based and vice versa) and I don't really see 
how this patch is supposed to work.

If there are bios that are in flight and that already passed through 
blk_queue_bio, and you change the device from request-based to bio-based, 
what are you going to do with them? - The patch doesn't do anything about 
it.

A better approach would be to create a new request-based target "error-rq" 
and change the multipath target to "error-rq" target. That way, you don't 
have to change device type from request based to bio based.

Mikulas

> -v2: setup md->queue even target type is "error".
> 
> Signed-off-by: Joe Jin <joe.jin@oracle.com>
> ---
>  drivers/md/dm-ioctl.c |  4 ++++
>  drivers/md/dm-table.c | 12 ++++++++++++
>  drivers/md/dm.h       |  1 +
>  3 files changed, 17 insertions(+)
> 
> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> index f1b7586..2a9b63d 100644
> --- a/drivers/md/dm-ioctl.c
> +++ b/drivers/md/dm-ioctl.c
> @@ -1280,6 +1280,9 @@ static int table_load(struct dm_ioctl *param, size_t param_size)
>  		goto out;
>  	}
>  
> +	if (dm_is_error_target(t))
> +		goto error_target;
> +
>  	/* Protect md->type and md->queue against concurrent table loads. */
>  	dm_lock_md_type(md);
>  	if (dm_get_md_type(md) == DM_TYPE_NONE)
> @@ -1293,6 +1296,7 @@ static int table_load(struct dm_ioctl *param, size_t param_size)
>  		goto out;
>  	}
>  
> +error_target:
>  	/* setup md->queue to reflect md's type (may block) */
>  	r = dm_setup_md_queue(md);
>  	if (r) {
> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index f221812..27be46a 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -184,6 +184,18 @@ static int alloc_targets(struct dm_table *t, unsigned int num)
>  	return 0;
>  }
>  
> +bool dm_is_error_target(struct dm_table *t)
> +{
> +	unsigned i;
> +
> +	for (i = 0; i < t->num_targets; i++) {
> +		struct dm_target *tgt = t->targets + i;
> +		if (strcmp(tgt->type->name, "error") == 0)
> +			return true;
> +	}
> +	return false;
> +}
> +
>  int dm_table_create(struct dm_table **result, fmode_t mode,
>  		    unsigned num_targets, struct mapped_device *md)
>  {
> diff --git a/drivers/md/dm.h b/drivers/md/dm.h
> index 45b97da..c7bceeb 100644
> --- a/drivers/md/dm.h
> +++ b/drivers/md/dm.h
> @@ -69,6 +69,7 @@ unsigned dm_table_get_type(struct dm_table *t);
>  struct target_type *dm_table_get_immutable_target_type(struct dm_table *t);
>  bool dm_table_request_based(struct dm_table *t);
>  bool dm_table_supports_discards(struct dm_table *t);
> +bool dm_is_error_target(struct dm_table *t);
>  int dm_table_alloc_md_mempools(struct dm_table *t);
>  void dm_table_free_md_mempools(struct dm_table *t);
>  struct dm_md_mempools *dm_table_get_md_mempools(struct dm_table *t);
> -- 
> 1.8.3.1
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
> 

Re: [PATCH v2] dm ioctl: allow change device target type to error

[Mike Snitzer]

On Wed, Aug 21 2013 at 10:48am -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> 
> 
> On Wed, 21 Aug 2013, Joe Jin wrote:
> 
> > commit a5664da "dm ioctl: make bio or request based device type immutable"
> > prevented "dmsetup wape_table" change the target type to "error".
> 
> That commit a5664da is there for a reason (it is not possible to change 
> bio-based device to request-based and vice versa) and I don't really see 
> how this patch is supposed to work.
> 
> If there are bios that are in flight and that already passed through 
> blk_queue_bio, and you change the device from request-based to bio-based, 
> what are you going to do with them? - The patch doesn't do anything about 
> it.
> 
> A better approach would be to create a new request-based target "error-rq" 
> and change the multipath target to "error-rq" target. That way, you don't 
> have to change device type from request based to bio based.

My thoughts _exactly_.  This patch is very confused.

Joe, what are you looking to be able to do?  Switch a dm-multipath
device to error?  Or allowing switching a target that has
DM_TARGET_IMMUTABLE flag set to be switched to error target?

The latter restriction was introduced with commit 36a0456fb ("dm table:
add immutable feature").

Mike

Re: [PATCH v2] dm ioctl: allow change device target type to error

[Joe Jin]

On 08/21/13 23:06, Mike Snitzer wrote:
> On Wed, Aug 21 2013 at 10:48am -0400,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
>>
>>
>> On Wed, 21 Aug 2013, Joe Jin wrote:
>>
>>> commit a5664da "dm ioctl: make bio or request based device type immutable"
>>> prevented "dmsetup wape_table" change the target type to "error".
>>
>> That commit a5664da is there for a reason (it is not possible to change 
>> bio-based device to request-based and vice versa) and I don't really see 
>> how this patch is supposed to work.
>>
>> If there are bios that are in flight and that already passed through 
>> blk_queue_bio, and you change the device from request-based to bio-based, 
>> what are you going to do with them? - The patch doesn't do anything about 
>> it.
>>
>> A better approach would be to create a new request-based target "error-rq" 
>> and change the multipath target to "error-rq" target. That way, you don't 
>> have to change device type from request based to bio based.
> 
> My thoughts _exactly_.  This patch is very confused.
> 
> Joe, what are you looking to be able to do?  Switch a dm-multipath
> device to error?  Or allowing switching a target that has
> DM_TARGET_IMMUTABLE flag set to be switched to error target?
> 
> The latter restriction was introduced with commit 36a0456fb ("dm table:
> add immutable feature").

Hi Mike,

So far dmsetup support wipe_table:
https://bugzilla.redhat.com/show_bug.cgi?id=742607
As description in the bug Doc Text, "This could be useful, for example, 
if a long-running process keeps a device open after it has finished using
it and you need to release the underlying devices before that process exits."

After apply the commit, wipe_table no long works.

Thanks,
Joe

Re: [dm-devel] [PATCH v2] dm ioctl: allow change device target type to error

[Joe Jin]

Mikulas, thanks for you suggestions, I create new patch, can you please help
review?

Subject: dm: add map_rq define for error

commit a5664da "dm ioctl: make bio or request based device type immutable"
prevented "dmsetup wape_table" change the target type to "error" for there
is not map_rq for error target type.

Signed-off-by: Joe Jin <joe.jin@oracle.com>
---
 drivers/md/dm-target.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/md/dm-target.c b/drivers/md/dm-target.c
index 37ba5db..b690910 100644
--- a/drivers/md/dm-target.c
+++ b/drivers/md/dm-target.c
@@ -131,12 +131,19 @@ static int io_err_map(struct dm_target *tt, struct bio *bio)
 	return -EIO;
 }
 
+static int io_err_map_rq(struct dm_target *ti, struct request *clone,
+			 union map_info *map_context)
+{
+	return -EIO;
+}
+
 static struct target_type error_target = {
 	.name = "error",
 	.version = {1, 1, 0},
 	.ctr  = io_err_ctr,
 	.dtr  = io_err_dtr,
 	.map  = io_err_map,
+	.map_rq = io_err_map_rq,
 };
 
 int __init dm_target_init(void)
-- 
1.8.3.1

On 08/21/13 22:48, Mikulas Patocka wrote:
> 
> 
> On Wed, 21 Aug 2013, Joe Jin wrote:
> 
>> commit a5664da "dm ioctl: make bio or request based device type immutable"
>> prevented "dmsetup wape_table" change the target type to "error".
> 
> That commit a5664da is there for a reason (it is not possible to change 
> bio-based device to request-based and vice versa) and I don't really see 
> how this patch is supposed to work.
> 
> If there are bios that are in flight and that already passed through 
> blk_queue_bio, and you change the device from request-based to bio-based, 
> what are you going to do with them? - The patch doesn't do anything about 
> it.
> 
> A better approach would be to create a new request-based target "error-rq" 
> and change the multipath target to "error-rq" target. That way, you don't 
> have to change device type from request based to bio based.
> 
> Mikulas
> 
>> -v2: setup md->queue even target type is "error".
>>
>> Signed-off-by: Joe Jin <joe.jin@oracle.com>
>> ---
>>  drivers/md/dm-ioctl.c |  4 ++++
>>  drivers/md/dm-table.c | 12 ++++++++++++
>>  drivers/md/dm.h       |  1 +
>>  3 files changed, 17 insertions(+)
>>
>> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
>> index f1b7586..2a9b63d 100644
>> --- a/drivers/md/dm-ioctl.c
>> +++ b/drivers/md/dm-ioctl.c
>> @@ -1280,6 +1280,9 @@ static int table_load(struct dm_ioctl *param, size_t param_size)
>>  		goto out;
>>  	}
>>  
>> +	if (dm_is_error_target(t))
>> +		goto error_target;
>> +
>>  	/* Protect md->type and md->queue against concurrent table loads. */
>>  	dm_lock_md_type(md);
>>  	if (dm_get_md_type(md) == DM_TYPE_NONE)
>> @@ -1293,6 +1296,7 @@ static int table_load(struct dm_ioctl *param, size_t param_size)
>>  		goto out;
>>  	}
>>  
>> +error_target:
>>  	/* setup md->queue to reflect md's type (may block) */
>>  	r = dm_setup_md_queue(md);
>>  	if (r) {
>> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
>> index f221812..27be46a 100644
>> --- a/drivers/md/dm-table.c
>> +++ b/drivers/md/dm-table.c
>> @@ -184,6 +184,18 @@ static int alloc_targets(struct dm_table *t, unsigned int num)
>>  	return 0;
>>  }
>>  
>> +bool dm_is_error_target(struct dm_table *t)
>> +{
>> +	unsigned i;
>> +
>> +	for (i = 0; i < t->num_targets; i++) {
>> +		struct dm_target *tgt = t->targets + i;
>> +		if (strcmp(tgt->type->name, "error") == 0)
>> +			return true;
>> +	}
>> +	return false;
>> +}
>> +
>>  int dm_table_create(struct dm_table **result, fmode_t mode,
>>  		    unsigned num_targets, struct mapped_device *md)
>>  {
>> diff --git a/drivers/md/dm.h b/drivers/md/dm.h
>> index 45b97da..c7bceeb 100644
>> --- a/drivers/md/dm.h
>> +++ b/drivers/md/dm.h
>> @@ -69,6 +69,7 @@ unsigned dm_table_get_type(struct dm_table *t);
>>  struct target_type *dm_table_get_immutable_target_type(struct dm_table *t);
>>  bool dm_table_request_based(struct dm_table *t);
>>  bool dm_table_supports_discards(struct dm_table *t);
>> +bool dm_is_error_target(struct dm_table *t);
>>  int dm_table_alloc_md_mempools(struct dm_table *t);
>>  void dm_table_free_md_mempools(struct dm_table *t);
>>  struct dm_md_mempools *dm_table_get_md_mempools(struct dm_table *t);
>> -- 
>> 1.8.3.1
>>
>> --
>> dm-devel mailing list
>> dm-devel@redhat.com
>> https://www.redhat.com/mailman/listinfo/dm-devel
>>

Re: [PATCH v2] dm ioctl: allow change device target type to error

[Mike Snitzer]

On Wed, Aug 21 2013 at 10:40pm -0400,
Joe Jin <joe.jin@oracle.com> wrote:

> On 08/21/13 23:06, Mike Snitzer wrote:
> > On Wed, Aug 21 2013 at 10:48am -0400,
> > Mikulas Patocka <mpatocka@redhat.com> wrote:
> > 
> >>
> >>
> >> On Wed, 21 Aug 2013, Joe Jin wrote:
> >>
> >>> commit a5664da "dm ioctl: make bio or request based device type immutable"
> >>> prevented "dmsetup wape_table" change the target type to "error".
> >>
> >> That commit a5664da is there for a reason (it is not possible to change 
> >> bio-based device to request-based and vice versa) and I don't really see 
> >> how this patch is supposed to work.
> >>
> >> If there are bios that are in flight and that already passed through 
> >> blk_queue_bio, and you change the device from request-based to bio-based, 
> >> what are you going to do with them? - The patch doesn't do anything about 
> >> it.
> >>
> >> A better approach would be to create a new request-based target "error-rq" 
> >> and change the multipath target to "error-rq" target. That way, you don't 
> >> have to change device type from request based to bio based.
> > 
> > My thoughts _exactly_.  This patch is very confused.
> > 
> > Joe, what are you looking to be able to do?  Switch a dm-multipath
> > device to error?  Or allowing switching a target that has
> > DM_TARGET_IMMUTABLE flag set to be switched to error target?
> > 
> > The latter restriction was introduced with commit 36a0456fb ("dm table:
> > add immutable feature").
> 
> Hi Mike,
> 
> So far dmsetup support wipe_table:
> https://bugzilla.redhat.com/show_bug.cgi?id=742607
> As description in the bug Doc Text, "This could be useful, for example, 
> if a long-running process keeps a device open after it has finished using
> it and you need to release the underlying devices before that process exits."
> 
> After apply the commit, wipe_table no long works.

Well, it never _really_ worked even before that commit because it was
switching from request-based to bio-based.  Some queued requests
could've easily slipped through the cracks.

But I now understand what it is you want to be able to do.

Re: [PATCH v2] dm ioctl: allow change device target type to error

[Mike Snitzer]

On Wed, Aug 21 2013 at 10:50pm -0400,
Joe Jin <joe.jin@oracle.com> wrote:

> Mikulas, thanks for you suggestions, I create new patch, can you please help
> review?
> 
> Subject: dm: add map_rq define for error
> 
> commit a5664da "dm ioctl: make bio or request based device type immutable"
> prevented "dmsetup wape_table" change the target type to "error" for there
> is not map_rq for error target type.
> 
> Signed-off-by: Joe Jin <joe.jin@oracle.com>
> ---
>  drivers/md/dm-target.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/md/dm-target.c b/drivers/md/dm-target.c
> index 37ba5db..b690910 100644
> --- a/drivers/md/dm-target.c
> +++ b/drivers/md/dm-target.c
> @@ -131,12 +131,19 @@ static int io_err_map(struct dm_target *tt, struct bio *bio)
>  	return -EIO;
>  }
>  
> +static int io_err_map_rq(struct dm_target *ti, struct request *clone,
> +			 union map_info *map_context)
> +{
> +	return -EIO;
> +}
> +
>  static struct target_type error_target = {
>  	.name = "error",
>  	.version = {1, 1, 0},
>  	.ctr  = io_err_ctr,
>  	.dtr  = io_err_dtr,
>  	.map  = io_err_map,
> +	.map_rq = io_err_map_rq,
>  };
>  
>  int __init dm_target_init(void)
> -- 
> 1.8.3.1

Hi Joe,

Unfortunately this isn't going to work properly.  Mikulas suggested a
new "error-rq" target.

I do like the idea of a single error target that is hybrid (supports
both bio-based and request-based) but the DM core would need to be
updated to support this.

Specifically, we'd need to check if the device (and active table) is
already bio-based or request-based and select the appropriate type.  If
it is a new device, default to selecting bio-based.

There are some wrappers and other logic thoughout DM core that will need
auditing too.

Mike

[PATCH] dm: allow error target to replace either bio-based and request-based targets

[Mike Snitzer]

On Thu, Aug 22 2013 at  4:19pm -0400,
Mike Snitzer <snitzer@redhat.com> wrote:

> Hi Joe,
> 
> Unfortunately this isn't going to work properly.  Mikulas suggested a
> new "error-rq" target.
> 
> I do like the idea of a single error target that is hybrid (supports
> both bio-based and request-based) but the DM core would need to be
> updated to support this.
> 
> Specifically, we'd need to check if the device (and active table) is
> already bio-based or request-based and select the appropriate type.  If
> it is a new device, default to selecting bio-based.
> 
> There are some wrappers and other logic thoughout DM core that will need
> auditing too.

Here is a patch that should work for your needs (I tested it to work
with 'dmsetup wipe_table' on both request-based and bio-based devices):

From: Mike Snitzer <snitzer@redhat.com>
Date: Thu, 22 Aug 2013 18:21:38 -0400
Subject: [PATCH] dm: allow error target to replace either bio-based and request-based targets

In may be useful to switch a request-based table to the "error" target.
Enhance the DM core to allow a single hybrid target to be capable of
handling either bios or requests.

Add a request-based (.map_rq) member to the error target_type and train
dm_table_set_type() to prefer the md's established type (request-based
or bio-based).  If the md doesn't have an established type default to
making the hybrid target bio-based.

Cc: Joe Jin <joe.jin@oracle.com>
Cc: Mikulas Patocka <mpatocka@redhat.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-table.c  |   18 +++++++++++++++++-
 drivers/md/dm-target.c |    9 ++++++++-
 drivers/md/dm.h        |   11 +++++++++++
 3 files changed, 36 insertions(+), 2 deletions(-)

Index: linux/drivers/md/dm-table.c
===================================================================
--- linux.orig/drivers/md/dm-table.c
+++ linux/drivers/md/dm-table.c
@@ -864,10 +864,26 @@ static int dm_table_set_type(struct dm_t
 	struct dm_target *tgt;
 	struct dm_dev_internal *dd;
 	struct list_head *devices;
+	unsigned live_md_type;
+
+	dm_lock_md_type(t->md);
+	live_md_type = dm_get_md_type(t->md);
+	dm_unlock_md_type(t->md);
 
 	for (i = 0; i < t->num_targets; i++) {
 		tgt = t->targets + i;
-		if (dm_target_request_based(tgt))
+		if (dm_target_hybrid(tgt)) {
+			switch (live_md_type) {
+			case DM_TYPE_NONE:
+			case DM_TYPE_BIO_BASED:
+				bio_based = 1;
+				break;
+			case DM_TYPE_REQUEST_BASED:
+				request_based = 1;
+				break;
+			}
+		}
+		else if (dm_target_request_based(tgt))
 			request_based = 1;
 		else
 			bio_based = 1;
Index: linux/drivers/md/dm-target.c
===================================================================
--- linux.orig/drivers/md/dm-target.c
+++ linux/drivers/md/dm-target.c
@@ -131,12 +131,19 @@ static int io_err_map(struct dm_target *
 	return -EIO;
 }
 
+static int io_err_map_rq(struct dm_target *ti, struct request *clone,
+			 union map_info *map_context)
+{
+	return -EIO;
+}
+
 static struct target_type error_target = {
 	.name = "error",
-	.version = {1, 1, 0},
+	.version = {1, 2, 0},
 	.ctr  = io_err_ctr,
 	.dtr  = io_err_dtr,
 	.map  = io_err_map,
+	.map_rq = io_err_map_rq,
 };
 
 int __init dm_target_init(void)
Index: linux/drivers/md/dm.h
===================================================================
--- linux.orig/drivers/md/dm.h
+++ linux/drivers/md/dm.h
@@ -91,10 +91,21 @@ int dm_setup_md_queue(struct mapped_devi
 #define dm_target_is_valid(t) ((t)->table)
 
 /*
+ * To check whether the target type is bio-based or not (request-based).
+ */
+#define dm_target_bio_based(t) ((t)->type->map != NULL)
+
+/*
  * To check whether the target type is request-based or not (bio-based).
  */
 #define dm_target_request_based(t) ((t)->type->map_rq != NULL)
 
+/*
+ * To check whether the target type is a hybrid (capable of being
+ * either request-based or bio-based).
+ */
+#define dm_target_hybrid(t) (dm_target_bio_based(t) && dm_target_request_based(t))
+
 /*-----------------------------------------------------------------
  * A registry of target types.
  *---------------------------------------------------------------*/

Re: [PATCH] dm: allow error target to replace either bio-based and request-based targets

[Joe Jin]

On 08/23/13 08:17, Mike Snitzer wrote:
> Here is a patch that should work for your needs (I tested it to work
> with 'dmsetup wipe_table' on both request-based and bio-based devices):

This really what I looking for, thanks!

> 
> From: Mike Snitzer <snitzer@redhat.com>
> Date: Thu, 22 Aug 2013 18:21:38 -0400
> Subject: [PATCH] dm: allow error target to replace either bio-based and request-based targets
> 
> In may be useful to switch a request-based table to the "error" target.
> Enhance the DM core to allow a single hybrid target to be capable of
> handling either bios or requests.
> 
> Add a request-based (.map_rq) member to the error target_type and train
> dm_table_set_type() to prefer the md's established type (request-based
> or bio-based).  If the md doesn't have an established type default to
> making the hybrid target bio-based.

Signed-off-by: Joe Jin <joe.jin@oracle.com>

Thanks,
Joe

Re: [dm-devel] [PATCH] dm: allow error target to replace either bio-based and request-based targets

[Jun'ichi Nomura]

Hello Mike,

On 08/23/13 09:17, Mike Snitzer wrote:
>> I do like the idea of a single error target that is hybrid (supports
>> both bio-based and request-based) but the DM core would need to be
>> updated to support this.
>>
>> Specifically, we'd need to check if the device (and active table) is
>> already bio-based or request-based and select the appropriate type.  If
>> it is a new device, default to selecting bio-based.
>>
>> There are some wrappers and other logic thoughout DM core that will need
>> auditing too.
> 
> Here is a patch that should work for your needs (I tested it to work
> with 'dmsetup wipe_table' on both request-based and bio-based devices):

How about moving the default handling in dm_table_set_type() outside of
the for-each-target loop, like the modified patch below?

For example, if a table has 2 targets, hybrid and request_based,
and live_md_type is DM_TYPE_NONE, the table should be considered as
request_based, not inconsistent.
Though the end result is same as such a table is rejected by other
constraint anyway, I think it's good to keep the semantics clean
and error messages consistent.

I.e. for the above case, the error message should be
"Request-based dm doesn't support multiple targets yet",
not "Inconsistent table: different target types can't be mixed up".

---
Jun'ichi Nomura, NEC Corporation


diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index f221812..6e683c8 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -860,14 +860,16 @@ EXPORT_SYMBOL(dm_consume_args);
 static int dm_table_set_type(struct dm_table *t)
 {
 	unsigned i;
-	unsigned bio_based = 0, request_based = 0;
+	unsigned bio_based = 0, request_based = 0, hybrid = 0;
 	struct dm_target *tgt;
 	struct dm_dev_internal *dd;
 	struct list_head *devices;
 
 	for (i = 0; i < t->num_targets; i++) {
 		tgt = t->targets + i;
-		if (dm_target_request_based(tgt))
+		if (dm_target_hybrid(tgt))
+			hybrid = 1;
+		else if (dm_target_request_based(tgt))
 			request_based = 1;
 		else
 			bio_based = 1;
@@ -879,6 +881,25 @@ static int dm_table_set_type(struct dm_table *t)
 		}
 	}
 
+	if (hybrid && !bio_based && !request_based) {
+		/*
+		 * The targets can work either way.
+		 * Determine the type from the live device.
+		 */
+		unsigned live_md_type;
+		dm_lock_md_type(t->md);
+		live_md_type = dm_get_md_type(t->md);
+		dm_unlock_md_type(t->md);
+		switch (live_md_type) {
+		case DM_TYPE_REQUEST_BASED:
+			request_based = 1;
+			break;
+		default:
+			bio_based = 1;
+			break;
+		}
+	}
+
 	if (bio_based) {
 		/* We must use this table as bio-based */
 		t->type = DM_TYPE_BIO_BASED;
diff --git a/drivers/md/dm-target.c b/drivers/md/dm-target.c
index 37ba5db..242e3ce 100644
--- a/drivers/md/dm-target.c
+++ b/drivers/md/dm-target.c
@@ -131,12 +131,19 @@ static int io_err_map(struct dm_target *tt, struct bio *bio)
 	return -EIO;
 }
 
+static int io_err_map_rq(struct dm_target *ti, struct request *clone,
+			 union map_info *map_context)
+{
+	return -EIO;
+}
+
 static struct target_type error_target = {
 	.name = "error",
-	.version = {1, 1, 0},
+	.version = {1, 2, 0},
 	.ctr  = io_err_ctr,
 	.dtr  = io_err_dtr,
 	.map  = io_err_map,
+	.map_rq = io_err_map_rq,
 };
 
 int __init dm_target_init(void)
diff --git a/drivers/md/dm.h b/drivers/md/dm.h
index 45b97da..8b4c075 100644
--- a/drivers/md/dm.h
+++ b/drivers/md/dm.h
@@ -89,10 +89,21 @@ int dm_setup_md_queue(struct mapped_device *md);
 #define dm_target_is_valid(t) ((t)->table)
 
 /*
+ * To check whether the target type is bio-based or not (request-based).
+ */
+#define dm_target_bio_based(t) ((t)->type->map != NULL)
+
+/*
  * To check whether the target type is request-based or not (bio-based).
  */
 #define dm_target_request_based(t) ((t)->type->map_rq != NULL)
 
+/*
+ * To check whether the target type is a hybrid (capable of being
+ * either request-based or bio-based).
+ */
+#define dm_target_hybrid(t) (dm_target_bio_based(t) && dm_target_request_based(t))
+
 /*-----------------------------------------------------------------
  * A registry of target types.
  *---------------------------------------------------------------*/

Re: [PATCH] dm: allow error target to replace either bio-based and request-based targets

[Jun'ichi Nomura]

Hello Mike,

On 08/23/13 09:17, Mike Snitzer wrote:
>> I do like the idea of a single error target that is hybrid (supports
>> both bio-based and request-based) but the DM core would need to be
>> updated to support this.
>>
>> Specifically, we'd need to check if the device (and active table) is
>> already bio-based or request-based and select the appropriate type.  If
>> it is a new device, default to selecting bio-based.
>>
>> There are some wrappers and other logic thoughout DM core that will need
>> auditing too.
> 
> Here is a patch that should work for your needs (I tested it to work
> with 'dmsetup wipe_table' on both request-based and bio-based devices):

How about moving the default handling in dm_table_set_type() outside of
the for-each-target loop, like the modified patch below?

For example, if a table has 2 targets, hybrid and request_based,
and live_md_type is DM_TYPE_NONE, the table should be considered as
request_based, not inconsistent.
Though the end result is same as such a table is rejected by other
constraint anyway, I think it's good to keep the semantics clean
and error messages consistent.

I.e. for the above case, the error message should be
"Request-based dm doesn't support multiple targets yet",
not "Inconsistent table: different target types can't be mixed up".

---
Jun'ichi Nomura, NEC Corporation


diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index f221812..6e683c8 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -860,14 +860,16 @@ EXPORT_SYMBOL(dm_consume_args);
 static int dm_table_set_type(struct dm_table *t)
 {
 	unsigned i;
-	unsigned bio_based = 0, request_based = 0;
+	unsigned bio_based = 0, request_based = 0, hybrid = 0;
 	struct dm_target *tgt;
 	struct dm_dev_internal *dd;
 	struct list_head *devices;
 
 	for (i = 0; i < t->num_targets; i++) {
 		tgt = t->targets + i;
-		if (dm_target_request_based(tgt))
+		if (dm_target_hybrid(tgt))
+			hybrid = 1;
+		else if (dm_target_request_based(tgt))
 			request_based = 1;
 		else
 			bio_based = 1;
@@ -879,6 +881,25 @@ static int dm_table_set_type(struct dm_table *t)
 		}
 	}
 
+	if (hybrid && !bio_based && !request_based) {
+		/*
+		 * The targets can work either way.
+		 * Determine the type from the live device.
+		 */
+		unsigned live_md_type;
+		dm_lock_md_type(t->md);
+		live_md_type = dm_get_md_type(t->md);
+		dm_unlock_md_type(t->md);
+		switch (live_md_type) {
+		case DM_TYPE_REQUEST_BASED:
+			request_based = 1;
+			break;
+		default:
+			bio_based = 1;
+			break;
+		}
+	}
+
 	if (bio_based) {
 		/* We must use this table as bio-based */
 		t->type = DM_TYPE_BIO_BASED;
diff --git a/drivers/md/dm-target.c b/drivers/md/dm-target.c
index 37ba5db..242e3ce 100644
--- a/drivers/md/dm-target.c
+++ b/drivers/md/dm-target.c
@@ -131,12 +131,19 @@ static int io_err_map(struct dm_target *tt, struct bio *bio)
 	return -EIO;
 }
 
+static int io_err_map_rq(struct dm_target *ti, struct request *clone,
+			 union map_info *map_context)
+{
+	return -EIO;
+}
+
 static struct target_type error_target = {
 	.name = "error",
-	.version = {1, 1, 0},
+	.version = {1, 2, 0},
 	.ctr  = io_err_ctr,
 	.dtr  = io_err_dtr,
 	.map  = io_err_map,
+	.map_rq = io_err_map_rq,
 };
 
 int __init dm_target_init(void)
diff --git a/drivers/md/dm.h b/drivers/md/dm.h
index 45b97da..8b4c075 100644
--- a/drivers/md/dm.h
+++ b/drivers/md/dm.h
@@ -89,10 +89,21 @@ int dm_setup_md_queue(struct mapped_device *md);
 #define dm_target_is_valid(t) ((t)->table)
 
 /*
+ * To check whether the target type is bio-based or not (request-based).
+ */
+#define dm_target_bio_based(t) ((t)->type->map != NULL)
+
+/*
  * To check whether the target type is request-based or not (bio-based).
  */
 #define dm_target_request_based(t) ((t)->type->map_rq != NULL)
 
+/*
+ * To check whether the target type is a hybrid (capable of being
+ * either request-based or bio-based).
+ */
+#define dm_target_hybrid(t) (dm_target_bio_based(t) && dm_target_request_based(t))
+
 /*-----------------------------------------------------------------
  * A registry of target types.
  *---------------------------------------------------------------*/

Re: dm: allow error target to replace either bio-based and request-based targets

[Mike Snitzer]

On Thu, Aug 22 2013 at 11:14pm -0400,
Jun'ichi Nomura <j-nomura@ce.jp.nec.com> wrote:

> Hello Mike,
> 
> On 08/23/13 09:17, Mike Snitzer wrote:
> >> I do like the idea of a single error target that is hybrid (supports
> >> both bio-based and request-based) but the DM core would need to be
> >> updated to support this.
> >>
> >> Specifically, we'd need to check if the device (and active table) is
> >> already bio-based or request-based and select the appropriate type.  If
> >> it is a new device, default to selecting bio-based.
> >>
> >> There are some wrappers and other logic thoughout DM core that will need
> >> auditing too.
> > 
> > Here is a patch that should work for your needs (I tested it to work
> > with 'dmsetup wipe_table' on both request-based and bio-based devices):
> 
> How about moving the default handling in dm_table_set_type() outside of
> the for-each-target loop, like the modified patch below?
> 
> For example, if a table has 2 targets, hybrid and request_based,
> and live_md_type is DM_TYPE_NONE, the table should be considered as
> request_based, not inconsistent.
> Though the end result is same as such a table is rejected by other
> constraint anyway, I think it's good to keep the semantics clean
> and error messages consistent.
> 
> I.e. for the above case, the error message should be
> "Request-based dm doesn't support multiple targets yet",
> not "Inconsistent table: different target types can't be mixed up".

Hi,

Looks good, I'll fold your changes in and add your Signed-off-by.

Thanks,
Mike