[PATCH 0/8 v3] block: Fix IO priority mess

[PATCH 0/8 v3] block: Fix IO priority mess

[Jan Kara]

Hello,

This is the third revision of my patches fixing IO priority handling in the
block layer.

Changes since v2:
* added some comments to better explain things
* changed handling of ioprio_get(2)
* a few small tweaks based on Damien's feedback

Original cover letter:
Recently, I've been looking into 10% regression reported by our performance
measurement infrastructure in reaim benchmark that was bisected down to
5a9d041ba2f6 ("block: move io_context creation into where it's needed"). This
didn't really make much sense and it took me a while to understand this but the
culprit is actually in even older commit e70344c05995 ("block: fix default IO
priority handling") and 5a9d041ba2f6 just made the breakage visible.
Essentially the problem was that after these commits some IO was queued with IO
priority class IOPRIO_CLASS_BE while other IO was queued with IOPRIO_CLASS_NONE
and as a result they could not be merged together resulting in performance
regression. I think what commit e70344c05995 ("block: fix default IO priority
handling") did is actually broken not only because of this performance
regression but because of other reasons as well (see changelog of patch 3/8 for
details). Besides this problem, there are multiple other inconsistencies in the
IO priority handling throughout the block stack we have identified when
discussing this with Damien Le Moal. So this patch set aims at fixing all these
various issues.

Note that there are a few choices I've made I'm not 100% sold on. In particular
the conversion of blk-ioprio from rqos is somewhat disputable since it now
incurs a cost similar to blk-throttle in the bio submission fast path (need to
load bio->bi_blkg->pd[ioprio_policy.plid]).  If people think the removed
boilerplate code is not worth the cost, I can certainly go via the "additional
rqos hook" path.

								Honza
Previous versions:
Link: http://lore.kernel.org/r/20220601132347.13543-1-jack@suse.cz # v1
Link: http://lore.kernel.org/r/20220615160437.5478-1-jack@suse.cz # v2

[PATCH 1/8] block: fix default IO priority handling again

[Jan Kara]

Commit e70344c05995 ("block: fix default IO priority handling")
introduced an inconsistency in get_current_ioprio() that tasks without
IO context return IOPRIO_DEFAULT priority while tasks with freshly
allocated IO context will return 0 (IOPRIO_CLASS_NONE/0) IO priority.
Tasks without IO context used to be rare before 5a9d041ba2f6 ("block:
move io_context creation into where it's needed") but after this commit
they became common because now only BFQ IO scheduler setups task's IO
context. Similar inconsistency is there for get_task_ioprio() so this
inconsistency is now exposed to userspace and userspace will see
different IO priority for tasks operating on devices with BFQ compared
to devices without BFQ. Furthemore the changes done by commit
e70344c05995 change the behavior when no IO priority is set for BFQ IO
scheduler which is also documented in ioprio_set(2) manpage:

"If no I/O scheduler has been set for a thread, then by default the I/O
priority will follow the CPU nice value (setpriority(2)).  In Linux
kernels before version 2.6.24, once an I/O priority had been set using
ioprio_set(), there was no way to reset the I/O scheduling behavior to
the default. Since Linux 2.6.24, specifying ioprio as 0 can be used to
reset to the default I/O scheduling behavior."

So make sure we default to IOPRIO_CLASS_NONE as used to be the case
before commit e70344c05995. Also cleanup alloc_io_context() to
explicitely set this IO priority for the allocated IO context to avoid
future surprises. Note that we tweak ioprio_best() to maintain
ioprio_get(2) behavior and make this commit easily backportable.

Fixes: e70344c05995 ("block: fix default IO priority handling")
Signed-off-by: Jan Kara <jack@suse.cz>
---
 block/blk-ioc.c        | 2 ++
 block/ioprio.c         | 4 ++--
 include/linux/ioprio.h | 2 +-
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index df9cfe4ca532..63fc02042408 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -247,6 +247,8 @@ static struct io_context *alloc_io_context(gfp_t gfp_flags, int node)
 	INIT_HLIST_HEAD(&ioc->icq_list);
 	INIT_WORK(&ioc->release_work, ioc_release_fn);
 #endif
+	ioc->ioprio = IOPRIO_DEFAULT;
+
 	return ioc;
 }
 
diff --git a/block/ioprio.c b/block/ioprio.c
index 2fe068fcaad5..2a34cbca18ae 100644
--- a/block/ioprio.c
+++ b/block/ioprio.c
@@ -157,9 +157,9 @@ static int get_task_ioprio(struct task_struct *p)
 int ioprio_best(unsigned short aprio, unsigned short bprio)
 {
 	if (!ioprio_valid(aprio))
-		aprio = IOPRIO_DEFAULT;
+		aprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_BE_NORM);
 	if (!ioprio_valid(bprio))
-		bprio = IOPRIO_DEFAULT;
+		bprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_BE_NORM);
 
 	return min(aprio, bprio);
 }
diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
index 3f53bc27a19b..3d088a88f832 100644
--- a/include/linux/ioprio.h
+++ b/include/linux/ioprio.h
@@ -11,7 +11,7 @@
 /*
  * Default IO priority.
  */
-#define IOPRIO_DEFAULT	IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_BE_NORM)
+#define IOPRIO_DEFAULT	IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0)
 
 /*
  * Check that a priority value has a valid class.
-- 
2.35.3

[PATCH 2/8] block: Return effective IO priority from get_current_ioprio()

[Jan Kara]

get_current_ioprio() is used to initialize IO priority of various
requests. As such it should be returning the effective IO priority of
the task (i.e., reflecting the fact that unset IO priority should get
set based on task's CPU priority) so that the conversion is concentrated
in one place.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 include/linux/ioprio.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
index 3d088a88f832..61ed6bb4998e 100644
--- a/include/linux/ioprio.h
+++ b/include/linux/ioprio.h
@@ -53,10 +53,17 @@ static inline int task_nice_ioclass(struct task_struct *task)
 static inline int get_current_ioprio(void)
 {
 	struct io_context *ioc = current->io_context;
+	int prio;
 
 	if (ioc)
-		return ioc->ioprio;
-	return IOPRIO_DEFAULT;
+		prio = ioc->ioprio;
+	else
+		prio = IOPRIO_DEFAULT;
+
+	if (IOPRIO_PRIO_CLASS(prio) == IOPRIO_CLASS_NONE)
+		prio = IOPRIO_PRIO_VALUE(task_nice_ioclass(current),
+					 task_nice_ioprio(current));
+	return prio;
 }
 
 /*
-- 
2.35.3

[PATCH 3/8] block: Make ioprio_best() static

[Jan Kara]

Nobody outside of block/ioprio.c uses it.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 block/ioprio.c         | 2 +-
 include/linux/ioprio.h | 5 -----
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/block/ioprio.c b/block/ioprio.c
index 2a34cbca18ae..b5cf7339709b 100644
--- a/block/ioprio.c
+++ b/block/ioprio.c
@@ -154,7 +154,7 @@ static int get_task_ioprio(struct task_struct *p)
 	return ret;
 }
 
-int ioprio_best(unsigned short aprio, unsigned short bprio)
+static int ioprio_best(unsigned short aprio, unsigned short bprio)
 {
 	if (!ioprio_valid(aprio))
 		aprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_BE_NORM);
diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
index 61ed6bb4998e..519d51fc8902 100644
--- a/include/linux/ioprio.h
+++ b/include/linux/ioprio.h
@@ -66,11 +66,6 @@ static inline int get_current_ioprio(void)
 	return prio;
 }
 
-/*
- * For inheritance, return the highest of the two given priorities
- */
-extern int ioprio_best(unsigned short aprio, unsigned short bprio);
-
 extern int set_task_ioprio(struct task_struct *task, int ioprio);
 
 #ifdef CONFIG_BLOCK
-- 
2.35.3

