dm ioctl: allow change device target type to error
PATCH v2 1/1 dm ioctl: allow change device target type to error
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".
--- drivers/md/dm-ioctl.c | 4 ++++ drivers/md/dm-table.c | 12 ++++++++++++ drivers/md/dm.h | 1 + 3 files changed, 17 insertions(+)
drivers/md/dm-ioctl.c +4 -0
@@ -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) {
drivers/md/dm-table.c +12 -0
@@ -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) {
drivers/md/dm.h +1 -0
@@ -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);
Signature · 1 line
1.8.3.1
Mikulas Patocka
maintainerQuoted 2 lines · Joe Jin
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
Quoted 74 lines
-v2: setup
md->queueeven target type is "error".Signed-off-by Joe Jin <[email protected]>--- drivers/md/dm-ioctl.c | 4 ++++ drivers/md/dm-table.c | 12 ++++++++++++ drivers/md/dm.h | 1 + 3 files changed, 17 insertions(+)drivers/md/dm-ioctl.c +4
-0@@ -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) {drivers/md/dm-table.c +12
-0@@ -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) {drivers/md/dm.h +1
-0@@ -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);Signature · 6 lines
1.8.3.1 -- dm-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/dm-devel
Quoted 11 lines · Mikulas Patocka
Quoted 2 lines · Joe Jin
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.
Quoted 8 lines
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
Quoted 23 lines · Mike Snitzer
Quoted 11 lines · Mikulas Patocka
Quoted 2 lines · Joe Jin
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.
Quoted 8 lines
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.
Quoted 8 lines
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_IMMUTABLEflag 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
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.
--- drivers/md/dm-target.c | 7 +++++++ 1 file changed, 7 insertions(+)
drivers/md/dm-target.c +7 -0
@@ -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 )
Signature · 1 line
1.8.3.1
Quoted 96 lines · Mikulas Patocka
Quoted 2 lines · Joe Jin
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
Quoted 74 lines
-v2: setup
md->queueeven target type is "error".Signed-off-by Joe Jin <[email protected]>--- drivers/md/dm-ioctl.c | 4 ++++ drivers/md/dm-table.c | 12 ++++++++++++ drivers/md/dm.h | 1 + 3 files changed, 17 insertions(+)drivers/md/dm-ioctl.c +4
-0@@ -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) {drivers/md/dm-table.c +12
-0@@ -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) {drivers/md/dm.h +1
-0@@ -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);Signature · 6 lines
1.8.3.1 -- dm-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/dm-devel
Quoted 35 lines · Joe Jin
Quoted 23 lines · Mike Snitzer
Quoted 11 lines · Mikulas Patocka
Quoted 2 lines · Joe Jin
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.
Quoted 8 lines
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.
Quoted 8 lines
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_IMMUTABLEflag set to be switched to error target?The latter restriction was introduced with commit
36a0456fb("dm table:
add immutable feature").Hi Mike,
Quoted 7 lines
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_tableno 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.
Quoted 40 lines · Joe Jin
Mikulas, thanks for you suggestions, I create new patch, can you please help review?
Subject: dm: add
map_rqdefine for errorcommit a5664da "dm ioctl: make bio or request based device type immutable" prevented "dmsetup
wape_table" change the target type to "error" for there is notmap_rqfor error target type.Signed-off-by Joe Jin <[email protected]>--- drivers/md/dm-target.c | 7 +++++++ 1 file changed, 7 insertions(+)drivers/md/dm-target.c +7
-0@@ -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 )Signature · 1 line
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 1/1 dm: allow error target to replace either bio-based and request-based targets
Quoted 9 lines · Mike Snitzer
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.
Quoted 6 lines
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 <[email protected]>
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.
--- drivers/md/dm-table.c | 18 +++++++++++++++++- drivers/md/dm-target.c | 9 ++++++++- drivers/md/dm.h | 11 +++++++++++ 3 files changed, 36 insertions(+), 2 deletions(-)
drivers/md/dm-table.c +17 -1
@@ -864,10 +864,26 @@ static int dm_table_set_type(struct dm_tstruct 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 ;
drivers/md/dm-target.c +8 -1
@@ -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 )
drivers/md/dm.h +11 -0
@@ -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
Quoted 2 lines · Mike Snitzer
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!
Quoted 5 lines
From: Mike Snitzer <[email protected]>
Date: Thu, 22 Aug 2013 18:21:38 -0400
Subject: [PATCH] dm: allow error target to replace either bio-based and request-based targets
Quoted 8 lines
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_typeand traindm_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.
Thanks,
Joe
Re: [dm-devel] [PATCH] dm: allow error target to replace either bio-based and request-based targets
In reply to Mike SnitzerHello Mike,
Quoted 8 lines · Mike Snitzer
Quoted 8 lines
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.
Quoted 5 lines
Quoted 2 lines
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
drivers/md/dm-table.c +23 -2
@@ -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 ;
drivers/md/dm-target.c +8 -1
@@ -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 )
drivers/md/dm.h +11 -0
@@ -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
Quoted 27 lines · Jun'ichi Nomura
Hello Mike,
Quoted 8 lines · Mike Snitzer
Quoted 8 lines
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.
Quoted 5 lines
Quoted 2 lines
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, andlive_md_typeisDM_TYPE_NONE, the table should be considered asrequest_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.
Quoted 3 lines
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