[RFC V4 0/6] blk: make blk-rq-qos policies pluggable and modular

[RFC V4 0/6] blk: make blk-rq-qos policies pluggable and modular

[Wang Jianchao (Kuaishou)]

Hi Jens

blk-rq-qos is a standalone framework out of io-sched and can be used to
control or observe the IO progress in block-layer with hooks. blk-rq-qos
is a great design but right now, it is totally fixed and built-in and shut
out peoples who want to use it with external module.

This patchset attempts to make blk-rq-qos framework pluggable and modular.
Then we can update the blk-rq-qos policy module w/o stopping the IO workload.
And it is more convenient to introduce new policy on old machines w/o udgrade
kernel. And we can close all of the blk-rq-qos policy if we needn't any of
them. At the moment, the request_queue.rqos list is empty, we needn't to
waste cpu cyles on them.

Changes since v3:
 - Fix some code sytle issues
 - Rename policies to blk-wbt, blk-iolat, blk-iocost, blk-ioprio

Changes since v2:
 Refactor the patches,
 - patch01 only provide new interfaces and not export the sysfs interface.
 - patch02 adapt the wbt to new interface, reserve the previous action
   when write to wbt_lat
 - export sysfs interface in last patch when all of the blk-rqos policies
   are preprared well.

Changes since v1:
 - Just make iocost and iolatency pluggable, then we needn't to export
   those interfaces
 - Remove the iostat rqos policy
 - Rename module of blk-ioprio to io-prio to avoid rename ioprio.c file

WangJianchao(6):
	blk: prepare to make blk-rq-qos pluggable and modular
    blk-wbt: make wbt pluggable
    blk-iolatency: make iolatency pluggable
    blk-iocost: make iocost pluggable
    blk-ioprio: make ioprio pluggable and modular
    blk: export the sysfs for switching qos

 block/Kconfig          |   2 +-
 block/Makefile         |   3 +-
 block/blk-cgroup.c     |  11 ---
 block/blk-iocost.c     |  49 +++++++-----
 block/blk-iolatency.c  |  33 +++++++--
 block/blk-ioprio.c     |  50 ++++++++-----
 block/blk-ioprio.h     |  19 -----
 block/blk-mq-debugfs.c |  23 ++----
 block/blk-rq-qos.c     | 303 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 block/blk-rq-qos.h     |  96 +++++++++---------------
 block/blk-sysfs.c      |   2 +
 block/blk-wbt.c        |  52 +++++++++++--
 block/blk-wbt.h        |   7 +-
 block/blk.h            |   6 --
 include/linux/blkdev.h |   4 +
 15 files changed, 488 insertions(+), 172 deletions(-)

[RFC V4 1/6] blk: prepare to make blk-rq-qos pluggable and modular

[Wang Jianchao (Kuaishou)]

blk-rq-qos is a standalone framework out of io-sched and can be
used to control or observe the IO progress in block-layer with
hooks. blk-rq-qos is a great design but right now, it is totally
fixed and built-in and shut out peoples who want to use it with
external module.

This patch make blk-rq-qos policies pluggable and modular.
(1) Add code to maintain the rq_qos_ops. A rq-qos module need to
    register itself with rq_qos_register(). The original enum
    rq_qos_id will be removed in following patch. They will use
    a dynamic id maintained by rq_qos_ida.
(2) Add .init callback into rq_qos_ops. We use it to initialize the
    resource.
(3) Add /sys/block/x/queue/qos
    We can use '+name' or "-name" to open or close the blk-rq-qos
    policy.

This patch mainly prepare help interfaces and no functional changes.
Following patches will adpat the code of wbt, iolatency, iocost and
ioprio to make them pluggable and modular one by one. And after that,
the /sys/block/xxx/queue/qos interfaces will be exported.

