Re: [PATCH v2 1/2] block: Imporve checks on zone resource limits

[Damien Le Moal <dlemoal@kernel.org>]

On 6/5/24 13:17, Christoph Hellwig wrote:
> improve is misspelled in the subject.
> 
>> @@ -80,6 +80,10 @@ static int blk_validate_zoned_limits(struct queue_limits *lim)
>>  	if (WARN_ON_ONCE(!IS_ENABLED(CONFIG_BLK_DEV_ZONED)))
>>  		return -EINVAL;
>>  
>> +	if (lim->max_active_zones &&
>> +	    WARN_ON_ONCE(lim->max_open_zones > lim->max_active_zones))
>> +		lim->max_open_zones = lim->max_active_zones;
> 
> Given how active zones are defined this is an error condition, and
> should return -EINVAL.
> 
>> diff --git a/block/blk-zoned.c b/block/blk-zoned.c
>> index 52abebf56027..2af4d5ca81d2 100644
>> --- a/block/blk-zoned.c
>> +++ b/block/blk-zoned.c
>> @@ -1660,6 +1660,11 @@ static int disk_update_zone_resources(struct gendisk *disk,
>>  	lim = queue_limits_start_update(q);
>>  
>>  	nr_seq_zones = disk->nr_zones - nr_conv_zones;
>> +	if (WARN_ON_ONCE(lim.max_active_zones > nr_seq_zones))
>> +		lim.max_active_zones = 0;
>> +	if (WARN_ON_ONCE(lim.max_open_zones > nr_seq_zones))
>> +		lim.max_open_zones = 0;
> 
> Why would you warn about this?  Offering an open/active limit larger
> than the number of sequential zones is a pretty natural condition
> for certain corner cases (e.g. create only a tiny namespace on a ZNS
> SSD).  This could also use a code comment explaining why the limit
> is adjusted.

Right. I actually did not consider that case, which is indeed valid given that
for nvme, the limits are per controller, not namespace (which is a very
unfortunate design flaw...).

I will remove the warn and add a comment.

-- 
Damien Le Moal
Western Digital Research