* [PATCH 0/2] ublk_drv: make sure that correct flags(features) returned to userspace @ 2022-07-22 2:36 Ming Lei 2022-07-22 2:36 ` [PATCH 1/2] ublk_drv: move destroying device out of ublk_add_dev Ming Lei 2022-07-22 2:36 ` [PATCH 2/2] ublk_drv: make sure that correct flags(features) returned to userspace Ming Lei 0 siblings, 2 replies; 6+ messages in thread From: Ming Lei @ 2022-07-22 2:36 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-block, Ming Lei Hello Jens, The 1st patch cleans ublk_add_dev a bit, meantime fix one potential mutex_destroy() issue. The 2nd one makes sure that driver supported flags returned to userspace, this way is important for maintaining compatibility. Ming Lei (2): ublk_drv: move destroying device out of ublk_add_dev ublk_drv: make sure that correct flags(features) returned to userspace drivers/block/ublk_drv.c | 32 ++++++++++++++++++++++++-------- include/uapi/linux/ublk_cmd.h | 11 +++++++++-- 2 files changed, 33 insertions(+), 10 deletions(-) -- 2.31.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] ublk_drv: move destroying device out of ublk_add_dev 2022-07-22 2:36 [PATCH 0/2] ublk_drv: make sure that correct flags(features) returned to userspace Ming Lei @ 2022-07-22 2:36 ` Ming Lei 2022-07-22 2:58 ` Ziyang Zhang 2022-07-22 2:36 ` [PATCH 2/2] ublk_drv: make sure that correct flags(features) returned to userspace Ming Lei 1 sibling, 1 reply; 6+ messages in thread From: Ming Lei @ 2022-07-22 2:36 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-block, Ming Lei ublk_device is allocated in ublk_ctrl_add_dev(), so code will become more readable by just letting ublk_ctrl_add_dev() destroy ublk_device in case of ublk_add_dev() failure. Meantime ub->mutex is destroyed in __ublk_destroy_dev(), but it may not be initialized when ublk_add_dev() fails, so fix it by moving mutex_init(ub->mutex) before any failure path. Signed-off-by: Ming Lei <ming.lei@redhat.com> --- drivers/block/ublk_drv.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index f058f40b639c..d03563286c76 100644 --- a/drivers/block/ublk_drv.c +++ b/drivers/block/ublk_drv.c @@ -1106,9 +1106,10 @@ static int ublk_add_dev(struct ublk_device *ub) INIT_WORK(&ub->stop_work, ublk_stop_work_fn); INIT_DELAYED_WORK(&ub->monitor_work, ublk_daemon_monitor_work); + mutex_init(&ub->mutex); if (ublk_init_queues(ub)) - goto out_destroy_dev; + return err; ub->tag_set.ops = &ublk_mq_ops; ub->tag_set.nr_hw_queues = ub->dev_info.nr_hw_queues; @@ -1122,7 +1123,6 @@ static int ublk_add_dev(struct ublk_device *ub) goto out_deinit_queues; ublk_align_max_io_size(ub); - mutex_init(&ub->mutex); spin_lock_init(&ub->mm_lock); /* add char dev so that ublksrv daemon can be setup */ @@ -1130,8 +1130,6 @@ static int ublk_add_dev(struct ublk_device *ub) out_deinit_queues: ublk_deinit_queues(ub); -out_destroy_dev: - __ublk_destroy_dev(ub); return err; } @@ -1331,8 +1329,10 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd) ub->dev_info.dev_id = ub->ub_number; ret = ublk_add_dev(ub); - if (ret) + if (ret) { + __ublk_destroy_dev(ub); goto out_unlock; + } if (copy_to_user(argp, &ub->dev_info, sizeof(info))) { ublk_remove(ub); -- 2.31.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] ublk_drv: move destroying device out of ublk_add_dev 2022-07-22 2:36 ` [PATCH 1/2] ublk_drv: move destroying device out of ublk_add_dev Ming Lei @ 2022-07-22 2:58 ` Ziyang Zhang 2022-07-22 3:28 ` Ming Lei 0 siblings, 1 reply; 6+ messages in thread From: Ziyang Zhang @ 2022-07-22 2:58 UTC (permalink / raw) To: Ming Lei; +Cc: linux-block, Jens Axboe On 2022/7/22 10:36, Ming Lei wrote: > ublk_device is allocated in ublk_ctrl_add_dev(), so code will become more > readable by just letting ublk_ctrl_add_dev() destroy ublk_device in case > of ublk_add_dev() failure. > > Meantime ub->mutex is destroyed in __ublk_destroy_dev(), but it may > not be initialized when ublk_add_dev() fails, so fix it by moving > mutex_init(ub->mutex) before any failure path. > > Signed-off-by: Ming Lei <ming.lei@redhat.com> > --- > drivers/block/ublk_drv.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c > index f058f40b639c..d03563286c76 100644 > --- a/drivers/block/ublk_drv.c > +++ b/drivers/block/ublk_drv.c > @@ -1106,9 +1106,10 @@ static int ublk_add_dev(struct ublk_device *ub) > > INIT_WORK(&ub->stop_work, ublk_stop_work_fn); > INIT_DELAYED_WORK(&ub->monitor_work, ublk_daemon_monitor_work); > + mutex_init(&ub->mutex); > > if (ublk_init_queues(ub)) > - goto out_destroy_dev; > + return err; > > ub->tag_set.ops = &ublk_mq_ops; > ub->tag_set.nr_hw_queues = ub->dev_info.nr_hw_queues; > @@ -1122,7 +1123,6 @@ static int ublk_add_dev(struct ublk_device *ub) > goto out_deinit_queues; > > ublk_align_max_io_size(ub); > - mutex_init(&ub->mutex); > spin_lock_init(&ub->mm_lock); > > /* add char dev so that ublksrv daemon can be setup */ > @@ -1130,8 +1130,6 @@ static int ublk_add_dev(struct ublk_device *ub) > > out_deinit_queues: > ublk_deinit_queues(ub); > -out_destroy_dev: > - __ublk_destroy_dev(ub); > return err; > } > > @@ -1331,8 +1329,10 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd) > ub->dev_info.dev_id = ub->ub_number; > > ret = ublk_add_dev(ub); > - if (ret) > + if (ret) { > + __ublk_destroy_dev(ub); > goto out_unlock; > + } Hi, Ming. Now, if ublk_add_dev() returns failure, __ublk_destroy_dev() is called anyway. However, in current ublk_drv:ublk_add_dev(): ... return ublk_add_chdev(ub); <---- here out_deinit_queues: ublk_deinit_queues(ub); out_destroy_dev: __ublk_destroy_dev(ub); return err; ublk_add_chdev() returns and the returned value(maybe a failure) directly pass to ublk_ctrl_add_dev which does NOT call __ublk_destroy_dev() please check it is correct to call __ublk_destroy_dev() if ublk_add_chdev() fails. > > if (copy_to_user(argp, &ub->dev_info, sizeof(info))) { > ublk_remove(ub); ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] ublk_drv: move destroying device out of ublk_add_dev 2022-07-22 2:58 ` Ziyang Zhang @ 2022-07-22 3:28 ` Ming Lei 0 siblings, 0 replies; 6+ messages in thread From: Ming Lei @ 2022-07-22 3:28 UTC (permalink / raw) To: Ziyang Zhang; +Cc: linux-block, Jens Axboe, ming.lei On Fri, Jul 22, 2022 at 10:58:59AM +0800, Ziyang Zhang wrote: > On 2022/7/22 10:36, Ming Lei wrote: > > ublk_device is allocated in ublk_ctrl_add_dev(), so code will become more > > readable by just letting ublk_ctrl_add_dev() destroy ublk_device in case > > of ublk_add_dev() failure. > > > > Meantime ub->mutex is destroyed in __ublk_destroy_dev(), but it may > > not be initialized when ublk_add_dev() fails, so fix it by moving > > mutex_init(ub->mutex) before any failure path. > > > > Signed-off-by: Ming Lei <ming.lei@redhat.com> > > --- > > drivers/block/ublk_drv.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c > > index f058f40b639c..d03563286c76 100644 > > --- a/drivers/block/ublk_drv.c > > +++ b/drivers/block/ublk_drv.c > > @@ -1106,9 +1106,10 @@ static int ublk_add_dev(struct ublk_device *ub) > > > > INIT_WORK(&ub->stop_work, ublk_stop_work_fn); > > INIT_DELAYED_WORK(&ub->monitor_work, ublk_daemon_monitor_work); > > + mutex_init(&ub->mutex); > > > > if (ublk_init_queues(ub)) > > - goto out_destroy_dev; > > + return err; > > > > ub->tag_set.ops = &ublk_mq_ops; > > ub->tag_set.nr_hw_queues = ub->dev_info.nr_hw_queues; > > @@ -1122,7 +1123,6 @@ static int ublk_add_dev(struct ublk_device *ub) > > goto out_deinit_queues; > > > > ublk_align_max_io_size(ub); > > - mutex_init(&ub->mutex); > > spin_lock_init(&ub->mm_lock); > > > > /* add char dev so that ublksrv daemon can be setup */ > > @@ -1130,8 +1130,6 @@ static int ublk_add_dev(struct ublk_device *ub) > > > > out_deinit_queues: > > ublk_deinit_queues(ub); > > -out_destroy_dev: > > - __ublk_destroy_dev(ub); > > return err; > > } > > > > @@ -1331,8 +1329,10 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd) > > ub->dev_info.dev_id = ub->ub_number; > > > > ret = ublk_add_dev(ub); > > - if (ret) > > + if (ret) { > > + __ublk_destroy_dev(ub); > > goto out_unlock; > > + } > > Hi, Ming. > > Now, if ublk_add_dev() returns failure, __ublk_destroy_dev() is called anyway. > > However, in current ublk_drv:ublk_add_dev(): > > ... > return ublk_add_chdev(ub); <---- here > out_deinit_queues: > ublk_deinit_queues(ub); > out_destroy_dev: > __ublk_destroy_dev(ub); > return err; > > > ublk_add_chdev() returns and the returned value(maybe a failure) directly > pass to ublk_ctrl_add_dev which does NOT call __ublk_destroy_dev() > > please check it is correct to call __ublk_destroy_dev() if ublk_add_chdev() fails. If ublk_add_chdev fails, we shouldn't call __ublk_destroy_dev() any more, since ublk_add_chdev() does handle the cleanup, so this patch is wrong. will fix it in V2. Thanks, Ming ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] ublk_drv: make sure that correct flags(features) returned to userspace 2022-07-22 2:36 [PATCH 0/2] ublk_drv: make sure that correct flags(features) returned to userspace Ming Lei 2022-07-22 2:36 ` [PATCH 1/2] ublk_drv: move destroying device out of ublk_add_dev Ming Lei @ 2022-07-22 2:36 ` Ming Lei 2022-07-22 3:04 ` Ziyang Zhang 1 sibling, 1 reply; 6+ messages in thread From: Ming Lei @ 2022-07-22 2:36 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-block, Ming Lei Userspace may support more features or new added flags, but the driver side can be old, so make sure correct flags(features) returned to userpsace, then userspace can work as expected. Signed-off-by: Ming Lei <ming.lei@redhat.com> --- drivers/block/ublk_drv.c | 22 +++++++++++++++++++--- include/uapi/linux/ublk_cmd.h | 11 +++++++++-- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index d03563286c76..778d4c63a985 100644 --- a/drivers/block/ublk_drv.c +++ b/drivers/block/ublk_drv.c @@ -1092,14 +1092,28 @@ static void ublk_align_max_io_size(struct ublk_device *ub) round_down(max_rq_bytes, PAGE_SIZE) >> ub->bs_shift; } -/* add tag_set & cdev, cleanup everything in case of failure */ -static int ublk_add_dev(struct ublk_device *ub) +static void ublk_negotiate_features(struct ublk_device *ub) { - int err = -ENOMEM; + unsigned long *map = (unsigned long *)&ub->dev_info.flags[0]; /* We are not ready to support zero copy */ ub->dev_info.flags[0] &= ~UBLK_F_SUPPORT_ZERO_COPY; + /* + * 128bit flags will be copied back to userspace as feature + * negotiation result, so have to clear flags which driver + * doesn't support yet, then userspace can get correct flags + * (features) to handle. + */ + bitmap_clear(map, __UBLK_F_NR_BITS, 128 - __UBLK_F_NR_BITS); +} + +/* add tag_set & cdev, cleanup everything in case of failure */ +static int ublk_add_dev(struct ublk_device *ub) +{ + int err = -ENOMEM; + + ublk_negotiate_features(ub); 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); @@ -1491,6 +1505,8 @@ static int __init ublk_init(void) { int ret; + BUILD_BUG_ON(__UBLK_F_NR_BITS > 128); + 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 917580b34198..49e4950a9181 100644 --- a/include/uapi/linux/ublk_cmd.h +++ b/include/uapi/linux/ublk_cmd.h @@ -42,17 +42,24 @@ /* tag bit is 12bit, so at most 4096 IOs for each queue */ #define UBLK_MAX_QUEUE_DEPTH 4096 + +enum ublk_flag_bits { + __UBLK_F_SUPPORT_ZERO_COPY, + __UBLK_F_URING_CMD_COMP_IN_TASK, + __UBLK_F_NR_BITS, +}; + /* * zero copy requires 4k block size, and can remap ublk driver's io * request into ublksrv's vm space */ -#define UBLK_F_SUPPORT_ZERO_COPY (1UL << 0) +#define UBLK_F_SUPPORT_ZERO_COPY (1ULL << __UBLK_F_SUPPORT_ZERO_COPY) /* * Force to complete io cmd via io_uring_cmd_complete_in_task so that * performance comparison is done easily with using task_work_add */ -#define UBLK_F_URING_CMD_COMP_IN_TASK (1UL << 1) +#define UBLK_F_URING_CMD_COMP_IN_TASK (1ULL << __UBLK_F_URING_CMD_COMP_IN_TASK) /* device state */ #define UBLK_S_DEV_DEAD 0 -- 2.31.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ublk_drv: make sure that correct flags(features) returned to userspace 2022-07-22 2:36 ` [PATCH 2/2] ublk_drv: make sure that correct flags(features) returned to userspace Ming Lei @ 2022-07-22 3:04 ` Ziyang Zhang 0 siblings, 0 replies; 6+ messages in thread From: Ziyang Zhang @ 2022-07-22 3:04 UTC (permalink / raw) To: Ming Lei, Jens Axboe; +Cc: linux-block On 2022/7/22 10:36, Ming Lei wrote: > Userspace may support more features or new added flags, but the driver > side can be old, so make sure correct flags(features) returned to > userpsace, then userspace can work as expected. > > Signed-off-by: Ming Lei <ming.lei@redhat.com> > --- Reviewed-by: ZiyangZhang <ZiyangZhang@linux.alibaba.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-07-22 3:28 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-07-22 2:36 [PATCH 0/2] ublk_drv: make sure that correct flags(features) returned to userspace Ming Lei 2022-07-22 2:36 ` [PATCH 1/2] ublk_drv: move destroying device out of ublk_add_dev Ming Lei 2022-07-22 2:58 ` Ziyang Zhang 2022-07-22 3:28 ` Ming Lei 2022-07-22 2:36 ` [PATCH 2/2] ublk_drv: make sure that correct flags(features) returned to userspace Ming Lei 2022-07-22 3:04 ` Ziyang Zhang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox