[PATCH V3 0/5] ublk_drv: add generic mechanism to get/set parameters

[PATCH V3 0/5] ublk_drv: add generic mechanism to get/set parameters

[Ming Lei]

Hello Jens,

The 1st two patches fixes ublk device leak or hang issue in case of some
failure path, such as, failing to start device.

The 3rd patch adds two control commands for setting/getting device
parameters in generic way, and easy to extend to add new parameter
type.

The 4th patch adds two parameter type: basic and discard, and basic
parameter covers basic block queue setting or someone which can't
be grouped to other types, basic parameter has to be set; discard
parameter is optional.

The 5th patch cleans UAPI of ublksrv_ctrl_dev_info, and userspace has
to be updated for this driver change, so please consider this patchset
for v5.20.

Verified by all targets in the following branch:

https://github.com/ming1/ubdsrv/tree/parameter

Also all device data including parameters is exported as json in
above tree.


V3:
- drop device reference after add_disk fails, as suggested by Christoph, 1/5
- simplify the 3rd patch: replace xarray with plain array, remove
  function table and avoid indirect call, then reduce boiler plate for
  addressing Christoph's comment

V2:
- re-organize patches
- limit parameter max length
- take Christoph's approach to replace bitfields with flags, and use
char type to define block size shift
- add two fixes, which is triggered when testing set/get parameter
  commands
- cleanup uapi header


Ming Lei (5):
  ublk_drv: cancel device even though disk isn't up
  ublk_drv: fix ublk device leak in case that add_disk fails
  ublk_drv: add SET_PARAM/GET_PARAM control command
  ublk_drv: add two parameter types
  ublk_drv: cleanup ublksrv_ctrl_dev_info

 drivers/block/ublk_drv.c      | 333 ++++++++++++++++++++++++++++++----
 include/uapi/linux/ublk_cmd.h |  57 +++++-
 2 files changed, 352 insertions(+), 38 deletions(-)

-- 
2.31.1

[PATCH V3 1/5] ublk_drv: cancel device even though disk isn't up

[Ming Lei]

Each ublk queue is started before adding disk, we have to cancel queues in
ublk_stop_dev() so that ubq daemon can be exited, otherwise DEL_DEV command
may hang forever.

Also avoid to cancel queues two times by checking if queue is ready,
otherwise use-after-free on io_uring may be triggered because ublk_stop_dev
is called by ublk_remove() too.

Fixes: 71f28f3136af ("ublk_drv: add io_uring based userspace block driver")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/block/ublk_drv.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 3f1906965ac8..7ece4c2ef062 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -788,16 +788,27 @@ static void ublk_daemon_monitor_work(struct work_struct *work)
 				UBLK_DAEMON_MONITOR_PERIOD);
 }
 
+static inline bool ublk_queue_ready(struct ublk_queue *ubq)
+{
+	return ubq->nr_io_ready == ubq->q_depth;
+}
+
 static void ublk_cancel_queue(struct ublk_queue *ubq)
 {
 	int i;
 
+	if (!ublk_queue_ready(ubq))
+		return;
+
 	for (i = 0; i < ubq->q_depth; i++) {
 		struct ublk_io *io = &ubq->ios[i];
 
 		if (io->flags & UBLK_IO_FLAG_ACTIVE)
 			io_uring_cmd_done(io->cmd, UBLK_IO_RES_ABORT, 0);
 	}
+
+	/* all io commands are canceled */
+	ubq->nr_io_ready = 0;
 }
 
 /* Cancel all pending commands, must be called after del_gendisk() returns */
@@ -818,19 +829,14 @@ static void ublk_stop_dev(struct ublk_device *ub)
 	del_gendisk(ub->ub_disk);
 	ub->dev_info.state = UBLK_S_DEV_DEAD;
 	ub->dev_info.ublksrv_pid = -1;
-	ublk_cancel_dev(ub);
 	put_disk(ub->ub_disk);
 	ub->ub_disk = NULL;
  unlock:
+	ublk_cancel_dev(ub);
 	mutex_unlock(&ub->mutex);
 	cancel_delayed_work_sync(&ub->monitor_work);
 }
 
