Skip to contentSkip to conversation

Misc block cleanups

PATCH Superseded Jens Axboe 3 participants 15 messages Nov 23, 2021

Subsystems (2)
Block layer BFQ I/O scheduler

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Jens Axboe

author maintainer
#0 lore raw

cover Misc block cleanups

Hi,

First patch avoids setting up an io_context for the cases where we don't need them, second patch optimizes re-writing to the bio if not needed, and prunes a huge chunk of memory in the request_queue and makes it dynamic instead.

Signature · 1 line
Jens Axboe
author maintainer
#1 lore raw

PATCH 1/3 block: move io_context creation into where it's needed

The only user of the io_context for IO is BFQ, yet we put the checking and logic of it into the normal IO path.

Move the assignment and creation into BFQ.

Signed-off-by Jens Axboe <[email protected]>
---
 block/bfq-iosched.c  | 6 ++++++
 block/blk-core.c     | 9 ---------
 block/blk-ioc.c      | 1 +
 block/blk-mq-sched.c | 1 +
 block/blk-mq.c       | 3 ---
 5 files changed, 8 insertions(+), 12 deletions(-)
block/bfq-iosched.c +6 -0
index fec18118dc30..a82475361a7e 100644
@@ -6573,6 +6573,12 @@ static struct bfq_queue *bfq_get_bfqq_handle_split(struct bfq_data *bfqd,
  */
 static void bfq_prepare_request(struct request *rq)
 {
+	/* create task io_context, if we don't have one already */
+	if (unlikely(!current->io_context))
+		create_task_io_context(current, GFP_ATOMIC, rq->q->node);
+
+	blk_mq_sched_assign_ioc(rq);
+
 	/*
 	 * Regardless of whether we have an icq attached, we have to
 	 * clear the scheduler pointers, as they might point to
block/blk-core.c +0 -9
index 6443f2dfe43e..6ae8297b033f 100644
@@ -750,15 +750,6 @@ noinline_for_stack bool submit_bio_checks(struct bio *bio)
 		break;
 	}
 
-	/*
-	 * Various block parts want %current->io_context, so allocate it up
-	 * front rather than dealing with lots of pain to allocate it only
-	 * where needed. This may fail and the block layer knows how to live
-	 * with it.
-	 */
-	if (unlikely(!current->io_context))
-		create_task_io_context(current, GFP_ATOMIC, q->node);
-
 	if (blk_throtl_bio(bio))
 		return false;
block/blk-ioc.c +1 -0
index 57299f860d41..736e0280d76f 100644
@@ -286,6 +286,7 @@ int create_task_io_context(struct task_struct *task, gfp_t gfp_flags, int node)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(create_task_io_context);
 
 /**
  * get_task_io_context - get io_context of a task
block/blk-mq-sched.c +1 -0
index ba21449439cc..550e27189be2 100644
@@ -43,6 +43,7 @@ void blk_mq_sched_assign_ioc(struct request *rq)
 	get_io_context(icq->ioc);
 	rq->elv.icq = icq;
 }
+EXPORT_SYMBOL_GPL(blk_mq_sched_assign_ioc);
 
 /*
  * Mark a hardware queue as needing a restart. For shared queues, maintain
block/blk-mq.c +0 -3
index 4c00b22590cc..20a6445f6a01 100644
@@ -406,9 +406,6 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
 
 		if (!op_is_flush(data->cmd_flags) &&
 		    e->type->ops.prepare_request) {
-			if (e->type->icq_cache)
-				blk_mq_sched_assign_ioc(rq);
-
 			e->type->ops.prepare_request(rq);
 			rq->rq_flags |= RQF_ELVPRIV;
 		}
Signature · 1 line
2.34.0
#2 lore raw

Re: [PATCH 1/3] block: move io_context creation into where it's needed

block/bfq-iosched.c static struct bfq_queue *bfq_get_bfqq_handle_split(struct bfq_data *bfqd, · line 6578

Quoted 3 lines
block/bfq-iosched.c +3 -0
+	/* create task io_context, if we don't have one already */
+	if (unlikely(!current->io_context))
+		create_task_io_context(current, GFP_ATOMIC, rq->q->node);

Wouldn't it be nicer to have an interface that hides the check and the somewhat pointless current argument? And name that with a blk_ prefix?

block/bfq-iosched.c static struct bfq_queue *bfq_get_bfqq_handle_split(struct bfq_data *bfqd, · line 6580

Quoted 2 lines
block/bfq-iosched.c +2 -0
+
+	blk_mq_sched_assign_ioc(rq);

But thinking about this a little more:

struct io_context has two uses, one is the unsigned short ioprio, and the other is the whole bfq mess.

Can't we just split the ioprio out (we'll find a hole somewhere in task_struct) and then just allocate the io_context in blk_mq_sched_assign_ioc, which would also avoid the pointless ioc_lookup_icq on a newly allocated io_context. I'd also really expect blk_mq_sched_assign_ioc to be implemented in blk-ioc.c.

While we're at it: bfq_bic_lookup is a really weird helper which gets passed an unused bfqd argument.

author maintainer
#3 lore raw

Re: [PATCH 1/3] block: move io_context creation into where it's needed

Quoted 7 lines · Christoph Hellwig

block/bfq-iosched.c static struct bfq_queue *bfq_get_bfqq_handle_split(struct bfq_data *bfqd, · line 6578

Quoted 3 lines
block/bfq-iosched.c +3 -0
+	/* create task io_context, if we don't have one already */
+	if (unlikely(!current->io_context))
+		create_task_io_context(current, GFP_ATOMIC, rq->q->node);

Wouldn't it be nicer to have an interface that hides the check and the somewhat pointless current argument? And name that with a blk_ prefix?

Yeah, we can do that.

Quoted 5 lines

block/bfq-iosched.c static struct bfq_queue *bfq_get_bfqq_handle_split(struct bfq_data *bfqd, · line 6580

Quoted 2 lines
block/bfq-iosched.c +2 -0
+
+	blk_mq_sched_assign_ioc(rq);

But thinking about this a little more:

Quoted 8 lines

struct io_context has two uses, one is the unsigned short ioprio, and the other is the whole bfq mess.

Can't we just split the ioprio out (we'll find a hole somewhere in task_struct) and then just allocate the io_context in blk_mq_sched_assign_ioc, which would also avoid the pointless ioc_lookup_icq on a newly allocated io_context. I'd also really expect blk_mq_sched_assign_ioc to be implemented in blk-ioc.c.

It would be nice to decouple ioprio from the io_context, and just leave the io_context for BFQ essentially.

I'll give it a shot.

Quoted 2 lines

While we're at it: bfq_bic_lookup is a really weird helper which gets passed an unused bfqd argument.

Unsurprising.

Signature · 1 line
Jens Axboe
author maintainer
#4 lore raw

Re: [PATCH 1/3] block: move io_context creation into where it's needed

Quoted 25 lines · Jens Axboe
Quoted 7 lines · Christoph Hellwig

block/bfq-iosched.c static struct bfq_queue *bfq_get_bfqq_handle_split(struct bfq_data *bfqd, · line 6578

Quoted 3 lines
block/bfq-iosched.c +3 -0
+	/* create task io_context, if we don't have one already */
+	if (unlikely(!current->io_context))
+		create_task_io_context(current, GFP_ATOMIC, rq->q->node);

Wouldn't it be nicer to have an interface that hides the check and the somewhat pointless current argument? And name that with a blk_ prefix?

Yeah, we can do that.

Quoted 5 lines

block/bfq-iosched.c static struct bfq_queue *bfq_get_bfqq_handle_split(struct bfq_data *bfqd, · line 6580

Quoted 2 lines
block/bfq-iosched.c +2 -0
+
+	blk_mq_sched_assign_ioc(rq);

But thinking about this a little more:

Quoted 8 lines

struct io_context has two uses, one is the unsigned short ioprio, and the other is the whole bfq mess.

Can't we just split the ioprio out (we'll find a hole somewhere in task_struct) and then just allocate the io_context in blk_mq_sched_assign_ioc, which would also avoid the pointless ioc_lookup_icq on a newly allocated io_context. I'd also really expect blk_mq_sched_assign_ioc to be implemented in blk-ioc.c.

Quoted 4 lines

It would be nice to decouple ioprio from the io_context, and just leave the io_context for BFQ essentially.

I'll give it a shot.

Actually may not be that trivial - if a thread is cloned with CLONE_IO, then it shares the io_context, and hence also the io priority. That will auto-propagate if one linked task changes it, and putting it in the task_struct would break that.

So I don't think we can do that - which is a shame, as it would be a nice cleanup.

Signature · 1 line
Jens Axboe
#5 lore raw

Re: [PATCH 1/3] block: move io_context creation into where it's needed

Quoted 7 lines · Jens Axboe

Actually may not be that trivial - if a thread is cloned with CLONE_IO, then it shares the io_context, and hence also the io priority. That will auto-propagate if one linked task changes it, and putting it in the task_struct would break that.

So I don't think we can do that - which is a shame, as it would be a nice cleanup.

Indeed. We should still be able to move the call to create_task_io_context into blk_mq_sched_assign_ioc

author maintainer
#6 lore raw

Re: [PATCH 1/3] block: move io_context creation into where it's needed

Quoted 11 lines · Christoph Hellwig
Quoted 7 lines · Jens Axboe

Actually may not be that trivial - if a thread is cloned with CLONE_IO, then it shares the io_context, and hence also the io priority. That will auto-propagate if one linked task changes it, and putting it in the task_struct would break that.

So I don't think we can do that - which is a shame, as it would be a nice cleanup.

Indeed. We should still be able to move the call to create_task_io_context into blk_mq_sched_assign_ioc

Definitely, I'll make that change.

Signature · 1 line
Jens Axboe
author maintainer
#7 lore raw

PATCH 2/3 blk-ioprio: don't set bio priority if not needed

In reply to Jens Axboe

We don't need to write to the bio if:

1) No ioprio value has ever been assigned to the blkcg
2) We wouldn't anyway, depending on bio and blkcg IO priority

Signed-off-by Jens Axboe <[email protected]>
---
 block/blk-ioprio.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)
block/blk-ioprio.c +10 -3
index 332a07761bf8..2e7f10e1c03f 100644
@@ -62,6 +62,7 @@ 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)

