[v4,1/5] block: Fix bio IO priority setting

Commit Message

Hongyu Jin Dec. 12, 2023, 11:11 a.m. UTC

From: Hongyu Jin <[e-mail]>

Move bio_set_ioprio() into submit_bio():
1. Only call bio_set_ioprio() once to set the priority of original bio,
   the bio that cloned and splited from original bio will auto inherit
   the priority of original bio in clone process.

2. The IO priority can be passed to module that implement
   struct gendisk::fops::submit_bio, help resolve some
   of the IO priority loss issues.

This patch depends on commit 82b74cac2849 ("blk-ioprio: Convert from
rqos policy to direct call")

Fixes: a78418e6a04c ("block: Always initialize bio IO priority on submit")

Co-developed-by: Yibin Ding <[e-mail]>
Signed-off-by: Yibin Ding <[e-mail]>
Signed-off-by: Hongyu Jin <[e-mail]>
---
 block/blk-core.c | 10 ++++++++++
 block/blk-mq.c   | 11 -----------
 2 files changed, 10 insertions(+), 11 deletions(-)

Comments

Christoph Hellwig Dec. 12, 2023, 1:13 p.m. UTC | #1

On Tue, Dec 12, 2023 at 07:11:46PM +0800, Hongyu Jin wrote:
> From: Hongyu Jin <[e-mail]>
> 
> Move bio_set_ioprio() into submit_bio():
> 1. Only call bio_set_ioprio() once to set the priority of original bio,
>    the bio that cloned and splited from original bio will auto inherit
>    the priority of original bio in clone process.
> 
> 2. The IO priority can be passed to module that implement
>    struct gendisk::fops::submit_bio, help resolve some
>    of the IO priority loss issues.

Can we reword this a bit.  AFAICS what this primarily does it to ensure
the priority is set before dispatching to submit_bio based drivers or
blk-mq instead of just blk-mq, and the rest follows from that.

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

I don't think we need the check here as anyone resubmitting a bio should
be using submit_bio_noact.

Mike Snitzer Dec. 12, 2023, 6:02 p.m. UTC | #2

On Tue, Dec 12 2023 at  8:13P -0500,
Christoph Hellwig <[e-mail]> wrote:

> On Tue, Dec 12, 2023 at 07:11:46PM +0800, Hongyu Jin wrote:
> > From: Hongyu Jin <[e-mail]>
> > 
> > Move bio_set_ioprio() into submit_bio():
> > 1. Only call bio_set_ioprio() once to set the priority of original bio,
> >    the bio that cloned and splited from original bio will auto inherit
> >    the priority of original bio in clone process.
> > 
> > 2. The IO priority can be passed to module that implement
> >    struct gendisk::fops::submit_bio, help resolve some
> >    of the IO priority loss issues.
> 
> Can we reword this a bit.  AFAICS what this primarily does it to ensure
> the priority is set before dispatching to submit_bio based drivers or
> blk-mq instead of just blk-mq, and the rest follows from that.

Yeah, I agree.. something like:

Move bio_set_ioprio() and caller up from blk_mq_submit_bio() to
submit_bio(). This ensures all block drivers call bio_set_ioprio()
during initial bio submission.
 
> > +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);
> > +}
> 
> I don't think we need the check here as anyone resubmitting a bio should
> be using submit_bio_noact.

This patch moves the caller from blk_mq_submit_bio() to submit_bio().

So I'm not sure why you're seizing on the "resubmitting a bio" usecase
as reason for dropping this check (which occurs in submit_bio).

The original justification for the check is detailed in commit
a78418e6a04c93b ("block: Always initialize bio IO priority on submit").

Mike

Patch

diff --git a/block/blk-core.c b/block/blk-core.c
index fdf25b8d6e78..68158c327aea 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -49,6 +49,7 @@ 
 #include "blk-pm.h"
 #include "blk-cgroup.h"
 #include "blk-throttle.h"
+#include "blk-ioprio.h"
 
 struct dentry *blk_debugfs_root;
 
@@ -809,6 +810,14 @@  void submit_bio_noacct(struct bio *bio)
 }
 EXPORT_SYMBOL(submit_bio_noacct);
 
+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);
+}
+
 /**
  * submit_bio - submit a bio to the block device layer for I/O
  * @bio: The &struct bio which describes the I/O
@@ -831,6 +840,7 @@  void submit_bio(struct bio *bio)
 		count_vm_events(PGPGOUT, bio_sectors(bio));
 	}
 
+	bio_set_ioprio(bio);
 	submit_bio_noacct(bio);
 }
 EXPORT_SYMBOL(submit_bio);
diff --git a/block/blk-mq.c b/block/blk-mq.c
index e2d11183f62e..a6e2609df9c9 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -40,7 +40,6 @@ 
 #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);
 static DEFINE_PER_CPU(call_single_data_t, blk_cpu_csd);
@@ -2922,14 +2921,6 @@  static inline struct request *blk_mq_get_cached_request(struct request_queue *q,
 	return rq;
 }
 
-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);
-}
-
 /**
  * blk_mq_submit_bio - Create and send a request to block device.
  * @bio: Bio pointer.
@@ -2963,8 +2954,6 @@  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)