[v2] block: ublk: enable zoned storage support

  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

This allows implementation of zoned storage devices in user space. An
example user space implementation based on ubdsrv is available [1].

[1] https://github.com/metaspace/ubdsrv/commit/14a2b708f74f70cfecb076d92e680dc718cc1f6d

Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
---
Changes since v1:
 - Fixed conditional compilation bug
 - Refactored to collect conditional code additions together
 - Fixed style errors
 - Zero stack allocated value used for zone report

Reported-by: Niklas Cassel <Niklas.Cassel@wdc.com>
Reported-by: kernel test robot <lkp@intel.com>
Link: https://lore.kernel.org/oe-kbuild-all/202302250222.XOrw9j6z-lkp@intel.com/
v1: https://lore.kernel.org/linux-block/20230224125950.214779-1-nmi@metaspace.dk/

 drivers/block/ublk_drv.c      | 150 ++++++++++++++++++++++++++++++++--
 include/uapi/linux/ublk_cmd.h |  18 ++++
 2 files changed, 162 insertions(+), 6 deletions(-)


base-commit: c9c3395d5e3dcc6daee66c6908354d47bf98cb0c
  

On Fri, Feb 24, 2023 at 09:05:01PM +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
> 
> This allows implementation of zoned storage devices in user space. An
> example user space implementation based on ubdsrv is available [1].
> 
> [1] https://github.com/metaspace/ubdsrv/commit/14a2b708f74f70cfecb076d92e680dc718cc1f6d
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
> ---
> Changes since v1:
>  - Fixed conditional compilation bug
>  - Refactored to collect conditional code additions together
>  - Fixed style errors
>  - Zero stack allocated value used for zone report
> 
> Reported-by: Niklas Cassel <Niklas.Cassel@wdc.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Link: https://lore.kernel.org/oe-kbuild-all/202302250222.XOrw9j6z-lkp@intel.com/
> v1: https://lore.kernel.org/linux-block/20230224125950.214779-1-nmi@metaspace.dk/
> 
>  drivers/block/ublk_drv.c      | 150 ++++++++++++++++++++++++++++++++--
>  include/uapi/linux/ublk_cmd.h |  18 ++++
>  2 files changed, 162 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 6368b56eacf1..37e516903867 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -20,6 +20,7 @@
>  #include <linux/major.h>
>  #include <linux/wait.h>
>  #include <linux/blkdev.h>
> +#include <linux/blkzoned.h>
>  #include <linux/init.h>
>  #include <linux/swap.h>
>  #include <linux/slab.h>
> @@ -51,10 +52,12 @@
>  		| UBLK_F_URING_CMD_COMP_IN_TASK \
>  		| UBLK_F_NEED_GET_DATA \
>  		| UBLK_F_USER_RECOVERY \
> -		| UBLK_F_USER_RECOVERY_REISSUE)
> +		| UBLK_F_USER_RECOVERY_REISSUE \
> +		| UBLK_F_ZONED)
>  
>  /* All UBLK_PARAM_TYPE_* should be included here */
> -#define UBLK_PARAM_TYPE_ALL (UBLK_PARAM_TYPE_BASIC | UBLK_PARAM_TYPE_DISCARD)
> +#define UBLK_PARAM_TYPE_ALL (UBLK_PARAM_TYPE_BASIC | UBLK_PARAM_TYPE_DISCARD \
> +			     | UBLK_PARAM_TYPE_ZONED)
>  
>  struct ublk_rq_data {
>  	struct llist_node node;
> @@ -187,6 +190,98 @@ static DEFINE_MUTEX(ublk_ctl_mutex);
>  
>  static struct miscdevice ublk_misc;
>  
> +#ifdef CONFIG_BLK_DEV_ZONED
> +static void ublk_set_nr_zones(struct ublk_device *ub)
> +{
> +	const struct ublk_param_basic *p = &ub->params.basic;
> +
> +	if (ub->dev_info.flags & UBLK_F_ZONED && p->chunk_sectors)
> +		ub->ub_disk->nr_zones = p->dev_sectors / p->chunk_sectors;
> +}
> +
> +static 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);
> +	}
> +}
> +
> +static int ublk_revalidate_disk_zones(struct gendisk *disk)
> +{
> +	return blk_revalidate_disk_zones(disk, NULL);
> +}
> +
> +static int ublk_report_zones(struct gendisk *disk, sector_t sector,
> +			     unsigned int nr_zones, report_zones_cb cb,
> +			     void *data)
> +{
> +	struct ublk_device *ub;
> +	unsigned int zone_size;
> +	unsigned int first_zone;
> +	int ret = 0;
> +
> +	ub = disk->private_data;
> +
> +	if (!(ub->dev_info.flags & UBLK_F_ZONED))
> +		return -EINVAL;
> +
> +	zone_size = disk->queue->limits.chunk_sectors;
> +	first_zone = sector >> ilog2(zone_size);
> +	nr_zones = min(ub->ub_disk->nr_zones - first_zone, nr_zones);
> +
> +	for (unsigned int i = 0; i < nr_zones; i++) {
> +		struct request *req;
> +		blk_status_t status;
> +		struct blk_zone info = {0};
> +
> +		req = blk_mq_alloc_request(disk->queue, REQ_OP_DRV_IN, 0);
> +
> +		if (IS_ERR(req)) {
> +			ret = PTR_ERR(req);
> +			goto out;
> +		}
> +
> +		req->__sector = sector;
> +
> +		ret = blk_rq_map_kern(disk->queue, req, &info, sizeof(info),
> +				      GFP_KERNEL);
> +
> +		if (ret)
> +			goto out;
> +
> +		status = blk_execute_rq(req, 0);
> +		ret = blk_status_to_errno(status);
> +		if (ret)
> +			goto out;
> +
> +		blk_mq_free_request(req);
> +
> +		ret = cb(&info, i, data);
> +		if (ret)
> +			goto out;
> +
> +		/* A zero length zone means don't ask for more zones */
> +		if (!info.len) {
> +			nr_zones = i;
> +			break;
> +		}
> +
> +		sector += zone_size;
> +	}
> +	ret = nr_zones;
> +
> + out:
> +	return ret;
> +}
> +#else
> +void ublk_set_nr_zones(struct ublk_device *ub);
> +void ublk_dev_param_zoned_apply(struct ublk_device *ub);
> +int ublk_revalidate_disk_zones(struct gendisk *disk);

These are declarations, shouldn't they be dummy definitions instead?

e.g.
static int ublk_revalidate_disk_zones(struct gendisk *disk) { return -EOPNOTSUPP; };


It would be nice if they could be avoided altogether.

Looking how e.g. null-blk and btrfs has solved this:
https://github.com/torvalds/linux/blob/v6.2/fs/btrfs/Makefile#L39
https://github.com/torvalds/linux/blob/v6.2/drivers/block/null_blk/Makefile#L11

They have put the zoned stuff in a separate C file that is only compiled
when CONFIG_BLK_DEV_ZONED is set.

I'm not sure if a similar design is desired for ublk or not.

However, if a similar design pattern was used, it could probably avoid
some of these unpleasant dummy definitions altogether.


Kind regards,
Niklas

> +#endif
> +
  

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

> On Fri, Feb 24, 2023 at 09:05:01PM +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
>> 
>> This allows implementation of zoned storage devices in user space. An
>> example user space implementation based on ubdsrv is available [1].
>> 
>> [1] https://github.com/metaspace/ubdsrv/commit/14a2b708f74f70cfecb076d92e680dc718cc1f6d
>> 
>> Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
>> ---
>> Changes since v1:
>>  - Fixed conditional compilation bug
>>  - Refactored to collect conditional code additions together
>>  - Fixed style errors
>>  - Zero stack allocated value used for zone report
>> 
>> Reported-by: Niklas Cassel <Niklas.Cassel@wdc.com>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Link: https://lore.kernel.org/oe-kbuild-all/202302250222.XOrw9j6z-lkp@intel.com/
>> v1: https://lore.kernel.org/linux-block/20230224125950.214779-1-nmi@metaspace.dk/
>> 
>>  drivers/block/ublk_drv.c      | 150 ++++++++++++++++++++++++++++++++--
>>  include/uapi/linux/ublk_cmd.h |  18 ++++
>>  2 files changed, 162 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
>> index 6368b56eacf1..37e516903867 100644
>> --- a/drivers/block/ublk_drv.c
>> +++ b/drivers/block/ublk_drv.c
>> @@ -20,6 +20,7 @@
>>  #include <linux/major.h>
>>  #include <linux/wait.h>
>>  #include <linux/blkdev.h>
>> +#include <linux/blkzoned.h>
>>  #include <linux/init.h>
>>  #include <linux/swap.h>
>>  #include <linux/slab.h>
>> @@ -51,10 +52,12 @@
>>  		| UBLK_F_URING_CMD_COMP_IN_TASK \
>>  		| UBLK_F_NEED_GET_DATA \
>>  		| UBLK_F_USER_RECOVERY \
>> -		| UBLK_F_USER_RECOVERY_REISSUE)
>> +		| UBLK_F_USER_RECOVERY_REISSUE \
>> +		| UBLK_F_ZONED)
>>  
>>  /* All UBLK_PARAM_TYPE_* should be included here */
>> -#define UBLK_PARAM_TYPE_ALL (UBLK_PARAM_TYPE_BASIC | UBLK_PARAM_TYPE_DISCARD)
>> +#define UBLK_PARAM_TYPE_ALL (UBLK_PARAM_TYPE_BASIC | UBLK_PARAM_TYPE_DISCARD \
>> +			     | UBLK_PARAM_TYPE_ZONED)
>>  
>>  struct ublk_rq_data {
>>  	struct llist_node node;
>> @@ -187,6 +190,98 @@ static DEFINE_MUTEX(ublk_ctl_mutex);
>>  
>>  static struct miscdevice ublk_misc;
>>  
>> +#ifdef CONFIG_BLK_DEV_ZONED
>> +static void ublk_set_nr_zones(struct ublk_device *ub)
>> +{
>> +	const struct ublk_param_basic *p = &ub->params.basic;
>> +
>> +	if (ub->dev_info.flags & UBLK_F_ZONED && p->chunk_sectors)
>> +		ub->ub_disk->nr_zones = p->dev_sectors / p->chunk_sectors;
>> +}
>> +
>> +static 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);
>> +	}
>> +}
>> +
>> +static int ublk_revalidate_disk_zones(struct gendisk *disk)
>> +{
>> +	return blk_revalidate_disk_zones(disk, NULL);
>> +}
>> +
>> +static int ublk_report_zones(struct gendisk *disk, sector_t sector,
>> +			     unsigned int nr_zones, report_zones_cb cb,
>> +			     void *data)
>> +{
>> +	struct ublk_device *ub;
>> +	unsigned int zone_size;
>> +	unsigned int first_zone;
>> +	int ret = 0;
>> +
>> +	ub = disk->private_data;
>> +
>> +	if (!(ub->dev_info.flags & UBLK_F_ZONED))
>> +		return -EINVAL;
>> +
>> +	zone_size = disk->queue->limits.chunk_sectors;
>> +	first_zone = sector >> ilog2(zone_size);
>> +	nr_zones = min(ub->ub_disk->nr_zones - first_zone, nr_zones);
>> +
>> +	for (unsigned int i = 0; i < nr_zones; i++) {
>> +		struct request *req;
>> +		blk_status_t status;
>> +		struct blk_zone info = {0};
>> +
>> +		req = blk_mq_alloc_request(disk->queue, REQ_OP_DRV_IN, 0);
>> +
>> +		if (IS_ERR(req)) {
>> +			ret = PTR_ERR(req);
>> +			goto out;
>> +		}
>> +
>> +		req->__sector = sector;
>> +
>> +		ret = blk_rq_map_kern(disk->queue, req, &info, sizeof(info),
>> +				      GFP_KERNEL);
>> +
>> +		if (ret)
>> +			goto out;
>> +
>> +		status = blk_execute_rq(req, 0);
>> +		ret = blk_status_to_errno(status);
>> +		if (ret)
>> +			goto out;
>> +
>> +		blk_mq_free_request(req);
>> +
>> +		ret = cb(&info, i, data);
>> +		if (ret)
>> +			goto out;
>> +
>> +		/* A zero length zone means don't ask for more zones */
>> +		if (!info.len) {
>> +			nr_zones = i;
>> +			break;
>> +		}
>> +
>> +		sector += zone_size;
>> +	}
>> +	ret = nr_zones;
>> +
>> + out:
>> +	return ret;
>> +}
>> +#else
>> +void ublk_set_nr_zones(struct ublk_device *ub);
>> +void ublk_dev_param_zoned_apply(struct ublk_device *ub);
>> +int ublk_revalidate_disk_zones(struct gendisk *disk);
>
> These are declarations, shouldn't they be dummy definitions instead?

I looked at how nvme host defines nvme_revalidate_zones() when I did
this. The functions become undefined symbols but because the call sites
are optimized out they go away.

>
> e.g.
> static int ublk_revalidate_disk_zones(struct gendisk *disk) { return -EOPNOTSUPP; };

Not sure how this is better?

>
>
> It would be nice if they could be avoided altogether.
>
> Looking how e.g. null-blk and btrfs has solved this:
> https://github.com/torvalds/linux/blob/v6.2/fs/btrfs/Makefile#L39
> https://github.com/torvalds/linux/blob/v6.2/drivers/block/null_blk/Makefile#L11
>
> They have put the zoned stuff in a separate C file that is only compiled
> when CONFIG_BLK_DEV_ZONED is set.
>
> I'm not sure if a similar design is desired for ublk or not.
>
> However, if a similar design pattern was used, it could probably avoid
> some of these unpleasant dummy definitions altogether.

This is the same as I do here, except I put the declarations in the c
file instead of a header. I did this for two reasons 1) there is no ublk
header besides the uapi header (I would add a header just for this), 2)
the declarations need only exist inside ublk_drv.c. For btrfs, null_blk,
nvme, the declarations go in a header file and the functions in question
do not have static linkage.

I could move the function declarations out of the #else block, but then
they would need to be declared static and that gives a compiler warning
when the implementation is not present.

BR Andreas
  

On Mon, Feb 27, 2023 at 12:59:45PM +0100, Andreas Hindborg wrote:

(snip)

> >> +#else
> >> +void ublk_set_nr_zones(struct ublk_device *ub);
> >> +void ublk_dev_param_zoned_apply(struct ublk_device *ub);
> >> +int ublk_revalidate_disk_zones(struct gendisk *disk);
> >
> > These are declarations, shouldn't they be dummy definitions instead?
> 
> I looked at how nvme host defines nvme_revalidate_zones() when I did
> this. The functions become undefined symbols but because the call sites
> are optimized out they go away.

