On 6/6/24 4:47 AM, Benjamin Marzinski wrote:
> On Wed, Jun 05, 2024 at 04:51:43PM +0900, Damien Le Moal wrote:
>> +static int dm_device_count_zones(struct dm_dev *dev,
>> + struct dm_device_zone_count *zc)
>> +{
>> + int ret;
>> +
>> + ret = blkdev_report_zones(dev->bdev, 0, UINT_MAX,
>> + dm_device_count_zones_cb, zc);
>
> Other than the nitpick that BLK_ALL_ZONES looks better than UINT_MAX
> here, looks good.
Thanks, will make this change.
However, I realized that we have another serious issue with this, so more
changes are needed. The issue is that we have the call chain:
dm_table_set_restrictions(t, q, lim) -> dm_set_zones_restrictions(t, q, lim) ->
dm_set_zone_resource_limits(md, t, lim) which is fine as all these functions
operate looking at the same limits. But then after calling
dm_set_zone_resource_limits() which may modify the max open/max active limits,
dm_set_zones_restrictions() calls dm_revalidate_zones(md, t), which then calls
blk_revalidate_disk_zones(). This last function looks at the max open/max
active limits of the disk queue limits to setup zone write plugging (if needed
in the case of DM). But the disk queue limits are *different* from the lim
pointer passed from dm_table_set_restrictions() as these have not been applied
yet. So we have blk_revalidate_disk_zones() looking at the "old", not yet
corrected zone resource limits.
I have 22 different test cases for testing this and none of them can detect a
problem with this. But it is still wrong and needs to be fixed.
Christoph,
Unless you have a better idea, I think we need to pass a queue_limits struct
pointer to blk_revalidate_disk_zones() -> disk_update_zone_resources(). This
pointer can be NULL, in which case disk_update_zone_resources() needs to do the
limit start_update+commit. But if it is not NULL, then
disk_update_zone_resources() must use the passed limits.
--
Damien Le Moal
Western Digital Research
