References: <20230224125950.214779-1-nmi@metaspace.dk>
 <Y/jdkCR4ug7iZZ+L@x1-carbon> <Y/jfpKyntm9KSBVo@x1-carbon>
User-agent: mu4e 1.9.18; emacs 28.2.50
From:   Andreas Hindborg <nmi@metaspace.dk>
To:     Niklas Cassel <Niklas.Cassel@wdc.com>
Cc:     "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
        Hans Holmberg <Hans.Holmberg@wdc.com>,
        Matias =?utf-8?Q?Bj=C3=B8rling?= <Matias.Bjorling@wdc.com>,
        Ming Lei <ming.lei@redhat.com>, Jens Axboe <axboe@kernel.dk>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] block: ublk: enable zoned storage support
Date:   Fri, 24 Feb 2023 19:20:03 +0100
In-reply-to: <Y/jfpKyntm9KSBVo@x1-carbon>
Message-ID: <87edqec453.fsf@metaspace.dk>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


Niklas Cassel <Niklas.Cassel@wdc.com> writes:

> On Fri, Feb 24, 2023 at 04:53:52PM +0100, Niklas Cassel wrote:
>> Hello Andreas,
>>=20
>> On Fri, Feb 24, 2023 at 01:59:50PM +0100, Andreas Hindborg wrote:
>> > Add zoned storage support to ublk: report_zones and operations:
>> >  - REQ_OP_ZONE_OPEN
>> >  - REQ_OP_ZONE_CLOSE
>> >  - REQ_OP_ZONE_FINISH
>> >  - REQ_OP_ZONE_RESET
>> >=20
>> > This allows implementation of zoned storage devices in user space. An
>> > example user space implementation based on ubdsrv is available [1].
>> >=20
>> > [1] https://github.com/metaspace/ubdsrv/commit/14a2b708f74f70cfecb076d=
92e680dc718cc1f6d
>> >=20
>> > Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
>> > ---
>>=20
>> Did you try to build this patch with CONFIG_BLK_DEV_ZONED disabled?
>>=20
>> I got the following build errors when building it on top of vanilla v6.2
>> when CONFIG_BLK_DEV_ZONED is disabled:
>>=20
>> drivers/block/ublk_drv.c: In function =E2=80=98ublk_dev_param_basic_appl=
y=E2=80=99:
>> drivers/block/ublk_drv.c:221:28: error: =E2=80=98struct gendisk=E2=80=99=
 has no member named =E2=80=98nr_zones=E2=80=99
>>   221 |                 ub->ub_disk->nr_zones =3D p->dev_sectors / p->ch=
unk_sectors;
>>       |                            ^~
>> drivers/block/ublk_drv.c: In function =E2=80=98ublk_dev_param_zoned_appl=
y=E2=80=99:
>> drivers/block/ublk_drv.c:244:17: error: implicit declaration of function=
 =E2=80=98disk_set_max_active_zones=E2=80=99; did you mean =E2=80=98bdev_ma=
x_active_zones=E2=80=99? [-Werror=3Dimplicit-function-declaration]
>>   244 |                 disk_set_max_active_zones(ub->ub_disk, p->max_ac=
tive_zones);
>>       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~
>>       |                 bdev_max_active_zones
>> drivers/block/ublk_drv.c:245:17: error: implicit declaration of function=
 =E2=80=98disk_set_max_open_zones=E2=80=99; did you mean =E2=80=98bdev_max_=
open_zones=E2=80=99? [-Werror=3Dimplicit-function-declaration]
>>   245 |                 disk_set_max_open_zones(ub->ub_disk, p->max_open=
_zones);
>>=20
>
> The problem here is probably that blkdev.h does not have dummy functions =
for
> disk_set_max_active_zones() and disk_set_max_open_zones()
>
> in the:
> #else /* CONFIG_BLK_DEV_ZONED */
> case.
>
> I do see dummy functions in blkdev.h for disk_nr_zones(), disk_zone_is_se=
q()
> and disk_zone_no() when CONFIG_BLK_DEV_ZONED is not set.

Thanks for the comments Niklas :)

I completely forgot to test without CONFIG_BLK_DEV_ZONED =F0=9F=A4=A6 What =
is the
preferred resolution here? Conditionally remove the lines with #ifdef
rather than use if(IS_ENABLED(...))? Dummy functions would resolve lines
244 and 245, but would not help with assignment of nr_zones at 221 as
that will not exist when CONFIG_BLK_DEV_ZONED is disabled.

BR Andreas