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.
