On Wed, Mar 10 2021 at 3:25am -0500,
Shin'ichiro Kawasaki <shinichiro.kawasaki@xxxxxxx> 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@xxxxxxx>
> 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@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
