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

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

[Jan Kara]

Hello,

This is the second revision of my patches fixing IO priority handling in the
block layer. 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

[PATCH 1/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) and
with the following fix it will not even match returned IO priority for a
single task.

Fix IO priority comparison to treat unset IO priority as the lowest
possible one. This way we will return IOPRIO_CLASS_NONE priority only if
none of the considered tasks has explicitely set IO priority, otherwise
we return the highest set IO priority. This changes userspace visible
behavior but this way the caller really gets back the highest set IO
priority without contaminating the result with
IOPRIO_CLASS_BE/IOPRIO_BE_NORM from tasks which have IO priority unset.

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

diff --git a/block/ioprio.c b/block/ioprio.c
index 2fe068fcaad5..62890391fc80 100644
--- a/block/ioprio.c
+++ b/block/ioprio.c
@@ -157,10 +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;
+		return bprio;
 	if (!ioprio_valid(bprio))
-		bprio = IOPRIO_DEFAULT;
-
+		return aprio;
 	return min(aprio, bprio);
 }
 
-- 
2.35.3

[PATCH 2/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 62890391fc80..18f7e16882fe 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))
 		return bprio;
diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
index 3f53bc27a19b..774bb90ad668 100644
--- a/include/linux/ioprio.h
+++ b/include/linux/ioprio.h
@@ -59,11 +59,6 @@ static inline int get_current_ioprio(void)
 	return IOPRIO_DEFAULT;
 }
 
-/*
- * 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 3/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.

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

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/include/linux/ioprio.h b/include/linux/ioprio.h
index 774bb90ad668..d9dc78a15301 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 4/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 d9dc78a15301..519d51fc8902 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;
 }
 
 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 | 6 ++++++
 1 file changed, 6 insertions(+)

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

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

[Damien Le Moal]

On 6/16/22 01:16, 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
> 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)

See my comment on patch 8.

>   		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);


-- 
Damien Le Moal
Western Digital Research

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

[Damien Le Moal]

On 6/16/22 01:16, 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 | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index e17d822e6051..e976f696babc 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2793,7 +2793,13 @@ static inline struct request *blk_mq_get_cached_request(struct request_queue *q,
>   
>   static void bio_set_ioprio(struct bio *bio)
>   {
> +	unsigned short ioprio_class;
> +
>   	blkcg_set_ioprio(bio);

Shouldn't this function set the default using the below "if" code ?

> +	ioprio_class = IOPRIO_PRIO_CLASS(bio->bi_ioprio);
> +	/* Nobody set ioprio so far? Initialize it based on task's nice value */

I do not think that the ioprio_class variable is useful.
This can be:

	if (IOPRIO_PRIO_CLASS(bio->bi_ioprio) == IOPRIO_CLASS_NONE)
		bio->bi_ioprio = get_current_ioprio();

> +	if (ioprio_class == IOPRIO_CLASS_NONE)
> +		bio->bi_ioprio = get_current_ioprio();
>   }
>   
>   /**

Beside this comment, I am still scratching my head regarding what the 
user gets with ioprio_get(). If I understood your patches correctly, the 
user may still see IOPRIO_CLASS_NONE ?
For that case, to be in sync with the man page, I thought the returned 
ioprio should be the effective one based on the task io nice value, that 
is, the value returned by get_current_ioprio(). Am I missing something... ?
	
-- 
Damien Le Moal
Western Digital Research

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

[Jan Kara]

On Thu 16-06-22 12:15:25, Damien Le Moal wrote:
> On 6/16/22 01:16, 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 | 6 ++++++
> >   1 file changed, 6 insertions(+)
> > 
> > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > index e17d822e6051..e976f696babc 100644
> > --- a/block/blk-mq.c
> > +++ b/block/blk-mq.c
> > @@ -2793,7 +2793,13 @@ static inline struct request *blk_mq_get_cached_request(struct request_queue *q,
> >   static void bio_set_ioprio(struct bio *bio)
> >   {
> > +	unsigned short ioprio_class;
> > +
> >   	blkcg_set_ioprio(bio);
> 
> Shouldn't this function set the default using the below "if" code ?
> 
> > +	ioprio_class = IOPRIO_PRIO_CLASS(bio->bi_ioprio);
> > +	/* Nobody set ioprio so far? Initialize it based on task's nice value */
> 
> I do not think that the ioprio_class variable is useful.
> This can be:
> 
> 	if (IOPRIO_PRIO_CLASS(bio->bi_ioprio) == IOPRIO_CLASS_NONE)
> 		bio->bi_ioprio = get_current_ioprio();

