| Message ID | 20220727141628.985429-4-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ublk_drv: add generic mechanism to get/set parameters | expand |
As stated in the previous discussion I think this is a very bad idea that leads to a lot of boiler plate code. If you don't want to rely on zeroed fields we can add a mask of valid fields, similar to how e.g. the statx API works.
Hi Christoph, On Wed, Jul 27, 2022 at 06:22:34PM +0200, Christoph Hellwig wrote: > As stated in the previous discussion I think this is a very bad idea > that leads to a lot of boiler plate code. If you don't want to rely But you never point out where the boiler plate code is, care to share what/where it is? If you don't like xarray, we can replace it with one plain array from the beginning, this change is just in implementation level. Given it is ABI interface, I'd suggest to consolidate it from the beginning. > on zeroed fields we can add a mask of valid fields, similar to how > e.g. the statx API works. With one mask for marking valid fields, we still need to group fields, and one bit in mask has to represent one single group, and it can't represent each single field. Also one length field has to be added in the header for keeping compatibility with old app. With the above two change, it becomes very similar with the approach in this patch. IMO there are more advantages in grouping parameter explicitly: 1) avoid big chunk of memory for storing lots of unnecessary parameter if the big params data structure is extended, and we just store whatever the userspace cares. 2) easy to add new type by just implementing two callbacks(both optionally), and code is actually well organized, and each callback just focuses on the interested parameter type 3) parameter is easier to verify, since we know each parameter's length and type. With mask, we can only know if one parameter type exists in the big parameters data structure or not. Thanks, Ming
On Thu, Jul 28, 2022 at 09:56:26AM +0800, Ming Lei wrote: > Hi Christoph, > > On Wed, Jul 27, 2022 at 06:22:34PM +0200, Christoph Hellwig wrote: > > As stated in the previous discussion I think this is a very bad idea > > that leads to a lot of boiler plate code. If you don't want to rely > > But you never point out where the boiler plate code is, care to share > what/where it is? > > If you don't like xarray, we can replace it with one plain array > from the beginning, this change is just in implementation level. > > Given it is ABI interface, I'd suggest to consolidate it from the > beginning. > > > on zeroed fields we can add a mask of valid fields, similar to how > > e.g. the statx API works. > > With one mask for marking valid fields, we still need to group > fields, and one bit in mask has to represent one single group, and > it can't represent each single field. > > Also one length field has to be added in the header for keeping > compatibility with old app. > > With the above two change, it becomes very similar with the approach > in this patch. > > IMO there are more advantages in grouping parameter explicitly: > > 1) avoid big chunk of memory for storing lots of unnecessary parameter > if the big params data structure is extended, and we just store whatever > the userspace cares. > > 2) easy to add new type by just implementing two callbacks(both optionally), > and code is actually well organized, and each callback just focuses on the > interested parameter type > > 3) parameter is easier to verify, since we know each parameter's length and type. > With mask, we can only know if one parameter type exists in the big > parameters data structure or not. Another thing with putting everything into single structure is that the params structure will become a mess sooner or later, since all kinds of parameters will be added at the end, none of them are related. For example, Ziyang is working on userspace recovery feature which may need some data which can't be hold in single bit flag, meantime someone may work on zoned target implementation[1] which needs to send zoned parameter, and I am planning to add backing pages for handling running out of pages, which needs userspace to pre-allocate/send buffer to driver as backing page[s], ... Then all these unrelated parameters are added at the end, and the single structure becomes fatter and fatter, then become unreadable and hard to maintain. [1] https://lore.kernel.org/qemu-devel/fa7de750-8def-c532-8c86-64c7505608e0@opensource.wdc.com/T/#u Thanks, Ming
diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index cb880308a378..39bb2d943dc2 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 xarray params; + struct completion completion; unsigned int nr_queues_ready; atomic_t nr_aborted_queues; @@ -160,6 +162,18 @@ 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) +{ + return -EINVAL; +} + +static int ublk_install_param(struct ublk_device *ub, + struct ublk_param_header *h) +{ + return -EINVAL; +} + static inline bool ublk_can_use_task_work(const struct ublk_queue *ubq) { if (IS_BUILTIN(CONFIG_BLK_DEV_UBLK) && @@ -1055,6 +1069,7 @@ static void ublk_cdev_rel(struct device *dev) ublk_deinit_queues(ub); ublk_free_dev_number(ub); mutex_destroy(&ub->mutex); + xa_destroy(&ub->params); kfree(ub); } @@ -1300,6 +1315,7 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd) spin_lock_init(&ub->mm_lock); INIT_WORK(&ub->stop_work, ublk_stop_work_fn); INIT_DELAYED_WORK(&ub->monitor_work, ublk_daemon_monitor_work); + xa_init(&ub->params); ret = ublk_alloc_dev_number(ub, header->dev_id); if (ret < 0) @@ -1445,6 +1461,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 = xa_load(&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) { @@ -1480,6 +1577,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..1fb56d35ba8a 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,12 @@ struct ublksrv_io_cmd { __u64 addr; }; +/* 512 is big enough for single parameter */ +#define UBLK_MAX_PARAM_LEN 512 + +struct ublk_param_header { + __u16 type; + __u16 len; +}; + #endif
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 xarray because most of parameters are optional, and their type is used as key of xarray, meantime in future there may be lots of parameters. 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. Signed-off-by: Ming Lei <ming.lei@redhat.com> --- drivers/block/ublk_drv.c | 103 ++++++++++++++++++++++++++++++++++ include/uapi/linux/ublk_cmd.h | 10 ++++ 2 files changed, 113 insertions(+)