diff mbox series

[RFC,V2,5/6] blk-ioprio: make ioprio pluggable and modular

Message ID 20220215123705.58968-6-jianchao.wan9@gmail.com (mailing list archive)
State New, archived
Headers show
Series blk: make blk-rq-qos policies pluggable and modular | expand

Commit Message

Wang Jianchao Feb. 15, 2022, 12:37 p.m. UTC 
Make blk-ioprio pluggable and modular. Then we can close or open
it through /sys/block/xxx/queue/qos and rmmod the module if we don't
need it which can release one blkcg policy slot.

Signed-off-by: Wang Jianchao (Kuaishou) <jianchao.wan9@gmail.com>
---
 block/Kconfig          |  2 +-
 block/Makefile         |  3 ++-
 block/blk-cgroup.c     |  5 -----
 block/blk-ioprio.c     | 50 ++++++++++++++++++++++++++++--------------
 block/blk-ioprio.h     | 19 ----------------
 block/blk-mq-debugfs.c |  4 ----
 block/blk-rq-qos.c     |  2 +-
 block/blk-rq-qos.h     |  2 +-
 8 files changed, 38 insertions(+), 49 deletions(-)
 delete mode 100644 block/blk-ioprio.h

Comments

Bart Van Assche Feb. 15, 2022, 9:26 p.m. UTC | #1 
On 2/15/22 04:37, Wang Jianchao (Kuaishou) wrote:
> diff --git a/block/Makefile b/block/Makefile
> index f38eaa612929..f6a3995af285 100644
> --- a/block/Makefile
> +++ b/block/Makefile
> @@ -17,7 +17,8 @@ obj-$(CONFIG_BLK_DEV_BSGLIB)	+= bsg-lib.o
>   obj-$(CONFIG_BLK_CGROUP)	+= blk-cgroup.o
>   obj-$(CONFIG_BLK_CGROUP_RWSTAT)	+= blk-cgroup-rwstat.o
>   obj-$(CONFIG_BLK_DEV_THROTTLING)	+= blk-throttle.o
> -obj-$(CONFIG_BLK_CGROUP_IOPRIO)	+= blk-ioprio.o
> +io-prio-y 			:= blk-ioprio.o
> +obj-$(CONFIG_BLK_CGROUP_IOPRIO)	+= io-prio.o
>   obj-$(CONFIG_BLK_CGROUP_IOLATENCY)	+= blk-iolatency.o
>   obj-$(CONFIG_BLK_CGROUP_IOCOST)	+= blk-iocost.o
>   obj-$(CONFIG_MQ_IOSCHED_DEADLINE)	+= mq-deadline.o

Is the above change really necessary?