[PATCH 4/8] block: Fix handling of tasks without ioprio in ioprio_get(2)

[Jan Kara]

ioprio_get(2) can be asked to return the best IO priority from several
tasks (IOPRIO_WHO_PGRP, IOPRIO_WHO_USER). Currently the call treats
tasks without set IO priority as having priority
IOPRIO_CLASS_BE/IOPRIO_BE_NORM however this does not really reflect the
IO priority the task will get (which depends on task's nice value).

Fix the code to use the real IO priority task's IO will use. For this we
do some factoring out to share the code converting task's CPU priority
to IO priority and we also have to modify code for
ioprio_get(IOPRIO_WHO_PROCESS) to keep returning IOPRIO_CLASS_NONE
priority for tasks without set IO priority as a special case to maintain
userspace visible API.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 block/ioprio.c         | 49 ++++++++++++++++++++++++++++++++++++------
 include/linux/ioprio.h | 19 +++-------------
 2 files changed, 45 insertions(+), 23 deletions(-)

diff --git a/block/ioprio.c b/block/ioprio.c
index b5cf7339709b..a4c19ce0de4c 100644
--- a/block/ioprio.c
+++ b/block/ioprio.c
@@ -138,6 +138,27 @@ SYSCALL_DEFINE3(ioprio_set, int, which, int, who, int, ioprio)
 	return ret;
 }
 
+/*
+ * If the task has set an I/O priority, use that. Otherwise, return
+ * the default I/O priority.
+ */
+int __get_task_ioprio(struct task_struct *p)
+{
+	struct io_context *ioc = p->io_context;
+	int prio;
+
+	if (ioc)
+		prio = ioc->ioprio;
+	else
+		prio = IOPRIO_DEFAULT;
+
+	if (IOPRIO_PRIO_CLASS(prio) == IOPRIO_CLASS_NONE)
+		prio = IOPRIO_PRIO_VALUE(task_nice_ioclass(p),
+					 task_nice_ioprio(p));
+	return prio;
+}
+EXPORT_SYMBOL_GPL(__get_task_ioprio);
+
 static int get_task_ioprio(struct task_struct *p)
 {
 	int ret;
@@ -145,10 +166,29 @@ static int get_task_ioprio(struct task_struct *p)
 	ret = security_task_getioprio(p);
 	if (ret)
 		goto out;
-	ret = IOPRIO_DEFAULT;
+	task_lock(p);
+	ret = __get_task_ioprio(p);
+	task_unlock(p);
+out:
+	return ret;
+}
+
+/*
+ * Return raw IO priority value as set by userspace. We use this for
+ * ioprio_get(pid, IOPRIO_WHO_PROCESS) so that we keep historical behavior and
+ * also so that userspace can distinguish unset IO priority (which just gets
+ * overriden based on task's nice value) from IO priority set to some value.
+ */
+static int get_task_raw_ioprio(struct task_struct *p) { int ret;
+
+	ret = security_task_getioprio(p);
+	if (ret)
+		goto out;
 	task_lock(p);
 	if (p->io_context)
 		ret = p->io_context->ioprio;
+	else
+		ret = IOPRIO_DEFAULT;
 	task_unlock(p);
 out:
 	return ret;
@@ -156,11 +196,6 @@ static int get_task_ioprio(struct task_struct *p)
 
 static int ioprio_best(unsigned short aprio, unsigned short bprio)
 {
-	if (!ioprio_valid(aprio))
-		aprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_BE_NORM);
-	if (!ioprio_valid(bprio))
-		bprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_BE_NORM);
-
 	return min(aprio, bprio);
 }
 
@@ -181,7 +216,7 @@ SYSCALL_DEFINE2(ioprio_get, int, which, int, who)
 			else
 				p = find_task_by_vpid(who);
 			if (p)
-				ret = get_task_ioprio(p);
+				ret = get_task_raw_ioprio(p);
 			break;
 		case IOPRIO_WHO_PGRP:
 			if (!who)
diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
index 519d51fc8902..24e648dc4fb3 100644
--- a/include/linux/ioprio.h
+++ b/include/linux/ioprio.h
@@ -46,24 +46,11 @@ static inline int task_nice_ioclass(struct task_struct *task)
 		return IOPRIO_CLASS_BE;
 }
 
-/*
- * If the calling process has set an I/O priority, use that. Otherwise, return
- * the default I/O priority.
- */
+int __get_task_ioprio(struct task_struct *p);
+
 static inline int get_current_ioprio(void)
 {
-	struct io_context *ioc = current->io_context;
-	int prio;
-
-	if (ioc)
-		prio = ioc->ioprio;
-	else
-		prio = IOPRIO_DEFAULT;
-
-	if (IOPRIO_PRIO_CLASS(prio) == IOPRIO_CLASS_NONE)
-		prio = IOPRIO_PRIO_VALUE(task_nice_ioclass(current),
-					 task_nice_ioprio(current));
-	return prio;
+	return __get_task_ioprio(current);
 }
 
 extern int set_task_ioprio(struct task_struct *task, int ioprio);
-- 
2.35.3

[PATCH 5/8] blk-ioprio: Remove unneeded field

[Jan Kara]