You are right. Fixed.

> > +	if (ioprio_class == IOPRIO_CLASS_NONE)
> > +		bio->bi_ioprio = get_current_ioprio();
> >   }
> >   /**
> 
> Beside this comment, I am still scratching my head regarding what the user
> gets with ioprio_get(). If I understood your patches correctly, the user may
> still see IOPRIO_CLASS_NONE ?
> For that case, to be in sync with the man page, I thought the returned
> ioprio should be the effective one based on the task io nice value, that is,
> the value returned by get_current_ioprio(). Am I missing something... ?

The trouble with returning "effective ioprio" is that with IOPRIO_WHO_PGRP
or IOPRIO_WHO_USER the effective IO priority may be different for different
processes considered and it can be also further influenced by blk-ioprio
settings. But thinking about it now after things have settled I agree that
what you suggests makes more sense. I'll fix that. Thanks for suggestion!

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

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

[Jan Kara]

On Thu 16-06-22 13:23:03, Jan Kara wrote:
> On Thu 16-06-22 12:15:25, Damien Le Moal wrote:
> > On 6/16/22 01:16, Jan Kara wrote:
> > > +	if (ioprio_class == IOPRIO_CLASS_NONE)
> > > +		bio->bi_ioprio = get_current_ioprio();
> > >   }
> > >   /**
> > 
> > Beside this comment, I am still scratching my head regarding what the user
> > gets with ioprio_get(). If I understood your patches correctly, the user may
> > still see IOPRIO_CLASS_NONE ?
> > For that case, to be in sync with the man page, I thought the returned
> > ioprio should be the effective one based on the task io nice value, that is,
> > the value returned by get_current_ioprio(). Am I missing something... ?
> 
> The trouble with returning "effective ioprio" is that with IOPRIO_WHO_PGRP
> or IOPRIO_WHO_USER the effective IO priority may be different for different
> processes considered and it can be also further influenced by blk-ioprio
> settings. But thinking about it now after things have settled I agree that
> what you suggests makes more sense. I'll fix that. Thanks for suggestion!

Oh, now I've remembered why I've done it that way. With IOPRIO_WHO_PROCESS
(which is probably the most used and the best defined variant), we were
returning IOPRIO_CLASS_NONE if the task didn't have set IO priority until
commit e70344c05995 ("block: fix default IO priority handling"). So my
patch was just making behavior of IOPRIO_WHO_PGRP & IOPRIO_WHO_USER
consistent with the behavior of IOPRIO_WHO_PROCESS. I'd be reluctant to
change the behavior of IOPRIO_WHO_PROCESS because that has the biggest
chances for userspace regressions. But perhaps it makes sense to keep
IOPRIO_WHO_PGRP & IOPRIO_WHO_USER inconsistent with IOPRIO_WHO_PROCESS and
just use effective IO priority in those two variants. That looks like the
smallest API change to make things at least somewhat sensible...

								Honza

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

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

[Damien Le Moal]

On 6/16/22 21:24, Jan Kara wrote:
> On Thu 16-06-22 13:23:03, Jan Kara wrote:
>> On Thu 16-06-22 12:15:25, Damien Le Moal wrote:
>>> On 6/16/22 01:16, Jan Kara wrote:
>>>> +	if (ioprio_class == IOPRIO_CLASS_NONE)
>>>> +		bio->bi_ioprio = get_current_ioprio();
>>>>   }
>>>>   /**
>>>
>>> Beside this comment, I am still scratching my head regarding what the user
>>> gets with ioprio_get(). If I understood your patches correctly, the user may
>>> still see IOPRIO_CLASS_NONE ?
>>> For that case, to be in sync with the man page, I thought the returned
>>> ioprio should be the effective one based on the task io nice value, that is,
>>> the value returned by get_current_ioprio(). Am I missing something... ?
>>
>> The trouble with returning "effective ioprio" is that with IOPRIO_WHO_PGRP
>> or IOPRIO_WHO_USER the effective IO priority may be different for different
>> processes considered and it can be also further influenced by blk-ioprio
>> settings. But thinking about it now after things have settled I agree that
>> what you suggests makes more sense. I'll fix that. Thanks for suggestion!
> 
> Oh, now I've remembered why I've done it that way. With IOPRIO_WHO_PROCESS
> (which is probably the most used and the best defined variant), we were
> returning IOPRIO_CLASS_NONE if the task didn't have set IO priority until
> commit e70344c05995 ("block: fix default IO priority handling"). So my
> patch was just making behavior of IOPRIO_WHO_PGRP & IOPRIO_WHO_USER
> consistent with the behavior of IOPRIO_WHO_PROCESS. I'd be reluctant to
> change the behavior of IOPRIO_WHO_PROCESS because that has the biggest
> chances for userspace regressions. But perhaps it makes sense to keep
> IOPRIO_WHO_PGRP & IOPRIO_WHO_USER inconsistent with IOPRIO_WHO_PROCESS and
> just use effective IO priority in those two variants. That looks like the
> smallest API change to make things at least somewhat sensible...

Still bit lost. Let me try to summarize your goal:

1) If IOPRIO_WHO_PGRP is not set, ioprio_get(IOPRIO_WHO_PGRP) will return
the effective priority

2) If IOPRIO_WHO_USER is not set, ioprio_get(IOPRIO_WHO_USER) will also
return the effective priority

3) if IOPRIO_WHO_PROCESS is not set, return ? I am lost for this one. Do
you want to go back to IOPRIO_CLASS_NONE ? Keep default (IOPRIO_CLASS_BE)
? Or switch to using the effective IO priority ? Not that the last 2
choices are actually equivalent if the user did not IO nice the process
(the default for the effective IO prio is class BE)

For (1) and (2), I am not sure. Given that my last changes to the ioprio
default did not seem to have bothered anyone (nobody screamed at me :)) I
am tempted to say: any choice is OK. So we should try to get as close as
the man page defined behavior as possible.



-- 
Damien Le Moal
Western Digital Research

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

[Damien Le Moal]

On 6/16/22 20:23, Jan Kara wrote:
> On Thu 16-06-22 12:15:25, Damien Le Moal wrote:
>> On 6/16/22 01:16, 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 | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>> index e17d822e6051..e976f696babc 100644
>>> --- a/block/blk-mq.c
>>> +++ b/block/blk-mq.c
>>> @@ -2793,7 +2793,13 @@ static inline struct request *blk_mq_get_cached_request(struct request_queue *q,
>>>   static void bio_set_ioprio(struct bio *bio)
>>>   {
>>> +	unsigned short ioprio_class;
>>> +
>>>   	blkcg_set_ioprio(bio);
>>
>> Shouldn't this function set the default using the below "if" code ?
>>
>>> +	ioprio_class = IOPRIO_PRIO_CLASS(bio->bi_ioprio);
>>> +	/* Nobody set ioprio so far? Initialize it based on task's nice value */
>>
>> I do not think that the ioprio_class variable is useful.
>> This can be:
>>
>> 	if (IOPRIO_PRIO_CLASS(bio->bi_ioprio) == IOPRIO_CLASS_NONE)
>> 		bio->bi_ioprio = get_current_ioprio();
> 
> You are right. Fixed.

