* [PATCH 1/3] dm ioctl: increase granularity of type_lock when loading table @ 2013-08-29 3:21 Mike Snitzer 2013-08-29 3:21 ` [PATCH 2/3] dm ioctl: cleanup error handling in table_load Mike Snitzer 2013-08-29 3:21 ` [PATCH 3/3] dm: allow error target to replace immutable target Mike Snitzer 0 siblings, 2 replies; 8+ messages in thread From: Mike Snitzer @ 2013-08-29 3:21 UTC (permalink / raw) To: dm-devel Hold the mapped device's type_lock before calling populate_table() since it is where the table's type is determined based on the specified targets. There is no need to allow concurrent table loads to race to establish the table's targets or type. This eliminates the need to grab the lock in dm_table_set_type(). Signed-off-by: Mike Snitzer <snitzer@redhat.com> --- drivers/md/dm-ioctl.c | 6 ++++-- drivers/md/dm-table.c | 2 -- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c index 40e6dce..ec69f23 100644 --- a/drivers/md/dm-ioctl.c +++ b/drivers/md/dm-ioctl.c @@ -1264,9 +1264,12 @@ static int table_load(struct dm_ioctl *param, size_t param_size) if (r) goto out; + /* Protect md->type and md->queue against concurrent table loads. */ + dm_lock_md_type(md); r = populate_table(t, param, param_size); if (r) { dm_table_destroy(t); + dm_unlock_md_type(md); goto out; } @@ -1276,12 +1279,11 @@ static int table_load(struct dm_ioctl *param, size_t param_size) DMWARN("can't replace immutable target type %s", immutable_target_type->name); dm_table_destroy(t); + dm_unlock_md_type(md); r = -EINVAL; goto out; } - /* Protect md->type and md->queue against concurrent table loads. */ - dm_lock_md_type(md); if (dm_get_md_type(md) == DM_TYPE_NONE) /* Initial table load: acquire type of table. */ dm_set_md_type(md, dm_table_get_type(t)); diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c index f309477..8f87835 100644 --- a/drivers/md/dm-table.c +++ b/drivers/md/dm-table.c @@ -888,9 +888,7 @@ static int dm_table_set_type(struct dm_table *t) * Determine the type from the live device. * Default to bio-based if device is new. */ - dm_lock_md_type(t->md); live_md_type = dm_get_md_type(t->md); - dm_unlock_md_type(t->md); if (live_md_type == DM_TYPE_REQUEST_BASED) request_based = 1; else -- 1.8.1.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/3] dm ioctl: cleanup error handling in table_load 2013-08-29 3:21 [PATCH 1/3] dm ioctl: increase granularity of type_lock when loading table Mike Snitzer @ 2013-08-29 3:21 ` Mike Snitzer 2013-08-29 3:21 ` [PATCH 3/3] dm: allow error target to replace immutable target Mike Snitzer 1 sibling, 0 replies; 8+ messages in thread From: Mike Snitzer @ 2013-08-29 3:21 UTC (permalink / raw) To: dm-devel Make use of common cleanup code. Signed-off-by: Mike Snitzer <snitzer@redhat.com> --- drivers/md/dm-ioctl.c | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c index ec69f23..301e0a5 100644 --- a/drivers/md/dm-ioctl.c +++ b/drivers/md/dm-ioctl.c @@ -1267,21 +1267,16 @@ static int table_load(struct dm_ioctl *param, size_t param_size) /* Protect md->type and md->queue against concurrent table loads. */ dm_lock_md_type(md); r = populate_table(t, param, param_size); - if (r) { - dm_table_destroy(t); - dm_unlock_md_type(md); - goto out; - } + if (r) + goto out_unlock_md_type; immutable_target_type = dm_get_immutable_target_type(md); if (immutable_target_type && (immutable_target_type != dm_table_get_immutable_target_type(t))) { DMWARN("can't replace immutable target type %s", immutable_target_type->name); - dm_table_destroy(t); - dm_unlock_md_type(md); r = -EINVAL; - goto out; + goto out_unlock_md_type; } if (dm_get_md_type(md) == DM_TYPE_NONE) @@ -1289,19 +1284,15 @@ static int table_load(struct dm_ioctl *param, size_t param_size) dm_set_md_type(md, dm_table_get_type(t)); else if (dm_get_md_type(md) != dm_table_get_type(t)) { DMWARN("can't change device type after initial table load."); - dm_table_destroy(t); - dm_unlock_md_type(md); r = -EINVAL; - goto out; + goto out_unlock_md_type; } /* setup md->queue to reflect md's type (may block) */ r = dm_setup_md_queue(md); if (r) { DMWARN("unable to set up device queue for new table."); - dm_table_destroy(t); - dm_unlock_md_type(md); - goto out; + goto out_unlock_md_type; } dm_unlock_md_type(md); @@ -1311,9 +1302,8 @@ static int table_load(struct dm_ioctl *param, size_t param_size) if (!hc || hc->md != md) { DMWARN("device has been removed from the dev hash table."); up_write(&_hash_lock); - dm_table_destroy(t); r = -ENXIO; - goto out; + goto out_destroy_table; } if (hc->new_map) @@ -1324,7 +1314,6 @@ static int table_load(struct dm_ioctl *param, size_t param_size) param->flags |= DM_INACTIVE_PRESENT_FLAG; __dev_status(md, param); -out: if (old_map) { dm_sync_table(md); dm_table_destroy(old_map); @@ -1332,6 +1321,15 @@ out: dm_put(md); + return 0; + +out_unlock_md_type: + dm_unlock_md_type(md); +out_destroy_table: + dm_table_destroy(t); +out: + dm_put(md); + return r; } -- 1.8.1.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/3] dm: allow error target to replace immutable target 2013-08-29 3:21 [PATCH 1/3] dm ioctl: increase granularity of type_lock when loading table Mike Snitzer 2013-08-29 3:21 ` [PATCH 2/3] dm ioctl: cleanup error handling in table_load Mike Snitzer @ 2013-08-29 3:21 ` Mike Snitzer 2013-08-29 14:37 ` Mikulas Patocka 1 sibling, 1 reply; 8+ messages in thread From: Mike Snitzer @ 2013-08-29 3:21 UTC (permalink / raw) To: dm-devel Introduce DM_TARGET_ALWAYS_RETURNS_IO_ERROR to indicate that a target always returns IO error. Because the target will error all IO it can safely replace any target (including an immutable target) as along as the associated mapped device is not open. If an error target replaces an immutable target it is elevated to the mapped device's immutable target type. The "error" target can now replace an immutable target like the thin provisioning pool ("thin-pool") target. Signed-off-by: Mike Snitzer <snitzer@redhat.com> --- drivers/md/dm-ioctl.c | 27 ++++++++++++++++++++++++++- drivers/md/dm-table.c | 9 ++++++++- drivers/md/dm-target.c | 1 + include/linux/device-mapper.h | 10 ++++++++++ 4 files changed, 45 insertions(+), 2 deletions(-) diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c index 301e0a5..08186cb 100644 --- a/drivers/md/dm-ioctl.c +++ b/drivers/md/dm-ioctl.c @@ -1248,6 +1248,30 @@ static int populate_table(struct dm_table *table, return dm_table_complete(table); } +static bool immutable_target_type_is_valid(struct mapped_device *md, + struct target_type *immutable_tt, + struct target_type *table_immutable_tt) +{ + if (immutable_tt == table_immutable_tt) + return true; + + if (!table_immutable_tt) + return false; + + if (dm_target_always_returns_io_error(table_immutable_tt)) { + /* + * Only allow a transition to an error target_type if + * the mapped_device is no longer open. + */ + if (!dm_open_count(md)) + return true; + + DMERR("can't change target type to error while device is in use"); + } + + return false; +} + static int table_load(struct dm_ioctl *param, size_t param_size) { int r; @@ -1272,7 +1296,8 @@ static int table_load(struct dm_ioctl *param, size_t param_size) immutable_target_type = dm_get_immutable_target_type(md); if (immutable_target_type && - (immutable_target_type != dm_table_get_immutable_target_type(t))) { + !immutable_target_type_is_valid(md, immutable_target_type, + dm_table_get_immutable_target_type(t))) { DMWARN("can't replace immutable target type %s", immutable_target_type->name); r = -EINVAL; diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c index 8f87835..70d3067 100644 --- a/drivers/md/dm-table.c +++ b/drivers/md/dm-table.c @@ -745,7 +745,14 @@ int dm_table_add_target(struct dm_table *t, const char *type, return -EINVAL; } - if (t->immutable_target_type) { + if (dm_target_always_returns_io_error(tgt->type) && + dm_get_immutable_target_type(t->md)) { + /* + * This error target must be upgraded to immutable because + * the mapped device is already using an immutable target. + */ + t->immutable_target_type = tgt->type; + } else if (t->immutable_target_type) { if (t->immutable_target_type != tgt->type) { DMERR("%s: immutable target type %s cannot be mixed with other target types", dm_device_name(t->md), t->immutable_target_type->name); diff --git a/drivers/md/dm-target.c b/drivers/md/dm-target.c index 242e3ce..3b9a988 100644 --- a/drivers/md/dm-target.c +++ b/drivers/md/dm-target.c @@ -139,6 +139,7 @@ static int io_err_map_rq(struct dm_target *ti, struct request *clone, static struct target_type error_target = { .name = "error", + .features = DM_TARGET_ALWAYS_RETURNS_IO_ERROR, .version = {1, 2, 0}, .ctr = io_err_ctr, .dtr = io_err_dtr, diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h index 653073d..2451e6b 100644 --- a/include/linux/device-mapper.h +++ b/include/linux/device-mapper.h @@ -192,6 +192,16 @@ struct target_type { #define dm_target_is_immutable(type) ((type)->features & DM_TARGET_IMMUTABLE) /* + * Indicates that a target always returns IO error. Because the target will error + * all IO it can safely replace any target (including an immutable target) as long + * as the associated mapped device is not open. If an error target replaces an + * immutable target it is elevated to the mapped device's immutable target type. + */ +#define DM_TARGET_ALWAYS_RETURNS_IO_ERROR 0x00000008 +#define dm_target_always_returns_io_error(type) \ + ((type)->features & DM_TARGET_ALWAYS_RETURNS_IO_ERROR) + +/* * Some targets need to be sent the same WRITE bio severals times so * that they can send copies of it to different devices. This function * examines any supplied bio and returns the number of copies of it the -- 1.8.1.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] dm: allow error target to replace immutable target 2013-08-29 3:21 ` [PATCH 3/3] dm: allow error target to replace immutable target Mike Snitzer @ 2013-08-29 14:37 ` Mikulas Patocka 2013-08-29 14:44 ` Mike Snitzer 0 siblings, 1 reply; 8+ messages in thread From: Mikulas Patocka @ 2013-08-29 14:37 UTC (permalink / raw) To: Mike Snitzer; +Cc: dm-devel As I said - I think it would be better to remove the immutable flag than to create more flags to bypass it. Mikulas On Wed, 28 Aug 2013, Mike Snitzer wrote: > Introduce DM_TARGET_ALWAYS_RETURNS_IO_ERROR to indicate that a target > always returns IO error. Because the target will error all IO it can > safely replace any target (including an immutable target) as along as > the associated mapped device is not open. If an error target replaces > an immutable target it is elevated to the mapped device's immutable > target type. > > The "error" target can now replace an immutable target like the thin > provisioning pool ("thin-pool") target. > > Signed-off-by: Mike Snitzer <snitzer@redhat.com> > --- > drivers/md/dm-ioctl.c | 27 ++++++++++++++++++++++++++- > drivers/md/dm-table.c | 9 ++++++++- > drivers/md/dm-target.c | 1 + > include/linux/device-mapper.h | 10 ++++++++++ > 4 files changed, 45 insertions(+), 2 deletions(-) > > diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c > index 301e0a5..08186cb 100644 > --- a/drivers/md/dm-ioctl.c > +++ b/drivers/md/dm-ioctl.c > @@ -1248,6 +1248,30 @@ static int populate_table(struct dm_table *table, > return dm_table_complete(table); > } > > +static bool immutable_target_type_is_valid(struct mapped_device *md, > + struct target_type *immutable_tt, > + struct target_type *table_immutable_tt) > +{ > + if (immutable_tt == table_immutable_tt) > + return true; > + > + if (!table_immutable_tt) > + return false; > + > + if (dm_target_always_returns_io_error(table_immutable_tt)) { > + /* > + * Only allow a transition to an error target_type if > + * the mapped_device is no longer open. > + */ > + if (!dm_open_count(md)) > + return true; > + > + DMERR("can't change target type to error while device is in use"); > + } > + > + return false; > +} > + > static int table_load(struct dm_ioctl *param, size_t param_size) > { > int r; > @@ -1272,7 +1296,8 @@ static int table_load(struct dm_ioctl *param, size_t param_size) > > immutable_target_type = dm_get_immutable_target_type(md); > if (immutable_target_type && > - (immutable_target_type != dm_table_get_immutable_target_type(t))) { > + !immutable_target_type_is_valid(md, immutable_target_type, > + dm_table_get_immutable_target_type(t))) { > DMWARN("can't replace immutable target type %s", > immutable_target_type->name); > r = -EINVAL; > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c > index 8f87835..70d3067 100644 > --- a/drivers/md/dm-table.c > +++ b/drivers/md/dm-table.c > @@ -745,7 +745,14 @@ int dm_table_add_target(struct dm_table *t, const char *type, > return -EINVAL; > } > > - if (t->immutable_target_type) { > + if (dm_target_always_returns_io_error(tgt->type) && > + dm_get_immutable_target_type(t->md)) { > + /* > + * This error target must be upgraded to immutable because > + * the mapped device is already using an immutable target. > + */ > + t->immutable_target_type = tgt->type; > + } else if (t->immutable_target_type) { > if (t->immutable_target_type != tgt->type) { > DMERR("%s: immutable target type %s cannot be mixed with other target types", > dm_device_name(t->md), t->immutable_target_type->name); > diff --git a/drivers/md/dm-target.c b/drivers/md/dm-target.c > index 242e3ce..3b9a988 100644 > --- a/drivers/md/dm-target.c > +++ b/drivers/md/dm-target.c > @@ -139,6 +139,7 @@ static int io_err_map_rq(struct dm_target *ti, struct request *clone, > > static struct target_type error_target = { > .name = "error", > + .features = DM_TARGET_ALWAYS_RETURNS_IO_ERROR, > .version = {1, 2, 0}, > .ctr = io_err_ctr, > .dtr = io_err_dtr, > diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h > index 653073d..2451e6b 100644 > --- a/include/linux/device-mapper.h > +++ b/include/linux/device-mapper.h > @@ -192,6 +192,16 @@ struct target_type { > #define dm_target_is_immutable(type) ((type)->features & DM_TARGET_IMMUTABLE) > > /* > + * Indicates that a target always returns IO error. Because the target will error > + * all IO it can safely replace any target (including an immutable target) as long > + * as the associated mapped device is not open. If an error target replaces an > + * immutable target it is elevated to the mapped device's immutable target type. > + */ > +#define DM_TARGET_ALWAYS_RETURNS_IO_ERROR 0x00000008 > +#define dm_target_always_returns_io_error(type) \ > + ((type)->features & DM_TARGET_ALWAYS_RETURNS_IO_ERROR) > + > +/* > * Some targets need to be sent the same WRITE bio severals times so > * that they can send copies of it to different devices. This function > * examines any supplied bio and returns the number of copies of it the > -- > 1.8.1.4 > > -- > dm-devel mailing list > dm-devel@redhat.com > https://www.redhat.com/mailman/listinfo/dm-devel > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] dm: allow error target to replace immutable target 2013-08-29 14:37 ` Mikulas Patocka @ 2013-08-29 14:44 ` Mike Snitzer 2013-08-29 17:07 ` Mikulas Patocka 0 siblings, 1 reply; 8+ messages in thread From: Mike Snitzer @ 2013-08-29 14:44 UTC (permalink / raw) To: Mikulas Patocka; +Cc: dm-devel On Thu, Aug 29 2013 at 10:37am -0400, Mikulas Patocka <mpatocka@redhat.com> wrote: > As I said - I think it would be better to remove the immutable flag than > to create more flags to bypass it. Immutable was introduced to prevent dangerous scenarios that weren't accounted for in original design, etc. It solved your crash when you rep;aced a thin-pool with an empty table. immutable is staying for now. And as it turns out allowing error target to override an immutable target was always held to be a logical/possible future relaxation of the immutable constraint. Mike ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] dm: allow error target to replace immutable target 2013-08-29 14:44 ` Mike Snitzer @ 2013-08-29 17:07 ` Mikulas Patocka 2013-08-29 18:04 ` Mike Snitzer 0 siblings, 1 reply; 8+ messages in thread From: Mikulas Patocka @ 2013-08-29 17:07 UTC (permalink / raw) To: Mike Snitzer; +Cc: dm-devel On Thu, 29 Aug 2013, Mike Snitzer wrote: > On Thu, Aug 29 2013 at 10:37am -0400, > Mikulas Patocka <mpatocka@redhat.com> wrote: > > > As I said - I think it would be better to remove the immutable flag than > > to create more flags to bypass it. > > Immutable was introduced to prevent dangerous scenarios that weren't > accounted for in original design, etc. It solved your crash when you > rep;aced a thin-pool with an empty table. > > immutable is staying for now. And as it turns out allowing error target > to override an immutable target was always held to be a logical/possible > future relaxation of the immutable constraint. > > Mike If you can replace a thin-pool target with an error target - so why can't you replace a thin-pool target with linear (or any other) target? I don't see why the error target should be special. Mikulas ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] dm: allow error target to replace immutable target 2013-08-29 17:07 ` Mikulas Patocka @ 2013-08-29 18:04 ` Mike Snitzer 2013-08-29 19:09 ` Mikulas Patocka 0 siblings, 1 reply; 8+ messages in thread From: Mike Snitzer @ 2013-08-29 18:04 UTC (permalink / raw) To: Mikulas Patocka; +Cc: dm-devel On Thu, Aug 29 2013 at 1:07pm -0400, Mikulas Patocka <mpatocka@redhat.com> wrote: > > > On Thu, 29 Aug 2013, Mike Snitzer wrote: > > > On Thu, Aug 29 2013 at 10:37am -0400, > > Mikulas Patocka <mpatocka@redhat.com> wrote: > > > > > As I said - I think it would be better to remove the immutable flag than > > > to create more flags to bypass it. > > > > Immutable was introduced to prevent dangerous scenarios that weren't > > accounted for in original design, etc. It solved your crash when you > > rep;aced a thin-pool with an empty table. > > > > immutable is staying for now. And as it turns out allowing error target > > to override an immutable target was always held to be a logical/possible > > future relaxation of the immutable constraint. > > > > Mike > > If you can replace a thin-pool target with an error target - so why can't > you replace a thin-pool target with linear (or any other) target? > > I don't see why the error target should be special. error target gives us the ability to disconnect the thin-pool from the underlying devices. It is practical for testing, etc. What use-case are you saying will be valid to switch a thin-pool target to linear? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] dm: allow error target to replace immutable target 2013-08-29 18:04 ` Mike Snitzer @ 2013-08-29 19:09 ` Mikulas Patocka 0 siblings, 0 replies; 8+ messages in thread From: Mikulas Patocka @ 2013-08-29 19:09 UTC (permalink / raw) To: Mike Snitzer; +Cc: dm-devel On Thu, 29 Aug 2013, Mike Snitzer wrote: > On Thu, Aug 29 2013 at 1:07pm -0400, > Mikulas Patocka <mpatocka@redhat.com> wrote: > > > > > > > On Thu, 29 Aug 2013, Mike Snitzer wrote: > > > > > On Thu, Aug 29 2013 at 10:37am -0400, > > > Mikulas Patocka <mpatocka@redhat.com> wrote: > > > > > > > As I said - I think it would be better to remove the immutable flag than > > > > to create more flags to bypass it. > > > > > > Immutable was introduced to prevent dangerous scenarios that weren't > > > accounted for in original design, etc. It solved your crash when you > > > rep;aced a thin-pool with an empty table. > > > > > > immutable is staying for now. And as it turns out allowing error target > > > to override an immutable target was always held to be a logical/possible > > > future relaxation of the immutable constraint. > > > > > > Mike > > > > If you can replace a thin-pool target with an error target - so why can't > > you replace a thin-pool target with linear (or any other) target? > > > > I don't see why the error target should be special. > > error target gives us the ability to disconnect the thin-pool from the > underlying devices. It is practical for testing, etc. > > What use-case are you saying will be valid to switch a thin-pool target > to linear? There is no use case. The user can do many stupid useless things with dmsetup and it is not the kernel's job to prevent him from doing them. What I am trying to say is that - adding an immutable flag and then adding another flag that bypasses the immutable flag is just pointless complication in the code. If you removed the immutable flag and then removed the DM_TARGET_ALWAYS_RETURNS_IO_ERROR flag, you end up with simpler code. Mikulas ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-08-29 19:09 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-08-29 3:21 [PATCH 1/3] dm ioctl: increase granularity of type_lock when loading table Mike Snitzer 2013-08-29 3:21 ` [PATCH 2/3] dm ioctl: cleanup error handling in table_load Mike Snitzer 2013-08-29 3:21 ` [PATCH 3/3] dm: allow error target to replace immutable target Mike Snitzer 2013-08-29 14:37 ` Mikulas Patocka 2013-08-29 14:44 ` Mike Snitzer 2013-08-29 17:07 ` Mikulas Patocka 2013-08-29 18:04 ` Mike Snitzer 2013-08-29 19:09 ` Mikulas Patocka
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).