Looking at e.g. nvme_revalidate_zones

$ git grep nvme_revalidate_zones
drivers/nvme/host/core.c:               ret = nvme_revalidate_zones(ns);
drivers/nvme/host/nvme.h:int nvme_revalidate_zones(struct nvme_ns *ns);
drivers/nvme/host/zns.c:int nvme_revalidate_zones(struct nvme_ns *ns)

The function is declared in nvme.h, but like you say, without any definition.

zns.c provides a definition, but that file is only build if
CONFIG_BLK_DEV_ZONED is set.


> > https://github.com/torvalds/linux/blob/v6.2/fs/btrfs/Makefile#L39
> > https://github.com/torvalds/linux/blob/v6.2/drivers/block/null_blk/Makefile#L11
> >
> > They have put the zoned stuff in a separate C file that is only compiled
> > when CONFIG_BLK_DEV_ZONED is set.
> >
> > I'm not sure if a similar design is desired for ublk or not.
> >
> > However, if a similar design pattern was used, it could probably avoid
> > some of these unpleasant dummy definitions altogether.
> 
> This is the same as I do here, except I put the declarations in the c
> file instead of a header. I did this for two reasons 1) there is no ublk
> header besides the uapi header (I would add a header just for this), 2)
> the declarations need only exist inside ublk_drv.c. For btrfs, null_blk,
> nvme, the declarations go in a header file and the functions in question
> do not have static linkage.
> 
> I could move the function declarations out of the #else block, but then
> they would need to be declared static and that gives a compiler warning
> when the implementation is not present.

I would love to hear someone else's opinion about this as well, but I do
think that having #ifdef and #else with both a declaration and a definition
in the C file is quite ugly.

If having an internal only header (in the same directory as the C file),
makes the C code easier to read, I'm all for it.

It seems to work for nvme to only have a declaration in an internal header
file, and only provide a definition if CONFIG_BLK_DEV_ZONED is set,
presumably without giving a warning. Perhaps ublk can do the same?


Kind regards,
Niklas
  

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

> On Mon, Feb 27, 2023 at 12:59:45PM +0100, Andreas Hindborg wrote:
>
> (snip)
>
>> >> +#else
>> >> +void ublk_set_nr_zones(struct ublk_device *ub);
>> >> +void ublk_dev_param_zoned_apply(struct ublk_device *ub);
>> >> +int ublk_revalidate_disk_zones(struct gendisk *disk);
>> >
>> > These are declarations, shouldn't they be dummy definitions instead?
>> 
>> I looked at how nvme host defines nvme_revalidate_zones() when I did
>> this. The functions become undefined symbols but because the call sites
>> are optimized out they go away.
>
> Looking at e.g. nvme_revalidate_zones
>
> $ git grep nvme_revalidate_zones
> drivers/nvme/host/core.c:               ret = nvme_revalidate_zones(ns);
> drivers/nvme/host/nvme.h:int nvme_revalidate_zones(struct nvme_ns *ns);
> drivers/nvme/host/zns.c:int nvme_revalidate_zones(struct nvme_ns *ns)
>
> The function is declared in nvme.h, but like you say, without any definition.
>
> zns.c provides a definition, but that file is only build if
> CONFIG_BLK_DEV_ZONED is set.
>
>
>> > https://github.com/torvalds/linux/blob/v6.2/fs/btrfs/Makefile#L39
>> > https://github.com/torvalds/linux/blob/v6.2/drivers/block/null_blk/Makefile#L11
>> >
>> > They have put the zoned stuff in a separate C file that is only compiled
>> > when CONFIG_BLK_DEV_ZONED is set.
>> >
>> > I'm not sure if a similar design is desired for ublk or not.
>> >
>> > However, if a similar design pattern was used, it could probably avoid
>> > some of these unpleasant dummy definitions altogether.
>> 
>> This is the same as I do here, except I put the declarations in the c
>> file instead of a header. I did this for two reasons 1) there is no ublk
>> header besides the uapi header (I would add a header just for this), 2)
>> the declarations need only exist inside ublk_drv.c. For btrfs, null_blk,
>> nvme, the declarations go in a header file and the functions in question
>> do not have static linkage.
>> 
>> I could move the function declarations out of the #else block, but then
>> they would need to be declared static and that gives a compiler warning
>> when the implementation is not present.
>
> I would love to hear someone else's opinion about this as well, but I do
> think that having #ifdef and #else with both a declaration and a definition
> in the C file is quite ugly.
>
> If having an internal only header (in the same directory as the C file),
> makes the C code easier to read, I'm all for it.
>
> It seems to work for nvme to only have a declaration in an internal header
> file, and only provide a definition if CONFIG_BLK_DEV_ZONED is set,
> presumably without giving a warning. Perhaps ublk can do the same?

Sure, I can do that if that is preferred. As I said the result will be
he same with he exception that the function symbols will not have static
linkage when defined in a separate file with declarations in a header.

I will let this version sit for a while to see if anyone has an opinion,
and then I will ship a new version next week.

BR Andreas
  

On 23-02-24 21:05:01, Andreas Hindborg wrote:
> +static int ublk_report_zones(struct gendisk *disk, sector_t sector,
> +			     unsigned int nr_zones, report_zones_cb cb,
> +			     void *data)
> +{
> +	struct ublk_device *ub;
> +	unsigned int zone_size;
> +	unsigned int first_zone;
> +	int ret = 0;
> +
> +	ub = disk->private_data;
> +
> +	if (!(ub->dev_info.flags & UBLK_F_ZONED))
> +		return -EINVAL;
> +
> +	zone_size = disk->queue->limits.chunk_sectors;
> +	first_zone = sector >> ilog2(zone_size);
> +	nr_zones = min(ub->ub_disk->nr_zones - first_zone, nr_zones);
> +
> +	for (unsigned int i = 0; i < nr_zones; i++) {
> +		struct request *req;
> +		blk_status_t status;
> +		struct blk_zone info = {0};
> +
> +		req = blk_mq_alloc_request(disk->queue, REQ_OP_DRV_IN, 0);
> +
> +		if (IS_ERR(req)) {
> +			ret = PTR_ERR(req);
> +			goto out;
> +		}

Can we just return directly just like above (-EINVAL)?

if (IS_ERR(req))
	return PTR_ERR(req);

> +
> +		req->__sector = sector;
> +
> +		ret = blk_rq_map_kern(disk->queue, req, &info, sizeof(info),
> +				      GFP_KERNEL);
> +
> +		if (ret)
> +			goto out;

If we really have to use goto here, then I think we have choices:
put blk_mq_free_request(req); here or put it to the out: area.

> +
> +		status = blk_execute_rq(req, 0);
> +		ret = blk_status_to_errno(status);
> +		if (ret)
> +			goto out;
> +
> +		blk_mq_free_request(req);
> +
> +		ret = cb(&info, i, data);
> +		if (ret)
> +			goto out;
> +
> +		/* A zero length zone means don't ask for more zones */
> +		if (!info.len) {
> +			nr_zones = i;
> +			break;
> +		}
> +
> +		sector += zone_size;
> +	}
> +	ret = nr_zones;
> +
> + out:
> +	return ret;
> +}
  

Minwoo Im <minwoo.im.dev@gmail.com> writes:

> On 23-02-24 21:05:01, Andreas Hindborg wrote:
>> +static int ublk_report_zones(struct gendisk *disk, sector_t sector,
>> +			     unsigned int nr_zones, report_zones_cb cb,
>> +			     void *data)
>> +{
>> +	struct ublk_device *ub;
>> +	unsigned int zone_size;
>> +	unsigned int first_zone;
>> +	int ret = 0;
>> +
>> +	ub = disk->private_data;
>> +
>> +	if (!(ub->dev_info.flags & UBLK_F_ZONED))
>> +		return -EINVAL;
>> +
>> +	zone_size = disk->queue->limits.chunk_sectors;
>> +	first_zone = sector >> ilog2(zone_size);
>> +	nr_zones = min(ub->ub_disk->nr_zones - first_zone, nr_zones);
>> +
>> +	for (unsigned int i = 0; i < nr_zones; i++) {
>> +		struct request *req;
>> +		blk_status_t status;
>> +		struct blk_zone info = {0};
>> +
>> +		req = blk_mq_alloc_request(disk->queue, REQ_OP_DRV_IN, 0);
>> +
>> +		if (IS_ERR(req)) {
>> +			ret = PTR_ERR(req);
>> +			goto out;
>> +		}
>
> Can we just return directly just like above (-EINVAL)?
>
> if (IS_ERR(req))
> 	return PTR_ERR(req);

Yes, I agree.

>
>> +
>> +		req->__sector = sector;
>> +
>> +		ret = blk_rq_map_kern(disk->queue, req, &info, sizeof(info),
>> +				      GFP_KERNEL);
>> +
>> +		if (ret)
>> +			goto out;
>
> If we really have to use goto here, then I think we have choices:
> put blk_mq_free_request(req); here or put it to the out: area.

Lets free here and drop the goto.

Thanks,
Andreas

>> +
>> +		status = blk_execute_rq(req, 0);
>> +		ret = blk_status_to_errno(status);
>> +		if (ret)
>> +			goto out;
>> +
>> +		blk_mq_free_request(req);
>> +
>> +		ret = cb(&info, i, data);
>> +		if (ret)
>> +			goto out;
>> +
>> +		/* A zero length zone means don't ask for more zones */
>> +		if (!info.len) {
>> +			nr_zones = i;
>> +			break;
>> +		}
>> +
>> +		sector += zone_size;
>> +	}
>> +	ret = nr_zones;
>> +
>> + out:
>> +	return ret;
>> +}
  

On Fri, Feb 24, 2023 at 9:06 PM Andreas Hindborg <nmi@metaspace.dk> 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

Reset all is missing, right? Might as well define that before there are
users of the ublk<->ublksrv interface.