-static inline bool ublk_queue_ready(struct ublk_queue *ubq)
-{
-	return ubq->nr_io_ready == ubq->q_depth;
-}
-
 /* device can only be started after all IOs are ready */
 static void ublk_mark_io_ready(struct ublk_device *ub, struct ublk_queue *ubq)
 {
-- 
2.31.1

[PATCH V3 2/5] ublk_drv: fix ublk device leak in case that add_disk fails

[Ming Lei]

->free_disk is only called after disk is added successfully, so
drop ublk device reference in case of add_disk() failure.

Fixes: 6d9e6dfdf3b2 ("ublk: defer disk allocation")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/block/ublk_drv.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 7ece4c2ef062..00b04f395de0 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -1190,6 +1190,11 @@ static int ublk_ctrl_start_dev(struct io_uring_cmd *cmd)
 	get_device(&ub->cdev_dev);
 	ret = add_disk(disk);
 	if (ret) {
+		/*
+		 * has to drop the reference since ->free_disk won't be
+		 * called in case of add_disk failure
+		 */
+		ublk_put_device(ub);
 		put_disk(disk);
 		goto out_unlock;
 	}
-- 
2.31.1

[PATCH V3 3/5] ublk_drv: add SET_PARAM/GET_PARAM control command

[Ming Lei]

Add two commands to set/get parameters generically.

One important goal of ublk is to provide generic framework for making
block device by userspace flexibly.

As one generic block device, there are still lots of block parameters,
such as max_sectors, write_cache/fua, discard related limits,
zoned parameters, ...., so this patch starts to add mechanism for set/get
device parameters.

Both generic block parameters(all kinds of queue settings) and ublk feature
related parameters can be covered with this way, then it becomes quite easy
to extend in future.

The parameter passed from userspace is added to one array, and the type is
used as index of the array. The following patch will add two parameter
types: basic(covers basic queue setting and misc settings which can't be grouped
easily) and discard. Also segment, zoned[1], userspace recovery[2] and
userspace backing buffer(in my todo list) parameters will be added in near
future.

This way provides mechanism to simulate any kind of generic block device from
userspace easily, from both block queue setting viewpoint or ublk feature
viewpoint.

[1] https://lore.kernel.org/qemu-devel/fa7de750-8def-c532-8c86-64c7505608e0@opensource.wdc.com/T/#u
[2] https://lore.kernel.org/linux-block/60bc9e53-605e-ee1d-9bd2-020693768339@linux.alibaba.com/

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/block/ublk_drv.c      | 111 ++++++++++++++++++++++++++++++++++
 include/uapi/linux/ublk_cmd.h |  14 +++++
 2 files changed, 125 insertions(+)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 00b04f395de0..f949c0d96360 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -137,6 +137,8 @@ struct ublk_device {
 	spinlock_t		mm_lock;
 	struct mm_struct	*mm;
 
+	struct ublk_param_header *params[UBLK_PARAM_TYPE_LAST];
+
 	struct completion	completion;
 	unsigned int		nr_queues_ready;
 	atomic_t		nr_aborted_queues;
@@ -160,6 +162,28 @@ static DEFINE_MUTEX(ublk_ctl_mutex);
 
 static struct miscdevice ublk_misc;
 
+static int ublk_validate_param_header(const struct ublk_device *ub,
+		const struct ublk_param_header *h)
+{
+	if (h->type >= UBLK_PARAM_TYPE_LAST)
+		return -EINVAL;
+	if (h->len > UBLK_MAX_PARAM_LEN)
+		return -EINVAL;
+	return 0;
+}
+
+/* Old parameter with same type will be overridden */
+static int ublk_install_param(struct ublk_device *ub,
+		struct ublk_param_header *h)
+{
+	struct ublk_param_header *old = ub->params[h->type];
+
+	kfree(old);
+	ub->params[h->type] = h;
+
+	return 0;
+}
+
 static inline bool ublk_can_use_task_work(const struct ublk_queue *ubq)
 {
 	if (IS_BUILTIN(CONFIG_BLK_DEV_UBLK) &&
@@ -1447,6 +1471,87 @@ static int ublk_ctrl_get_dev_info(struct io_uring_cmd *cmd)
 	return ret;
 }
 
+static int ublk_ctrl_get_param(struct io_uring_cmd *cmd)
+{
+	struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd;
+	void __user *argp = (void __user *)(unsigned long)header->addr;
+	struct ublk_device *ub;
+	struct ublk_param_header ph;
+	struct ublk_param_header *param = NULL;
+	int ret = 0;
+
+	if (header->len <= sizeof(ph) || !header->addr)
+		return -EINVAL;
+
+	ub = ublk_get_device_from_id(header->dev_id);
+	if (!ub)
+		return -EINVAL;
+
+	ret = -EFAULT;
+	if (copy_from_user(&ph, argp, sizeof(ph)))
+		goto out_put;
+
+	ret = ublk_validate_param_header(ub, &ph);
+	if (ret)
+		goto out_put;
+
+	mutex_lock(&ub->mutex);
+	param = ub->params[ph.type];
+	mutex_unlock(&ub->mutex);
+	if (!param)
+		ret = -EINVAL;
+	else if (copy_to_user(argp, param, ph.len))
+		ret = -EFAULT;
+out_put:
+	ublk_put_device(ub);
+	return ret;
+}
+
+static int ublk_ctrl_set_param(struct io_uring_cmd *cmd)
+{
+	struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd;
+	void __user *argp = (void __user *)(unsigned long)header->addr;
+	struct ublk_device *ub;
+	struct ublk_param_header ph;
+	struct ublk_param_header *param = NULL;
+	int ret = -EFAULT;
+
+	if (header->len <= sizeof(ph) || !header->addr)
+		return -EINVAL;
+
+	ub = ublk_get_device_from_id(header->dev_id);
+	if (!ub)
+		return -EINVAL;
+
+	if (copy_from_user(&ph, argp, sizeof(ph)))
+		goto out_put;
+
+	ret = ublk_validate_param_header(ub, &ph);
+	if (ret)
+		goto out_put;
+
+	param = kmalloc(ph.len, GFP_KERNEL);
+	if (!param) {
+		ret = -ENOMEM;
+	} else if (copy_from_user(param, argp, ph.len)) {
+		ret = -EFAULT;
+	} else {
+		/* parameters can only be changed when device isn't live */
+		mutex_lock(&ub->mutex);
+		if (ub->dev_info.state != UBLK_S_DEV_LIVE)
+			ret = ublk_install_param(ub, param);
+		else
+			ret = -EACCES;
+		mutex_unlock(&ub->mutex);
+	}
+out_put:
+	ublk_put_device(ub);
+	if (ret)
+		kfree(param);
+
+	return ret;
+}
+
 static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
 		unsigned int issue_flags)
 {
@@ -1482,6 +1587,12 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
 	case UBLK_CMD_GET_QUEUE_AFFINITY:
 		ret = ublk_ctrl_get_queue_affinity(cmd);
 		break;
+	case UBLK_CMD_GET_PARAM:
+		ret = ublk_ctrl_get_param(cmd);
+		break;
+	case UBLK_CMD_SET_PARAM:
+		ret = ublk_ctrl_set_param(cmd);
+		break;
 	default:
 		break;
 	}
diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
index ca33092354ab..f6433bb1f5f6 100644
--- a/include/uapi/linux/ublk_cmd.h
+++ b/include/uapi/linux/ublk_cmd.h
@@ -15,6 +15,8 @@
 #define	UBLK_CMD_DEL_DEV		0x05
 #define	UBLK_CMD_START_DEV	0x06
 #define	UBLK_CMD_STOP_DEV	0x07
+#define	UBLK_CMD_SET_PARAM	0x08
+#define	UBLK_CMD_GET_PARAM	0x09
 
 /*
  * IO commands, issued by ublk server, and handled by ublk driver.
@@ -158,4 +160,16 @@ struct ublksrv_io_cmd {
 	__u64	addr;
 };
 
+/* 512 is big enough for single parameter */
+#define UBLK_MAX_PARAM_LEN	512
+
+enum {
+	UBLK_PARAM_TYPE_LAST,
+};
+
+struct ublk_param_header {
+	__u16 type;
+	__u16 len;
+};
+
 #endif
-- 
2.31.1

[PATCH V3 4/5] ublk_drv: add two parameter types

[Ming Lei]

Each block devices have lots of queue setting, most of them needs
device's knowledge to configure correctly. Device parameter can be
thought as device's internal knowledge since ublk does implement block
device from userspace.

Add ublk_basic_param & ublk_discard_param first, both two types are
used now.

Another change is that discard support becomes not enabled at default,
and it needs userspace to send ublk_discard_param for enabling it.

Also ublk_basic_param has to be set before sending START_DEV, otherwise
ublk block device won't be setup. Meantime not set dev_blocks from
handling START_DEV any more.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/block/ublk_drv.c      | 190 ++++++++++++++++++++++++++++++----
 include/uapi/linux/ublk_cmd.h |  33 ++++++
 2 files changed, 202 insertions(+), 21 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index f949c0d96360..2f55c1c07a09 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -129,6 +129,7 @@ struct ublk_device {
 
 #define UB_STATE_OPEN		0
 #define UB_STATE_USED		1
+#define UB_STATE_CONFIGURED	2
 	unsigned long		state;
 	int			ub_number;
 
@@ -162,6 +163,108 @@ static DEFINE_MUTEX(ublk_ctl_mutex);
 
 static struct miscdevice ublk_misc;
 
+static int ublk_dev_param_basic_validate(const struct ublk_device *ub,
+		const struct ublk_param_header *header)
+{
+	const struct ublk_basic_param *p = (struct ublk_basic_param *)header;
+
+	if (p->logical_bs_shift > PAGE_SHIFT)
+		return -EINVAL;
+
+	if (p->logical_bs_shift > p->physical_bs_shift)
+		return -EINVAL;
+
+	if (p->max_sectors > (ub->dev_info.rq_max_blocks << (ub->bs_shift - 9)))
+		return -EINVAL;
+
+	return 0;
+}
+
+/* basic param is the only one parameter which has to be set */
+static int ublk_dev_param_basic_apply(struct ublk_device *ub,
+		const struct ublk_param_header *header)
+{
+	struct request_queue *q = ub->ub_disk->queue;
+	const struct ublk_basic_param *p = (struct ublk_basic_param *)header;
+
+	blk_queue_logical_block_size(q, 1 << p->logical_bs_shift);
+	blk_queue_physical_block_size(q, 1 << p->physical_bs_shift);
+	blk_queue_io_min(q, 1 << p->io_min_shift);
+	blk_queue_io_opt(q, 1 << p->io_opt_shift);
+
+	blk_queue_write_cache(q, p->attrs & UBLK_ATTR_VOLATILE_CACHE,
+			p->attrs & UBLK_ATTR_FUA);
+	if (p->attrs & UBLK_ATTR_ROTATIONAL)
+		blk_queue_flag_clear(QUEUE_FLAG_NONROT, q);
+	else
+		blk_queue_flag_set(QUEUE_FLAG_NONROT, q);
+
+	blk_queue_max_hw_sectors(q, p->max_sectors);
+	blk_queue_chunk_sectors(q, p->chunk_sectors);
+	blk_queue_virt_boundary(q, p->virt_boundary_mask);
+
+	if (p->attrs & UBLK_ATTR_READ_ONLY)
+		set_disk_ro(ub->ub_disk, true);
+
+	set_capacity(ub->ub_disk, p->dev_sectors);
+
+	set_bit(UB_STATE_CONFIGURED, &ub->state);
+
+	return 0;
+}
+
+static int ublk_dev_param_discard_validate(const struct ublk_device *ub,
+		const struct ublk_param_header *header)
+{
+	const struct ublk_discard_param *p = (struct ublk_discard_param *)header;
+
+	/* So far, only support single segment discard */
+	if (p->max_discard_sectors && p->max_discard_segments != 1)
+		return -EINVAL;
+	return 0;
+}
+
+static int ublk_dev_param_discard_apply(struct ublk_device *ub,
+		const struct ublk_param_header *header)
+{
+	struct request_queue *q = ub->ub_disk->queue;
+	const struct ublk_discard_param *p = (struct ublk_discard_param *)
+		header;
+
+	q->limits.discard_alignment = p->discard_alignment;
+	q->limits.discard_granularity = p->discard_granularity;
+	blk_queue_max_discard_sectors(q, p->max_discard_sectors);
+	blk_queue_max_write_zeroes_sectors(q,
+			p->max_write_zeroes_sectors);
+	blk_queue_max_discard_segments(q, p->max_discard_segments);
+
+	return 0;
+}
+
+#define __ublk_param_run_op(ub, h, op, param)		\
+	ublk_dev_param_##param##_##op((ub), (h))
+
+#define ublk_param_run_op(ub, h, op)  ({				\
+	int __ret;							\
+									\
+	switch ((h)->type) {						\
+	case UBLK_PARAM_TYPE_BASIC:					\
+		__ret = __ublk_param_run_op(ub, h, op, basic);		\
+		break;							\
+	case UBLK_PARAM_TYPE_DISCARD:					\
+		__ret = __ublk_param_run_op(ub, h, op, discard);	\
+		break;							\
+	default:							\
+		__ret = -EINVAL;					\
+	}								\
+	__ret;								\
+})
+
+static const unsigned short param_len[] = {
+	[UBLK_PARAM_TYPE_BASIC] = sizeof(struct ublk_basic_param),
+	[UBLK_PARAM_TYPE_DISCARD] = sizeof(struct ublk_discard_param),
+};
+
 static int ublk_validate_param_header(const struct ublk_device *ub,
 		const struct ublk_param_header *h)
 {
@@ -169,6 +272,22 @@ static int ublk_validate_param_header(const struct ublk_device *ub,
 		return -EINVAL;
 	if (h->len > UBLK_MAX_PARAM_LEN)
 		return -EINVAL;
+
+	if (h->len != param_len[h->type])
+		return -EINVAL;
+	return 0;
+}
+
+static int ublk_validate_param(const struct ublk_device *ub,
+		const struct ublk_param_header *h)
+{
+	int ret = ublk_validate_param_header(ub, h);
+
+	if (ret)
+		return ret;
+
+	ret = ublk_param_run_op(ub, h, validate);
+
 	return 0;
 }
 
@@ -177,11 +296,39 @@ static int ublk_install_param(struct ublk_device *ub,
 		struct ublk_param_header *h)
 {
 	struct ublk_param_header *old = ub->params[h->type];
+	int ret;
 
-	kfree(old);
-	ub->params[h->type] = h;
+	ret = ublk_validate_param(ub, h);
+	if (!ret) {
+		kfree(old);
+		ub->params[h->type] = h;
+	}
 
-	return 0;
+	return ret;
+}
+
+static void ublk_apply_params(struct ublk_device *ub)
+{
+	int type;
+
+	for (type = 0; type < UBLK_PARAM_TYPE_LAST; type++) {
+		struct ublk_param_header *h = ub->params[type];
+		int ret = 0;
+
+		if (h)
+			ret = ublk_param_run_op(ub, h, apply);
+		if (ret)
+			dev_warn(&ub->cdev_dev, "fail to apply para %d\n",
+					type);
+	}
+}
+
+static void ublk_uninstall_params(struct ublk_device *ub)
+{
+	int type;
+
+	for (type = 0; type < UBLK_PARAM_TYPE_LAST; type++)
+		kfree(ub->params[type]);
 }
 
 static inline bool ublk_can_use_task_work(const struct ublk_queue *ubq)
@@ -239,6 +386,7 @@ static void ublk_free_disk(struct gendisk *disk)
 {
 	struct ublk_device *ub = disk->private_data;
 
+	clear_bit(UB_STATE_CONFIGURED, &ub->state);
 	clear_bit(UB_STATE_USED, &ub->state);
 	put_device(&ub->cdev_dev);
 }
@@ -1074,6 +1222,7 @@ static void ublk_cdev_rel(struct device *dev)
 
 	blk_mq_free_tag_set(&ub->tag_set);
 	ublk_deinit_queues(ub);
+	ublk_uninstall_params(ub);
 	ublk_free_dev_number(ub);
 	mutex_destroy(&ub->mutex);
 	kfree(ub);
@@ -1162,7 +1311,6 @@ static int ublk_ctrl_start_dev(struct io_uring_cmd *cmd)
 {
 	struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd;
 	int ublksrv_pid = (int)header->data[0];
-	unsigned long dev_blocks = header->data[1];
 	struct ublk_device *ub;
 	struct gendisk *disk;
 	int ret = -EINVAL;
@@ -1185,10 +1333,6 @@ static int ublk_ctrl_start_dev(struct io_uring_cmd *cmd)
 		goto out_unlock;
 	}
 
-	/* We may get disk size updated */
-	if (dev_blocks)
-		ub->dev_info.dev_blocks = dev_blocks;
-
 	disk = blk_mq_alloc_disk(&ub->tag_set, ub);
 	if (IS_ERR(disk)) {
 		ret = PTR_ERR(disk);
@@ -1198,32 +1342,33 @@ static int ublk_ctrl_start_dev(struct io_uring_cmd *cmd)
 	disk->fops = &ub_fops;
 	disk->private_data = ub;
 
-	blk_queue_logical_block_size(disk->queue, ub->dev_info.block_size);
-	blk_queue_physical_block_size(disk->queue, ub->dev_info.block_size);
-	blk_queue_io_min(disk->queue, ub->dev_info.block_size);
-	blk_queue_max_hw_sectors(disk->queue,
-		ub->dev_info.rq_max_blocks << (ub->bs_shift - 9));
-	disk->queue->limits.discard_granularity = PAGE_SIZE;
-	blk_queue_max_discard_sectors(disk->queue, UINT_MAX >> 9);
-	blk_queue_max_write_zeroes_sectors(disk->queue, UINT_MAX >> 9);
-
-	set_capacity(disk, ub->dev_info.dev_blocks << (ub->bs_shift - 9));
-
 	ub->dev_info.ublksrv_pid = ublksrv_pid;
 	ub->ub_disk = disk;
+
+	ublk_apply_params(ub);
+
+	/* ublk_basic_param has to be set from userspace */
+	if (!test_bit(UB_STATE_CONFIGURED, &ub->state)) {
+		ret = -EINVAL;
+		goto out_put_disk;
+	}
+
 	get_device(&ub->cdev_dev);
 	ret = add_disk(disk);
 	if (ret) {
+		clear_bit(UB_STATE_CONFIGURED, &ub->state);
 		/*
 		 * has to drop the reference since ->free_disk won't be
 		 * called in case of add_disk failure
 		 */
 		ublk_put_device(ub);
-		put_disk(disk);
-		goto out_unlock;
+		goto out_put_disk;
 	}
 	set_bit(UB_STATE_USED, &ub->state);
 	ub->dev_info.state = UBLK_S_DEV_LIVE;
+out_put_disk:
+	if (ret)
+		put_disk(disk);
 out_unlock:
 	mutex_unlock(&ub->mutex);
 	ublk_put_device(ub);
@@ -1620,6 +1765,9 @@ static int __init ublk_init(void)
 {
 	int ret;
 
+	BUILD_BUG_ON(sizeof(struct ublk_basic_param) > UBLK_MAX_PARAM_LEN);
+	BUILD_BUG_ON(sizeof(struct ublk_discard_param) > UBLK_MAX_PARAM_LEN);
+
 	init_waitqueue_head(&ublk_idr_wq);
 
 	ret = misc_register(&ublk_misc);
diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
index f6433bb1f5f6..78c2b420b720 100644
--- a/include/uapi/linux/ublk_cmd.h
+++ b/include/uapi/linux/ublk_cmd.h
@@ -164,6 +164,8 @@ struct ublksrv_io_cmd {
 #define UBLK_MAX_PARAM_LEN	512
 
 enum {
+	UBLK_PARAM_TYPE_BASIC,
+	UBLK_PARAM_TYPE_DISCARD,
 	UBLK_PARAM_TYPE_LAST,
 };
 
@@ -172,4 +174,35 @@ struct ublk_param_header {
 	__u16 len;
 };
 
+struct ublk_basic_param {
+	struct ublk_param_header  header;
+#define UBLK_ATTR_READ_ONLY            (1 << 0)
+#define UBLK_ATTR_ROTATIONAL           (1 << 1)
+#define UBLK_ATTR_VOLATILE_CACHE       (1 << 2)
+#define UBLK_ATTR_FUA                  (1 << 3)
+	__u32	attrs;
+	__u8	logical_bs_shift;
+	__u8	physical_bs_shift;
+	__u8	io_opt_shift;
+	__u8	io_min_shift;
+
+	__u32	max_sectors;
+	__u32	chunk_sectors;
+
+	__u64   dev_sectors;
+	__u64   virt_boundary_mask;
+};
+
+struct ublk_discard_param {
+	struct ublk_param_header  header;
+	__u32	discard_alignment;
+
+	__u32	discard_granularity;
+	__u32	max_discard_sectors;
+
+	__u32	max_write_zeroes_sectors;
+	__u16	max_discard_segments;
+	__u16	reserved0;
+};
+
 #endif
-- 
2.31.1

[PATCH V3 5/5] ublk_drv: cleanup ublksrv_ctrl_dev_info

[Ming Lei]

Remove all block device related info from ublksrv_ctrl_dev_info,
meantime reduce its size into 64 bytes because:

1) ublksrv_ctrl_dev_info becomes cleaner without including any
block related info

2) generic set/get parameter command can be used to set block
related setting easily and cleanly

3) generic set/get parameter command can be used for extending
ublk without needing more info in ublksrv_ctrl_dev_info

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/block/ublk_drv.c      | 17 +++++++----------
 include/uapi/linux/ublk_cmd.h | 10 +++++-----
 2 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 2f55c1c07a09..b7dd50099aef 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -119,7 +119,6 @@ struct ublk_device {
 	char	*__queues;
 
 	unsigned short  queue_size;
-	unsigned short  bs_shift;
 	struct ublksrv_ctrl_dev_info	dev_info;
 
 	struct blk_mq_tag_set	tag_set;
@@ -174,7 +173,7 @@ static int ublk_dev_param_basic_validate(const struct ublk_device *ub,
 	if (p->logical_bs_shift > p->physical_bs_shift)
 		return -EINVAL;
 
-	if (p->max_sectors > (ub->dev_info.rq_max_blocks << (ub->bs_shift - 9)))
+	if (p->max_sectors > (ub->dev_info.max_io_buf_bytes >> 9))
 		return -EINVAL;
 
 	return 0;
@@ -1262,13 +1261,13 @@ static void ublk_stop_work_fn(struct work_struct *work)
 	ublk_stop_dev(ub);
 }
 
-/* align maximum I/O size to PAGE_SIZE */
+/* align max io buffer size with PAGE_SIZE */
 static void ublk_align_max_io_size(struct ublk_device *ub)
 {
-	unsigned int max_rq_bytes = ub->dev_info.rq_max_blocks << ub->bs_shift;
+	unsigned int max_io_bytes = ub->dev_info.max_io_buf_bytes;
 
-	ub->dev_info.rq_max_blocks =
-		round_down(max_rq_bytes, PAGE_SIZE) >> ub->bs_shift;
+	ub->dev_info.max_io_buf_bytes =
+		round_down(max_io_bytes, PAGE_SIZE);
 }
 
 static int ublk_add_tag_set(struct ublk_device *ub)
@@ -1430,9 +1429,8 @@ static inline void ublk_dump_dev_info(struct ublksrv_ctrl_dev_info *info)
 {
 	pr_devel("%s: dev id %d flags %llx\n", __func__,
 			info->dev_id, info->flags);
-	pr_devel("\t nr_hw_queues %d queue_depth %d block size %d dev_capacity %lld\n",
-			info->nr_hw_queues, info->queue_depth,
-			info->block_size, info->dev_blocks);
+	pr_devel("\t nr_hw_queues %d queue_depth %d\n",
+			info->nr_hw_queues, info->queue_depth);
 }
 
 static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd)
@@ -1492,7 +1490,6 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd)
 	/* We are not ready to support zero copy */
 	ub->dev_info.flags &= ~UBLK_F_SUPPORT_ZERO_COPY;
 
-	ub->bs_shift = ilog2(ub->dev_info.block_size);
 	ub->dev_info.nr_hw_queues = min_t(unsigned int,
 			ub->dev_info.nr_hw_queues, nr_cpu_ids);
 	ublk_align_max_io_size(ub);
diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
index 78c2b420b720..c5a01268060c 100644
--- a/include/uapi/linux/ublk_cmd.h
+++ b/include/uapi/linux/ublk_cmd.h
@@ -80,22 +80,22 @@ struct ublksrv_ctrl_cmd {
 struct ublksrv_ctrl_dev_info {
 	__u16	nr_hw_queues;
 	__u16	queue_depth;
-	__u16	block_size;
+	__u16	reserved0;
 	__u16	state;
 
-	__u32	rq_max_blocks;
+	__u32	max_io_buf_bytes;
 	__u32	dev_id;
 
-	__u64   dev_blocks;
+	__u64   reserved1;
 
 	__s32	ublksrv_pid;
-	__s32	reserved0;
+	__s32	reserved2;
 	__u64	flags;
 	__u64	flags_reserved;
 
 	/* For ublksrv internal use, invisible to ublk driver */
 	__u64	ublksrv_flags;
-	__u64	reserved1[9];
+	__u64	reserved3;
 };
 
 #define		UBLK_IO_OP_READ		0
-- 
2.31.1

Re: [PATCH V3 1/5] ublk_drv: cancel device even though disk isn't up

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH V3 2/5] ublk_drv: fix ublk device leak in case that add_disk fails

[Christoph Hellwig]

On Fri, Jul 29, 2022 at 03:29:51PM +0800, Ming Lei wrote:
>  	get_device(&ub->cdev_dev);
>  	ret = add_disk(disk);
>  	if (ret) {
> +		/*
> +		 * has to drop the reference since ->free_disk won't be
> +		 * called in case of add_disk failure
> +		 */

Nit: Multi-line comments typically start with a capitalized word and
end with a dot to form a proper sentence.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH V3 3/5] ublk_drv: add SET_PARAM/GET_PARAM control command

[Christoph Hellwig]

On Fri, Jul 29, 2022 at 03:29:52PM +0800, Ming Lei wrote:
> The parameter passed from userspace is added to one array, and the type is
> used as index of the array. The following patch will add two parameter
> types: basic(covers basic queue setting and misc settings which can't be grouped

A bunch of overly long lines here.

But I still think this is the wrong design.  The number of potential
parameter is very limited, so splitting them over multiple ioctls
and data structure for no good reason is really a de-optimization
that makes the code more complex and slower.

Re: [PATCH V3 4/5] ublk_drv: add two parameter types

[Christoph Hellwig]

> Another change is that discard support becomes not enabled at default,
> and it needs userspace to send ublk_discard_param for enabling it.

FYI, independent of my deep diѕagreement with the model I agree
with that change.

Re: [PATCH V3 5/5] ublk_drv: cleanup ublksrv_ctrl_dev_info

[Christoph Hellwig]

On Fri, Jul 29, 2022 at 03:29:54PM +0800, Ming Lei wrote:
> Remove all block device related info from ublksrv_ctrl_dev_info,
> meantime reduce its size into 64 bytes because:
> 
> 1) ublksrv_ctrl_dev_info becomes cleaner without including any
> block related info
> 
> 2) generic set/get parameter command can be used to set block
> related setting easily and cleanly
> 
> 3) generic set/get parameter command can be used for extending
> ublk without needing more info in ublksrv_ctrl_dev_info

