* [PATCHSET 0/3] Misc block cleanups
@ 2021-11-23 16:18 Jens Axboe
2021-11-23 16:18 ` [PATCH 1/3] block: move io_context creation into where it's needed Jens Axboe
` (2 more replies)
0 siblings, 3 replies; 18+ messages in thread
From: Jens Axboe @ 2021-11-23 16:18 UTC (permalink / raw)
To: linux-block
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.
--
Jens Axboe
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/3] block: move io_context creation into where it's needed
2021-11-23 16:18 [PATCHSET 0/3] Misc block cleanups Jens Axboe
@ 2021-11-23 16:18 ` Jens Axboe
2021-11-23 16:39 ` Christoph Hellwig
2021-11-23 16:18 ` [PATCH 2/3] blk-ioprio: don't set bio priority if not needed Jens Axboe
2021-11-23 16:18 ` [PATCH 3/3] block: only allocate poll_stats if there's a user of them Jens Axboe
2 siblings, 1 reply; 18+ messages in thread
From: Jens Axboe @ 2021-11-23 16:18 UTC (permalink / raw)
To: linux-block; +Cc: Jens Axboe
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 <axboe@kernel.dk>
---
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(-)
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index fec18118dc30..a82475361a7e 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -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
diff --git a/block/blk-core.c b/block/blk-core.c
index 6443f2dfe43e..6ae8297b033f 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -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;
diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index 57299f860d41..736e0280d76f 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -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
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index ba21449439cc..550e27189be2 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -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
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 4c00b22590cc..20a6445f6a01 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -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;
}
--
2.34.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/3] blk-ioprio: don't set bio priority if not needed
2021-11-23 16:18 [PATCHSET 0/3] Misc block cleanups Jens Axboe
2021-11-23 16:18 ` [PATCH 1/3] block: move io_context creation into where it's needed Jens Axboe
@ 2021-11-23 16:18 ` Jens Axboe
2021-11-23 16:18 ` [PATCH 3/3] block: only allocate poll_stats if there's a user of them Jens Axboe
2 siblings, 0 replies; 18+ messages in thread
From: Jens Axboe @ 2021-11-23 16:18 UTC (permalink / raw)
To: linux-block; +Cc: 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 <axboe@kernel.dk>
---
block/blk-ioprio.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/block/blk-ioprio.c b/block/blk-ioprio.c
index 332a07761bf8..2e7f10e1c03f 100644
--- a/block/blk-ioprio.c
+++ b/block/blk-ioprio.c
@@ -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)
--
2.34.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 3/3] block: only allocate poll_stats if there's a user of them
2021-11-23 16:18 [PATCHSET 0/3] Misc block cleanups Jens Axboe
2021-11-23 16:18 ` [PATCH 1/3] block: move io_context creation into where it's needed Jens Axboe
2021-11-23 16:18 ` [PATCH 2/3] blk-ioprio: don't set bio priority if not needed Jens Axboe
@ 2021-11-23 16:18 ` Jens Axboe
2021-11-23 16:21 ` Johannes Thumshirn
2021-11-23 16:41 ` Christoph Hellwig
2 siblings, 2 replies; 18+ messages in thread
From: Jens Axboe @ 2021-11-23 16:18 UTC (permalink / raw)
To: linux-block; +Cc: 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 <axboe@kernel.dk>
---
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(-)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 20a6445f6a01..cb41c441aa8f 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -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;
}
diff --git a/block/blk-stat.c b/block/blk-stat.c
index ae3dd1fb8e61..7ba504166d1b 100644
--- a/block/blk-stat.c
+++ b/block/blk-stat.c
@@ -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;
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index cd75b0f73dc6..e1b846ec58cb 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -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);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index bd4370baccca..b46fd2a80062 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -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;
--
2.34.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] block: only allocate poll_stats if there's a user of them
2021-11-23 16:18 ` [PATCH 3/3] block: only allocate poll_stats if there's a user of them Jens Axboe
@ 2021-11-23 16:21 ` Johannes Thumshirn
2021-11-23 16:27 ` Jens Axboe
2021-11-23 16:41 ` Christoph Hellwig
1 sibling, 1 reply; 18+ messages in thread
From: Johannes Thumshirn @ 2021-11-23 16:21 UTC (permalink / raw)
To: Jens Axboe, linux-block
On 23/11/2021 17:18, Jens Axboe wrote:
> + poll_stat = kzalloc(BLK_MQ_POLL_STATS_BKTS * sizeof(*poll_stat),
> + GFP_ATOMIC);
Why not kcalloc()?
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] block: only allocate poll_stats if there's a user of them
2021-11-23 16:21 ` Johannes Thumshirn
@ 2021-11-23 16:27 ` Jens Axboe
0 siblings, 0 replies; 18+ messages in thread
From: Jens Axboe @ 2021-11-23 16:27 UTC (permalink / raw)
To: Johannes Thumshirn, linux-block
On 11/23/21 9:21 AM, Johannes Thumshirn wrote:
> On 23/11/2021 17:18, Jens Axboe wrote:
>> + poll_stat = kzalloc(BLK_MQ_POLL_STATS_BKTS * sizeof(*poll_stat),
>> + GFP_ATOMIC);
>
> Why not kcalloc()?
Sure, can do.
--
Jens Axboe
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] block: move io_context creation into where it's needed
2021-11-23 16:18 ` [PATCH 1/3] block: move io_context creation into where it's needed Jens Axboe
@ 2021-11-23 16:39 ` Christoph Hellwig
2021-11-23 16:46 ` Jens Axboe
0 siblings, 1 reply; 18+ messages in thread
From: Christoph Hellwig @ 2021-11-23 16:39 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block
> + /* 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?
> +
> + 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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] block: only allocate poll_stats if there's a user of them
2021-11-23 16:18 ` [PATCH 3/3] block: only allocate poll_stats if there's a user of them Jens Axboe
2021-11-23 16:21 ` Johannes Thumshirn
@ 2021-11-23 16:41 ` Christoph Hellwig
2021-11-23 16:44 ` Jens Axboe
1 sibling, 1 reply; 18+ messages in thread
From: Christoph Hellwig @ 2021-11-23 16:41 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block
On Tue, Nov 23, 2021 at 09:18:13AM -0700, Jens Axboe wrote:
> 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 <axboe@kernel.dk>
> ---
> 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(-)
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 20a6445f6a01..cb41c441aa8f 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -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?
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] block: only allocate poll_stats if there's a user of them
2021-11-23 16:41 ` Christoph Hellwig
@ 2021-11-23 16:44 ` Jens Axboe
2021-11-23 17:05 ` Christoph Hellwig
0 siblings, 1 reply; 18+ messages in thread
From: Jens Axboe @ 2021-11-23 16:44 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-block
On 11/23/21 9:41 AM, Christoph Hellwig wrote:
> On Tue, Nov 23, 2021 at 09:18:13AM -0700, Jens Axboe wrote:
>> 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 <axboe@kernel.dk>
>> ---
>> 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(-)
>>
>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>> index 20a6445f6a01..cb41c441aa8f 100644
>> --- a/block/blk-mq.c
>> +++ b/block/blk-mq.c
>> @@ -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?
I think so:
diff --git a/block/blk-mq.c b/block/blk-mq.c
index f011fa3ebcc7..af4580bdf931 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -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);
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index e1b846ec58cb..c079be1c58a3 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -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);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 3d558cb397d5..20cf877d6627 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -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 */
--
Jens Axboe
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] block: move io_context creation into where it's needed
2021-11-23 16:39 ` Christoph Hellwig
@ 2021-11-23 16:46 ` Jens Axboe
2021-11-23 16:53 ` Jens Axboe
0 siblings, 1 reply; 18+ messages in thread
From: Jens Axboe @ 2021-11-23 16:46 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-block
On 11/23/21 9:39 AM, Christoph Hellwig wrote:
>> + /* 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.
>> +
>> + 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.
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.
> While we're at it: bfq_bic_lookup is a really weird helper which
> gets passed an unused bfqd argument.
Unsurprising.
--
Jens Axboe
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] block: move io_context creation into where it's needed
2021-11-23 16:46 ` Jens Axboe
@ 2021-11-23 16:53 ` Jens Axboe
2021-11-23 16:56 ` Christoph Hellwig
0 siblings, 1 reply; 18+ messages in thread
From: Jens Axboe @ 2021-11-23 16:53 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-block
On 11/23/21 9:46 AM, Jens Axboe wrote:
> On 11/23/21 9:39 AM, Christoph Hellwig wrote:
>>> + /* 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.
>
>>> +
>>> + 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.
>
> 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.
--
Jens Axboe
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] block: move io_context creation into where it's needed
2021-11-23 16:53 ` Jens Axboe
@ 2021-11-23 16:56 ` Christoph Hellwig
2021-11-23 17:04 ` Jens Axboe
0 siblings, 1 reply; 18+ messages in thread
From: Christoph Hellwig @ 2021-11-23 16:56 UTC (permalink / raw)
To: Jens Axboe; +Cc: Christoph Hellwig, linux-block
On Tue, Nov 23, 2021 at 09:53:58AM -0700, Jens Axboe wrote:
> 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] block: move io_context creation into where it's needed
2021-11-23 16:56 ` Christoph Hellwig
@ 2021-11-23 17:04 ` Jens Axboe
0 siblings, 0 replies; 18+ messages in thread
From: Jens Axboe @ 2021-11-23 17:04 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-block
On 11/23/21 9:56 AM, Christoph Hellwig wrote:
> On Tue, Nov 23, 2021 at 09:53:58AM -0700, Jens Axboe wrote:
>> 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.
--
Jens Axboe
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] block: only allocate poll_stats if there's a user of them
2021-11-23 16:44 ` Jens Axboe
@ 2021-11-23 17:05 ` Christoph Hellwig
2021-11-23 17:06 ` Jens Axboe
0 siblings, 1 reply; 18+ messages in thread
From: Christoph Hellwig @ 2021-11-23 17:05 UTC (permalink / raw)
To: Jens Axboe; +Cc: Christoph Hellwig, linux-block
On Tue, Nov 23, 2021 at 09:44:18AM -0700, Jens Axboe wrote:
> 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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] block: only allocate poll_stats if there's a user of them
2021-11-23 17:05 ` Christoph Hellwig
@ 2021-11-23 17:06 ` Jens Axboe
0 siblings, 0 replies; 18+ messages in thread
From: Jens Axboe @ 2021-11-23 17:06 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-block
On 11/23/21 10:05 AM, Christoph Hellwig wrote:
> On Tue, Nov 23, 2021 at 09:44:18AM -0700, Jens Axboe wrote:
>> 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.
--
Jens Axboe
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] block: move io_context creation into where it's needed
2021-11-23 18:46 ` Christoph Hellwig
@ 2021-11-23 18:58 ` Jens Axboe
0 siblings, 0 replies; 18+ messages in thread
From: Jens Axboe @ 2021-11-23 18:58 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-block
On 11/23/21 11:46 AM, Christoph Hellwig wrote:
> On Tue, Nov 23, 2021 at 10:10:56AM -0700, Jens Axboe wrote:
>> --- a/block/blk-ioc.c
>> +++ b/block/blk-ioc.c
>> @@ -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);
>
> No need to export this now.
Indeed, killed.
--
Jens Axboe
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] block: move io_context creation into where it's needed
2021-11-23 17:10 ` [PATCH 1/3] block: move io_context creation into where it's needed Jens Axboe
@ 2021-11-23 18:46 ` Christoph Hellwig
2021-11-23 18:58 ` Jens Axboe
0 siblings, 1 reply; 18+ messages in thread
From: Christoph Hellwig @ 2021-11-23 18:46 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block
On Tue, Nov 23, 2021 at 10:10:56AM -0700, Jens Axboe wrote:
> --- a/block/blk-ioc.c
> +++ b/block/blk-ioc.c
> @@ -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);
No need to export this now.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/3] block: move io_context creation into where it's needed
2021-11-23 17:10 [PATCHSET 0/3 v2] Misc block cleanups Jens Axboe
@ 2021-11-23 17:10 ` Jens Axboe
2021-11-23 18:46 ` Christoph Hellwig
0 siblings, 1 reply; 18+ messages in thread
From: Jens Axboe @ 2021-11-23 17:10 UTC (permalink / raw)
To: linux-block; +Cc: Jens Axboe
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.
Put the creation into blk_mq_sched_assign_ioc(), and have BFQ use that
helper.
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
block/bfq-iosched.c | 2 ++
block/blk-core.c | 9 ---------
block/blk-ioc.c | 1 +
block/blk-mq-sched.c | 5 +++++
block/blk-mq.c | 3 ---
5 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index fec18118dc30..1ce1a99a7160 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -6573,6 +6573,8 @@ static struct bfq_queue *bfq_get_bfqq_handle_split(struct bfq_data *bfqd,
*/
static void bfq_prepare_request(struct request *rq)
{
+ 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
diff --git a/block/blk-core.c b/block/blk-core.c
index 6443f2dfe43e..6ae8297b033f 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -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;
diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index 57299f860d41..736e0280d76f 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -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
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index ba21449439cc..b942b38000e5 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -24,6 +24,10 @@ void blk_mq_sched_assign_ioc(struct request *rq)
struct io_context *ioc;
struct io_cq *icq;
+ /* create task io_context, if we don't have one already */
+ if (unlikely(!current->io_context))
+ create_task_io_context(current, GFP_ATOMIC, q->node);
+
/*
* May not have an IO context if it's a passthrough request
*/
@@ -43,6 +47,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
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 4c00b22590cc..20a6445f6a01 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -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;
}
--
2.34.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
end of thread, other threads:[~2021-11-23 18:59 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-23 16:18 [PATCHSET 0/3] Misc block cleanups Jens Axboe
2021-11-23 16:18 ` [PATCH 1/3] block: move io_context creation into where it's needed Jens Axboe
2021-11-23 16:39 ` Christoph Hellwig
2021-11-23 16:46 ` Jens Axboe
2021-11-23 16:53 ` Jens Axboe
2021-11-23 16:56 ` Christoph Hellwig
2021-11-23 17:04 ` Jens Axboe
2021-11-23 16:18 ` [PATCH 2/3] blk-ioprio: don't set bio priority if not needed Jens Axboe
2021-11-23 16:18 ` [PATCH 3/3] block: only allocate poll_stats if there's a user of them Jens Axboe
2021-11-23 16:21 ` Johannes Thumshirn
2021-11-23 16:27 ` Jens Axboe
2021-11-23 16:41 ` Christoph Hellwig
2021-11-23 16:44 ` Jens Axboe
2021-11-23 17:05 ` Christoph Hellwig
2021-11-23 17:06 ` Jens Axboe
2021-11-23 17:10 [PATCHSET 0/3 v2] Misc block cleanups Jens Axboe
2021-11-23 17:10 ` [PATCH 1/3] block: move io_context creation into where it's needed Jens Axboe
2021-11-23 18:46 ` Christoph Hellwig
2021-11-23 18:58 ` Jens Axboe
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.