On Jan 26, 2024 / 06:33, Chaitanya Kulkarni wrote:
> On 1/25/24 02:14, Shin'ichiro Kawasaki wrote:
> > Allow setting shared_tags through configfs, which could only be set as a
> > module parameter. For that purpose, delay tag_set initialization from
> > null_init() to null_add_dev(). Introduce the flag tag_set_initialized to
> > manage the initialization status of tag_set.
>
> we probably don't need the tag_set_initialized see below ..
>
> > The following parameters can not be set through configfs yet:
> >
> > timeout
> > requeue
> > init_hctx
>
> I've not seen something like this in the commit log, but if everyone is okay
> sure ...
The commit 7012eef520cb ("null_blk: add configfs variables for 2 options") has
similar log. I tried to make the commit logs consistent.
>
> > Signed-off-by: Shin'ichiro Kawasaki<shinichiro.kawasaki@xxxxxxx>
> > ---
> > This patch will allow running the blktests test cases block/010 and block/022
> > using the built-in null_blk driver. Corresponding blktests side changes are
> > drafted here [1].
> >
> > [1]https://github.com/kawasaki/blktests/tree/shared_tags
> >
> > Changes from v1:
> > * Removed unnecessary global variable initializer
> >
> > drivers/block/null_blk/main.c | 38 ++++++++++++++++---------------
> > drivers/block/null_blk/null_blk.h | 1 +
> > 2 files changed, 21 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> > index 36755f263e8e..1407d4e3452a 100644
> > --- a/drivers/block/null_blk/main.c
> > +++ b/drivers/block/null_blk/main.c
> > @@ -69,6 +69,7 @@ static LIST_HEAD(nullb_list);
> > static struct mutex lock;
> > static int null_major;
> > static DEFINE_IDA(nullb_indexes);
> > +static bool tag_set_initialized;
> > static struct blk_mq_tag_set tag_set;
> >
> >
>
> [...]
>
> > @@ -2124,7 +2129,13 @@ static int null_add_dev(struct nullb_device *dev)
> > goto out_free_nullb;
> >
> > if (dev->queue_mode == NULL_Q_MQ) {
> > - if (shared_tags) {
> > + if (dev->shared_tags) {
> > + if (!tag_set_initialized) {
> > + rv = null_init_tag_set(NULL, &tag_set);
> > + if (rv)
> > + goto out_cleanup_queues;
> > + tag_set_initialized = true;
> > + }
> > nullb->tag_set = &tag_set;
> > rv = 0;
> > } else {
> > @@ -2311,18 +2322,12 @@ static int __init null_init(void)
> > g_submit_queues = 1;
> > }
> >
> > - if (g_queue_mode == NULL_Q_MQ && shared_tags) {
> > - ret = null_init_tag_set(NULL, &tag_set);
> > - if (ret)
> > - return ret;
> > - }
> > -
> > config_group_init(&nullb_subsys.su_group);
> > mutex_init(&nullb_subsys.su_mutex);
> >
> > ret = configfs_register_subsystem(&nullb_subsys);
> > if (ret)
> > - goto err_tagset;
> > + return ret;
> >
> > mutex_init(&lock);
> >
> > @@ -2349,9 +2354,6 @@ static int __init null_init(void)
> > unregister_blkdev(null_major, "nullb");
> > err_conf:
> > configfs_unregister_subsystem(&nullb_subsys);
> > -err_tagset:
> > - if (g_queue_mode == NULL_Q_MQ && shared_tags)
> > - blk_mq_free_tag_set(&tag_set);
> > return ret;
> > }
> >
> > @@ -2370,7 +2372,7 @@ static void __exit null_exit(void)
> > }
> > mutex_unlock(&lock);
> >
> > - if (g_queue_mode == NULL_Q_MQ && shared_tags)
> > + if (tag_set_initialized)
> > blk_mq_free_tag_set(&tag_set);
> > }
> >
> >
>
> The global variable tag_set_initialized is used to indicate if global
> variable
> struct blk_mq_tag_set tag_set is initialized or not, it only allow
> tag_set to be
> initialized once when dev->shared_tags == true first time and in
> null_exit() we
> can call blk_mq_free_tag_set(&tag_set).
>
> One way to remove tag_set_initialized is to replace tag_set_initialized with
> global variable tag_set.ops with NULL check.
>
> This might work since in null_add_dev() when dev->shared_tags == true
> for the
> first time tags_set.ops will be NULL because it is only initialized in
> null_init_tag_set() and for subsequent calls to null_add_dev() tag_set.ops
> will not be NULL, ensuring only one call to null_init_tag_set() with global
> variable tag_set.
>
> Unless there is any objection on using tag_set.ops, how about following
> (totally untested) on the top of yours, that removes tag_set_initialized ?
Thank you for this idea. I think it is the simpler and the better.
>
> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> index 1407d4e3452a..3d69c7b9fa7f 100644
> --- a/drivers/block/null_blk/main.c
> +++ b/drivers/block/null_blk/main.c
> @@ -69,7 +69,6 @@ static LIST_HEAD(nullb_list);
> static struct mutex lock;
> static int null_major;
> static DEFINE_IDA(nullb_indexes);
> -static bool tag_set_initialized;
> static struct blk_mq_tag_set tag_set;
>
> enum {
> @@ -2130,11 +2129,10 @@ static int null_add_dev(struct nullb_device *dev)
>
> if (dev->queue_mode == NULL_Q_MQ) {
> if (dev->shared_tags) {
> - if (!tag_set_initialized) {
> + if (!tag_set.ops) {
> rv = null_init_tag_set(NULL, &tag_set);
> if (rv)
> goto out_cleanup_queues;
> - tag_set_initialized = true;
> }
> nullb->tag_set = &tag_set;
> rv = 0;
I think NULL should be set to tag_set.opts in case null_init_tag_set() failed.
So I think the hunk above will be as follows.
@@ -2124,7 +2128,14 @@ static int null_add_dev(struct nullb_device *dev)
goto out_free_nullb;
if (dev->queue_mode == NULL_Q_MQ) {
- if (shared_tags) {
+ if (dev->shared_tags) {
+ if (!tag_set.ops) {
+ rv = null_init_tag_set(NULL, &tag_set);
+ if (rv) {
+ tag_set.ops = NULL;
+ goto out_cleanup_queues;
+ }
+ }
nullb->tag_set = &tag_set;
rv = 0;
} else {
> @@ -2372,7 +2370,7 @@ static void __exit null_exit(void)
> }
> mutex_unlock(&lock);
>
> - if (tag_set_initialized)
> + if (tags_set.ops)
> blk_mq_free_tag_set(&tag_set);
> }
>
> any thoughts ?
If there is no other comment, I will revise the patch with your idea and post v3
next week.
>
> I've tested your original patch with blktests:block/010, it looks good
> to me ...
Thanks!