Signed-off-by: Wang Jianchao (Kuaishou) <jianchao.wan9@gmail.com>
---
 block/blk-mq-debugfs.c |   9 +-
 block/blk-rq-qos.c     | 301 ++++++++++++++++++++++++++++++++++++++++-
 block/blk-rq-qos.h     |  39 +++++-
 include/linux/blkdev.h |   4 +
 4 files changed, 348 insertions(+), 5 deletions(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 3a790eb4995c..8b6d557e1ad6 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -729,7 +729,10 @@ void blk_mq_debugfs_register(struct request_queue *q)
 
 	if (q->rq_qos) {
 		struct rq_qos *rqos = q->rq_qos;
-
+		/*
+		 * queue has not been registered right now, it is safe to
+		 * iterate the rqos w/o lock
+		 */
 		while (rqos) {
 			blk_mq_debugfs_register_rqos(rqos);
 			rqos = rqos->next;
@@ -844,7 +847,9 @@ void blk_mq_debugfs_unregister_rqos(struct rq_qos *rqos)
 void blk_mq_debugfs_register_rqos(struct rq_qos *rqos)
 {
 	struct request_queue *q = rqos->q;
-	const char *dir_name = rq_qos_id_to_name(rqos->id);
+	const char *dir_name;
+
+	dir_name = rqos->ops->name ? rqos->ops->name : rq_qos_id_to_name(rqos->id);
 
 	if (rqos->debugfs_dir || !rqos->ops->debugfs_attrs)
 		return;
diff --git a/block/blk-rq-qos.c b/block/blk-rq-qos.c
index e83af7bc7591..db13581ae878 100644
--- a/block/blk-rq-qos.c
+++ b/block/blk-rq-qos.c
@@ -2,6 +2,11 @@
 
 #include "blk-rq-qos.h"
 
+static DEFINE_IDA(rq_qos_ida);
+static int nr_rqos_blkcg_pols;
+static DEFINE_MUTEX(rq_qos_mutex);
+static LIST_HEAD(rq_qos_list);
+
 /*
  * Increment 'v', if 'v' is below 'below'. Returns true if we succeeded,
  * false if 'v' + 1 would be bigger than 'below'.
@@ -294,11 +299,303 @@ void rq_qos_wait(struct rq_wait *rqw, void *private_data,
 
 void rq_qos_exit(struct request_queue *q)
 {
-	blk_mq_debugfs_unregister_queue_rqos(q);
-
+	/*
+	 * queue must have been unregistered here, it is safe to iterate
+	 * the list w/o lock
+	 */
 	while (q->rq_qos) {
 		struct rq_qos *rqos = q->rq_qos;
 		q->rq_qos = rqos->next;
 		rqos->ops->exit(rqos);
 	}
+	blk_mq_debugfs_unregister_queue_rqos(q);
+}
+
+static struct rq_qos *rq_qos_by_name(struct request_queue *q,
+		const char *name)
+{
+	struct rq_qos *rqos;
+
+	for (rqos = q->rq_qos; rqos; rqos = rqos->next) {
+		if (!rqos->ops->name)
+			continue;
+
+		if (!strncmp(rqos->ops->name, name,
+					strlen(rqos->ops->name)))
+			return rqos;
+	}
+	return NULL;
+}
+
+/*
+ * After the pluggable blk-qos, rqos's life cycle become complicated,
+ * as we may modify the rqos list there. Except for the places where
+ * queue is not registered, there are following places may access rqos
+ * list concurrently:
+ * (1) normal IO path, can be serialized by queue freezing
+ * (2) blkg_create, the .pd_init_fn() may access rqos, can be serialized
+ *     by queue_lock.
+ * (3) cgroup file, such as ioc_cost_model_write, rq_qos_get is for this
+ *     case to keep the rqos alive.
+ */
+struct rq_qos *rq_qos_get(struct request_queue *q, int id)
+{
+	struct rq_qos *rqos;
+
+	spin_lock_irq(&q->queue_lock);
+	rqos = rq_qos_by_id(q, id);
+	if (rqos && rqos->dying)
+		rqos = NULL;
+	if (rqos)
+		refcount_inc(&rqos->ref);
+	spin_unlock_irq(&q->queue_lock);
+	return rqos;
+}
+EXPORT_SYMBOL_GPL(rq_qos_get);
+
+void rq_qos_put(struct rq_qos *rqos)
+{
+	struct request_queue *q = rqos->q;
+
+	spin_lock_irq(&q->queue_lock);
+	refcount_dec(&rqos->ref);
+	if (rqos->dying)
+		wake_up(&rqos->waitq);
+	spin_unlock_irq(&q->queue_lock);
+}
+EXPORT_SYMBOL_GPL(rq_qos_put);
+
+void rq_qos_activate(struct request_queue *q,
+		struct rq_qos *rqos, const struct rq_qos_ops *ops)
+{
+	struct rq_qos *pos;
+
+	rqos->dying = false;
+	refcount_set(&rqos->ref, 1);
+	init_waitqueue_head(&rqos->waitq);
+	rqos->id = ops->id;
+	rqos->ops = ops;
+	rqos->q = q;
+	rqos->next = NULL;
+
+	spin_lock_irq(&q->queue_lock);
+	pos = q->rq_qos;
+	if (pos) {
+		while (pos->next)
+			pos = pos->next;
+		pos->next = rqos;
+	} else {
+		q->rq_qos = rqos;
+	}
+	spin_unlock_irq(&q->queue_lock);
+
+	if (rqos->ops->debugfs_attrs)
+		blk_mq_debugfs_register_rqos(rqos);
+
+	if (ops->owner)
+		__module_get(ops->owner);
+}
+EXPORT_SYMBOL_GPL(rq_qos_activate);
+
+void rq_qos_deactivate(struct rq_qos *rqos)
+{
+	struct request_queue *q = rqos->q;
+	struct rq_qos **cur;
+
+	spin_lock_irq(&q->queue_lock);
+	rqos->dying = true;
+	/*
+	 * Drain all of the usage of get/put_rqos()
+	 */
+	wait_event_lock_irq(rqos->waitq,
+		refcount_read(&rqos->ref) == 1, q->queue_lock);
+	for (cur = &q->rq_qos; *cur; cur = &(*cur)->next) {
+		if (*cur == rqos) {
+			*cur = rqos->next;
+			break;
+		}
+	}
+	spin_unlock_irq(&q->queue_lock);
+	blk_mq_debugfs_unregister_rqos(rqos);
+
+	if (rqos->ops->owner)
+		module_put(rqos->ops->owner);
+}
+EXPORT_SYMBOL_GPL(rq_qos_deactivate);
+
+static struct rq_qos_ops *rq_qos_op_find(const char *name)
+{
+	struct rq_qos_ops *pos;
+
+	list_for_each_entry(pos, &rq_qos_list, node) {
+		if (!strncmp(pos->name, name, strlen(pos->name)))
+			return pos;
+	}
+
+	return NULL;
+}
+
+int rq_qos_register(struct rq_qos_ops *ops)
+{
+	int ret, start;
+
+	mutex_lock(&rq_qos_mutex);
+
+	if (rq_qos_op_find(ops->name)) {
+		ret = -EEXIST;
+		goto out;
+	}
+
+	if (ops->flags & RQOS_FLAG_CGRP_POL &&
+	    nr_rqos_blkcg_pols >= (BLKCG_MAX_POLS - BLKCG_NON_RQOS_POLS)) {
+		ret = -ENOSPC;
+		goto out;
+	}
+
+	start = RQ_QOS_IOPRIO + 1;
+	ret = ida_simple_get(&rq_qos_ida, start, INT_MAX, GFP_KERNEL);
+	if (ret < 0)
+		goto out;
+
+	if (ops->flags & RQOS_FLAG_CGRP_POL)
+		nr_rqos_blkcg_pols++;
+
+	ops->id = ret;
+	ret = 0;
+	INIT_LIST_HEAD(&ops->node);
+	list_add_tail(&ops->node, &rq_qos_list);
+out:
+	mutex_unlock(&rq_qos_mutex);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(rq_qos_register);
+
+void rq_qos_unregister(struct rq_qos_ops *ops)
+{
+	mutex_lock(&rq_qos_mutex);
+
+	if (ops->flags & RQOS_FLAG_CGRP_POL)
+		nr_rqos_blkcg_pols--;
+	list_del_init(&ops->node);
+	ida_simple_remove(&rq_qos_ida, ops->id);
+	mutex_unlock(&rq_qos_mutex);
+}
+EXPORT_SYMBOL_GPL(rq_qos_unregister);
+
+ssize_t queue_qos_show(struct request_queue *q, char *buf)
+{
+	struct rq_qos_ops *ops;
+	struct rq_qos *rqos;
+	int ret = 0;
+
+	mutex_lock(&rq_qos_mutex);
+	/*
+	 * Show the policies in the order of being invoked.
+	 * queue_lock is not needed here as the sysfs_lock is
+	 * protected us from the queue_qos_store()
+	 */
+	for (rqos = q->rq_qos; rqos; rqos = rqos->next) {
+		if (!rqos->ops->name)
+			continue;
+		ret += sprintf(buf + ret, "[%s] ", rqos->ops->name);
+	}
+	list_for_each_entry(ops, &rq_qos_list, node) {
+		if (!rq_qos_by_name(q, ops->name))
+			ret += sprintf(buf + ret, "%s ", ops->name);
+	}
+
+	ret--; /* overwrite the last space */
+	ret += sprintf(buf + ret, "\n");
+	mutex_unlock(&rq_qos_mutex);
+
+	return ret;
+}
+
+static int rq_qos_switch(struct request_queue *q,
+		const struct rq_qos_ops *ops,
+		struct rq_qos *rqos)
+{
+	int ret;
+
+	blk_mq_freeze_queue(q);
+	if (!rqos) {
+		ret = ops->init(q);
+	} else {
+		ops->exit(rqos);
+		ret = 0;
+	}
+	blk_mq_unfreeze_queue(q);
+
+	return ret;
+}
+
+ssize_t queue_qos_store(struct request_queue *q, const char *page,
+			  size_t count)
+{
+	const struct rq_qos_ops *ops;
+	struct rq_qos *rqos;
+	const char *qosname;
+	char *buf;
+	bool add;
+	int ret;
+
+	if (!blk_queue_registered(q))
+		return -ENOENT;
+
+	buf = kstrdup(page, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	buf = strim(buf);
+	if (buf[0] != '+' && buf[0] != '-') {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	add = buf[0] == '+';
+	qosname = buf + 1;
+
+	rqos = rq_qos_by_name(q, qosname);
+	if ((buf[0] == '+' && rqos)) {
+		ret = -EEXIST;
+		goto out;
+	}
+
+	if ((buf[0] == '-' && !rqos)) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	if (add) {
+		mutex_lock(&rq_qos_mutex);
+		ops = rq_qos_op_find(qosname);
+		if (!ops) {
+			/*
+			 * module_init callback may request this mutex
+			 */
+			mutex_unlock(&rq_qos_mutex);
+			request_module("%s", qosname);
+			mutex_lock(&rq_qos_mutex);
+			ops = rq_qos_op_find(qosname);
+		}
+		if (!ops) {
+			ret = -EINVAL;
+		} else if (ops->owner && !try_module_get(ops->owner)) {
+			ops = NULL;
+			ret = -EAGAIN;
+		}
+		mutex_unlock(&rq_qos_mutex);
+		if (!ops)
+			goto out;
+	} else {
+		ops = rqos->ops;
+	}
+
+	ret = rq_qos_switch(q, ops, add ? NULL : rqos);
+
+	if (add)
+		module_put(ops->owner);
+out:
+	kfree(buf);
+	return ret ? ret : count;
 }
diff --git a/block/blk-rq-qos.h b/block/blk-rq-qos.h
index 3cfbc8668cba..586c3f5ec152 100644
--- a/block/blk-rq-qos.h
+++ b/block/blk-rq-qos.h
@@ -26,16 +26,28 @@ struct rq_wait {
 };
 
 struct rq_qos {
-	struct rq_qos_ops *ops;
+	const struct rq_qos_ops *ops;
 	struct request_queue *q;
 	enum rq_qos_id id;
+	refcount_t ref;
+	wait_queue_head_t waitq;
+	bool dying;
 	struct rq_qos *next;
 #ifdef CONFIG_BLK_DEBUG_FS
 	struct dentry *debugfs_dir;
 #endif
 };
 
+enum {
+	RQOS_FLAG_CGRP_POL = 1 << 0,
+};
+
 struct rq_qos_ops {
+	struct list_head node;
+	struct module *owner;
+	const char *name;
+	int flags;
+	int id;
 	void (*throttle)(struct rq_qos *, struct bio *);
 	void (*track)(struct rq_qos *, struct request *, struct bio *);
 	void (*merge)(struct rq_qos *, struct request *, struct bio *);
@@ -46,6 +58,7 @@ struct rq_qos_ops {
 	void (*cleanup)(struct rq_qos *, struct bio *);
 	void (*queue_depth_changed)(struct rq_qos *);
 	void (*exit)(struct rq_qos *);
+	int (*init)(struct request_queue *);
 	const struct blk_mq_debugfs_attr *debugfs_attrs;
 };
 
@@ -70,6 +83,19 @@ static inline struct rq_qos *rq_qos_id(struct request_queue *q,
 	return rqos;
 }
 
+static inline struct rq_qos *rq_qos_by_id(struct request_queue *q, int id)
+{
+	struct rq_qos *rqos;
+
+	WARN_ON(!mutex_is_locked(&q->sysfs_lock) && !spin_is_locked(&q->queue_lock));
+
+	for (rqos = q->rq_qos; rqos; rqos = rqos->next) {
+		if (rqos->id == id)
+			break;
+	}
+	return rqos;
+}
+
 static inline struct rq_qos *wbt_rq_qos(struct request_queue *q)
 {
 	return rq_qos_id(q, RQ_QOS_WBT);
@@ -132,6 +158,17 @@ static inline void rq_qos_del(struct request_queue *q, struct rq_qos *rqos)
 	blk_mq_debugfs_unregister_rqos(rqos);
 }
 
+int rq_qos_register(struct rq_qos_ops *ops);
+void rq_qos_unregister(struct rq_qos_ops *ops);
+void rq_qos_activate(struct request_queue *q,
+		struct rq_qos *rqos, const struct rq_qos_ops *ops);
+void rq_qos_deactivate(struct rq_qos *rqos);
+ssize_t queue_qos_show(struct request_queue *q, char *buf);
+ssize_t queue_qos_store(struct request_queue *q, const char *page,
+			  size_t count);
+struct rq_qos *rq_qos_get(struct request_queue *q, int id);
+void rq_qos_put(struct rq_qos *rqos);
+
 typedef bool (acquire_inflight_cb_t)(struct rq_wait *rqw, void *private_data);
 typedef void (cleanup_cb_t)(struct rq_wait *rqw, void *private_data);
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index f35aea98bc35..d5698a7cda67 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -44,6 +44,10 @@ struct blk_crypto_profile;
  * Defined here to simplify include dependency.
  */
 #define BLKCG_MAX_POLS		6
+/*
+ * Non blk-rq-qos blkcg policies include blk-throttle and bfq
+ */
+#define BLKCG_NON_RQOS_POLS		2
 
 static inline int blk_validate_block_size(unsigned long bsize)
 {
-- 
2.17.1

Re: [RFC V4 1/6] blk: prepare to make blk-rq-qos pluggable and modular

[Christoph Hellwig]

>  {
>  	struct request_queue *q = rqos->q;
> -	const char *dir_name = rq_qos_id_to_name(rqos->id);
> +	const char *dir_name;
> +
> +	dir_name = rqos->ops->name ? rqos->ops->name : rq_qos_id_to_name(rqos->id);

Overly long line here.  And it would be much more readable if you used
a good old if/else.

> +static DEFINE_IDA(rq_qos_ida);
> +static int nr_rqos_blkcg_pols;
> +static DEFINE_MUTEX(rq_qos_mutex);
> +static LIST_HEAD(rq_qos_list);

Please use an allocating xarray instead of an IDA plus list.

> +	/*
> +	 * queue must have been unregistered here, it is safe to iterate
> +	 * the list w/o lock
> +	 */

Please capitalize multi-line comments.

> + * After the pluggable blk-qos, rqos's life cycle become complicated,
> + * as we may modify the rqos list there. Except for the places where
> + * queue is not registered, there are following places may access rqos
> + * list concurrently:

Code comments are not the place to explain history.  PLease explain the
current situation.

> +struct rq_qos *rq_qos_get(struct request_queue *q, int id)
> +{
> +	struct rq_qos *rqos;
> +
> +	spin_lock_irq(&q->queue_lock);

Please don't use the grab all queue_lock for new code.  It badly needs
to be split and documented, and new code is the best place to start
that.

Also with all the new code please add a new config option that is
selected by all rq-pos implementations so that blk-rq-qos.c only gets
built when actually needed.

> +static inline struct rq_qos *rq_qos_by_id(struct request_queue *q, int id)
> +{
> +	struct rq_qos *rqos;
> +
> +	WARN_ON(!mutex_is_locked(&q->sysfs_lock) && !spin_is_locked(&q->queue_lock));

Another overly long line.  And in doubt split this into two helpers
so that you cna use lockdep_assert_held instead of doing the incorrect
asserts.

Re: [RFC V4 1/6] blk: prepare to make blk-rq-qos pluggable and modular

[Wang Jianchao]

On 2022/2/17 4:48 下午, Christoph Hellwig wrote:
>>  {
>>  	struct request_queue *q = rqos->q;
>> -	const char *dir_name = rq_qos_id_to_name(rqos->id);
>> +	const char *dir_name;
>> +
>> +	dir_name = rqos->ops->name ? rqos->ops->name : rq_qos_id_to_name(rqos->id);
> 
> Overly long line here.  And it would be much more readable if you used
> a good old if/else.
> 
>> +static DEFINE_IDA(rq_qos_ida);
>> +static int nr_rqos_blkcg_pols;
>> +static DEFINE_MUTEX(rq_qos_mutex);
>> +static LIST_HEAD(rq_qos_list);
> 
> Please use an allocating xarray instead of an IDA plus list.
> 
>> +	/*
>> +	 * queue must have been unregistered here, it is safe to iterate
>> +	 * the list w/o lock
>> +	 */
> 
> Please capitalize multi-line comments.
> 
>> + * After the pluggable blk-qos, rqos's life cycle become complicated,
>> + * as we may modify the rqos list there. Except for the places where
>> + * queue is not registered, there are following places may access rqos
>> + * list concurrently:
> 
> Code comments are not the place to explain history.  PLease explain the
> current situation.
> 
>> +struct rq_qos *rq_qos_get(struct request_queue *q, int id)
>> +{
>> +	struct rq_qos *rqos;
>> +
>> +	spin_lock_irq(&q->queue_lock);
> 
> Please don't use the grab all queue_lock for new code.  It badly needs
> to be split and documented, and new code is the best place to start
> that.
> 
> Also with all the new code please add a new config option that is
> selected by all rq-pos implementations so that blk-rq-qos.c only gets
> built when actually needed.
> 
>> +static inline struct rq_qos *rq_qos_by_id(struct request_queue *q, int id)
>> +{
>> +	struct rq_qos *rqos;
>> +
>> +	WARN_ON(!mutex_is_locked(&q->sysfs_lock) && !spin_is_locked(&q->queue_lock));
> 
> Another overly long line.  And in doubt split this into two helpers
> so that you cna use lockdep_assert_held instead of doing the incorrect
> asserts.

Thanks so much for your kindly comment. I'd change the code in next version.

Regards
Jianchao

Re: [RFC V4 1/6] blk: prepare to make blk-rq-qos pluggable and modular

[Tejun Heo]

Hello,

On Thu, Feb 17, 2022 at 11:13:44AM +0800, Wang Jianchao (Kuaishou) wrote:
> (3) Add /sys/block/x/queue/qos
>     We can use '+name' or "-name" to open or close the blk-rq-qos
>     policy.

I don't understand why we're modularizing rq-qos in this non-standard way
instead of modprobing to enable a policy and rmmoding to disable. Why are we
building in qos names into the kernel and adding an extra module handling
interface?

Thanks.

-- 
tejun

Re: [RFC V4 1/6] blk: prepare to make blk-rq-qos pluggable and modular

[Wang Jianchao]

On 2022/2/23 1:19 上午, Tejun Heo wrote:
> Hello,
> 
> On Thu, Feb 17, 2022 at 11:13:44AM +0800, Wang Jianchao (Kuaishou) wrote:
>> (3) Add /sys/block/x/queue/qos
>>     We can use '+name' or "-name" to open or close the blk-rq-qos
>>     policy.
> 
> I don't understand why we're modularizing rq-qos in this non-standard way
> instead of modprobing to enable a policy and rmmoding to disable. Why are we
> building in qos names into the kernel and adding an extra module handling
> interface?

Hi Tejun

We just want to provide the flexibility for the user to open/close a policy
per device. If we need to the policy on a device, we needn't to waste cpu
cycles and memory for it.

Thanks
Jianchao

Re: [RFC V4 1/6] blk: prepare to make blk-rq-qos pluggable and modular

[Wang Jianchao]

On 2022/2/23 11:08 上午, Wang Jianchao wrote:
> 
> 
> On 2022/2/23 1:19 上午, Tejun Heo wrote:
>> Hello,
>>
>> On Thu, Feb 17, 2022 at 11:13:44AM +0800, Wang Jianchao (Kuaishou) wrote:
>>> (3) Add /sys/block/x/queue/qos
>>>     We can use '+name' or "-name" to open or close the blk-rq-qos
>>>     policy.
>>
>> I don't understand why we're modularizing rq-qos in this non-standard way
>> instead of modprobing to enable a policy and rmmoding to disable. Why are we
>> building in qos names into the kernel and adding an extra module handling
>> interface?
> 
> Hi Tejun
> 
> We just want to provide the flexibility for the user to open/close a policy
> per device. If we need to the policy on a device, we needn't to waste cpu
sorry, it should be "If we don't need the policy on a device" ;)

Thanks
Jianchao
> cycles and memory for it.
> 
> Thanks
> Jianchao

Re: [RFC V4 1/6] blk: prepare to make blk-rq-qos pluggable and modular

[Tejun Heo]

Hello,

> > We just want to provide the flexibility for the user to open/close a policy
> > per device. If we need to the policy on a device, we needn't to waste cpu
> sorry, it should be "If we don't need the policy on a device" ;)

Yeah, that's what modularization does but why does it need a separate user
interface for loading? Everything else inits on insmod and exits on rmmod
and autoloading has been delegated to udev a very long time ago. The
interface you added for loading module doesn't make sense to me.

Thanks.

-- 
tejun

Re: [RFC V4 1/6] blk: prepare to make blk-rq-qos pluggable and modular

[Wang Jianchao]

On 2022/2/24 5:37 上午, Tejun Heo wrote:
> Hello,
> 
>>> We just want to provide the flexibility for the user to open/close a policy
>>> per device. If we need to the policy on a device, we needn't to waste cpu
>> sorry, it should be "If we don't need the policy on a device" ;)
> 
> Yeah, that's what modularization does but why does it need a separate user
> interface for loading? Everything else inits on insmod and exits on rmmod
> and autoloading has been delegated to udev a very long time ago. The
> interface you added for loading module doesn't make sense to me.
> 

The initial version of this patchset has two targets：
(1) Add a sysfs interface to open/close the policy per device. Then we needn't
    waste cpu cycles and memory if the device doesn't need the policy.
(2) Make the policies modular, then it easy to maintain the code of policy in
    production environment as we only need to close the policy and replace the
    .ko file.

The loading module when open policy in sysfs interface is just to avoid modprobe
manually. There is similar operation when switch io scheduler.

And as Christoph suggested, the modularization has been get rid of in next version.

Thanks
Jianchao

Re: [RFC V4 1/6] blk: prepare to make blk-rq-qos pluggable and modular

[Tejun Heo]

Hello,

On Thu, Feb 24, 2022 at 09:51:04AM +0800, Wang Jianchao wrote:
> The initial version of this patchset has two targets：
> (1) Add a sysfs interface to open/close the policy per device. Then we needn't
>     waste cpu cycles and memory if the device doesn't need the policy.
> (2) Make the policies modular, then it easy to maintain the code of policy in
>     production environment as we only need to close the policy and replace the
>     .ko file.
> 
> The loading module when open policy in sysfs interface is just to avoid modprobe
> manually. There is similar operation when switch io scheduler.

Each rq-qos mechanism already needs and has a way to turn off itself.
There's no reason to add another layer on top. If the current way of
disabling isn't efficient, we should improve that instead of adding a new
layer of interface on top.

And please don't add a custom interface to avoid modprobing. All it adds is
unnecessary deviation. There's no benefit to echoing a selector to a custom
sysfs file compared to explicitly modprobing it.

Thanks.

-- 
tejun

Re: [RFC V4 1/6] blk: prepare to make blk-rq-qos pluggable and modular

[Wang Jianchao]

On 2022/2/24 10:07 上午, Tejun Heo wrote:
> Hello,
> 
> On Thu, Feb 24, 2022 at 09:51:04AM +0800, Wang Jianchao wrote:
>> The initial version of this patchset has two targets：
>> (1) Add a sysfs interface to open/close the policy per device. Then we needn't
>>     waste cpu cycles and memory if the device doesn't need the policy.
>> (2) Make the policies modular, then it easy to maintain the code of policy in
>>     production environment as we only need to close the policy and replace the
>>     .ko file.
>>
>> The loading module when open policy in sysfs interface is just to avoid modprobe
>> manually. There is similar operation when switch io scheduler.
> 
> Each rq-qos mechanism already needs and has a way to turn off itself.
> There's no reason to add another layer on top. If the current way of
> disabling isn't efficient, we should improve that instead of adding a new
> layer of interface on top.

Yes, right now, every policy has their own way to turn off, but we always need to
iterate the rqos list and enter into the policy's callback to check it. And every
blkio cgroup needs to allocate memory for it even we don't use it.

I don't this patchset is adding a new layer, but blk-rq-qos layer has been already
there , we just add a unified interface to open/close the policies.

Thanks
Jianchao

Re: [RFC V4 1/6] blk: prepare to make blk-rq-qos pluggable and modular

[Tejun Heo]

On Thu, Feb 24, 2022 at 10:50:22AM +0800, Wang Jianchao wrote:
> Yes, right now, every policy has their own way to turn off, but we always need to
> iterate the rqos list and enter into the policy's callback to check it. And every
> blkio cgroup needs to allocate memory for it even we don't use it.
> 
> I don't this patchset is adding a new layer, but blk-rq-qos layer has been already
> there , we just add a unified interface to open/close the policies.

We're talking in circles. We already know when a policy is inactive. If it
sits in hot path in that state, take it off whatever gets iterated in hot
path and put it back on when it actually gets enabled. The same goes for
memory allocation. If there's substantial amount of memory allocted while
not used, make that dynamic and trigger it when the policy starts getting
used. It makes no sense to add another enable/disable interface on top.

FWIW, please consider the series nacked on this side.

Thanks.

-- 
tejun

Re: [RFC V4 1/6] blk: prepare to make blk-rq-qos pluggable and modular

[Wang Jianchao]

On 2022/2/25 12:53 上午, Tejun Heo wrote:
> On Thu, Feb 24, 2022 at 10:50:22AM +0800, Wang Jianchao wrote:
>> Yes, right now, every policy has their own way to turn off, but we always need to
>> iterate the rqos list and enter into the policy's callback to check it. And every
>> blkio cgroup needs to allocate memory for it even we don't use it.
>>
>> I don't this patchset is adding a new layer, but blk-rq-qos layer has been already
>> there , we just add a unified interface to open/close the policies.
> 
> We're talking in circles. We already know when a policy is inactive. If it
> sits in hot path in that state, take it off whatever gets iterated in hot
> path and put it back on when it actually gets enabled. The same goes for
> memory allocation. If there's substantial amount of memory allocted while
> not used, make that dynamic and trigger it when the policy starts getting
> used. It makes no sense to add another enable/disable interface on top.
>
It can make things more complicated if we does as above...

> FWIW, please consider the series nacked on this side.

Anyway, thanks so much for all of your comment ;)

Regards
Jianchao

[RFC V4 2/6] blk-wbt: make wbt pluggable

[Wang Jianchao (Kuaishou)]

This patch makes wbt pluggable through /sys/block/xxx/queue/qos.
Some queue_lock/unlock is added to protect rq_qos_by_id() in
wbt_rq_qos(). By default, wbt is enabled which is same with previous
code.

Signed-off-by: Wang Jianchao (Kuaishou) <jianchao.wan9@gmail.com>
---
 block/blk-mq-debugfs.c |  2 --
 block/blk-rq-qos.h     |  6 -----
 block/blk-wbt.c        | 51 +++++++++++++++++++++++++++++++++++-------
 block/blk-wbt.h        |  6 +++++
 4 files changed, 49 insertions(+), 16 deletions(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 8b6d557e1ad6..a5d78b094234 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -826,8 +826,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_WBT:
-		return "wbt";
 	case RQ_QOS_LATENCY:
 		return "latency";
 	case RQ_QOS_COST:
diff --git a/block/blk-rq-qos.h b/block/blk-rq-qos.h
index 586c3f5ec152..171a83d6de45 100644
--- a/block/blk-rq-qos.h
+++ b/block/blk-rq-qos.h
@@ -14,7 +14,6 @@
 struct blk_mq_debugfs_attr;
 
 enum rq_qos_id {
-	RQ_QOS_WBT,
 	RQ_QOS_LATENCY,
 	RQ_QOS_COST,
 	RQ_QOS_IOPRIO,
@@ -96,11 +95,6 @@ static inline struct rq_qos *rq_qos_by_id(struct request_queue *q, int id)
 	return rqos;
 }
 
-static inline struct rq_qos *wbt_rq_qos(struct request_queue *q)
-{
-	return rq_qos_id(q, RQ_QOS_WBT);
-}
-
 static inline struct rq_qos *blkcg_rq_qos(struct request_queue *q)
 {
 	return rq_qos_id(q, RQ_QOS_LATENCY);
diff --git a/block/blk-wbt.c b/block/blk-wbt.c
index 0c119be0e813..8aa85303bbbc 100644
--- a/block/blk-wbt.c
+++ b/block/blk-wbt.c
@@ -31,6 +31,13 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/wbt.h>
 
+static struct rq_qos_ops wbt_rqos_ops;
+
+struct rq_qos *wbt_rq_qos(struct request_queue *q)
+{
+	return rq_qos_by_id(q, wbt_rqos_ops.id);
+}
+
 static inline void wbt_clear_state(struct request *rq)
 {
 	rq->wbt_flags = 0;
@@ -628,9 +635,13 @@ static void wbt_requeue(struct rq_qos *rqos, struct request *rq)
 
 void wbt_set_write_cache(struct request_queue *q, bool write_cache_on)
 {
-	struct rq_qos *rqos = wbt_rq_qos(q);
+	struct rq_qos *rqos;
+
+	spin_lock_irq(&q->queue_lock);
+	rqos = wbt_rq_qos(q);
 	if (rqos)
 		RQWB(rqos)->wc = write_cache_on;
+	spin_unlock_irq(&q->queue_lock);
 }
 
 /*
@@ -638,14 +649,20 @@ void wbt_set_write_cache(struct request_queue *q, bool write_cache_on)
  */
 void wbt_enable_default(struct request_queue *q)
 {
-	struct rq_qos *rqos = wbt_rq_qos(q);
+	struct rq_qos *rqos;
+
+	spin_lock_irq(&q->queue_lock);
+	rqos = wbt_rq_qos(q);
 
 	/* Throttling already enabled? */
 	if (rqos) {
 		if (RQWB(rqos)->enable_state == WBT_STATE_OFF_DEFAULT)
 			RQWB(rqos)->enable_state = WBT_STATE_ON_DEFAULT;
+
+		spin_unlock_irq(&q->queue_lock);
 		return;
 	}
+	spin_unlock_irq(&q->queue_lock);
 
 	/* Queue not registered? Maybe shutting down... */
 	if (!blk_queue_registered(q))
@@ -692,6 +709,7 @@ static void wbt_exit(struct rq_qos *rqos)
 	struct rq_wb *rwb = RQWB(rqos);
 	struct request_queue *q = rqos->q;
 
+	rq_qos_deactivate(rqos);
 	blk_stat_remove_callback(q, rwb->cb);
 	blk_stat_free_callback(rwb->cb);
 	kfree(rwb);
@@ -702,15 +720,21 @@ static void wbt_exit(struct rq_qos *rqos)
  */
 void wbt_disable_default(struct request_queue *q)
 {
-	struct rq_qos *rqos = wbt_rq_qos(q);
+	struct rq_qos *rqos;
 	struct rq_wb *rwb;
+
+	spin_lock_irq(&q->queue_lock);
+	rqos = wbt_rq_qos(q);
 	if (!rqos)
-		return;
+		goto out;
+
 	rwb = RQWB(rqos);
 	if (rwb->enable_state == WBT_STATE_ON_DEFAULT) {
 		blk_stat_deactivate(rwb->cb);
 		rwb->enable_state = WBT_STATE_OFF_DEFAULT;
 	}
+out:
+	spin_unlock_irq(&q->queue_lock);
 }
 EXPORT_SYMBOL_GPL(wbt_disable_default);
 
@@ -803,6 +827,7 @@ static const struct blk_mq_debugfs_attr wbt_debugfs_attrs[] = {
 #endif
 
 static struct rq_qos_ops wbt_rqos_ops = {
+	.name = "blk-wbt",
 	.throttle = wbt_wait,
 	.issue = wbt_issue,
 	.track = wbt_track,
@@ -811,6 +836,7 @@ static struct rq_qos_ops wbt_rqos_ops = {
 	.cleanup = wbt_cleanup,
 	.queue_depth_changed = wbt_queue_depth_changed,
 	.exit = wbt_exit,
+	.init = wbt_init,
 #ifdef CONFIG_BLK_DEBUG_FS
 	.debugfs_attrs = wbt_debugfs_attrs,
 #endif
@@ -834,9 +860,6 @@ int wbt_init(struct request_queue *q)
 	for (i = 0; i < WBT_NUM_RWQ; i++)
 		rq_wait_init(&rwb->rq_wait[i]);
 
-	rwb->rqos.id = RQ_QOS_WBT;
-	rwb->rqos.ops = &wbt_rqos_ops;
-	rwb->rqos.q = q;
 	rwb->last_comp = rwb->last_issue = jiffies;
 	rwb->win_nsec = RWB_WINDOW_NSEC;
 	rwb->enable_state = WBT_STATE_ON_DEFAULT;
@@ -846,7 +869,7 @@ int wbt_init(struct request_queue *q)
 	/*
 	 * Assign rwb and add the stats callback.
 	 */
-	rq_qos_add(q, &rwb->rqos);
+	rq_qos_activate(q, &rwb->rqos, &wbt_rqos_ops);
 	blk_stat_add_callback(q, rwb->cb);
 
 	rwb->min_lat_nsec = wbt_default_latency_nsec(q);
@@ -856,3 +879,15 @@ int wbt_init(struct request_queue *q)
 
 	return 0;
 }
+
+static __init int wbt_mod_init(void)
+{
+	return rq_qos_register(&wbt_rqos_ops);
+}
+
+static __exit void wbt_mod_exit(void)
+{
+	return rq_qos_unregister(&wbt_rqos_ops);
+}
+module_init(wbt_mod_init);
+module_exit(wbt_mod_exit);
diff --git a/block/blk-wbt.h b/block/blk-wbt.h
index 2eb01becde8c..e0d051cb00f7 100644
--- a/block/blk-wbt.h
+++ b/block/blk-wbt.h
@@ -88,6 +88,7 @@ static inline unsigned int wbt_inflight(struct rq_wb *rwb)
 
 #ifdef CONFIG_BLK_WBT
 
+struct rq_qos *wbt_rq_qos(struct request_queue *q);
 int wbt_init(struct request_queue *);
 void wbt_disable_default(struct request_queue *);
 void wbt_enable_default(struct request_queue *);
@@ -101,6 +102,11 @@ u64 wbt_default_latency_nsec(struct request_queue *);
 
 #else
 
+static inline struct rq_qos *wbt_rq_qos(struct request_queue *q)
+{
+	return NULL;
+}
+
 static inline void wbt_track(struct request *rq, enum wbt_flags flags)
 {
 }
-- 
2.17.1

Re: [RFC V4 2/6] blk-wbt: make wbt pluggable

[Christoph Hellwig]

> +struct rq_qos *wbt_rq_qos(struct request_queue *q);
>  int wbt_init(struct request_queue *);


Please move the wb_lat sysfs attribute into blk-wbt.c as well, which
removes the need to expose these two functions.

Re: [RFC V4 2/6] blk-wbt: make wbt pluggable

[Wang Jianchao]

On 2022/2/17 4:50 下午, Christoph Hellwig wrote:
>> +struct rq_qos *wbt_rq_qos(struct request_queue *q);
>>  int wbt_init(struct request_queue *);
> 
> 
> Please move the wb_lat sysfs attribute into blk-wbt.c as well, which
> removes the need to expose these two functions.
> 

Given this patchset:
(1) Do we need to reserve the wb_lat sysfs when we turn off the wbt ?
(2) Do we need to disable wbt automatically when switch io scheduler
    to bfq ? Or just tell the user turn off the wbt by themselves ?

Thanks
Jianchao

Re: [RFC V4 2/6] blk-wbt: make wbt pluggable

[Christoph Hellwig]

On Fri, Feb 18, 2022 at 11:41:11AM +0800, Wang Jianchao wrote:
> 
> 
> On 2022/2/17 4:50 下午, Christoph Hellwig wrote:
> >> +struct rq_qos *wbt_rq_qos(struct request_queue *q);
> >>  int wbt_init(struct request_queue *);
> > 
> > 
> > Please move the wb_lat sysfs attribute into blk-wbt.c as well, which
> > removes the need to expose these two functions.
> > 
> 
> Given this patchset:
> (1) Do we need to reserve the wb_lat sysfs when we turn off the wbt ?
> (2) Do we need to disable wbt automatically when switch io scheduler
>     to bfq ? Or just tell the user turn off the wbt by themselves ?

I don't think this needs any changes to what is done there today,
i.e. keep the sysfs attribute around.

[RFC V4 3/6] blk-iolatency: make iolatency pluggable

[Wang Jianchao (Kuaishou)]

Make blk-iolatency pluggable. Then we can close or open it through
/sys/block/xxx/queue/qos.

Signed-off-by: Wang Jianchao (Kuaishou) <jianchao.wan9@gmail.com>
---
 block/blk-cgroup.c     |  6 ------
 block/blk-iolatency.c  | 34 ++++++++++++++++++++++++++--------
 block/blk-mq-debugfs.c |  2 --
 block/blk-rq-qos.h     |  6 ------
 block/blk.h            |  6 ------
 5 files changed, 26 insertions(+), 28 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 650f7e27989f..3ae2aa557aef 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1203,12 +1203,6 @@ int blkcg_init_queue(struct request_queue *q)
 	if (ret)
 		goto err_destroy_all;
 
-	ret = blk_iolatency_init(q);
-	if (ret) {
-		blk_throtl_exit(q);
-		goto err_destroy_all;
-	}
-
 	return 0;
 
 err_destroy_all:
diff --git a/block/blk-iolatency.c b/block/blk-iolatency.c
index 6593c7123b97..a8a201d6d669 100644
--- a/block/blk-iolatency.c
+++ b/block/blk-iolatency.c
@@ -90,6 +90,12 @@ struct blk_iolatency {
 	atomic_t enabled;
 };
 
+static struct rq_qos_ops blkcg_iolatency_ops;
+static inline struct rq_qos *blkcg_rq_qos(struct request_queue *q)
+{
+	return rq_qos_by_id(q, blkcg_iolatency_ops.id);
+}
+
 static inline struct blk_iolatency *BLKIOLATENCY(struct rq_qos *rqos)
 {
 	return container_of(rqos, struct blk_iolatency, rqos);
@@ -646,13 +652,19 @@ static void blkcg_iolatency_exit(struct rq_qos *rqos)
 
 	del_timer_sync(&blkiolat->timer);
 	blkcg_deactivate_policy(rqos->q, &blkcg_policy_iolatency);
+	rq_qos_deactivate(rqos);
 	kfree(blkiolat);
 }
 
+static int blk_iolatency_init(struct request_queue *q);
+
 static struct rq_qos_ops blkcg_iolatency_ops = {
+	.name = "blk-iolat",
+	.flags = RQOS_FLAG_CGRP_POL,
 	.throttle = blkcg_iolatency_throttle,
 	.done_bio = blkcg_iolatency_done_bio,
 	.exit = blkcg_iolatency_exit,
+	.init = blk_iolatency_init,
 };
 
 static void blkiolatency_timer_fn(struct timer_list *t)
@@ -727,15 +739,10 @@ int blk_iolatency_init(struct request_queue *q)
 		return -ENOMEM;
 
 	rqos = &blkiolat->rqos;
-	rqos->id = RQ_QOS_LATENCY;
-	rqos->ops = &blkcg_iolatency_ops;
-	rqos->q = q;
-
-	rq_qos_add(q, rqos);
-
+	rq_qos_activate(q, rqos, &blkcg_iolatency_ops);
 	ret = blkcg_activate_policy(q, &blkcg_policy_iolatency);
 	if (ret) {
-		rq_qos_del(q, rqos);
+		rq_qos_deactivate(rqos);
 		kfree(blkiolat);
 		return ret;
 	}
@@ -1046,12 +1053,23 @@ static struct blkcg_policy blkcg_policy_iolatency = {
 
 static int __init iolatency_init(void)
 {
-	return blkcg_policy_register(&blkcg_policy_iolatency);
+	int ret;
+
+	ret = rq_qos_register(&blkcg_iolatency_ops);
+	if (ret)
+		return ret;
+
+	ret = blkcg_policy_register(&blkcg_policy_iolatency);
+	if (ret)
+		rq_qos_unregister(&blkcg_iolatency_ops);
+
+	return ret;
 }
 
 static void __exit iolatency_exit(void)
 {
 	blkcg_policy_unregister(&blkcg_policy_iolatency);
+	rq_qos_unregister(&blkcg_iolatency_ops);
 }
 
 module_init(iolatency_init);
diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index a5d78b094234..918870b8de5b 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -826,8 +826,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_LATENCY:
-		return "latency";
 	case RQ_QOS_COST:
 		return "cost";
 	case RQ_QOS_IOPRIO:
diff --git a/block/blk-rq-qos.h b/block/blk-rq-qos.h
index 171a83d6de45..2a919db52fef 100644
--- a/block/blk-rq-qos.h
+++ b/block/blk-rq-qos.h
@@ -14,7 +14,6 @@
 struct blk_mq_debugfs_attr;
 
 enum rq_qos_id {
-	RQ_QOS_LATENCY,
 	RQ_QOS_COST,
 	RQ_QOS_IOPRIO,
 };
@@ -95,11 +94,6 @@ static inline struct rq_qos *rq_qos_by_id(struct request_queue *q, int id)
 	return rqos;
 }
 
-static inline struct rq_qos *blkcg_rq_qos(struct request_queue *q)
-{
-	return rq_qos_id(q, RQ_QOS_LATENCY);
-}
-
 static inline void rq_wait_init(struct rq_wait *rq_wait)
 {
 	atomic_set(&rq_wait->inflight, 0);
diff --git a/block/blk.h b/block/blk.h
index 8bd43b3ad33d..1a314257b6a3 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -400,12 +400,6 @@ static inline void blk_queue_bounce(struct request_queue *q, struct bio **bio)
 		__blk_queue_bounce(q, bio);	
 }
 
-#ifdef CONFIG_BLK_CGROUP_IOLATENCY
-extern int blk_iolatency_init(struct request_queue *q);
-#else
-static inline int blk_iolatency_init(struct request_queue *q) { return 0; }
-#endif
-
 struct bio *blk_next_bio(struct bio *bio, unsigned int nr_pages, gfp_t gfp);
 
 #ifdef CONFIG_BLK_DEV_ZONED
-- 
2.17.1

Re: [RFC V4 3/6] blk-iolatency: make iolatency pluggable

[Christoph Hellwig]

> +static inline struct rq_qos *blkcg_rq_qos(struct request_queue *q)
> +{
> +	return rq_qos_by_id(q, blkcg_iolatency_ops.id);
> +}

This just has a single user, so open code it.

> +static int blk_iolatency_init(struct request_queue *q);
> +
>  static struct rq_qos_ops blkcg_iolatency_ops = {
> +	.name = "blk-iolat",
> +	.flags = RQOS_FLAG_CGRP_POL,
>  	.throttle = blkcg_iolatency_throttle,
>  	.done_bio = blkcg_iolatency_done_bio,
>  	.exit = blkcg_iolatency_exit,
> +	.init = blk_iolatency_init,
>  };

I'd move this structure below blk_iolatency_init to avoid the forward
declaration of blk_iolatency_init

Re: [RFC V4 3/6] blk-iolatency: make iolatency pluggable

[Wang Jianchao]

On 2022/2/17 4:51 下午, Christoph Hellwig wrote:
>> +static inline struct rq_qos *blkcg_rq_qos(struct request_queue *q)
>> +{
>> +	return rq_qos_by_id(q, blkcg_iolatency_ops.id);
>> +}
> 
> This just has a single user, so open code it.
> 
>> +static int blk_iolatency_init(struct request_queue *q);
>> +
>>  static struct rq_qos_ops blkcg_iolatency_ops = {
>> +	.name = "blk-iolat",
>> +	.flags = RQOS_FLAG_CGRP_POL,
>>  	.throttle = blkcg_iolatency_throttle,
>>  	.done_bio = blkcg_iolatency_done_bio,
>>  	.exit = blkcg_iolatency_exit,
>> +	.init = blk_iolatency_init,
>>  };
> 
> I'd move this structure below blk_iolatency_init to avoid the forward
> declaration of blk_iolatency_init

Got it

Thanks
Jianchao

[RFC V4 4/6] blk-iocost: make iocost pluggable

[Wang Jianchao (Kuaishou)]

Make blk-iocost pluggable. Then we can close or open it through
/sys/block/xxx/queue/qos.

Signed-off-by: Wang Jianchao (Kuaishou) <jianchao.wan9@gmail.com>
---
 block/blk-iocost.c     | 52 ++++++++++++++++++++++++++----------------
 block/blk-mq-debugfs.c |  2 --
 block/blk-rq-qos.h     |  1 -
 3 files changed, 32 insertions(+), 23 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 769b64394298..5a3a45985b49 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -660,9 +660,10 @@ static struct ioc *rqos_to_ioc(struct rq_qos *rqos)
 	return container_of(rqos, struct ioc, rqos);
 }
 
+static struct rq_qos_ops ioc_rqos_ops;
 static struct ioc *q_to_ioc(struct request_queue *q)
 {
-	return rqos_to_ioc(rq_qos_id(q, RQ_QOS_COST));
+	return rqos_to_ioc(rq_qos_by_id(q, ioc_rqos_ops.id));
 }
 
 static const char *q_name(struct request_queue *q)
@@ -2810,6 +2811,7 @@ static void ioc_rqos_exit(struct rq_qos *rqos)
 	struct ioc *ioc = rqos_to_ioc(rqos);
 
 	blkcg_deactivate_policy(rqos->q, &blkcg_policy_iocost);
+	rq_qos_deactivate(rqos);
 
 	spin_lock_irq(&ioc->lock);
 	ioc->running = IOC_STOP;
@@ -2820,13 +2822,18 @@ static void ioc_rqos_exit(struct rq_qos *rqos)
 	kfree(ioc);
 }
 
+static int blk_iocost_init(struct request_queue *q);
+
 static struct rq_qos_ops ioc_rqos_ops = {
+	.name = "blk-iocost",
+	.flags = RQOS_FLAG_CGRP_POL,
 	.throttle = ioc_rqos_throttle,
 	.merge = ioc_rqos_merge,
 	.done_bio = ioc_rqos_done_bio,
 	.done = ioc_rqos_done,
 	.queue_depth_changed = ioc_rqos_queue_depth_changed,
 	.exit = ioc_rqos_exit,
+	.init = blk_iocost_init,
 };
 
 static int blk_iocost_init(struct request_queue *q)
@@ -2856,10 +2863,7 @@ static int blk_iocost_init(struct request_queue *q)
 	}
 
 	rqos = &ioc->rqos;
-	rqos->id = RQ_QOS_COST;
-	rqos->ops = &ioc_rqos_ops;
-	rqos->q = q;
-
+	rq_qos_activate(q, rqos, &ioc_rqos_ops);
 	spin_lock_init(&ioc->lock);
 	timer_setup(&ioc->timer, ioc_timer_fn, 0);
 	INIT_LIST_HEAD(&ioc->active_iocgs);
@@ -2883,10 +2887,9 @@ static int blk_iocost_init(struct request_queue *q)
 	 * called before policy activation completion, can't assume that the
 	 * target bio has an iocg associated and need to test for NULL iocg.
 	 */
-	rq_qos_add(q, rqos);
 	ret = blkcg_activate_policy(q, &blkcg_policy_iocost);
 	if (ret) {
-		rq_qos_del(q, rqos);
+		rq_qos_deactivate(rqos);
 		free_percpu(ioc->pcpu_stat);
 		kfree(ioc);
 		return ret;
@@ -3162,6 +3165,7 @@ static ssize_t ioc_qos_write(struct kernfs_open_file *of, char *input,
 			     size_t nbytes, loff_t off)
 {
 	struct block_device *bdev;
+	struct rq_qos *rqos;
 	struct ioc *ioc;
 	u32 qos[NR_QOS_PARAMS];
 	bool enable, user;
@@ -3172,12 +3176,10 @@ static ssize_t ioc_qos_write(struct kernfs_open_file *of, char *input,
 	if (IS_ERR(bdev))
 		return PTR_ERR(bdev);
 
-	ioc = q_to_ioc(bdev_get_queue(bdev));
-	if (!ioc) {
-		ret = blk_iocost_init(bdev_get_queue(bdev));
-		if (ret)
-			goto err;
-		ioc = q_to_ioc(bdev_get_queue(bdev));
+	rqos = rq_qos_get(bdev_get_queue(bdev), ioc_rqos_ops.id);
+	if (!rqos) {
+		ret = -EOPNOTSUPP;
+		goto err;
 	}
 
 	spin_lock_irq(&ioc->lock);
@@ -3329,6 +3331,7 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
 				    size_t nbytes, loff_t off)
 {
 	struct block_device *bdev;
+	struct rq_qos *rqos;
 	struct ioc *ioc;
 	u64 u[NR_I_LCOEFS];
 	bool user;
@@ -3339,12 +3342,10 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
 	if (IS_ERR(bdev))
 		return PTR_ERR(bdev);
 
-	ioc = q_to_ioc(bdev_get_queue(bdev));
-	if (!ioc) {
-		ret = blk_iocost_init(bdev_get_queue(bdev));
-		if (ret)
-			goto err;
-		ioc = q_to_ioc(bdev_get_queue(bdev));
+	rqos = rq_qos_get(bdev_get_queue(bdev), ioc_rqos_ops.id);
+	if (!rqos) {
+		ret = -EOPNOTSUPP;
+		goto err;
 	}
 
 	spin_lock_irq(&ioc->lock);
@@ -3441,12 +3442,23 @@ static struct blkcg_policy blkcg_policy_iocost = {
 
 static int __init ioc_init(void)
 {
-	return blkcg_policy_register(&blkcg_policy_iocost);
+	int ret;
+
+	ret = rq_qos_register(&ioc_rqos_ops);
+	if (ret)
+		return ret;
+
+	ret = blkcg_policy_register(&blkcg_policy_iocost);
+	if (ret)
+		rq_qos_unregister(&ioc_rqos_ops);
+
+	return ret;
 }
 
 static void __exit ioc_exit(void)
 {
 	blkcg_policy_unregister(&blkcg_policy_iocost);
+	rq_qos_unregister(&ioc_rqos_ops);
 }
 
 module_init(ioc_init);
diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 918870b8de5b..45da42e9e242 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -826,8 +826,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_COST:
-		return "cost";
 	case RQ_QOS_IOPRIO:
 		return "ioprio";
 	}
diff --git a/block/blk-rq-qos.h b/block/blk-rq-qos.h
index 2a919db52fef..6d691527cb51 100644
--- a/block/blk-rq-qos.h
+++ b/block/blk-rq-qos.h
@@ -14,7 +14,6 @@
 struct blk_mq_debugfs_attr;
 
 enum rq_qos_id {
-	RQ_QOS_COST,
 	RQ_QOS_IOPRIO,
 };
 
-- 
2.17.1

Re: [RFC V4 4/6] blk-iocost: make iocost pluggable

[Christoph Hellwig]

On Thu, Feb 17, 2022 at 11:13:47AM +0800, Wang Jianchao (Kuaishou) wrote:
> Make blk-iocost pluggable. Then we can close or open it through
> /sys/block/xxx/queue/qos.
> 
> Signed-off-by: Wang Jianchao (Kuaishou) <jianchao.wan9@gmail.com>
> ---
>  block/blk-iocost.c     | 52 ++++++++++++++++++++++++++----------------
>  block/blk-mq-debugfs.c |  2 --
>  block/blk-rq-qos.h     |  1 -
>  3 files changed, 32 insertions(+), 23 deletions(-)
> 
> diff --git a/block/blk-iocost.c b/block/blk-iocost.c
> index 769b64394298..5a3a45985b49 100644
> --- a/block/blk-iocost.c
> +++ b/block/blk-iocost.c
> @@ -660,9 +660,10 @@ static struct ioc *rqos_to_ioc(struct rq_qos *rqos)
>  	return container_of(rqos, struct ioc, rqos);
>  }
>  
> +static struct rq_qos_ops ioc_rqos_ops;
>  static struct ioc *q_to_ioc(struct request_queue *q)
>  {
> -	return rqos_to_ioc(rq_qos_id(q, RQ_QOS_COST));
> +	return rqos_to_ioc(rq_qos_by_id(q, ioc_rqos_ops.id));
>  }

This has a single caller, so just open code it.

> +static int blk_iocost_init(struct request_queue *q);
> +
>  static struct rq_qos_ops ioc_rqos_ops = {
> +	.name = "blk-iocost",
> +	.flags = RQOS_FLAG_CGRP_POL,
>  	.throttle = ioc_rqos_throttle,
>  	.merge = ioc_rqos_merge,
>  	.done_bio = ioc_rqos_done_bio,
>  	.done = ioc_rqos_done,
>  	.queue_depth_changed = ioc_rqos_queue_depth_changed,
>  	.exit = ioc_rqos_exit,
> +	.init = blk_iocost_init,
>  };

Again, move rq_qos_ops below the init function to avoid the forward
declaration.

Re: [RFC V4 4/6] blk-iocost: make iocost pluggable

[Wang Jianchao]

On 2022/2/17 4:52 下午, Christoph Hellwig wrote:
> On Thu, Feb 17, 2022 at 11:13:47AM +0800, Wang Jianchao (Kuaishou) wrote:
>> Make blk-iocost pluggable. Then we can close or open it through
>> /sys/block/xxx/queue/qos.
>>
>> Signed-off-by: Wang Jianchao (Kuaishou) <jianchao.wan9@gmail.com>
>> ---
>>  block/blk-iocost.c     | 52 ++++++++++++++++++++++++++----------------
>>  block/blk-mq-debugfs.c |  2 --
>>  block/blk-rq-qos.h     |  1 -
>>  3 files changed, 32 insertions(+), 23 deletions(-)
>>
>> diff --git a/block/blk-iocost.c b/block/blk-iocost.c
>> index 769b64394298..5a3a45985b49 100644
>> --- a/block/blk-iocost.c
>> +++ b/block/blk-iocost.c
>> @@ -660,9 +660,10 @@ static struct ioc *rqos_to_ioc(struct rq_qos *rqos)
>>  	return container_of(rqos, struct ioc, rqos);
>>  }
>>  
>> +static struct rq_qos_ops ioc_rqos_ops;
>>  static struct ioc *q_to_ioc(struct request_queue *q)
>>  {
>> -	return rqos_to_ioc(rq_qos_id(q, RQ_QOS_COST));
>> +	return rqos_to_ioc(rq_qos_by_id(q, ioc_rqos_ops.id));
>>  }
> 
> This has a single caller, so just open code it.
> 
>> +static int blk_iocost_init(struct request_queue *q);
>> +
>>  static struct rq_qos_ops ioc_rqos_ops = {
>> +	.name = "blk-iocost",
>> +	.flags = RQOS_FLAG_CGRP_POL,
>>  	.throttle = ioc_rqos_throttle,
>>  	.merge = ioc_rqos_merge,
>>  	.done_bio = ioc_rqos_done_bio,
>>  	.done = ioc_rqos_done,
>>  	.queue_depth_changed = ioc_rqos_queue_depth_changed,
>>  	.exit = ioc_rqos_exit,
>> +	.init = blk_iocost_init,
>>  };
> 
> Again, move rq_qos_ops below the init function to avoid the forward
> declaration.

Got it

Thanks
Jianchao

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

[Wang Jianchao (Kuaishou)]

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. The blk-ioprio.h
is removed as we needn't to make blk_ioprio_init() external.

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

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/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..1ec56f617202 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,24 @@ 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	= "blk-ioprio",
 	.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 +242,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 45da42e9e242..cbbd668029a1 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -825,10 +825,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 db13581ae878..23cb7a3fa9b2 100644
--- a/block/blk-rq-qos.c
+++ b/block/blk-rq-qos.c
@@ -452,7 +452,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 6d691527cb51..bba829bbb461 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 {
-- 
2.17.1

Re: [RFC V4 5/6] blk-ioprio: make ioprio pluggable and modular

[Christoph Hellwig]

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

Same ordering comment as for the last ones.

[RFC V4 6/6] blk: export the sysfs for switching qos

[Wang Jianchao (Kuaishou)]

All of the blk-rq-qos policies has been changed to use the new
interfaces. Thus we can export the sysfs interface, namely
/sys/block/xxx/queue/qos and get rid of the unused interfaces.

Signed-off-by: Wang Jianchao (Kuaishou) <jianchao.wan9@gmail.com>
---
 block/blk-mq-debugfs.c | 10 +------
 block/blk-rq-qos.h     | 63 +-----------------------------------------
 block/blk-sysfs.c      |  2 ++
 3 files changed, 4 insertions(+), 71 deletions(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index cbbd668029a1..3defd5cb1cea 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -823,11 +823,6 @@ void blk_mq_debugfs_unregister_sched(struct request_queue *q)
 	q->sched_debugfs_dir = NULL;
 }
 
-static const char *rq_qos_id_to_name(enum rq_qos_id id)
-{
-	return "unknown";
-}
-
 void blk_mq_debugfs_unregister_rqos(struct rq_qos *rqos)
 {
 	debugfs_remove_recursive(rqos->debugfs_dir);
@@ -837,9 +832,6 @@ void blk_mq_debugfs_unregister_rqos(struct rq_qos *rqos)
 void blk_mq_debugfs_register_rqos(struct rq_qos *rqos)
 {
 	struct request_queue *q = rqos->q;
-	const char *dir_name;
-
-	dir_name = rqos->ops->name ? rqos->ops->name : rq_qos_id_to_name(rqos->id);
 
 	if (rqos->debugfs_dir || !rqos->ops->debugfs_attrs)
 		return;
@@ -848,7 +840,7 @@ void blk_mq_debugfs_register_rqos(struct rq_qos *rqos)
 		q->rqos_debugfs_dir = debugfs_create_dir("rqos",
 							 q->debugfs_dir);
 
-	rqos->debugfs_dir = debugfs_create_dir(dir_name,
+	rqos->debugfs_dir = debugfs_create_dir(rqos->ops->name,
 					       rqos->q->rqos_debugfs_dir);
 
 	debugfs_create_files(rqos->debugfs_dir, rqos, rqos->ops->debugfs_attrs);
diff --git a/block/blk-rq-qos.h b/block/blk-rq-qos.h
index bba829bbb461..262d221794f5 100644
--- a/block/blk-rq-qos.h
+++ b/block/blk-rq-qos.h
@@ -13,10 +13,6 @@
 
 struct blk_mq_debugfs_attr;
 
-enum rq_qos_id {
-	RQ_QOS_UNUSED,
-};
-
 struct rq_wait {
 	wait_queue_head_t wait;
 	atomic_t inflight;
@@ -25,7 +21,7 @@ struct rq_wait {
 struct rq_qos {
 	const struct rq_qos_ops *ops;
 	struct request_queue *q;
-	enum rq_qos_id id;
+	int id;
 	refcount_t ref;
 	wait_queue_head_t waitq;
 	bool dying;
@@ -69,17 +65,6 @@ struct rq_depth {
 	unsigned int default_depth;
 };
 
-static inline struct rq_qos *rq_qos_id(struct request_queue *q,
-				       enum rq_qos_id id)
-{
-	struct rq_qos *rqos;
-	for (rqos = q->rq_qos; rqos; rqos = rqos->next) {
-		if (rqos->id == id)
-			break;
-	}
-	return rqos;
-}
-
 static inline struct rq_qos *rq_qos_by_id(struct request_queue *q, int id)
 {
 	struct rq_qos *rqos;
@@ -99,52 +84,6 @@ static inline void rq_wait_init(struct rq_wait *rq_wait)
 	init_waitqueue_head(&rq_wait->wait);
 }
 
-static inline void rq_qos_add(struct request_queue *q, struct rq_qos *rqos)
-{
-	/*
-	 * No IO can be in-flight when adding rqos, so freeze queue, which
-	 * is fine since we only support rq_qos for blk-mq queue.
-	 *
-	 * Reuse ->queue_lock for protecting against other concurrent
-	 * rq_qos adding/deleting
-	 */
-	blk_mq_freeze_queue(q);
-
-	spin_lock_irq(&q->queue_lock);
-	rqos->next = q->rq_qos;
-	q->rq_qos = rqos;
-	spin_unlock_irq(&q->queue_lock);
-
-	blk_mq_unfreeze_queue(q);
-
-	if (rqos->ops->debugfs_attrs)
-		blk_mq_debugfs_register_rqos(rqos);
-}
-
-static inline void rq_qos_del(struct request_queue *q, struct rq_qos *rqos)
-{
-	struct rq_qos **cur;
-
-	/*
-	 * See comment in rq_qos_add() about freezing queue & using
-	 * ->queue_lock.
-	 */
-	blk_mq_freeze_queue(q);
-
-	spin_lock_irq(&q->queue_lock);
-	for (cur = &q->rq_qos; *cur; cur = &(*cur)->next) {
-		if (*cur == rqos) {
-			*cur = rqos->next;
-			break;
-		}
-	}
-	spin_unlock_irq(&q->queue_lock);
-
-	blk_mq_unfreeze_queue(q);
-
-	blk_mq_debugfs_unregister_rqos(rqos);
-}
-
 int rq_qos_register(struct rq_qos_ops *ops);
 void rq_qos_unregister(struct rq_qos_ops *ops);
 void rq_qos_activate(struct request_queue *q,
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 9f32882ceb2f..c02747db4e3b 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -574,6 +574,7 @@ QUEUE_RO_ENTRY(queue_max_segments, "max_segments");
 QUEUE_RO_ENTRY(queue_max_integrity_segments, "max_integrity_segments");
 QUEUE_RO_ENTRY(queue_max_segment_size, "max_segment_size");
 QUEUE_RW_ENTRY(elv_iosched, "scheduler");
+QUEUE_RW_ENTRY(queue_qos, "qos");
 
 QUEUE_RO_ENTRY(queue_logical_block_size, "logical_block_size");
 QUEUE_RO_ENTRY(queue_physical_block_size, "physical_block_size");
@@ -633,6 +634,7 @@ static struct attribute *queue_attrs[] = {
 	&queue_max_integrity_segments_entry.attr,
 	&queue_max_segment_size_entry.attr,
 	&elv_iosched_entry.attr,
+	&queue_qos_entry.attr,
 	&queue_hw_sector_size_entry.attr,
 	&queue_logical_block_size_entry.attr,
 	&queue_physical_block_size_entry.attr,
-- 
2.17.1

Re: [RFC V4 6/6] blk: export the sysfs for switching qos

[Christoph Hellwig]

On Thu, Feb 17, 2022 at 11:13:49AM +0800, Wang Jianchao (Kuaishou) wrote:
> -static const char *rq_qos_id_to_name(enum rq_qos_id id)
> -{
> -	return "unknown";
> -}

Please split the removal of dead code into a separate patch.

Re: [RFC V4 6/6] blk: export the sysfs for switching qos

[Wang Jianchao]

On 2022/2/17 4:55 下午, Christoph Hellwig wrote:
> On Thu, Feb 17, 2022 at 11:13:49AM +0800, Wang Jianchao (Kuaishou) wrote:
>> -static const char *rq_qos_id_to_name(enum rq_qos_id id)
>> -{
>> -	return "unknown";
>> -}
> 
> Please split the removal of dead code into a separate patch.

Got it

Thanks
Jianchao

Re: [RFC V4 0/6] blk: make blk-rq-qos policies pluggable and modular

[Jens Axboe]

On 2/16/22 8:13 PM, Wang Jianchao (Kuaishou) wrote:
> Hi Jens
> 
> blk-rq-qos is a standalone framework out of io-sched and can be used to
> control or observe the IO progress in block-layer with hooks. blk-rq-qos
> is a great design but right now, it is totally fixed and built-in and shut
> out peoples who want to use it with external module.
> 
> This patchset attempts to make blk-rq-qos framework pluggable and modular.
> Then we can update the blk-rq-qos policy module w/o stopping the IO workload.
> And it is more convenient to introduce new policy on old machines w/o udgrade
> kernel. And we can close all of the blk-rq-qos policy if we needn't any of
> them. At the moment, the request_queue.rqos list is empty, we needn't to
> waste cpu cyles on them.

I like this patchset, would be a lot more convenient and helps
efficiency.

What kind of testing have you done on it?

-- 
Jens Axboe

Re: [RFC V4 0/6] blk: make blk-rq-qos policies pluggable and modular

[Wang Jianchao]

On 2022/2/17 11:21 上午, Jens Axboe wrote:
> On 2/16/22 8:13 PM, Wang Jianchao (Kuaishou) wrote:
>> Hi Jens
>>
>> blk-rq-qos is a standalone framework out of io-sched and can be used to
>> control or observe the IO progress in block-layer with hooks. blk-rq-qos
>> is a great design but right now, it is totally fixed and built-in and shut
>> out peoples who want to use it with external module.
>>
>> This patchset attempts to make blk-rq-qos framework pluggable and modular.
>> Then we can update the blk-rq-qos policy module w/o stopping the IO workload.
>> And it is more convenient to introduce new policy on old machines w/o udgrade
>> kernel. And we can close all of the blk-rq-qos policy if we needn't any of
>> them. At the moment, the request_queue.rqos list is empty, we needn't to
>> waste cpu cyles on them.
> 
> I like this patchset, would be a lot more convenient and helps
> efficiency.
> 
> What kind of testing have you done on it?
> 
I have run blktests against this patchset in a VM machine along with a new test
case for switching rqos policies while running IO. 

https://marc.info/?l=linux-block&m=164506848620632&w=2

And also there is a bit different version running on v4.18 with more tests

Thanks
Jianchao

Re: [RFC V4 0/6] blk: make blk-rq-qos policies pluggable and modular

[Christoph Hellwig]

On Thu, Feb 17, 2022 at 11:13:43AM +0800, Wang Jianchao (Kuaishou) wrote:
> Hi Jens
> 
> blk-rq-qos is a standalone framework out of io-sched and can be used to
> control or observe the IO progress in block-layer with hooks. blk-rq-qos
> is a great design but right now, it is totally fixed and built-in and shut
> out peoples who want to use it with external module.

And that's got.  External modules do not matter.

That being said this series has a bunch of nice cleanups, but we really
do not need the exports and modular build.

Re: [RFC V4 0/6] blk: make blk-rq-qos policies pluggable and modular

[Wang Jianchao]

On 2022/2/17 4:43 下午, Christoph Hellwig wrote:
> On Thu, Feb 17, 2022 at 11:13:43AM +0800, Wang Jianchao (Kuaishou) wrote:
>> Hi Jens
>>
>> blk-rq-qos is a standalone framework out of io-sched and can be used to
>> control or observe the IO progress in block-layer with hooks. blk-rq-qos
>> is a great design but right now, it is totally fixed and built-in and shut
>> out peoples who want to use it with external module.
> 
> And that's got.  External modules do not matter.
> 
> That being said this series has a bunch of nice cleanups, but we really
> do not need the exports and modular build.

Got it.
I will get rid of the code for supporting external module in next patch.

Thanks
Jianchao