> +static int blk_ioprio_init(struct request_queue *q);
>   static struct rq_qos_ops blkcg_ioprio_ops = {

Please insert a blank line between a function declaration and a 
structure definition.

Thanks,

Bart.
Wang Jianchao Feb. 16, 2022, 2:09 a.m. UTC | #2 
On 2022/2/16 5:26 上午, Bart Van Assche wrote:
> On 2/15/22 04:37, Wang Jianchao (Kuaishou) wrote:
>> diff --git a/block/Makefile b/block/Makefile
>> index f38eaa612929..f6a3995af285 100644
>> --- a/block/Makefile
>> +++ b/block/Makefile
>> @@ -17,7 +17,8 @@ obj-$(CONFIG_BLK_DEV_BSGLIB)    += bsg-lib.o
>>   obj-$(CONFIG_BLK_CGROUP)    += blk-cgroup.o
>>   obj-$(CONFIG_BLK_CGROUP_RWSTAT)    += blk-cgroup-rwstat.o
>>   obj-$(CONFIG_BLK_DEV_THROTTLING)    += blk-throttle.o
>> -obj-$(CONFIG_BLK_CGROUP_IOPRIO)    += blk-ioprio.o
>> +io-prio-y             := blk-ioprio.o
>> +obj-$(CONFIG_BLK_CGROUP_IOPRIO)    += io-prio.o
>>   obj-$(CONFIG_BLK_CGROUP_IOLATENCY)    += blk-iolatency.o
>>   obj-$(CONFIG_BLK_CGROUP_IOCOST)    += blk-iocost.o
>>   obj-$(CONFIG_MQ_IOSCHED_DEADLINE)    += mq-deadline.o
> 
> Is the above change really necessary?

Except for making maintaining easier on a running system, removing a
rqos policy module with cgroup supporting can release a blk-cgroup
policy slots. As BLKCG_MAX_POLS, the max slots number is fixed now.

> 
>> +static int blk_ioprio_init(struct request_queue *q);
>>   static struct rq_qos_ops blkcg_ioprio_ops = {
> 
> Please insert a blank line between a function declaration and a structure definition.

Yes, I will do it in next version.

Thanks
Jianchao
Bart Van Assche Feb. 16, 2022, 6:01 p.m. UTC | #3 
On 2/15/22 18:09, Wang Jianchao wrote:
> On 2022/2/16 5:26 上午, Bart Van Assche wrote:
>> On 2/15/22 04:37, Wang Jianchao (Kuaishou) wrote:
>>> diff --git a/block/Makefile b/block/Makefile
>>> index f38eaa612929..f6a3995af285 100644
>>> --- a/block/Makefile
>>> +++ b/block/Makefile
>>> @@ -17,7 +17,8 @@ obj-$(CONFIG_BLK_DEV_BSGLIB)    += bsg-lib.o
>>>    obj-$(CONFIG_BLK_CGROUP)    += blk-cgroup.o
>>>    obj-$(CONFIG_BLK_CGROUP_RWSTAT)    += blk-cgroup-rwstat.o
>>>    obj-$(CONFIG_BLK_DEV_THROTTLING)    += blk-throttle.o
>>> -obj-$(CONFIG_BLK_CGROUP_IOPRIO)    += blk-ioprio.o
>>> +io-prio-y             := blk-ioprio.o
>>> +obj-$(CONFIG_BLK_CGROUP_IOPRIO)    += io-prio.o
>>>    obj-$(CONFIG_BLK_CGROUP_IOLATENCY)    += blk-iolatency.o
>>>    obj-$(CONFIG_BLK_CGROUP_IOCOST)    += blk-iocost.o
>>>    obj-$(CONFIG_MQ_IOSCHED_DEADLINE)    += mq-deadline.o
>>
>> Is the above change really necessary?
> 
> Except for making maintaining easier on a running system, removing a
> rqos policy module with cgroup supporting can release a blk-cgroup
> policy slots. As BLKCG_MAX_POLS, the max slots number is fixed now.

It seems like my question was not clear? What I meant is that I think 
that the above changes are not necessary to build blk-ioprio as a kernel 
module.

Thanks,

Bart.
Wang Jianchao Feb. 17, 2022, 2:56 a.m. UTC | #4 
On 2022/2/17 2:01 上午, Bart Van Assche wrote:
> On 2/15/22 18:09, Wang Jianchao wrote:
>> On 2022/2/16 5:26 上午, Bart Van Assche wrote:
>>> On 2/15/22 04:37, Wang Jianchao (Kuaishou) wrote:
>>>> diff --git a/block/Makefile b/block/Makefile
>>>> index f38eaa612929..f6a3995af285 100644
>>>> --- a/block/Makefile
>>>> +++ b/block/Makefile
>>>> @@ -17,7 +17,8 @@ obj-$(CONFIG_BLK_DEV_BSGLIB)    += bsg-lib.o
>>>>    obj-$(CONFIG_BLK_CGROUP)    += blk-cgroup.o
>>>>    obj-$(CONFIG_BLK_CGROUP_RWSTAT)    += blk-cgroup-rwstat.o
>>>>    obj-$(CONFIG_BLK_DEV_THROTTLING)    += blk-throttle.o
>>>> -obj-$(CONFIG_BLK_CGROUP_IOPRIO)    += blk-ioprio.o
>>>> +io-prio-y             := blk-ioprio.o
>>>> +obj-$(CONFIG_BLK_CGROUP_IOPRIO)    += io-prio.o
>>>>    obj-$(CONFIG_BLK_CGROUP_IOLATENCY)    += blk-iolatency.o
>>>>    obj-$(CONFIG_BLK_CGROUP_IOCOST)    += blk-iocost.o
>>>>    obj-$(CONFIG_MQ_IOSCHED_DEADLINE)    += mq-deadline.o
>>>
>>> Is the above change really necessary?
>>
>> Except for making maintaining easier on a running system, removing a
>> rqos policy module with cgroup supporting can release a blk-cgroup
>> policy slots. As BLKCG_MAX_POLS, the max slots number is fixed now.
> 
> It seems like my question was not clear? What I meant is that I think that the above changes are not necessary to build blk-ioprio as a kernel module.
> 
Thanks so much for your kindly remind. I have changed it.

Regards
Jianchao
diff mbox series

Patch

diff --git a/block/Kconfig b/block/Kconfig
index d5d4197b7ed2..9cc8e4688953 100644
--- a/block/Kconfig
+++ b/block/Kconfig
@@ -145,7 +145,7 @@  config BLK_CGROUP_IOCOST
 	their share of the overall weight distribution.
 
 config BLK_CGROUP_IOPRIO
-	bool "Cgroup I/O controller for assigning an I/O priority class"
+	tristate "Cgroup I/O controller for assigning an I/O priority class"
 	depends on BLK_CGROUP
 	help
 	Enable the .prio interface for assigning an I/O priority class to
diff --git a/block/Makefile b/block/Makefile
index f38eaa612929..f6a3995af285 100644
--- a/block/Makefile
+++ b/block/Makefile
@@ -17,7 +17,8 @@  obj-$(CONFIG_BLK_DEV_BSGLIB)	+= bsg-lib.o
 obj-$(CONFIG_BLK_CGROUP)	+= blk-cgroup.o
 obj-$(CONFIG_BLK_CGROUP_RWSTAT)	+= blk-cgroup-rwstat.o
 obj-$(CONFIG_BLK_DEV_THROTTLING)	+= blk-throttle.o
-obj-$(CONFIG_BLK_CGROUP_IOPRIO)	+= blk-ioprio.o
+io-prio-y 			:= blk-ioprio.o
+obj-$(CONFIG_BLK_CGROUP_IOPRIO)	+= io-prio.o
 obj-$(CONFIG_BLK_CGROUP_IOLATENCY)	+= blk-iolatency.o
 obj-$(CONFIG_BLK_CGROUP_IOCOST)	+= blk-iocost.o
 obj-$(CONFIG_MQ_IOSCHED_DEADLINE)	+= mq-deadline.o
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 3ae2aa557aef..f617f7ba311d 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -32,7 +32,6 @@ 
 #include <linux/psi.h>
 #include <linux/part_stat.h>
 #include "blk.h"
-#include "blk-ioprio.h"
 #include "blk-throttle.h"
 
 /*
@@ -1195,10 +1194,6 @@  int blkcg_init_queue(struct request_queue *q)
 	if (preloaded)
 		radix_tree_preload_end();
 
-	ret = blk_ioprio_init(q);
-	if (ret)
-		goto err_destroy_all;
-
 	ret = blk_throtl_init(q);
 	if (ret)
 		goto err_destroy_all;
diff --git a/block/blk-ioprio.c b/block/blk-ioprio.c
index 2e7f10e1c03f..074cc0978d0b 100644
--- a/block/blk-ioprio.c
+++ b/block/blk-ioprio.c
@@ -17,7 +17,6 @@ 
 #include <linux/blk_types.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
-#include "blk-ioprio.h"
 #include "blk-rq-qos.h"
 
 /**
@@ -216,15 +215,23 @@  static void blkcg_ioprio_exit(struct rq_qos *rqos)
 		container_of(rqos, typeof(*blkioprio_blkg), rqos);
 
 	blkcg_deactivate_policy(rqos->q, &ioprio_policy);
+	rq_qos_deactivate(rqos);
 	kfree(blkioprio_blkg);
 }
 
+static int blk_ioprio_init(struct request_queue *q);
 static struct rq_qos_ops blkcg_ioprio_ops = {
+#if IS_MODULE(CONFIG_BLK_CGROUP_IOPRIO)
+	.owner	= THIS_MODULE,
+#endif
+	.flags	= RQOS_FLAG_CGRP_POL,
+	.name	= "io-prio",
 	.track	= blkcg_ioprio_track,
 	.exit	= blkcg_ioprio_exit,
+	.init	= blk_ioprio_init,
 };
 
-int blk_ioprio_init(struct request_queue *q)
+static int blk_ioprio_init(struct request_queue *q)
 {
 	struct blk_ioprio *blkioprio_blkg;
 	struct rq_qos *rqos;
@@ -234,36 +241,45 @@  int blk_ioprio_init(struct request_queue *q)
 	if (!blkioprio_blkg)
 		return -ENOMEM;
 
+	/*
+	 * No need to worry ioprio_blkcg_from_css return NULL as
+	 * the queue is frozen right now.
+	 */
+	rqos = &blkioprio_blkg->rqos;
+	rq_qos_activate(q, rqos, &blkcg_ioprio_ops);
+
 	ret = blkcg_activate_policy(q, &ioprio_policy);
 	if (ret) {
+		rq_qos_deactivate(rqos);
 		kfree(blkioprio_blkg);
-		return ret;
 	}
 
-	rqos = &blkioprio_blkg->rqos;
-	rqos->id = RQ_QOS_IOPRIO;
-	rqos->ops = &blkcg_ioprio_ops;
-	rqos->q = q;
-
-	/*
-	 * Registering the rq-qos policy after activating the blk-cgroup
-	 * policy guarantees that ioprio_blkcg_from_bio(bio) != NULL in the
-	 * rq-qos callbacks.
-	 */
-	rq_qos_add(q, rqos);
-
-	return 0;
+	return ret;
 }
 
 static int __init ioprio_init(void)
 {
-	return blkcg_policy_register(&ioprio_policy);
+	int ret;
+
+	ret = rq_qos_register(&blkcg_ioprio_ops);
+	if (ret)
+		return ret;
+
+	ret = blkcg_policy_register(&ioprio_policy);
+	if (ret)
+		rq_qos_unregister(&blkcg_ioprio_ops);
+
+	return ret;
 }
 
 static void __exit ioprio_exit(void)
 {
 	blkcg_policy_unregister(&ioprio_policy);
+	rq_qos_unregister(&blkcg_ioprio_ops);
 }
 
 module_init(ioprio_init);
 module_exit(ioprio_exit);
+MODULE_AUTHOR("Bart Van Assche");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Cgroup I/O controller for assigning an I/O priority class");
diff --git a/block/blk-ioprio.h b/block/blk-ioprio.h
deleted file mode 100644
index a7785c2f1aea..000000000000
--- a/block/blk-ioprio.h
+++ /dev/null
@@ -1,19 +0,0 @@ 
-/* SPDX-License-Identifier: GPL-2.0 */
-
-#ifndef _BLK_IOPRIO_H_
-#define _BLK_IOPRIO_H_
-
-#include <linux/kconfig.h>
-
-struct request_queue;
-
-#ifdef CONFIG_BLK_CGROUP_IOPRIO
-int blk_ioprio_init(struct request_queue *q);
-#else
-static inline int blk_ioprio_init(struct request_queue *q)
-{
-	return 0;
-}
-#endif
-
-#endif /* _BLK_IOPRIO_H_ */
diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 9bb1dabc223b..70a3e4599d99 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -822,10 +822,6 @@  void blk_mq_debugfs_unregister_sched(struct request_queue *q)
 
 static const char *rq_qos_id_to_name(enum rq_qos_id id)
 {
-	switch (id) {
-	case RQ_QOS_IOPRIO:
-		return "ioprio";
-	}
 	return "unknown";
 }
 
diff --git a/block/blk-rq-qos.c b/block/blk-rq-qos.c
index a94ff872722b..629d521e20a7 100644
--- a/block/blk-rq-qos.c
+++ b/block/blk-rq-qos.c
@@ -457,7 +457,7 @@  int rq_qos_register(struct rq_qos_ops *ops)
 		goto out;
 	}
 
-	start = RQ_QOS_IOPRIO + 1;
+	start = 1;
 	ret = ida_simple_get(&rq_qos_ida, start, INT_MAX, GFP_KERNEL);
 	if (ret < 0)
 		goto out;
diff --git a/block/blk-rq-qos.h b/block/blk-rq-qos.h
index 4eef53f2c290..ee396367a5b2 100644
--- a/block/blk-rq-qos.h
+++ b/block/blk-rq-qos.h
@@ -14,7 +14,7 @@ 
 struct blk_mq_debugfs_attr;
 
 enum rq_qos_id {
-	RQ_QOS_IOPRIO,
+	RQ_QOS_UNUSED,
 };
 
 struct rq_wait {