[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 at 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 at 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 at 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