| Message ID | 20230628190649.11233-4-nmi@metaspace.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ublk: add zoned storage support | expand |
On 6/29/23 04:06, Andreas Hindborg wrote: > From: Andreas Hindborg <a.hindborg@samsung.com> > > 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 > > Note: This commit changes the ublk kernel module name from `ublk_drv.ko` to > `ublk.ko` in order to link multiple translation units into the module. You probably could rename ublk_drv.c to ublk.c to avoid that and keep the module name as it was, ublk_drv.ko. > > Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com> > --- > MAINTAINERS | 1 + > drivers/block/Kconfig | 4 + > drivers/block/Makefile | 4 +- > drivers/block/ublk_drv-zoned.c | 144 +++++++++++++++++++++++++++++++++ > drivers/block/ublk_drv.c | 64 +++++++++++++-- > drivers/block/ublk_drv.h | 15 ++++ > include/uapi/linux/ublk_cmd.h | 14 ++++ > 7 files changed, 239 insertions(+), 7 deletions(-) > create mode 100644 drivers/block/ublk_drv-zoned.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index ace71c90751c..db8a8deb5926 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -21555,6 +21555,7 @@ S: Maintained > F: Documentation/block/ublk.rst > F: drivers/block/ublk_drv.c > F: drivers/block/ublk_drv.h > +F: drivers/block/ublk_drv-zoned.c > F: include/uapi/linux/ublk_cmd.h > > UCLINUX (M68KNOMMU AND COLDFIRE) > diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig > index 5b9d4aaebb81..c58dfd035557 100644 > --- a/drivers/block/Kconfig > +++ b/drivers/block/Kconfig > @@ -402,6 +402,10 @@ config BLKDEV_UBLK_LEGACY_OPCODES > suggested to enable N if your application(ublk server) switches to > ioctl command encoding. > > +config BLK_DEV_UBLK_ZONED > + def_bool y This can be "bool" only. > + depends on BLK_DEV_UBLK && BLK_DEV_ZONED > + > source "drivers/block/rnbd/Kconfig" > > endif # BLK_DEV > diff --git a/drivers/block/Makefile b/drivers/block/Makefile > index 101612cba303..bc1649e20ec2 100644 > --- a/drivers/block/Makefile > +++ b/drivers/block/Makefile > @@ -37,6 +37,8 @@ obj-$(CONFIG_BLK_DEV_RNBD) += rnbd/ > > obj-$(CONFIG_BLK_DEV_NULL_BLK) += null_blk/ > > -obj-$(CONFIG_BLK_DEV_UBLK) += ublk_drv.o > +obj-$(CONFIG_BLK_DEV_UBLK) += ublk.o > +ublk-$(CONFIG_BLK_DEV_UBLK) += ublk_drv.o > +ublk-$(CONFIG_BLK_DEV_UBLK_ZONED) += ublk_drv-zoned.o > > swim_mod-y := swim.o swim_asm.o > diff --git a/drivers/block/ublk_drv-zoned.c b/drivers/block/ublk_drv-zoned.c > new file mode 100644 > index 000000000000..ea86bf4b3681 > --- /dev/null > +++ b/drivers/block/ublk_drv-zoned.c > @@ -0,0 +1,144 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright 2023 Andreas Hindborg <a.hindborg@samsung.com> > + */ > +#include <linux/blkzoned.h> > +#include <linux/ublk_cmd.h> > +#include <linux/vmalloc.h> > + > +#include "ublk_drv.h" > + > +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) If UBLK_F_ZONED is set but chunk_sectors is not, that is a bug on the user driver side, no ? So an error message about that would be nice instead of silently ignoring the zoned flag. > + ub->ub_disk->nr_zones = p->dev_sectors / p->chunk_sectors; > +} > + > +void ublk_dev_param_zoned_apply(struct ublk_device *ub) > +{ > + const struct ublk_param_zoned *p = &ub->params.zoned; > + > + if (ub->dev_info.flags & UBLK_F_ZONED) { > + disk_set_max_active_zones(ub->ub_disk, p->max_active_zones); > + disk_set_max_open_zones(ub->ub_disk, p->max_open_zones); You do not need to check if the max_active_zones and max_open_zones values are sensible ? E.g. what if they are larger than the number of zones ? > + } > +} > + > +int ublk_revalidate_disk_zones(struct gendisk *disk) > +{ > + return blk_revalidate_disk_zones(disk, NULL); > +} I do not think this helper is needed at all (see below comment on the call site). > + > +/* Based on virtblk_alloc_report_buffer */ > +static void *ublk_alloc_report_buffer(struct ublk_device *ublk, > + unsigned int nr_zones, > + unsigned int zone_sectors, size_t *buflen) > +{ > + struct request_queue *q = ublk->ub_disk->queue; > + size_t bufsize; > + void *buf; > + > + nr_zones = min_t(unsigned int, nr_zones, > + get_capacity(ublk->ub_disk) >> ilog2(zone_sectors)); > + > + bufsize = nr_zones * sizeof(struct blk_zone); > + bufsize = > + min_t(size_t, bufsize, queue_max_hw_sectors(q) << SECTOR_SHIFT); > + bufsize = min_t(size_t, bufsize, queue_max_segments(q) << PAGE_SHIFT); > + > + while (bufsize >= sizeof(struct blk_zone)) { > + buf = __vmalloc(bufsize, GFP_KERNEL | __GFP_NORETRY); > + if (buf) { > + *buflen = bufsize; > + return buf; > + } > + bufsize >>= 1; > + } > + > + bufsize = 0; This is not needed. This should rather be "*buflen = 0;". > + return NULL; > +} > + > +int ublk_report_zones(struct gendisk *disk, sector_t sector, > + unsigned int nr_zones, report_zones_cb cb, void *data) > +{ > + struct ublk_device *ub = disk->private_data; > + unsigned int zone_size_sectors = disk->queue->limits.chunk_sectors; > + unsigned int first_zone = sector >> ilog2(zone_size_sectors); > + unsigned int done_zones = 0; > + unsigned int max_zones_per_request; > + struct blk_zone *buffer; > + size_t buffer_length; > + > + if (!(ub->dev_info.flags & UBLK_F_ZONED)) This is repeated a lot. So a small inline helper ublk_dev_is_zoned() would be nice. > + return -EOPNOTSUPP; > + > + nr_zones = min_t(unsigned int, ub->ub_disk->nr_zones - first_zone, > + nr_zones); > + > + buffer = ublk_alloc_report_buffer(ub, nr_zones, zone_size_sectors, > + &buffer_length); > + if (!buffer) > + return -ENOMEM; > + > + max_zones_per_request = buffer_length / sizeof(struct blk_zone); > + > + while (done_zones < nr_zones) { > + unsigned int remaining_zones = nr_zones - done_zones; > + unsigned int zones_in_request = min_t( > + unsigned int, remaining_zones, max_zones_per_request); > + int err = 0; > + struct request *req; > + struct ublk_rq_data *pdu; > + blk_status_t status; > + > + memset(buffer, 0, buffer_length); > + > + req = blk_mq_alloc_request(disk->queue, REQ_OP_DRV_IN, 0); > + if (IS_ERR(req)) > + return PTR_ERR(req); You are leaking buffer. > + > + pdu = blk_mq_rq_to_pdu(req); > + pdu->operation = UBLK_IO_OP_REPORT_ZONES; > + pdu->sector = sector; > + pdu->nr_sectors = remaining_zones * zone_size_sectors; > + > + err = blk_rq_map_kern(disk->queue, req, buffer, buffer_length, > + GFP_KERNEL); > + if (err) { > + blk_mq_free_request(req); > + kvfree(buffer); > + return err; > + } > + > + status = blk_execute_rq(req, 0); > + err = blk_status_to_errno(status); > + blk_mq_free_request(req); > + if (err) { > + kvfree(buffer); > + return err; You are repeating this a lot. Use a goto to cleanup on error. > + } > + > + for (unsigned int i = 0; i < zones_in_request; i++) { > + struct blk_zone *zone = buffer + i; > + > + err = cb(zone, i, data); > + if (err) > + return err; > + > + done_zones++; > + sector += zone_size_sectors; > + > + /* A zero length zone means don't ask for more zones */ > + if (!zone->len) { > + kvfree(buffer); > + return done_zones; > + } > + } > + } > + > + kvfree(buffer); > + return done_zones; > +} > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c > index e519dc0d9fe7..88fa39853c61 100644 > --- a/drivers/block/ublk_drv.c > +++ b/drivers/block/ublk_drv.c > @@ -57,12 +57,13 @@ > | UBLK_F_USER_RECOVERY_REISSUE \ > | UBLK_F_UNPRIVILEGED_DEV \ > | UBLK_F_CMD_IOCTL_ENCODE \ > - | UBLK_F_USER_COPY) > + | UBLK_F_USER_COPY \ > + | UBLK_F_ZONED) > > /* All UBLK_PARAM_TYPE_* should be included here */ > -#define UBLK_PARAM_TYPE_ALL (UBLK_PARAM_TYPE_BASIC | \ > - UBLK_PARAM_TYPE_DISCARD | UBLK_PARAM_TYPE_DEVT) > - > +#define UBLK_PARAM_TYPE_ALL \ > + (UBLK_PARAM_TYPE_BASIC | UBLK_PARAM_TYPE_DISCARD | \ > + UBLK_PARAM_TYPE_DEVT | UBLK_PARAM_TYPE_ZONED) > > struct ublk_uring_cmd_pdu { > struct ublk_queue *ubq; > @@ -209,6 +210,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); So if the user is attempting to setup a zoned drive but the kernel does not have CONFIG_BLK_DEV_ZONED=y, the user setup will be silently ignored. Not exactly nice I think, unless I am missing something. Also, repeating that "if (IS_ENABLED(CONFIG_BLK_DEV_ZONED))" for all zone related functions is very verbose. Stub the functions in ublk_drv.h. That will make the main C code lighter. > } > > static void ublk_dev_param_discard_apply(struct ublk_device *ub) > @@ -269,6 +273,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); Similar comment as above. If the user tries to apply zoned parameters to a non zoned drive, no error reported... > + > return 0; > } > > @@ -439,6 +446,7 @@ static const struct block_device_operations ub_fops = { > .owner = THIS_MODULE, > .open = ublk_open, > .free_disk = ublk_free_disk, > + .report_zones = ublk_report_zones, > }; > > #define UBLK_MAX_PIN_PAGES 32 > @@ -553,7 +561,8 @@ static inline bool ublk_need_map_req(const struct request *req) > > static inline bool ublk_need_unmap_req(const struct request *req) > { > - return ublk_rq_has_data(req) && req_op(req) == REQ_OP_READ; > + return ublk_rq_has_data(req) && > + (req_op(req) == REQ_OP_READ || req_op(req) == REQ_OP_DRV_IN); > } > > static int ublk_map_io(const struct ublk_queue *ubq, const struct request *req, > @@ -637,6 +646,7 @@ static blk_status_t ublk_setup_iod(struct ublk_queue *ubq, struct request *req) > { > struct ublksrv_io_desc *iod = ublk_get_iod(ubq, req->tag); > struct ublk_io *io = &ubq->ios[req->tag]; > + struct ublk_rq_data *pdu = blk_mq_rq_to_pdu(req); > u32 ublk_op; > > switch (req_op(req)) { > @@ -655,6 +665,35 @@ 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; > + 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 = pdu->operation; > + switch (ublk_op) { > + case UBLK_IO_OP_REPORT_ZONES: > + iod->op_flags = ublk_op | ublk_req_build_flags(req); > + iod->nr_sectors = pdu->nr_sectors; > + iod->start_sector = pdu->sector; > + iod->addr = io->addr; > + return BLK_STS_OK; > + default: > + return BLK_STS_IOERR; > + } > + case REQ_OP_ZONE_APPEND: > + case REQ_OP_ZONE_RESET_ALL: > + case REQ_OP_DRV_OUT: > + /* We do not support zone append or reset_all yet */ > + fallthrough; Not OK ! zone append is mandatory for zoned block devices. So zone append support needs to come with this patch. reset all can be a different patch as that is optional. > default: > return BLK_STS_IOERR; > } > @@ -708,7 +747,8 @@ static inline 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) > goto exit; > > /* for READ request, writing data in iod->addr to rq buffers */ > @@ -1835,6 +1875,15 @@ static int ublk_ctrl_start_dev(struct ublk_device *ub, struct io_uring_cmd *cmd) > if (ub->nr_privileged_daemon != ub->nr_queues_ready) > set_bit(GD_SUPPRESS_PART_SCAN, &disk->state); > > + if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) && > + ub->dev_info.flags & UBLK_F_ZONED) { Same comment as above about error return instead of silently ignoring the zoned flag for the CONFIG_BLK_DEV_ZONED=n case. > + disk_set_zoned(disk, BLK_ZONED_HM); > + blk_queue_required_elevator_features(disk->queue, ELEVATOR_F_ZBD_SEQ_WRITE); > + ret = ublk_revalidate_disk_zones(disk); > + if (ret) > + goto out_put_disk; This should be all a helper ublk_set_zoned() or something. > + } > + > get_device(&ub->cdev_dev); > ub->dev_info.state = UBLK_S_DEV_LIVE; > ret = add_disk(disk); > @@ -1997,6 +2046,9 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd) > if (ub->dev_info.flags & UBLK_F_USER_COPY) > ub->dev_info.flags &= ~UBLK_F_NEED_GET_DATA; > > + if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED)) > + ub->dev_info.flags &= ~UBLK_F_ZONED; Arg, no. The user should be notified with an error that he/she is attempting to create a zoned device that cannot be supported. > + > /* We are not ready to support zero copy */ > ub->dev_info.flags &= ~UBLK_F_SUPPORT_ZERO_COPY; > > diff --git a/drivers/block/ublk_drv.h b/drivers/block/ublk_drv.h > index f81e62256456..7242430fd6b9 100644 > --- a/drivers/block/ublk_drv.h > +++ b/drivers/block/ublk_drv.h > @@ -50,6 +50,21 @@ struct ublk_rq_data { > struct llist_node node; > > struct kref ref; > + enum ublk_op operation; > + __u64 sector; > + __u32 nr_sectors; > }; > > +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); > + > +#ifdef CONFIG_BLK_DEV_UBLK_ZONED > +int ublk_report_zones(struct gendisk *disk, sector_t sector, > + unsigned int nr_zones, report_zones_cb cb, > + void *data); > +#else > +#define ublk_report_zones NULL > +#endif > + > #endif > diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h > index 471b3b983045..436525afffe8 100644 > --- a/include/uapi/linux/ublk_cmd.h > +++ b/include/uapi/linux/ublk_cmd.h > @@ -176,6 +176,11 @@ > /* Copy between request and user buffer by pread()/pwrite() */ > #define UBLK_F_USER_COPY (1UL << 7) > > +/* > + * Enable zoned device support Isn't this for "Indicate that the device is zoned" ? > + */ > +#define UBLK_F_ZONED (1ULL << 8) > + > /* device state */ > #define UBLK_S_DEV_DEAD 0 > #define UBLK_S_DEV_LIVE 1 > @@ -242,6 +247,7 @@ enum ublk_op { > UBLK_IO_OP_ZONE_APPEND = 13, > UBLK_IO_OP_ZONE_RESET = 15, > __UBLK_IO_OP_DRV_IN_START = 32, > + UBLK_IO_OP_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, > @@ -342,6 +348,12 @@ struct ublk_param_devt { > __u32 disk_minor; > }; > > +struct ublk_param_zoned { > + __u32 max_open_zones; > + __u32 max_active_zones; > + __u8 reserved[24]; > +}; > + > struct ublk_params { > /* > * Total length of parameters, userspace has to set 'len' for both > @@ -353,11 +365,13 @@ struct ublk_params { > #define UBLK_PARAM_TYPE_BASIC (1 << 0) > #define UBLK_PARAM_TYPE_DISCARD (1 << 1) > #define UBLK_PARAM_TYPE_DEVT (1 << 2) > +#define UBLK_PARAM_TYPE_ZONED (1 << 3) > __u32 types; /* types of parameter included */ > > struct ublk_param_basic basic; > struct ublk_param_discard discard; > struct ublk_param_devt devt; > + struct ublk_param_zoned zoned; > }; > > #endif
On Wed, Jun 28, 2023 at 09:06:48PM +0200, Andreas Hindborg wrote: > From: Andreas Hindborg <a.hindborg@samsung.com> > > 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 > > Note: This commit changes the ublk kernel module name from `ublk_drv.ko` to > `ublk.ko` in order to link multiple translation units into the module. > > Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com> > --- > MAINTAINERS | 1 + > drivers/block/Kconfig | 4 + > drivers/block/Makefile | 4 +- > drivers/block/ublk_drv-zoned.c | 144 +++++++++++++++++++++++++++++++++ > drivers/block/ublk_drv.c | 64 +++++++++++++-- > drivers/block/ublk_drv.h | 15 ++++ > include/uapi/linux/ublk_cmd.h | 14 ++++ > 7 files changed, 239 insertions(+), 7 deletions(-) > create mode 100644 drivers/block/ublk_drv-zoned.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index ace71c90751c..db8a8deb5926 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -21555,6 +21555,7 @@ S: Maintained > F: Documentation/block/ublk.rst > F: drivers/block/ublk_drv.c > F: drivers/block/ublk_drv.h > +F: drivers/block/ublk_drv-zoned.c > F: include/uapi/linux/ublk_cmd.h > > UCLINUX (M68KNOMMU AND COLDFIRE) > diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig > index 5b9d4aaebb81..c58dfd035557 100644 > --- a/drivers/block/Kconfig > +++ b/drivers/block/Kconfig > @@ -402,6 +402,10 @@ config BLKDEV_UBLK_LEGACY_OPCODES > suggested to enable N if your application(ublk server) switches to > ioctl command encoding. > > +config BLK_DEV_UBLK_ZONED > + def_bool y > + depends on BLK_DEV_UBLK && BLK_DEV_ZONED > + > source "drivers/block/rnbd/Kconfig" > > endif # BLK_DEV > diff --git a/drivers/block/Makefile b/drivers/block/Makefile > index 101612cba303..bc1649e20ec2 100644 > --- a/drivers/block/Makefile > +++ b/drivers/block/Makefile > @@ -37,6 +37,8 @@ obj-$(CONFIG_BLK_DEV_RNBD) += rnbd/ > > obj-$(CONFIG_BLK_DEV_NULL_BLK) += null_blk/ > > -obj-$(CONFIG_BLK_DEV_UBLK) += ublk_drv.o > +obj-$(CONFIG_BLK_DEV_UBLK) += ublk.o > +ublk-$(CONFIG_BLK_DEV_UBLK) += ublk_drv.o > +ublk-$(CONFIG_BLK_DEV_UBLK_ZONED) += ublk_drv-zoned.o > > swim_mod-y := swim.o swim_asm.o > diff --git a/drivers/block/ublk_drv-zoned.c b/drivers/block/ublk_drv-zoned.c > new file mode 100644 > index 000000000000..ea86bf4b3681 > --- /dev/null > +++ b/drivers/block/ublk_drv-zoned.c > @@ -0,0 +1,144 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright 2023 Andreas Hindborg <a.hindborg@samsung.com> > + */ > +#include <linux/blkzoned.h> > +#include <linux/ublk_cmd.h> > +#include <linux/vmalloc.h> > + > +#include "ublk_drv.h" > + > +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; > +} > + > +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); > + } > +} > + > +int ublk_revalidate_disk_zones(struct gendisk *disk) > +{ > + return blk_revalidate_disk_zones(disk, NULL); > +} > + > +/* Based on virtblk_alloc_report_buffer */ > +static void *ublk_alloc_report_buffer(struct ublk_device *ublk, > + unsigned int nr_zones, > + unsigned int zone_sectors, size_t *buflen) > +{ > + struct request_queue *q = ublk->ub_disk->queue; > + size_t bufsize; > + void *buf; > + > + nr_zones = min_t(unsigned int, nr_zones, > + get_capacity(ublk->ub_disk) >> ilog2(zone_sectors)); > + > + bufsize = nr_zones * sizeof(struct blk_zone); > + bufsize = > + min_t(size_t, bufsize, queue_max_hw_sectors(q) << SECTOR_SHIFT); > + bufsize = min_t(size_t, bufsize, queue_max_segments(q) << PAGE_SHIFT); This means 'blk_zone'(includes any sub-field type) is reused as part of ublk's UAPI, and it is smart, please document it around UBLK_IO_OP_REPORT_ZONES's definition. > + > + while (bufsize >= sizeof(struct blk_zone)) { > + buf = __vmalloc(bufsize, GFP_KERNEL | __GFP_NORETRY); > + if (buf) { > + *buflen = bufsize; > + return buf; > + } > + bufsize >>= 1; > + } > + > + bufsize = 0; > + return NULL; > +} > + > +int ublk_report_zones(struct gendisk *disk, sector_t sector, > + unsigned int nr_zones, report_zones_cb cb, void *data) > +{ > + struct ublk_device *ub = disk->private_data; > + unsigned int zone_size_sectors = disk->queue->limits.chunk_sectors; > + unsigned int first_zone = sector >> ilog2(zone_size_sectors); > + unsigned int done_zones = 0; > + unsigned int max_zones_per_request; > + struct blk_zone *buffer; > + size_t buffer_length; > + > + if (!(ub->dev_info.flags & UBLK_F_ZONED)) > + return -EOPNOTSUPP; > + > + nr_zones = min_t(unsigned int, ub->ub_disk->nr_zones - first_zone, > + nr_zones); > + > + buffer = ublk_alloc_report_buffer(ub, nr_zones, zone_size_sectors, > + &buffer_length); > + if (!buffer) > + return -ENOMEM; > + > + max_zones_per_request = buffer_length / sizeof(struct blk_zone); > + > + while (done_zones < nr_zones) { > + unsigned int remaining_zones = nr_zones - done_zones; > + unsigned int zones_in_request = min_t( > + unsigned int, remaining_zones, max_zones_per_request); > + int err = 0; > + struct request *req; > + struct ublk_rq_data *pdu; > + blk_status_t status; > + > + memset(buffer, 0, buffer_length); > + > + req = blk_mq_alloc_request(disk->queue, REQ_OP_DRV_IN, 0); > + if (IS_ERR(req)) > + return PTR_ERR(req); > + > + pdu = blk_mq_rq_to_pdu(req); > + pdu->operation = UBLK_IO_OP_REPORT_ZONES; > + pdu->sector = sector; > + pdu->nr_sectors = remaining_zones * zone_size_sectors; > + > + err = blk_rq_map_kern(disk->queue, req, buffer, buffer_length, > + GFP_KERNEL); > + if (err) { > + blk_mq_free_request(req); > + kvfree(buffer); > + return err; > + } > + > + status = blk_execute_rq(req, 0); > + err = blk_status_to_errno(status); > + blk_mq_free_request(req); > + if (err) { > + kvfree(buffer); > + return err; > + } > + > + for (unsigned int i = 0; i < zones_in_request; i++) { > + struct blk_zone *zone = buffer + i; > + > + err = cb(zone, i, data); > + if (err) > + return err; > + > + done_zones++; > + sector += zone_size_sectors; > + > + /* A zero length zone means don't ask for more zones */ > + if (!zone->len) { This way is part of UAPI UBLK_IO_OP_REPORT_ZONES, please document it around UBLK_IO_OP_REPORT_ZONES. > + kvfree(buffer); > + return done_zones; > + } > + } > + } > + > + kvfree(buffer); > + return done_zones; > +} > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c > index e519dc0d9fe7..88fa39853c61 100644 > --- a/drivers/block/ublk_drv.c > +++ b/drivers/block/ublk_drv.c > @@ -57,12 +57,13 @@ > | UBLK_F_USER_RECOVERY_REISSUE \ > | UBLK_F_UNPRIVILEGED_DEV \ > | UBLK_F_CMD_IOCTL_ENCODE \ > - | UBLK_F_USER_COPY) > + | UBLK_F_USER_COPY \ > + | UBLK_F_ZONED) > > /* All UBLK_PARAM_TYPE_* should be included here */ > -#define UBLK_PARAM_TYPE_ALL (UBLK_PARAM_TYPE_BASIC | \ > - UBLK_PARAM_TYPE_DISCARD | UBLK_PARAM_TYPE_DEVT) > - > +#define UBLK_PARAM_TYPE_ALL \ > + (UBLK_PARAM_TYPE_BASIC | UBLK_PARAM_TYPE_DISCARD | \ > + UBLK_PARAM_TYPE_DEVT | UBLK_PARAM_TYPE_ZONED) > > struct ublk_uring_cmd_pdu { > struct ublk_queue *ubq; > @@ -209,6 +210,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) > @@ -269,6 +273,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; > } > > @@ -439,6 +446,7 @@ static const struct block_device_operations ub_fops = { > .owner = THIS_MODULE, > .open = ublk_open, > .free_disk = ublk_free_disk, > + .report_zones = ublk_report_zones, > }; > > #define UBLK_MAX_PIN_PAGES 32 > @@ -553,7 +561,8 @@ static inline bool ublk_need_map_req(const struct request *req) > > static inline bool ublk_need_unmap_req(const struct request *req) > { > - return ublk_rq_has_data(req) && req_op(req) == REQ_OP_READ; > + return ublk_rq_has_data(req) && > + (req_op(req) == REQ_OP_READ || req_op(req) == REQ_OP_DRV_IN); > } > > static int ublk_map_io(const struct ublk_queue *ubq, const struct request *req, > @@ -637,6 +646,7 @@ static blk_status_t ublk_setup_iod(struct ublk_queue *ubq, struct request *req) > { > struct ublksrv_io_desc *iod = ublk_get_iod(ubq, req->tag); > struct ublk_io *io = &ubq->ios[req->tag]; > + struct ublk_rq_data *pdu = blk_mq_rq_to_pdu(req); > u32 ublk_op; > > switch (req_op(req)) { > @@ -655,6 +665,35 @@ 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; > + 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 = pdu->operation; > + switch (ublk_op) { > + case UBLK_IO_OP_REPORT_ZONES: > + iod->op_flags = ublk_op | ublk_req_build_flags(req); > + iod->nr_sectors = pdu->nr_sectors; > + iod->start_sector = pdu->sector; > + iod->addr = io->addr; As Damien commented, zone append needs to be moved into this patch, so zoned depends on user copy feature, and you can simply fail to add device if user copy isn't enabled. So the above 'iod->addr = io->addr' can be removed. > + return BLK_STS_OK; > + default: > + return BLK_STS_IOERR; > + } > + case REQ_OP_ZONE_APPEND: > + case REQ_OP_ZONE_RESET_ALL: > + case REQ_OP_DRV_OUT: > + /* We do not support zone append or reset_all yet */ > + fallthrough; > default: > return BLK_STS_IOERR; > } > @@ -708,7 +747,8 @@ static inline 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) > goto exit; > > /* for READ request, writing data in iod->addr to rq buffers */ > @@ -1835,6 +1875,15 @@ static int ublk_ctrl_start_dev(struct ublk_device *ub, struct io_uring_cmd *cmd) > if (ub->nr_privileged_daemon != ub->nr_queues_ready) > set_bit(GD_SUPPRESS_PART_SCAN, &disk->state); > > + 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); > ub->dev_info.state = UBLK_S_DEV_LIVE; > ret = add_disk(disk); > @@ -1997,6 +2046,9 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd) > if (ub->dev_info.flags & UBLK_F_USER_COPY) > ub->dev_info.flags &= ~UBLK_F_NEED_GET_DATA; > > + if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED)) > + ub->dev_info.flags &= ~UBLK_F_ZONED; > + > /* We are not ready to support zero copy */ > ub->dev_info.flags &= ~UBLK_F_SUPPORT_ZERO_COPY; > > diff --git a/drivers/block/ublk_drv.h b/drivers/block/ublk_drv.h > index f81e62256456..7242430fd6b9 100644 > --- a/drivers/block/ublk_drv.h > +++ b/drivers/block/ublk_drv.h > @@ -50,6 +50,21 @@ struct ublk_rq_data { > struct llist_node node; > > struct kref ref; > + enum ublk_op operation; > + __u64 sector; > + __u32 nr_sectors; > }; > > +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); > + > +#ifdef CONFIG_BLK_DEV_UBLK_ZONED > +int ublk_report_zones(struct gendisk *disk, sector_t sector, > + unsigned int nr_zones, report_zones_cb cb, > + void *data); > +#else > +#define ublk_report_zones NULL > +#endif > + > #endif > diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h > index 471b3b983045..436525afffe8 100644 > --- a/include/uapi/linux/ublk_cmd.h > +++ b/include/uapi/linux/ublk_cmd.h > @@ -176,6 +176,11 @@ > /* Copy between request and user buffer by pread()/pwrite() */ > #define UBLK_F_USER_COPY (1UL << 7) > > +/* > + * Enable zoned device support > + */ > +#define UBLK_F_ZONED (1ULL << 8) > + > /* device state */ > #define UBLK_S_DEV_DEAD 0 > #define UBLK_S_DEV_LIVE 1 > @@ -242,6 +247,7 @@ enum ublk_op { > UBLK_IO_OP_ZONE_APPEND = 13, > UBLK_IO_OP_ZONE_RESET = 15, > __UBLK_IO_OP_DRV_IN_START = 32, > + UBLK_IO_OP_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, > @@ -342,6 +348,12 @@ struct ublk_param_devt { > __u32 disk_minor; > }; > > +struct ublk_param_zoned { > + __u32 max_open_zones; > + __u32 max_active_zones; > + __u8 reserved[24]; > +}; Maybe you can simply borrow virtio_blk_zoned_characteristics definition here, and make several reserved words. But that is fine, given six u32 should be flexible enough compared with virtio_blk_zoned_characteristics. Thanks, Ming
Hi Andreas, kernel test robot noticed the following build errors: [auto build test ERROR on 3261ea42710e9665c9151006049411bd23b5411f] url: https://github.com/intel-lab-lkp/linux/commits/Andreas-Hindborg/ublk-change-ublk-IO-command-defines-to-enum/20230629-031015 base: 3261ea42710e9665c9151006049411bd23b5411f patch link: https://lore.kernel.org/r/20230628190649.11233-4-nmi%40metaspace.dk patch subject: [PATCH v4 3/4] ublk: enable zoned storage support config: hexagon-randconfig-r041-20230628 (https://download.01.org/0day-ci/archive/20230629/202306291026.DwVHcWAA-lkp@intel.com/config) compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a) reproduce: (https://download.01.org/0day-ci/archive/20230629/202306291026.DwVHcWAA-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202306291026.DwVHcWAA-lkp@intel.com/ All errors (new ones prefixed by >>): >> ld.lld: error: undefined symbol: __hexagon_udivdi3 >>> referenced by ublk_drv-zoned.c:16 (drivers/block/ublk_drv-zoned.c:16) >>> drivers/block/ublk_drv-zoned.o:(ublk_set_nr_zones) in archive vmlinux.a >>> referenced by ublk_drv-zoned.c:16 (drivers/block/ublk_drv-zoned.c:16) >>> drivers/block/ublk_drv-zoned.o:(ublk_set_nr_zones) in archive vmlinux.a >>> did you mean: __hexagon_udivsi3 >>> defined in: vmlinux.a(arch/hexagon/lib/udivsi3.o)
What's the point in having a separate file for about 100 lines of actual code? Especially as that needs a header, intefaces, etc?
Hi Andreas, kernel test robot noticed the following build errors: [auto build test ERROR on 3261ea42710e9665c9151006049411bd23b5411f] url: https://github.com/intel-lab-lkp/linux/commits/Andreas-Hindborg/ublk-change-ublk-IO-command-defines-to-enum/20230629-031015 base: 3261ea42710e9665c9151006049411bd23b5411f patch link: https://lore.kernel.org/r/20230628190649.11233-4-nmi%40metaspace.dk patch subject: [PATCH v4 3/4] ublk: enable zoned storage support config: i386-randconfig-i006-20230628 (https://download.01.org/0day-ci/archive/20230629/202306291515.z7ox8OTz-lkp@intel.com/config) compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a) reproduce: (https://download.01.org/0day-ci/archive/20230629/202306291515.z7ox8OTz-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202306291515.z7ox8OTz-lkp@intel.com/ All errors (new ones prefixed by >>): >> ld.lld: error: undefined symbol: __udivdi3 >>> referenced by ublk_drv-zoned.c:16 (drivers/block/ublk_drv-zoned.c:16) >>> drivers/block/ublk_drv-zoned.o:(ublk_set_nr_zones) in archive vmlinux.a
Christoph Hellwig <hch@infradead.org> writes: > What's the point in having a separate file for about 100 lines of > actual code? Especially as that needs a header, intefaces, etc? This was suggested by Niklas [1] to make the conditionally compiled code more readable. The alternative would be masking out the code with a big `#ifdef`. If you prefer the latter, I can change it back, no problem. But I must say I think it comes off cleaner when we move as much as possible of the zoned code to a separate translation unit. [1] https://lore.kernel.org/all/Y%2Fy+UFEHn1F1sg4i@x1-carbon/
On Thu, Jun 29, 2023 at 09:25:32AM +0200, Andreas Hindborg (Samsung) wrote: > If you prefer the latter, I can change it back, no problem. But I must > say I think it comes off cleaner when we move as much as possible of the > zoned code to a separate translation unit. I have to say I much prefer the ifdef. In the end Ming is the maintainer though, so I'll defer to him.
Damien Le Moal <dlemoal@kernel.org> writes: > On 6/29/23 04:06, Andreas Hindborg wrote: >> From: Andreas Hindborg <a.hindborg@samsung.com> >> >> 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 >> >> Note: This commit changes the ublk kernel module name from `ublk_drv.ko` to >> `ublk.ko` in order to link multiple translation units into the module. > > You probably could rename ublk_drv.c to ublk.c to avoid that and keep the module > name as it was, ublk_drv.ko. OK, will change. > >> >> Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com> >> --- >> MAINTAINERS | 1 + >> drivers/block/Kconfig | 4 + >> drivers/block/Makefile | 4 +- >> drivers/block/ublk_drv-zoned.c | 144 +++++++++++++++++++++++++++++++++ >> drivers/block/ublk_drv.c | 64 +++++++++++++-- >> drivers/block/ublk_drv.h | 15 ++++ >> include/uapi/linux/ublk_cmd.h | 14 ++++ >> 7 files changed, 239 insertions(+), 7 deletions(-) >> create mode 100644 drivers/block/ublk_drv-zoned.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index ace71c90751c..db8a8deb5926 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -21555,6 +21555,7 @@ S: Maintained >> F: Documentation/block/ublk.rst >> F: drivers/block/ublk_drv.c >> F: drivers/block/ublk_drv.h >> +F: drivers/block/ublk_drv-zoned.c >> F: include/uapi/linux/ublk_cmd.h >> >> UCLINUX (M68KNOMMU AND COLDFIRE) >> diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig >> index 5b9d4aaebb81..c58dfd035557 100644 >> --- a/drivers/block/Kconfig >> +++ b/drivers/block/Kconfig >> @@ -402,6 +402,10 @@ config BLKDEV_UBLK_LEGACY_OPCODES >> suggested to enable N if your application(ublk server) switches to >> ioctl command encoding. >> >> +config BLK_DEV_UBLK_ZONED >> + def_bool y > > This can be "bool" only. I did it like this because I wanted BLK_DEV_UBLK_ZONED to be set automatically to "y" when BLK_DEV_UBLK and BLK_DEV_ZONED are enabled. BLK_DEV_UBLK_ZONED has no menu entry. If we change it to "bool" it will be default "n" and we would have to make it appear in menuconfig to be manually enabled. Isn't it more sane if it is just unconditionally enabled when BLK_DEV_ZONED and BLK_DEV_UBLK is enabled? > >> + depends on BLK_DEV_UBLK && BLK_DEV_ZONED >> + >> source "drivers/block/rnbd/Kconfig" >> >> endif # BLK_DEV >> diff --git a/drivers/block/Makefile b/drivers/block/Makefile >> index 101612cba303..bc1649e20ec2 100644 >> --- a/drivers/block/Makefile >> +++ b/drivers/block/Makefile >> @@ -37,6 +37,8 @@ obj-$(CONFIG_BLK_DEV_RNBD) += rnbd/ >> >> obj-$(CONFIG_BLK_DEV_NULL_BLK) += null_blk/ >> >> -obj-$(CONFIG_BLK_DEV_UBLK) += ublk_drv.o >> +obj-$(CONFIG_BLK_DEV_UBLK) += ublk.o >> +ublk-$(CONFIG_BLK_DEV_UBLK) += ublk_drv.o >> +ublk-$(CONFIG_BLK_DEV_UBLK_ZONED) += ublk_drv-zoned.o >> >> swim_mod-y := swim.o swim_asm.o >> diff --git a/drivers/block/ublk_drv-zoned.c b/drivers/block/ublk_drv-zoned.c >> new file mode 100644 >> index 000000000000..ea86bf4b3681 >> --- /dev/null >> +++ b/drivers/block/ublk_drv-zoned.c >> @@ -0,0 +1,144 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright 2023 Andreas Hindborg <a.hindborg@samsung.com> >> + */ >> +#include <linux/blkzoned.h> >> +#include <linux/ublk_cmd.h> >> +#include <linux/vmalloc.h> >> + >> +#include "ublk_drv.h" >> + >> +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) > > If UBLK_F_ZONED is set but chunk_sectors is not, that is a bug on the user > driver side, no ? So an error message about that would be nice instead of > silently ignoring the zoned flag. Yes, I'll add a check to `ublk_validate_params()`. > >> + ub->ub_disk->nr_zones = p->dev_sectors / p->chunk_sectors; >> +} >> + >> +void ublk_dev_param_zoned_apply(struct ublk_device *ub) >> +{ >> + const struct ublk_param_zoned *p = &ub->params.zoned; >> + >> + if (ub->dev_info.flags & UBLK_F_ZONED) { >> + disk_set_max_active_zones(ub->ub_disk, p->max_active_zones); >> + disk_set_max_open_zones(ub->ub_disk, p->max_open_zones); > > You do not need to check if the max_active_zones and max_open_zones values are > sensible ? E.g. what if they are larger than the number of zones ? I don't think it will be a problem. I assume you can set them to 0 to indicate infinite. If user space sets them to more than the actual number of available zones (that user space set up also), user space will have to handle that when it gets a zone request for a zone outside the logical drive LBA space. I don't think the kernel has to care. Will this break anything kernel side? > >> + } >> +} >> + >> +int ublk_revalidate_disk_zones(struct gendisk *disk) >> +{ >> + return blk_revalidate_disk_zones(disk, NULL); >> +} > > I do not think this helper is needed at all (see below comment on the call site). This is required because the prototype for `blk_revalidate_disk_zones()` is not defined when BLK_DEV_ZONED is not defined. Without this helper for which the prototype is always defined, we will have a compile error at the call site. > >> + >> +/* Based on virtblk_alloc_report_buffer */ >> +static void *ublk_alloc_report_buffer(struct ublk_device *ublk, >> + unsigned int nr_zones, >> + unsigned int zone_sectors, size_t *buflen) >> +{ >> + struct request_queue *q = ublk->ub_disk->queue; >> + size_t bufsize; >> + void *buf; >> + >> + nr_zones = min_t(unsigned int, nr_zones, >> + get_capacity(ublk->ub_disk) >> ilog2(zone_sectors)); >> + >> + bufsize = nr_zones * sizeof(struct blk_zone); >> + bufsize = >> + min_t(size_t, bufsize, queue_max_hw_sectors(q) << SECTOR_SHIFT); >> + bufsize = min_t(size_t, bufsize, queue_max_segments(q) << PAGE_SHIFT); >> + >> + while (bufsize >= sizeof(struct blk_zone)) { >> + buf = __vmalloc(bufsize, GFP_KERNEL | __GFP_NORETRY); >> + if (buf) { >> + *buflen = bufsize; >> + return buf; >> + } >> + bufsize >>= 1; >> + } >> + >> + bufsize = 0; > > This is not needed. This should rather be "*buflen = 0;". Yes, this an ugly oops. > >> + return NULL; >> +} >> + >> +int ublk_report_zones(struct gendisk *disk, sector_t sector, >> + unsigned int nr_zones, report_zones_cb cb, void *data) >> +{ >> + struct ublk_device *ub = disk->private_data; >> + unsigned int zone_size_sectors = disk->queue->limits.chunk_sectors; >> + unsigned int first_zone = sector >> ilog2(zone_size_sectors); >> + unsigned int done_zones = 0; >> + unsigned int max_zones_per_request; >> + struct blk_zone *buffer; >> + size_t buffer_length; >> + >> + if (!(ub->dev_info.flags & UBLK_F_ZONED)) > > This is repeated a lot. So a small inline helper ublk_dev_is_zoned() would be nice. Yes, thanks. > >> + return -EOPNOTSUPP; >> + >> + nr_zones = min_t(unsigned int, ub->ub_disk->nr_zones - first_zone, >> + nr_zones); >> + >> + buffer = ublk_alloc_report_buffer(ub, nr_zones, zone_size_sectors, >> + &buffer_length); >> + if (!buffer) >> + return -ENOMEM; >> + >> + max_zones_per_request = buffer_length / sizeof(struct blk_zone); >> + >> + while (done_zones < nr_zones) { >> + unsigned int remaining_zones = nr_zones - done_zones; >> + unsigned int zones_in_request = min_t( >> + unsigned int, remaining_zones, max_zones_per_request); >> + int err = 0; >> + struct request *req; >> + struct ublk_rq_data *pdu; >> + blk_status_t status; >> + >> + memset(buffer, 0, buffer_length); >> + >> + req = blk_mq_alloc_request(disk->queue, REQ_OP_DRV_IN, 0); >> + if (IS_ERR(req)) >> + return PTR_ERR(req); > > You are leaking buffer.
On 6/29/23 16:50, Andreas Hindborg (Samsung) wrote: >>> UCLINUX (M68KNOMMU AND COLDFIRE) >>> diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig >>> index 5b9d4aaebb81..c58dfd035557 100644 >>> --- a/drivers/block/Kconfig >>> +++ b/drivers/block/Kconfig >>> @@ -402,6 +402,10 @@ config BLKDEV_UBLK_LEGACY_OPCODES >>> suggested to enable N if your application(ublk server) switches to >>> ioctl command encoding. >>> >>> +config BLK_DEV_UBLK_ZONED >>> + def_bool y >> >> This can be "bool" only. > > I did it like this because I wanted BLK_DEV_UBLK_ZONED to be set > automatically to "y" when BLK_DEV_UBLK and BLK_DEV_ZONED are enabled. > BLK_DEV_UBLK_ZONED has no menu entry. If we change it to "bool" it will > be default "n" and we would have to make it appear in menuconfig to be > manually enabled. > > Isn't it more sane if it is just unconditionally enabled when > BLK_DEV_ZONED and BLK_DEV_UBLK is enabled? Maybe something like this then: diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig index 5b9d4aaebb81..105e1121bf5f 100644 --- a/drivers/block/Kconfig +++ b/drivers/block/Kconfig @@ -370,9 +370,13 @@ config BLK_DEV_RBD If unsure, say N. +config BLK_DEV_UBLK_ZONED + bool + config BLK_DEV_UBLK tristate "Userspace block driver (Experimental)" select IO_URING + select BLK_DEV_UBLK_ZONED if BLK_DEV_ZONED help io_uring based userspace block driver. Together with ublk server, ublk has been working well, but interface with userspace or command data >>> + ub->ub_disk->nr_zones = p->dev_sectors / p->chunk_sectors; >>> +} >>> + >>> +void ublk_dev_param_zoned_apply(struct ublk_device *ub) >>> +{ >>> + const struct ublk_param_zoned *p = &ub->params.zoned; >>> + >>> + if (ub->dev_info.flags & UBLK_F_ZONED) { >>> + disk_set_max_active_zones(ub->ub_disk, p->max_active_zones); >>> + disk_set_max_open_zones(ub->ub_disk, p->max_open_zones); >> >> You do not need to check if the max_active_zones and max_open_zones values are >> sensible ? E.g. what if they are larger than the number of zones ? > > I don't think it will be a problem. I assume you can set them to 0 to > indicate infinite. If user space sets them to more than the actual > number of available zones (that user space set up also), user space will > have to handle that when it gets a zone request for a zone outside the > logical drive LBA space. I don't think the kernel has to care. Will this > break anything kernel side? zonefs and btrfs look at these limits. So I would rather prefer seeing sensible values. 0 == no limit is fine. But seeing maz or moz > nr_zones for the device is just too strange and I would prefer an error in that case to let the user know it is not doing something right. Getting moz > maz is also nonsense that needs to be checked. There is one case we could silently change: if maz == nr_zones or moz == nr_zones, then that essentially means "no limit", so we could use 0 to let the in-kernel users that they do not need to care about the limits. > >> >>> + } >>> +} >>> + >>> +int ublk_revalidate_disk_zones(struct gendisk *disk) >>> +{ >>> + return blk_revalidate_disk_zones(disk, NULL); >>> +} >> >> I do not think this helper is needed at all (see below comment on the call site). > > This is required because the prototype for `blk_revalidate_disk_zones()` > is not defined when BLK_DEV_ZONED is not defined. Without this helper > for which the prototype is always defined, we will have a compile error > at the call site. You have this hunk in ublk_ctrl_start_dev: disk_set_zoned(disk, BLK_ZONED_HM); blk_queue_required_elevator_features(disk->queue, ELEVATOR_F_ZBD_SEQ_WRITE); ret = ublk_revalidate_disk_zones(disk); if (ret) goto out_put_disk; And that is called under "if (IS_ENABLED(CONFIG_BLK_DEV_ZONED)". So I do not see how you can get compile errors using directly blk_revalidate_disk_zones(). The entire hunk above should be a helper function I think. >>> + >>> + if (IS_ENABLED(CONFIG_BLK_DEV_ZONED)) >>> + ublk_set_nr_zones(ub); >> >> So if the user is attempting to setup a zoned drive but the kernel does not have >> CONFIG_BLK_DEV_ZONED=y, the user setup will be silently ignored. Not exactly >> nice I think, unless I am missing something. > > We have this in `ublk_ctrl_add_dev()`: > > if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED)) > ub->dev_info.flags &= ~UBLK_F_ZONED; > > User space is supposed to check the flags after the call to know the > kernel capabilities. And you trust user space to be always correct ? I never do :) So definitely, check and error if not supported. No silently clearing the device zoned flag. >> Also, repeating that "if (IS_ENABLED(CONFIG_BLK_DEV_ZONED))" for all zone >> related functions is very verbose. Stub the functions in ublk_drv.h. That will >> make the main C code lighter. > > Not sure what you mean "stub the functions". Like so: > > @@ -216,8 +216,7 @@ static void ublk_dev_param_basic_apply(struct ublk_device *ub) > > set_capacity(ub->ub_disk, p->dev_sectors); > > - if (IS_ENABLED(CONFIG_BLK_DEV_ZONED)) > - ublk_set_nr_zones(ub); > + ublk_config_nr_zones(ub); > } > > And then in the header: > > void ublk_config_nr_zones(struct ublk_device *ub) > { > if (IS_ENABLED(CONFIG_BLK_DEV_ZONED)) > ublk_set_nr_zones(ub); > } > > Or something else? #ifndef CONFIG_BLK_DEV_ZONED static inline void ublk_set_nr_zones() {} #endif That avoid repeating that if (IS_ENABLED(CONFIG_BLK_DEV_ZONED)) all over the place. The stubbed functions can be called only for "if (ub->dev_info.flags & UBLK_F_ZONED", which is OK since this flag is never supposed to be accepted and set for the CONFIG_BLK_DEV_ZONED=n case. >>> + disk_set_zoned(disk, BLK_ZONED_HM); >>> + blk_queue_required_elevator_features(disk->queue, ELEVATOR_F_ZBD_SEQ_WRITE); >>> + ret = ublk_revalidate_disk_zones(disk); >>> + if (ret) >>> + goto out_put_disk; >> >> This should be all a helper ublk_set_zoned() or something. > > Ok
> From: Andreas Hindborg <a.hindborg@samsung.com > <mailto:a.hindborg@samsung.com>> > > > 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 > > > Note: This commit changes the ublk kernel module name from > `ublk_drv.ko` to > `ublk.ko` in order to link multiple translation units into the module. > > > Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com > <mailto:a.hindborg@samsung.com>> > --- > MAINTAINERS | 1 + > drivers/block/Kconfig | 4 + > drivers/block/Makefile | 4 +- > drivers/block/ublk_drv-zoned.c | 144 +++++++++++++++++++++++++++++++++ > drivers/block/ublk_drv.c | 64 +++++++++++++-- > drivers/block/ublk_drv.h | 15 ++++ > include/uapi/linux/ublk_cmd.h | 14 ++++ > 7 files changed, 239 insertions(+), 7 deletions(-) > create mode 100644 drivers/block/ublk_drv-zoned.c > > > diff --git a/MAINTAINERS b/MAINTAINERS > index ace71c90751c..db8a8deb5926 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -21555,6 +21555,7 @@ S: Maintained > F: Documentation/block/ublk.rst > F: drivers/block/ublk_drv.c > F: drivers/block/ublk_drv.h > +F: drivers/block/ublk_drv-zoned.c nitpick: checkpatch.pl gives a warning on the ordering of the files. WARNING: Misordered MAINTAINERS entry - list file patterns in alphabetic order > F: include/uapi/linux/ublk_cmd.h > > > UCLINUX (M68KNOMMU AND COLDFIRE) > diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig > index 5b9d4aaebb81..c58dfd035557 100644 > --- a/drivers/block/Kconfig > +++ b/drivers/block/Kconfig > @@ -402,6 +402,10 @@ config BLKDEV_UBLK_LEGACY_OPCODES > suggested to enable N if your application(ublk server) switches to > ioctl command encoding. > > > +config BLK_DEV_UBLK_ZONED > + def_bool y > + depends on BLK_DEV_UBLK && BLK_DEV_ZONED > + > source "drivers/block/rnbd/Kconfig" > > > endif # BLK_DEV > diff --git a/drivers/block/Makefile b/drivers/block/Makefile > index 101612cba303..bc1649e20ec2 100644 > --- a/drivers/block/Makefile > +++ b/drivers/block/Makefile > @@ -37,6 +37,8 @@ obj-$(CONFIG_BLK_DEV_RNBD) += rnbd/ > > > obj-$(CONFIG_BLK_DEV_NULL_BLK) += null_blk/ > > > -obj-$(CONFIG_BLK_DEV_UBLK) += ublk_drv.o > +obj-$(CONFIG_BLK_DEV_UBLK) += ublk.o > +ublk-$(CONFIG_BLK_DEV_UBLK) += ublk_drv.o > +ublk-$(CONFIG_BLK_DEV_UBLK_ZONED) += ublk_drv-zoned.o > > > swim_mod-y := swim.o swim_asm.o > diff --git a/drivers/block/ublk_drv-zoned.c > b/drivers/block/ublk_drv-zoned.c > new file mode 100644 > index 000000000000..ea86bf4b3681 > --- /dev/null > +++ b/drivers/block/ublk_drv-zoned.c > @@ -0,0 +1,144 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright 2023 Andreas Hindborg <a.hindborg@samsung.com > <mailto:a.hindborg@samsung.com>> > + */ > +#include <linux/blkzoned.h> > +#include <linux/ublk_cmd.h> > +#include <linux/vmalloc.h> > + > +#include "ublk_drv.h" > + > +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; > +} > + > +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); > + } > +} > + > +int ublk_revalidate_disk_zones(struct gendisk *disk) > +{ > + return blk_revalidate_disk_zones(disk, NULL); > +} > + > +/* Based on virtblk_alloc_report_buffer */ > +static void *ublk_alloc_report_buffer(struct ublk_device *ublk, > + unsigned int nr_zones, > + unsigned int zone_sectors, size_t *buflen) > +{ > + struct request_queue *q = ublk->ub_disk->queue; > + size_t bufsize; > + void *buf; > + > + nr_zones = min_t(unsigned int, nr_zones, > + get_capacity(ublk->ub_disk) >> ilog2(zone_sectors)); > + > + bufsize = nr_zones * sizeof(struct blk_zone); > + bufsize = > + min_t(size_t, bufsize, queue_max_hw_sectors(q) << SECTOR_SHIFT); > + bufsize = min_t(size_t, bufsize, queue_max_segments(q) << > PAGE_SHIFT); > + > + while (bufsize >= sizeof(struct blk_zone)) { > + buf = __vmalloc(bufsize, GFP_KERNEL | __GFP_NORETRY); > + if (buf) { > + *buflen = bufsize; > + return buf; > + } > + bufsize >>= 1; > + } > + > + bufsize = 0; > + return NULL; > +} > + > +int ublk_report_zones(struct gendisk *disk, sector_t sector, > + unsigned int nr_zones, report_zones_cb cb, void *data) > +{ > + struct ublk_device *ub = disk->private_data; > + unsigned int zone_size_sectors = disk->queue->limits.chunk_sectors; > + unsigned int first_zone = sector >> ilog2(zone_size_sectors); > + unsigned int done_zones = 0; > + unsigned int max_zones_per_request; > + struct blk_zone *buffer; > + size_t buffer_length; > + > + if (!(ub->dev_info.flags & UBLK_F_ZONED)) > + return -EOPNOTSUPP; > + > + nr_zones = min_t(unsigned int, ub->ub_disk->nr_zones - first_zone, > + nr_zones); > + > + buffer = ublk_alloc_report_buffer(ub, nr_zones, zone_size_sectors, > + &buffer_length); > + if (!buffer) > + return -ENOMEM; > + > + max_zones_per_request = buffer_length / sizeof(struct blk_zone); > + > + while (done_zones < nr_zones) { > + unsigned int remaining_zones = nr_zones - done_zones; > + unsigned int zones_in_request = min_t( > + unsigned int, remaining_zones, max_zones_per_request); > + int err = 0; > + struct request *req; > + struct ublk_rq_data *pdu; > + blk_status_t status; > + > + memset(buffer, 0, buffer_length); > + > + req = blk_mq_alloc_request(disk->queue, REQ_OP_DRV_IN, 0); > + if (IS_ERR(req)) > + return PTR_ERR(req); > + > + pdu = blk_mq_rq_to_pdu(req); > + pdu->operation = UBLK_IO_OP_REPORT_ZONES; > + pdu->sector = sector; > + pdu->nr_sectors = remaining_zones * zone_size_sectors; > + > + err = blk_rq_map_kern(disk->queue, req, buffer, buffer_length, > + GFP_KERNEL); > + if (err) { > + blk_mq_free_request(req); > + kvfree(buffer); > + return err; > + } > + > + status = blk_execute_rq(req, 0); > + err = blk_status_to_errno(status); > + blk_mq_free_request(req); > + if (err) { > + kvfree(buffer); > + return err; > + } > + > + for (unsigned int i = 0; i < zones_in_request; i++) { > + struct blk_zone *zone = buffer + i; > + > + err = cb(zone, i, data); > + if (err) > + return err; > + > + done_zones++; > + sector += zone_size_sectors; > + > + /* A zero length zone means don't ask for more zones */ > + if (!zone->len) { > + kvfree(buffer); > + return done_zones; > + } > + } > + } > + > + kvfree(buffer); > + return done_zones; > +} > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c > index e519dc0d9fe7..88fa39853c61 100644 > --- a/drivers/block/ublk_drv.c > +++ b/drivers/block/ublk_drv.c > @@ -57,12 +57,13 @@ > | UBLK_F_USER_RECOVERY_REISSUE \ > | UBLK_F_UNPRIVILEGED_DEV \ > | UBLK_F_CMD_IOCTL_ENCODE \ > - | UBLK_F_USER_COPY) > + | UBLK_F_USER_COPY \ > + | UBLK_F_ZONED) > > > /* All UBLK_PARAM_TYPE_* should be included here */ > -#define UBLK_PARAM_TYPE_ALL (UBLK_PARAM_TYPE_BASIC | \ > - UBLK_PARAM_TYPE_DISCARD | UBLK_PARAM_TYPE_DEVT) > - > +#define UBLK_PARAM_TYPE_ALL \ > + (UBLK_PARAM_TYPE_BASIC | UBLK_PARAM_TYPE_DISCARD | \ > + UBLK_PARAM_TYPE_DEVT | UBLK_PARAM_TYPE_ZONED) > > > struct ublk_uring_cmd_pdu { > struct ublk_queue *ubq; > @@ -209,6 +210,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) > @@ -269,6 +273,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; > } > > > @@ -439,6 +446,7 @@ static const struct block_device_operations ub_fops > = { > .owner = THIS_MODULE, > .open = ublk_open, > .free_disk = ublk_free_disk, > + .report_zones = ublk_report_zones, > }; > > > #define UBLK_MAX_PIN_PAGES 32 > @@ -553,7 +561,8 @@ static inline bool ublk_need_map_req(const struct > request *req) > > > static inline bool ublk_need_unmap_req(const struct request *req) > { > - return ublk_rq_has_data(req) && req_op(req) == REQ_OP_READ; > + return ublk_rq_has_data(req) && > + (req_op(req) == REQ_OP_READ || req_op(req) == REQ_OP_DRV_IN); > } > > > static int ublk_map_io(const struct ublk_queue *ubq, const struct > request *req, > @@ -637,6 +646,7 @@ static blk_status_t ublk_setup_iod(struct > ublk_queue *ubq, struct request *req) > { > struct ublksrv_io_desc *iod = ublk_get_iod(ubq, req->tag); > struct ublk_io *io = &ubq->ios[req->tag]; > + struct ublk_rq_data *pdu = blk_mq_rq_to_pdu(req); > u32 ublk_op; > > > switch (req_op(req)) { > @@ -655,6 +665,35 @@ 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; > + 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 = pdu->operation; > + switch (ublk_op) { > + case UBLK_IO_OP_REPORT_ZONES: > + iod->op_flags = ublk_op | ublk_req_build_flags(req); > + iod->nr_sectors = pdu->nr_sectors; > + iod->start_sector = pdu->sector; > + iod->addr = io->addr; > + return BLK_STS_OK; > + default: > + return BLK_STS_IOERR; > + } > + case REQ_OP_ZONE_APPEND: > + case REQ_OP_ZONE_RESET_ALL: > + case REQ_OP_DRV_OUT: > + /* We do not support zone append or reset_all yet */ > + fallthrough; > default: > return BLK_STS_IOERR; > } > @@ -708,7 +747,8 @@ static inline 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) > goto exit; > > > /* for READ request, writing data in iod->addr to rq buffers */ > @@ -1835,6 +1875,15 @@ static int ublk_ctrl_start_dev(struct > ublk_device *ub, struct io_uring_cmd *cmd) > if (ub->nr_privileged_daemon != ub->nr_queues_ready) > set_bit(GD_SUPPRESS_PART_SCAN, &disk->state); > > > + 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); > ub->dev_info.state = UBLK_S_DEV_LIVE; > ret = add_disk(disk); > @@ -1997,6 +2046,9 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd > *cmd) > if (ub->dev_info.flags & UBLK_F_USER_COPY) > ub->dev_info.flags &= ~UBLK_F_NEED_GET_DATA; > > > + if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED)) > + ub->dev_info.flags &= ~UBLK_F_ZONED; > + > /* We are not ready to support zero copy */ > ub->dev_info.flags &= ~UBLK_F_SUPPORT_ZERO_COPY; > > > diff --git a/drivers/block/ublk_drv.h b/drivers/block/ublk_drv.h > index f81e62256456..7242430fd6b9 100644 > --- a/drivers/block/ublk_drv.h > +++ b/drivers/block/ublk_drv.h > @@ -50,6 +50,21 @@ struct ublk_rq_data { > struct llist_node node; > > > struct kref ref; > + enum ublk_op operation; > + __u64 sector; > + __u32 nr_sectors; > }; > > > +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); > + > +#ifdef CONFIG_BLK_DEV_UBLK_ZONED > +int ublk_report_zones(struct gendisk *disk, sector_t sector, > + unsigned int nr_zones, report_zones_cb cb, > + void *data); > +#else > +#define ublk_report_zones NULL > +#endif > + > #endif > diff --git a/include/uapi/linux/ublk_cmd.h > b/include/uapi/linux/ublk_cmd.h > index 471b3b983045..436525afffe8 100644 > --- a/include/uapi/linux/ublk_cmd.h > +++ b/include/uapi/linux/ublk_cmd.h > @@ -176,6 +176,11 @@ > /* Copy between request and user buffer by pread()/pwrite() */ > #define UBLK_F_USER_COPY (1UL << 7) > > > +/* > + * Enable zoned device support > + */ > +#define UBLK_F_ZONED (1ULL << 8) > + > /* device state */ > #define UBLK_S_DEV_DEAD 0 > #define UBLK_S_DEV_LIVE 1 > @@ -242,6 +247,7 @@ enum ublk_op { > UBLK_IO_OP_ZONE_APPEND = 13, > UBLK_IO_OP_ZONE_RESET = 15, > __UBLK_IO_OP_DRV_IN_START = 32, > + UBLK_IO_OP_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, > @@ -342,6 +348,12 @@ struct ublk_param_devt { > __u32 disk_minor; > }; > > > +struct ublk_param_zoned { > + __u32 max_open_zones; > + __u32 max_active_zones; > + __u8 reserved[24]; > +}; > + > struct ublk_params { > /* > * Total length of parameters, userspace has to set 'len' for both > @@ -353,11 +365,13 @@ struct ublk_params { > #define UBLK_PARAM_TYPE_BASIC (1 << 0) > #define UBLK_PARAM_TYPE_DISCARD (1 << 1) > #define UBLK_PARAM_TYPE_DEVT (1 << 2) > +#define UBLK_PARAM_TYPE_ZONED (1 << 3) > __u32 types; /* types of parameter included */ > > > struct ublk_param_basic basic; > struct ublk_param_discard discard; > struct ublk_param_devt devt; > + struct ublk_param_zoned zoned; > }; > > > #endif I applied the patches and verified it compiles with and without the CONFIG_BLK_DEV_ZONED flag. Working on creating a ublk device also. Regards, Aravind
Damien Le Moal <dlemoal@kernel.org> writes: > On 6/29/23 16:50, Andreas Hindborg (Samsung) wrote: >>>> UCLINUX (M68KNOMMU AND COLDFIRE) >>>> diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig >>>> index 5b9d4aaebb81..c58dfd035557 100644 >>>> --- a/drivers/block/Kconfig >>>> +++ b/drivers/block/Kconfig >>>> @@ -402,6 +402,10 @@ config BLKDEV_UBLK_LEGACY_OPCODES >>>> suggested to enable N if your application(ublk server) switches to >>>> ioctl command encoding. >>>> >>>> +config BLK_DEV_UBLK_ZONED >>>> + def_bool y >>> >>> This can be "bool" only. >> >> I did it like this because I wanted BLK_DEV_UBLK_ZONED to be set >> automatically to "y" when BLK_DEV_UBLK and BLK_DEV_ZONED are enabled. >> BLK_DEV_UBLK_ZONED has no menu entry. If we change it to "bool" it will >> be default "n" and we would have to make it appear in menuconfig to be >> manually enabled. >> >> Isn't it more sane if it is just unconditionally enabled when >> BLK_DEV_ZONED and BLK_DEV_UBLK is enabled? > > Maybe something like this then: > > diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig > index 5b9d4aaebb81..105e1121bf5f 100644 > --- a/drivers/block/Kconfig > +++ b/drivers/block/Kconfig > @@ -370,9 +370,13 @@ config BLK_DEV_RBD > > If unsure, say N. > > +config BLK_DEV_UBLK_ZONED > + bool > + > config BLK_DEV_UBLK > tristate "Userspace block driver (Experimental)" > select IO_URING > + select BLK_DEV_UBLK_ZONED if BLK_DEV_ZONED > help > io_uring based userspace block driver. Together with ublk server, ublk > has been working well, but interface with userspace or command data OK
aravind.ramesh@opensource.wdc.com writes: >> From: Andreas Hindborg <a.hindborg@samsung.com >> <mailto:a.hindborg@samsung.com>> >> 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 >> Note: This commit changes the ublk kernel module name from `ublk_drv.ko` to >> `ublk.ko` in order to link multiple translation units into the module. >> Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com >> <mailto:a.hindborg@samsung.com>> >> --- >> MAINTAINERS | 1 + >> drivers/block/Kconfig | 4 + >> drivers/block/Makefile | 4 +- >> drivers/block/ublk_drv-zoned.c | 144 +++++++++++++++++++++++++++++++++ >> drivers/block/ublk_drv.c | 64 +++++++++++++-- >> drivers/block/ublk_drv.h | 15 ++++ >> include/uapi/linux/ublk_cmd.h | 14 ++++ >> 7 files changed, 239 insertions(+), 7 deletions(-) >> create mode 100644 drivers/block/ublk_drv-zoned.c >> diff --git a/MAINTAINERS b/MAINTAINERS >> index ace71c90751c..db8a8deb5926 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -21555,6 +21555,7 @@ S: Maintained >> F: Documentation/block/ublk.rst >> F: drivers/block/ublk_drv.c >> F: drivers/block/ublk_drv.h >> +F: drivers/block/ublk_drv-zoned.c > > nitpick: checkpatch.pl gives a warning on the ordering of the files. > WARNING: Misordered MAINTAINERS entry - list file patterns in alphabetic order > >> F: include/uapi/linux/ublk_cmd.h >> UCLINUX (M68KNOMMU AND COLDFIRE) >> diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig >> index 5b9d4aaebb81..c58dfd035557 100644 >> --- a/drivers/block/Kconfig >> +++ b/drivers/block/Kconfig >> @@ -402,6 +402,10 @@ config BLKDEV_UBLK_LEGACY_OPCODES >> suggested to enable N if your application(ublk server) switches to >> ioctl command encoding. >> +config BLK_DEV_UBLK_ZONED >> + def_bool y >> + depends on BLK_DEV_UBLK && BLK_DEV_ZONED >> + >> source "drivers/block/rnbd/Kconfig" >> endif # BLK_DEV >> diff --git a/drivers/block/Makefile b/drivers/block/Makefile >> index 101612cba303..bc1649e20ec2 100644 >> --- a/drivers/block/Makefile >> +++ b/drivers/block/Makefile >> @@ -37,6 +37,8 @@ obj-$(CONFIG_BLK_DEV_RNBD) += rnbd/ >> obj-$(CONFIG_BLK_DEV_NULL_BLK) += null_blk/ >> -obj-$(CONFIG_BLK_DEV_UBLK) += ublk_drv.o >> +obj-$(CONFIG_BLK_DEV_UBLK) += ublk.o >> +ublk-$(CONFIG_BLK_DEV_UBLK) += ublk_drv.o >> +ublk-$(CONFIG_BLK_DEV_UBLK_ZONED) += ublk_drv-zoned.o >> swim_mod-y := swim.o swim_asm.o >> diff --git a/drivers/block/ublk_drv-zoned.c b/drivers/block/ublk_drv-zoned.c >> new file mode 100644 >> index 000000000000..ea86bf4b3681 >> --- /dev/null >> +++ b/drivers/block/ublk_drv-zoned.c >> @@ -0,0 +1,144 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright 2023 Andreas Hindborg <a.hindborg@samsung.com >> <mailto:a.hindborg@samsung.com>> >> + */ >> +#include <linux/blkzoned.h> >> +#include <linux/ublk_cmd.h> >> +#include <linux/vmalloc.h> >> + >> +#include "ublk_drv.h" >> + >> +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; >> +} >> + >> +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); >> + } >> +} >> + >> +int ublk_revalidate_disk_zones(struct gendisk *disk) >> +{ >> + return blk_revalidate_disk_zones(disk, NULL); >> +} >> + >> +/* Based on virtblk_alloc_report_buffer */ >> +static void *ublk_alloc_report_buffer(struct ublk_device *ublk, >> + unsigned int nr_zones, >> + unsigned int zone_sectors, size_t *buflen) >> +{ >> + struct request_queue *q = ublk->ub_disk->queue; >> + size_t bufsize; >> + void *buf; >> + >> + nr_zones = min_t(unsigned int, nr_zones, >> + get_capacity(ublk->ub_disk) >> ilog2(zone_sectors)); >> + >> + bufsize = nr_zones * sizeof(struct blk_zone); >> + bufsize = >> + min_t(size_t, bufsize, queue_max_hw_sectors(q) << SECTOR_SHIFT); >> + bufsize = min_t(size_t, bufsize, queue_max_segments(q) << PAGE_SHIFT); >> + >> + while (bufsize >= sizeof(struct blk_zone)) { >> + buf = __vmalloc(bufsize, GFP_KERNEL | __GFP_NORETRY); >> + if (buf) { >> + *buflen = bufsize; >> + return buf; >> + } >> + bufsize >>= 1; >> + } >> + >> + bufsize = 0; >> + return NULL; >> +} >> + >> +int ublk_report_zones(struct gendisk *disk, sector_t sector, >> + unsigned int nr_zones, report_zones_cb cb, void *data) >> +{ >> + struct ublk_device *ub = disk->private_data; >> + unsigned int zone_size_sectors = disk->queue->limits.chunk_sectors; >> + unsigned int first_zone = sector >> ilog2(zone_size_sectors); >> + unsigned int done_zones = 0; >> + unsigned int max_zones_per_request; >> + struct blk_zone *buffer; >> + size_t buffer_length; >> + >> + if (!(ub->dev_info.flags & UBLK_F_ZONED)) >> + return -EOPNOTSUPP; >> + >> + nr_zones = min_t(unsigned int, ub->ub_disk->nr_zones - first_zone, >> + nr_zones); >> + >> + buffer = ublk_alloc_report_buffer(ub, nr_zones, zone_size_sectors, >> + &buffer_length); >> + if (!buffer) >> + return -ENOMEM; >> + >> + max_zones_per_request = buffer_length / sizeof(struct blk_zone); >> + >> + while (done_zones < nr_zones) { >> + unsigned int remaining_zones = nr_zones - done_zones; >> + unsigned int zones_in_request = min_t( >> + unsigned int, remaining_zones, max_zones_per_request); >> + int err = 0; >> + struct request *req; >> + struct ublk_rq_data *pdu; >> + blk_status_t status; >> + >> + memset(buffer, 0, buffer_length); >> + >> + req = blk_mq_alloc_request(disk->queue, REQ_OP_DRV_IN, 0); >> + if (IS_ERR(req)) >> + return PTR_ERR(req); >> + >> + pdu = blk_mq_rq_to_pdu(req); >> + pdu->operation = UBLK_IO_OP_REPORT_ZONES; >> + pdu->sector = sector; >> + pdu->nr_sectors = remaining_zones * zone_size_sectors; >> + >> + err = blk_rq_map_kern(disk->queue, req, buffer, buffer_length, >> + GFP_KERNEL); >> + if (err) { >> + blk_mq_free_request(req); >> + kvfree(buffer); >> + return err; >> + } >> + >> + status = blk_execute_rq(req, 0); >> + err = blk_status_to_errno(status); >> + blk_mq_free_request(req); >> + if (err) { >> + kvfree(buffer); >> + return err; >> + } >> + >> + for (unsigned int i = 0; i < zones_in_request; i++) { >> + struct blk_zone *zone = buffer + i; >> + >> + err = cb(zone, i, data); >> + if (err) >> + return err; >> + >> + done_zones++; >> + sector += zone_size_sectors; >> + >> + /* A zero length zone means don't ask for more zones */ >> + if (!zone->len) { >> + kvfree(buffer); >> + return done_zones; >> + } >> + } >> + } >> + >> + kvfree(buffer); >> + return done_zones; >> +} >> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c >> index e519dc0d9fe7..88fa39853c61 100644 >> --- a/drivers/block/ublk_drv.c >> +++ b/drivers/block/ublk_drv.c >> @@ -57,12 +57,13 @@ >> | UBLK_F_USER_RECOVERY_REISSUE \ >> | UBLK_F_UNPRIVILEGED_DEV \ >> | UBLK_F_CMD_IOCTL_ENCODE \ >> - | UBLK_F_USER_COPY) >> + | UBLK_F_USER_COPY \ >> + | UBLK_F_ZONED) >> /* All UBLK_PARAM_TYPE_* should be included here */ >> -#define UBLK_PARAM_TYPE_ALL (UBLK_PARAM_TYPE_BASIC | \ >> - UBLK_PARAM_TYPE_DISCARD | UBLK_PARAM_TYPE_DEVT) >> - >> +#define UBLK_PARAM_TYPE_ALL \ >> + (UBLK_PARAM_TYPE_BASIC | UBLK_PARAM_TYPE_DISCARD | \ >> + UBLK_PARAM_TYPE_DEVT | UBLK_PARAM_TYPE_ZONED) >> struct ublk_uring_cmd_pdu { >> struct ublk_queue *ubq; >> @@ -209,6 +210,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) >> @@ -269,6 +273,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; >> } >> @@ -439,6 +446,7 @@ static const struct block_device_operations ub_fops = { >> .owner = THIS_MODULE, >> .open = ublk_open, >> .free_disk = ublk_free_disk, >> + .report_zones = ublk_report_zones, >> }; >> #define UBLK_MAX_PIN_PAGES 32 >> @@ -553,7 +561,8 @@ static inline bool ublk_need_map_req(const struct >> request *req) >> static inline bool ublk_need_unmap_req(const struct request *req) >> { >> - return ublk_rq_has_data(req) && req_op(req) == REQ_OP_READ; >> + return ublk_rq_has_data(req) && >> + (req_op(req) == REQ_OP_READ || req_op(req) == REQ_OP_DRV_IN); >> } >> static int ublk_map_io(const struct ublk_queue *ubq, const struct request >> *req, >> @@ -637,6 +646,7 @@ static blk_status_t ublk_setup_iod(struct >> ublk_queue *ubq, struct request *req) >> { >> struct ublksrv_io_desc *iod = ublk_get_iod(ubq, req->tag); >> struct ublk_io *io = &ubq->ios[req->tag]; >> + struct ublk_rq_data *pdu = blk_mq_rq_to_pdu(req); >> u32 ublk_op; >> switch (req_op(req)) { >> @@ -655,6 +665,35 @@ 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; >> + 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 = pdu->operation; >> + switch (ublk_op) { >> + case UBLK_IO_OP_REPORT_ZONES: >> + iod->op_flags = ublk_op | ublk_req_build_flags(req); >> + iod->nr_sectors = pdu->nr_sectors; >> + iod->start_sector = pdu->sector; >> + iod->addr = io->addr; >> + return BLK_STS_OK; >> + default: >> + return BLK_STS_IOERR; >> + } >> + case REQ_OP_ZONE_APPEND: >> + case REQ_OP_ZONE_RESET_ALL: >> + case REQ_OP_DRV_OUT: >> + /* We do not support zone append or reset_all yet */ >> + fallthrough; >> default: >> return BLK_STS_IOERR; >> } >> @@ -708,7 +747,8 @@ static inline 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) >> goto exit; >> /* for READ request, writing data in iod->addr to rq buffers */ >> @@ -1835,6 +1875,15 @@ static int ublk_ctrl_start_dev(struct >> ublk_device *ub, struct io_uring_cmd *cmd) >> if (ub->nr_privileged_daemon != ub->nr_queues_ready) >> set_bit(GD_SUPPRESS_PART_SCAN, &disk->state); >> + 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); >> ub->dev_info.state = UBLK_S_DEV_LIVE; >> ret = add_disk(disk); >> @@ -1997,6 +2046,9 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd) >> if (ub->dev_info.flags & UBLK_F_USER_COPY) >> ub->dev_info.flags &= ~UBLK_F_NEED_GET_DATA; >> + if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED)) >> + ub->dev_info.flags &= ~UBLK_F_ZONED; >> + >> /* We are not ready to support zero copy */ >> ub->dev_info.flags &= ~UBLK_F_SUPPORT_ZERO_COPY; >> diff --git a/drivers/block/ublk_drv.h b/drivers/block/ublk_drv.h >> index f81e62256456..7242430fd6b9 100644 >> --- a/drivers/block/ublk_drv.h >> +++ b/drivers/block/ublk_drv.h >> @@ -50,6 +50,21 @@ struct ublk_rq_data { >> struct llist_node node; >> struct kref ref; >> + enum ublk_op operation; >> + __u64 sector; >> + __u32 nr_sectors; >> }; >> +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); >> + >> +#ifdef CONFIG_BLK_DEV_UBLK_ZONED >> +int ublk_report_zones(struct gendisk *disk, sector_t sector, >> + unsigned int nr_zones, report_zones_cb cb, >> + void *data); >> +#else >> +#define ublk_report_zones NULL >> +#endif >> + >> #endif >> diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h >> index 471b3b983045..436525afffe8 100644 >> --- a/include/uapi/linux/ublk_cmd.h >> +++ b/include/uapi/linux/ublk_cmd.h >> @@ -176,6 +176,11 @@ >> /* Copy between request and user buffer by pread()/pwrite() */ >> #define UBLK_F_USER_COPY (1UL << 7) >> +/* >> + * Enable zoned device support >> + */ >> +#define UBLK_F_ZONED (1ULL << 8) >> + >> /* device state */ >> #define UBLK_S_DEV_DEAD 0 >> #define UBLK_S_DEV_LIVE 1 >> @@ -242,6 +247,7 @@ enum ublk_op { >> UBLK_IO_OP_ZONE_APPEND = 13, >> UBLK_IO_OP_ZONE_RESET = 15, >> __UBLK_IO_OP_DRV_IN_START = 32, >> + UBLK_IO_OP_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, >> @@ -342,6 +348,12 @@ struct ublk_param_devt { >> __u32 disk_minor; >> }; >> +struct ublk_param_zoned { >> + __u32 max_open_zones; >> + __u32 max_active_zones; >> + __u8 reserved[24]; >> +}; >> + >> struct ublk_params { >> /* >> * Total length of parameters, userspace has to set 'len' for both >> @@ -353,11 +365,13 @@ struct ublk_params { >> #define UBLK_PARAM_TYPE_BASIC (1 << 0) >> #define UBLK_PARAM_TYPE_DISCARD (1 << 1) >> #define UBLK_PARAM_TYPE_DEVT (1 << 2) >> +#define UBLK_PARAM_TYPE_ZONED (1 << 3) >> __u32 types; /* types of parameter included */ >> struct ublk_param_basic basic; >> struct ublk_param_discard discard; >> struct ublk_param_devt devt; >> + struct ublk_param_zoned zoned; >> }; >> #endif > I applied the patches and verified it compiles with and without the > CONFIG_BLK_DEV_ZONED flag. > Working on creating a ublk device also. For reference, here are my test scripts: ------------------------------------ test-append.sh: #!/bin/bash set -e set -x /mnt/nullblk.py ./ublk add -x --usercopy -t loop -f /dev/nullblk -q 8 -d 128 mkzonefs -f /dev/ublkb0 mount -t zonefs /dev/ublkb0 /zone fio /mnt/append.fio umount /zone ./ublk del 0 ------------------------------------ nullblk.py: #!/usr/bin/env python3 from subprocess import run from pathlib import Path block_size_bytes = 4096 zone_size_mib = 1024 num_zones = 32 config_path = Path("/sys/kernel/config/nullb") # Remove all devices for node in config_path.iterdir(): if node.is_dir(): node.rmdir() device_capacity_mib = zone_size_mib * num_zones control = config_path / "nullblk" control.mkdir() (control / "blocksize").write_text(str(block_size_bytes)) (control / "completion_nsec").write_text("0") (control / "irqmode").write_text("0") (control / "queue_mode").write_text("2") (control / "hw_queue_depth").write_text("1024") (control / "memory_backed").write_text("1") (control / "size").write_text(str(device_capacity_mib)) (control / "zoned").write_text("1") (control / "zone_size").write_text(str(zone_size_mib)) (control / "zone_nr_conv").write_text("0") (control / "zone_max_active").write_text("14") (control / "zone_max_open").write_text("14") (control / "power").write_text("1") ------------------------------------ append.fio: [global] ioengine=psync do_verify=1 verify=md5 random_generator=lfsr [append0] file_append=1 rw=write bs=4k direct=1 size=1024m filename=/zone/seq/0 [append1] file_append=1 rw=write bs=4k direct=1 size=1024m filename=/zone/seq/1 [append2] file_append=1 rw=write bs=4k direct=1 size=1024m filename=/zone/seq/2 [append3] file_append=1 rw=write bs=4k direct=1 size=1024m filename=/zone/seq/3 [append4] file_append=1 rw=write bs=4k direct=1 size=1024m filename=/zone/seq/4 [append5] file_append=1 rw=write bs=4k direct=1 size=1024m filename=/zone/seq/5 ... Nothing fancy, but it will test the append path. I have been running the ZenFS test suite as well, but that does not hit the append path. Maybe if you run it against zonefs? Best regards, Andreas
Ming Lei <ming.lei@redhat.com> writes: > On Wed, Jun 28, 2023 at 09:06:48PM +0200, Andreas Hindborg wrote: >> From: Andreas Hindborg <a.hindborg@samsung.com> >> >> 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 >> >> Note: This commit changes the ublk kernel module name from `ublk_drv.ko` to >> `ublk.ko` in order to link multiple translation units into the module. >> >> Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com> >> --- >> MAINTAINERS | 1 + >> drivers/block/Kconfig | 4 + >> drivers/block/Makefile | 4 +- >> drivers/block/ublk_drv-zoned.c | 144 +++++++++++++++++++++++++++++++++ >> drivers/block/ublk_drv.c | 64 +++++++++++++-- >> drivers/block/ublk_drv.h | 15 ++++ >> include/uapi/linux/ublk_cmd.h | 14 ++++ >> 7 files changed, 239 insertions(+), 7 deletions(-) >> create mode 100644 drivers/block/ublk_drv-zoned.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index ace71c90751c..db8a8deb5926 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -21555,6 +21555,7 @@ S: Maintained >> F: Documentation/block/ublk.rst >> F: drivers/block/ublk_drv.c >> F: drivers/block/ublk_drv.h >> +F: drivers/block/ublk_drv-zoned.c >> F: include/uapi/linux/ublk_cmd.h >> >> UCLINUX (M68KNOMMU AND COLDFIRE) >> diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig >> index 5b9d4aaebb81..c58dfd035557 100644 >> --- a/drivers/block/Kconfig >> +++ b/drivers/block/Kconfig >> @@ -402,6 +402,10 @@ config BLKDEV_UBLK_LEGACY_OPCODES >> suggested to enable N if your application(ublk server) switches to >> ioctl command encoding. >> >> +config BLK_DEV_UBLK_ZONED >> + def_bool y >> + depends on BLK_DEV_UBLK && BLK_DEV_ZONED >> + >> source "drivers/block/rnbd/Kconfig" >> >> endif # BLK_DEV >> diff --git a/drivers/block/Makefile b/drivers/block/Makefile >> index 101612cba303..bc1649e20ec2 100644 >> --- a/drivers/block/Makefile >> +++ b/drivers/block/Makefile >> @@ -37,6 +37,8 @@ obj-$(CONFIG_BLK_DEV_RNBD) += rnbd/ >> >> obj-$(CONFIG_BLK_DEV_NULL_BLK) += null_blk/ >> >> -obj-$(CONFIG_BLK_DEV_UBLK) += ublk_drv.o >> +obj-$(CONFIG_BLK_DEV_UBLK) += ublk.o >> +ublk-$(CONFIG_BLK_DEV_UBLK) += ublk_drv.o >> +ublk-$(CONFIG_BLK_DEV_UBLK_ZONED) += ublk_drv-zoned.o >> >> swim_mod-y := swim.o swim_asm.o >> diff --git a/drivers/block/ublk_drv-zoned.c b/drivers/block/ublk_drv-zoned.c >> new file mode 100644 >> index 000000000000..ea86bf4b3681 >> --- /dev/null >> +++ b/drivers/block/ublk_drv-zoned.c >> @@ -0,0 +1,144 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright 2023 Andreas Hindborg <a.hindborg@samsung.com> >> + */ >> +#include <linux/blkzoned.h> >> +#include <linux/ublk_cmd.h> >> +#include <linux/vmalloc.h> >> + >> +#include "ublk_drv.h" >> + >> +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; >> +} >> + >> +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); >> + } >> +} >> + >> +int ublk_revalidate_disk_zones(struct gendisk *disk) >> +{ >> + return blk_revalidate_disk_zones(disk, NULL); >> +} >> + >> +/* Based on virtblk_alloc_report_buffer */ >> +static void *ublk_alloc_report_buffer(struct ublk_device *ublk, >> + unsigned int nr_zones, >> + unsigned int zone_sectors, size_t *buflen) >> +{ >> + struct request_queue *q = ublk->ub_disk->queue; >> + size_t bufsize; >> + void *buf; >> + >> + nr_zones = min_t(unsigned int, nr_zones, >> + get_capacity(ublk->ub_disk) >> ilog2(zone_sectors)); >> + >> + bufsize = nr_zones * sizeof(struct blk_zone); >> + bufsize = >> + min_t(size_t, bufsize, queue_max_hw_sectors(q) << SECTOR_SHIFT); >> + bufsize = min_t(size_t, bufsize, queue_max_segments(q) << PAGE_SHIFT); > > This means 'blk_zone'(includes any sub-field type) is reused as part of ublk's UAPI, > and it is smart, please document it around UBLK_IO_OP_REPORT_ZONES's definition. Will do
diff --git a/MAINTAINERS b/MAINTAINERS index ace71c90751c..db8a8deb5926 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -21555,6 +21555,7 @@ S: Maintained F: Documentation/block/ublk.rst F: drivers/block/ublk_drv.c F: drivers/block/ublk_drv.h +F: drivers/block/ublk_drv-zoned.c F: include/uapi/linux/ublk_cmd.h UCLINUX (M68KNOMMU AND COLDFIRE) diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig index 5b9d4aaebb81..c58dfd035557 100644 --- a/drivers/block/Kconfig +++ b/drivers/block/Kconfig @@ -402,6 +402,10 @@ config BLKDEV_UBLK_LEGACY_OPCODES suggested to enable N if your application(ublk server) switches to ioctl command encoding. +config BLK_DEV_UBLK_ZONED + def_bool y + depends on BLK_DEV_UBLK && BLK_DEV_ZONED + source "drivers/block/rnbd/Kconfig" endif # BLK_DEV diff --git a/drivers/block/Makefile b/drivers/block/Makefile index 101612cba303..bc1649e20ec2 100644 --- a/drivers/block/Makefile +++ b/drivers/block/Makefile @@ -37,6 +37,8 @@ obj-$(CONFIG_BLK_DEV_RNBD) += rnbd/ obj-$(CONFIG_BLK_DEV_NULL_BLK) += null_blk/ -obj-$(CONFIG_BLK_DEV_UBLK) += ublk_drv.o +obj-$(CONFIG_BLK_DEV_UBLK) += ublk.o +ublk-$(CONFIG_BLK_DEV_UBLK) += ublk_drv.o +ublk-$(CONFIG_BLK_DEV_UBLK_ZONED) += ublk_drv-zoned.o swim_mod-y := swim.o swim_asm.o diff --git a/drivers/block/ublk_drv-zoned.c b/drivers/block/ublk_drv-zoned.c new file mode 100644 index 000000000000..ea86bf4b3681 --- /dev/null +++ b/drivers/block/ublk_drv-zoned.c @@ -0,0 +1,144 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright 2023 Andreas Hindborg <a.hindborg@samsung.com> + */ +#include <linux/blkzoned.h> +#include <linux/ublk_cmd.h> +#include <linux/vmalloc.h> + +#include "ublk_drv.h" + +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; +} + +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); + } +} + +int ublk_revalidate_disk_zones(struct gendisk *disk) +{ + return blk_revalidate_disk_zones(disk, NULL); +} + +/* Based on virtblk_alloc_report_buffer */ +static void *ublk_alloc_report_buffer(struct ublk_device *ublk, + unsigned int nr_zones, + unsigned int zone_sectors, size_t *buflen) +{ + struct request_queue *q = ublk->ub_disk->queue; + size_t bufsize; + void *buf; + + nr_zones = min_t(unsigned int, nr_zones, + get_capacity(ublk->ub_disk) >> ilog2(zone_sectors)); + + bufsize = nr_zones * sizeof(struct blk_zone); + bufsize = + min_t(size_t, bufsize, queue_max_hw_sectors(q) << SECTOR_SHIFT); + bufsize = min_t(size_t, bufsize, queue_max_segments(q) << PAGE_SHIFT); + + while (bufsize >= sizeof(struct blk_zone)) { + buf = __vmalloc(bufsize, GFP_KERNEL | __GFP_NORETRY); + if (buf) { + *buflen = bufsize; + return buf; + } + bufsize >>= 1; + } + + bufsize = 0; + return NULL; +} + +int ublk_report_zones(struct gendisk *disk, sector_t sector, + unsigned int nr_zones, report_zones_cb cb, void *data) +{ + struct ublk_device *ub = disk->private_data; + unsigned int zone_size_sectors = disk->queue->limits.chunk_sectors; + unsigned int first_zone = sector >> ilog2(zone_size_sectors); + unsigned int done_zones = 0; + unsigned int max_zones_per_request; + struct blk_zone *buffer; + size_t buffer_length; + + if (!(ub->dev_info.flags & UBLK_F_ZONED)) + return -EOPNOTSUPP; + + nr_zones = min_t(unsigned int, ub->ub_disk->nr_zones - first_zone, + nr_zones); + + buffer = ublk_alloc_report_buffer(ub, nr_zones, zone_size_sectors, + &buffer_length); + if (!buffer) + return -ENOMEM; + + max_zones_per_request = buffer_length / sizeof(struct blk_zone); + + while (done_zones < nr_zones) { + unsigned int remaining_zones = nr_zones - done_zones; + unsigned int zones_in_request = min_t( + unsigned int, remaining_zones, max_zones_per_request); + int err = 0; + struct request *req; + struct ublk_rq_data *pdu; + blk_status_t status; + + memset(buffer, 0, buffer_length); + + req = blk_mq_alloc_request(disk->queue, REQ_OP_DRV_IN, 0); + if (IS_ERR(req)) + return PTR_ERR(req); + + pdu = blk_mq_rq_to_pdu(req); + pdu->operation = UBLK_IO_OP_REPORT_ZONES; + pdu->sector = sector; + pdu->nr_sectors = remaining_zones * zone_size_sectors; + + err = blk_rq_map_kern(disk->queue, req, buffer, buffer_length, + GFP_KERNEL); + if (err) { + blk_mq_free_request(req); + kvfree(buffer); + return err; + } + + status = blk_execute_rq(req, 0); + err = blk_status_to_errno(status); + blk_mq_free_request(req); + if (err) { + kvfree(buffer); + return err; + } + + for (unsigned int i = 0; i < zones_in_request; i++) { + struct blk_zone *zone = buffer + i; + + err = cb(zone, i, data); + if (err) + return err; + + done_zones++; + sector += zone_size_sectors; + + /* A zero length zone means don't ask for more zones */ + if (!zone->len) { + kvfree(buffer); + return done_zones; + } + } + } + + kvfree(buffer); + return done_zones; +} diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index e519dc0d9fe7..88fa39853c61 100644 --- a/drivers/block/ublk_drv.c +++ b/drivers/block/ublk_drv.c @@ -57,12 +57,13 @@ | UBLK_F_USER_RECOVERY_REISSUE \ | UBLK_F_UNPRIVILEGED_DEV \ | UBLK_F_CMD_IOCTL_ENCODE \ - | UBLK_F_USER_COPY) + | UBLK_F_USER_COPY \ + | UBLK_F_ZONED) /* All UBLK_PARAM_TYPE_* should be included here */ -#define UBLK_PARAM_TYPE_ALL (UBLK_PARAM_TYPE_BASIC | \ - UBLK_PARAM_TYPE_DISCARD | UBLK_PARAM_TYPE_DEVT) - +#define UBLK_PARAM_TYPE_ALL \ + (UBLK_PARAM_TYPE_BASIC | UBLK_PARAM_TYPE_DISCARD | \ + UBLK_PARAM_TYPE_DEVT | UBLK_PARAM_TYPE_ZONED) struct ublk_uring_cmd_pdu { struct ublk_queue *ubq; @@ -209,6 +210,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) @@ -269,6 +273,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; } @@ -439,6 +446,7 @@ static const struct block_device_operations ub_fops = { .owner = THIS_MODULE, .open = ublk_open, .free_disk = ublk_free_disk, + .report_zones = ublk_report_zones, }; #define UBLK_MAX_PIN_PAGES 32 @@ -553,7 +561,8 @@ static inline bool ublk_need_map_req(const struct request *req) static inline bool ublk_need_unmap_req(const struct request *req) { - return ublk_rq_has_data(req) && req_op(req) == REQ_OP_READ; + return ublk_rq_has_data(req) && + (req_op(req) == REQ_OP_READ || req_op(req) == REQ_OP_DRV_IN); } static int ublk_map_io(const struct ublk_queue *ubq, const struct request *req, @@ -637,6 +646,7 @@ static blk_status_t ublk_setup_iod(struct ublk_queue *ubq, struct request *req) { struct ublksrv_io_desc *iod = ublk_get_iod(ubq, req->tag); struct ublk_io *io = &ubq->ios[req->tag]; + struct ublk_rq_data *pdu = blk_mq_rq_to_pdu(req); u32 ublk_op; switch (req_op(req)) { @@ -655,6 +665,35 @@ 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; + 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 = pdu->operation; + switch (ublk_op) { + case UBLK_IO_OP_REPORT_ZONES: + iod->op_flags = ublk_op | ublk_req_build_flags(req); + iod->nr_sectors = pdu->nr_sectors; + iod->start_sector = pdu->sector; + iod->addr = io->addr; + return BLK_STS_OK; + default: + return BLK_STS_IOERR; + } + case REQ_OP_ZONE_APPEND: + case REQ_OP_ZONE_RESET_ALL: + case REQ_OP_DRV_OUT: + /* We do not support zone append or reset_all yet */ + fallthrough; default: return BLK_STS_IOERR; } @@ -708,7 +747,8 @@ static inline 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) goto exit; /* for READ request, writing data in iod->addr to rq buffers */ @@ -1835,6 +1875,15 @@ static int ublk_ctrl_start_dev(struct ublk_device *ub, struct io_uring_cmd *cmd) if (ub->nr_privileged_daemon != ub->nr_queues_ready) set_bit(GD_SUPPRESS_PART_SCAN, &disk->state); + 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); ub->dev_info.state = UBLK_S_DEV_LIVE; ret = add_disk(disk); @@ -1997,6 +2046,9 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd) if (ub->dev_info.flags & UBLK_F_USER_COPY) ub->dev_info.flags &= ~UBLK_F_NEED_GET_DATA; + if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED)) + ub->dev_info.flags &= ~UBLK_F_ZONED; + /* We are not ready to support zero copy */ ub->dev_info.flags &= ~UBLK_F_SUPPORT_ZERO_COPY; diff --git a/drivers/block/ublk_drv.h b/drivers/block/ublk_drv.h index f81e62256456..7242430fd6b9 100644 --- a/drivers/block/ublk_drv.h +++ b/drivers/block/ublk_drv.h @@ -50,6 +50,21 @@ struct ublk_rq_data { struct llist_node node; struct kref ref; + enum ublk_op operation; + __u64 sector; + __u32 nr_sectors; }; +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); + +#ifdef CONFIG_BLK_DEV_UBLK_ZONED +int ublk_report_zones(struct gendisk *disk, sector_t sector, + unsigned int nr_zones, report_zones_cb cb, + void *data); +#else +#define ublk_report_zones NULL +#endif + #endif diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h index 471b3b983045..436525afffe8 100644 --- a/include/uapi/linux/ublk_cmd.h +++ b/include/uapi/linux/ublk_cmd.h @@ -176,6 +176,11 @@ /* Copy between request and user buffer by pread()/pwrite() */ #define UBLK_F_USER_COPY (1UL << 7) +/* + * Enable zoned device support + */ +#define UBLK_F_ZONED (1ULL << 8) + /* device state */ #define UBLK_S_DEV_DEAD 0 #define UBLK_S_DEV_LIVE 1 @@ -242,6 +247,7 @@ enum ublk_op { UBLK_IO_OP_ZONE_APPEND = 13, UBLK_IO_OP_ZONE_RESET = 15, __UBLK_IO_OP_DRV_IN_START = 32, + UBLK_IO_OP_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, @@ -342,6 +348,12 @@ struct ublk_param_devt { __u32 disk_minor; }; +struct ublk_param_zoned { + __u32 max_open_zones; + __u32 max_active_zones; + __u8 reserved[24]; +}; + struct ublk_params { /* * Total length of parameters, userspace has to set 'len' for both @@ -353,11 +365,13 @@ struct ublk_params { #define UBLK_PARAM_TYPE_BASIC (1 << 0) #define UBLK_PARAM_TYPE_DISCARD (1 << 1) #define UBLK_PARAM_TYPE_DEVT (1 << 2) +#define UBLK_PARAM_TYPE_ZONED (1 << 3) __u32 types; /* types of parameter included */ struct ublk_param_basic basic; struct ublk_param_discard discard; struct ublk_param_devt devt; + struct ublk_param_zoned zoned; }; #endif