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

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