[dm-devel] [PATCH] dm table: Fix zoned model check and zone sectors check

[dm-devel] [PATCH] dm table: Fix zoned model check and zone sectors check

[Shin'ichiro Kawasaki]

Commit 24f6b6036c9e ("dm table: fix zoned iterate_devices based device
capability checks") triggered dm table load failure when dm-zoned device
is set up for zoned block devices and a regular device for cache.

The commit inverted logic of two callback functions for iterate_devices:
device_is_zoned_model() and device_matches_zone_sectors(). The logic of
device_is_zoned_model() was inverted then all destination devices of all
targets in dm table are required to have the expected zoned model. This
is fine for dm-linear, dm-flakey and dm-crypt on zoned block devices
since each target has only one destination device. However, this results
in failure for dm-zoned with regular cache device since that target has
both regular block device and zoned block devices.

As for device_matches_zone_sectors(), the commit inverted the logic to
require all zoned block devices in each target have the specified
zone_sectors. This check also fails for regular block device which does
not have zones.

To avoid the check failures, fix the zone model check and the zone
sectors check. For zone model check, invert the device_is_zoned_model()
logic again to require at least one destination device in one target has
the specified zoned model. For zone sectors check, skip the check if the
destination device is not a zoned block device. Also add comments and
improve error messages to clarify expectations to the two checks.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Fixes: 24f6b6036c9e ("dm table: fix zoned iterate_devices based device capability checks")
---
 drivers/md/dm-table.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 95391f78b8d5..04b7a3978ef8 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1585,13 +1585,13 @@ bool dm_table_has_no_data_devices(struct dm_table *table)
 	return true;
 }
 
-static int device_not_zoned_model(struct dm_target *ti, struct dm_dev *dev,
-				  sector_t start, sector_t len, void *data)
+static int device_is_zoned_model(struct dm_target *ti, struct dm_dev *dev,
+				 sector_t start, sector_t len, void *data)
 {
 	struct request_queue *q = bdev_get_queue(dev->bdev);
 	enum blk_zoned_model *zoned_model = data;
 
-	return blk_queue_zoned_model(q) != *zoned_model;
+	return blk_queue_zoned_model(q) == *zoned_model;
 }
 
 static bool dm_table_supports_zoned_model(struct dm_table *t,
@@ -1608,7 +1608,7 @@ static bool dm_table_supports_zoned_model(struct dm_table *t,
 			return false;
 
 		if (!ti->type->iterate_devices ||
-		    ti->type->iterate_devices(ti, device_not_zoned_model, &zoned_model))
+		    !ti->type->iterate_devices(ti, device_is_zoned_model, &zoned_model))
 			return false;
 	}
 
@@ -1621,9 +1621,18 @@ static int device_not_matches_zone_sectors(struct dm_target *ti, struct dm_dev *
 	struct request_queue *q = bdev_get_queue(dev->bdev);
 	unsigned int *zone_sectors = data;
 
+	if (blk_queue_zoned_model(q) == BLK_ZONED_NONE)
+		return 0;
+
 	return blk_queue_zone_sectors(q) != *zone_sectors;
 }
 
+/*
+ * Check consistency of zoned model and zone sectors across all targets.
+ * For zoned model, at least one destination device used by each target shall
+ * have the zoned model. For zone sectors, if the destination device is a zoned
+ * block device, it shall have the specified zone_sectors.
+ */
 static int validate_hardware_zoned_model(struct dm_table *table,
 					 enum blk_zoned_model zoned_model,
 					 unsigned int zone_sectors)
@@ -1632,7 +1641,7 @@ static int validate_hardware_zoned_model(struct dm_table *table,
 		return 0;
 
 	if (!dm_table_supports_zoned_model(table, zoned_model)) {
-		DMERR("%s: zoned model is not consistent across all devices",
+		DMERR("%s: zoned model is not consistent across all targets",
 		      dm_device_name(table->md));
 		return -EINVAL;
 	}
@@ -1642,7 +1651,7 @@ static int validate_hardware_zoned_model(struct dm_table *table,
 		return -EINVAL;
 
 	if (dm_table_any_dev_attr(table, device_not_matches_zone_sectors, &zone_sectors)) {
-		DMERR("%s: zone sectors is not consistent across all devices",
+		DMERR("%s: zone sectors is not consistent across all zoned devices",
 		      dm_device_name(table->md));
 		return -EINVAL;
 	}
-- 
2.29.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH] dm table: Fix zoned model check and zone sectors check

[Damien Le Moal]

On 2021/03/10 17:25, Shin'ichiro Kawasaki wrote:
> Commit 24f6b6036c9e ("dm table: fix zoned iterate_devices based device
> capability checks") triggered dm table load failure when dm-zoned device
> is set up for zoned block devices and a regular device for cache.
> 
> The commit inverted logic of two callback functions for iterate_devices:
> device_is_zoned_model() and device_matches_zone_sectors(). The logic of
> device_is_zoned_model() was inverted then all destination devices of all
> targets in dm table are required to have the expected zoned model. This
> is fine for dm-linear, dm-flakey and dm-crypt on zoned block devices
> since each target has only one destination device. However, this results
> in failure for dm-zoned with regular cache device since that target has
> both regular block device and zoned block devices.
> 
> As for device_matches_zone_sectors(), the commit inverted the logic to
> require all zoned block devices in each target have the specified
> zone_sectors. This check also fails for regular block device which does
> not have zones.
> 
> To avoid the check failures, fix the zone model check and the zone
> sectors check. For zone model check, invert the device_is_zoned_model()
> logic again to require at least one destination device in one target has
> the specified zoned model. For zone sectors check, skip the check if the
> destination device is not a zoned block device. Also add comments and
> improve error messages to clarify expectations to the two checks.
> 
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Fixes: 24f6b6036c9e ("dm table: fix zoned iterate_devices based device capability checks")
> ---
>  drivers/md/dm-table.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index 95391f78b8d5..04b7a3978ef8 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -1585,13 +1585,13 @@ bool dm_table_has_no_data_devices(struct dm_table *table)
>  	return true;
>  }
>  
> -static int device_not_zoned_model(struct dm_target *ti, struct dm_dev *dev,
> -				  sector_t start, sector_t len, void *data)
> +static int device_is_zoned_model(struct dm_target *ti, struct dm_dev *dev,
> +				 sector_t start, sector_t len, void *data)
>  {
>  	struct request_queue *q = bdev_get_queue(dev->bdev);
>  	enum blk_zoned_model *zoned_model = data;
>  
> -	return blk_queue_zoned_model(q) != *zoned_model;
> +	return blk_queue_zoned_model(q) == *zoned_model;
>  }
>  
>  static bool dm_table_supports_zoned_model(struct dm_table *t,
> @@ -1608,7 +1608,7 @@ static bool dm_table_supports_zoned_model(struct dm_table *t,
>  			return false;
>  
>  		if (!ti->type->iterate_devices ||
> -		    ti->type->iterate_devices(ti, device_not_zoned_model, &zoned_model))
> +		    !ti->type->iterate_devices(ti, device_is_zoned_model, &zoned_model))
>  			return false;
>  	}
>  
> @@ -1621,9 +1621,18 @@ static int device_not_matches_zone_sectors(struct dm_target *ti, struct dm_dev *
>  	struct request_queue *q = bdev_get_queue(dev->bdev);
>  	unsigned int *zone_sectors = data;
>  
> +	if (blk_queue_zoned_model(q) == BLK_ZONED_NONE)
> +		return 0;
> +
>  	return blk_queue_zone_sectors(q) != *zone_sectors;
>  }
>  
> +/*
> + * Check consistency of zoned model and zone sectors across all targets.
> + * For zoned model, at least one destination device used by each target shall
> + * have the zoned model. For zone sectors, if the destination device is a zoned
> + * block device, it shall have the specified zone_sectors.
> + */
>  static int validate_hardware_zoned_model(struct dm_table *table,
>  					 enum blk_zoned_model zoned_model,
>  					 unsigned int zone_sectors)
> @@ -1632,7 +1641,7 @@ static int validate_hardware_zoned_model(struct dm_table *table,
>  		return 0;
>  
>  	if (!dm_table_supports_zoned_model(table, zoned_model)) {
> -		DMERR("%s: zoned model is not consistent across all devices",
> +		DMERR("%s: zoned model is not consistent across all targets",
>  		      dm_device_name(table->md));
>  		return -EINVAL;
>  	}
> @@ -1642,7 +1651,7 @@ static int validate_hardware_zoned_model(struct dm_table *table,
>  		return -EINVAL;
>  
>  	if (dm_table_any_dev_attr(table, device_not_matches_zone_sectors, &zone_sectors)) {
> -		DMERR("%s: zone sectors is not consistent across all devices",
> +		DMERR("%s: zone sectors is not consistent across all zoned devices",
>  		      dm_device_name(table->md));
>  		return -EINVAL;
>  	}
> 

Looks good to me.

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>

-- 
Damien Le Moal
Western Digital Research



--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] dm table: Fix zoned model check and zone sectors check

[Mike Snitzer]

On Wed, Mar 10 2021 at  3:25am -0500,
Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> wrote:

> Commit 24f6b6036c9e ("dm table: fix zoned iterate_devices based device
> capability checks") triggered dm table load failure when dm-zoned device
> is set up for zoned block devices and a regular device for cache.
> 
> The commit inverted logic of two callback functions for iterate_devices:
> device_is_zoned_model() and device_matches_zone_sectors(). The logic of
> device_is_zoned_model() was inverted then all destination devices of all
> targets in dm table are required to have the expected zoned model. This
> is fine for dm-linear, dm-flakey and dm-crypt on zoned block devices
> since each target has only one destination device. However, this results
> in failure for dm-zoned with regular cache device since that target has
> both regular block device and zoned block devices.
> 
> As for device_matches_zone_sectors(), the commit inverted the logic to
> require all zoned block devices in each target have the specified
> zone_sectors. This check also fails for regular block device which does
> not have zones.
> 
> To avoid the check failures, fix the zone model check and the zone
> sectors check. For zone model check, invert the device_is_zoned_model()
> logic again to require at least one destination device in one target has
> the specified zoned model. For zone sectors check, skip the check if the
> destination device is not a zoned block device. Also add comments and
> improve error messages to clarify expectations to the two checks.
> 
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Fixes: 24f6b6036c9e ("dm table: fix zoned iterate_devices based device capability checks")
> ---
>  drivers/md/dm-table.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index 95391f78b8d5..04b7a3978ef8 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -1585,13 +1585,13 @@ bool dm_table_has_no_data_devices(struct dm_table *table)
>  	return true;
>  }
>  
> -static int device_not_zoned_model(struct dm_target *ti, struct dm_dev *dev,
> -				  sector_t start, sector_t len, void *data)
> +static int device_is_zoned_model(struct dm_target *ti, struct dm_dev *dev,
> +				 sector_t start, sector_t len, void *data)
>  {
>  	struct request_queue *q = bdev_get_queue(dev->bdev);
>  	enum blk_zoned_model *zoned_model = data;
>  
> -	return blk_queue_zoned_model(q) != *zoned_model;
> +	return blk_queue_zoned_model(q) == *zoned_model;
>  }
>  
>  static bool dm_table_supports_zoned_model(struct dm_table *t,
> @@ -1608,7 +1608,7 @@ static bool dm_table_supports_zoned_model(struct dm_table *t,
>  			return false;
>  
>  		if (!ti->type->iterate_devices ||
> -		    ti->type->iterate_devices(ti, device_not_zoned_model, &zoned_model))
> +		    !ti->type->iterate_devices(ti, device_is_zoned_model, &zoned_model))
>  			return false;
>  	}

The point here is to ensure all zoned devices match the specific model,
right?

I understand commit 24f6b6036c9e wasn't correct, sorry about that.
But I don't think your change is correct either.  It'll allow a mix of
various zoned models (that might come after the first positive match for
the specified zoned_model)... but because the first match short-circuits
the loop those later mismatched zoned devices aren't checked.

Should device_is_zoned_model() also be trained to ignore BLK_ZONED_NONE
(like you did below)?

But _not_ invert the logic, so keep device_not_zoned_model.. otherwise
the first positive return of a match will short-circuit checking all
other devices match.

>  
> @@ -1621,9 +1621,18 @@ static int device_not_matches_zone_sectors(struct dm_target *ti, struct dm_dev *
>  	struct request_queue *q = bdev_get_queue(dev->bdev);
>  	unsigned int *zone_sectors = data;
>  
> +	if (blk_queue_zoned_model(q) == BLK_ZONED_NONE)
> +		return 0;
> +
>  	return blk_queue_zone_sectors(q) != *zone_sectors;
>  }

Thanks,
Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] dm table: Fix zoned model check and zone sectors check

[Damien Le Moal]

On 2021/03/12 2:54, Mike Snitzer wrote:
> On Wed, Mar 10 2021 at  3:25am -0500,
> Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> wrote:
> 
>> Commit 24f6b6036c9e ("dm table: fix zoned iterate_devices based device
>> capability checks") triggered dm table load failure when dm-zoned device
>> is set up for zoned block devices and a regular device for cache.
>>
>> The commit inverted logic of two callback functions for iterate_devices:
>> device_is_zoned_model() and device_matches_zone_sectors(). The logic of
>> device_is_zoned_model() was inverted then all destination devices of all
>> targets in dm table are required to have the expected zoned model. This
>> is fine for dm-linear, dm-flakey and dm-crypt on zoned block devices
>> since each target has only one destination device. However, this results
>> in failure for dm-zoned with regular cache device since that target has
>> both regular block device and zoned block devices.
>>
>> As for device_matches_zone_sectors(), the commit inverted the logic to
>> require all zoned block devices in each target have the specified
>> zone_sectors. This check also fails for regular block device which does
>> not have zones.
>>
>> To avoid the check failures, fix the zone model check and the zone
>> sectors check. For zone model check, invert the device_is_zoned_model()
>> logic again to require at least one destination device in one target has
>> the specified zoned model. For zone sectors check, skip the check if the
>> destination device is not a zoned block device. Also add comments and
>> improve error messages to clarify expectations to the two checks.
>>
>> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
>> Fixes: 24f6b6036c9e ("dm table: fix zoned iterate_devices based device capability checks")
>> ---
>>  drivers/md/dm-table.c | 21 +++++++++++++++------
>>  1 file changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
>> index 95391f78b8d5..04b7a3978ef8 100644
>> --- a/drivers/md/dm-table.c
>> +++ b/drivers/md/dm-table.c
>> @@ -1585,13 +1585,13 @@ bool dm_table_has_no_data_devices(struct dm_table *table)
>>  	return true;
>>  }
>>  
>> -static int device_not_zoned_model(struct dm_target *ti, struct dm_dev *dev,
>> -				  sector_t start, sector_t len, void *data)
>> +static int device_is_zoned_model(struct dm_target *ti, struct dm_dev *dev,
>> +				 sector_t start, sector_t len, void *data)
>>  {
>>  	struct request_queue *q = bdev_get_queue(dev->bdev);
>>  	enum blk_zoned_model *zoned_model = data;
>>  
>> -	return blk_queue_zoned_model(q) != *zoned_model;
>> +	return blk_queue_zoned_model(q) == *zoned_model;
>>  }
>>  
>>  static bool dm_table_supports_zoned_model(struct dm_table *t,
>> @@ -1608,7 +1608,7 @@ static bool dm_table_supports_zoned_model(struct dm_table *t,
>>  			return false;
>>  
>>  		if (!ti->type->iterate_devices ||
>> -		    ti->type->iterate_devices(ti, device_not_zoned_model, &zoned_model))
>> +		    !ti->type->iterate_devices(ti, device_is_zoned_model, &zoned_model))
>>  			return false;
>>  	}
> 
> The point here is to ensure all zoned devices match the specific model,
> right?
> 
> I understand commit 24f6b6036c9e wasn't correct, sorry about that.
> But I don't think your change is correct either.  It'll allow a mix of
> various zoned models (that might come after the first positive match for
> the specified zoned_model)... but because the first match short-circuits
> the loop those later mismatched zoned devices aren't checked.
> 
> Should device_is_zoned_model() also be trained to ignore BLK_ZONED_NONE
> (like you did below)?

Thinking more about this, I think we may have a deeper problem here. We need to
allow the combination of BLK_ZONED_NONE and BLK_ZONED_HM for dm-zoned multi
drive config using a regular SSD as cache. But blindly allowing such combination
of zoned and non-zoned drives will also end up allowing a setup combining these
drive types with dm-linear or dm-flakey or any other target that has the
DM_TARGET_ZONED_HM feature flag set. And that will definitely be bad and break
things if the target is not prepared for that.

Should we introduce a new feature flag ? Something like DM_TARGET_MIXED_ZONED_HM
? (not sure about the name of the flag. Suggestions ?)
We can then refine the validation and keep it as is (no mixed drive types) for a
target that has DM_TARGET_ZONED_HM, and allow mixing drive types if the target
has DM_TARGET_MIXED_ZONED_HM. This last case would be dm-zoned only for now.
Thoughts ?

> 
> But _not_ invert the logic, so keep device_not_zoned_model.. otherwise
> the first positive return of a match will short-circuit checking all
> other devices match.
> 
>>  
>> @@ -1621,9 +1621,18 @@ static int device_not_matches_zone_sectors(struct dm_target *ti, struct dm_dev *
>>  	struct request_queue *q = bdev_get_queue(dev->bdev);
>>  	unsigned int *zone_sectors = data;
>>  
>> +	if (blk_queue_zoned_model(q) == BLK_ZONED_NONE)
>> +		return 0;
>> +
>>  	return blk_queue_zone_sectors(q) != *zone_sectors;
>>  }
> 
> Thanks,
> Mike
> 
> 


-- 
Damien Le Moal
Western Digital Research



--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] dm table: Fix zoned model check and zone sectors check

[Mike Snitzer]

On Thu, Mar 11 2021 at  6:30pm -0500,
Damien Le Moal <Damien.LeMoal@wdc.com> wrote:

> On 2021/03/12 2:54, Mike Snitzer wrote:
> > On Wed, Mar 10 2021 at  3:25am -0500,
> > Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> wrote:
> > 
> >> Commit 24f6b6036c9e ("dm table: fix zoned iterate_devices based device
> >> capability checks") triggered dm table load failure when dm-zoned device
> >> is set up for zoned block devices and a regular device for cache.
> >>
> >> The commit inverted logic of two callback functions for iterate_devices:
> >> device_is_zoned_model() and device_matches_zone_sectors(). The logic of
> >> device_is_zoned_model() was inverted then all destination devices of all
> >> targets in dm table are required to have the expected zoned model. This
> >> is fine for dm-linear, dm-flakey and dm-crypt on zoned block devices
> >> since each target has only one destination device. However, this results
> >> in failure for dm-zoned with regular cache device since that target has
> >> both regular block device and zoned block devices.
> >>
> >> As for device_matches_zone_sectors(), the commit inverted the logic to
> >> require all zoned block devices in each target have the specified
> >> zone_sectors. This check also fails for regular block device which does
> >> not have zones.
> >>
> >> To avoid the check failures, fix the zone model check and the zone
> >> sectors check. For zone model check, invert the device_is_zoned_model()
> >> logic again to require at least one destination device in one target has
> >> the specified zoned model. For zone sectors check, skip the check if the
> >> destination device is not a zoned block device. Also add comments and
> >> improve error messages to clarify expectations to the two checks.
> >>
> >> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> >> Fixes: 24f6b6036c9e ("dm table: fix zoned iterate_devices based device capability checks")
> >> ---
> >>  drivers/md/dm-table.c | 21 +++++++++++++++------
> >>  1 file changed, 15 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> >> index 95391f78b8d5..04b7a3978ef8 100644
> >> --- a/drivers/md/dm-table.c
> >> +++ b/drivers/md/dm-table.c
> >> @@ -1585,13 +1585,13 @@ bool dm_table_has_no_data_devices(struct dm_table *table)
> >>  	return true;
> >>  }
> >>  
> >> -static int device_not_zoned_model(struct dm_target *ti, struct dm_dev *dev,
> >> -				  sector_t start, sector_t len, void *data)
> >> +static int device_is_zoned_model(struct dm_target *ti, struct dm_dev *dev,
> >> +				 sector_t start, sector_t len, void *data)
> >>  {
> >>  	struct request_queue *q = bdev_get_queue(dev->bdev);
> >>  	enum blk_zoned_model *zoned_model = data;
> >>  
> >> -	return blk_queue_zoned_model(q) != *zoned_model;
> >> +	return blk_queue_zoned_model(q) == *zoned_model;
> >>  }
> >>  
> >>  static bool dm_table_supports_zoned_model(struct dm_table *t,
> >> @@ -1608,7 +1608,7 @@ static bool dm_table_supports_zoned_model(struct dm_table *t,
> >>  			return false;
> >>  
> >>  		if (!ti->type->iterate_devices ||
> >> -		    ti->type->iterate_devices(ti, device_not_zoned_model, &zoned_model))
> >> +		    !ti->type->iterate_devices(ti, device_is_zoned_model, &zoned_model))
> >>  			return false;
> >>  	}
> > 
> > The point here is to ensure all zoned devices match the specific model,
> > right?
> > 
> > I understand commit 24f6b6036c9e wasn't correct, sorry about that.
> > But I don't think your change is correct either.  It'll allow a mix of
> > various zoned models (that might come after the first positive match for
> > the specified zoned_model)... but because the first match short-circuits
> > the loop those later mismatched zoned devices aren't checked.
> > 
> > Should device_is_zoned_model() also be trained to ignore BLK_ZONED_NONE
> > (like you did below)?
> 
> Thinking more about this, I think we may have a deeper problem here. We need to
> allow the combination of BLK_ZONED_NONE and BLK_ZONED_HM for dm-zoned multi
> drive config using a regular SSD as cache. But blindly allowing such combination
> of zoned and non-zoned drives will also end up allowing a setup combining these
> drive types with dm-linear or dm-flakey or any other target that has the
> DM_TARGET_ZONED_HM feature flag set. And that will definitely be bad and break
> things if the target is not prepared for that.
> 
> Should we introduce a new feature flag ? Something like DM_TARGET_MIXED_ZONED_HM
> ? (not sure about the name of the flag. Suggestions ?)
> We can then refine the validation and keep it as is (no mixed drive types) for a
> target that has DM_TARGET_ZONED_HM, and allow mixing drive types if the target
> has DM_TARGET_MIXED_ZONED_HM. This last case would be dm-zoned only for now.
> Thoughts ?

Think I'll struggle to give you a great answer until I understand which
target(s) would be setting DM_TARGET_MIXED_ZONED_HM (or whatever name).

I'll defer to you to sort out how best to validate only the supported
configs are allowed.  I trust you! ;)

Think this an instance where a patch (RFC or otherwise) would be quicker
way to discuss.

Thanks,
Mike

> 
> > 
> > But _not_ invert the logic, so keep device_not_zoned_model.. otherwise
> > the first positive return of a match will short-circuit checking all
> > other devices match.
> > 
> >>  
> >> @@ -1621,9 +1621,18 @@ static int device_not_matches_zone_sectors(struct dm_target *ti, struct dm_dev *
> >>  	struct request_queue *q = bdev_get_queue(dev->bdev);
> >>  	unsigned int *zone_sectors = data;
> >>  
> >> +	if (blk_queue_zoned_model(q) == BLK_ZONED_NONE)
> >> +		return 0;
> >> +
> >>  	return blk_queue_zone_sectors(q) != *zone_sectors;
> >>  }
> > 
> > Thanks,
> > Mike
> > 
> > 
> 
> 
> -- 
> Damien Le Moal
> Western Digital Research
> 
> 

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] dm table: Fix zoned model check and zone sectors check

[Damien Le Moal]

On 2021/03/13 4:09, Mike Snitzer wrote:
> On Thu, Mar 11 2021 at  6:30pm -0500,
> Damien Le Moal <Damien.LeMoal@wdc.com> wrote:
> 
>> On 2021/03/12 2:54, Mike Snitzer wrote:
>>> On Wed, Mar 10 2021 at  3:25am -0500,
>>> Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> wrote:
>>>
>>>> Commit 24f6b6036c9e ("dm table: fix zoned iterate_devices based device
>>>> capability checks") triggered dm table load failure when dm-zoned device
>>>> is set up for zoned block devices and a regular device for cache.
>>>>
>>>> The commit inverted logic of two callback functions for iterate_devices:
>>>> device_is_zoned_model() and device_matches_zone_sectors(). The logic of
>>>> device_is_zoned_model() was inverted then all destination devices of all
>>>> targets in dm table are required to have the expected zoned model. This
>>>> is fine for dm-linear, dm-flakey and dm-crypt on zoned block devices
>>>> since each target has only one destination device. However, this results
>>>> in failure for dm-zoned with regular cache device since that target has
>>>> both regular block device and zoned block devices.
>>>>
>>>> As for device_matches_zone_sectors(), the commit inverted the logic to
>>>> require all zoned block devices in each target have the specified
>>>> zone_sectors. This check also fails for regular block device which does
>>>> not have zones.
>>>>
>>>> To avoid the check failures, fix the zone model check and the zone
>>>> sectors check. For zone model check, invert the device_is_zoned_model()
>>>> logic again to require at least one destination device in one target has
>>>> the specified zoned model. For zone sectors check, skip the check if the
>>>> destination device is not a zoned block device. Also add comments and
>>>> improve error messages to clarify expectations to the two checks.
>>>>
>>>> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
>>>> Fixes: 24f6b6036c9e ("dm table: fix zoned iterate_devices based device capability checks")
>>>> ---
>>>>  drivers/md/dm-table.c | 21 +++++++++++++++------
>>>>  1 file changed, 15 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
>>>> index 95391f78b8d5..04b7a3978ef8 100644
>>>> --- a/drivers/md/dm-table.c
>>>> +++ b/drivers/md/dm-table.c
>>>> @@ -1585,13 +1585,13 @@ bool dm_table_has_no_data_devices(struct dm_table *table)
>>>>  	return true;
>>>>  }
>>>>  
>>>> -static int device_not_zoned_model(struct dm_target *ti, struct dm_dev *dev,
>>>> -				  sector_t start, sector_t len, void *data)
>>>> +static int device_is_zoned_model(struct dm_target *ti, struct dm_dev *dev,
>>>> +				 sector_t start, sector_t len, void *data)
>>>>  {
>>>>  	struct request_queue *q = bdev_get_queue(dev->bdev);
>>>>  	enum blk_zoned_model *zoned_model = data;
>>>>  
>>>> -	return blk_queue_zoned_model(q) != *zoned_model;
>>>> +	return blk_queue_zoned_model(q) == *zoned_model;
>>>>  }
>>>>  
>>>>  static bool dm_table_supports_zoned_model(struct dm_table *t,
>>>> @@ -1608,7 +1608,7 @@ static bool dm_table_supports_zoned_model(struct dm_table *t,
>>>>  			return false;
>>>>  
>>>>  		if (!ti->type->iterate_devices ||
>>>> -		    ti->type->iterate_devices(ti, device_not_zoned_model, &zoned_model))
>>>> +		    !ti->type->iterate_devices(ti, device_is_zoned_model, &zoned_model))
>>>>  			return false;
>>>>  	}
>>>
>>> The point here is to ensure all zoned devices match the specific model,
>>> right?
>>>
>>> I understand commit 24f6b6036c9e wasn't correct, sorry about that.
>>> But I don't think your change is correct either.  It'll allow a mix of
>>> various zoned models (that might come after the first positive match for
>>> the specified zoned_model)... but because the first match short-circuits
>>> the loop those later mismatched zoned devices aren't checked.
>>>
>>> Should device_is_zoned_model() also be trained to ignore BLK_ZONED_NONE
>>> (like you did below)?
>>
>> Thinking more about this, I think we may have a deeper problem here. We need to
>> allow the combination of BLK_ZONED_NONE and BLK_ZONED_HM for dm-zoned multi
>> drive config using a regular SSD as cache. But blindly allowing such combination
>> of zoned and non-zoned drives will also end up allowing a setup combining these
>> drive types with dm-linear or dm-flakey or any other target that has the
>> DM_TARGET_ZONED_HM feature flag set. And that will definitely be bad and break
>> things if the target is not prepared for that.
>>
>> Should we introduce a new feature flag ? Something like DM_TARGET_MIXED_ZONED_HM
>> ? (not sure about the name of the flag. Suggestions ?)
>> We can then refine the validation and keep it as is (no mixed drive types) for a
>> target that has DM_TARGET_ZONED_HM, and allow mixing drive types if the target
>> has DM_TARGET_MIXED_ZONED_HM. This last case would be dm-zoned only for now.
>> Thoughts ?
> 
> Think I'll struggle to give you a great answer until I understand which
> target(s) would be setting DM_TARGET_MIXED_ZONED_HM (or whatever name).

That would be dm-zoned only for now. dm-crypt, dm-linear and dm-flakey must keep
using DM_TARGET_ZONED_HM as they do not have any code allowing to handle mixed
drive type.

> 
> I'll defer to you to sort out how best to validate only the supported
> configs are allowed.  I trust you! ;)
> 
> Think this an instance where a patch (RFC or otherwise) would be quicker
> way to discuss.

Sounds good. I will work with Shin'ichiro to cook something and send it asap.

Thanks !

> 
> Thanks,
> Mike
> 
>>
>>>
>>> But _not_ invert the logic, so keep device_not_zoned_model.. otherwise
>>> the first positive return of a match will short-circuit checking all
>>> other devices match.
>>>
>>>>  
>>>> @@ -1621,9 +1621,18 @@ static int device_not_matches_zone_sectors(struct dm_target *ti, struct dm_dev *
>>>>  	struct request_queue *q = bdev_get_queue(dev->bdev);
>>>>  	unsigned int *zone_sectors = data;
>>>>  
>>>> +	if (blk_queue_zoned_model(q) == BLK_ZONED_NONE)
>>>> +		return 0;
>>>> +
>>>>  	return blk_queue_zone_sectors(q) != *zone_sectors;
>>>>  }
>>>
>>> Thanks,
>>> Mike
>>>
>>>
>>
>>
>> -- 
>> Damien Le Moal
>> Western Digital Research
>>
>>
> 
> 


-- 
Damien Le Moal
Western Digital Research



--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel