| Message ID | 20211123161813.326307-2-axboe@kernel.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Misc block cleanups | expand |
> + /* 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.
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.
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.
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
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.
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; }
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(-)