On 2021/03/12 2:54, Mike Snitzer wrote:
> 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)?
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@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