What about moving this inside blkcg_set_ioprio() ? bio_set_ioprio() would
then not be needed at all and blkcg_set_ioprio() called directly ?

> 
>>> +	if (ioprio_class == IOPRIO_CLASS_NONE)
>>> +		bio->bi_ioprio = get_current_ioprio();
>>>   }
>>>   /**
>>
>> Beside this comment, I am still scratching my head regarding what the user
>> gets with ioprio_get(). If I understood your patches correctly, the user may
>> still see IOPRIO_CLASS_NONE ?
>> For that case, to be in sync with the man page, I thought the returned
>> ioprio should be the effective one based on the task io nice value, that is,
>> the value returned by get_current_ioprio(). Am I missing something... ?
> 
> The trouble with returning "effective ioprio" is that with IOPRIO_WHO_PGRP
> or IOPRIO_WHO_USER the effective IO priority may be different for different
> processes considered and it can be also further influenced by blk-ioprio
> settings. But thinking about it now after things have settled I agree that
> what you suggests makes more sense. I'll fix that. Thanks for suggestion!
> 
> 								Honza


-- 
Damien Le Moal
Western Digital Research

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

[Jan Kara]

On Fri 17-06-22 09:04:34, Damien Le Moal wrote:
> On 6/16/22 21:24, Jan Kara wrote:
> > On Thu 16-06-22 13:23:03, Jan Kara wrote:
> >> On Thu 16-06-22 12:15:25, Damien Le Moal wrote:
> >>> On 6/16/22 01:16, Jan Kara wrote:
> >>>> +	if (ioprio_class == IOPRIO_CLASS_NONE)
> >>>> +		bio->bi_ioprio = get_current_ioprio();
> >>>>   }
> >>>>   /**
> >>>
> >>> Beside this comment, I am still scratching my head regarding what the user
> >>> gets with ioprio_get(). If I understood your patches correctly, the user may
> >>> still see IOPRIO_CLASS_NONE ?
> >>> For that case, to be in sync with the man page, I thought the returned
> >>> ioprio should be the effective one based on the task io nice value, that is,
> >>> the value returned by get_current_ioprio(). Am I missing something... ?
> >>
> >> The trouble with returning "effective ioprio" is that with IOPRIO_WHO_PGRP
> >> or IOPRIO_WHO_USER the effective IO priority may be different for different
> >> processes considered and it can be also further influenced by blk-ioprio
> >> settings. But thinking about it now after things have settled I agree that
> >> what you suggests makes more sense. I'll fix that. Thanks for suggestion!
> > 
> > Oh, now I've remembered why I've done it that way. With IOPRIO_WHO_PROCESS
> > (which is probably the most used and the best defined variant), we were
> > returning IOPRIO_CLASS_NONE if the task didn't have set IO priority until
> > commit e70344c05995 ("block: fix default IO priority handling"). So my
> > patch was just making behavior of IOPRIO_WHO_PGRP & IOPRIO_WHO_USER
> > consistent with the behavior of IOPRIO_WHO_PROCESS. I'd be reluctant to
> > change the behavior of IOPRIO_WHO_PROCESS because that has the biggest
> > chances for userspace regressions. But perhaps it makes sense to keep
> > IOPRIO_WHO_PGRP & IOPRIO_WHO_USER inconsistent with IOPRIO_WHO_PROCESS and
> > just use effective IO priority in those two variants. That looks like the
> > smallest API change to make things at least somewhat sensible...
> 
> Still bit lost. Let me try to summarize your goal:
> 
> 1) If IOPRIO_WHO_PGRP is not set, ioprio_get(IOPRIO_WHO_PGRP) will return
> the effective priority

You make it sound here like IOPRIO_WHO_PGRP would be some different type of
IO priority. For record it is not, there's just one IO priority per task,
if you set ioprio with IOPRIO_WHO_PGRP, it will just iterate all the tasks
in PGRP and set IO priority for each task. After my patches,
ioprio_get(IOPRIO_WHO_PGRPIO) will return the best of the effective IO
priorities of tasks within PGRP. Before my patch it was doing the same but
if IO priority was unset for some task it considered it to be CLASS_BE,4.

> 2) If IOPRIO_WHO_USER is not set, ioprio_get(IOPRIO_WHO_USER) will also
> return the effective priority.

This is the same as above. Just the calls iterate over all tasks of the
given user...

> 3) if IOPRIO_WHO_PROCESS is not set, return ? I am lost for this one. Do
> you want to go back to IOPRIO_CLASS_NONE ? Keep default (IOPRIO_CLASS_BE)
> ? Or switch to using the effective IO priority ? Not that the last 2
> choices are actually equivalent if the user did not IO nice the process
> (the default for the effective IO prio is class BE)
 
I want to go back to returning IOPRIO_CLASS_NONE for tasks with unset IO
priority.

> For (1) and (2), I am not sure. Given that my last changes to the ioprio
> default did not seem to have bothered anyone (nobody screamed at me :)) I
> am tempted to say: any choice is OK. So we should try to get as close as
> the man page defined behavior as possible.

I also don't find (1) and (2) too important. (3) is IMHO somewhat important
and I think that the reason why nobody complained about the change there is
because your change is relatively new so it didn't propagate yet to any
widely used distro kernel...

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

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

[Jan Kara]

On Fri 17-06-22 09:05:12, Damien Le Moal wrote:
> On 6/16/22 20:23, Jan Kara wrote:
> > On Thu 16-06-22 12:15:25, Damien Le Moal wrote:
> >> On 6/16/22 01:16, 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 | 6 ++++++
> >>>   1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/block/blk-mq.c b/block/blk-mq.c
> >>> index e17d822e6051..e976f696babc 100644
> >>> --- a/block/blk-mq.c
> >>> +++ b/block/blk-mq.c
> >>> @@ -2793,7 +2793,13 @@ static inline struct request *blk_mq_get_cached_request(struct request_queue *q,
> >>>   static void bio_set_ioprio(struct bio *bio)
> >>>   {
> >>> +	unsigned short ioprio_class;
> >>> +
> >>>   	blkcg_set_ioprio(bio);
> >>
> >> Shouldn't this function set the default using the below "if" code ?
> >>
> >>> +	ioprio_class = IOPRIO_PRIO_CLASS(bio->bi_ioprio);
> >>> +	/* Nobody set ioprio so far? Initialize it based on task's nice value */
> >>
> >> I do not think that the ioprio_class variable is useful.
> >> This can be:
> >>
> >> 	if (IOPRIO_PRIO_CLASS(bio->bi_ioprio) == IOPRIO_CLASS_NONE)
> >> 		bio->bi_ioprio = get_current_ioprio();
> > 
> > You are right. Fixed.
> 
> What about moving this inside blkcg_set_ioprio() ? bio_set_ioprio() would
> then not be needed at all and blkcg_set_ioprio() called directly ?

