Re: [PATCH v8 02/11] block: allow blk-zoned devices to have non-power-of-2 zone size

[Pankaj Raghav <p.raghav@samsung.com>]

On 2022-07-28 01:16, Bart Van Assche wrote:
> On 7/27/22 09:22, Pankaj Raghav wrote:
>> Checking if a given sector is aligned to a zone is a common
>> operation that is performed for zoned devices. Add
>> bdev_is_zone_start helper to check for this instead of opencoding it
>> everywhere.
> 
> I can't find the bdev_is_zone_start() function in this patch?
> 
I made the name change from bdev_is_zone_start to bdev_is_zone_aligned
last moment and missed changing it in the commit log.

>> To make this work bdev_get_queue(), bdev_zone_sectors() and
>> bdev_is_zoned() are moved earlier without modifications.
> 
> Can that change perhaps be isolated into a separate patch?
> 
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index 3d286a256d3d..1f7e9a90e198 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -570,7 +570,7 @@ static inline blk_status_t
>> blk_check_zone_append(struct request_queue *q,
>>           return BLK_STS_NOTSUPP;
>>         /* The bio sector must point to the start of a sequential zone */
>> -    if (bio->bi_iter.bi_sector & (bdev_zone_sectors(bio->bi_bdev) -
>> 1) ||
>> +    if (!bdev_is_zone_aligned(bio->bi_bdev, bio->bi_iter.bi_sector) ||
>>           !bio_zone_is_seq(bio))
>>           return BLK_STS_IOERR;
> 
> The bdev_is_zone_start() name seems more clear to me than
> bdev_is_zone_aligned(). Has there already been a discussion about which
> name to use for this function?
> 
The reason I did s/bdev_is_zone_start/bdev_is_zone_aligned is that this
name makes more sense for also checking if a given size is a multiple of
zone sectors for e.g., used in PATCH 9:

-		if (len & (zone_sectors - 1)) {
+		if (!bdev_is_zone_aligned(bdev, len)) {

I felt `bdev_is_zone_aligned` fits the use case of checking if the
sector starts at the start of a zone and also check if a given length of
sectors also align with the zone sectors. bdev_is_zone_start does not
make the intention clear for the latter use case IMO.

But I am fine with going back to bdev_is_zone_start if you and Damien
feel strongly otherwise.
>> +        /*
>> +         * Non power-of-2 zone size support was added to remove the
>> +         * gap between zone capacity and zone size. Though it is
>> technically
>> +         * possible to have gaps in a non power-of-2 device, Linux
>> requires
>> +         * the zone size to be equal to zone capacity for non power-of-2
>> +         * zoned devices.
>> +         */
>> +        if (!is_power_of_2(zone->len) && zone->capacity < zone->len) {
>> +            pr_warn("%s: Invalid zone capacity for non power of 2
>> zone size",
>> +                disk->disk_name);
> 
> Given the severity of this error, shouldn't the zone capacity and length
> be reported in the error message?
> 
Ok.
> Thanks,
> 
> Bart.