blkcg->ioprio_set field is not really useful except for avoiding
possibly more expensive checks inside blkcg_ioprio_track(). The check
for blkcg->prio_policy being equal to POLICY_NO_CHANGE does the same
service so just remove the ioprio_set field and replace the check.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 block/blk-ioprio.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/block/blk-ioprio.c b/block/blk-ioprio.c
index 79e797f5d194..3f605583598b 100644
--- a/block/blk-ioprio.c
+++ b/block/blk-ioprio.c
@@ -62,7 +62,6 @@ struct ioprio_blkg {
 struct ioprio_blkcg {
 	struct blkcg_policy_data cpd;
 	enum prio_policy	 prio_policy;
-	bool			 prio_set;
 };
 
 static inline struct ioprio_blkg *pd_to_ioprio(struct blkg_policy_data *pd)
@@ -113,7 +112,6 @@ static ssize_t ioprio_set_prio_policy(struct kernfs_open_file *of, char *buf,
 	if (ret < 0)
 		return ret;
 	blkcg->prio_policy = ret;
-	blkcg->prio_set = true;
 	return nbytes;
 }
 
@@ -193,16 +191,15 @@ static void blkcg_ioprio_track(struct rq_qos *rqos, struct request *rq,
 	struct ioprio_blkcg *blkcg = ioprio_blkcg_from_bio(bio);
 	u16 prio;
 
-	if (!blkcg->prio_set)
+	if (blkcg->prio_policy == POLICY_NO_CHANGE)
 		return;
 
 	/*
 	 * Except for IOPRIO_CLASS_NONE, higher I/O priority numbers
 	 * correspond to a lower priority. Hence, the max_t() below selects
 	 * the lower priority of bi_ioprio and the cgroup I/O priority class.
-	 * If the cgroup policy has been set to POLICY_NO_CHANGE == 0, the
-	 * bio I/O priority is not modified. If the bio I/O priority equals
-	 * IOPRIO_CLASS_NONE, the cgroup I/O priority is assigned to the bio.
+	 * If the bio I/O priority equals IOPRIO_CLASS_NONE, the cgroup I/O
+	 * priority is assigned to the bio.
 	 */
 	prio = max_t(u16, bio->bi_ioprio,
 			IOPRIO_PRIO_VALUE(blkcg->prio_policy, 0));
-- 
2.35.3

[PATCH 6/8] blk-ioprio: Convert from rqos policy to direct call

[Jan Kara]

Convert blk-ioprio handling from a rqos policy to a direct call from
blk_mq_submit_bio(). Firstly, blk-ioprio is not a much of a rqos policy
anyway, it just needs a hook in bio submission path to set the bio's IO
priority. Secondly, the rqos .track hook gets actually called too late
for blk-ioprio purposes and introducing a special rqos hook just for
blk-ioprio looks even weirder.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 block/blk-cgroup.c |  1 +
 block/blk-ioprio.c | 50 +++++-----------------------------------------
 block/blk-ioprio.h |  9 +++++++++
 block/blk-mq.c     |  8 ++++++++
 4 files changed, 23 insertions(+), 45 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 764e740b0c0f..6906981563f8 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1299,6 +1299,7 @@ int blkcg_init_queue(struct request_queue *q)
 	ret = blk_iolatency_init(q);
 	if (ret) {
 		blk_throtl_exit(q);
+		blk_ioprio_exit(q);
 		goto err_destroy_all;
 	}
 
diff --git a/block/blk-ioprio.c b/block/blk-ioprio.c
index 3f605583598b..c00060a02c6e 100644
--- a/block/blk-ioprio.c
+++ b/block/blk-ioprio.c
@@ -181,17 +181,12 @@ static struct blkcg_policy ioprio_policy = {
 	.pd_free_fn	= ioprio_free_pd,
 };
 
-struct blk_ioprio {
-	struct rq_qos rqos;
-};
-
-static void blkcg_ioprio_track(struct rq_qos *rqos, struct request *rq,
-			       struct bio *bio)
+void blkcg_set_ioprio(struct bio *bio)
 {
 	struct ioprio_blkcg *blkcg = ioprio_blkcg_from_bio(bio);
 	u16 prio;
 
-	if (blkcg->prio_policy == POLICY_NO_CHANGE)
+	if (!blkcg || blkcg->prio_policy == POLICY_NO_CHANGE)
 		return;
 
 	/*
@@ -207,49 +202,14 @@ static void blkcg_ioprio_track(struct rq_qos *rqos, struct request *rq,
 		bio->bi_ioprio = prio;
 }
 
-static void blkcg_ioprio_exit(struct rq_qos *rqos)
+void blk_ioprio_exit(struct request_queue *q)
 {
-	struct blk_ioprio *blkioprio_blkg =
-		container_of(rqos, typeof(*blkioprio_blkg), rqos);
-
-	blkcg_deactivate_policy(rqos->q, &ioprio_policy);
-	kfree(blkioprio_blkg);
+	blkcg_deactivate_policy(q, &ioprio_policy);
 }
 
-static struct rq_qos_ops blkcg_ioprio_ops = {
-	.track	= blkcg_ioprio_track,
-	.exit	= blkcg_ioprio_exit,
-};
-
 int blk_ioprio_init(struct request_queue *q)
 {
-	struct blk_ioprio *blkioprio_blkg;
-	struct rq_qos *rqos;
-	int ret;
-
-	blkioprio_blkg = kzalloc(sizeof(*blkioprio_blkg), GFP_KERNEL);
-	if (!blkioprio_blkg)
-		return -ENOMEM;
-
-	ret = blkcg_activate_policy(q, &ioprio_policy);
-	if (ret) {
-		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 blkcg_activate_policy(q, &ioprio_policy);
 }
 
 static int __init ioprio_init(void)
diff --git a/block/blk-ioprio.h b/block/blk-ioprio.h
index a7785c2f1aea..5a1eb550e178 100644
--- a/block/blk-ioprio.h
+++ b/block/blk-ioprio.h
@@ -6,14 +6,23 @@
 #include <linux/kconfig.h>
 
 struct request_queue;
+struct bio;
 
 #ifdef CONFIG_BLK_CGROUP_IOPRIO
 int blk_ioprio_init(struct request_queue *q);
+void blk_ioprio_exit(struct request_queue *q);
+void blkcg_set_ioprio(struct bio *bio);
 #else
 static inline int blk_ioprio_init(struct request_queue *q)
 {
 	return 0;
 }
+static inline void blk_ioprio_exit(struct request_queue *q)
+{
+}
+static inline void blkcg_set_ioprio(struct bio *bio)
+{
+}
 #endif
 
 #endif /* _BLK_IOPRIO_H_ */
diff --git a/block/blk-mq.c b/block/blk-mq.c
index e9bf950983c7..67a7bfa58b7c 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -42,6 +42,7 @@
 #include "blk-stat.h"
 #include "blk-mq-sched.h"
 #include "blk-rq-qos.h"
+#include "blk-ioprio.h"
 
 static DEFINE_PER_CPU(struct llist_head, blk_cpu_done);
 
@@ -2790,6 +2791,11 @@ static inline struct request *blk_mq_get_cached_request(struct request_queue *q,
 	return rq;
 }
 
+static void bio_set_ioprio(struct bio *bio)
+{
+	blkcg_set_ioprio(bio);
+}
+
 /**
  * blk_mq_submit_bio - Create and send a request to block device.
  * @bio: Bio pointer.
@@ -2830,6 +2836,8 @@ void blk_mq_submit_bio(struct bio *bio)
 
 	trace_block_getrq(bio);
 
+	bio_set_ioprio(bio);
+
 	rq_qos_track(q, rq, bio);
 
 	blk_mq_bio_to_request(rq, bio, nr_segs);
-- 
2.35.3

[PATCH 7/8] block: Initialize bio priority earlier

[Jan Kara]

Bio's IO priority needs to be initialized before we try to merge the bio
with other bios. Otherwise we could merge bios which would otherwise
receive different IO priorities leading to possible QoS issues.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 block/blk-mq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 67a7bfa58b7c..e17d822e6051 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2825,6 +2825,8 @@ void blk_mq_submit_bio(struct bio *bio)
 	if (!bio_integrity_prep(bio))
 		return;
 
+	bio_set_ioprio(bio);
+
 	rq = blk_mq_get_cached_request(q, plug, &bio, nr_segs);
 	if (!rq) {
 		if (!bio)
@@ -2836,8 +2838,6 @@ void blk_mq_submit_bio(struct bio *bio)
 
 	trace_block_getrq(bio);
 
-	bio_set_ioprio(bio);
-
 	rq_qos_track(q, rq, bio);
 
 	blk_mq_bio_to_request(rq, bio, nr_segs);
-- 
2.35.3

[PATCH 8/8] block: Always initialize bio IO priority on submit

[Jan Kara]

Currently, IO priority set in task's IO context is not reflected in the
bio->bi_ioprio for most IO (only io_uring and direct IO set it). This
results in odd results where process is submitting some bios with one
priority and other bios with a different (unset) priority and due to
differing priorities bios cannot be merged. Make sure bio->bi_ioprio is
always set on bio submission.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 block/blk-mq.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index e17d822e6051..7548f8aebea8 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2793,6 +2793,9 @@ static inline struct request *blk_mq_get_cached_request(struct request_queue *q,
 
 static void bio_set_ioprio(struct bio *bio)
 {
+	/* Nobody set ioprio so far? Initialize it based on task's nice value */
+	if (IOPRIO_PRIO_CLASS(bio->bi_ioprio) == IOPRIO_CLASS_NONE)
+		bio->bi_ioprio = get_current_ioprio();
 	blkcg_set_ioprio(bio);
 }
 
-- 
2.35.3

Re: [PATCH 4/8] block: Fix handling of tasks without ioprio in ioprio_get(2)

[kernel test robot]

Hi Jan,

I love your patch! Yet something to improve:

[auto build test ERROR on axboe-block/for-next]
[also build test ERROR on linus/master v5.19-rc2 next-20220617]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Jan-Kara/block-Fix-IO-priority-mess/20220621-001427
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: s390-randconfig-r044-20220620 (https://download.01.org/0day-ci/archive/20220621/202206210431.uTcCuUoS-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project af6d2a0b6825e71965f3e2701a63c239fa0ad70f)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install s390 cross compiling tool for clang build
        # apt-get install binutils-s390x-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/73f39284cec50db7a3e973a9a4ed56f7f706dd1b
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Jan-Kara/block-Fix-IO-priority-mess/20220621-001427
        git checkout 73f39284cec50db7a3e973a9a4ed56f7f706dd1b
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   /opt/cross/gcc-11.3.0-nolibc/s390x-linux/bin/s390x-linux-ld: fs/read_write.o: in function `get_current_ioprio':
>> include/linux/ioprio.h:53: undefined reference to `__get_task_ioprio'
>> /opt/cross/gcc-11.3.0-nolibc/s390x-linux/bin/s390x-linux-ld: include/linux/ioprio.h:53: undefined reference to `__get_task_ioprio'
>> /opt/cross/gcc-11.3.0-nolibc/s390x-linux/bin/s390x-linux-ld: include/linux/ioprio.h:53: undefined reference to `__get_task_ioprio'
>> /opt/cross/gcc-11.3.0-nolibc/s390x-linux/bin/s390x-linux-ld: include/linux/ioprio.h:53: undefined reference to `__get_task_ioprio'
>> /opt/cross/gcc-11.3.0-nolibc/s390x-linux/bin/s390x-linux-ld: include/linux/ioprio.h:53: undefined reference to `__get_task_ioprio'
   /opt/cross/gcc-11.3.0-nolibc/s390x-linux/bin/s390x-linux-ld: fs/seq_file.o:include/linux/ioprio.h:53: more undefined references to `__get_task_ioprio' follow


vim +53 include/linux/ioprio.h

    50	
    51	static inline int get_current_ioprio(void)
    52	{
  > 53		return __get_task_ioprio(current);
    54	}
    55	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 4/8] block: Fix handling of tasks without ioprio in ioprio_get(2)

[kernel test robot]

Hi Jan,

I love your patch! Yet something to improve:

[auto build test ERROR on axboe-block/for-next]
[also build test ERROR on linus/master v5.19-rc2 next-20220617]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Jan-Kara/block-Fix-IO-priority-mess/20220621-001427
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: x86_64-randconfig-a003-20220620 (https://download.01.org/0day-ci/archive/20220621/202206210442.Th4HzcuP-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/73f39284cec50db7a3e973a9a4ed56f7f706dd1b
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Jan-Kara/block-Fix-IO-priority-mess/20220621-001427
        git checkout 73f39284cec50db7a3e973a9a4ed56f7f706dd1b
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ld: vmlinux.o: in function `get_current_ioprio':
>> include/linux/ioprio.h:53: undefined reference to `__get_task_ioprio'
>> ld: include/linux/ioprio.h:53: undefined reference to `__get_task_ioprio'
>> ld: include/linux/ioprio.h:53: undefined reference to `__get_task_ioprio'
>> ld: include/linux/ioprio.h:53: undefined reference to `__get_task_ioprio'
>> ld: include/linux/ioprio.h:53: undefined reference to `__get_task_ioprio'


vim +53 include/linux/ioprio.h

    50	
    51	static inline int get_current_ioprio(void)
    52	{
  > 53		return __get_task_ioprio(current);
    54	}
    55	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 4/8] block: Fix handling of tasks without ioprio in ioprio_get(2)

[kernel test robot]

Hi Jan,

I love your patch! Yet something to improve:

[auto build test ERROR on axboe-block/for-next]
[also build test ERROR on linus/master v5.19-rc2 next-20220617]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Jan-Kara/block-Fix-IO-priority-mess/20220621-001427
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: riscv-nommu_k210_defconfig (https://download.01.org/0day-ci/archive/20220621/202206210522.eVU8S1DL-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/73f39284cec50db7a3e973a9a4ed56f7f706dd1b
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Jan-Kara/block-Fix-IO-priority-mess/20220621-001427
        git checkout 73f39284cec50db7a3e973a9a4ed56f7f706dd1b
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   riscv64-linux-ld: fs/read_write.o: in function `.L10':
   read_write.c:(.text+0x80): undefined reference to `__get_task_ioprio'
   riscv64-linux-ld: fs/seq_file.o: in function `seq_read':
>> seq_file.c:(.text+0x458): undefined reference to `__get_task_ioprio'
   riscv64-linux-ld: fs/splice.o: in function `.L171':
>> splice.c:(.text+0x8c6): undefined reference to `__get_task_ioprio'

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 1/8] block: fix default IO priority handling again

[Damien Le Moal]

On 6/21/22 01:11, Jan Kara wrote:
> Commit e70344c05995 ("block: fix default IO priority handling")
> introduced an inconsistency in get_current_ioprio() that tasks without
> IO context return IOPRIO_DEFAULT priority while tasks with freshly
> allocated IO context will return 0 (IOPRIO_CLASS_NONE/0) IO priority.
> Tasks without IO context used to be rare before 5a9d041ba2f6 ("block:
> move io_context creation into where it's needed") but after this commit
> they became common because now only BFQ IO scheduler setups task's IO
> context. Similar inconsistency is there for get_task_ioprio() so this
> inconsistency is now exposed to userspace and userspace will see
> different IO priority for tasks operating on devices with BFQ compared
> to devices without BFQ. Furthemore the changes done by commit
> e70344c05995 change the behavior when no IO priority is set for BFQ IO
> scheduler which is also documented in ioprio_set(2) manpage:
> 
> "If no I/O scheduler has been set for a thread, then by default the I/O
> priority will follow the CPU nice value (setpriority(2)).  In Linux
> kernels before version 2.6.24, once an I/O priority had been set using
> ioprio_set(), there was no way to reset the I/O scheduling behavior to
> the default. Since Linux 2.6.24, specifying ioprio as 0 can be used to
> reset to the default I/O scheduling behavior."
> 
> So make sure we default to IOPRIO_CLASS_NONE as used to be the case
> before commit e70344c05995. Also cleanup alloc_io_context() to
> explicitely set this IO priority for the allocated IO context to avoid
> future surprises. Note that we tweak ioprio_best() to maintain
> ioprio_get(2) behavior and make this commit easily backportable.
> 
> Fixes: e70344c05995 ("block: fix default IO priority handling")
> Signed-off-by: Jan Kara <jack@suse.cz>

Looks good to me.

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

> ---
>  block/blk-ioc.c        | 2 ++
>  block/ioprio.c         | 4 ++--
>  include/linux/ioprio.h | 2 +-
>  3 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/block/blk-ioc.c b/block/blk-ioc.c
> index df9cfe4ca532..63fc02042408 100644
> --- a/block/blk-ioc.c
> +++ b/block/blk-ioc.c
> @@ -247,6 +247,8 @@ static struct io_context *alloc_io_context(gfp_t gfp_flags, int node)
>  	INIT_HLIST_HEAD(&ioc->icq_list);
>  	INIT_WORK(&ioc->release_work, ioc_release_fn);
>  #endif
> +	ioc->ioprio = IOPRIO_DEFAULT;
> +
>  	return ioc;
>  }
>  
> diff --git a/block/ioprio.c b/block/ioprio.c
> index 2fe068fcaad5..2a34cbca18ae 100644
> --- a/block/ioprio.c
> +++ b/block/ioprio.c
> @@ -157,9 +157,9 @@ static int get_task_ioprio(struct task_struct *p)
>  int ioprio_best(unsigned short aprio, unsigned short bprio)
>  {
>  	if (!ioprio_valid(aprio))
> -		aprio = IOPRIO_DEFAULT;
> +		aprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_BE_NORM);
>  	if (!ioprio_valid(bprio))
> -		bprio = IOPRIO_DEFAULT;
> +		bprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_BE_NORM);
>  
>  	return min(aprio, bprio);
>  }
> diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
> index 3f53bc27a19b..3d088a88f832 100644
> --- a/include/linux/ioprio.h
> +++ b/include/linux/ioprio.h
> @@ -11,7 +11,7 @@
>  /*
>   * Default IO priority.
>   */
> -#define IOPRIO_DEFAULT	IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_BE_NORM)
> +#define IOPRIO_DEFAULT	IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0)
>  
>  /*
>   * Check that a priority value has a valid class.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/8] block: Return effective IO priority from get_current_ioprio()

[Damien Le Moal]

On 6/21/22 01:11, Jan Kara wrote:
> get_current_ioprio() is used to initialize IO priority of various
> requests. As such it should be returning the effective IO priority of
> the task (i.e., reflecting the fact that unset IO priority should get
> set based on task's CPU priority) so that the conversion is concentrated
> in one place.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  include/linux/ioprio.h | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
> index 3d088a88f832..61ed6bb4998e 100644
> --- a/include/linux/ioprio.h
> +++ b/include/linux/ioprio.h
> @@ -53,10 +53,17 @@ static inline int task_nice_ioclass(struct task_struct *task)
>  static inline int get_current_ioprio(void)
>  {
>  	struct io_context *ioc = current->io_context;
> +	int prio;
>  
>  	if (ioc)
> -		return ioc->ioprio;
> -	return IOPRIO_DEFAULT;
> +		prio = ioc->ioprio;
> +	else
> +		prio = IOPRIO_DEFAULT;
> +
> +	if (IOPRIO_PRIO_CLASS(prio) == IOPRIO_CLASS_NONE)
> +		prio = IOPRIO_PRIO_VALUE(task_nice_ioclass(current),
> +					 task_nice_ioprio(current));
> +	return prio;
>  }
>  
>  /*

Looks good to me.

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/8] block: Make ioprio_best() static

[Damien Le Moal]

On 6/21/22 01:11, Jan Kara wrote:
> Nobody outside of block/ioprio.c uses it.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  block/ioprio.c         | 2 +-
>  include/linux/ioprio.h | 5 -----
>  2 files changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/block/ioprio.c b/block/ioprio.c
> index 2a34cbca18ae..b5cf7339709b 100644
> --- a/block/ioprio.c
> +++ b/block/ioprio.c
> @@ -154,7 +154,7 @@ static int get_task_ioprio(struct task_struct *p)
>  	return ret;
>  }
>  
> -int ioprio_best(unsigned short aprio, unsigned short bprio)
> +static int ioprio_best(unsigned short aprio, unsigned short bprio)
>  {
>  	if (!ioprio_valid(aprio))
>  		aprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_BE_NORM);
> diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
> index 61ed6bb4998e..519d51fc8902 100644
> --- a/include/linux/ioprio.h
> +++ b/include/linux/ioprio.h
> @@ -66,11 +66,6 @@ static inline int get_current_ioprio(void)
>  	return prio;
>  }
>  
> -/*
> - * For inheritance, return the highest of the two given priorities
> - */

Nit: you could move this comment over the static function. But the name
makes it fairly obvious what it does :)

> -extern int ioprio_best(unsigned short aprio, unsigned short bprio);
> -
>  extern int set_task_ioprio(struct task_struct *task, int ioprio);
>  
>  #ifdef CONFIG_BLOCK
Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 4/8] block: Fix handling of tasks without ioprio in ioprio_get(2)

[Damien Le Moal]

On 6/21/22 01:11, Jan Kara wrote:
> ioprio_get(2) can be asked to return the best IO priority from several
> tasks (IOPRIO_WHO_PGRP, IOPRIO_WHO_USER). Currently the call treats
> tasks without set IO priority as having priority
> IOPRIO_CLASS_BE/IOPRIO_BE_NORM however this does not really reflect the
> IO priority the task will get (which depends on task's nice value).
> 
> Fix the code to use the real IO priority task's IO will use. For this we
> do some factoring out to share the code converting task's CPU priority
> to IO priority and we also have to modify code for
> ioprio_get(IOPRIO_WHO_PROCESS) to keep returning IOPRIO_CLASS_NONE
> priority for tasks without set IO priority as a special case to maintain
> userspace visible API.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  block/ioprio.c         | 49 ++++++++++++++++++++++++++++++++++++------
>  include/linux/ioprio.h | 19 +++-------------
>  2 files changed, 45 insertions(+), 23 deletions(-)
> 
> diff --git a/block/ioprio.c b/block/ioprio.c
> index b5cf7339709b..a4c19ce0de4c 100644
> --- a/block/ioprio.c
> +++ b/block/ioprio.c
> @@ -138,6 +138,27 @@ SYSCALL_DEFINE3(ioprio_set, int, which, int, who, int, ioprio)
>  	return ret;
>  }
>  
> +/*
> + * If the task has set an I/O priority, use that. Otherwise, return
> + * the default I/O priority.
> + */
> +int __get_task_ioprio(struct task_struct *p)
> +{
> +	struct io_context *ioc = p->io_context;
> +	int prio;
> +
> +	if (ioc)
> +		prio = ioc->ioprio;
> +	else
> +		prio = IOPRIO_DEFAULT;
> +
> +	if (IOPRIO_PRIO_CLASS(prio) == IOPRIO_CLASS_NONE)
> +		prio = IOPRIO_PRIO_VALUE(task_nice_ioclass(p),
> +					 task_nice_ioprio(p));
> +	return prio;
> +}
> +EXPORT_SYMBOL_GPL(__get_task_ioprio);
> +
>  static int get_task_ioprio(struct task_struct *p)
>  {
>  	int ret;
> @@ -145,10 +166,29 @@ static int get_task_ioprio(struct task_struct *p)
>  	ret = security_task_getioprio(p);
>  	if (ret)
>  		goto out;
> -	ret = IOPRIO_DEFAULT;
> +	task_lock(p);
> +	ret = __get_task_ioprio(p);
> +	task_unlock(p);
> +out:
> +	return ret;
> +}
> +
> +/*
> + * Return raw IO priority value as set by userspace. We use this for
> + * ioprio_get(pid, IOPRIO_WHO_PROCESS) so that we keep historical behavior and
> + * also so that userspace can distinguish unset IO priority (which just gets
> + * overriden based on task's nice value) from IO priority set to some value.
> + */
> +static int get_task_raw_ioprio(struct task_struct *p) { int ret;

The "int ret;" declaration is on the wrong line, so is the curly bracket.

> +
> +	ret = security_task_getioprio(p);
> +	if (ret)
> +		goto out;
>  	task_lock(p);
>  	if (p->io_context)
>  		ret = p->io_context->ioprio;
> +	else
> +		ret = IOPRIO_DEFAULT;
>  	task_unlock(p);
>  out:
>  	return ret;
> @@ -156,11 +196,6 @@ static int get_task_ioprio(struct task_struct *p)
>  
>  static int ioprio_best(unsigned short aprio, unsigned short bprio)
>  {
> -	if (!ioprio_valid(aprio))
> -		aprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_BE_NORM);
> -	if (!ioprio_valid(bprio))
> -		bprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_BE_NORM);
> -
>  	return min(aprio, bprio);
>  }

This function could be declared as inline now...

>  
> @@ -181,7 +216,7 @@ SYSCALL_DEFINE2(ioprio_get, int, which, int, who)
>  			else
>  				p = find_task_by_vpid(who);
>  			if (p)
> -				ret = get_task_ioprio(p);
> +				ret = get_task_raw_ioprio(p);
>  			break;
>  		case IOPRIO_WHO_PGRP:
>  			if (!who)
> diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
> index 519d51fc8902..24e648dc4fb3 100644
> --- a/include/linux/ioprio.h
> +++ b/include/linux/ioprio.h
> @@ -46,24 +46,11 @@ static inline int task_nice_ioclass(struct task_struct *task)
>  		return IOPRIO_CLASS_BE;
>  }
>  
> -/*
> - * If the calling process has set an I/O priority, use that. Otherwise, return
> - * the default I/O priority.
> - */
> +int __get_task_ioprio(struct task_struct *p);
> +
>  static inline int get_current_ioprio(void)
>  {
> -	struct io_context *ioc = current->io_context;
> -	int prio;
> -
> -	if (ioc)
> -		prio = ioc->ioprio;
> -	else
> -		prio = IOPRIO_DEFAULT;
> -
> -	if (IOPRIO_PRIO_CLASS(prio) == IOPRIO_CLASS_NONE)
> -		prio = IOPRIO_PRIO_VALUE(task_nice_ioclass(current),
> -					 task_nice_ioprio(current));
> -	return prio;
> +	return __get_task_ioprio(current);

The build bot complained about this one, but I do not understand why.
Could it be because you do not have declared __get_task_ioprio() as "extern" ?

Also, to reduce refactoring changes in this patch, you could introduce
__get_task_ioprio() and make the above change in patch 2. No ?

>  }
>  
>  extern int set_task_ioprio(struct task_struct *task, int ioprio);


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 5/8] blk-ioprio: Remove unneeded field

[Damien Le Moal]

On 6/21/22 01:11, Jan Kara wrote:
> blkcg->ioprio_set field is not really useful except for avoiding
> possibly more expensive checks inside blkcg_ioprio_track(). The check
> for blkcg->prio_policy being equal to POLICY_NO_CHANGE does the same
> service so just remove the ioprio_set field and replace the check.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Looks good to me.

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

> ---
>  block/blk-ioprio.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/block/blk-ioprio.c b/block/blk-ioprio.c
> index 79e797f5d194..3f605583598b 100644
> --- a/block/blk-ioprio.c
> +++ b/block/blk-ioprio.c
> @@ -62,7 +62,6 @@ struct ioprio_blkg {
>  struct ioprio_blkcg {
>  	struct blkcg_policy_data cpd;
>  	enum prio_policy	 prio_policy;
> -	bool			 prio_set;
>  };
>  
>  static inline struct ioprio_blkg *pd_to_ioprio(struct blkg_policy_data *pd)
> @@ -113,7 +112,6 @@ static ssize_t ioprio_set_prio_policy(struct kernfs_open_file *of, char *buf,
>  	if (ret < 0)
>  		return ret;
>  	blkcg->prio_policy = ret;
> -	blkcg->prio_set = true;
>  	return nbytes;
>  }
>  
> @@ -193,16 +191,15 @@ static void blkcg_ioprio_track(struct rq_qos *rqos, struct request *rq,
>  	struct ioprio_blkcg *blkcg = ioprio_blkcg_from_bio(bio);
>  	u16 prio;
>  
> -	if (!blkcg->prio_set)
> +	if (blkcg->prio_policy == POLICY_NO_CHANGE)
>  		return;
>  
>  	/*
>  	 * Except for IOPRIO_CLASS_NONE, higher I/O priority numbers
>  	 * correspond to a lower priority. Hence, the max_t() below selects
>  	 * the lower priority of bi_ioprio and the cgroup I/O priority class.
> -	 * If the cgroup policy has been set to POLICY_NO_CHANGE == 0, the
> -	 * bio I/O priority is not modified. If the bio I/O priority equals
> -	 * IOPRIO_CLASS_NONE, the cgroup I/O priority is assigned to the bio.
> +	 * If the bio I/O priority equals IOPRIO_CLASS_NONE, the cgroup I/O
> +	 * priority is assigned to the bio.
>  	 */
>  	prio = max_t(u16, bio->bi_ioprio,
>  			IOPRIO_PRIO_VALUE(blkcg->prio_policy, 0));


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 6/8] blk-ioprio: Convert from rqos policy to direct call

[Damien Le Moal]

On 6/21/22 01:11, Jan Kara wrote:
> Convert blk-ioprio handling from a rqos policy to a direct call from
> blk_mq_submit_bio(). Firstly, blk-ioprio is not a much of a rqos policy

s/not a much/not much

> anyway, it just needs a hook in bio submission path to set the bio's IO
> priority. Secondly, the rqos .track hook gets actually called too late
> for blk-ioprio purposes and introducing a special rqos hook just for
> blk-ioprio looks even weirder.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  block/blk-cgroup.c |  1 +
>  block/blk-ioprio.c | 50 +++++-----------------------------------------
>  block/blk-ioprio.h |  9 +++++++++
>  block/blk-mq.c     |  8 ++++++++
>  4 files changed, 23 insertions(+), 45 deletions(-)
> 
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index 764e740b0c0f..6906981563f8 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -1299,6 +1299,7 @@ int blkcg_init_queue(struct request_queue *q)
>  	ret = blk_iolatency_init(q);
>  	if (ret) {
>  		blk_throtl_exit(q);
> +		blk_ioprio_exit(q);
>  		goto err_destroy_all;
>  	}
>  
> diff --git a/block/blk-ioprio.c b/block/blk-ioprio.c
> index 3f605583598b..c00060a02c6e 100644
> --- a/block/blk-ioprio.c
> +++ b/block/blk-ioprio.c
> @@ -181,17 +181,12 @@ static struct blkcg_policy ioprio_policy = {
>  	.pd_free_fn	= ioprio_free_pd,
>  };
>  
> -struct blk_ioprio {
> -	struct rq_qos rqos;
> -};
> -
> -static void blkcg_ioprio_track(struct rq_qos *rqos, struct request *rq,
> -			       struct bio *bio)
> +void blkcg_set_ioprio(struct bio *bio)
>  {
>  	struct ioprio_blkcg *blkcg = ioprio_blkcg_from_bio(bio);
>  	u16 prio;
>  
> -	if (blkcg->prio_policy == POLICY_NO_CHANGE)
> +	if (!blkcg || blkcg->prio_policy == POLICY_NO_CHANGE)
>  		return;
>  
>  	/*
> @@ -207,49 +202,14 @@ static void blkcg_ioprio_track(struct rq_qos *rqos, struct request *rq,
>  		bio->bi_ioprio = prio;
>  }
>  
> -static void blkcg_ioprio_exit(struct rq_qos *rqos)
> +void blk_ioprio_exit(struct request_queue *q)
>  {
> -	struct blk_ioprio *blkioprio_blkg =
> -		container_of(rqos, typeof(*blkioprio_blkg), rqos);
> -
> -	blkcg_deactivate_policy(rqos->q, &ioprio_policy);
> -	kfree(blkioprio_blkg);
> +	blkcg_deactivate_policy(q, &ioprio_policy);
>  }
>  
> -static struct rq_qos_ops blkcg_ioprio_ops = {
> -	.track	= blkcg_ioprio_track,
> -	.exit	= blkcg_ioprio_exit,
> -};
> -
>  int blk_ioprio_init(struct request_queue *q)
>  {
> -	struct blk_ioprio *blkioprio_blkg;
> -	struct rq_qos *rqos;
> -	int ret;
> -
> -	blkioprio_blkg = kzalloc(sizeof(*blkioprio_blkg), GFP_KERNEL);
> -	if (!blkioprio_blkg)
> -		return -ENOMEM;
> -
> -	ret = blkcg_activate_policy(q, &ioprio_policy);
> -	if (ret) {
> -		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 blkcg_activate_policy(q, &ioprio_policy);
>  }
>  
>  static int __init ioprio_init(void)
> diff --git a/block/blk-ioprio.h b/block/blk-ioprio.h
> index a7785c2f1aea..5a1eb550e178 100644
> --- a/block/blk-ioprio.h
> +++ b/block/blk-ioprio.h
> @@ -6,14 +6,23 @@
>  #include <linux/kconfig.h>
>  
>  struct request_queue;
> +struct bio;
>  
>  #ifdef CONFIG_BLK_CGROUP_IOPRIO
>  int blk_ioprio_init(struct request_queue *q);
> +void blk_ioprio_exit(struct request_queue *q);
> +void blkcg_set_ioprio(struct bio *bio);
>  #else
>  static inline int blk_ioprio_init(struct request_queue *q)
>  {
>  	return 0;
>  }
> +static inline void blk_ioprio_exit(struct request_queue *q)
> +{
> +}
> +static inline void blkcg_set_ioprio(struct bio *bio)
> +{
> +}
>  #endif
>  
>  #endif /* _BLK_IOPRIO_H_ */
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index e9bf950983c7..67a7bfa58b7c 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -42,6 +42,7 @@
>  #include "blk-stat.h"
>  #include "blk-mq-sched.h"
>  #include "blk-rq-qos.h"
> +#include "blk-ioprio.h"
>  
>  static DEFINE_PER_CPU(struct llist_head, blk_cpu_done);
>  
> @@ -2790,6 +2791,11 @@ static inline struct request *blk_mq_get_cached_request(struct request_queue *q,
>  	return rq;
>  }
>  
> +static void bio_set_ioprio(struct bio *bio)
> +{
> +	blkcg_set_ioprio(bio);
> +}

Nit: Make this inline ?

> +
>  /**
>   * blk_mq_submit_bio - Create and send a request to block device.
>   * @bio: Bio pointer.
> @@ -2830,6 +2836,8 @@ void blk_mq_submit_bio(struct bio *bio)
>  
>  	trace_block_getrq(bio);
>  
> +	bio_set_ioprio(bio);
> +
>  	rq_qos_track(q, rq, bio);
>  
>  	blk_mq_bio_to_request(rq, bio, nr_segs);

Apart from the nit, looks good to me.

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 7/8] block: Initialize bio priority earlier

[Damien Le Moal]

On 6/21/22 01:11, Jan Kara wrote:
> Bio's IO priority needs to be initialized before we try to merge the bio
> with other bios. Otherwise we could merge bios which would otherwise
> receive different IO priorities leading to possible QoS issues.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  block/blk-mq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 67a7bfa58b7c..e17d822e6051 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2825,6 +2825,8 @@ void blk_mq_submit_bio(struct bio *bio)
>  	if (!bio_integrity_prep(bio))
>  		return;
>  
> +	bio_set_ioprio(bio);
> +
>  	rq = blk_mq_get_cached_request(q, plug, &bio, nr_segs);
>  	if (!rq) {
>  		if (!bio)
> @@ -2836,8 +2838,6 @@ void blk_mq_submit_bio(struct bio *bio)
>  
>  	trace_block_getrq(bio);
>  
> -	bio_set_ioprio(bio);
> -
>  	rq_qos_track(q, rq, bio);
>  
>  	blk_mq_bio_to_request(rq, bio, nr_segs);

Looks good to me.

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 8/8] block: Always initialize bio IO priority on submit

[Damien Le Moal]

On 6/21/22 01:11, Jan Kara wrote:
> Currently, IO priority set in task's IO context is not reflected in the
> bio->bi_ioprio for most IO (only io_uring and direct IO set it). This
> results in odd results where process is submitting some bios with one
> priority and other bios with a different (unset) priority and due to
> differing priorities bios cannot be merged. Make sure bio->bi_ioprio is
> always set on bio submission.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  block/blk-mq.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index e17d822e6051..7548f8aebea8 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2793,6 +2793,9 @@ static inline struct request *blk_mq_get_cached_request(struct request_queue *q,
>  
>  static void bio_set_ioprio(struct bio *bio)
>  {
> +	/* Nobody set ioprio so far? Initialize it based on task's nice value */
> +	if (IOPRIO_PRIO_CLASS(bio->bi_ioprio) == IOPRIO_CLASS_NONE)
> +		bio->bi_ioprio = get_current_ioprio();
>  	blkcg_set_ioprio(bio);
>  }
>  

Looks good to me.

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 4/8] block: Fix handling of tasks without ioprio in ioprio_get(2)

[kernel test robot]

Hi Jan,

I love your patch! Perhaps something to improve:

[auto build test WARNING on axboe-block/for-next]
[also build test WARNING on linus/master v5.19-rc2 next-20220617]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Jan-Kara/block-Fix-IO-priority-mess/20220621-001427
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: mips-randconfig-m031-20220619 (https://download.01.org/0day-ci/archive/20220621/202206210847.sBhjsEiQ-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 11.3.0

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

smatch warnings:
block/ioprio.c:184 get_task_raw_ioprio() warn: inconsistent indenting

vim +184 block/ioprio.c

   183	
 > 184		ret = security_task_getioprio(p);
   185		if (ret)
   186			goto out;
   187		task_lock(p);
   188		if (p->io_context)
   189			ret = p->io_context->ioprio;
   190		else
   191			ret = IOPRIO_DEFAULT;
   192		task_unlock(p);
   193	out:
   194		return ret;
   195	}
   196	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 3/8] block: Make ioprio_best() static

[Jan Kara]

On Tue 21-06-22 08:47:29, Damien Le Moal wrote:
> > diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
> > index 61ed6bb4998e..519d51fc8902 100644
> > --- a/include/linux/ioprio.h
> > +++ b/include/linux/ioprio.h
> > @@ -66,11 +66,6 @@ static inline int get_current_ioprio(void)
> >  	return prio;
> >  }
> >  
> > -/*
> > - * For inheritance, return the highest of the two given priorities
> > - */
> 
> Nit: you could move this comment over the static function. But the name
> makes it fairly obvious what it does :)

Yeah, I didn't find it particularly useful, especially after ioprio_best()
becomes pure min() so I've just deleted it...

> > -extern int ioprio_best(unsigned short aprio, unsigned short bprio);
> > -
> >  extern int set_task_ioprio(struct task_struct *task, int ioprio);
> >  
> >  #ifdef CONFIG_BLOCK
> Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

Thanks for review!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 4/8] block: Fix handling of tasks without ioprio in ioprio_get(2)

[Jan Kara]

On Tue 21-06-22 08:57:29, Damien Le Moal wrote:
> On 6/21/22 01:11, Jan Kara wrote:
> > ioprio_get(2) can be asked to return the best IO priority from several
> > tasks (IOPRIO_WHO_PGRP, IOPRIO_WHO_USER). Currently the call treats
> > tasks without set IO priority as having priority
> > IOPRIO_CLASS_BE/IOPRIO_BE_NORM however this does not really reflect the
> > IO priority the task will get (which depends on task's nice value).
> > 
> > Fix the code to use the real IO priority task's IO will use. For this we
> > do some factoring out to share the code converting task's CPU priority
> > to IO priority and we also have to modify code for
> > ioprio_get(IOPRIO_WHO_PROCESS) to keep returning IOPRIO_CLASS_NONE
> > priority for tasks without set IO priority as a special case to maintain
> > userspace visible API.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>

...

> > +/*
> > + * Return raw IO priority value as set by userspace. We use this for
> > + * ioprio_get(pid, IOPRIO_WHO_PROCESS) so that we keep historical behavior and
> > + * also so that userspace can distinguish unset IO priority (which just gets
> > + * overriden based on task's nice value) from IO priority set to some value.
> > + */
> > +static int get_task_raw_ioprio(struct task_struct *p) { int ret;
> 
> The "int ret;" declaration is on the wrong line, so is the curly bracket.

Huh, don't know how that got messed up. Anyway fixed. Thanks for noticing.

> > +
> > +	ret = security_task_getioprio(p);
> > +	if (ret)
> > +		goto out;
> >  	task_lock(p);
> >  	if (p->io_context)
> >  		ret = p->io_context->ioprio;
> > +	else
> > +		ret = IOPRIO_DEFAULT;
> >  	task_unlock(p);
> >  out:
> >  	return ret;
> > @@ -156,11 +196,6 @@ static int get_task_ioprio(struct task_struct *p)
> >  
> >  static int ioprio_best(unsigned short aprio, unsigned short bprio)
> >  {
> > -	if (!ioprio_valid(aprio))
> > -		aprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_BE_NORM);
> > -	if (!ioprio_valid(bprio))
> > -		bprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_BE_NORM);
> > -
> >  	return min(aprio, bprio);
> >  }
> 
> This function could be declared as inline now...

Yeah, but compilers inline (or not inline!) static functions as they see
fit anyway. So 'inline' keyword for static functions is generally a noise
which is why I just avoid it. But please let me know if you feel strongly
about that.

> >  static inline int get_current_ioprio(void)
> >  {
> > -	struct io_context *ioc = current->io_context;
> > -	int prio;
> > -
> > -	if (ioc)
> > -		prio = ioc->ioprio;
> > -	else
> > -		prio = IOPRIO_DEFAULT;
> > -
> > -	if (IOPRIO_PRIO_CLASS(prio) == IOPRIO_CLASS_NONE)
> > -		prio = IOPRIO_PRIO_VALUE(task_nice_ioclass(current),
> > -					 task_nice_ioprio(current));
> > -	return prio;
> > +	return __get_task_ioprio(current);
> 
> The build bot complained about this one, but I do not understand why.
> Could it be because you do not have declared __get_task_ioprio() as "extern" ?

No, likely it is because !CONFIG_BLOCK kernel does not have ioprio support
but something uses the get_current_ioprio() anyway. I'll fix it up.

> Also, to reduce refactoring changes in this patch, you could introduce
> __get_task_ioprio() and make the above change in patch 2. No ?

OK, I will move some refactoring.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 6/8] blk-ioprio: Convert from rqos policy to direct call

[Jan Kara]

On Tue 21-06-22 09:00:57, Damien Le Moal wrote:
> On 6/21/22 01:11, Jan Kara wrote:
> > @@ -2790,6 +2791,11 @@ static inline struct request *blk_mq_get_cached_request(struct request_queue *q,
> >  	return rq;
> >  }
> >  
> > +static void bio_set_ioprio(struct bio *bio)
> > +{
> > +	blkcg_set_ioprio(bio);
> > +}
> 
> Nit: Make this inline ?

Similar response as to the other static function you wanted to make inline.
I'm pretty sure it will get inlined regardless of whether I write there
inline or not so I prefer not to clutter the declaration :).

> > +
> >  /**
> >   * blk_mq_submit_bio - Create and send a request to block device.
> >   * @bio: Bio pointer.
> > @@ -2830,6 +2836,8 @@ void blk_mq_submit_bio(struct bio *bio)
> >  
> >  	trace_block_getrq(bio);
> >  
> > +	bio_set_ioprio(bio);
> > +
> >  	rq_qos_track(q, rq, bio);
> >  
> >  	blk_mq_bio_to_request(rq, bio, nr_segs);
> 
> Apart from the nit, looks good to me.
> 
> Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

Thanks for review!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 4/8] block: Fix handling of tasks without ioprio in ioprio_get(2)

[Damien Le Moal]

On 6/21/22 17:15, Jan Kara wrote:
> On Tue 21-06-22 08:57:29, Damien Le Moal wrote:
>> On 6/21/22 01:11, Jan Kara wrote:
>>> ioprio_get(2) can be asked to return the best IO priority from several
>>> tasks (IOPRIO_WHO_PGRP, IOPRIO_WHO_USER). Currently the call treats
>>> tasks without set IO priority as having priority
>>> IOPRIO_CLASS_BE/IOPRIO_BE_NORM however this does not really reflect the
>>> IO priority the task will get (which depends on task's nice value).
>>>
>>> Fix the code to use the real IO priority task's IO will use. For this we
>>> do some factoring out to share the code converting task's CPU priority
>>> to IO priority and we also have to modify code for
>>> ioprio_get(IOPRIO_WHO_PROCESS) to keep returning IOPRIO_CLASS_NONE
>>> priority for tasks without set IO priority as a special case to maintain
>>> userspace visible API.
>>>
>>> Signed-off-by: Jan Kara <jack@suse.cz>
> 
> ...
> 
>>> +/*
>>> + * Return raw IO priority value as set by userspace. We use this for
>>> + * ioprio_get(pid, IOPRIO_WHO_PROCESS) so that we keep historical behavior and
>>> + * also so that userspace can distinguish unset IO priority (which just gets
>>> + * overriden based on task's nice value) from IO priority set to some value.
>>> + */
>>> +static int get_task_raw_ioprio(struct task_struct *p) { int ret;
>>
>> The "int ret;" declaration is on the wrong line, so is the curly bracket.
> 
> Huh, don't know how that got messed up. Anyway fixed. Thanks for noticing.
> 
>>> +
>>> +	ret = security_task_getioprio(p);
>>> +	if (ret)
>>> +		goto out;
>>>  	task_lock(p);
>>>  	if (p->io_context)
>>>  		ret = p->io_context->ioprio;
>>> +	else
>>> +		ret = IOPRIO_DEFAULT;
>>>  	task_unlock(p);
>>>  out:
>>>  	return ret;
>>> @@ -156,11 +196,6 @@ static int get_task_ioprio(struct task_struct *p)
>>>  
>>>  static int ioprio_best(unsigned short aprio, unsigned short bprio)
>>>  {
>>> -	if (!ioprio_valid(aprio))
>>> -		aprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_BE_NORM);
>>> -	if (!ioprio_valid(bprio))
>>> -		bprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_BE_NORM);
>>> -
>>>  	return min(aprio, bprio);
>>>  }
>>
>> This function could be declared as inline now...
> 
> Yeah, but compilers inline (or not inline!) static functions as they see
> fit anyway. So 'inline' keyword for static functions is generally a noise
> which is why I just avoid it. But please let me know if you feel strongly
> about that.

Not at all. Fine as-is !

> 
>>>  static inline int get_current_ioprio(void)
>>>  {
>>> -	struct io_context *ioc = current->io_context;
>>> -	int prio;
>>> -
>>> -	if (ioc)
>>> -		prio = ioc->ioprio;
>>> -	else
>>> -		prio = IOPRIO_DEFAULT;
>>> -
>>> -	if (IOPRIO_PRIO_CLASS(prio) == IOPRIO_CLASS_NONE)
>>> -		prio = IOPRIO_PRIO_VALUE(task_nice_ioclass(current),
>>> -					 task_nice_ioprio(current));
>>> -	return prio;
>>> +	return __get_task_ioprio(current);
>>
>> The build bot complained about this one, but I do not understand why.
>> Could it be because you do not have declared __get_task_ioprio() as "extern" ?
> 
> No, likely it is because !CONFIG_BLOCK kernel does not have ioprio support
> but something uses the get_current_ioprio() anyway. I'll fix it up.
> 
>> Also, to reduce refactoring changes in this patch, you could introduce
>> __get_task_ioprio() and make the above change in patch 2. No ?
> 
> OK, I will move some refactoring.
> 
> 								Honza


-- 
Damien Le Moal
Western Digital Research

[PATCH 7/8] block: Initialize bio priority earlier

[Jan Kara]

Bio's IO priority needs to be initialized before we try to merge the bio
with other bios. Otherwise we could merge bios which would otherwise
receive different IO priorities leading to possible QoS issues.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 block/blk-mq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 67a7bfa58b7c..e17d822e6051 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2825,6 +2825,8 @@ void blk_mq_submit_bio(struct bio *bio)
 	if (!bio_integrity_prep(bio))
 		return;
 
+	bio_set_ioprio(bio);
+
 	rq = blk_mq_get_cached_request(q, plug, &bio, nr_segs);
 	if (!rq) {
 		if (!bio)
@@ -2836,8 +2838,6 @@ void blk_mq_submit_bio(struct bio *bio)
 
 	trace_block_getrq(bio);
 
-	bio_set_ioprio(bio);
-
 	rq_qos_track(q, rq, bio);
 
 	blk_mq_bio_to_request(rq, bio, nr_segs);
-- 
2.35.3