What I dislike about this is that it is counterintuitive that
blkcg_set_prio() does anything when blkcgs are disabled in kernel config
(and it would have to to keep things working). So if you dislike
bio_set_ioprio(), I can just opencode this function in its single call
site. Would that be better?

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

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

[Damien Le Moal]

On 6/17/22 20:52, Jan Kara wrote:
> On Fri 17-06-22 09:05:12, Damien Le Moal wrote:
>> On 6/16/22 20:23, Jan Kara wrote:
>>> On Thu 16-06-22 12:15:25, Damien Le Moal wrote:
>>>> On 6/16/22 01:16, 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 | 6 ++++++
>>>>>   1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>>>> index e17d822e6051..e976f696babc 100644
>>>>> --- a/block/blk-mq.c
>>>>> +++ b/block/blk-mq.c
>>>>> @@ -2793,7 +2793,13 @@ static inline struct request *blk_mq_get_cached_request(struct request_queue *q,
>>>>>   static void bio_set_ioprio(struct bio *bio)
>>>>>   {
>>>>> +	unsigned short ioprio_class;
>>>>> +
>>>>>   	blkcg_set_ioprio(bio);
>>>>
>>>> Shouldn't this function set the default using the below "if" code ?
>>>>
>>>>> +	ioprio_class = IOPRIO_PRIO_CLASS(bio->bi_ioprio);
>>>>> +	/* Nobody set ioprio so far? Initialize it based on task's nice value */
>>>>
>>>> I do not think that the ioprio_class variable is useful.
>>>> This can be:
>>>>
>>>> 	if (IOPRIO_PRIO_CLASS(bio->bi_ioprio) == IOPRIO_CLASS_NONE)
>>>> 		bio->bi_ioprio = get_current_ioprio();
>>>
>>> You are right. Fixed.
>>
>> What about moving this inside blkcg_set_ioprio() ? bio_set_ioprio() would
>> then not be needed at all and blkcg_set_ioprio() called directly ?
> 
> What I dislike about this is that it is counterintuitive that
> blkcg_set_prio() does anything when blkcgs are disabled in kernel config
> (and it would have to to keep things working). So if you dislike
> bio_set_ioprio(), I can just opencode this function in its single call
> site. Would that be better?

Ah, yes, blkcg may be disabled. Got it. So sure, bio_set_ioprio() is fine
as is.

> 
> 								Honza


-- 
Damien Le Moal
Western Digital Research

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

[Damien Le Moal]

On 6/17/22 20:49, Jan Kara wrote:
> On Fri 17-06-22 09:04:34, Damien Le Moal wrote:
>> On 6/16/22 21:24, Jan Kara wrote:
>>> On Thu 16-06-22 13:23:03, Jan Kara wrote:
>>>> On Thu 16-06-22 12:15:25, Damien Le Moal wrote:
>>>>> On 6/16/22 01:16, Jan Kara wrote:
>>>>>> +	if (ioprio_class == IOPRIO_CLASS_NONE)
>>>>>> +		bio->bi_ioprio = get_current_ioprio();
>>>>>>   }
>>>>>>   /**
>>>>>
>>>>> Beside this comment, I am still scratching my head regarding what the user
>>>>> gets with ioprio_get(). If I understood your patches correctly, the user may
>>>>> still see IOPRIO_CLASS_NONE ?
>>>>> For that case, to be in sync with the man page, I thought the returned
>>>>> ioprio should be the effective one based on the task io nice value, that is,
>>>>> the value returned by get_current_ioprio(). Am I missing something... ?
>>>>
>>>> The trouble with returning "effective ioprio" is that with IOPRIO_WHO_PGRP
>>>> or IOPRIO_WHO_USER the effective IO priority may be different for different
>>>> processes considered and it can be also further influenced by blk-ioprio
>>>> settings. But thinking about it now after things have settled I agree that
>>>> what you suggests makes more sense. I'll fix that. Thanks for suggestion!
>>>
>>> Oh, now I've remembered why I've done it that way. With IOPRIO_WHO_PROCESS
>>> (which is probably the most used and the best defined variant), we were
>>> returning IOPRIO_CLASS_NONE if the task didn't have set IO priority until
>>> commit e70344c05995 ("block: fix default IO priority handling"). So my
>>> patch was just making behavior of IOPRIO_WHO_PGRP & IOPRIO_WHO_USER
>>> consistent with the behavior of IOPRIO_WHO_PROCESS. I'd be reluctant to
>>> change the behavior of IOPRIO_WHO_PROCESS because that has the biggest
>>> chances for userspace regressions. But perhaps it makes sense to keep
>>> IOPRIO_WHO_PGRP & IOPRIO_WHO_USER inconsistent with IOPRIO_WHO_PROCESS and
>>> just use effective IO priority in those two variants. That looks like the
>>> smallest API change to make things at least somewhat sensible...
>>
>> Still bit lost. Let me try to summarize your goal:
>>
>> 1) If IOPRIO_WHO_PGRP is not set, ioprio_get(IOPRIO_WHO_PGRP) will return
>> the effective priority
> 
> You make it sound here like IOPRIO_WHO_PGRP would be some different type of
> IO priority. For record it is not, there's just one IO priority per task,
> if you set ioprio with IOPRIO_WHO_PGRP, it will just iterate all the tasks
> in PGRP and set IO priority for each task. After my patches,
> ioprio_get(IOPRIO_WHO_PGRPIO) will return the best of the effective IO
> priorities of tasks within PGRP. Before my patch it was doing the same but
> if IO priority was unset for some task it considered it to be CLASS_BE,4.

OK. Got it. Thanks for clarifying.

> 
>> 2) If IOPRIO_WHO_USER is not set, ioprio_get(IOPRIO_WHO_USER) will also
>> return the effective priority.
> 
> This is the same as above. Just the calls iterate over all tasks of the
> given user...
> 
>> 3) if IOPRIO_WHO_PROCESS is not set, return ? I am lost for this one. Do
>> you want to go back to IOPRIO_CLASS_NONE ? Keep default (IOPRIO_CLASS_BE)
>> ? Or switch to using the effective IO priority ? Not that the last 2
>> choices are actually equivalent if the user did not IO nice the process
>> (the default for the effective IO prio is class BE)
>  
> I want to go back to returning IOPRIO_CLASS_NONE for tasks with unset IO
> priority.

And that would be to retain the older (broken) behavior. Because if we
consider the man page, tasks with an unset IO prio should be reported as
having the effective IO nice based priority, which is class BE if IO nice
is not set. Right ? I am OK with that, but I think we should add this
explanation as a comment somewhere in the prio code. No ?

> 
>> For (1) and (2), I am not sure. Given that my last changes to the ioprio
>> default did not seem to have bothered anyone (nobody screamed at me :)) I
>> am tempted to say: any choice is OK. So we should try to get as close as
>> the man page defined behavior as possible.
> 
> I also don't find (1) and (2) too important. (3) is IMHO somewhat important
> and I think that the reason why nobody complained about the change there is
> because your change is relatively new so it didn't propagate yet to any
> widely used distro kernel...

Indeed.

> 
> 								Honza


-- 
Damien Le Moal
Western Digital Research

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

[Jan Kara]

On Fri 17-06-22 21:03:45, Damien Le Moal wrote:
> On 6/17/22 20:49, Jan Kara wrote:
> > On Fri 17-06-22 09:04:34, Damien Le Moal wrote:
> >> 2) If IOPRIO_WHO_USER is not set, ioprio_get(IOPRIO_WHO_USER) will also
> >> return the effective priority.
> > 
> > This is the same as above. Just the calls iterate over all tasks of the
> > given user...
> > 
> >> 3) if IOPRIO_WHO_PROCESS is not set, return ? I am lost for this one. Do
> >> you want to go back to IOPRIO_CLASS_NONE ? Keep default (IOPRIO_CLASS_BE)
> >> ? Or switch to using the effective IO priority ? Not that the last 2
> >> choices are actually equivalent if the user did not IO nice the process
> >> (the default for the effective IO prio is class BE)
> >  
> > I want to go back to returning IOPRIO_CLASS_NONE for tasks with unset IO
> > priority.
> 
> And that would be to retain the older (broken) behavior. Because if we
> consider the man page, tasks with an unset IO prio should be reported as
> having the effective IO nice based priority, which is class BE if IO nice
> is not set. Right ? I am OK with that, but I think we should add this
> explanation as a comment somewhere in the prio code. No ?

Adding a comment regarding this is certainly a good idea, I'll do that. WRT
whether the old behavior is broken or not - I actually think the old
behavior is more useful because it allows userspace to distinguish a
situation when IO priority is set based on nice value from a situation when
IO priority is set to a fixed value. Also the old behavior makes

  ioprio_set(pid, IOPRIO_WHO_PROCESS, ioprio_get(pid, IOPRIO_WHO_PROCESS))

a noop which is IMO a good property to have for a get/set APIs.

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

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

[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