Message-ID: <87a03e67-67f0-5ac7-41c7-5ccd393d0b66@kernel.org>
Date:   Thu, 29 Jun 2023 18:41:34 +0900
From:   Damien Le Moal <dlemoal@...nel.org>
To:     "Andreas Hindborg (Samsung)" <nmi@...aspace.dk>
Cc:     Ming Lei <ming.lei@...hat.com>,
        Hans Holmberg <Hans.Holmberg@....com>,
        Aravind Ramesh <Aravind.Ramesh@....com>,
        Jens Axboe <axboe@...nel.dk>,
        "open list:BLOCK LAYER" <linux-block@...r.kernel.org>,
        Christoph Hellwig <hch@...radead.org>,
        Matias Bjorling <Matias.Bjorling@....com>,
        open list <linux-kernel@...r.kernel.org>, gost.dev@...sung.com,
        Minwoo Im <minwoo.im.dev@...il.com>
Subject: Re: [PATCH v4 3/4] ublk: enable zoned storage support

On 6/29/23 16:50, Andreas Hindborg (Samsung) wrote:
>>>  UCLINUX (M68KNOMMU AND COLDFIRE)
>>> diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
>>> index 5b9d4aaebb81..c58dfd035557 100644
>>> --- a/drivers/block/Kconfig
>>> +++ b/drivers/block/Kconfig
>>> @@ -402,6 +402,10 @@ config BLKDEV_UBLK_LEGACY_OPCODES
>>>  	  suggested to enable N if your application(ublk server) switches to
>>>  	  ioctl command encoding.
>>>  
>>> +config BLK_DEV_UBLK_ZONED
>>> +	def_bool y
>>
>> This can be "bool" only.
> 
> I did it like this because I wanted BLK_DEV_UBLK_ZONED to be set
> automatically to "y" when BLK_DEV_UBLK and BLK_DEV_ZONED are enabled.
> BLK_DEV_UBLK_ZONED has no menu entry. If we change it to "bool" it will
> be default "n" and we would have to make it appear in menuconfig to be
> manually enabled.
> 
> Isn't it more sane if it is just unconditionally enabled when
> BLK_DEV_ZONED and BLK_DEV_UBLK is enabled?

Maybe something like this then:

diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
index 5b9d4aaebb81..105e1121bf5f 100644
--- a/drivers/block/Kconfig
+++ b/drivers/block/Kconfig
@@ -370,9 +370,13 @@ config BLK_DEV_RBD

          If unsure, say N.

+config BLK_DEV_UBLK_ZONED
+       bool
+
 config BLK_DEV_UBLK
        tristate "Userspace block driver (Experimental)"
        select IO_URING
+       select BLK_DEV_UBLK_ZONED if BLK_DEV_ZONED
        help
          io_uring based userspace block driver. Together with ublk server, ublk
          has been working well, but interface with userspace or command data
>>> +		ub->ub_disk->nr_zones = p->dev_sectors / p->chunk_sectors;
>>> +}
>>> +
>>> +void ublk_dev_param_zoned_apply(struct ublk_device *ub)
>>> +{
>>> +	const struct ublk_param_zoned *p = &ub->params.zoned;
>>> +
>>> +	if (ub->dev_info.flags & UBLK_F_ZONED) {
>>> +		disk_set_max_active_zones(ub->ub_disk, p->max_active_zones);
>>> +		disk_set_max_open_zones(ub->ub_disk, p->max_open_zones);
>>
>> You do not need to check if the max_active_zones and max_open_zones values are
>> sensible ? E.g. what if they are larger than the number of zones ?
> 
> I don't think it will be a problem. I assume you can set them to 0 to
> indicate infinite. If user space sets them to more than the actual
> number of available zones (that user space set up also), user space will
> have to handle that when it gets a zone request for a zone outside the
> logical drive LBA space. I don't think the kernel has to care. Will this
> break anything kernel side?

zonefs and btrfs look at these limits. So I would rather prefer seeing sensible
values. 0 == no limit is fine. But seeing maz or moz > nr_zones for the device
is just too strange and I would prefer an error in that case to let the user
know it is not doing something right. Getting moz > maz is also nonsense that
needs to be checked. There is one case we could silently change: if maz ==
nr_zones or moz == nr_zones, then that essentially means "no limit", so we could
use 0 to let the in-kernel users that they do not need to care about the limits.

> 
>>
>>> +	}
>>> +}
>>> +
>>> +int ublk_revalidate_disk_zones(struct gendisk *disk)
>>> +{
>>> +	return blk_revalidate_disk_zones(disk, NULL);
>>> +}
>>
>> I do not think this helper is needed at all (see below comment on the call site).
> 
> This is required because the prototype for `blk_revalidate_disk_zones()`
> is not defined when BLK_DEV_ZONED is not defined. Without this helper
> for which the prototype is always defined, we will have a compile error
> at the call site.

You have this hunk in ublk_ctrl_start_dev:

disk_set_zoned(disk, BLK_ZONED_HM);
blk_queue_required_elevator_features(disk->queue, ELEVATOR_F_ZBD_SEQ_WRITE);
ret = ublk_revalidate_disk_zones(disk);
if (ret)
	goto out_put_disk;