@@ -112,7 +113,7 @@ 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;
 }
 

@@ -190,6 +191,10 @@ static void blkcg_ioprio_track(struct rq_qos *rqos, struct request *rq,
 			       struct bio *bio)
 {
 	struct ioprio_blkcg *blkcg = ioprio_blkcg_from_bio(bio);
+	u16 prio;
+
+	if (!blkcg->prio_set)
+		return;
 
 	/*
 	 * Except for IOPRIO_CLASS_NONE, higher I/O priority numbers

@@ -199,8 +204,10 @@ static void blkcg_ioprio_track(struct rq_qos *rqos, struct request *rq,
 	 * 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.
 	 */
-	bio->bi_ioprio = max_t(u16, bio->bi_ioprio,
-			       IOPRIO_PRIO_VALUE(blkcg->prio_policy, 0));
+	prio = max_t(u16, bio->bi_ioprio,
+			IOPRIO_PRIO_VALUE(blkcg->prio_policy, 0));
+	if (prio > bio->bi_ioprio)
+		bio->bi_ioprio = prio;
 }
 
 static void blkcg_ioprio_exit(struct rq_qos *rqos)
Signature · 1 line
2.34.0
author maintainer
#8 lore raw

PATCH 3/3 block: only allocate poll_stats if there's a user of them