>
> This allows implementation of zoned storage devices in user space. An
> example user space implementation based on ubdsrv is available [1].
>
> [1] https://github.com/metaspace/ubdsrv/commit/14a2b708f74f70cfecb076d92e680dc718cc1f6d
>
> Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
> ---
> Changes since v1:
>  - Fixed conditional compilation bug
>  - Refactored to collect conditional code additions together
>  - Fixed style errors
>  - Zero stack allocated value used for zone report
>
> Reported-by: Niklas Cassel <Niklas.Cassel@wdc.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Link: https://lore.kernel.org/oe-kbuild-all/202302250222.XOrw9j6z-lkp@intel.com/
> v1: https://lore.kernel.org/linux-block/20230224125950.214779-1-nmi@metaspace.dk/
>
>  drivers/block/ublk_drv.c      | 150 ++++++++++++++++++++++++++++++++--
>  include/uapi/linux/ublk_cmd.h |  18 ++++
>  2 files changed, 162 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 6368b56eacf1..37e516903867 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -20,6 +20,7 @@
>  #include <linux/major.h>
>  #include <linux/wait.h>
>  #include <linux/blkdev.h>
> +#include <linux/blkzoned.h>
>  #include <linux/init.h>
>  #include <linux/swap.h>
>  #include <linux/slab.h>
> @@ -51,10 +52,12 @@
>                 | UBLK_F_URING_CMD_COMP_IN_TASK \
>                 | UBLK_F_NEED_GET_DATA \
>                 | UBLK_F_USER_RECOVERY \
> -               | UBLK_F_USER_RECOVERY_REISSUE)
> +               | UBLK_F_USER_RECOVERY_REISSUE \
> +               | UBLK_F_ZONED)
>
>  /* All UBLK_PARAM_TYPE_* should be included here */
> -#define UBLK_PARAM_TYPE_ALL (UBLK_PARAM_TYPE_BASIC | UBLK_PARAM_TYPE_DISCARD)
> +#define UBLK_PARAM_TYPE_ALL (UBLK_PARAM_TYPE_BASIC | UBLK_PARAM_TYPE_DISCARD \
> +                            | UBLK_PARAM_TYPE_ZONED)
>
>  struct ublk_rq_data {
>         struct llist_node node;
> @@ -187,6 +190,98 @@ static DEFINE_MUTEX(ublk_ctl_mutex);
>
>  static struct miscdevice ublk_misc;
>
> +#ifdef CONFIG_BLK_DEV_ZONED
> +static void ublk_set_nr_zones(struct ublk_device *ub)
> +{
> +       const struct ublk_param_basic *p = &ub->params.basic;
> +
> +       if (ub->dev_info.flags & UBLK_F_ZONED && p->chunk_sectors)
> +               ub->ub_disk->nr_zones = p->dev_sectors / p->chunk_sectors;
> +}
> +
> +static 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);
> +       }
> +}
> +
> +static int ublk_revalidate_disk_zones(struct gendisk *disk)
> +{
> +       return blk_revalidate_disk_zones(disk, NULL);
> +}
> +
> +static int ublk_report_zones(struct gendisk *disk, sector_t sector,
> +                            unsigned int nr_zones, report_zones_cb cb,
> +                            void *data)
> +{
> +       struct ublk_device *ub;
> +       unsigned int zone_size;
> +       unsigned int first_zone;
> +       int ret = 0;
> +
> +       ub = disk->private_data;
> +
> +       if (!(ub->dev_info.flags & UBLK_F_ZONED))
> +               return -EINVAL;
> +
> +       zone_size = disk->queue->limits.chunk_sectors;
> +       first_zone = sector >> ilog2(zone_size);
> +       nr_zones = min(ub->ub_disk->nr_zones - first_zone, nr_zones);
> +
> +       for (unsigned int i = 0; i < nr_zones; i++) {
> +               struct request *req;
> +               blk_status_t status;
> +               struct blk_zone info = {0};
> +
> +               req = blk_mq_alloc_request(disk->queue, REQ_OP_DRV_IN, 0);
> +
> +               if (IS_ERR(req)) {
> +                       ret = PTR_ERR(req);
> +                       goto out;
> +               }
> +
> +               req->__sector = sector;
> +
> +               ret = blk_rq_map_kern(disk->queue, req, &info, sizeof(info),
> +                                     GFP_KERNEL);
> +
> +               if (ret)
> +                       goto out;
> +
> +               status = blk_execute_rq(req, 0);

Pingponging with userspace to retrieve thousand(s) of zone states is
very inefficient, so I don't think we
want to define the report zone ublk uapi to be one-zone-per-call.

Also, just overloading REQ_OP_DRV_IN with report zones instead of
defining a zone-report specific call does
not seem future proof - there might be a need to do other
driver-specific calls in the future.

Virtio blk implements reporting in a better way - se
virtblk_report_zones (that just got merged in Linus tree)
The same type of interface should be possible for ublk.

Cheers,
Hans

> +               ret = blk_status_to_errno(status);
> +               if (ret)
> +                       goto out;
> +
> +               blk_mq_free_request(req);
> +
> +               ret = cb(&info, i, data);
> +               if (ret)
> +                       goto out;
> +
> +               /* A zero length zone means don't ask for more zones */
> +               if (!info.len) {
> +                       nr_zones = i;
> +                       break;
> +               }
> +
> +               sector += zone_size;
> +       }
> +       ret = nr_zones;
> +
> + out:
> +       return ret;
> +}
> +#else
> +void ublk_set_nr_zones(struct ublk_device *ub);
> +void ublk_dev_param_zoned_apply(struct ublk_device *ub);
> +int ublk_revalidate_disk_zones(struct gendisk *disk);
> +#endif
> +
>  static void ublk_dev_param_basic_apply(struct ublk_device *ub)
>  {
>         struct request_queue *q = ub->ub_disk->queue;
> @@ -212,6 +307,9 @@ static void ublk_dev_param_basic_apply(struct ublk_device *ub)
>                 set_disk_ro(ub->ub_disk, true);
>
>         set_capacity(ub->ub_disk, p->dev_sectors);
> +
> +       if (IS_ENABLED(CONFIG_BLK_DEV_ZONED))
> +               ublk_set_nr_zones(ub);
>  }
>
>  static void ublk_dev_param_discard_apply(struct ublk_device *ub)
> @@ -268,6 +366,9 @@ static int ublk_apply_params(struct ublk_device *ub)
>         if (ub->params.types & UBLK_PARAM_TYPE_DISCARD)
>                 ublk_dev_param_discard_apply(ub);
>
> +       if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) && (ub->params.types & UBLK_PARAM_TYPE_ZONED))
> +               ublk_dev_param_zoned_apply(ub);
> +
>         return 0;
>  }
>
> @@ -361,9 +462,13 @@ static void ublk_free_disk(struct gendisk *disk)
>         put_device(&ub->cdev_dev);
>  }
>
> +
>  static const struct block_device_operations ub_fops = {
> -       .owner =        THIS_MODULE,
> -       .free_disk =    ublk_free_disk,
> +       .owner = THIS_MODULE,
> +       .free_disk = ublk_free_disk,
> +#ifdef CONFIG_BLK_DEV_ZONED
> +       .report_zones = ublk_report_zones,
> +#endif
>  };
>
>  #define UBLK_MAX_PIN_PAGES     32
> @@ -499,7 +604,7 @@ static int ublk_unmap_io(const struct ublk_queue *ubq,
>  {
>         const unsigned int rq_bytes = blk_rq_bytes(req);
>
> -       if (req_op(req) == REQ_OP_READ && ublk_rq_has_data(req)) {
> +       if ((req_op(req) == REQ_OP_READ || req_op(req) == REQ_OP_DRV_IN) && ublk_rq_has_data(req)) {
>                 struct ublk_map_data data = {
>                         .ubq    =       ubq,
>                         .rq     =       req,
> @@ -566,6 +671,26 @@ static blk_status_t ublk_setup_iod(struct ublk_queue *ubq, struct request *req)
>         case REQ_OP_WRITE_ZEROES:
>                 ublk_op = UBLK_IO_OP_WRITE_ZEROES;
>                 break;
> +#ifdef CONFIG_BLK_DEV_ZONED
> +       case REQ_OP_ZONE_OPEN:
> +               ublk_op = UBLK_IO_OP_ZONE_OPEN;
> +               break;
> +       case REQ_OP_ZONE_CLOSE:
> +               ublk_op = UBLK_IO_OP_ZONE_CLOSE;
> +               break;
> +       case REQ_OP_ZONE_FINISH:
> +               ublk_op = UBLK_IO_OP_ZONE_FINISH;
> +               break;
> +       case REQ_OP_ZONE_RESET:
> +               ublk_op = UBLK_IO_OP_ZONE_RESET;
> +               break;
> +       case REQ_OP_DRV_IN:
> +               ublk_op = UBLK_IO_OP_DRV_IN;
> +               break;
> +       case REQ_OP_ZONE_APPEND:
> +               /* We do not support zone append yet */
> +               fallthrough;
> +#endif
>         default:
>                 return BLK_STS_IOERR;
>         }
> @@ -612,7 +737,8 @@ static void ublk_complete_rq(struct request *req)
>          *
>          * Both the two needn't unmap.
>          */
> -       if (req_op(req) != REQ_OP_READ && req_op(req) != REQ_OP_WRITE) {
> +       if (req_op(req) != REQ_OP_READ && req_op(req) != REQ_OP_WRITE &&
> +           req_op(req) != REQ_OP_DRV_IN) {
>                 blk_mq_end_request(req, BLK_STS_OK);
>                 return;
>         }
> @@ -1535,6 +1661,15 @@ static int ublk_ctrl_start_dev(struct io_uring_cmd *cmd)
>         if (ret)
>                 goto out_put_disk;
>
> +       if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) &&
> +           ub->dev_info.flags & UBLK_F_ZONED) {
> +               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;
> +       }
> +
>         get_device(&ub->cdev_dev);
>         ret = add_disk(disk);
>         if (ret) {
> @@ -1673,6 +1808,9 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd)
>         if (!IS_BUILTIN(CONFIG_BLK_DEV_UBLK))
>                 ub->dev_info.flags |= UBLK_F_URING_CMD_COMP_IN_TASK;
>
> +       if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED))
> +               ub->dev_info.flags &= ~UBLK_F_ZONED;
> +
>         /* We are not ready to support zero copy */
>         ub->dev_info.flags &= ~UBLK_F_SUPPORT_ZERO_COPY;
>
> diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
> index 8f88e3a29998..074b97821575 100644
> --- a/include/uapi/linux/ublk_cmd.h
> +++ b/include/uapi/linux/ublk_cmd.h
> @@ -78,6 +78,10 @@
>  #define UBLK_F_USER_RECOVERY   (1UL << 3)
>
>  #define UBLK_F_USER_RECOVERY_REISSUE   (1UL << 4)
> +/*
> + * Enable zoned device support
> + */
> +#define UBLK_F_ZONED (1ULL << 5)
>
>  /* device state */
>  #define UBLK_S_DEV_DEAD        0
> @@ -129,6 +133,12 @@ struct ublksrv_ctrl_dev_info {
>  #define                UBLK_IO_OP_DISCARD      3
>  #define                UBLK_IO_OP_WRITE_SAME   4
>  #define                UBLK_IO_OP_WRITE_ZEROES 5
> +#define                UBLK_IO_OP_ZONE_OPEN            10
> +#define                UBLK_IO_OP_ZONE_CLOSE           11
> +#define                UBLK_IO_OP_ZONE_FINISH          12
> +#define                UBLK_IO_OP_ZONE_APPEND          13
> +#define                UBLK_IO_OP_ZONE_RESET           15
> +#define                UBLK_IO_OP_DRV_IN               34
>
>  #define                UBLK_IO_F_FAILFAST_DEV          (1U << 8)
>  #define                UBLK_IO_F_FAILFAST_TRANSPORT    (1U << 9)
> @@ -214,6 +224,12 @@ struct ublk_param_discard {
>         __u16   reserved0;
>  };
>
> +struct ublk_param_zoned {
> +       __u64   max_open_zones;
> +       __u64   max_active_zones;
> +       __u64   max_append_size;
> +};
> +
>  struct ublk_params {
>         /*
>          * Total length of parameters, userspace has to set 'len' for both
> @@ -224,10 +240,12 @@ struct ublk_params {
>         __u32   len;
>  #define UBLK_PARAM_TYPE_BASIC           (1 << 0)
>  #define UBLK_PARAM_TYPE_DISCARD         (1 << 1)
> +#define UBLK_PARAM_TYPE_ZONED           (1 << 2)
>         __u32   types;                  /* types of parameter included */
>
>         struct ublk_param_basic         basic;
>         struct ublk_param_discard       discard;
> +       struct ublk_param_zoned         zoned;
>  };
>
>  #endif
>
> base-commit: c9c3395d5e3dcc6daee66c6908354d47bf98cb0c
> --
> 2.39.2
>
  

Hans Holmberg <hans@owltronix.com> writes:

> On Fri, Feb 24, 2023 at 9:06 PM Andreas Hindborg <nmi@metaspace.dk> 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
>
> Reset all is missing, right? Might as well define that before there are
> users of the ublk<->ublksrv interface.

REQ_OP_ZONE_APPEND and REQ_OP_ZONE_RESET_ALL are not handled in this
patch. REQ_OP_* are converted to UBLK_IO_OP_* in ublk_setup_iod() in an
extensible way. It should be no problem adding support for more
operations later without breaking backwards compatibility.

>
>>
>> This allows implementation of zoned storage devices in user space. An
>> example user space implementation based on ubdsrv is available [1].
>>
>> [1] https://github.com/metaspace/ubdsrv/commit/14a2b708f74f70cfecb076d92e680dc718cc1f6d
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
>> ---
>> Changes since v1:
>>  - Fixed conditional compilation bug
>>  - Refactored to collect conditional code additions together
>>  - Fixed style errors
>>  - Zero stack allocated value used for zone report
>>
>> Reported-by: Niklas Cassel <Niklas.Cassel@wdc.com>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Link: https://lore.kernel.org/oe-kbuild-all/202302250222.XOrw9j6z-lkp@intel.com/
>> v1: https://lore.kernel.org/linux-block/20230224125950.214779-1-nmi@metaspace.dk/
>>
>>  drivers/block/ublk_drv.c      | 150 ++++++++++++++++++++++++++++++++--
>>  include/uapi/linux/ublk_cmd.h |  18 ++++
>>  2 files changed, 162 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
>> index 6368b56eacf1..37e516903867 100644
>> --- a/drivers/block/ublk_drv.c
>> +++ b/drivers/block/ublk_drv.c
>> @@ -20,6 +20,7 @@
>>  #include <linux/major.h>
>>  #include <linux/wait.h>
>>  #include <linux/blkdev.h>
>> +#include <linux/blkzoned.h>
>>  #include <linux/init.h>
>>  #include <linux/swap.h>
>>  #include <linux/slab.h>
>> @@ -51,10 +52,12 @@
>>                 | UBLK_F_URING_CMD_COMP_IN_TASK \
>>                 | UBLK_F_NEED_GET_DATA \
>>                 | UBLK_F_USER_RECOVERY \
>> -               | UBLK_F_USER_RECOVERY_REISSUE)
>> +               | UBLK_F_USER_RECOVERY_REISSUE \
>> +               | UBLK_F_ZONED)
>>
>>  /* All UBLK_PARAM_TYPE_* should be included here */
>> -#define UBLK_PARAM_TYPE_ALL (UBLK_PARAM_TYPE_BASIC | UBLK_PARAM_TYPE_DISCARD)
>> +#define UBLK_PARAM_TYPE_ALL (UBLK_PARAM_TYPE_BASIC | UBLK_PARAM_TYPE_DISCARD \
>> +                            | UBLK_PARAM_TYPE_ZONED)
>>
>>  struct ublk_rq_data {
>>         struct llist_node node;
>> @@ -187,6 +190,98 @@ static DEFINE_MUTEX(ublk_ctl_mutex);
>>
>>  static struct miscdevice ublk_misc;
>>
>> +#ifdef CONFIG_BLK_DEV_ZONED
>> +static void ublk_set_nr_zones(struct ublk_device *ub)
>> +{
>> +       const struct ublk_param_basic *p = &ub->params.basic;
>> +
>> +       if (ub->dev_info.flags & UBLK_F_ZONED && p->chunk_sectors)
>> +               ub->ub_disk->nr_zones = p->dev_sectors / p->chunk_sectors;
>> +}
>> +
>> +static 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);
>> +       }
>> +}
>> +
>> +static int ublk_revalidate_disk_zones(struct gendisk *disk)
>> +{
>> +       return blk_revalidate_disk_zones(disk, NULL);
>> +}
>> +
>> +static int ublk_report_zones(struct gendisk *disk, sector_t sector,
>> +                            unsigned int nr_zones, report_zones_cb cb,
>> +                            void *data)
>> +{
>> +       struct ublk_device *ub;
>> +       unsigned int zone_size;
>> +       unsigned int first_zone;
>> +       int ret = 0;
>> +
>> +       ub = disk->private_data;
>> +
>> +       if (!(ub->dev_info.flags & UBLK_F_ZONED))
>> +               return -EINVAL;
>> +
>> +       zone_size = disk->queue->limits.chunk_sectors;
>> +       first_zone = sector >> ilog2(zone_size);
>> +       nr_zones = min(ub->ub_disk->nr_zones - first_zone, nr_zones);
>> +
>> +       for (unsigned int i = 0; i < nr_zones; i++) {
>> +               struct request *req;
>> +               blk_status_t status;
>> +               struct blk_zone info = {0};
>> +
>> +               req = blk_mq_alloc_request(disk->queue, REQ_OP_DRV_IN, 0);
>> +
>> +               if (IS_ERR(req)) {
>> +                       ret = PTR_ERR(req);
>> +                       goto out;
>> +               }
>> +
>> +               req->__sector = sector;
>> +
>> +               ret = blk_rq_map_kern(disk->queue, req, &info, sizeof(info),
>> +                                     GFP_KERNEL);
>> +
>> +               if (ret)
>> +                       goto out;
>> +
>> +               status = blk_execute_rq(req, 0);
>
> Pingponging with userspace to retrieve thousand(s) of zone states is
> very inefficient, so I don't think we
> want to define the report zone ublk uapi to be one-zone-per-call.

I agree. I chose this implementation because it was not clear to me how
to communicate the zone report request details to user space. But I
think we can overload the nr_sectors and start_sector of the iod to
communicate the size of the report request.

> Also, just overloading REQ_OP_DRV_IN with report zones instead of
> defining a zone-report specific call does
> not seem future proof - there might be a need to do other
> driver-specific calls in the future.

Yea, ublk does not currently have a command field in the request pdu, so
I did not want to add that. However, it makes sense if we ever want to
use REQ_OP_DRV_IN for anything other than zone report.

>
> Virtio blk implements reporting in a better way - se
> virtblk_report_zones (that just got merged in Linus tree)
> The same type of interface should be possible for ublk.

Something like that could work 👍

>> +               ret = blk_status_to_errno(status);
>> +               if (ret)
>> +                       goto out;
>> +
>> +               blk_mq_free_request(req);
>> +
>> +               ret = cb(&info, i, data);
>> +               if (ret)
>> +                       goto out;
>> +
>> +               /* A zero length zone means don't ask for more zones */
>> +               if (!info.len) {
>> +                       nr_zones = i;
>> +                       break;
>> +               }
>> +
>> +               sector += zone_size;
>> +       }
>> +       ret = nr_zones;
>> +
>> + out:
>> +       return ret;
>> +}
>> +#else
>> +void ublk_set_nr_zones(struct ublk_device *ub);
>> +void ublk_dev_param_zoned_apply(struct ublk_device *ub);
>> +int ublk_revalidate_disk_zones(struct gendisk *disk);
>> +#endif
>> +
>>  static void ublk_dev_param_basic_apply(struct ublk_device *ub)
>>  {
>>         struct request_queue *q = ub->ub_disk->queue;
>> @@ -212,6 +307,9 @@ static void ublk_dev_param_basic_apply(struct ublk_device *ub)
>>                 set_disk_ro(ub->ub_disk, true);
>>
>>         set_capacity(ub->ub_disk, p->dev_sectors);
>> +
>> +       if (IS_ENABLED(CONFIG_BLK_DEV_ZONED))
>> +               ublk_set_nr_zones(ub);
>>  }
>>
>>  static void ublk_dev_param_discard_apply(struct ublk_device *ub)
>> @@ -268,6 +366,9 @@ static int ublk_apply_params(struct ublk_device *ub)
>>         if (ub->params.types & UBLK_PARAM_TYPE_DISCARD)
>>                 ublk_dev_param_discard_apply(ub);
>>
>> +       if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) && (ub->params.types & UBLK_PARAM_TYPE_ZONED))
>> +               ublk_dev_param_zoned_apply(ub);
>> +
>>         return 0;
>>  }
>>
>> @@ -361,9 +462,13 @@ static void ublk_free_disk(struct gendisk *disk)
>>         put_device(&ub->cdev_dev);
>>  }
>>
>> +
>>  static const struct block_device_operations ub_fops = {
>> -       .owner =        THIS_MODULE,
>> -       .free_disk =    ublk_free_disk,
>> +       .owner = THIS_MODULE,
>> +       .free_disk = ublk_free_disk,
>> +#ifdef CONFIG_BLK_DEV_ZONED
>> +       .report_zones = ublk_report_zones,
>> +#endif
>>  };
>>
>>  #define UBLK_MAX_PIN_PAGES     32
>> @@ -499,7 +604,7 @@ static int ublk_unmap_io(const struct ublk_queue *ubq,
>>  {
>>         const unsigned int rq_bytes = blk_rq_bytes(req);
>>
>> -       if (req_op(req) == REQ_OP_READ && ublk_rq_has_data(req)) {
>> +       if ((req_op(req) == REQ_OP_READ || req_op(req) == REQ_OP_DRV_IN) && ublk_rq_has_data(req)) {
>>                 struct ublk_map_data data = {
>>                         .ubq    =       ubq,
>>                         .rq     =       req,
>> @@ -566,6 +671,26 @@ static blk_status_t ublk_setup_iod(struct ublk_queue *ubq, struct request *req)
>>         case REQ_OP_WRITE_ZEROES:
>>                 ublk_op = UBLK_IO_OP_WRITE_ZEROES;
>>                 break;
>> +#ifdef CONFIG_BLK_DEV_ZONED
>> +       case REQ_OP_ZONE_OPEN:
>> +               ublk_op = UBLK_IO_OP_ZONE_OPEN;
>> +               break;
>> +       case REQ_OP_ZONE_CLOSE:
>> +               ublk_op = UBLK_IO_OP_ZONE_CLOSE;
>> +               break;
>> +       case REQ_OP_ZONE_FINISH:
>> +               ublk_op = UBLK_IO_OP_ZONE_FINISH;
>> +               break;
>> +       case REQ_OP_ZONE_RESET:
>> +               ublk_op = UBLK_IO_OP_ZONE_RESET;
>> +               break;
>> +       case REQ_OP_DRV_IN:
>> +               ublk_op = UBLK_IO_OP_DRV_IN;
>> +               break;
>> +       case REQ_OP_ZONE_APPEND:
>> +               /* We do not support zone append yet */
>> +               fallthrough;
>> +#endif
>>         default:
>>                 return BLK_STS_IOERR;
>>         }
>> @@ -612,7 +737,8 @@ static void ublk_complete_rq(struct request *req)
>>          *
>>          * Both the two needn't unmap.
>>          */
>> -       if (req_op(req) != REQ_OP_READ && req_op(req) != REQ_OP_WRITE) {
>> +       if (req_op(req) != REQ_OP_READ && req_op(req) != REQ_OP_WRITE &&
>> +           req_op(req) != REQ_OP_DRV_IN) {
>>                 blk_mq_end_request(req, BLK_STS_OK);
>>                 return;
>>         }
>> @@ -1535,6 +1661,15 @@ static int ublk_ctrl_start_dev(struct io_uring_cmd *cmd)
>>         if (ret)
>>                 goto out_put_disk;
>>
>> +       if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) &&
>> +           ub->dev_info.flags & UBLK_F_ZONED) {
>> +               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;
>> +       }
>> +
>>         get_device(&ub->cdev_dev);
>>         ret = add_disk(disk);
>>         if (ret) {
>> @@ -1673,6 +1808,9 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd)
>>         if (!IS_BUILTIN(CONFIG_BLK_DEV_UBLK))
>>                 ub->dev_info.flags |= UBLK_F_URING_CMD_COMP_IN_TASK;
>>
>> +       if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED))
>> +               ub->dev_info.flags &= ~UBLK_F_ZONED;
>> +
>>         /* We are not ready to support zero copy */
>>         ub->dev_info.flags &= ~UBLK_F_SUPPORT_ZERO_COPY;
>>
>> diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
>> index 8f88e3a29998..074b97821575 100644
>> --- a/include/uapi/linux/ublk_cmd.h
>> +++ b/include/uapi/linux/ublk_cmd.h
>> @@ -78,6 +78,10 @@
>>  #define UBLK_F_USER_RECOVERY   (1UL << 3)
>>
>>  #define UBLK_F_USER_RECOVERY_REISSUE   (1UL << 4)
>> +/*
>> + * Enable zoned device support
>> + */
>> +#define UBLK_F_ZONED (1ULL << 5)
>>
>>  /* device state */
>>  #define UBLK_S_DEV_DEAD        0
>> @@ -129,6 +133,12 @@ struct ublksrv_ctrl_dev_info {
>>  #define                UBLK_IO_OP_DISCARD      3
>>  #define                UBLK_IO_OP_WRITE_SAME   4
>>  #define                UBLK_IO_OP_WRITE_ZEROES 5
>> +#define                UBLK_IO_OP_ZONE_OPEN            10
>> +#define                UBLK_IO_OP_ZONE_CLOSE           11
>> +#define                UBLK_IO_OP_ZONE_FINISH          12
>> +#define                UBLK_IO_OP_ZONE_APPEND          13
>> +#define                UBLK_IO_OP_ZONE_RESET           15
>> +#define                UBLK_IO_OP_DRV_IN               34
>>
>>  #define                UBLK_IO_F_FAILFAST_DEV          (1U << 8)
>>  #define                UBLK_IO_F_FAILFAST_TRANSPORT    (1U << 9)
>> @@ -214,6 +224,12 @@ struct ublk_param_discard {
>>         __u16   reserved0;
>>  };
>>
>> +struct ublk_param_zoned {
>> +       __u64   max_open_zones;
>> +       __u64   max_active_zones;
>> +       __u64   max_append_size;
>> +};
>> +
>>  struct ublk_params {
>>         /*
>>          * Total length of parameters, userspace has to set 'len' for both
>> @@ -224,10 +240,12 @@ struct ublk_params {
>>         __u32   len;
>>  #define UBLK_PARAM_TYPE_BASIC           (1 << 0)
>>  #define UBLK_PARAM_TYPE_DISCARD         (1 << 1)
>> +#define UBLK_PARAM_TYPE_ZONED           (1 << 2)
>>         __u32   types;                  /* types of parameter included */
>>
>>         struct ublk_param_basic         basic;
>>         struct ublk_param_discard       discard;
>> +       struct ublk_param_zoned         zoned;
>>  };
>>
>>  #endif
>>
>> base-commit: c9c3395d5e3dcc6daee66c6908354d47bf98cb0c
>> --
>> 2.39.2
>>
  

On Fri, Feb 24, 2023 at 09:05:01PM +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
> 
> This allows implementation of zoned storage devices in user space. An
> example user space implementation based on ubdsrv is available [1].
> 
> [1] https://github.com/metaspace/ubdsrv/commit/14a2b708f74f70cfecb076d92e680dc718cc1f6d

As I suggested, please write one simple & clean zoned target for verifying
the interface, and better to not add to tgt_null.c.

> 
> Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
> ---
> Changes since v1:
>  - Fixed conditional compilation bug
>  - Refactored to collect conditional code additions together
>  - Fixed style errors
>  - Zero stack allocated value used for zone report
> 
> Reported-by: Niklas Cassel <Niklas.Cassel@wdc.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Link: https://lore.kernel.org/oe-kbuild-all/202302250222.XOrw9j6z-lkp@intel.com/
> v1: https://lore.kernel.org/linux-block/20230224125950.214779-1-nmi@metaspace.dk/
> 
>  drivers/block/ublk_drv.c      | 150 ++++++++++++++++++++++++++++++++--
>  include/uapi/linux/ublk_cmd.h |  18 ++++
>  2 files changed, 162 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 6368b56eacf1..37e516903867 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -20,6 +20,7 @@
>  #include <linux/major.h>
>  #include <linux/wait.h>
>  #include <linux/blkdev.h>
> +#include <linux/blkzoned.h>
>  #include <linux/init.h>
>  #include <linux/swap.h>
>  #include <linux/slab.h>
> @@ -51,10 +52,12 @@
>  		| UBLK_F_URING_CMD_COMP_IN_TASK \
>  		| UBLK_F_NEED_GET_DATA \
>  		| UBLK_F_USER_RECOVERY \
> -		| UBLK_F_USER_RECOVERY_REISSUE)
> +		| UBLK_F_USER_RECOVERY_REISSUE \
> +		| UBLK_F_ZONED)
>  
>  /* All UBLK_PARAM_TYPE_* should be included here */
> -#define UBLK_PARAM_TYPE_ALL (UBLK_PARAM_TYPE_BASIC | UBLK_PARAM_TYPE_DISCARD)
> +#define UBLK_PARAM_TYPE_ALL (UBLK_PARAM_TYPE_BASIC | UBLK_PARAM_TYPE_DISCARD \
> +			     | UBLK_PARAM_TYPE_ZONED)
>  
>  struct ublk_rq_data {
>  	struct llist_node node;
> @@ -187,6 +190,98 @@ static DEFINE_MUTEX(ublk_ctl_mutex);
>  
>  static struct miscdevice ublk_misc;
>  
> +#ifdef CONFIG_BLK_DEV_ZONED
> +static void ublk_set_nr_zones(struct ublk_device *ub)
> +{
> +	const struct ublk_param_basic *p = &ub->params.basic;
> +
> +	if (ub->dev_info.flags & UBLK_F_ZONED && p->chunk_sectors)
> +		ub->ub_disk->nr_zones = p->dev_sectors / p->chunk_sectors;
> +}
> +
> +static 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);
> +	}
> +}
> +
> +static int ublk_revalidate_disk_zones(struct gendisk *disk)
> +{
> +	return blk_revalidate_disk_zones(disk, NULL);
> +}
> +
> +static int ublk_report_zones(struct gendisk *disk, sector_t sector,
> +			     unsigned int nr_zones, report_zones_cb cb,
> +			     void *data)
> +{
> +	struct ublk_device *ub;
> +	unsigned int zone_size;
> +	unsigned int first_zone;
> +	int ret = 0;
> +
> +	ub = disk->private_data;
> +
> +	if (!(ub->dev_info.flags & UBLK_F_ZONED))
> +		return -EINVAL;
> +
> +	zone_size = disk->queue->limits.chunk_sectors;
> +	first_zone = sector >> ilog2(zone_size);
> +	nr_zones = min(ub->ub_disk->nr_zones - first_zone, nr_zones);
> +
> +	for (unsigned int i = 0; i < nr_zones; i++) {

The local variable 'i' needs to be declared in the front part
of this function body.

> +		struct request *req;
> +		blk_status_t status;
> +		struct blk_zone info = {0};
> +
> +		req = blk_mq_alloc_request(disk->queue, REQ_OP_DRV_IN, 0);
> +
> +		if (IS_ERR(req)) {
> +			ret = PTR_ERR(req);
> +			goto out;
> +		}
> +
> +		req->__sector = sector;

Why is req->__sector set?

> +
> +		ret = blk_rq_map_kern(disk->queue, req, &info, sizeof(info),
> +				      GFP_KERNEL);
> +
> +		if (ret)
> +			goto out;
> +
> +		status = blk_execute_rq(req, 0);
> +		ret = blk_status_to_errno(status);
> +		if (ret)
> +			goto out;
> +
> +		blk_mq_free_request(req);
> +
> +		ret = cb(&info, i, data);
> +		if (ret)
> +			goto out;
> +
> +		/* A zero length zone means don't ask for more zones */
> +		if (!info.len) {
> +			nr_zones = i;
> +			break;
> +		}
> +
> +		sector += zone_size;
> +	}

I'd suggest to report as many as possible zones in one command, and
the dev_info.max_io_buf_bytes is the max allowed buffer size for one
command, please refer to nvme_ns_report_zones().

Also we are going to extend ublk in the multiple LUN/NS style, and I
guess that won't be one issue since ->report_zones() is always done on
disk level, right?

> +	ret = nr_zones;
> +
> + out:
> +	return ret;
> +}
> +#else
> +void ublk_set_nr_zones(struct ublk_device *ub);
> +void ublk_dev_param_zoned_apply(struct ublk_device *ub);
> +int ublk_revalidate_disk_zones(struct gendisk *disk);
> +#endif
> +
>  static void ublk_dev_param_basic_apply(struct ublk_device *ub)
>  {
>  	struct request_queue *q = ub->ub_disk->queue;
> @@ -212,6 +307,9 @@ static void ublk_dev_param_basic_apply(struct ublk_device *ub)
>  		set_disk_ro(ub->ub_disk, true);
>  
>  	set_capacity(ub->ub_disk, p->dev_sectors);
> +
> +	if (IS_ENABLED(CONFIG_BLK_DEV_ZONED))
> +		ublk_set_nr_zones(ub);
>  }
>  
>  static void ublk_dev_param_discard_apply(struct ublk_device *ub)
> @@ -268,6 +366,9 @@ static int ublk_apply_params(struct ublk_device *ub)
>  	if (ub->params.types & UBLK_PARAM_TYPE_DISCARD)
>  		ublk_dev_param_discard_apply(ub);
>  
> +	if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) && (ub->params.types & UBLK_PARAM_TYPE_ZONED))
> +		ublk_dev_param_zoned_apply(ub);
> +
>  	return 0;
>  }
>  
> @@ -361,9 +462,13 @@ static void ublk_free_disk(struct gendisk *disk)
>  	put_device(&ub->cdev_dev);
>  }
>  
> +
>  static const struct block_device_operations ub_fops = {
> -	.owner =	THIS_MODULE,
> -	.free_disk =	ublk_free_disk,
> +	.owner = THIS_MODULE,
> +	.free_disk = ublk_free_disk,
> +#ifdef CONFIG_BLK_DEV_ZONED
> +	.report_zones = ublk_report_zones,
> +#endif

Define one null ublk_report_zones in #else branch of the above #ifdef, then we
can save one #ifdef.

>  };
>  
>  #define UBLK_MAX_PIN_PAGES	32
> @@ -499,7 +604,7 @@ static int ublk_unmap_io(const struct ublk_queue *ubq,
>  {
>  	const unsigned int rq_bytes = blk_rq_bytes(req);
>  
> -	if (req_op(req) == REQ_OP_READ && ublk_rq_has_data(req)) {
> +	if ((req_op(req) == REQ_OP_READ || req_op(req) == REQ_OP_DRV_IN) && ublk_rq_has_data(req)) {
>  		struct ublk_map_data data = {
>  			.ubq	=	ubq,
>  			.rq	=	req,
> @@ -566,6 +671,26 @@ static blk_status_t ublk_setup_iod(struct ublk_queue *ubq, struct request *req)
>  	case REQ_OP_WRITE_ZEROES:
>  		ublk_op = UBLK_IO_OP_WRITE_ZEROES;
>  		break;
> +#ifdef CONFIG_BLK_DEV_ZONED
> +	case REQ_OP_ZONE_OPEN:
> +		ublk_op = UBLK_IO_OP_ZONE_OPEN;
> +		break;
> +	case REQ_OP_ZONE_CLOSE:
> +		ublk_op = UBLK_IO_OP_ZONE_CLOSE;
> +		break;
> +	case REQ_OP_ZONE_FINISH:
> +		ublk_op = UBLK_IO_OP_ZONE_FINISH;
> +		break;
> +	case REQ_OP_ZONE_RESET:
> +		ublk_op = UBLK_IO_OP_ZONE_RESET;
> +		break;
> +	case REQ_OP_DRV_IN:
> +		ublk_op = UBLK_IO_OP_DRV_IN;
> +		break;
> +	case REQ_OP_ZONE_APPEND:
> +		/* We do not support zone append yet */
> +		fallthrough;
> +#endif

The above '#ifdef' is needn't, since  OP_ZONE should be defined no
matter if CONFIG_BLK_DEV_ZONED is enabled or not.

>  	default:
>  		return BLK_STS_IOERR;
>  	}
> @@ -612,7 +737,8 @@ static void ublk_complete_rq(struct request *req)
>  	 *
>  	 * Both the two needn't unmap.
>  	 */
> -	if (req_op(req) != REQ_OP_READ && req_op(req) != REQ_OP_WRITE) {
> +	if (req_op(req) != REQ_OP_READ && req_op(req) != REQ_OP_WRITE &&
> +	    req_op(req) != REQ_OP_DRV_IN) {
>  		blk_mq_end_request(req, BLK_STS_OK);
>  		return;
>  	}
> @@ -1535,6 +1661,15 @@ static int ublk_ctrl_start_dev(struct io_uring_cmd *cmd)
>  	if (ret)
>  		goto out_put_disk;
>  
> +	if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) &&
> +	    ub->dev_info.flags & UBLK_F_ZONED) {
> +		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;
> +	}
> +
>  	get_device(&ub->cdev_dev);
>  	ret = add_disk(disk);
>  	if (ret) {
> @@ -1673,6 +1808,9 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd)
>  	if (!IS_BUILTIN(CONFIG_BLK_DEV_UBLK))
>  		ub->dev_info.flags |= UBLK_F_URING_CMD_COMP_IN_TASK;
>  
> +	if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED))
> +		ub->dev_info.flags &= ~UBLK_F_ZONED;
> +
>  	/* We are not ready to support zero copy */
>  	ub->dev_info.flags &= ~UBLK_F_SUPPORT_ZERO_COPY;
>  
> diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
> index 8f88e3a29998..074b97821575 100644
> --- a/include/uapi/linux/ublk_cmd.h
> +++ b/include/uapi/linux/ublk_cmd.h
> @@ -78,6 +78,10 @@
>  #define UBLK_F_USER_RECOVERY	(1UL << 3)
>  
>  #define UBLK_F_USER_RECOVERY_REISSUE	(1UL << 4)
> +/*
> + * Enable zoned device support
> + */
> +#define UBLK_F_ZONED (1ULL << 5)
>  
>  /* device state */
>  #define UBLK_S_DEV_DEAD	0
> @@ -129,6 +133,12 @@ struct ublksrv_ctrl_dev_info {
>  #define		UBLK_IO_OP_DISCARD	3
>  #define		UBLK_IO_OP_WRITE_SAME	4
>  #define		UBLK_IO_OP_WRITE_ZEROES	5
> +#define		UBLK_IO_OP_ZONE_OPEN		10
> +#define		UBLK_IO_OP_ZONE_CLOSE		11
> +#define		UBLK_IO_OP_ZONE_FINISH		12
> +#define		UBLK_IO_OP_ZONE_APPEND		13
> +#define		UBLK_IO_OP_ZONE_RESET		15
> +#define		UBLK_IO_OP_DRV_IN		34
>  
>  #define		UBLK_IO_F_FAILFAST_DEV		(1U << 8)
>  #define		UBLK_IO_F_FAILFAST_TRANSPORT	(1U << 9)
> @@ -214,6 +224,12 @@ struct ublk_param_discard {
>  	__u16	reserved0;
>  };
>  
> +struct ublk_param_zoned {
> +	__u64	max_open_zones;
> +	__u64	max_active_zones;
> +	__u64	max_append_size;
> +};

Is the above zoned parameter enough for future extension?
Does ZNS need extra parameter? Or some zoned new(important) features?

I highly suggest to reserve some fields for extension, given
it is one ABI interface, which is supposed to be defined well
enough from the beginning.


Thanks, 
Ming
  

Hi Ming,

Ming Lei <ming.lei@redhat.com> writes:

> On Fri, Feb 24, 2023 at 09:05:01PM +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
>> 
>> This allows implementation of zoned storage devices in user space. An
>> example user space implementation based on ubdsrv is available [1].
>> 
>> [1] https://github.com/metaspace/ubdsrv/commit/14a2b708f74f70cfecb076d92e680dc718cc1f6d
>
> As I suggested, please write one simple & clean zoned target for verifying
> the interface, and better to not add to tgt_null.c.

For ubdsrv, I understand that you prefer to reimplement null and loop targets for
zoned storage. I don't understand why you think this is a good idea,
since we will have massive code duplication. This would be comparable to
having a separate null_blk driver for zoned storage. Am I understanding
you correctly?

Anyway, I think we can discuss the kernel patch even though the ubdsrv
implementation is not a separate target yet? The code would be almost
identical, it would just live in a separate translation unit.

>
>> 
>> Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
>> ---
>> Changes since v1:
>>  - Fixed conditional compilation bug
>>  - Refactored to collect conditional code additions together
>>  - Fixed style errors
>>  - Zero stack allocated value used for zone report
>> 
>> Reported-by: Niklas Cassel <Niklas.Cassel@wdc.com>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Link: https://lore.kernel.org/oe-kbuild-all/202302250222.XOrw9j6z-lkp@intel.com/
>> v1: https://lore.kernel.org/linux-block/20230224125950.214779-1-nmi@metaspace.dk/
>> 
>>  drivers/block/ublk_drv.c      | 150 ++++++++++++++++++++++++++++++++--
>>  include/uapi/linux/ublk_cmd.h |  18 ++++
>>  2 files changed, 162 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
>> index 6368b56eacf1..37e516903867 100644
>> --- a/drivers/block/ublk_drv.c
>> +++ b/drivers/block/ublk_drv.c
>> @@ -20,6 +20,7 @@
>>  #include <linux/major.h>
>>  #include <linux/wait.h>
>>  #include <linux/blkdev.h>
>> +#include <linux/blkzoned.h>
>>  #include <linux/init.h>
>>  #include <linux/swap.h>
>>  #include <linux/slab.h>
>> @@ -51,10 +52,12 @@
>>  		| UBLK_F_URING_CMD_COMP_IN_TASK \
>>  		| UBLK_F_NEED_GET_DATA \
>>  		| UBLK_F_USER_RECOVERY \
>> -		| UBLK_F_USER_RECOVERY_REISSUE)
>> +		| UBLK_F_USER_RECOVERY_REISSUE \
>> +		| UBLK_F_ZONED)
>>  
>>  /* All UBLK_PARAM_TYPE_* should be included here */
>> -#define UBLK_PARAM_TYPE_ALL (UBLK_PARAM_TYPE_BASIC | UBLK_PARAM_TYPE_DISCARD)
>> +#define UBLK_PARAM_TYPE_ALL (UBLK_PARAM_TYPE_BASIC | UBLK_PARAM_TYPE_DISCARD \
>> +			     | UBLK_PARAM_TYPE_ZONED)
>>  
>>  struct ublk_rq_data {
>>  	struct llist_node node;
>> @@ -187,6 +190,98 @@ static DEFINE_MUTEX(ublk_ctl_mutex);
>>  
>>  static struct miscdevice ublk_misc;
>>  
>> +#ifdef CONFIG_BLK_DEV_ZONED
>> +static void ublk_set_nr_zones(struct ublk_device *ub)
>> +{
>> +	const struct ublk_param_basic *p = &ub->params.basic;
>> +
>> +	if (ub->dev_info.flags & UBLK_F_ZONED && p->chunk_sectors)
>> +		ub->ub_disk->nr_zones = p->dev_sectors / p->chunk_sectors;
>> +}
>> +
>> +static 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);
>> +	}
>> +}
>> +
>> +static int ublk_revalidate_disk_zones(struct gendisk *disk)
>> +{
>> +	return blk_revalidate_disk_zones(disk, NULL);
>> +}
>> +
>> +static int ublk_report_zones(struct gendisk *disk, sector_t sector,
>> +			     unsigned int nr_zones, report_zones_cb cb,
>> +			     void *data)
>> +{
>> +	struct ublk_device *ub;
>> +	unsigned int zone_size;
>> +	unsigned int first_zone;
>> +	int ret = 0;
>> +
>> +	ub = disk->private_data;
>> +
>> +	if (!(ub->dev_info.flags & UBLK_F_ZONED))
>> +		return -EINVAL;
>> +
>> +	zone_size = disk->queue->limits.chunk_sectors;
>> +	first_zone = sector >> ilog2(zone_size);
>> +	nr_zones = min(ub->ub_disk->nr_zones - first_zone, nr_zones);
>> +
>> +	for (unsigned int i = 0; i < nr_zones; i++) {
>
> The local variable 'i' needs to be declared in the front part
> of this function body.

Ok.

>
>> +		struct request *req;
>> +		blk_status_t status;
>> +		struct blk_zone info = {0};
>> +
>> +		req = blk_mq_alloc_request(disk->queue, REQ_OP_DRV_IN, 0);
>> +
>> +		if (IS_ERR(req)) {
>> +			ret = PTR_ERR(req);
>> +			goto out;
>> +		}
>> +
>> +		req->__sector = sector;
>
> Why is req->__sector set?

I use it to carry information about the first zone of the report request.

>
>> +
>> +		ret = blk_rq_map_kern(disk->queue, req, &info, sizeof(info),
>> +				      GFP_KERNEL);
>> +
>> +		if (ret)
>> +			goto out;
>> +
>> +		status = blk_execute_rq(req, 0);
>> +		ret = blk_status_to_errno(status);
>> +		if (ret)
>> +			goto out;
>> +
>> +		blk_mq_free_request(req);
>> +
>> +		ret = cb(&info, i, data);
>> +		if (ret)
>> +			goto out;
>> +
>> +		/* A zero length zone means don't ask for more zones */
>> +		if (!info.len) {
>> +			nr_zones = i;
>> +			break;
>> +		}
>> +
>> +		sector += zone_size;
>> +	}
>
> I'd suggest to report as many as possible zones in one command, and
> the dev_info.max_io_buf_bytes is the max allowed buffer size for one
> command, please refer to nvme_ns_report_zones().

I agree about fetching more zones. However, it is no good to fetch up to
a max, since the requested zone report may less than max. I was
considering overloading req->__data_len and iod->nr_sectors to convey
the number of requested zones. What do you think about that?

>
> Also we are going to extend ublk in the multiple LUN/NS style, and I
> guess that won't be one issue since ->report_zones() is always done on
> disk level, right?

Yes, that should be fine.

>
>> +	ret = nr_zones;
>> +
>> + out:
>> +	return ret;
>> +}
>> +#else
>> +void ublk_set_nr_zones(struct ublk_device *ub);
>> +void ublk_dev_param_zoned_apply(struct ublk_device *ub);
>> +int ublk_revalidate_disk_zones(struct gendisk *disk);
>> +#endif
>> +
>>  static void ublk_dev_param_basic_apply(struct ublk_device *ub)
>>  {
>>  	struct request_queue *q = ub->ub_disk->queue;
>> @@ -212,6 +307,9 @@ static void ublk_dev_param_basic_apply(struct ublk_device *ub)
>>  		set_disk_ro(ub->ub_disk, true);
>>  
>>  	set_capacity(ub->ub_disk, p->dev_sectors);
>> +
>> +	if (IS_ENABLED(CONFIG_BLK_DEV_ZONED))
>> +		ublk_set_nr_zones(ub);
>>  }
>>  
>>  static void ublk_dev_param_discard_apply(struct ublk_device *ub)
>> @@ -268,6 +366,9 @@ static int ublk_apply_params(struct ublk_device *ub)
>>  	if (ub->params.types & UBLK_PARAM_TYPE_DISCARD)
>>  		ublk_dev_param_discard_apply(ub);
>>  
>> +	if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) && (ub->params.types & UBLK_PARAM_TYPE_ZONED))
>> +		ublk_dev_param_zoned_apply(ub);
>> +
>>  	return 0;
>>  }
>>  
>> @@ -361,9 +462,13 @@ static void ublk_free_disk(struct gendisk *disk)
>>  	put_device(&ub->cdev_dev);
>>  }
>>  
>> +
>>  static const struct block_device_operations ub_fops = {
>> -	.owner =	THIS_MODULE,
>> -	.free_disk =	ublk_free_disk,
>> +	.owner = THIS_MODULE,
>> +	.free_disk = ublk_free_disk,
>> +#ifdef CONFIG_BLK_DEV_ZONED
>> +	.report_zones = ublk_report_zones,
>> +#endif
>
> Define one null ublk_report_zones in #else branch of the above #ifdef, then we
> can save one #ifdef.

I would have to define it as a null pointer in the #else case then?

>
>>  };
>>  
>>  #define UBLK_MAX_PIN_PAGES	32
>> @@ -499,7 +604,7 @@ static int ublk_unmap_io(const struct ublk_queue *ubq,
>>  {
>>  	const unsigned int rq_bytes = blk_rq_bytes(req);
>>  
>> -	if (req_op(req) == REQ_OP_READ && ublk_rq_has_data(req)) {
>> +	if ((req_op(req) == REQ_OP_READ || req_op(req) == REQ_OP_DRV_IN) && ublk_rq_has_data(req)) {
>>  		struct ublk_map_data data = {
>>  			.ubq	=	ubq,
>>  			.rq	=	req,
>> @@ -566,6 +671,26 @@ static blk_status_t ublk_setup_iod(struct ublk_queue *ubq, struct request *req)
>>  	case REQ_OP_WRITE_ZEROES:
>>  		ublk_op = UBLK_IO_OP_WRITE_ZEROES;
>>  		break;
>> +#ifdef CONFIG_BLK_DEV_ZONED
>> +	case REQ_OP_ZONE_OPEN:
>> +		ublk_op = UBLK_IO_OP_ZONE_OPEN;
>> +		break;
>> +	case REQ_OP_ZONE_CLOSE:
>> +		ublk_op = UBLK_IO_OP_ZONE_CLOSE;
>> +		break;
>> +	case REQ_OP_ZONE_FINISH:
>> +		ublk_op = UBLK_IO_OP_ZONE_FINISH;
>> +		break;
>> +	case REQ_OP_ZONE_RESET:
>> +		ublk_op = UBLK_IO_OP_ZONE_RESET;
>> +		break;
>> +	case REQ_OP_DRV_IN:
>> +		ublk_op = UBLK_IO_OP_DRV_IN;
>> +		break;
>> +	case REQ_OP_ZONE_APPEND:
>> +		/* We do not support zone append yet */
>> +		fallthrough;
>> +#endif
>
> The above '#ifdef' is needn't, since  OP_ZONE should be defined no
> matter if CONFIG_BLK_DEV_ZONED is enabled or not.

I see. But do we want to process the requests if BLK_DEV_ZONED is not
enabled, or do we want to fail the IO?

>
>>  	default:
>>  		return BLK_STS_IOERR;
>>  	}
>> @@ -612,7 +737,8 @@ static void ublk_complete_rq(struct request *req)
>>  	 *
>>  	 * Both the two needn't unmap.
>>  	 */
>> -	if (req_op(req) != REQ_OP_READ && req_op(req) != REQ_OP_WRITE) {
>> +	if (req_op(req) != REQ_OP_READ && req_op(req) != REQ_OP_WRITE &&
>> +	    req_op(req) != REQ_OP_DRV_IN) {
>>  		blk_mq_end_request(req, BLK_STS_OK);
>>  		return;
>>  	}
>> @@ -1535,6 +1661,15 @@ static int ublk_ctrl_start_dev(struct io_uring_cmd *cmd)
>>  	if (ret)
>>  		goto out_put_disk;
>>  
>> +	if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) &&
>> +	    ub->dev_info.flags & UBLK_F_ZONED) {
>> +		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;
>> +	}
>> +
>>  	get_device(&ub->cdev_dev);
>>  	ret = add_disk(disk);
>>  	if (ret) {
>> @@ -1673,6 +1808,9 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd)
>>  	if (!IS_BUILTIN(CONFIG_BLK_DEV_UBLK))
>>  		ub->dev_info.flags |= UBLK_F_URING_CMD_COMP_IN_TASK;
>>  
>> +	if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED))
>> +		ub->dev_info.flags &= ~UBLK_F_ZONED;
>> +
>>  	/* We are not ready to support zero copy */
>>  	ub->dev_info.flags &= ~UBLK_F_SUPPORT_ZERO_COPY;
>>  
>> diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
>> index 8f88e3a29998..074b97821575 100644
>> --- a/include/uapi/linux/ublk_cmd.h
>> +++ b/include/uapi/linux/ublk_cmd.h
>> @@ -78,6 +78,10 @@
>>  #define UBLK_F_USER_RECOVERY	(1UL << 3)
>>  
>>  #define UBLK_F_USER_RECOVERY_REISSUE	(1UL << 4)
>> +/*
>> + * Enable zoned device support
>> + */
>> +#define UBLK_F_ZONED (1ULL << 5)
>>  
>>  /* device state */
>>  #define UBLK_S_DEV_DEAD	0
>> @@ -129,6 +133,12 @@ struct ublksrv_ctrl_dev_info {
>>  #define		UBLK_IO_OP_DISCARD	3
>>  #define		UBLK_IO_OP_WRITE_SAME	4
>>  #define		UBLK_IO_OP_WRITE_ZEROES	5
>> +#define		UBLK_IO_OP_ZONE_OPEN		10
>> +#define		UBLK_IO_OP_ZONE_CLOSE		11
>> +#define		UBLK_IO_OP_ZONE_FINISH		12
>> +#define		UBLK_IO_OP_ZONE_APPEND		13
>> +#define		UBLK_IO_OP_ZONE_RESET		15
>> +#define		UBLK_IO_OP_DRV_IN		34
>>  
>>  #define		UBLK_IO_F_FAILFAST_DEV		(1U << 8)
>>  #define		UBLK_IO_F_FAILFAST_TRANSPORT	(1U << 9)
>> @@ -214,6 +224,12 @@ struct ublk_param_discard {
>>  	__u16	reserved0;
>>  };
>>  
>> +struct ublk_param_zoned {
>> +	__u64	max_open_zones;
>> +	__u64	max_active_zones;
>> +	__u64	max_append_size;
>> +};
>
> Is the above zoned parameter enough for future extension?
> Does ZNS need extra parameter? Or some zoned new(important) features?

@Damien, @Hans, @Matias, what do you think?

>
> I highly suggest to reserve some fields for extension, given
> it is one ABI interface, which is supposed to be defined well
> enough from the beginning.

How many bytes would you reserve?

Thanks!

Best regards,
Andreas
  

On 3/2/23 16:31, Andreas Hindborg wrote:
>>> +struct ublk_param_zoned {
>>> +	__u64	max_open_zones;
>>> +	__u64	max_active_zones;
>>> +	__u64	max_append_size;
>>> +};
>>
>> Is the above zoned parameter enough for future extension?
>> Does ZNS need extra parameter? Or some zoned new(important) features?
> 
> @Damien, @Hans, @Matias, what do you think?

Yes, add some reserved fields. The struct is 24 B for now, you can make it 32 B.
But it is a little odd: why 64 bits for max open/active zones and max append ?
bio len is 32 bits and number of zones also 32 bits. You do not need 64 bits for
all these fields. Also, no zone model reported with this ? What about write
granularity (for SMR HDDs) too ?

So something like:

struct ublk_param_zoned {
	__u32	model;
	__u32	write_granularity;
	__u32	max_open_zones;
	__u32	max_active_zones;
	__u32	max_append_size;
	__u8	reserved[12];
};

looks better to me. Note sure about the need for model and write_granularity
here though. I did not follow zoned ublk patches.
  

On Thu, Mar 02, 2023 at 08:31:07AM +0100, Andreas Hindborg wrote:
> 
> Hi Ming,
> 
> Ming Lei <ming.lei@redhat.com> writes:
> 
> > On Fri, Feb 24, 2023 at 09:05:01PM +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
> >> 
> >> This allows implementation of zoned storage devices in user space. An
> >> example user space implementation based on ubdsrv is available [1].
> >> 
> >> [1] https://github.com/metaspace/ubdsrv/commit/14a2b708f74f70cfecb076d92e680dc718cc1f6d
> >
> > As I suggested, please write one simple & clean zoned target for verifying
> > the interface, and better to not add to tgt_null.c.
> 
> For ubdsrv, I understand that you prefer to reimplement null and loop targets for
> zoned storage. I don't understand why you think this is a good idea,
> since we will have massive code duplication. This would be comparable to
> having a separate null_blk driver for zoned storage. Am I understanding
> you correctly?

It is userspace, code duplication isn't a big deal, but putting lots
of un-related things together is just one mess.

> 
> Anyway, I think we can discuss the kernel patch even though the ubdsrv
> implementation is not a separate target yet? The code would be almost
> identical, it would just live in a separate translation unit.

I just suggest to make the userspace test code clean & easy, which plays
big role for looking kernel interface from user side.

> 
> >
> >> 
> >> Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
> >> ---
> >> Changes since v1:
> >>  - Fixed conditional compilation bug
> >>  - Refactored to collect conditional code additions together
> >>  - Fixed style errors
> >>  - Zero stack allocated value used for zone report
> >> 
> >> Reported-by: Niklas Cassel <Niklas.Cassel@wdc.com>
> >> Reported-by: kernel test robot <lkp@intel.com>
> >> Link: https://lore.kernel.org/oe-kbuild-all/202302250222.XOrw9j6z-lkp@intel.com/
> >> v1: https://lore.kernel.org/linux-block/20230224125950.214779-1-nmi@metaspace.dk/
> >> 
> >>  drivers/block/ublk_drv.c      | 150 ++++++++++++++++++++++++++++++++--
> >>  include/uapi/linux/ublk_cmd.h |  18 ++++
> >>  2 files changed, 162 insertions(+), 6 deletions(-)
> >> 
> >> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> >> index 6368b56eacf1..37e516903867 100644
> >> --- a/drivers/block/ublk_drv.c
> >> +++ b/drivers/block/ublk_drv.c
> >> @@ -20,6 +20,7 @@
> >>  #include <linux/major.h>
> >>  #include <linux/wait.h>
> >>  #include <linux/blkdev.h>
> >> +#include <linux/blkzoned.h>
> >>  #include <linux/init.h>
> >>  #include <linux/swap.h>
> >>  #include <linux/slab.h>
> >> @@ -51,10 +52,12 @@
> >>  		| UBLK_F_URING_CMD_COMP_IN_TASK \
> >>  		| UBLK_F_NEED_GET_DATA \
> >>  		| UBLK_F_USER_RECOVERY \
> >> -		| UBLK_F_USER_RECOVERY_REISSUE)
> >> +		| UBLK_F_USER_RECOVERY_REISSUE \
> >> +		| UBLK_F_ZONED)
> >>  
> >>  /* All UBLK_PARAM_TYPE_* should be included here */
> >> -#define UBLK_PARAM_TYPE_ALL (UBLK_PARAM_TYPE_BASIC | UBLK_PARAM_TYPE_DISCARD)
> >> +#define UBLK_PARAM_TYPE_ALL (UBLK_PARAM_TYPE_BASIC | UBLK_PARAM_TYPE_DISCARD \
> >> +			     | UBLK_PARAM_TYPE_ZONED)
> >>  
> >>  struct ublk_rq_data {
> >>  	struct llist_node node;
> >> @@ -187,6 +190,98 @@ static DEFINE_MUTEX(ublk_ctl_mutex);
> >>  
> >>  static struct miscdevice ublk_misc;
> >>  
> >> +#ifdef CONFIG_BLK_DEV_ZONED
> >> +static void ublk_set_nr_zones(struct ublk_device *ub)
> >> +{
> >> +	const struct ublk_param_basic *p = &ub->params.basic;
> >> +
> >> +	if (ub->dev_info.flags & UBLK_F_ZONED && p->chunk_sectors)
> >> +		ub->ub_disk->nr_zones = p->dev_sectors / p->chunk_sectors;
> >> +}
> >> +
> >> +static 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);
> >> +	}
> >> +}
> >> +
> >> +static int ublk_revalidate_disk_zones(struct gendisk *disk)
> >> +{
> >> +	return blk_revalidate_disk_zones(disk, NULL);
> >> +}
> >> +
> >> +static int ublk_report_zones(struct gendisk *disk, sector_t sector,
> >> +			     unsigned int nr_zones, report_zones_cb cb,
> >> +			     void *data)
> >> +{
> >> +	struct ublk_device *ub;
> >> +	unsigned int zone_size;
> >> +	unsigned int first_zone;
> >> +	int ret = 0;
> >> +
> >> +	ub = disk->private_data;
> >> +
> >> +	if (!(ub->dev_info.flags & UBLK_F_ZONED))
> >> +		return -EINVAL;
> >> +
> >> +	zone_size = disk->queue->limits.chunk_sectors;
> >> +	first_zone = sector >> ilog2(zone_size);
> >> +	nr_zones = min(ub->ub_disk->nr_zones - first_zone, nr_zones);
> >> +
> >> +	for (unsigned int i = 0; i < nr_zones; i++) {
> >
> > The local variable 'i' needs to be declared in the front part
> > of this function body.
> 
> Ok.
> 
> >
> >> +		struct request *req;
> >> +		blk_status_t status;
> >> +		struct blk_zone info = {0};
> >> +
> >> +		req = blk_mq_alloc_request(disk->queue, REQ_OP_DRV_IN, 0);
> >> +
> >> +		if (IS_ERR(req)) {
> >> +			ret = PTR_ERR(req);
> >> +			goto out;
> >> +		}
> >> +
> >> +		req->__sector = sector;
> >
> > Why is req->__sector set?
> 
> I use it to carry information about the first zone of the report request.

No, please do not use __sector in this way, which is supposed to be used by
driver for ZNS/ZONE_APPEND only.

> 
> >
> >> +
> >> +		ret = blk_rq_map_kern(disk->queue, req, &info, sizeof(info),
> >> +				      GFP_KERNEL);
> >> +
> >> +		if (ret)
> >> +			goto out;
> >> +
> >> +		status = blk_execute_rq(req, 0);
> >> +		ret = blk_status_to_errno(status);
> >> +		if (ret)
> >> +			goto out;
> >> +
> >> +		blk_mq_free_request(req);
> >> +
> >> +		ret = cb(&info, i, data);
> >> +		if (ret)
> >> +			goto out;
> >> +
> >> +		/* A zero length zone means don't ask for more zones */
> >> +		if (!info.len) {
> >> +			nr_zones = i;
> >> +			break;
> >> +		}
> >> +
> >> +		sector += zone_size;
> >> +	}
> >
> > I'd suggest to report as many as possible zones in one command, and
> > the dev_info.max_io_buf_bytes is the max allowed buffer size for one
> > command, please refer to nvme_ns_report_zones().
> 
> I agree about fetching more zones. However, it is no good to fetch up to
> a max, since the requested zone report may less than max. I was

Short read should always be supported, so the interface may need to 
return how many zones in single command, please refer to nvme_ns_report_zones().

> considering overloading req->__data_len and iod->nr_sectors to convey
> the number of requested zones. What do you think about that?

Any field pre-suffixed with "__" in 'struct request' should be only for
block layer, but __sector is one exception and its scenario is obvious.

So please do not misuse req->__data_len, but using iod is fine.

> 
> >
> > Also we are going to extend ublk in the multiple LUN/NS style, and I
> > guess that won't be one issue since ->report_zones() is always done on
> > disk level, right?
> 
> Yes, that should be fine.
> 
> >
> >> +	ret = nr_zones;
> >> +
> >> + out:
> >> +	return ret;
> >> +}
> >> +#else
> >> +void ublk_set_nr_zones(struct ublk_device *ub);
> >> +void ublk_dev_param_zoned_apply(struct ublk_device *ub);
> >> +int ublk_revalidate_disk_zones(struct gendisk *disk);
> >> +#endif
> >> +
> >>  static void ublk_dev_param_basic_apply(struct ublk_device *ub)
> >>  {
> >>  	struct request_queue *q = ub->ub_disk->queue;
> >> @@ -212,6 +307,9 @@ static void ublk_dev_param_basic_apply(struct ublk_device *ub)
> >>  		set_disk_ro(ub->ub_disk, true);
> >>  
> >>  	set_capacity(ub->ub_disk, p->dev_sectors);
> >> +
> >> +	if (IS_ENABLED(CONFIG_BLK_DEV_ZONED))
> >> +		ublk_set_nr_zones(ub);
> >>  }
> >>  
> >>  static void ublk_dev_param_discard_apply(struct ublk_device *ub)
> >> @@ -268,6 +366,9 @@ static int ublk_apply_params(struct ublk_device *ub)
> >>  	if (ub->params.types & UBLK_PARAM_TYPE_DISCARD)
> >>  		ublk_dev_param_discard_apply(ub);
> >>  
> >> +	if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) && (ub->params.types & UBLK_PARAM_TYPE_ZONED))
> >> +		ublk_dev_param_zoned_apply(ub);
> >> +
> >>  	return 0;
> >>  }
> >>  
> >> @@ -361,9 +462,13 @@ static void ublk_free_disk(struct gendisk *disk)
> >>  	put_device(&ub->cdev_dev);
> >>  }
> >>  
> >> +
> >>  static const struct block_device_operations ub_fops = {
> >> -	.owner =	THIS_MODULE,
> >> -	.free_disk =	ublk_free_disk,
> >> +	.owner = THIS_MODULE,
> >> +	.free_disk = ublk_free_disk,
> >> +#ifdef CONFIG_BLK_DEV_ZONED
> >> +	.report_zones = ublk_report_zones,
> >> +#endif
> >
> > Define one null ublk_report_zones in #else branch of the above #ifdef, then we
> > can save one #ifdef.
> 
> I would have to define it as a null pointer in the #else case then?

#define ublk_report_zones       NULL

> 
> >
> >>  };
> >>  
> >>  #define UBLK_MAX_PIN_PAGES	32
> >> @@ -499,7 +604,7 @@ static int ublk_unmap_io(const struct ublk_queue *ubq,
> >>  {
> >>  	const unsigned int rq_bytes = blk_rq_bytes(req);
> >>  
> >> -	if (req_op(req) == REQ_OP_READ && ublk_rq_has_data(req)) {
> >> +	if ((req_op(req) == REQ_OP_READ || req_op(req) == REQ_OP_DRV_IN) && ublk_rq_has_data(req)) {
> >>  		struct ublk_map_data data = {
> >>  			.ubq	=	ubq,
> >>  			.rq	=	req,
> >> @@ -566,6 +671,26 @@ static blk_status_t ublk_setup_iod(struct ublk_queue *ubq, struct request *req)
> >>  	case REQ_OP_WRITE_ZEROES:
> >>  		ublk_op = UBLK_IO_OP_WRITE_ZEROES;
> >>  		break;
> >> +#ifdef CONFIG_BLK_DEV_ZONED
> >> +	case REQ_OP_ZONE_OPEN:
> >> +		ublk_op = UBLK_IO_OP_ZONE_OPEN;
> >> +		break;
> >> +	case REQ_OP_ZONE_CLOSE:
> >> +		ublk_op = UBLK_IO_OP_ZONE_CLOSE;
> >> +		break;
> >> +	case REQ_OP_ZONE_FINISH:
> >> +		ublk_op = UBLK_IO_OP_ZONE_FINISH;
> >> +		break;
> >> +	case REQ_OP_ZONE_RESET:
> >> +		ublk_op = UBLK_IO_OP_ZONE_RESET;
> >> +		break;
> >> +	case REQ_OP_DRV_IN:
> >> +		ublk_op = UBLK_IO_OP_DRV_IN;
> >> +		break;
> >> +	case REQ_OP_ZONE_APPEND:
> >> +		/* We do not support zone append yet */
> >> +		fallthrough;
> >> +#endif
> >
> > The above '#ifdef' is needn't, since  OP_ZONE should be defined no
> > matter if CONFIG_BLK_DEV_ZONED is enabled or not.
> 
> I see. But do we want to process the requests if BLK_DEV_ZONED is not
> enabled, or do we want to fail the IO?

Here we should trust block core code.

> 
> >
> >>  	default:
> >>  		return BLK_STS_IOERR;
> >>  	}
> >> @@ -612,7 +737,8 @@ static void ublk_complete_rq(struct request *req)
> >>  	 *
> >>  	 * Both the two needn't unmap.
> >>  	 */
> >> -	if (req_op(req) != REQ_OP_READ && req_op(req) != REQ_OP_WRITE) {
> >> +	if (req_op(req) != REQ_OP_READ && req_op(req) != REQ_OP_WRITE &&
> >> +	    req_op(req) != REQ_OP_DRV_IN) {
> >>  		blk_mq_end_request(req, BLK_STS_OK);
> >>  		return;
> >>  	}
> >> @@ -1535,6 +1661,15 @@ static int ublk_ctrl_start_dev(struct io_uring_cmd *cmd)
> >>  	if (ret)
> >>  		goto out_put_disk;
> >>  
> >> +	if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) &&
> >> +	    ub->dev_info.flags & UBLK_F_ZONED) {
> >> +		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;
> >> +	}
> >> +
> >>  	get_device(&ub->cdev_dev);
> >>  	ret = add_disk(disk);
> >>  	if (ret) {
> >> @@ -1673,6 +1808,9 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd)
> >>  	if (!IS_BUILTIN(CONFIG_BLK_DEV_UBLK))
> >>  		ub->dev_info.flags |= UBLK_F_URING_CMD_COMP_IN_TASK;
> >>  
> >> +	if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED))
> >> +		ub->dev_info.flags &= ~UBLK_F_ZONED;
> >> +
> >>  	/* We are not ready to support zero copy */
> >>  	ub->dev_info.flags &= ~UBLK_F_SUPPORT_ZERO_COPY;
> >>  
> >> diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
> >> index 8f88e3a29998..074b97821575 100644
> >> --- a/include/uapi/linux/ublk_cmd.h
> >> +++ b/include/uapi/linux/ublk_cmd.h
> >> @@ -78,6 +78,10 @@
> >>  #define UBLK_F_USER_RECOVERY	(1UL << 3)
> >>  
> >>  #define UBLK_F_USER_RECOVERY_REISSUE	(1UL << 4)
> >> +/*
> >> + * Enable zoned device support
> >> + */
> >> +#define UBLK_F_ZONED (1ULL << 5)
> >>  
> >>  /* device state */
> >>  #define UBLK_S_DEV_DEAD	0
> >> @@ -129,6 +133,12 @@ struct ublksrv_ctrl_dev_info {
> >>  #define		UBLK_IO_OP_DISCARD	3
> >>  #define		UBLK_IO_OP_WRITE_SAME	4
> >>  #define		UBLK_IO_OP_WRITE_ZEROES	5
> >> +#define		UBLK_IO_OP_ZONE_OPEN		10
> >> +#define		UBLK_IO_OP_ZONE_CLOSE		11
> >> +#define		UBLK_IO_OP_ZONE_FINISH		12
> >> +#define		UBLK_IO_OP_ZONE_APPEND		13
> >> +#define		UBLK_IO_OP_ZONE_RESET		15
> >> +#define		UBLK_IO_OP_DRV_IN		34
> >>  
> >>  #define		UBLK_IO_F_FAILFAST_DEV		(1U << 8)
> >>  #define		UBLK_IO_F_FAILFAST_TRANSPORT	(1U << 9)
> >> @@ -214,6 +224,12 @@ struct ublk_param_discard {
> >>  	__u16	reserved0;
> >>  };
> >>  
> >> +struct ublk_param_zoned {
> >> +	__u64	max_open_zones;
> >> +	__u64	max_active_zones;
> >> +	__u64	max_append_size;
> >> +};
> >
> > Is the above zoned parameter enough for future extension?
> > Does ZNS need extra parameter? Or some zoned new(important) features?
> 
> @Damien, @Hans, @Matias, what do you think?
> 
> >
> > I highly suggest to reserve some fields for extension, given
> > it is one ABI interface, which is supposed to be defined well
> > enough from the beginning.
> 
> How many bytes would you reserve?

It really depends on storage/zoned field knowledge.


Thanks,
Ming
  

On Thu, Mar 02, 2023 at 04:32:21PM +0800, Ming Lei wrote:
> On Thu, Mar 02, 2023 at 08:31:07AM +0100, Andreas Hindborg wrote:
> > 

...

> > 
> > I agree about fetching more zones. However, it is no good to fetch up to
> > a max, since the requested zone report may less than max. I was
> 
> Short read should always be supported, so the interface may need to 
> return how many zones in single command, please refer to nvme_ns_report_zones().

blk_zone is part of uapi, maybe the short read can be figured out by
one all-zeroed 'blk_zone'?  then no extra uapi data is needed for
reporting zones.


thanks,
Ming
  

On Thu, Mar 2, 2023 at 5:02 PM Ming Lei <ming.lei@redhat.com> wrote:
>
> On Thu, Mar 02, 2023 at 04:32:21PM +0800, Ming Lei wrote:
> > On Thu, Mar 02, 2023 at 08:31:07AM +0100, Andreas Hindborg wrote:
> > >
>
> ...
>
> > >
> > > I agree about fetching more zones. However, it is no good to fetch up to
> > > a max, since the requested zone report may less than max. I was
> >
> > Short read should always be supported, so the interface may need to
> > return how many zones in single command, please refer to nvme_ns_report_zones().
>
> blk_zone is part of uapi, maybe the short read can be figured out by
> one all-zeroed 'blk_zone'?  then no extra uapi data is needed for
> reporting zones.

oops, we have blk_zone_report data for reporting zones to userspace already,
see blkdev_report_zones_ioctl(), then this way can be re-used for getting zone
report from ublk server too, right?

Thanks,
Ming
  

Ming Lei <ming.lei@redhat.com> writes:

> On Thu, Mar 2, 2023 at 5:02 PM Ming Lei <ming.lei@redhat.com> wrote:
>>
>> On Thu, Mar 02, 2023 at 04:32:21PM +0800, Ming Lei wrote:
>> > On Thu, Mar 02, 2023 at 08:31:07AM +0100, Andreas Hindborg wrote:
>> > >
>>
>> ...
>>
>> > >
>> > > I agree about fetching more zones. However, it is no good to fetch up to
>> > > a max, since the requested zone report may less than max. I was
>> >
>> > Short read should always be supported, so the interface may need to
>> > return how many zones in single command, please refer to nvme_ns_report_zones().
>>
>> blk_zone is part of uapi, maybe the short read can be figured out by
>> one all-zeroed 'blk_zone'?  then no extra uapi data is needed for
>> reporting zones.
>
> oops, we have blk_zone_report data for reporting zones to userspace already,
> see blkdev_report_zones_ioctl(), then this way can be re-used for getting zone
> report from ublk server too, right?

Yes that would be nice. But I did the report_zone command like a read
operation, so we are not currently copying any buffers to user space
when issuing the command, we just rely on the iod. I think it would be
better to use the start_sectors and nr_sectors of the iod instead. Then
we don't have to copy the blk_zone_report. What do you think?

BR Andreas
  

On Thu, Mar 02, 2023 at 11:07:15AM +0100, Andreas Hindborg wrote:
> 
> Ming Lei <ming.lei@redhat.com> writes:
> 
> > On Thu, Mar 2, 2023 at 5:02 PM Ming Lei <ming.lei@redhat.com> wrote:
> >>
> >> On Thu, Mar 02, 2023 at 04:32:21PM +0800, Ming Lei wrote:
> >> > On Thu, Mar 02, 2023 at 08:31:07AM +0100, Andreas Hindborg wrote:
> >> > >
> >>
> >> ...
> >>
> >> > >
> >> > > I agree about fetching more zones. However, it is no good to fetch up to
> >> > > a max, since the requested zone report may less than max. I was
> >> >
> >> > Short read should always be supported, so the interface may need to
> >> > return how many zones in single command, please refer to nvme_ns_report_zones().
> >>
> >> blk_zone is part of uapi, maybe the short read can be figured out by
> >> one all-zeroed 'blk_zone'?  then no extra uapi data is needed for
> >> reporting zones.
> >
> > oops, we have blk_zone_report data for reporting zones to userspace already,
> > see blkdev_report_zones_ioctl(), then this way can be re-used for getting zone
> > report from ublk server too, right?
> 
> Yes that would be nice. But I did the report_zone command like a read
> operation, so we are not currently copying any buffers to user space
> when issuing the command, we just rely on the iod.

What I meant is to reuse the format of blk_zone_report for returning
multiple 'blk_zone' info in single command.

The only change is that you need to allocate one bigger kernel buffer
to hold more 'blk_zone' in single report zone request.

> I think it would be
> better to use the start_sectors and nr_sectors of the iod instead. Then
> we don't have to copy the blk_zone_report. What do you think?

For IN parameter of report zone command, you still can reuse
blk_zone_report:

struct blk_zone_report {
        __u64           sector;
        __u32           nr_zones;
        __u32           flags;
};

Just by using the 1st two 64b words of iod for holding 'blk_zone_report', and
keep the iod->addr field not touched.


[1] https://lore.kernel.org/linux-block/20230301140611.163055-1-ming.lei@redhat.com/T/#md36358552d45a7d563940632d4c779a99f72916d

Thanks,
Ming
  

Ming Lei <ming.lei@redhat.com> writes:

> On Thu, Mar 02, 2023 at 11:07:15AM +0100, Andreas Hindborg wrote:
>> 
>> Ming Lei <ming.lei@redhat.com> writes:
>> 
>> > On Thu, Mar 2, 2023 at 5:02 PM Ming Lei <ming.lei@redhat.com> wrote:
>> >>
>> >> On Thu, Mar 02, 2023 at 04:32:21PM +0800, Ming Lei wrote:
>> >> > On Thu, Mar 02, 2023 at 08:31:07AM +0100, Andreas Hindborg wrote:
>> >> > >
>> >>
>> >> ...
>> >>
>> >> > >
>> >> > > I agree about fetching more zones. However, it is no good to fetch up to
>> >> > > a max, since the requested zone report may less than max. I was
>> >> >
>> >> > Short read should always be supported, so the interface may need to
>> >> > return how many zones in single command, please refer to nvme_ns_report_zones().
>> >>
>> >> blk_zone is part of uapi, maybe the short read can be figured out by
>> >> one all-zeroed 'blk_zone'?  then no extra uapi data is needed for
>> >> reporting zones.
>> >
>> > oops, we have blk_zone_report data for reporting zones to userspace already,
>> > see blkdev_report_zones_ioctl(), then this way can be re-used for getting zone
>> > report from ublk server too, right?
>> 
>> Yes that would be nice. But I did the report_zone command like a read
>> operation, so we are not currently copying any buffers to user space
>> when issuing the command, we just rely on the iod.
>
> What I meant is to reuse the format of blk_zone_report for returning
> multiple 'blk_zone' info in single command.
>
> The only change is that you need to allocate one bigger kernel buffer
> to hold more 'blk_zone' in single report zone request.
>
>> I think it would be
>> better to use the start_sectors and nr_sectors of the iod instead. Then
>> we don't have to copy the blk_zone_report. What do you think?
>
> For IN parameter of report zone command, you still can reuse
> blk_zone_report:
>
> struct blk_zone_report {
>         __u64           sector;
>         __u32           nr_zones;
>         __u32           flags;
> };
>
> Just by using the 1st two 64b words of iod for holding 'blk_zone_report', and
> keep the iod->addr field not touched.

I see. Would you make the first part of `struct ublksrv_io_desc` a union
for this, or would you just cast it at the use site?

BR Andreas
  

On Thu, Mar 02, 2023 at 02:28:33PM +0100, Andreas Hindborg wrote:
> 
> Ming Lei <ming.lei@redhat.com> writes:
> 
> > On Thu, Mar 02, 2023 at 11:07:15AM +0100, Andreas Hindborg wrote:
> >> 
> >> Ming Lei <ming.lei@redhat.com> writes:
> >> 
> >> > On Thu, Mar 2, 2023 at 5:02 PM Ming Lei <ming.lei@redhat.com> wrote:
> >> >>
> >> >> On Thu, Mar 02, 2023 at 04:32:21PM +0800, Ming Lei wrote:
> >> >> > On Thu, Mar 02, 2023 at 08:31:07AM +0100, Andreas Hindborg wrote:
> >> >> > >
> >> >>
> >> >> ...
> >> >>
> >> >> > >
> >> >> > > I agree about fetching more zones. However, it is no good to fetch up to
> >> >> > > a max, since the requested zone report may less than max. I was
> >> >> >
> >> >> > Short read should always be supported, so the interface may need to
> >> >> > return how many zones in single command, please refer to nvme_ns_report_zones().
> >> >>
> >> >> blk_zone is part of uapi, maybe the short read can be figured out by
> >> >> one all-zeroed 'blk_zone'?  then no extra uapi data is needed for
> >> >> reporting zones.
> >> >
> >> > oops, we have blk_zone_report data for reporting zones to userspace already,
> >> > see blkdev_report_zones_ioctl(), then this way can be re-used for getting zone
> >> > report from ublk server too, right?
> >> 
> >> Yes that would be nice. But I did the report_zone command like a read
> >> operation, so we are not currently copying any buffers to user space
> >> when issuing the command, we just rely on the iod.
> >
> > What I meant is to reuse the format of blk_zone_report for returning
> > multiple 'blk_zone' info in single command.
> >
> > The only change is that you need to allocate one bigger kernel buffer
> > to hold more 'blk_zone' in single report zone request.
> >
> >> I think it would be
> >> better to use the start_sectors and nr_sectors of the iod instead. Then
> >> we don't have to copy the blk_zone_report. What do you think?
> >
> > For IN parameter of report zone command, you still can reuse
> > blk_zone_report:
> >
> > struct blk_zone_report {
> >         __u64           sector;
> >         __u32           nr_zones;
> >         __u32           flags;
> > };
> >
> > Just by using the 1st two 64b words of iod for holding 'blk_zone_report', and
> > keep the iod->addr field not touched.
> 
> I see. Would you make the first part of `struct ublksrv_io_desc` a union
> for this, or would you just cast it at the use site?

oops, you still need iod->op_flags for recognizing the io op, so just
start_sector and nr_sectors can be used.

However, this way isn't good too, cause UBLK_IO_OP_DRV_IN is just mapped
to 'report zone' command in your implementation, what if new pt request
is required in future?

We need to think about how to support ublk pt request in generic way.

Thanks,
Ming
  

Ming Lei <ming.lei@redhat.com> writes:

> On Thu, Mar 02, 2023 at 02:28:33PM +0100, Andreas Hindborg wrote:
>> 
>> Ming Lei <ming.lei@redhat.com> writes:
>> 
>> > On Thu, Mar 02, 2023 at 11:07:15AM +0100, Andreas Hindborg wrote:
>> >> 
>> >> Ming Lei <ming.lei@redhat.com> writes:
>> >> 
>> >> > On Thu, Mar 2, 2023 at 5:02 PM Ming Lei <ming.lei@redhat.com> wrote:
>> >> >>
>> >> >> On Thu, Mar 02, 2023 at 04:32:21PM +0800, Ming Lei wrote:
>> >> >> > On Thu, Mar 02, 2023 at 08:31:07AM +0100, Andreas Hindborg wrote:
>> >> >> > >
>> >> >>
>> >> >> ...
>> >> >>
>> >> >> > >
>> >> >> > > I agree about fetching more zones. However, it is no good to fetch up to
>> >> >> > > a max, since the requested zone report may less than max. I was
>> >> >> >
>> >> >> > Short read should always be supported, so the interface may need to
>> >> >> > return how many zones in single command, please refer to nvme_ns_report_zones().
>> >> >>
>> >> >> blk_zone is part of uapi, maybe the short read can be figured out by
>> >> >> one all-zeroed 'blk_zone'?  then no extra uapi data is needed for
>> >> >> reporting zones.
>> >> >
>> >> > oops, we have blk_zone_report data for reporting zones to userspace already,
>> >> > see blkdev_report_zones_ioctl(), then this way can be re-used for getting zone
>> >> > report from ublk server too, right?
>> >> 
>> >> Yes that would be nice. But I did the report_zone command like a read
>> >> operation, so we are not currently copying any buffers to user space
>> >> when issuing the command, we just rely on the iod.
>> >
>> > What I meant is to reuse the format of blk_zone_report for returning
>> > multiple 'blk_zone' info in single command.
>> >
>> > The only change is that you need to allocate one bigger kernel buffer
>> > to hold more 'blk_zone' in single report zone request.
>> >
>> >> I think it would be
>> >> better to use the start_sectors and nr_sectors of the iod instead. Then
>> >> we don't have to copy the blk_zone_report. What do you think?
>> >
>> > For IN parameter of report zone command, you still can reuse
>> > blk_zone_report:
>> >
>> > struct blk_zone_report {
>> >         __u64           sector;
>> >         __u32           nr_zones;
>> >         __u32           flags;
>> > };
>> >
>> > Just by using the 1st two 64b words of iod for holding 'blk_zone_report', and
>> > keep the iod->addr field not touched.
>> 
>> I see. Would you make the first part of `struct ublksrv_io_desc` a union
>> for this, or would you just cast it at the use site?
>
> oops, you still need iod->op_flags for recognizing the io op, so just
> start_sector and nr_sectors can be used.

We do not actually need to pass the flags to user space, or back from
user space to kernel for ublk zone report. They are currently used to
tell user space if the zone report contains capacity field. We could
exclude them from the ublk kabi since the zone report will always
contain capacity? But it might be good to have a flags field or future
things.

> However, this way isn't good too, cause UBLK_IO_OP_DRV_IN is just mapped
> to 'report zone' command in your implementation, what if new pt request
> is required in future?

We are currently mapping REQ_OP_* 1:1 to  UBLK_OP_OP_*. If we relax
this, we can have a UBLK_IO_OP_REPORT_ZONES.

>
> We need to think about how to support ublk pt request in generic way.

Another option is to allow REQ_OP_DRV_IN to pass a buffer to user space.
Instead of being similar to a read operation, it could be a combination of
a read and a write operation.

BR Andreas
  

On Fri, Mar 03, 2023 at 09:27:58AM +0100, Andreas Hindborg wrote:
> 
> Ming Lei <ming.lei@redhat.com> writes:
> 
> > On Thu, Mar 02, 2023 at 02:28:33PM +0100, Andreas Hindborg wrote:
> >> 
> >> Ming Lei <ming.lei@redhat.com> writes:
> >> 
> >> > On Thu, Mar 02, 2023 at 11:07:15AM +0100, Andreas Hindborg wrote:
> >> >> 
> >> >> Ming Lei <ming.lei@redhat.com> writes:
> >> >> 
> >> >> > On Thu, Mar 2, 2023 at 5:02 PM Ming Lei <ming.lei@redhat.com> wrote:
> >> >> >>
> >> >> >> On Thu, Mar 02, 2023 at 04:32:21PM +0800, Ming Lei wrote:
> >> >> >> > On Thu, Mar 02, 2023 at 08:31:07AM +0100, Andreas Hindborg wrote:
> >> >> >> > >
> >> >> >>
> >> >> >> ...
> >> >> >>
> >> >> >> > >
> >> >> >> > > I agree about fetching more zones. However, it is no good to fetch up to
> >> >> >> > > a max, since the requested zone report may less than max. I was
> >> >> >> >
> >> >> >> > Short read should always be supported, so the interface may need to
> >> >> >> > return how many zones in single command, please refer to nvme_ns_report_zones().
> >> >> >>
> >> >> >> blk_zone is part of uapi, maybe the short read can be figured out by
> >> >> >> one all-zeroed 'blk_zone'?  then no extra uapi data is needed for
> >> >> >> reporting zones.
> >> >> >
> >> >> > oops, we have blk_zone_report data for reporting zones to userspace already,
> >> >> > see blkdev_report_zones_ioctl(), then this way can be re-used for getting zone
> >> >> > report from ublk server too, right?
> >> >> 
> >> >> Yes that would be nice. But I did the report_zone command like a read
> >> >> operation, so we are not currently copying any buffers to user space
> >> >> when issuing the command, we just rely on the iod.
> >> >
> >> > What I meant is to reuse the format of blk_zone_report for returning
> >> > multiple 'blk_zone' info in single command.
> >> >
> >> > The only change is that you need to allocate one bigger kernel buffer
> >> > to hold more 'blk_zone' in single report zone request.
> >> >
> >> >> I think it would be
> >> >> better to use the start_sectors and nr_sectors of the iod instead. Then
> >> >> we don't have to copy the blk_zone_report. What do you think?
> >> >
> >> > For IN parameter of report zone command, you still can reuse
> >> > blk_zone_report:
> >> >
> >> > struct blk_zone_report {
> >> >         __u64           sector;
> >> >         __u32           nr_zones;
> >> >         __u32           flags;
> >> > };
> >> >
> >> > Just by using the 1st two 64b words of iod for holding 'blk_zone_report', and
> >> > keep the iod->addr field not touched.
> >> 
> >> I see. Would you make the first part of `struct ublksrv_io_desc` a union
> >> for this, or would you just cast it at the use site?
> >
> > oops, you still need iod->op_flags for recognizing the io op, so just
> > start_sector and nr_sectors can be used.
> 
> We do not actually need to pass the flags to user space, or back from
> user space to kernel for ublk zone report. They are currently used to
> tell user space if the zone report contains capacity field. We could
> exclude them from the ublk kabi since the zone report will always
> contain capacity? But it might be good to have a flags field or future
> things.
> 
> > However, this way isn't good too, cause UBLK_IO_OP_DRV_IN is just mapped
> > to 'report zone' command in your implementation, what if new pt request
> > is required in future?
> 
> We are currently mapping REQ_OP_* 1:1 to  UBLK_OP_OP_*. If we relax
> this, we can have a UBLK_IO_OP_REPORT_ZONES.

The op takes 8 bits, and enough to cover all normal block layer OPs and
driver specific OPs, so I'd suggest this way, and ublk device
specific OP can be started from 32, prefixed with

	UBLK_IO_OP_DRV_IN				//[32, 96)
or
	UBLK_IO_OP_DRV_OUT				//[96, 160)

such as, report zones can be defined as

	enum {
		__UBLK_IO_OP_DRV_IN_START = 32,
		UBLK_IO_OP_DRV_IN_REPORT_ZONES = __UBLK_IO_OP_DRV_IN_START,
		__UBLK_IO_OP_DRV_IN_END = 96,

		__UBLK_IO_OP_DRV_OUT_START = __UBLK_IO_OP_DRV_IN_END,
		__UBLK_IO_OP_DRV_OUT_END = 160,
	};

For any DRV OPs, iod header(not include ->addr) and buffer format can be re-defined
as uapi structure.

What do you think of this way?

> 
> >
> > We need to think about how to support ublk pt request in generic way.
> 
> Another option is to allow REQ_OP_DRV_IN to pass a buffer to user space.
> Instead of being similar to a read operation, it could be a combination of
> a read and a write operation.

That might be more flexible, but could add driver & userspace's
complexity, so I'd suggest to try to avoid bidirectional buffer asap,
but we still reserve support for it via UBLK_IO_OP_DRV_IN_OUT*.

Thanks,
Ming