And that is called under "if (IS_ENABLED(CONFIG_BLK_DEV_ZONED)". So I do not see
how you can get compile errors using directly blk_revalidate_disk_zones(). The
entire hunk above should be a helper function I think.

>>> +
>>> +	if (IS_ENABLED(CONFIG_BLK_DEV_ZONED))
>>> +		ublk_set_nr_zones(ub);
>>
>> So if the user is attempting to setup a zoned drive but the kernel does not have
>> CONFIG_BLK_DEV_ZONED=y, the user setup will be silently ignored. Not exactly
>> nice I think, unless I am missing something.
> 
> We have this in `ublk_ctrl_add_dev()`:
> 
> 	if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED))
> 		ub->dev_info.flags &= ~UBLK_F_ZONED;
> 
> User space is supposed to check the flags after the call to know the
> kernel capabilities.

And you trust user space to be always correct ? I never do :)
So definitely, check and error if not supported. No silently clearing the device
zoned flag.


>> Also, repeating that "if (IS_ENABLED(CONFIG_BLK_DEV_ZONED))" for all zone
>> related functions is very verbose. Stub the functions in ublk_drv.h. That will
>> make the main C code lighter.
> 
> Not sure what you mean "stub the functions". Like so:
> 
> @@ -216,8 +216,7 @@ static void ublk_dev_param_basic_apply(struct ublk_device *ub)
>  
>  	set_capacity(ub->ub_disk, p->dev_sectors);
>  
> -	if (IS_ENABLED(CONFIG_BLK_DEV_ZONED))
> -		ublk_set_nr_zones(ub);
> +	ublk_config_nr_zones(ub);
>  }
> 
> And then in the header:
> 
> void ublk_config_nr_zones(struct ublk_device *ub)
> {
> 	if (IS_ENABLED(CONFIG_BLK_DEV_ZONED))
> 		ublk_set_nr_zones(ub);
> }
> 
> Or something else?

#ifndef CONFIG_BLK_DEV_ZONED
static inline void ublk_set_nr_zones() {}
#endif

That avoid repeating that if (IS_ENABLED(CONFIG_BLK_DEV_ZONED)) all over the
place. The stubbed functions can be called only for "if (ub->dev_info.flags &
UBLK_F_ZONED", which is OK since this flag is never supposed to be accepted and
set for the CONFIG_BLK_DEV_ZONED=n case.

>>> +		disk_set_zoned(disk, BLK_ZONED_HM);
>>> +		blk_queue_required_elevator_features(disk->queue, ELEVATOR_F_ZBD_SEQ_WRITE);
>>> +		ret = ublk_revalidate_disk_zones(disk);
>>> +		if (ret)
>>> +			goto out_put_disk;
>>
>> This should be all a helper ublk_set_zoned() or something.
> 
> Ok 👍 Unfortunately this block of code is split up in the zone append
> patch. I will see what I can do, maybe 2 functions.

Beware that I just posted patches that require zone size (chunk_sectors limit)
and max append sectors limit to be set *before* calling
blk_revalidate_disk_zones(). Otherwise, you will get an error.

>>>  	get_device(&ub->cdev_dev);
>>>  	ub->dev_info.state = UBLK_S_DEV_LIVE;
>>>  	ret = add_disk(disk);
>>> @@ -1997,6 +2046,9 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd)
>>>  	if (ub->dev_info.flags & UBLK_F_USER_COPY)
>>>  		ub->dev_info.flags &= ~UBLK_F_NEED_GET_DATA;
>>>  
>>> +	if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED))
>>> +		ub->dev_info.flags &= ~UBLK_F_ZONED;
>>
>> Arg, no. The user should be notified with an error that he/she is attempting to
>> create a zoned device that cannot be supported.
> 
> As I wrote above, this is part of the ublk uapi. This is the way user
> space does kernel capability check. User space will check the flags when
> this call returns. @Ming did I understand this correctly or did I miss something?

OK. So this is like a virtio capability flags thing ? Negotiation between device
and driver ?

>>> diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
>>> index 471b3b983045..436525afffe8 100644
>>> --- a/include/uapi/linux/ublk_cmd.h
>>> +++ b/include/uapi/linux/ublk_cmd.h
>>> @@ -176,6 +176,11 @@
>>>  /* Copy between request and user buffer by pread()/pwrite() */
>>>  #define UBLK_F_USER_COPY	(1UL << 7)
>>>  
>>> +/*
>>> + * Enable zoned device support
>>
>> Isn't this for "Indicate that the device is zoned" ?
> 
> User space sets this flag when setting up the device to enable zoned
> storage support. Kernel may reject it. I think the wording is
> sufficient. Could be updated to:
> 
> "User space sets this flag when setting up the device to request zoned
>  storage support. Kernel may deny the request by clearing the flag
>  during setup."

I really have a big problem with this. If the user driver does not check that
the flag was cleared, the user will assume that the device is zoned, but it is
not. This is not super nice... I would really prefer just failing the devie
creation if zoned was requested but cannot be satisfied, either because the
kernel does not support zoned block devices or when the user specified something
nonsensical.


-- 
Damien Le Moal
Western Digital Research

Powered by blists - more mailing lists