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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <eed7d9ee-fd7f-e57c-598e-909dbb0d2380@samsung.com>
Date:   Thu, 28 Jul 2022 14:11:51 +0200
From:   Pankaj Raghav <p.raghav@...sung.com>
To:     Bart Van Assche <bvanassche@....org>,
        <damien.lemoal@...nsource.wdc.com>, <hch@....de>,
        <axboe@...nel.dk>, <snitzer@...nel.org>,
        <Johannes.Thumshirn@....com>
CC:     <matias.bjorling@....com>, <gost.dev@...sung.com>,
        <linux-kernel@...r.kernel.org>, <hare@...e.de>,
        <linux-block@...r.kernel.org>, <pankydev8@...il.com>,
        <jaegeuk@...nel.org>, <dm-devel@...hat.com>,
        <linux-nvme@...ts.infradead.org>,
        Luis Chamberlain <mcgrof@...nel.org>
Subject: Re: [PATCH v8 02/11] block: allow blk-zoned devices to have
 non-power-of-2 zone size

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.