[dm-devel] dm table: Fix zoned model check and zone sectors check

[Mike Snitzer]

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

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.

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