This should condense the structure instead of spreading random
reserveѕ.

One more reason why we should not just merge half-baked UAPIs before
a few good rounds of review :(

Re: [PATCH V3 3/5] ublk_drv: add SET_PARAM/GET_PARAM control command

[Ming Lei]

On Fri, Jul 29, 2022 at 04:22:26PM +0200, Christoph Hellwig wrote:
> On Fri, Jul 29, 2022 at 03:29:52PM +0800, Ming Lei wrote:
> > The parameter passed from userspace is added to one array, and the type is
> > used as index of the array. The following patch will add two parameter
> > types: basic(covers basic queue setting and misc settings which can't be grouped
> 
> A bunch of overly long lines here.
> 
> But I still think this is the wrong design.  The number of potential
> parameter is very limited, so splitting them over multiple ioctls

So far, at least the following 6 parameters are to be added:

- basic
- discard
- segment
- zoned
- user space crash recovery parameter
- userspace backing buffer parameter

And each one is basically not related with others.

And ublk is really one generic userspace driver framework, it is very
likely to have more parameters added in future.

> and data structure for no good reason is really a de-optimization
> that makes the code more complex and slower.

Fine, I don't have time to argue which one is better.

For the timing's reason, I switch to the single parameter way. If turns
outs single parameter can't work well enough in future, I will restart
to add set/get parameter command.


Thanks,
Ming

Re: [PATCH V3 5/5] ublk_drv: cleanup ublksrv_ctrl_dev_info

[Ming Lei]

On Fri, Jul 29, 2022 at 04:24:31PM +0200, Christoph Hellwig wrote:
> On Fri, Jul 29, 2022 at 03:29:54PM +0800, Ming Lei wrote:
> > Remove all block device related info from ublksrv_ctrl_dev_info,
> > meantime reduce its size into 64 bytes because:
> > 
> > 1) ublksrv_ctrl_dev_info becomes cleaner without including any
> > block related info
> > 
> > 2) generic set/get parameter command can be used to set block
> > related setting easily and cleanly
> > 
> > 3) generic set/get parameter command can be used for extending
> > ublk without needing more info in ublksrv_ctrl_dev_info
> 
> This should condense the structure instead of spreading random
> reserveѕ.
> 
> One more reason why we should not just merge half-baked UAPIs before
> a few good rounds of review :(

This patch does depend on set/get parameter(s) command.



Thanks,
Ming