[PATCH 1/3] dm ioctl: increase granularity of type_lock when loading table

[PATCH 1/3] dm ioctl: increase granularity of type_lock when loading table

[Mike Snitzer]

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

[PATCH 2/3] dm ioctl: cleanup error handling in table_load

[Mike Snitzer]

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

[PATCH 3/3] dm: allow error target to replace immutable target

[Mike Snitzer]

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

Re: [PATCH 3/3] dm: allow error target to replace immutable target

[Mikulas Patocka]

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
> 

Re: [PATCH 3/3] dm: allow error target to replace immutable target

[Mike Snitzer]

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

Re: [PATCH 3/3] dm: allow error target to replace immutable target

[Mikulas Patocka]

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

Re: [PATCH 3/3] dm: allow error target to replace immutable target

[Mike Snitzer]

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?

Re: [PATCH 3/3] dm: allow error target to replace immutable target

[Mikulas Patocka]

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