In reply to Jens Axboe

This is essentially never used, yet it's about 1/3rd of the total queue size. Allocate it when needed, and don't embed it in the queue.

Signed-off-by Jens Axboe <[email protected]>
---
 block/blk-mq.c         | 20 ++++++++++++++++++--
 block/blk-stat.c       |  6 ------
 block/blk-sysfs.c      |  1 +
 include/linux/blkdev.h |  9 +++++++--
 4 files changed, 26 insertions(+), 10 deletions(-)
block/blk-mq.c +18 -2
index 20a6445f6a01..cb41c441aa8f 100644
@@ -4577,9 +4577,25 @@ EXPORT_SYMBOL_GPL(blk_mq_update_nr_hw_queues);
 /* Enable polling stats and return whether they were already enabled. */
 static bool blk_poll_stats_enable(struct request_queue *q)
 {
-	if (test_bit(QUEUE_FLAG_POLL_STATS, &q->queue_flags) ||
-	    blk_queue_flag_test_and_set(QUEUE_FLAG_POLL_STATS, q))
+	struct blk_rq_stat *poll_stat;
+
+	if (test_bit(QUEUE_FLAG_POLL_STATS, &q->queue_flags))
 		return true;
+
+	poll_stat = kzalloc(BLK_MQ_POLL_STATS_BKTS * sizeof(*poll_stat),
+				GFP_ATOMIC);
+	if (!poll_stat)
+		return false;
+
+	spin_lock_irq(&q->stats->lock);
+	if (blk_queue_flag_test_and_set(QUEUE_FLAG_POLL_STATS, q)) {
+		spin_unlock_irq(&q->stats->lock);
+		kfree(poll_stat);
+		return true;
+	}
+	q->poll_stat = poll_stat;
+	spin_unlock_irq(&q->stats->lock);
+
 	blk_stat_add_callback(q, q->poll_cb);
 	return false;
 }
