* [PATCH for-next] null_blk: add configfs variable shared_tags
@ 2024-01-16 3:39 Shin'ichiro Kawasaki
2024-01-17 4:13 ` Chaitanya Kulkarni
0 siblings, 1 reply; 5+ messages in thread
From: Shin'ichiro Kawasaki @ 2024-01-16 3:39 UTC (permalink / raw
To: linux-block, Jens Axboe; +Cc: Shin'ichiro Kawasaki
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.
The following parameters can not be set through configfs yet:
timeout
requeue
init_hctx
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
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
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..c3361e521564 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 = false;
static struct blk_mq_tag_set tag_set;
enum {
@@ -165,8 +166,8 @@ static bool g_blocking;
module_param_named(blocking, g_blocking, bool, 0444);
MODULE_PARM_DESC(blocking, "Register as a blocking blk-mq driver device");
-static bool shared_tags;
-module_param(shared_tags, bool, 0444);
+static bool g_shared_tags;
+module_param_named(shared_tags, g_shared_tags, bool, 0444);
MODULE_PARM_DESC(shared_tags, "Share tag set between devices for blk-mq");
static bool g_shared_tag_bitmap;
@@ -426,6 +427,7 @@ NULLB_DEVICE_ATTR(zone_max_open, uint, NULL);
NULLB_DEVICE_ATTR(zone_max_active, uint, NULL);
NULLB_DEVICE_ATTR(virt_boundary, bool, NULL);
NULLB_DEVICE_ATTR(no_sched, bool, NULL);
+NULLB_DEVICE_ATTR(shared_tags, bool, NULL);
NULLB_DEVICE_ATTR(shared_tag_bitmap, bool, NULL);
static ssize_t nullb_device_power_show(struct config_item *item, char *page)
@@ -571,6 +573,7 @@ static struct configfs_attribute *nullb_device_attrs[] = {
&nullb_device_attr_zone_offline,
&nullb_device_attr_virt_boundary,
&nullb_device_attr_no_sched,
+ &nullb_device_attr_shared_tags,
&nullb_device_attr_shared_tag_bitmap,
NULL,
};
@@ -653,10 +656,11 @@ static ssize_t memb_group_features_show(struct config_item *item, char *page)
"badblocks,blocking,blocksize,cache_size,"
"completion_nsec,discard,home_node,hw_queue_depth,"
"irqmode,max_sectors,mbps,memory_backed,no_sched,"
- "poll_queues,power,queue_mode,shared_tag_bitmap,size,"
- "submit_queues,use_per_node_hctx,virt_boundary,zoned,"
- "zone_capacity,zone_max_active,zone_max_open,"
- "zone_nr_conv,zone_offline,zone_readonly,zone_size\n");
+ "poll_queues,power,queue_mode,shared_tag_bitmap,"
+ "shared_tags,size,submit_queues,use_per_node_hctx,"
+ "virt_boundary,zoned,zone_capacity,zone_max_active,"
+ "zone_max_open,zone_nr_conv,zone_offline,zone_readonly,"
+ "zone_size\n");
}
CONFIGFS_ATTR_RO(memb_group_, features);
@@ -738,6 +742,7 @@ static struct nullb_device *null_alloc_dev(void)
dev->zone_max_active = g_zone_max_active;
dev->virt_boundary = g_virt_boundary;
dev->no_sched = g_no_sched;
+ dev->shared_tags = g_shared_tags;
dev->shared_tag_bitmap = g_shared_tag_bitmap;
return dev;
}
@@ -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);
}
diff --git a/drivers/block/null_blk/null_blk.h b/drivers/block/null_blk/null_blk.h
index 929f659dd255..7bcfc0922ae8 100644
--- a/drivers/block/null_blk/null_blk.h
+++ b/drivers/block/null_blk/null_blk.h
@@ -119,6 +119,7 @@ struct nullb_device {
bool zoned; /* if device is zoned */
bool virt_boundary; /* virtual boundary on/off for the device */
bool no_sched; /* no IO scheduler for the device */
+ bool shared_tags; /* share tag set between devices for blk-mq */
bool shared_tag_bitmap; /* use hostwide shared tags */
};
--
2.43.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH for-next] null_blk: add configfs variable shared_tags
2024-01-16 3:39 [PATCH for-next] null_blk: add configfs variable shared_tags Shin'ichiro Kawasaki
@ 2024-01-17 4:13 ` Chaitanya Kulkarni
2024-01-25 6:49 ` Shinichiro Kawasaki
2024-01-25 15:46 ` Jens Axboe
0 siblings, 2 replies; 5+ messages in thread
From: Chaitanya Kulkarni @ 2024-01-17 4:13 UTC (permalink / raw
To: Shin'ichiro Kawasaki; +Cc: Jens Axboe, linux-block@vger.kernel.org
On 1/15/24 19:39, 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.
>
> The following parameters can not be set through configfs yet:
>
> timeout
> requeue
> init_hctx
>
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
> 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
>
> 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..c3361e521564 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 = false;
explicit initializing global bool to false really needed ?
unless I did something see [1].
-ck
[1]
nvme (nvme-6.8) # git diff drivers/block/null_blk/
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index 9f7695f00c2d..12686aeaae38 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -68,6 +68,8 @@ struct nullb_page {
static LIST_HEAD(nullb_list);
static struct mutex lock;
static int null_major;
+static bool abcd;
+static bool abcd_init = false;
static DEFINE_IDA(nullb_indexes);
static struct blk_mq_tag_set tag_set;
@@ -2280,6 +2282,7 @@ static int __init null_init(void)
unsigned int i;
struct nullb *nullb;
+ pr_info("%s %d abcd %d abcd_init %d\n", __func__, __LINE__,
abcd, abcd_init);
if (g_bs > PAGE_SIZE) {
pr_warn("invalid block size\n");
pr_warn("defaults block size to %lu\n", PAGE_SIZE);
nvme (nvme-6.8) # makej M=drivers/block/ clean
CLEAN drivers/block/Module.symvers
nvme (nvme-6.8) # makej M=drivers/block/
CC [M] drivers/block/floppy.o
CC [M] drivers/block/brd.o
CC [M] drivers/block/loop.o
CC [M] drivers/block/nbd.o
CC [M] drivers/block/virtio_blk.o
CC [M] drivers/block/xen-blkback/blkback.o
CC [M] drivers/block/xen-blkfront.o
CC [M] drivers/block/rbd.o
CC [M] drivers/block/xen-blkback/xenbus.o
CC [M] drivers/block/zram/zcomp.o
CC [M] drivers/block/mtip32xx/mtip32xx.o
CC [M] drivers/block/drbd/drbd_bitmap.o
CC [M] drivers/block/drbd/drbd_buildtag.o
CC [M] drivers/block/zram/zram_drv.o
CC [M] drivers/block/drbd/drbd_proc.o
CC [M] drivers/block/drbd/drbd_worker.o
CC [M] drivers/block/drbd/drbd_receiver.o
CC [M] drivers/block/null_blk/main.o
CC [M] drivers/block/drbd/drbd_req.o
CC [M] drivers/block/null_blk/trace.o
CC [M] drivers/block/drbd/drbd_actlog.o
CC [M] drivers/block/drbd/drbd_main.o
CC [M] drivers/block/null_blk/zoned.o
CC [M] drivers/block/drbd/drbd_strings.o
CC [M] drivers/block/drbd/drbd_nl.o
CC [M] drivers/block/drbd/drbd_interval.o
CC [M] drivers/block/drbd/drbd_state.o
CC [M] drivers/block/drbd/drbd_nla.o
CC [M] drivers/block/drbd/drbd_debugfs.o
LD [M] drivers/block/zram/zram.o
LD [M] drivers/block/xen-blkback/xen-blkback.o
LD [M] drivers/block/null_blk/null_blk.o
LD [M] drivers/block/drbd/drbd.o
MODPOST drivers/block/Module.symvers
CC [M] drivers/block/floppy.mod.o
CC [M] drivers/block/brd.mod.o
CC [M] drivers/block/loop.mod.o
CC [M] drivers/block/nbd.mod.o
CC [M] drivers/block/virtio_blk.mod.o
CC [M] drivers/block/xen-blkfront.mod.o
CC [M] drivers/block/xen-blkback/xen-blkback.mod.o
CC [M] drivers/block/drbd/drbd.mod.o
CC [M] drivers/block/rbd.mod.o
CC [M] drivers/block/mtip32xx/mtip32xx.mod.o
CC [M] drivers/block/zram/zram.mod.o
CC [M] drivers/block/null_blk/null_blk.mod.o
LD [M] drivers/block/brd.ko
LD [M] drivers/block/xen-blkback/xen-blkback.ko
LD [M] drivers/block/drbd/drbd.ko
LD [M] drivers/block/loop.ko
LD [M] drivers/block/floppy.ko
LD [M] drivers/block/virtio_blk.ko
LD [M] drivers/block/null_blk/null_blk.ko
LD [M] drivers/block/rbd.ko
LD [M] drivers/block/mtip32xx/mtip32xx.ko
LD [M] drivers/block/zram/zram.ko
LD [M] drivers/block/nbd.ko
LD [M] drivers/block/xen-blkfront.ko
nvme (nvme-6.8) # insmod drivers/block/null_blk/null_blk.ko
nvme (nvme-6.8) # dmesg -c
[23228.355423] null_blk: null_init 2285 abcd 0 abcd_init 0
[23228.357571] null_blk: disk nullb0 created
[23228.357574] null_blk: module loaded
nvme (nvme-6.8) #
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH for-next] null_blk: add configfs variable shared_tags
2024-01-17 4:13 ` Chaitanya Kulkarni
@ 2024-01-25 6:49 ` Shinichiro Kawasaki
2024-01-25 15:46 ` Jens Axboe
1 sibling, 0 replies; 5+ messages in thread
From: Shinichiro Kawasaki @ 2024-01-25 6:49 UTC (permalink / raw
To: Chaitanya Kulkarni; +Cc: Jens Axboe, linux-block@vger.kernel.org
On Jan 17, 2024 / 04:13, Chaitanya Kulkarni wrote:
...
> > diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> > index 36755f263e8e..c3361e521564 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 = false;
>
> explicit initializing global bool to false really needed ?
That was the point I was not clear. Some of global variables in
drivers/block/null_blk/main.c are explicitly initialized, like:
...
static bool g_virt_boundary = false;
module_param_named(virt_boundary, g_virt_boundary, bool, 0444);
MODULE_PARM_DESC(virt_boundary, "Require a virtual boundary for the device. Default: False");
...
But I guess these existing global variables are explicitly initialized because
they are used to store module parameters.
The variable I introduced is not a module parameter, so now I think it should
not be initialized. Will send v2 with the fix.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH for-next] null_blk: add configfs variable shared_tags
2024-01-17 4:13 ` Chaitanya Kulkarni
2024-01-25 6:49 ` Shinichiro Kawasaki
@ 2024-01-25 15:46 ` Jens Axboe
2024-01-26 6:34 ` Chaitanya Kulkarni
1 sibling, 1 reply; 5+ messages in thread
From: Jens Axboe @ 2024-01-25 15:46 UTC (permalink / raw
To: Chaitanya Kulkarni, Shin'ichiro Kawasaki; +Cc: linux-block@vger.kernel.org
On 1/16/24 9:13 PM, Chaitanya Kulkarni wrote:
> On 1/15/24 19:39, 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.
>>
>> The following parameters can not be set through configfs yet:
>>
>> timeout
>> requeue
>> init_hctx
>>
>> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
>> ---
>> 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
>>
>> 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..c3361e521564 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 = false;
>
> explicit initializing global bool to false really needed ?
> unless I did something see [1].
>
> -ck
>
> nvme (nvme-6.8) # insmod drivers/block/null_blk/null_blk.ko
> nvme (nvme-6.8) # dmesg -c
> [23228.355423] null_blk: null_init 2285 abcd 0 abcd_init 0
> [23228.357571] null_blk: disk nullb0 created
> [23228.357574] null_blk: module loaded
Just as a heads-up, a single test is not proof of anything. That said,
static variables are cleared, so they never need to be setup to false.
--
Jens Axboe
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH for-next] null_blk: add configfs variable shared_tags
2024-01-25 15:46 ` Jens Axboe
@ 2024-01-26 6:34 ` Chaitanya Kulkarni
0 siblings, 0 replies; 5+ messages in thread
From: Chaitanya Kulkarni @ 2024-01-26 6:34 UTC (permalink / raw
To: Jens Axboe; +Cc: Shin'ichiro Kawasaki, linux-block@vger.kernel.org
>> explicit initializing global bool to false really needed ?
>> unless I did something see [1].
>>
>> -ck
>>
>> nvme (nvme-6.8) # insmod drivers/block/null_blk/null_blk.ko
>> nvme (nvme-6.8) # dmesg -c
>> [23228.355423] null_blk: null_init 2285 abcd 0 abcd_init 0
>> [23228.357571] null_blk: disk nullb0 created
>> [23228.357574] null_blk: module loaded
> Just as a heads-up, a single test is not proof of anything. That said,
> static variables are cleared, so they never need to be setup to false.
>
thanks for the suggestion, will provide detailed explanation for the test
to justify the overall impact from next time as I did it last time for
similar issue ...
-ck
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-01-26 6:34 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-16 3:39 [PATCH for-next] null_blk: add configfs variable shared_tags Shin'ichiro Kawasaki
2024-01-17 4:13 ` Chaitanya Kulkarni
2024-01-25 6:49 ` Shinichiro Kawasaki
2024-01-25 15:46 ` Jens Axboe
2024-01-26 6:34 ` Chaitanya Kulkarni
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).