On 2021/03/16 22:47, Mike Snitzer wrote:
> On Tue, Mar 16 2021 at 2:14am -0400,
> Damien Le Moal <Damien.LeMoal@xxxxxxx> wrote:
>
>> On 2021/03/16 13:36, 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, introduce the new feature flag
>>> DM_TARGET_MIXED_ZONED_MODEL, and set it to dm-zoned target. When the
>>> target has this flag, allow it to have destination devices with any
>>> zoned model. For zone sectors check, skip the check if the destination
>>> device is not a zoned block device. Also add comments and improve an
>>> error message to clarify expectations to the two checks.
>>>
>>> Fixes: 24f6b6036c9e ("dm table: fix zoned iterate_devices based device capability checks")
>>> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@xxxxxxx>
>>> Signed-off-by: Damien Le Moal <damien.lemoal@xxxxxxx>
>>> ---
>>> Changes from v1:
>>> * Added DM_TARGET_MIXED_ZONED_MODEL feature for zoned model check of dm-zoned
>>>
>>> drivers/md/dm-table.c | 34 ++++++++++++++++++++++++++--------
>>> drivers/md/dm-zoned-target.c | 2 +-
>>> include/linux/device-mapper.h | 15 ++++++++++++++-
>>> 3 files changed, 41 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
>>> index 95391f78b8d5..cc73d5b473eb 100644
>>> --- a/drivers/md/dm-table.c
>>> +++ b/drivers/md/dm-table.c
>>> @@ -1594,6 +1594,13 @@ static int device_not_zoned_model(struct dm_target *ti, struct dm_dev *dev,
>>> return blk_queue_zoned_model(q) != *zoned_model;
>>> }
>>>
>>> +/*
>>> + * Check the device zoned model based on the target feature flag. If the target
>>> + * has the DM_TARGET_ZONED_HM feature flag set, host-managed zoned devices are
>>> + * also accepted but all devices must have the same zoned model. If the target
>>> + * has the DM_TARGET_MIXED_ZONED_MODEL feature set, the devices can have any
>>> + * zoned model with all zoned devices having the same zone size.
>>> + */
>>> static bool dm_table_supports_zoned_model(struct dm_table *t,
>>> enum blk_zoned_model zoned_model)
>>> {
>>> @@ -1603,13 +1610,16 @@ static bool dm_table_supports_zoned_model(struct dm_table *t,
>>> for (i = 0; i < dm_table_get_num_targets(t); i++) {
>>> ti = dm_table_get_target(t, i);
>>>
>>> - if (zoned_model == BLK_ZONED_HM &&
>>> - !dm_target_supports_zoned_hm(ti->type))
>>> - return false;
>>> -
>>> - if (!ti->type->iterate_devices ||
>>> - ti->type->iterate_devices(ti, device_not_zoned_model, &zoned_model))
>>> - return false;
>>> + if (dm_target_supports_zoned_hm(ti->type)) {
>>> + if (!ti->type->iterate_devices ||
>>> + ti->type->iterate_devices(ti,
>>> + device_not_zoned_model,
>>> + &zoned_model))
>>> + return false;
>>> + } else if (!dm_target_supports_mixed_zoned_model(ti->type)) {
>>> + if (zoned_model == BLK_ZONED_HM)
>>> + return false;
>>> + }
>>> }
>>>
>>> return true;
>>> @@ -1621,9 +1631,17 @@ 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_is_zoned(q))
>>> + return 0;
>>> +
>>> return blk_queue_zone_sectors(q) != *zone_sectors;
>>> }
>>>
>>> +/*
>>> + * Check consistency of zoned model and zone sectors across all targets. 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)
>>> @@ -1642,7 +1660,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;
>>> }
>>> diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c
>>> index 697f9de37355..7e88df64d197 100644
>>> --- a/drivers/md/dm-zoned-target.c
>>> +++ b/drivers/md/dm-zoned-target.c
>>> @@ -1143,7 +1143,7 @@ static int dmz_message(struct dm_target *ti, unsigned int argc, char **argv,
>>> static struct target_type dmz_type = {
>>> .name = "zoned",
>>> .version = {2, 0, 0},
>>> - .features = DM_TARGET_SINGLETON | DM_TARGET_ZONED_HM,
>>> + .features = DM_TARGET_SINGLETON | DM_TARGET_MIXED_ZONED_MODEL,
>>
>> Thinking about it, DM_TARGET_SINGLETON is wrong for dm-zoned now that we can
>> create devices using multiple devices... But it does not seem to matter much
>> since it really looks like this flag is totally unused/unchecked by DM core.
>> Maybe something we can remove in a followup cleanup ? Mike ?
>
> Not sure why you think it unused, drivers/md/dm-table.c:dm_table_add_target:
>
> if (t->singleton) {
> DMERR("%s: target type %s must appear alone in table",
> dm_device_name(t->md), t->targets->type->name);
> return -EINVAL;
> }
>
> ...
>
> if (dm_target_needs_singleton(tgt->type)) {
> if (t->num_targets) {
> tgt->error = "singleton target type must appear alone in table";
> goto bad;
> }
> t->singleton = true;
> }
>
> So it really should be causing problems if you do in fact support/need
> multiple targets combined with "zoned".
I completely misread the code ! There is no problem :)
My apologies about the noise.
>
> Mike
>
>
--
Damien Le Moal
Western Digital Research
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
