Re: [PATCH v2] null_blk: add support for max open/active zone limit for zoned devices

[Damien Le Moal]

On 2020/08/28 19:06, Niklas Cassel wrote:
> On Fri, Aug 28, 2020 at 07:06:26AM +0000, Damien Le Moal wrote:
>> On 2020/08/27 22:50, Niklas Cassel wrote:
>>> Add support for user space to set a max open zone and a max active zone
>>> limit via configfs. By default, the default values are 0 == no limit.
>>>
>>> Call the block layer API functions used for exposing the configured
>>> limits to sysfs.
>>>
>>> Add accounting in null_blk_zoned so that these new limits are respected.
>>> Performing an operating that would exceed these limits results in a
>>
>> Performing a write operation that would result in exceeding these...
>>
>>> standard I/O error.
>>>
> 
> It is not only a write operation, also e.g. open zone operation.
> However I will s/Performing an operating/Performing an operation/
> 
>>> +/*
>>> + * This function matches the manage open zone resources function in the ZBC standard,
>>> + * with the addition of max active zones support (added in the ZNS standard).
>>> + *
>>> + * The function determines if a zone can transition to implicit open or explicit open,
>>> + * while maintaining the max open zone (and max active zone) limit(s). It may close an
>>> + * implicit open zone in order to make additional zone resources available.
>>> + *
>>> + * ZBC states that an implicit open zone shall be closed only if there is not
>>> + * room within the open limit. However, with the addition of an active limit,
>>> + * it is not certain that closing an implicit open zone will allow a new zone
>>> + * to be opened, since we might already be at the active limit capacity.
>>> + */
>>> +static bool null_manage_zone_resources(struct nullb_device *dev, struct blk_zone *zone)
>>
>> I still do not like the name. Since this return a bool, what about
>> null_has_zone_resources() ?
> 
> I also don't like the name :)
> 
> However, since the ZBC spec, in the descriptions of "Write operation, Finish
> operation, and Open operation", says that the "manage open zone resources"
> function must be called before each of these operations are performed,
> and that there is a section that defines how the "manage open zone resources"
> is defined, I was thinking that having a similar name would be of value.
> 
> And I agree that it is weird that it returns a bool, but that is how it is
> defined in the standard.
> 
> Perhaps it should have exactly the same name as the standard, i.e.
> null_manage_open_zone_resources() ?
> 
> However, if you don't think that there is any point of trying to have
> a similar name to the function in ZBC, then I will happily rename it :)

Well, I prefer to prioritize code readability over following a not-so-good name
that the standard chose. The function description makes it clear that it is zone
management a-la-ZBC, so a function name clarifying what is being checked is
better in my opinion. Not a blocker though. Feel free to chose what to do here.

Cheers.


-- 
Damien Le Moal
Western Digital Research