Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
IronPort-SDR: m6YKQBHMb80HI9zXLBGMWEyF133C83TTtvBRe32+d4dc8Yw5hCz68piP/V0xFAadknitXE1kQq
 v39596MNzfNt0TB/K4z09Uiwhi3I4Zk07RhdaGw0mxaMmld57oL0PTx3Nmn1HqGPYTgqqXsrO7
 EEPkqrxadaoIT8/9jAL1oGutma6NBruSo0jxzAgETaCQ2FtpiVTyhiwKGi9v+O9mhKp/zAKOkx
 tDZ2L7USpcenVC1/knYAThGNKwx58bONniwRCkDjUuVNnEcX9F2PWfnZ5XMxrnP/nFordrUMCN
 71lNUkRw/N+na2iWL92oBHhf
IronPort-SDR: psyq/078KpDJTpjqVuShOwUDNSFat5G+nmnI1go/qJSRfbwJvmBUH7aKe8MkYm1LW8v3RLU0xH
 hXuyEDeNY6wDQ1lv/uhkPwFg8QwZl+HlzpGIilH8uOunBaOgBFTf0kpcAne5wKRs8wniq9PZ8k
 74b8EqZWnTaTp5Zzr0sDiZGnt4ozYUkx6ZPlrV5DI7irFvNdn5m9P8hDKLeC+eZz3dTAPx34Y0
 /QaANidmR5rL+F8FdKeaeuiTHYb47oyQ7LPUh/PGQE0tA5CNRb7O/sc8rq3Lr+9Me13oAX2yC1
 Dk0=
WDCIronportException: Internal
Message-ID: <ab75c1ca-986f-c3af-6c8c-c2c5b7e40bbd@opensource.wdc.com>
Date:   Fri, 17 Jun 2022 08:30:46 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
Subject: Re: [dm-devel] [PATCH v7 02/13] block: allow blk-zoned devices to
 have non-power-of-2 zone size
Content-Language: en-US
To:     Pankaj Raghav <p.raghav@samsung.com>,
        Bart Van Assche <bvanassche@acm.org>, hch@lst.de,
        snitzer@redhat.com, axboe@kernel.dk
Cc:     pankydev8@gmail.com, gost.dev@samsung.com,
        jiangbo.365@bytedance.com, linux-nvme@lists.infradead.org,
        linux-kernel@vger.kernel.org, linux-block@vger.kernel.org,
        dm-devel@redhat.com, Luis Chamberlain <mcgrof@kernel.org>,
        jonathan.derrick@linux.dev, Johannes.Thumshirn@wdc.com,
        dsterba@suse.com, jaegeuk@kernel.org
References: <20220615101920.329421-1-p.raghav@samsung.com>
 <CGME20220615101931eucas1p15ed09ae433a2c378b599e9086130d8eb@eucas1p1.samsung.com>
 <20220615101920.329421-3-p.raghav@samsung.com>
 <857c444a-02b9-9cef-0c5b-2ecdb2fd46f6@acm.org>
 <e04db101-5628-2a1d-6b5c-997090484d7d@samsung.com>
From:   Damien Le Moal <damien.lemoal@opensource.wdc.com>
Organization: Western Digital Research
In-Reply-To: <e04db101-5628-2a1d-6b5c-997090484d7d@samsung.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On 6/16/22 19:09, Pankaj Raghav wrote:
> On 2022-06-15 22:28, Bart Van Assche wrote:
> isk_name, zone->len);
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (zone->len =3D=3D 0) {
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 p=
r_warn("%s: Invalid zone size", disk->disk_name);
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 r=
eturn -ENODEV;
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }
>>> +
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /*
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * Don't allow zoned=
 device with non power_of_2 zone size with
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * zone capacity les=
s than zone size.
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 */
>>
>=20
>> Please change "power_of_2" into "power-of-2".
>>
> Ok.
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (!is_power_of_2(zone->=
len) && zone->capacity < zone->len) {
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 p=
r_warn("%s: Invalid zone capacity for non power of 2
>>> zone size",
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0 disk->disk_name);
>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0 return -ENODEV;
>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }
>>
>> The above check seems wrong to me. I don't see why devices that report=
 a
>> capacity that is less than the zone size should be rejected.
>>
> This was brought up by Damien during previous reviews. The argument was
> that the reason to allow non power-of-2 zoned device is to remove the
> gaps between zone size and zone capacity. Allowing a npo2 zone size wit=
h
> a different capacity, even though it is technically possible, it does
> not make any practical sense. That is why this check was introduced.
> Does that answer your question?

Add a comment explaining this restriction, clearly mentioning that it is =
a
Linux restrictions and not mandated by the specifications.

>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /*
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * Division is used =
to calculate nr_zones for both power_of_2
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * and non power_of_=
2 zone sizes as it is not in the hot path.
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 */
>>
>> Shouldn't the above comment be moved to the patch description? I'm not
>> sure whether having such a comment in the source code is valuable.
>>
> Yeah, I will remove it. Maybe it is very obvious at this point.
>>> +static inline sector_t blk_queue_offset_from_zone_start(struct
>>> request_queue *q,
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 sector_t sec)
>>> +{
>>> +=C2=A0=C2=A0=C2=A0 sector_t zone_sectors =3D blk_queue_zone_sectors(=
q);
>>> +=C2=A0=C2=A0=C2=A0 u64 remainder =3D 0;
>>> +
>>> +=C2=A0=C2=A0=C2=A0 if (!blk_queue_is_zoned(q))
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return false;
>>
>> "return false" should only occur in functions returning a boolean. Thi=
s
>> function returns type sector_t.
>>
> Good catch. It was a copy paste mistake. Fixed it.
>> Thanks,
>>
>> Bart.
>=20
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://listman.redhat.com/mailman/listinfo/dm-devel


--=20
Damien Le Moal
Western Digital Research