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 ... > 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 ? 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; @@ -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 ? I've tested your original patch with blktests:block/010, it looks good to me ... -ck