block/blk-stat.c +0 -6
index ae3dd1fb8e61..7ba504166d1b 100644
@@ -12,12 +12,6 @@
 #include "blk-mq.h"
 #include "blk.h"
 
-struct blk_queue_stats {
-	struct list_head callbacks;
-	spinlock_t lock;
-	bool enable_accounting;
-};
-
 void blk_rq_stat_init(struct blk_rq_stat *stat)
 {
 	stat->min = -1ULL;
block/blk-sysfs.c +1 -0
index cd75b0f73dc6..e1b846ec58cb 100644
@@ -790,6 +790,7 @@ static void blk_release_queue(struct kobject *kobj)
 	blk_stat_free_callback(q->poll_cb);
 
 	blk_free_queue_stats(q->stats);
+	kfree(q->poll_stat);
 
 	blk_exit_queue(q);
include/linux/blkdev.h +7 -2
index bd4370baccca..b46fd2a80062 100644
@@ -28,10 +28,15 @@ struct blk_flush_queue;
 struct kiocb;
 struct pr_ops;
 struct rq_qos;
-struct blk_queue_stats;
 struct blk_stat_callback;
 struct blk_crypto_profile;
 
+struct blk_queue_stats {
+	struct list_head callbacks;
+	spinlock_t lock;
+	bool enable_accounting;
+};
+
 /* Must be consistent with blk_mq_poll_stats_bkt() */
 #define BLK_MQ_POLL_STATS_BKTS 16
 

@@ -267,7 +272,7 @@ struct request_queue {
 	int			poll_nsec;
 
 	struct blk_stat_callback	*poll_cb;
-	struct blk_rq_stat	poll_stat[BLK_MQ_POLL_STATS_BKTS];
+	struct blk_rq_stat	*poll_stat;
 
 	struct timer_list	timeout;
 	struct work_struct	timeout_work;
Signature · 1 line
2.34.0
#9 lore raw

Re: [PATCH 3/3] block: only allocate poll_stats if there's a user of them

block/blk-mq.c EXPORT_SYMBOL_GPL(blk_mq_update_nr_hw_queues); · line 4586

Quoted 2 lines · Jens Axboe
block/blk-mq.c +2 -0
+	poll_stat = kzalloc(BLK_MQ_POLL_STATS_BKTS * sizeof(*poll_stat),
+				GFP_ATOMIC);

Why not kcalloc()?

author maintainer
#10 lore raw

Re: [PATCH 3/3] block: only allocate poll_stats if there's a user of them

Quoted 5 lines · Johannes Thumshirn

block/blk-mq.c EXPORT_SYMBOL_GPL(blk_mq_update_nr_hw_queues); · line 4586

Quoted 2 lines · Jens Axboe
block/blk-mq.c +2 -0
+	poll_stat = kzalloc(BLK_MQ_POLL_STATS_BKTS * sizeof(*poll_stat),
+				GFP_ATOMIC);

Why not kcalloc()?

Sure, can do.

Signature · 1 line
Jens Axboe
#11 lore raw

Re: [PATCH 3/3] block: only allocate poll_stats if there's a user of them

In reply to Jens Axboe
Quoted 16 lines · Jens Axboe

This is essentially never used, yet it's about 1/3rd of the total queue size. Allocate it when needed, and don't embed it in the queue.

Signed-off-by Jens Axboe <[email protected]>
---
 block/blk-mq.c         | 20 ++++++++++++++++++--
 block/blk-stat.c       |  6 ------
 block/blk-sysfs.c      |  1 +
 include/linux/blkdev.h |  9 +++++++--
 4 files changed, 26 insertions(+), 10 deletions(-)
block/blk-mq.c +0 -0
index 20a6445f6a01..cb41c441aa8f 100644

block/blk-mq.c EXPORT_SYMBOL_GPL(blk_mq_update_nr_hw_queues); · line 4583

Quoted 9 lines
block/blk-mq.c +3 -2
@@ -4577,9 +4577,25 @@ EXPORT_SYMBOL_GPL(blk_mq_update_nr_hw_queues);
 /* Enable polling stats and return whether they were already enabled. */
 static bool blk_poll_stats_enable(struct request_queue *q)
 {
-	if (test_bit(QUEUE_FLAG_POLL_STATS, &q->queue_flags) ||
-	    blk_queue_flag_test_and_set(QUEUE_FLAG_POLL_STATS, q))
+	struct blk_rq_stat *poll_stat;
+
+	if (test_bit(QUEUE_FLAG_POLL_STATS, &q->queue_flags))
 		return true;

Can't we replace the checks for QUEUE_FLAG_POLL_STATS with checks for q->poll_stat now?

author maintainer
#12 lore raw

Re: [PATCH 3/3] block: only allocate poll_stats if there's a user of them

Quoted 21 lines · Christoph Hellwig
Quoted 20 lines · Jens Axboe

This is essentially never used, yet it's about 1/3rd of the total queue size. Allocate it when needed, and don't embed it in the queue.

Signed-off-by Jens Axboe <[email protected]>
---
 block/blk-mq.c         | 20 ++++++++++++++++++--
 block/blk-stat.c       |  6 ------
 block/blk-sysfs.c      |  1 +
 include/linux/blkdev.h |  9 +++++++--
 4 files changed, 26 insertions(+), 10 deletions(-)
block/blk-mq.c +0 -1
index 20a6445f6a01..cb41c441aa8f 100644
@@ -4577,9 +4577,25 @@ EXPORT_SYMBOL_GPL(blk_mq_update_nr_hw_queues);
 /* Enable polling stats and return whether they were already enabled. */
 static bool blk_poll_stats_enable(struct request_queue *q)
 {
-	if (test_bit(QUEUE_FLAG_POLL_STATS, &q->queue_flags) ||
Quoted 8 lines

block/blk-mq.c EXPORT_SYMBOL_GPL(blk_mq_update_nr_hw_queues); · line 4583

Quoted 5 lines
block/blk-mq.c +3 -1
-	    blk_queue_flag_test_and_set(QUEUE_FLAG_POLL_STATS, q))
+	struct blk_rq_stat *poll_stat;
+
+	if (test_bit(QUEUE_FLAG_POLL_STATS, &q->queue_flags))
 		return true;

Can't we replace the checks for QUEUE_FLAG_POLL_STATS with checks for q->poll_stat now?

I think so:

block/blk-mq.c +3 -4
index f011fa3ebcc7..af4580bdf931 100644
@@ -4593,7 +4593,7 @@ static bool blk_poll_stats_enable(struct request_queue *q)
 {
 	struct blk_rq_stat *poll_stat;
 
-	if (test_bit(QUEUE_FLAG_POLL_STATS, &q->queue_flags))
+	if (q->poll_stat)
 		return true;
 
 	poll_stat = kzalloc(BLK_MQ_POLL_STATS_BKTS * sizeof(*poll_stat),

@@ -4602,7 +4602,7 @@ static bool blk_poll_stats_enable(struct request_queue *q)
 		return false;
 
 	spin_lock_irq(&q->stats->lock);
-	if (blk_queue_flag_test_and_set(QUEUE_FLAG_POLL_STATS, q)) {
+	if (q->poll_stat) {
 		spin_unlock_irq(&q->stats->lock);
 		kfree(poll_stat);
 		return true;

@@ -4620,8 +4620,7 @@ static void blk_mq_poll_stats_start(struct request_queue *q)
 	 * We don't arm the callback if polling stats are not enabled or the
 	 * callback is already active.
 	 */
-	if (!test_bit(QUEUE_FLAG_POLL_STATS, &q->queue_flags) ||
-	    blk_stat_is_active(q->poll_cb))
+	if (!q->poll_stat || blk_stat_is_active(q->poll_cb))
 		return;
 
 	blk_stat_activate_msecs(q->poll_cb, 100);
block/blk-sysfs.c +1 -1
index e1b846ec58cb..c079be1c58a3 100644
@@ -785,7 +785,7 @@ static void blk_release_queue(struct kobject *kobj)
 
 	might_sleep();
 
-	if (test_bit(QUEUE_FLAG_POLL_STATS, &q->queue_flags))
+	if (q->poll_stat)
 		blk_stat_remove_callback(q, q->poll_cb);
 	blk_stat_free_callback(q->poll_cb);
include/linux/blkdev.h +0 -1
index 3d558cb397d5..20cf877d6627 100644
@@ -414,7 +414,6 @@ struct request_queue {
 #define QUEUE_FLAG_FUA		18	/* device supports FUA writes */
 #define QUEUE_FLAG_DAX		19	/* device supports DAX */
 #define QUEUE_FLAG_STATS	20	/* track IO start and completion times */
-#define QUEUE_FLAG_POLL_STATS	21	/* collecting stats for hybrid polling */
 #define QUEUE_FLAG_REGISTERED	22	/* queue has been registered to a disk */
 #define QUEUE_FLAG_QUIESCED	24	/* queue has been quiesced */
 #define QUEUE_FLAG_PCI_P2PDMA	25	/* device supports PCI p2p requests */
Signature · 1 line
Jens Axboe
#13 lore raw

Re: [PATCH 3/3] block: only allocate poll_stats if there's a user of them

Quoted 1 line · Jens Axboe

I think so:

I think my eternal enemy in blk-mq-debugfs.c will need an update for the flag removal, but otherwise this looks good.

author maintainer
#14 lore raw

Re: [PATCH 3/3] block: only allocate poll_stats if there's a user of them

Quoted 5 lines · Christoph Hellwig
Quoted 1 line · Jens Axboe

I think so:

I think my eternal enemy in blk-mq-debugfs.c will need an update for the flag removal, but otherwise this looks good.

Heh yes, it always evades grep.

Signature · 1 line
Jens Axboe