remove aux CQE caches
Subsystems (4)
v2 cover remove aux CQE caches
Patches 1-7 are cleanups mainly dealing with issue_flags conversions, misundertsandings of the flags and of the tw state. It'd be great to have even without even w/o the rest.
8-11 mandate ctx locking for task_work and finally removes the CQE caches, instead we post directly into the CQ. Note that the cache is used by multishot auxiliary completions.
Patches 12-14 are additional cleanups that were sent out in a separate patchset before.
Beware, there are problems reported coming from ublk testing, which may or may not be due to unrelated problems with for-next.
v2: Add Patch 3, which fixes deadlock due to nested locking
introduced in v1
Remove a fix, which was taken separately
Pile up more cleanups (Patches 12-14)
Pavel Begunkov (14):
io_uring/cmd: kill one issue_flags to tw conversion io_uring/cmd: fix tw <-> issue_flags conversion io_uring/cmd: make io_uring_cmd_done irq safe io_uring/cmd: introduce io_uring_cmd_complete
ublk: don't hard code IO_URING_F_UNLOCKED nvme/io_uring: don't hard code IO_URING_F_UNLOCKED io_uring/rw: avoid punting to io-wq directly
io_uring: force tw ctx locking
io_uring: remove struct io_tw_state::locked
io_uring: refactor io_fill_cqe_req_aux
io_uring: get rid of intermediate aux cqe caches
io_uring: remove current check from complete_post
io_uring: refactor io_req_complete_post()
io_uring: clean up io_lockdep_assert_cq_locked
drivers/block/ublk_drv.c | 18 ++-- drivers/nvme/host/ioctl.c | 9 +- include/linux/io_uring/cmd.h | 24 +++++ include/linux/io_uring_types.h | 5 +- io_uring/io_uring.c | 161 +++++++++------------------------ io_uring/io_uring.h | 17 +--- io_uring/net.c | 6 +- io_uring/poll.c | 3 +- io_uring/rw.c | 18 +--- io_uring/timeout.c | 8 +- io_uring/uring_cmd.c | 36 +++++--- io_uring/waitid.c | 2 +- 12 files changed, 122 insertions(+), 185 deletions(-)
Signature · 1 line
2.44.0
PATCH v2 1/14 io_uring/cmd: kill one issue_flags to tw conversion
io_uring cmd converts struct io_tw_state to issue_flags and later back to io_tw_state, it's awfully ill-fated, not to mention that intermediate issue_flags state is not correct.
Get rid of the last conversion, drag through tw everything that came with IO_URING_F_UNLOCKED, and replace io_req_complete_defer() with a direct call to io_req_complete_defer(), at least for the time being.
--- io_uring/uring_cmd.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
io_uring/uring_cmd.c +4 -4
@@ -100,11 +100,11 @@ void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2,if (req->ctx->flags &IORING_SETUP_IOPOLL ) {/* order with io_iopoll_req_issued() checking ->iopoll_complete */ smp_store_release (&req->iopoll_completed,1 ); + }else if (!(issue_flags &IO_URING_F_UNLOCKED )) { +io_req_complete_defer (req); }else { -struct io_tw_state ts = { - .locked = !(issue_flags &IO_URING_F_UNLOCKED ), - }; -io_req_task_complete (req, &ts); + req->io_task_work.func = io_req_task_complete; +io_req_task_work_add (req); } }EXPORT_SYMBOL_GPL (io_uring_cmd_done);
Signature · 1 line
2.44.0
PATCH v2 2/14 io_uring/cmd: fix tw <-> issue_flags conversion
In reply to Pavel Begunkov!IO_URING_F_UNLOCKED does not translate to availability of the deferred completion infra, IO_URING_F_COMPLETE_DEFER does, that what we should pass and look for to use io_req_complete_defer() and other variants.
Luckily, it's not a real problem as two wrongs actually made it right, at least as far as io_uring_cmd_work() goes.
--- io_uring/uring_cmd.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
io_uring/uring_cmd.c +8 -2
@@ -56,7 +56,11 @@ EXPORT_SYMBOL_GPL(io_uring_cmd_mark_cancelable);static void io_uring_cmd_work (struct io_kiocb *req,struct io_tw_state *ts) { struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd); -unsigned issue_flags = ts->locked ?0 :IO_URING_F_UNLOCKED ; +unsigned issue_flags =IO_URING_F_UNLOCKED ; + +/* locked task_work executor checks the deffered list completion */ +if (ts->locked) + issue_flags =IO_URING_F_COMPLETE_DEFER ; ioucmd->task_work_cb (ioucmd, issue_flags); }
@@ -100,7 +104,9 @@ void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2,if (req->ctx->flags &IORING_SETUP_IOPOLL ) {/* order with io_iopoll_req_issued() checking ->iopoll_complete */ smp_store_release (&req->iopoll_completed,1 ); - }else if (!(issue_flags &IO_URING_F_UNLOCKED )) { + }else if (issue_flags &IO_URING_F_COMPLETE_DEFER ) { +if (WARN_ON_ONCE (issue_flags &IO_URING_F_UNLOCKED )) +return ;io_req_complete_defer (req); }else { req->io_task_work.func = io_req_task_complete;
Signature · 1 line
2.44.0
Re: [PATCH v2 02/14] io_uring/cmd: fix tw <-> issue_flags conversion
Quoted 32 lines · Pavel Begunkov
!IO_URING_F_UNLOCKED does not translate to availability of the deferred completion infra,
IO_URING_F_COMPLETE_DEFERdoes, that what we should pass and look for to useio_req_complete_defer()and other variants.Luckily, it's not a real problem as two wrongs actually made it right, at least as far as
io_uring_cmd_work()goes.Signed-off-by Pavel Begunkov <[email protected]>Link https://lore.kernel.org/r/eb08e72e837106963bc7bc7dccfd93d646cc7f36.1710514702.git.asml.silence@gmail.com open hereSigned-off-by Jens Axboe <[email protected]>--- io_uring/uring_cmd.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)io_uring/uring_cmd.c +5
-1@@ -56,7 +56,11 @@ EXPORT_SYMBOL_GPL(io_uring_cmd_mark_cancelable);static void io_uring_cmd_work (struct io_kiocb *req,struct io_tw_state *ts) { struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd); -unsigned issue_flags = ts->locked ?0 :IO_URING_F_UNLOCKED ; +unsigned issue_flags =IO_URING_F_UNLOCKED ; + +/* locked task_work executor checks the deffered list completion */ +if (ts->locked) + issue_flags =IO_URING_F_COMPLETE_DEFER ; ioucmd->task_work_cb (ioucmd, issue_flags); }
io_uring/uring_cmd.c void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2, · line 112
Quoted 10 lines
io_uring/uring_cmd.c +3
-1@@ -100,7 +104,9 @@ void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2,if (req->ctx->flags &IORING_SETUP_IOPOLL ) {/* order with io_iopoll_req_issued() checking ->iopoll_complete */ smp_store_release (&req->iopoll_completed,1 ); - }else if (!(issue_flags &IO_URING_F_UNLOCKED )) { + }else if (issue_flags &IO_URING_F_COMPLETE_DEFER ) { +if (WARN_ON_ONCE (issue_flags &IO_URING_F_UNLOCKED )) +return ;io_req_complete_defer (req); }else { req->io_task_work.func = io_req_task_complete;
'git-bisect' shows the reported warning starts from this patch.
Thanks,
Ming
Jens Axboe
maintainerRe: [PATCH v2 02/14] io_uring/cmd: fix tw <-> issue_flags conversion
Quoted 30 lines · Ming Lei
Quoted 29 lines · Pavel Begunkov
!IO_URING_F_UNLOCKED does not translate to availability of the deferred completion infra,
IO_URING_F_COMPLETE_DEFERdoes, that what we should pass and look for to useio_req_complete_defer()and other variants.Luckily, it's not a real problem as two wrongs actually made it right, at least as far as
io_uring_cmd_work()goes.Signed-off-by Pavel Begunkov <[email protected]>Link https://lore.kernel.org/r/eb08e72e837106963bc7bc7dccfd93d646cc7f36.1710514702.git.asml.silence@gmail.com open hereSigned-off-by Jens Axboe <[email protected]>--- io_uring/uring_cmd.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)io_uring/uring_cmd.c +5
-1@@ -56,7 +56,11 @@ EXPORT_SYMBOL_GPL(io_uring_cmd_mark_cancelable);static void io_uring_cmd_work (struct io_kiocb *req,struct io_tw_state *ts) { struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd); -unsigned issue_flags = ts->locked ?0 :IO_URING_F_UNLOCKED ; +unsigned issue_flags =IO_URING_F_UNLOCKED ; + +/* locked task_work executor checks the deffered list completion */ +if (ts->locked) + issue_flags =IO_URING_F_COMPLETE_DEFER ;
Quoted 15 lines
io_uring/uring_cmd.c void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2, · line 112
Quoted 13 lines
io_uring/uring_cmd.c +3
-1ioucmd->task_work_cb (ioucmd, issue_flags); }@@ -100,7 +104,9 @@ void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2,if (req->ctx->flags &IORING_SETUP_IOPOLL ) {/* order with io_iopoll_req_issued() checking ->iopoll_complete */ smp_store_release (&req->iopoll_completed,1 ); - }else if (!(issue_flags &IO_URING_F_UNLOCKED )) { + }else if (issue_flags &IO_URING_F_COMPLETE_DEFER ) { +if (WARN_ON_ONCE (issue_flags &IO_URING_F_UNLOCKED )) +return ;io_req_complete_defer (req); }else { req->io_task_work.func = io_req_task_complete;'git-bisect' shows the reported warning starts from this patch.
That does make sense, as probably:
+ /* locked task_work executor checks the deffered list completion */
+ if (ts->locked)
+ issue_flags = IO_URING_F_COMPLETE_DEFER;
this assumption isn't true, and that would mess with the task management (which is in your oops).
Signature · 1 line
Jens Axboe
Re: [PATCH v2 02/14] io_uring/cmd: fix tw <-> issue_flags conversion
Quoted 6 lines · Jens Axboe
Quoted 5 lines · Ming Lei
Quoted 4 lines · Pavel Begunkov
!IO_URING_F_UNLOCKED does not translate to availability of the deferred completion infra,
IO_URING_F_COMPLETE_DEFERdoes, that what we should pass and look for to useio_req_complete_defer()and other variants.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Luckily, it's not a real problem as two wrongs actually made it right, at least as far as
io_uring_cmd_work()goes.
oops, I should've removed all the signed-offs
Quoted 19 lines
Quoted 19 lines
Quoted 19 lines
--- io_uring/uring_cmd.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)io_uring/uring_cmd.c +5
-1@@ -56,7 +56,11 @@ EXPORT_SYMBOL_GPL(io_uring_cmd_mark_cancelable);static void io_uring_cmd_work (struct io_kiocb *req,struct io_tw_state *ts) { struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd); -unsigned issue_flags = ts->locked ?0 :IO_URING_F_UNLOCKED ; +unsigned issue_flags =IO_URING_F_UNLOCKED ; + +/* locked task_work executor checks the deffered list completion */ +if (ts->locked) + issue_flags =IO_URING_F_COMPLETE_DEFER ;
Quoted 15 lines
Quoted 7 lines
Quoted 7 lines
io_uring/uring_cmd.c +0
-1ioucmd->task_work_cb (ioucmd, issue_flags); }@@ -100,7 +104,9 @@ void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2,if (req->ctx->flags &IORING_SETUP_IOPOLL ) {/* order with io_iopoll_req_issued() checking ->iopoll_complete */ smp_store_release (&req->iopoll_completed,1 ); - }else if (!(issue_flags &IO_URING_F_UNLOCKED )) {Quoted 8 lines
io_uring/uring_cmd.c void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2, · line 112
Quoted 6 lines
io_uring/uring_cmd.c +3
-0+ }else if (issue_flags &IO_URING_F_COMPLETE_DEFER ) { +if (WARN_ON_ONCE (issue_flags &IO_URING_F_UNLOCKED )) +return ;io_req_complete_defer (req); }else { req->io_task_work.func = io_req_task_complete;'git-bisect' shows the reported warning starts from this patch.
Thanks Ming
Quoted 9 lines
That does make sense, as probably:
io_uring/uring_cmd.c +3
-0+/* locked task_work executor checks the deffered list completion */ +if (ts->locked) + issue_flags =IO_URING_F_COMPLETE_DEFER ;this assumption isn't true, and that would mess with the task management (which is in your oops).
I'm missing it, how it's not true?
static void ctx_flush_and_put(struct io_ring_ctx *ctx, struct io_tw_state *ts)
{
...
if (ts->locked) {
io_submit_flush_completions(ctx);
...
}
}static __cold void io_fallback_req_func(struct work_struct *work)
{
...
mutex_lock(&ctx->uring_lock);
llist_for_each_entry_safe(req, tmp, node, io_task_work.node)
req->io_task_work.func(req, &ts);
io_submit_flush_completions(ctx);
mutex_unlock(&ctx->uring_lock);
...
}Signature · 1 line
Pavel Begunkov
Jens Axboe
maintainerRe: [PATCH v2 02/14] io_uring/cmd: fix tw <-> issue_flags conversion
Quoted 75 lines · Pavel Begunkov
Quoted 6 lines · Jens Axboe
Quoted 5 lines · Ming Lei
Quoted 4 lines · Pavel Begunkov
!IO_URING_F_UNLOCKED does not translate to availability of the deferred completion infra,
IO_URING_F_COMPLETE_DEFERdoes, that what we should pass and look for to useio_req_complete_defer()and other variants.Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Luckily, it's not a real problem as two wrongs actually made it right, at least as far as
io_uring_cmd_work()goes.oops, I should've removed all the signed-offs
Quoted 25 lines
Quoted 25 lines
Quoted 25 lines
--- io_uring/uring_cmd.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)io_uring/uring_cmd.c +5
-2@@ -56,7 +56,11 @@ EXPORT_SYMBOL_GPL(io_uring_cmd_mark_cancelable);static void io_uring_cmd_work (struct io_kiocb *req,struct io_tw_state *ts) { struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd); -unsigned issue_flags = ts->locked ?0 :IO_URING_F_UNLOCKED ; +unsigned issue_flags =IO_URING_F_UNLOCKED ; + +/* locked task_work executor checks the deffered list completion */ +if (ts->locked) + issue_flags =IO_URING_F_COMPLETE_DEFER ; ioucmd->task_work_cb (ioucmd, issue_flags); }@@ -100,7 +104,9 @@ void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2,if (req->ctx->flags &IORING_SETUP_IOPOLL ) {/* order with io_iopoll_req_issued() checking ->iopoll_complete */ smp_store_release (&req->iopoll_completed,1 ); - }else if (!(issue_flags &IO_URING_F_UNLOCKED )) {Quoted 8 lines
Quoted 8 lines
io_uring/uring_cmd.c void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2, · line 112
Quoted 6 lines
io_uring/uring_cmd.c +3
-0+ }else if (issue_flags &IO_URING_F_COMPLETE_DEFER ) { +if (WARN_ON_ONCE (issue_flags &IO_URING_F_UNLOCKED )) +return ;io_req_complete_defer (req); }else { req->io_task_work.func = io_req_task_complete;'git-bisect' shows the reported warning starts from this patch.
Thanks Ming
Quoted 9 lines
That does make sense, as probably:
io_uring/uring_cmd.c +3
-0+/* locked task_work executor checks the deffered list completion */ +if (ts->locked) + issue_flags =IO_URING_F_COMPLETE_DEFER ;this assumption isn't true, and that would mess with the task management (which is in your oops).
I'm missing it, how it's not true?
static void
ctx_flush_and_put(struct io_ring_ctx *ctx, struct io_tw_state *ts){ ... if (ts->locked) { io_submit_flush_completions(ctx); ... } }static
__coldvoidio_fallback_req_func(struct work_struct *work)
Quoted 9 lines
{
...
mutex_lock(&ctx->uring_lock);
llist_for_each_entry_safe(req, tmp, node, io_task_work.node)
req->io_task_work.func(req, &ts);
io_submit_flush_completions(ctx);
mutex_unlock(&ctx->uring_lock);
...
}I took a look too, and don't immediately see it. Those are also the two only cases I found, and before the patches, looks fine too.
So no immediate answer there... But I can confirm that before this patch, test passes fine. With the patch, it goes boom pretty quick. Either directly off putting the task, or an unrelated memory crash instead.
Signature · 1 line
Jens Axboe
Re: [PATCH v2 02/14] io_uring/cmd: fix tw <-> issue_flags conversion
Quoted 86 lines · Jens Axboe
Quoted 75 lines · Pavel Begunkov
Quoted 6 lines · Jens Axboe
Quoted 5 lines · Ming Lei
Quoted 4 lines · Pavel Begunkov
!IO_URING_F_UNLOCKED does not translate to availability of the deferred completion infra,
IO_URING_F_COMPLETE_DEFERdoes, that what we should pass and look for to useio_req_complete_defer()and other variants.Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Luckily, it's not a real problem as two wrongs actually made it right, at least as far as
io_uring_cmd_work()goes.oops, I should've removed all the signed-offs
Quoted 25 lines
Quoted 25 lines
Quoted 25 lines
--- io_uring/uring_cmd.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)io_uring/uring_cmd.c +5
-2@@ -56,7 +56,11 @@ EXPORT_SYMBOL_GPL(io_uring_cmd_mark_cancelable);static void io_uring_cmd_work (struct io_kiocb *req,struct io_tw_state *ts) { struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd); -unsigned issue_flags = ts->locked ?0 :IO_URING_F_UNLOCKED ; +unsigned issue_flags =IO_URING_F_UNLOCKED ; + +/* locked task_work executor checks the deffered list completion */ +if (ts->locked) + issue_flags =IO_URING_F_COMPLETE_DEFER ; ioucmd->task_work_cb (ioucmd, issue_flags); }@@ -100,7 +104,9 @@ void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2,if (req->ctx->flags &IORING_SETUP_IOPOLL ) {/* order with io_iopoll_req_issued() checking ->iopoll_complete */ smp_store_release (&req->iopoll_completed,1 ); - }else if (!(issue_flags &IO_URING_F_UNLOCKED )) {Quoted 8 lines
Quoted 8 lines
io_uring/uring_cmd.c void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2, · line 112
Quoted 6 lines
io_uring/uring_cmd.c +3
-0+ }else if (issue_flags &IO_URING_F_COMPLETE_DEFER ) { +if (WARN_ON_ONCE (issue_flags &IO_URING_F_UNLOCKED )) +return ;io_req_complete_defer (req); }else { req->io_task_work.func = io_req_task_complete;'git-bisect' shows the reported warning starts from this patch.
Thanks Ming
Quoted 9 lines
That does make sense, as probably:
io_uring/uring_cmd.c +3
-0+/* locked task_work executor checks the deffered list completion */ +if (ts->locked) + issue_flags =IO_URING_F_COMPLETE_DEFER ;this assumption isn't true, and that would mess with the task management (which is in your oops).
I'm missing it, how it's not true?
static void
ctx_flush_and_put(struct io_ring_ctx *ctx, struct io_tw_state *ts){ ... if (ts->locked) { io_submit_flush_completions(ctx); ... } }static
__coldvoidio_fallback_req_func(struct work_struct *work)Quoted 9 lines
{ ... mutex_lock(&ctx->uring_lock); llist_for_each_entry_safe(req, tmp, node, io_task_work.node) req->io_task_work.func(req, &ts); io_submit_flush_completions(ctx); mutex_unlock(&ctx->uring_lock); ... }
Quoted 5 lines
I took a look too, and don't immediately see it. Those are also the two only cases I found, and before the patches, looks fine too.
So no immediate answer there... But I can confirm that before this patch, test passes fine. With the patch, it goes boom pretty quick.
Which test is that? ublk generic/004?
Quoted 2 lines
Either directly off putting the task, or an unrelated memory crash instead.
If tw locks it should be checking for deferred completions, that's the rule. Maybe there is some rouge conversion and locked came not from task work executor... I'll need to stare more closely
Signature · 1 line
Pavel Begunkov
Jens Axboe
maintainerRe: [PATCH v2 02/14] io_uring/cmd: fix tw <-> issue_flags conversion
Quoted 65 lines · Pavel Begunkov
Quoted 65 lines
Quoted 64 lines
Quoted 21 lines
Quoted 21 lines
Quoted 21 lines
io_uring/uring_cmd.c +5
-2@@ -56,7 +56,11 @@ EXPORT_SYMBOL_GPL(io_uring_cmd_mark_cancelable);static void io_uring_cmd_work (struct io_kiocb *req,struct io_tw_state *ts) { struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd); -unsigned issue_flags = ts->locked ?0 :IO_URING_F_UNLOCKED ; +unsigned issue_flags =IO_URING_F_UNLOCKED ; + +/* locked task_work executor checks the deffered list completion */ +if (ts->locked) + issue_flags =IO_URING_F_COMPLETE_DEFER ; ioucmd->task_work_cb (ioucmd, issue_flags); }@@ -100,7 +104,9 @@ void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2,if (req->ctx->flags &IORING_SETUP_IOPOLL ) {/* order with io_iopoll_req_issued() checking ->iopoll_complete */ smp_store_release (&req->iopoll_completed,1 ); - }else if (!(issue_flags &IO_URING_F_UNLOCKED )) {Quoted 8 lines
Quoted 8 lines
io_uring/uring_cmd.c void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2, · line 112
Quoted 6 lines
io_uring/uring_cmd.c +3
-0+ }else if (issue_flags &IO_URING_F_COMPLETE_DEFER ) { +if (WARN_ON_ONCE (issue_flags &IO_URING_F_UNLOCKED )) +return ;io_req_complete_defer (req); }else { req->io_task_work.func = io_req_task_complete;'git-bisect' shows the reported warning starts from this patch.
Thanks Ming
Quoted 9 lines
That does make sense, as probably:
io_uring/uring_cmd.c +3
-0+/* locked task_work executor checks the deffered list completion */ +if (ts->locked) + issue_flags =IO_URING_F_COMPLETE_DEFER ;this assumption isn't true, and that would mess with the task management (which is in your oops).
I'm missing it, how it's not true?
static void
ctx_flush_and_put(struct io_ring_ctx *ctx, struct io_tw_state *ts){ ... if (ts->locked) { io_submit_flush_completions(ctx); ... } }static
__coldvoidio_fallback_req_func(struct work_struct *work){ ... mutex_lock(&ctx->uring_lock); llist_for_each_entry_safe(req, tmp, node, io_task_work.node) req->io_task_work.func(req, &ts); io_submit_flush_completions(ctx); mutex_unlock(&ctx->uring_lock); ... }
Quoted 7 lines
Quoted 5 lines
I took a look too, and don't immediately see it. Those are also the two only cases I found, and before the patches, looks fine too.
So no immediate answer there... But I can confirm that before this patch, test passes fine. With the patch, it goes boom pretty quick.
Which test is that? ublk generic/004?
Yep, it won't make it through one run of:
sudo make test T=generic/004
with it.
Quoted 7 lines
Quoted 2 lines
Either directly off putting the task, or an unrelated memory crash instead.
If tw locks it should be checking for deferred completions, that's the rule. Maybe there is some rouge conversion and locked came not from task work executor... I'll need to stare more closely
Yeah not sure what it is, but I hope you can reproduce with the above.
Signature · 1 line
Jens Axboe
Re: [PATCH v2 02/14] io_uring/cmd: fix tw <-> issue_flags conversion
In reply to Jens AxboeQuoted 86 lines · Jens Axboe
Quoted 75 lines · Pavel Begunkov
Quoted 6 lines · Jens Axboe
Quoted 5 lines · Ming Lei
Quoted 4 lines · Pavel Begunkov
!IO_URING_F_UNLOCKED does not translate to availability of the deferred completion infra,
IO_URING_F_COMPLETE_DEFERdoes, that what we should pass and look for to useio_req_complete_defer()and other variants.Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Luckily, it's not a real problem as two wrongs actually made it right, at least as far as
io_uring_cmd_work()goes.oops, I should've removed all the signed-offs
Quoted 25 lines
Quoted 25 lines
Quoted 25 lines
--- io_uring/uring_cmd.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)io_uring/uring_cmd.c +5
-2@@ -56,7 +56,11 @@ EXPORT_SYMBOL_GPL(io_uring_cmd_mark_cancelable);static void io_uring_cmd_work (struct io_kiocb *req,struct io_tw_state *ts) { struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd); -unsigned issue_flags = ts->locked ?0 :IO_URING_F_UNLOCKED ; +unsigned issue_flags =IO_URING_F_UNLOCKED ; + +/* locked task_work executor checks the deffered list completion */ +if (ts->locked) + issue_flags =IO_URING_F_COMPLETE_DEFER ; ioucmd->task_work_cb (ioucmd, issue_flags); }@@ -100,7 +104,9 @@ void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2,if (req->ctx->flags &IORING_SETUP_IOPOLL ) {/* order with io_iopoll_req_issued() checking ->iopoll_complete */ smp_store_release (&req->iopoll_completed,1 ); - }else if (!(issue_flags &IO_URING_F_UNLOCKED )) {Quoted 8 lines
Quoted 8 lines
io_uring/uring_cmd.c void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2, · line 112
Quoted 6 lines
io_uring/uring_cmd.c +3
-0+ }else if (issue_flags &IO_URING_F_COMPLETE_DEFER ) { +if (WARN_ON_ONCE (issue_flags &IO_URING_F_UNLOCKED )) +return ;io_req_complete_defer (req); }else { req->io_task_work.func = io_req_task_complete;'git-bisect' shows the reported warning starts from this patch.
Thanks Ming
Quoted 9 lines
That does make sense, as probably:
io_uring/uring_cmd.c +3
-0+/* locked task_work executor checks the deffered list completion */ +if (ts->locked) + issue_flags =IO_URING_F_COMPLETE_DEFER ;this assumption isn't true, and that would mess with the task management (which is in your oops).
I'm missing it, how it's not true?
static void
ctx_flush_and_put(struct io_ring_ctx *ctx, struct io_tw_state *ts){ ... if (ts->locked) { io_submit_flush_completions(ctx); ... } }static
__coldvoidio_fallback_req_func(struct work_struct *work)Quoted 9 lines
{ ... mutex_lock(&ctx->uring_lock); llist_for_each_entry_safe(req, tmp, node, io_task_work.node) req->io_task_work.func(req, &ts); io_submit_flush_completions(ctx); mutex_unlock(&ctx->uring_lock); ... }
Quoted 7 lines
I took a look too, and don't immediately see it. Those are also the two only cases I found, and before the patches, looks fine too.
So no immediate answer there... But I can confirm that before this patch, test passes fine. With the patch, it goes boom pretty quick. Either directly off putting the task, or an unrelated memory crash instead.
In ublk, the translated 'issue_flags' is passed to io_uring_cmd_done() from ioucmd->task_work_cb()(__ublk_rq_task_work()). That might be related with the reason.
Thanks,
Ming
Jens Axboe
maintainerRe: [PATCH v2 02/14] io_uring/cmd: fix tw <-> issue_flags conversion
Quoted 90 lines · Ming Lei
Quoted 89 lines · Jens Axboe
Quoted 75 lines · Pavel Begunkov
Quoted 6 lines · Jens Axboe
Quoted 5 lines · Ming Lei
Quoted 4 lines · Pavel Begunkov
!IO_URING_F_UNLOCKED does not translate to availability of the deferred completion infra,
IO_URING_F_COMPLETE_DEFERdoes, that what we should pass and look for to useio_req_complete_defer()and other variants.Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Luckily, it's not a real problem as two wrongs actually made it right, at least as far as
io_uring_cmd_work()goes.oops, I should've removed all the signed-offs
Quoted 25 lines
Quoted 25 lines
Quoted 25 lines
--- io_uring/uring_cmd.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)io_uring/uring_cmd.c +5
-2@@ -56,7 +56,11 @@ EXPORT_SYMBOL_GPL(io_uring_cmd_mark_cancelable);static void io_uring_cmd_work (struct io_kiocb *req,struct io_tw_state *ts) { struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd); -unsigned issue_flags = ts->locked ?0 :IO_URING_F_UNLOCKED ; +unsigned issue_flags =IO_URING_F_UNLOCKED ; + +/* locked task_work executor checks the deffered list completion */ +if (ts->locked) + issue_flags =IO_URING_F_COMPLETE_DEFER ; ioucmd->task_work_cb (ioucmd, issue_flags); }@@ -100,7 +104,9 @@ void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2,if (req->ctx->flags &IORING_SETUP_IOPOLL ) {/* order with io_iopoll_req_issued() checking ->iopoll_complete */ smp_store_release (&req->iopoll_completed,1 ); - }else if (!(issue_flags &IO_URING_F_UNLOCKED )) {Quoted 8 lines
Quoted 8 lines
io_uring/uring_cmd.c void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2, · line 112
Quoted 6 lines
io_uring/uring_cmd.c +3
-0+ }else if (issue_flags &IO_URING_F_COMPLETE_DEFER ) { +if (WARN_ON_ONCE (issue_flags &IO_URING_F_UNLOCKED )) +return ;io_req_complete_defer (req); }else { req->io_task_work.func = io_req_task_complete;'git-bisect' shows the reported warning starts from this patch.
Thanks Ming
Quoted 9 lines
That does make sense, as probably:
(quoted diff) +3
-0+ /* locked task_work executor checks the deffered list completion */ + if (ts->locked) + issue_flags = IO_URING_F_COMPLETE_DEFER;this assumption isn't true, and that would mess with the task management (which is in your oops).
I'm missing it, how it's not true?
static void
ctx_flush_and_put(struct io_ring_ctx *ctx, struct io_tw_state *ts){ ... if (ts->locked) { io_submit_flush_completions(ctx); ... } }static
__coldvoidio_fallback_req_func(struct work_struct *work)Quoted 9 lines
{ ... mutex_lock(&ctx->uring_lock); llist_for_each_entry_safe(req, tmp, node, io_task_work.node) req->io_task_work.func(req, &ts); io_submit_flush_completions(ctx); mutex_unlock(&ctx->uring_lock); ... }I took a look too, and don't immediately see it. Those are also the two only cases I found, and before the patches, looks fine too.
Quoted 8 lines
Quoted 4 lines
So no immediate answer there... But I can confirm that before this patch, test passes fine. With the patch, it goes boom pretty quick. Either directly off putting the task, or an unrelated memory crash instead.
In ublk, the translated '
issue_flags' is passed toio_uring_cmd_done()from ioucmd->task_work_cb()(__ublk_rq_task_work()). That might be related with the reason.
Or maybe ublk is doing multiple invocations of task_work completions? I added this:
io_uring/io_uring.c +1 -0
@@ -739,6 +739,7 @@ static void io_put_task_remote(struct task_struct *task) {struct io_uring_task *tctx = task->io_uring; +WARN_ON_ONCE (!percpu_counter_read (&tctx->inflight));percpu_counter_sub (&tctx->inflight,1 );if (unlikely (atomic_read (&tctx->in_cancel)))wake_up (&tctx->wait);
and hit this:
[ 77.386845] ------------[ cut here ]------------ [ 77.387128] WARNING: CPU: 5 PID: 109 at io_uring/io_uring.c:742 io_put_task_remote+0x164/0x1a8 [ 77.387608] Modules linked in: [ 77.387784] CPU: 5 PID: 109 Comm: kworker/5:1 Not tainted 6.8.0-11436-g340741d86a53-dirty #5750 [ 77.388277] Hardware name: linux,dummy-virt (DT) [ 77.388601] Workqueue: events io_fallback_req_func [ 77.388930] pstate: 81400005 (Nzcv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--) [ 77.389402] pc : io_put_task_remote+0x164/0x1a8 [ 77.389711] lr : __io_submit_flush_completions+0x8b8/0x1308 [ 77.390087] sp : ffff800087327a60 [ 77.390317] x29: ffff800087327a60 x28: 1fffe0002040b329 x27: 1fffe0002040b32f [ 77.390817] x26: ffff000103c4e900 x25: ffff000102059900 x24: ffff000104670000 [ 77.391314] x23: ffff0000d2195000 x22: 00000000002ce20c x21: ffff0000ced4fcc8 [ 77.391787] x20: ffff0000ced4fc00 x19: ffff000103c4e900 x18: 0000000000000000 [ 77.392209] x17: ffff8000814b0c34 x16: ffff8000814affac x15: ffff8000814ac4a8 [ 77.392633] x14: ffff80008069327c x13: ffff800080018c9c x12: ffff600020789d26 [ 77.393055] x11: 1fffe00020789d25 x10: ffff600020789d25 x9 : dfff800000000000 [ 77.393479] x8 : 00009fffdf8762db x7 : ffff000103c4e92b x6 : 0000000000000001 [ 77.393904] x5 : ffff000103c4e928 x4 : ffff600020789d26 x3 : 1fffe0001a432a7a [ 77.394334] x2 : 1fffe00019da9f9a x1 : 0000000000000000 x0 : 0000000000000000 [ 77.394761] Call trace: [ 77.394913] io_put_task_remote+0x164/0x1a8 [ 77.395168] __io_submit_flush_completions+0x8b8/0x1308 [ 77.395481] io_fallback_req_func+0x138/0x1e8 [ 77.395742] process_one_work+0x538/0x1048 [ 77.395992] worker_thread+0x760/0xbd4 [ 77.396221] kthread+0x2dc/0x368 [ 77.396417] ret_from_fork+0x10/0x20 [ 77.396634] ---[ end trace 0000000000000000 ]--- [ 77.397706] ------------[ cut here ]------------
which is showing either an imbalance in the task references, or multiple completions from the same io_uring request.
Anyway, I'll pop back in tomrrow, but hopefully the above is somewhat useful at least. I'd suspect the __ublk_rq_task_work() abort check for current != ubq->ubq_daemon and what happens off that.
Signature · 1 line
Jens Axboe
Re: [PATCH v2 02/14] io_uring/cmd: fix tw <-> issue_flags conversion
Quoted 162 lines · Jens Axboe
Quoted 90 lines · Ming Lei
Quoted 89 lines · Jens Axboe
Quoted 75 lines · Pavel Begunkov
Quoted 6 lines · Jens Axboe
Quoted 5 lines · Ming Lei
Quoted 4 lines · Pavel Begunkov
!IO_URING_F_UNLOCKED does not translate to availability of the deferred completion infra,
IO_URING_F_COMPLETE_DEFERdoes, that what we should pass and look for to useio_req_complete_defer()and other variants.Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Luckily, it's not a real problem as two wrongs actually made it right, at least as far as
io_uring_cmd_work()goes.oops, I should've removed all the signed-offs
Quoted 25 lines
Quoted 25 lines
Quoted 25 lines
--- io_uring/uring_cmd.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)io_uring/uring_cmd.c +5
-2@@ -56,7 +56,11 @@ EXPORT_SYMBOL_GPL(io_uring_cmd_mark_cancelable);static void io_uring_cmd_work (struct io_kiocb *req,struct io_tw_state *ts) { struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd); -unsigned issue_flags = ts->locked ?0 :IO_URING_F_UNLOCKED ; +unsigned issue_flags =IO_URING_F_UNLOCKED ; + +/* locked task_work executor checks the deffered list completion */ +if (ts->locked) + issue_flags =IO_URING_F_COMPLETE_DEFER ; ioucmd->task_work_cb (ioucmd, issue_flags); }@@ -100,7 +104,9 @@ void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2,if (req->ctx->flags &IORING_SETUP_IOPOLL ) {/* order with io_iopoll_req_issued() checking ->iopoll_complete */ smp_store_release (&req->iopoll_completed,1 ); - }else if (!(issue_flags &IO_URING_F_UNLOCKED )) {Quoted 8 lines
Quoted 8 lines
io_uring/uring_cmd.c void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2, · line 112
Quoted 6 lines
io_uring/uring_cmd.c +3
-0+ }else if (issue_flags &IO_URING_F_COMPLETE_DEFER ) { +if (WARN_ON_ONCE (issue_flags &IO_URING_F_UNLOCKED )) +return ;io_req_complete_defer (req); }else { req->io_task_work.func = io_req_task_complete;'git-bisect' shows the reported warning starts from this patch.
Thanks Ming
Quoted 9 lines
That does make sense, as probably:
(quoted diff) +3
-0+ /* locked task_work executor checks the deffered list completion */ + if (ts->locked) + issue_flags = IO_URING_F_COMPLETE_DEFER;this assumption isn't true, and that would mess with the task management (which is in your oops).
I'm missing it, how it's not true?
static void
ctx_flush_and_put(struct io_ring_ctx *ctx, struct io_tw_state *ts){ ... if (ts->locked) { io_submit_flush_completions(ctx); ... } }static
__coldvoidio_fallback_req_func(struct work_struct *work)Quoted 9 lines
{ ... mutex_lock(&ctx->uring_lock); llist_for_each_entry_safe(req, tmp, node, io_task_work.node) req->io_task_work.func(req, &ts); io_submit_flush_completions(ctx); mutex_unlock(&ctx->uring_lock); ... }I took a look too, and don't immediately see it. Those are also the two only cases I found, and before the patches, looks fine too.
Quoted 8 lines
Quoted 4 lines
So no immediate answer there... But I can confirm that before this patch, test passes fine. With the patch, it goes boom pretty quick. Either directly off putting the task, or an unrelated memory crash instead.
In ublk, the translated '
issue_flags' is passed toio_uring_cmd_done()from ioucmd->task_work_cb()(__ublk_rq_task_work()). That might be related with the reason.Or maybe ublk is doing multiple invocations of
task_workcompletions? I added this:io_uring/io_uring.c +1
-0@@ -739,6 +739,7 @@ static void io_put_task_remote(struct task_struct *task) {struct io_uring_task *tctx = task->io_uring; +WARN_ON_ONCE (!percpu_counter_read (&tctx->inflight));percpu_counter_sub (&tctx->inflight,1 );if (unlikely (atomic_read (&tctx->in_cancel)))wake_up (&tctx->wait);and hit this:
[ 77.386845] ------------[ cut here ]------------ [ 77.387128] WARNING: CPU: 5 PID: 109 at io_uring/io_uring.c:742 io_put_task_remote+0x164/0x1a8 [ 77.387608] Modules linked in: [ 77.387784] CPU: 5 PID: 109 Comm: kworker/5:1 Not tainted 6.8.0-11436-g340741d86a53-dirty #5750 [ 77.388277] Hardware name: linux,dummy-virt (DT) [ 77.388601] Workqueue: events io_fallback_req_func [ 77.388930] pstate: 81400005 (Nzcv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--) [ 77.389402] pc : io_put_task_remote+0x164/0x1a8 [ 77.389711] lr : __io_submit_flush_completions+0x8b8/0x1308 [ 77.390087] sp : ffff800087327a60 [ 77.390317] x29: ffff800087327a60 x28: 1fffe0002040b329 x27: 1fffe0002040b32f [ 77.390817] x26: ffff000103c4e900 x25: ffff000102059900 x24: ffff000104670000 [ 77.391314] x23: ffff0000d2195000 x22: 00000000002ce20c x21: ffff0000ced4fcc8 [ 77.391787] x20: ffff0000ced4fc00 x19: ffff000103c4e900 x18: 0000000000000000 [ 77.392209] x17: ffff8000814b0c34 x16: ffff8000814affac x15: ffff8000814ac4a8 [ 77.392633] x14: ffff80008069327c x13: ffff800080018c9c x12: ffff600020789d26 [ 77.393055] x11: 1fffe00020789d25 x10: ffff600020789d25 x9 : dfff800000000000 [ 77.393479] x8 : 00009fffdf8762db x7 : ffff000103c4e92b x6 : 0000000000000001 [ 77.393904] x5 : ffff000103c4e928 x4 : ffff600020789d26 x3 : 1fffe0001a432a7a [ 77.394334] x2 : 1fffe00019da9f9a x1 : 0000000000000000 x0 : 0000000000000000 [ 77.394761] Call trace: [ 77.394913] io_put_task_remote+0x164/0x1a8 [ 77.395168] __io_submit_flush_completions+0x8b8/0x1308 [ 77.395481] io_fallback_req_func+0x138/0x1e8 [ 77.395742] process_one_work+0x538/0x1048 [ 77.395992] worker_thread+0x760/0xbd4 [ 77.396221] kthread+0x2dc/0x368 [ 77.396417] ret_from_fork+0x10/0x20 [ 77.396634] ---[ end trace 0000000000000000 ]--- [ 77.397706] ------------[ cut here ]------------
Quoted 6 lines
which is showing either an imbalance in the task references, or multiple completions from the same
io_uringrequest.Anyway, I'll pop back in tomrrow, but hopefully the above is somewhat useful at least. I'd suspect the
__ublk_rq_task_work()abort check for current !=ubq->ubq_daemonand what happens off that.
We can enable refcounting for all requests, then it should trigger on double free. i.e. adding io_req_set_refcount(req) somewhere in io_init_req().
Signature · 1 line
Pavel Begunkov
Re: [PATCH v2 02/14] io_uring/cmd: fix tw <-> issue_flags conversion
In reply to Jens AxboeQuoted 96 lines · Jens Axboe
Quoted 95 lines · Ming Lei
Quoted 93 lines · Jens Axboe
Quoted 75 lines · Pavel Begunkov
Quoted 6 lines · Jens Axboe
Quoted 5 lines · Ming Lei
Quoted 4 lines · Pavel Begunkov
!IO_URING_F_UNLOCKED does not translate to availability of the deferred completion infra,
IO_URING_F_COMPLETE_DEFERdoes, that what we should pass and look for to useio_req_complete_defer()and other variants.Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Luckily, it's not a real problem as two wrongs actually made it right, at least as far as
io_uring_cmd_work()goes.oops, I should've removed all the signed-offs
Quoted 25 lines
Quoted 25 lines
Quoted 25 lines
--- io_uring/uring_cmd.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)io_uring/uring_cmd.c +5
-2@@ -56,7 +56,11 @@ EXPORT_SYMBOL_GPL(io_uring_cmd_mark_cancelable);static void io_uring_cmd_work (struct io_kiocb *req,struct io_tw_state *ts) { struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd); -unsigned issue_flags = ts->locked ?0 :IO_URING_F_UNLOCKED ; +unsigned issue_flags =IO_URING_F_UNLOCKED ; + +/* locked task_work executor checks the deffered list completion */ +if (ts->locked) + issue_flags =IO_URING_F_COMPLETE_DEFER ; ioucmd->task_work_cb (ioucmd, issue_flags); }@@ -100,7 +104,9 @@ void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2,if (req->ctx->flags &IORING_SETUP_IOPOLL ) {/* order with io_iopoll_req_issued() checking ->iopoll_complete */ smp_store_release (&req->iopoll_completed,1 ); - }else if (!(issue_flags &IO_URING_F_UNLOCKED )) {Quoted 8 lines
Quoted 8 lines
io_uring/uring_cmd.c void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2, · line 112
Quoted 6 lines
io_uring/uring_cmd.c +3
-0+ }else if (issue_flags &IO_URING_F_COMPLETE_DEFER ) { +if (WARN_ON_ONCE (issue_flags &IO_URING_F_UNLOCKED )) +return ;io_req_complete_defer (req); }else { req->io_task_work.func = io_req_task_complete;'git-bisect' shows the reported warning starts from this patch.
Thanks Ming
Quoted 9 lines
That does make sense, as probably:
(quoted diff) +3
-0+ /* locked task_work executor checks the deffered list completion */ + if (ts->locked) + issue_flags = IO_URING_F_COMPLETE_DEFER;this assumption isn't true, and that would mess with the task management (which is in your oops).
I'm missing it, how it's not true?
static void
ctx_flush_and_put(struct io_ring_ctx *ctx, struct io_tw_state *ts){ ... if (ts->locked) { io_submit_flush_completions(ctx); ... } }static
__coldvoidio_fallback_req_func(struct work_struct *work)Quoted 9 lines
{ ... mutex_lock(&ctx->uring_lock); llist_for_each_entry_safe(req, tmp, node, io_task_work.node) req->io_task_work.func(req, &ts); io_submit_flush_completions(ctx); mutex_unlock(&ctx->uring_lock); ... }I took a look too, and don't immediately see it. Those are also the two only cases I found, and before the patches, looks fine too.
So no immediate answer there... But I can confirm that before this patch, test passes fine. With the patch, it goes boom pretty quick. Either directly off putting the task, or an unrelated memory crash instead.
Quoted 6 lines
Quoted 3 lines
In ublk, the translated '
issue_flags' is passed toio_uring_cmd_done()from ioucmd->task_work_cb()(__ublk_rq_task_work()). That might be related with the reason.Or maybe ublk is doing multiple invocations of
task_workcompletions? I added this:
Yes, your debug log & point does help.
This patch convert zero flag(!IO_URING_F_UNLOCKED) into IO_URING_F_COMPLETE_DEFER, and somewhere is easily ignored, and follows the fix, which need to be folded into patch 2.
io_uring/io_uring.c +2 -1
@@ -3259,7 +3259,8 @@ static bool io_uring_try_cancel_uring_cmd(struct io_ring_ctx *ctx,/* ->sqe isn't available if no async data */ if (!req_has_async_data (req)) cmd->sqe =NULL ; - file->f_op->uring_cmd (cmd,IO_URING_F_CANCEL ); + file->f_op->uring_cmd (cmd,IO_URING_F_CANCEL | +IO_URING_F_COMPLETE_DEFER ); ret = true; } }
Thanks,
Ming
Re: [PATCH v2 02/14] io_uring/cmd: fix tw <-> issue_flags conversion
Quoted 101 lines · Ming Lei
Quoted 100 lines · Jens Axboe
Quoted 98 lines · Ming Lei
Quoted 86 lines · Jens Axboe
Quoted 75 lines · Pavel Begunkov
Quoted 6 lines · Jens Axboe
Quoted 5 lines · Ming Lei
Quoted 4 lines · Pavel Begunkov
!IO_URING_F_UNLOCKED does not translate to availability of the deferred completion infra,
IO_URING_F_COMPLETE_DEFERdoes, that what we should pass and look for to useio_req_complete_defer()and other variants.Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Luckily, it's not a real problem as two wrongs actually made it right, at least as far as
io_uring_cmd_work()goes.oops, I should've removed all the signed-offs
Quoted 25 lines
Quoted 25 lines
Quoted 25 lines
--- io_uring/uring_cmd.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)io_uring/uring_cmd.c +5
-2@@ -56,7 +56,11 @@ EXPORT_SYMBOL_GPL(io_uring_cmd_mark_cancelable);static void io_uring_cmd_work (struct io_kiocb *req,struct io_tw_state *ts) { struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd); -unsigned issue_flags = ts->locked ?0 :IO_URING_F_UNLOCKED ; +unsigned issue_flags =IO_URING_F_UNLOCKED ; + +/* locked task_work executor checks the deffered list completion */ +if (ts->locked) + issue_flags =IO_URING_F_COMPLETE_DEFER ; ioucmd->task_work_cb (ioucmd, issue_flags); }@@ -100,7 +104,9 @@ void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2,if (req->ctx->flags &IORING_SETUP_IOPOLL ) {/* order with io_iopoll_req_issued() checking ->iopoll_complete */ smp_store_release (&req->iopoll_completed,1 ); - }else if (!(issue_flags &IO_URING_F_UNLOCKED )) {Quoted 8 lines
Quoted 8 lines
io_uring/uring_cmd.c void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2, · line 112
Quoted 6 lines
io_uring/uring_cmd.c +3
-0+ }else if (issue_flags &IO_URING_F_COMPLETE_DEFER ) { +if (WARN_ON_ONCE (issue_flags &IO_URING_F_UNLOCKED )) +return ;io_req_complete_defer (req); }else { req->io_task_work.func = io_req_task_complete;'git-bisect' shows the reported warning starts from this patch.
Thanks Ming
Quoted 9 lines
That does make sense, as probably:
(quoted diff) +3
-0+ /* locked task_work executor checks the deffered list completion */ + if (ts->locked) + issue_flags = IO_URING_F_COMPLETE_DEFER;this assumption isn't true, and that would mess with the task management (which is in your oops).
I'm missing it, how it's not true?
static void
ctx_flush_and_put(struct io_ring_ctx *ctx, struct io_tw_state *ts){ ... if (ts->locked) { io_submit_flush_completions(ctx); ... } }static
__coldvoidio_fallback_req_func(struct work_struct *work)Quoted 9 lines
{ ... mutex_lock(&ctx->uring_lock); llist_for_each_entry_safe(req, tmp, node, io_task_work.node) req->io_task_work.func(req, &ts); io_submit_flush_completions(ctx); mutex_unlock(&ctx->uring_lock); ... }Quoted 7 lines
I took a look too, and don't immediately see it. Those are also the two only cases I found, and before the patches, looks fine too.
So no immediate answer there... But I can confirm that before this patch, test passes fine. With the patch, it goes boom pretty quick. Either directly off putting the task, or an unrelated memory crash instead.
In ublk, the translated '
issue_flags' is passed toio_uring_cmd_done()from ioucmd->task_work_cb()(__ublk_rq_task_work()). That might be related with the reason.
Quoted 8 lines
Quoted 2 lines
Or maybe ublk is doing multiple invocations of
task_workcompletions? I added this:Yes, your debug log & point does help.
This patch convert zero
flag(!IO_URING_F_UNLOCKED)intoIO_URING_F_COMPLETE_DEFER, and somewhere is easily ignored, and follows the fix, which need to be folded into patch 2.
Thanks, was totally unaware of this chunk. A side note, it's better to be moved to uring_cmd, i'll do the change
Quoted 14 lines
io_uring/io_uring.c +2
-1@@ -3259,7 +3259,8 @@ static bool io_uring_try_cancel_uring_cmd(struct io_ring_ctx *ctx,/* ->sqe isn't available if no async data */ if (!req_has_async_data (req)) cmd->sqe =NULL ; - file->f_op->uring_cmd (cmd,IO_URING_F_CANCEL ); + file->f_op->uring_cmd (cmd,IO_URING_F_CANCEL | +IO_URING_F_COMPLETE_DEFER ); ret = true; } }
Signature · 1 line
Pavel Begunkov
PATCH v2 3/14 io_uring/cmd: make io_uring_cmd_done irq safe
In reply to Pavel Begunkovio_uring_cmd_done() is called from the irq context and is considered to be irq safe, however that's not the case if the driver requires cancellations because io_uring_cmd_del_cancelable() could try to take the uring_lock mutex.
Clean up the confusion, by deferring cancellation handling to locked task_work if we came into io_uring_cmd_done() from iowq or other IO_URING_F_UNLOCKED path.
--- io_uring/uring_cmd.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)
io_uring/uring_cmd.c +17 -7
@@ -14,19 +14,18 @@#include "rsrc.h" #include "uring_cmd.h" -static void io_uring_cmd_del_cancelable (struct io_uring_cmd *cmd, -unsigned int issue_flags) +static void io_uring_cmd_del_cancelable (struct io_uring_cmd *cmd) {struct io_kiocb *req =cmd_to_io_kiocb (cmd);struct io_ring_ctx *ctx = req->ctx; +lockdep_assert_held (&ctx->uring_lock); +if (!(cmd->flags &IORING_URING_CMD_CANCELABLE ))return ; cmd->flags &= ~IORING_URING_CMD_CANCELABLE ; -io_ring_submit_lock (ctx, issue_flags);hlist_del (&req->hash_node); -io_ring_submit_unlock (ctx, issue_flags); }/*
@@ -44,6 +43,9 @@ void io_uring_cmd_mark_cancelable(struct io_uring_cmd *cmd,struct io_kiocb *req =cmd_to_io_kiocb (cmd);struct io_ring_ctx *ctx = req->ctx; +if (WARN_ON_ONCE (ctx->flags &IORING_SETUP_IOPOLL )) +return ; +if (!(cmd->flags &IORING_URING_CMD_CANCELABLE )) { cmd->flags |=IORING_URING_CMD_CANCELABLE ;io_ring_submit_lock (ctx, issue_flags);
@@ -84,6 +86,15 @@ static inline void io_req_set_cqe32_extra(struct io_kiocb *req, req->big_cqe.extra2 = extra2; } +static void io_req_cmd_task_complete (struct io_kiocb *req,struct io_tw_state *ts) +{ + struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd); + +io_tw_lock (req->ctx, ts); +io_uring_cmd_del_cancelable (ioucmd); +io_req_task_complete (req, ts); +} +/* * Called by consumers of io_uring_cmd, if they originally returned * -EIOCBQUEUED upon receiving the command.
@@ -93,8 +104,6 @@ void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2, {struct io_kiocb *req =cmd_to_io_kiocb (ioucmd); -io_uring_cmd_del_cancelable (ioucmd, issue_flags); -if (ret <0 )req_set_fail (req);
@@ -107,9 +116,10 @@ void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, ssize_t res2, }else if (issue_flags &IO_URING_F_COMPLETE_DEFER ) {if (WARN_ON_ONCE (issue_flags &IO_URING_F_UNLOCKED ))return ; +io_uring_cmd_del_cancelable (ioucmd);io_req_complete_defer (req); }else { - req->io_task_work.func = io_req_task_complete; + req->io_task_work.func = io_req_cmd_task_complete;io_req_task_work_add (req); } }
Signature · 1 line
2.44.0
Re: [PATCH v2 03/14] io_uring/cmd: make io_uring_cmd_done irq safe
Quoted 42 lines · Pavel Begunkov
io_uring_cmd_done()is called from the irq context and is considered to be irq safe, however that's not the case if the driver requires cancellations becauseio_uring_cmd_del_cancelable()could try to take theuring_lockmutex.Clean up the confusion, by deferring cancellation handling to locked
task_workif we came intoio_uring_cmd_done()from iowq or otherIO_URING_F_UNLOCKEDpath.Signed-off-by Pavel Begunkov <[email protected]>--- io_uring/uring_cmd.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)io_uring/uring_cmd.c +3
-4@@ -14,19 +14,18 @@#include "rsrc.h" #include "uring_cmd.h" -static void io_uring_cmd_del_cancelable (struct io_uring_cmd *cmd, -unsigned int issue_flags) +static void io_uring_cmd_del_cancelable (struct io_uring_cmd *cmd) {struct io_kiocb *req =cmd_to_io_kiocb (cmd);struct io_ring_ctx *ctx = req->ctx; +lockdep_assert_held (&ctx->uring_lock); +if (!(cmd->flags &IORING_URING_CMD_CANCELABLE ))return ; cmd->flags &= ~IORING_URING_CMD_CANCELABLE ; -io_ring_submit_lock (ctx, issue_flags);hlist_del (&req->hash_node); -io_ring_submit_unlock (ctx, issue_flags); }/*
io_uring/uring_cmd.c void io_uring_cmd_mark_cancelable(struct io_uring_cmd *cmd, · line 48
Quoted 6 lines
io_uring/uring_cmd.c +3
-0@@ -44,6 +43,9 @@ void io_uring_cmd_mark_cancelable(struct io_uring_cmd *cmd,struct io_kiocb *req =cmd_to_io_kiocb (cmd);struct io_ring_ctx *ctx = req->ctx; +if (WARN_ON_ONCE (ctx->flags &IORING_SETUP_IOPOLL )) +return ; +
This way limits cancelable command can't be used in iopoll context, and it isn't reasonable, and supporting iopoll has been in ublk todo list, especially single ring context is shared for both command and normal IO.
Thanks,
Ming
Re: [PATCH v2 03/14] io_uring/cmd: make io_uring_cmd_done irq safe
Quoted 46 lines · Ming Lei
Quoted 45 lines · Pavel Begunkov
io_uring_cmd_done()is called from the irq context and is considered to be irq safe, however that's not the case if the driver requires cancellations becauseio_uring_cmd_del_cancelable()could try to take theuring_lockmutex.Clean up the confusion, by deferring cancellation handling to locked
task_workif we came intoio_uring_cmd_done()from iowq or otherIO_URING_F_UNLOCKEDpath.Signed-off-by Pavel Begunkov <[email protected]>--- io_uring/uring_cmd.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)io_uring/uring_cmd.c +3
-4@@ -14,19 +14,18 @@ #include "rsrc.h"on#include "uring_cmd.h" -static void io_uring_cmd_del_cancelable (struct io_uring_cmd *cmd, -unsigned int issue_flags) +static void io_uring_cmd_del_cancelable (struct io_uring_cmd *cmd) {struct io_kiocb *req =cmd_to_io_kiocb (cmd);struct io_ring_ctx *ctx = req->ctx; +lockdep_assert_held (&ctx->uring_lock); +if (!(cmd->flags &IORING_URING_CMD_CANCELABLE ))return ; cmd->flags &= ~IORING_URING_CMD_CANCELABLE ; -io_ring_submit_lock (ctx, issue_flags);hlist_del (&req->hash_node); -io_ring_submit_unlock (ctx, issue_flags); }/* @@ -44,6 +43,9 @@ void io_uring_cmd_mark_cancelable(struct io_uring_cmd *cmd,struct io_kiocb *req =cmd_to_io_kiocb (cmd);struct io_ring_ctx *ctx = req->ctx;
Quoted 7 lines
io_uring/uring_cmd.c void io_uring_cmd_mark_cancelable(struct io_uring_cmd *cmd, · line 48
Quoted 3 lines
io_uring/uring_cmd.c +3
-0+if (WARN_ON_ONCE (ctx->flags &IORING_SETUP_IOPOLL )) +return ; +This way limits cancelable command can't be used in iopoll context, and it isn't reasonable, and supporting iopoll has been in ublk todo list, especially single ring context is shared for both command and normal IO.
That's something that can be solved when it's needed, and hence the warning so it's not missed. That would need del_cancelable on the ->iopoll side, but depends on the "leaving in cancel queue" problem resolution.
Signature · 1 line
Pavel Begunkov
Re: [PATCH v2 03/14] io_uring/cmd: make io_uring_cmd_done irq safe
Quoted 49 lines · Pavel Begunkov
Quoted 39 lines · Ming Lei
Quoted 38 lines · Pavel Begunkov
io_uring_cmd_done()is called from the irq context and is considered to be irq safe, however that's not the case if the driver requires cancellations becauseio_uring_cmd_del_cancelable()could try to take theuring_lockmutex.Clean up the confusion, by deferring cancellation handling to locked
task_workif we came intoio_uring_cmd_done()from iowq or otherIO_URING_F_UNLOCKEDpath.Signed-off-by Pavel Begunkov <[email protected]>--- io_uring/uring_cmd.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)io_uring/uring_cmd.c +3
-4@@ -14,19 +14,18 @@ #include "rsrc.h"on #include "uring_cmd.h" -static void io_uring_cmd_del_cancelable (struct io_uring_cmd *cmd, -unsigned int issue_flags) +static void io_uring_cmd_del_cancelable (struct io_uring_cmd *cmd) {struct io_kiocb *req =cmd_to_io_kiocb (cmd);struct io_ring_ctx *ctx = req->ctx; +lockdep_assert_held (&ctx->uring_lock); +if (!(cmd->flags &IORING_URING_CMD_CANCELABLE ))return ; cmd->flags &= ~IORING_URING_CMD_CANCELABLE ; -io_ring_submit_lock (ctx, issue_flags);hlist_del (&req->hash_node); -io_ring_submit_unlock (ctx, issue_flags); }/* @@ -44,6 +43,9 @@ void io_uring_cmd_mark_cancelable(struct io_uring_cmd *cmd,struct io_kiocb *req =cmd_to_io_kiocb (cmd);Quoted 8 lines
io_uring/uring_cmd.c void io_uring_cmd_mark_cancelable(struct io_uring_cmd *cmd, · line 48
Quoted 4 lines
io_uring/uring_cmd.c +3
-0struct io_ring_ctx *ctx = req->ctx; +if (WARN_ON_ONCE (ctx->flags &IORING_SETUP_IOPOLL )) +return ; +This way limits cancelable command can't be used in iopoll context, and it isn't reasonable, and supporting iopoll has been in ublk todo list, especially single ring context is shared for both command and normal IO.
Quoted 4 lines
That's something that can be solved when it's needed, and hence the warning so it's not missed. That would need
del_cancelableon the ->iopoll side, but depends on the "leaving in cancel queue" problem resolution.
The current code is actually working with iopoll, so adding the warning can be one regression. Maybe someone has been using ublk with iopoll.
Thanks,
Ming
Re: [PATCH v2 03/14] io_uring/cmd: make io_uring_cmd_done irq safe
Quoted 50 lines · Ming Lei
Quoted 49 lines · Pavel Begunkov
Quoted 47 lines · Ming Lei
Quoted 37 lines · Pavel Begunkov
io_uring_cmd_done()is called from the irq context and is considered to be irq safe, however that's not the case if the driver requires cancellations becauseio_uring_cmd_del_cancelable()could try to take theuring_lockmutex.Clean up the confusion, by deferring cancellation handling to locked
task_workif we came intoio_uring_cmd_done()from iowq or otherIO_URING_F_UNLOCKEDpath.Signed-off-by Pavel Begunkov <[email protected]>--- io_uring/uring_cmd.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)io_uring/uring_cmd.c +3
-4@@ -14,19 +14,18 @@ #include "rsrc.h"on #include "uring_cmd.h" -static void io_uring_cmd_del_cancelable (struct io_uring_cmd *cmd, -unsigned int issue_flags) +static void io_uring_cmd_del_cancelable (struct io_uring_cmd *cmd) {struct io_kiocb *req =cmd_to_io_kiocb (cmd);struct io_ring_ctx *ctx = req->ctx; +lockdep_assert_held (&ctx->uring_lock); +if (!(cmd->flags &IORING_URING_CMD_CANCELABLE ))return ; cmd->flags &= ~IORING_URING_CMD_CANCELABLE ; -io_ring_submit_lock (ctx, issue_flags);hlist_del (&req->hash_node); -io_ring_submit_unlock (ctx, issue_flags); }/* io_uring/uring_cmd.c void io_uring_cmd_mark_cancelable(struct io_uring_cmd *cmd, · line 47
Quoted 5 lines
io_uring/uring_cmd.c +3
-0@@ -44,6 +43,9 @@ void io_uring_cmd_mark_cancelable(struct io_uring_cmd *cmd,struct io_kiocb *req =cmd_to_io_kiocb (cmd);struct io_ring_ctx *ctx = req->ctx; +if (WARN_ON_ONCE (ctx->flags &IORING_SETUP_IOPOLL )) +return ; +This way limits cancelable command can't be used in iopoll context, and it isn't reasonable, and supporting iopoll has been in ublk todo list, especially single ring context is shared for both command and normal IO.
Quoted 7 lines
Quoted 4 lines
That's something that can be solved when it's needed, and hence the warning so it's not missed. That would need
del_cancelableon the ->iopoll side, but depends on the "leaving in cancel queue" problem resolution.The current code is actually working with iopoll, so adding the warning can be one regression. Maybe someone has been using ublk with iopoll.
Hmm, I don't see ->uring_cmd_iopoll implemented anywhere, and w/o it io_uring should fail the request:
int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
{
...
if (ctx->flags & IORING_SETUP_IOPOLL) {
if (!file->f_op->uring_cmd_iopoll)
return -EOPNOTSUPP;
issue_flags |= IO_URING_F_IOPOLL;
}
}Did I miss it somewhere?
Signature · 1 line
Pavel Begunkov
Re: [PATCH v2 03/14] io_uring/cmd: make io_uring_cmd_done irq safe
Quoted 72 lines · Pavel Begunkov
Quoted 50 lines · Ming Lei
Quoted 49 lines · Pavel Begunkov
Quoted 47 lines · Ming Lei
Quoted 37 lines · Pavel Begunkov
io_uring_cmd_done()is called from the irq context and is considered to be irq safe, however that's not the case if the driver requires cancellations becauseio_uring_cmd_del_cancelable()could try to take theuring_lockmutex.Clean up the confusion, by deferring cancellation handling to locked
task_workif we came intoio_uring_cmd_done()from iowq or otherIO_URING_F_UNLOCKEDpath.Signed-off-by Pavel Begunkov <[email protected]>--- io_uring/uring_cmd.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)io_uring/uring_cmd.c +3
-4@@ -14,19 +14,18 @@ #include "rsrc.h"on #include "uring_cmd.h" -static void io_uring_cmd_del_cancelable (struct io_uring_cmd *cmd, -unsigned int issue_flags) +static void io_uring_cmd_del_cancelable (struct io_uring_cmd *cmd) {struct io_kiocb *req =cmd_to_io_kiocb (cmd);struct io_ring_ctx *ctx = req->ctx; +lockdep_assert_held (&ctx->uring_lock); +if (!(cmd->flags &IORING_URING_CMD_CANCELABLE ))return ; cmd->flags &= ~IORING_URING_CMD_CANCELABLE ; -io_ring_submit_lock (ctx, issue_flags);hlist_del (&req->hash_node); -io_ring_submit_unlock (ctx, issue_flags); }/* io_uring/uring_cmd.c void io_uring_cmd_mark_cancelable(struct io_uring_cmd *cmd, · line 47
Quoted 5 lines
io_uring/uring_cmd.c +3
-0@@ -44,6 +43,9 @@ void io_uring_cmd_mark_cancelable(struct io_uring_cmd *cmd,struct io_kiocb *req =cmd_to_io_kiocb (cmd);struct io_ring_ctx *ctx = req->ctx; +if (WARN_ON_ONCE (ctx->flags &IORING_SETUP_IOPOLL )) +return ; +This way limits cancelable command can't be used in iopoll context, and it isn't reasonable, and supporting iopoll has been in ublk todo list, especially single ring context is shared for both command and normal IO.
Quoted 7 lines
Quoted 4 lines
That's something that can be solved when it's needed, and hence the warning so it's not missed. That would need
del_cancelableon the ->iopoll side, but depends on the "leaving in cancel queue" problem resolution.The current code is actually working with iopoll, so adding the warning can be one regression. Maybe someone has been using ublk with iopoll.
Hmm, I don't see ->uring_cmd_iopoll implemented anywhere, and w/o it
io_uringshould fail the request:int
io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags){ ... if (ctx->flags & IORING_SETUP_IOPOLL) { if (!file->f_op->uring_cmd_iopoll) return -EOPNOTSUPP; issue_flags |= IO_URING_F_IOPOLL; } }
Quoted 1 line
Did I miss it somewhere?
Looks I missed the point, now the WARN() is fine.
Thanks,
Ming
PATCH v2 4/14 io_uring/cmd: introduce io_uring_cmd_complete
In reply to Pavel Begunkovio_uring_cmd_complete() does exactly what io_uring_cmd_done() does, that is completing the request, but doesn't ask for issue_flags argument. We have a couple of users hardcoding some random issue_flags values in drivers, which they absolutely should not do. This function will be used to get rid of them. Also, add comments warning users that they're only allowed to pass issue_flags that were given from io_uring.
--- include/linux/io_uring/cmd.h | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
include/linux/io_uring/cmd.h +24 -0
@@ -26,12 +26,25 @@ static inline const void *io_uring_sqe_cmd(const struct io_uring_sqe *sqe)#if defined(CONFIG_IO_URING )int io_uring_cmd_import_fixed (u64 ubuf,unsigned long len,int rw,struct iov_iter *iter,void *ioucmd); + +/* +* Completes the request, i.e. posts an io_uring CQE and deallocates @ioucmd +* and a corresponding io_uring request. +* +* Note: the caller must never invent the @issue_flags mask, it's only allowed +* to pass what has been provided by the core io_uring code. +*/ void io_uring_cmd_done (struct io_uring_cmd *cmd,ssize_t ret,ssize_t res2,unsigned issue_flags); +void __io_uring_cmd_do_in_task (struct io_uring_cmd *ioucmd,void (*task_work_cb)(struct io_uring_cmd *,unsigned ),unsigned flags); +/* +* The caller must never invent the @issue_flags mask, it's only allowed +* to pass what has been provided by the core io_uring code. +*/ void io_uring_cmd_mark_cancelable (struct io_uring_cmd *cmd,unsigned int issue_flags);
@@ -56,6 +69,17 @@ static inline void io_uring_cmd_mark_cancelable(struct io_uring_cmd *cmd, }#endif +/* +* Completes the request, i.e. posts an io_uring CQE and deallocates @ioucmd +* and a corresponding io_uring request. Similar to io_uring_cmd_done() but +* without issue_flags argument. +*/ +static inline void io_uring_cmd_complete (struct io_uring_cmd *ioucmd, +ssize_t ret,ssize_t res2) +{ +io_uring_cmd_done (ioucmd, ret, res2,IO_URING_F_UNLOCKED ); +} +/* users must follow the IOU_F_TWQ_LAZY_WAKE semantics */ static inline void io_uring_cmd_do_in_task_lazy (struct io_uring_cmd *ioucmd,void (*task_work_cb)(struct io_uring_cmd *,unsigned ))
Signature · 1 line
2.44.0
PATCH v2 5/14 ublk: don't hard code IO_URING_F_UNLOCKED
In reply to Pavel Begunkovuring_cmd implementations should not try to guess issue_flags, just use a newly added io_uring_cmd_complete(). We're loosing an optimisation in the cancellation path in ublk_uring_cmd_cancel_fn(), but the assumption is that we don't care that much about it.
--- drivers/block/ublk_drv.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)
drivers/block/ublk_drv.c +8 -10
@@ -1417,8 +1417,7 @@ static bool ublk_abort_requests(struct ublk_device *ub, struct ublk_queue *ubq)return true; } -static void ublk_cancel_cmd (struct ublk_queue *ubq,struct ublk_io *io, -unsigned int issue_flags) +static void ublk_cancel_cmd (struct ublk_queue *ubq,struct ublk_io *io) {bool done;
@@ -1432,15 +1431,14 @@ static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io,spin_unlock (&ubq->cancel_lock);if (!done) -io_uring_cmd_done (io->cmd,UBLK_IO_RES_ABORT ,0 , issue_flags); +io_uring_cmd_complete (io->cmd,UBLK_IO_RES_ABORT ,0 ); }/* * The ublk char device won't be closed when calling cancel fn, so both * ublk device and queue are guaranteed to be live */ -static void ublk_uring_cmd_cancel_fn (struct io_uring_cmd *cmd, -unsigned int issue_flags) +static void ublk_uring_cmd_cancel_fn (struct io_uring_cmd *cmd) {struct ublk_uring_cmd_pdu *pdu =ublk_get_uring_cmd_pdu (cmd);struct ublk_queue *ubq = pdu->ubq;
@@ -1464,7 +1462,7 @@ static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd, io = &ubq->ios[pdu->tag];WARN_ON_ONCE (io->cmd != cmd); -ublk_cancel_cmd (ubq, io, issue_flags); +ublk_cancel_cmd (ubq, io);if (need_schedule) {if (ublk_can_use_recovery (ub))
@@ -1484,7 +1482,7 @@ static void ublk_cancel_queue(struct ublk_queue *ubq)int i;for (i =0 ; i < ubq->q_depth; i++) -ublk_cancel_cmd (ubq, &ubq->ios[i],IO_URING_F_UNLOCKED ); +ublk_cancel_cmd (ubq, &ubq->ios[i]); }/* Cancel all pending commands, must be called after del_gendisk() returns */
@@ -1777,7 +1775,7 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,return -EIOCBQUEUED ; out: -io_uring_cmd_done (cmd, ret,0 , issue_flags); +io_uring_cmd_complete (cmd, ret,0 );pr_devel ("%s: complete: cmd op %d, tag %d ret %x io_flags %x\n" , __func__, cmd_op, tag, ret, io->flags);return -EIOCBQUEUED ;
@@ -1842,7 +1840,7 @@ static void ublk_ch_uring_cmd_cb(struct io_uring_cmd *cmd,static int ublk_ch_uring_cmd (struct io_uring_cmd *cmd,unsigned int issue_flags) {if (unlikely (issue_flags &IO_URING_F_CANCEL )) { -ublk_uring_cmd_cancel_fn (cmd, issue_flags); +ublk_uring_cmd_cancel_fn (cmd);return 0 ; }
@@ -2930,7 +2928,7 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,if (ub)ublk_put_device (ub); out: -io_uring_cmd_done (cmd, ret,0 , issue_flags); +io_uring_cmd_complete (cmd, ret,0 );pr_devel ("%s: cmd done ret %d cmd_op %x, dev id %d qid %d\n" , __func__, ret, cmd->cmd_op, header->dev_id, header->queue_id);return -EIOCBQUEUED ;
Signature · 1 line
2.44.0
Re: [PATCH v2 05/14] ublk: don't hard code IO_URING_F_UNLOCKED
Quoted 45 lines · Pavel Begunkov
uring_cmdimplementations should not try to guessissue_flags, just use a newly addedio_uring_cmd_complete(). We're loosing an optimisation in the cancellation path inublk_uring_cmd_cancel_fn(), but the assumption is that we don't care that much about it.Signed-off-by Pavel Begunkov <[email protected]>Link https://lore.kernel.org/r/2f7bc9fbc98b11412d10b8fd88e58e35614e3147.1710514702.git.asml.silence@gmail.com open hereSigned-off-by Jens Axboe <[email protected]>--- drivers/block/ublk_drv.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)drivers/block/ublk_drv.c +3
-5@@ -1417,8 +1417,7 @@ static bool ublk_abort_requests(struct ublk_device *ub, struct ublk_queue *ubq)return true; } -static void ublk_cancel_cmd (struct ublk_queue *ubq,struct ublk_io *io, -unsigned int issue_flags) +static void ublk_cancel_cmd (struct ublk_queue *ubq,struct ublk_io *io) {bool done;@@ -1432,15 +1431,14 @@ static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io,spin_unlock (&ubq->cancel_lock);if (!done) -io_uring_cmd_done (io->cmd,UBLK_IO_RES_ABORT ,0 , issue_flags); +io_uring_cmd_complete (io->cmd,UBLK_IO_RES_ABORT ,0 ); }/* * The ublk char device won't be closed when calling cancel fn, so both * ublk device and queue are guaranteed to be live */ -static void ublk_uring_cmd_cancel_fn (struct io_uring_cmd *cmd, -unsigned int issue_flags) +static void ublk_uring_cmd_cancel_fn (struct io_uring_cmd *cmd) {struct ublk_uring_cmd_pdu *pdu =ublk_get_uring_cmd_pdu (cmd);struct ublk_queue *ubq = pdu->ubq;
drivers/block/ublk_drv.c static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd, · line 1465
Quoted 5 lines
drivers/block/ublk_drv.c +1
-1@@ -1464,7 +1462,7 @@ static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd, io = &ubq->ios[pdu->tag];WARN_ON_ONCE (io->cmd != cmd); -ublk_cancel_cmd (ubq, io, issue_flags); +ublk_cancel_cmd (ubq, io);
.cancel_fn is always called with .uring_lock held, so this 'issue_flags' can't be removed, otherwise double task run is caused because .cancel_fn can be called multiple times if the request stays in ctx->cancelable_uring_cmd.
Thanks,
Ming
Re: [PATCH v2 05/14] ublk: don't hard code IO_URING_F_UNLOCKED
Quoted 47 lines · Ming Lei
Quoted 46 lines · Pavel Begunkov
uring_cmdimplementations should not try to guessissue_flags, just use a newly addedio_uring_cmd_complete(). We're loosing an optimisation in the cancellation path inublk_uring_cmd_cancel_fn(), but the assumption is that we don't care that much about it.Signed-off-by Pavel Begunkov <[email protected]>Link https://lore.kernel.org/r/2f7bc9fbc98b11412d10b8fd88e58e35614e3147.1710514702.git.asml.silence@gmail.com open hereSigned-off-by Jens Axboe <[email protected]>--- drivers/block/ublk_drv.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)drivers/block/ublk_drv.c +3
-5@@ -1417,8 +1417,7 @@ static bool ublk_abort_requests(struct ublk_device *ub, struct ublk_queue *ubq)return true; } -static void ublk_cancel_cmd (struct ublk_queue *ubq,struct ublk_io *io, -unsigned int issue_flags) +static void ublk_cancel_cmd (struct ublk_queue *ubq,struct ublk_io *io) {bool done;@@ -1432,15 +1431,14 @@ static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io,spin_unlock (&ubq->cancel_lock);if (!done) -io_uring_cmd_done (io->cmd,UBLK_IO_RES_ABORT ,0 , issue_flags); +io_uring_cmd_complete (io->cmd,UBLK_IO_RES_ABORT ,0 ); }/* * The ublk char device won't be closed when calling cancel fn, so both * ublk device and queue are guaranteed to be live */ -static void ublk_uring_cmd_cancel_fn (struct io_uring_cmd *cmd, -unsigned int issue_flags) +static void ublk_uring_cmd_cancel_fn (struct io_uring_cmd *cmd) {struct ublk_uring_cmd_pdu *pdu =ublk_get_uring_cmd_pdu (cmd);struct ublk_queue *ubq = pdu->ubq;@@ -1464,7 +1462,7 @@ static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,
Quoted 8 lines
drivers/block/ublk_drv.c static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd, · line 1465
Quoted 4 lines
drivers/block/ublk_drv.c +1
-1io = &ubq->ios[pdu->tag];WARN_ON_ONCE (io->cmd != cmd); -ublk_cancel_cmd (ubq, io, issue_flags); +ublk_cancel_cmd (ubq, io);.cancel_fn is always called with .uring_lock held, so this '
issue_flags' can't be removed, otherwise double task run is caused because .cancel_fn can be called multiple times if the request stays inctx->cancelable_uring_cmd.
I see, that's exactly why I was asking whether it can be deferred to tw. Let me see if I can get by without that patch, but honestly it's a horrible abuse of the ring state. Any ideas how that can be cleaned up?
Signature · 1 line
Pavel Begunkov
Re: [PATCH v2 05/14] ublk: don't hard code IO_URING_F_UNLOCKED
Quoted 52 lines · Pavel Begunkov
Quoted 42 lines · Ming Lei
Quoted 41 lines · Pavel Begunkov
uring_cmdimplementations should not try to guessissue_flags, just use a newly addedio_uring_cmd_complete(). We're loosing an optimisation in the cancellation path inublk_uring_cmd_cancel_fn(), but the assumption is that we don't care that much about it.Signed-off-by Pavel Begunkov <[email protected]>Link https://lore.kernel.org/r/2f7bc9fbc98b11412d10b8fd88e58e35614e3147.1710514702.git.asml.silence@gmail.com open hereSigned-off-by Jens Axboe <[email protected]>---
drivers/block/ublk_drv.c| 18 ++++++++----------1 file changed, 8 insertions(+), 10 deletions(-)drivers/block/ublk_drv.c +3
-5@@ -1417,8 +1417,7 @@ static bool ublk_abort_requests(struct ublk_device *ub, struct ublk_queue *ubq) return true; } -static void ublk_cancel_cmd (struct ublk_queue *ubq,struct ublk_io *io, - unsigned int issue_flags) +static void ublk_cancel_cmd (struct ublk_queue *ubq,struct ublk_io *io) { bool done;@@ -1432,15 +1431,14 @@ static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io, spin_unlock(&ubq->cancel_lock); if (!done) - io_uring_cmd_done(io->cmd, UBLK_IO_RES_ABORT, 0, issue_flags); + io_uring_cmd_complete(io->cmd, UBLK_IO_RES_ABORT, 0); } /** The ublk char device won't be closed when calling cancel fn, so both * ublk device and queue are guaranteed to be live */ -static void ublk_uring_cmd_cancel_fn (struct io_uring_cmd *cmd, - unsigned int issue_flags) +static void ublk_uring_cmd_cancel_fn (struct io_uring_cmd *cmd) { struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd); struct ublk_queue *ubq = pdu->ubq;@@ -1464,7 +1462,7 @@ static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,Quoted 8 lines
drivers/block/ublk_drv.c static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd, · line 1465
Quoted 4 lines
drivers/block/ublk_drv.c +1
-1io = &ubq->ios[pdu->tag]; WARN_ON_ONCE(io->cmd != cmd); - ublk_cancel_cmd(ubq, io, issue_flags); + ublk_cancel_cmd(ubq, io);.cancel_fn is always called with .uring_lock held, so this '
issue_flags' can't be removed, otherwise double task run is caused because .cancel_fn can be called multiple times if the request stays inctx->cancelable_uring_cmd.
Quoted 4 lines
I see, that's exactly why I was asking whether it can be deferred to tw. Let me see if I can get by without that patch, but honestly it's a horrible abuse of the ring state. Any ideas how that can be cleaned up?
I assume io_uring_try_cancel_uring_cmd() can run in parallel with completions, so there can be two parallel calls calls to ->uring_cmd (e.g. io-wq + cancel), which gives me shivers. Also, I'd rather no cancel in place requests of another task, io_submit_flush_completions() but it complicates things.
Is there any argument against removing requests from the cancellation list in io_uring_try_cancel_uring_cmd() before calling ->uring_cmd?
io_uring_try_cancel_uring_cmd() {
lock();
for_each_req() {
remove_req_from_cancel_list(req);
req->file->uring_cmd();
}
unlock();
}Signature · 1 line
Pavel Begunkov
Re: [PATCH v2 05/14] ublk: don't hard code IO_URING_F_UNLOCKED
Quoted 58 lines · Pavel Begunkov
Quoted 52 lines · Pavel Begunkov
Quoted 42 lines · Ming Lei
Quoted 41 lines · Pavel Begunkov
uring_cmdimplementations should not try to guessissue_flags, just use a newly addedio_uring_cmd_complete(). We're loosing an optimisation in the cancellation path inublk_uring_cmd_cancel_fn(), but the assumption is that we don't care that much about it.Signed-off-by Pavel Begunkov <[email protected]>Link https://lore.kernel.org/r/2f7bc9fbc98b11412d10b8fd88e58e35614e3147.1710514702.git.asml.silence@gmail.com open hereSigned-off-by Jens Axboe <[email protected]>---
drivers/block/ublk_drv.c| 18 ++++++++----------1 file changed, 8 insertions(+), 10 deletions(-)drivers/block/ublk_drv.c +3
-5@@ -1417,8 +1417,7 @@ static bool ublk_abort_requests(struct ublk_device *ub, struct ublk_queue *ubq) return true; } -static void ublk_cancel_cmd (struct ublk_queue *ubq,struct ublk_io *io, - unsigned int issue_flags) +static void ublk_cancel_cmd (struct ublk_queue *ubq,struct ublk_io *io) { bool done;@@ -1432,15 +1431,14 @@ static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io, spin_unlock(&ubq->cancel_lock); if (!done) - io_uring_cmd_done(io->cmd, UBLK_IO_RES_ABORT, 0, issue_flags); + io_uring_cmd_complete(io->cmd, UBLK_IO_RES_ABORT, 0); } /** The ublk char device won't be closed when calling cancel fn, so both * ublk device and queue are guaranteed to be live */ -static void ublk_uring_cmd_cancel_fn (struct io_uring_cmd *cmd, - unsigned int issue_flags) +static void ublk_uring_cmd_cancel_fn (struct io_uring_cmd *cmd) { struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd); struct ublk_queue *ubq = pdu->ubq;@@ -1464,7 +1462,7 @@ static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,Quoted 8 lines
drivers/block/ublk_drv.c static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd, · line 1465
Quoted 4 lines
drivers/block/ublk_drv.c +1
-1io = &ubq->ios[pdu->tag]; WARN_ON_ONCE(io->cmd != cmd); - ublk_cancel_cmd(ubq, io, issue_flags); + ublk_cancel_cmd(ubq, io);.cancel_fn is always called with .uring_lock held, so this '
issue_flags' can't be removed, otherwise double task run is caused because .cancel_fn can be called multiple times if the request stays inctx->cancelable_uring_cmd.Quoted 4 lines
I see, that's exactly why I was asking whether it can be deferred to tw. Let me see if I can get by without that patch, but honestly it's a horrible abuse of the ring state. Any ideas how that can be cleaned up?
Quoted 5 lines
I assume
io_uring_try_cancel_uring_cmd()can run in parallel with completions, so there can be two parallel calls calls to ->uring_cmd (e.g. io-wq + cancel), which gives me shivers. Also, I'd rather no cancel in place requests of another task,io_submit_flush_completions()but it complicates things.
I'm wrong though on flush_completions, the task there cancels only its own requests
io_uring_try_cancel_uring_cmd() {
...
if (!cancel_all && req->task != task)
continue;
}Quoted 12 lines
Is there any argument against removing requests from the cancellation list in
io_uring_try_cancel_uring_cmd()before calling ->uring_cmd?
io_uring_try_cancel_uring_cmd(){
lock();
for_each_req() {
remove_req_from_cancel_list(req);
req->file->uring_cmd();
}
unlock();
}
Signature · 1 line
Pavel Begunkov
Re: [PATCH v2 05/14] ublk: don't hard code IO_URING_F_UNLOCKED
Quoted 65 lines · Pavel Begunkov
Quoted 58 lines · Pavel Begunkov
Quoted 52 lines · Pavel Begunkov
Quoted 42 lines · Ming Lei
Quoted 41 lines · Pavel Begunkov
uring_cmdimplementations should not try to guessissue_flags, just use a newly addedio_uring_cmd_complete(). We're loosing an optimisation in the cancellation path inublk_uring_cmd_cancel_fn(), but the assumption is that we don't care that much about it.Signed-off-by Pavel Begunkov <[email protected]>Link https://lore.kernel.org/r/2f7bc9fbc98b11412d10b8fd88e58e35614e3147.1710514702.git.asml.silence@gmail.com open hereSigned-off-by Jens Axboe <[email protected]>---
drivers/block/ublk_drv.c| 18 ++++++++----------1 file changed, 8 insertions(+), 10 deletions(-)drivers/block/ublk_drv.c +3
-5@@ -1417,8 +1417,7 @@ static bool ublk_abort_requests(struct ublk_device *ub, struct ublk_queue *ubq) return true; } -static void ublk_cancel_cmd (struct ublk_queue *ubq,struct ublk_io *io, - unsigned int issue_flags) +static void ublk_cancel_cmd (struct ublk_queue *ubq,struct ublk_io *io) { bool done;@@ -1432,15 +1431,14 @@ static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io, spin_unlock(&ubq->cancel_lock); if (!done) - io_uring_cmd_done(io->cmd, UBLK_IO_RES_ABORT, 0, issue_flags); + io_uring_cmd_complete(io->cmd, UBLK_IO_RES_ABORT, 0); } /** The ublk char device won't be closed when calling cancel fn, so both * ublk device and queue are guaranteed to be live */ -static void ublk_uring_cmd_cancel_fn (struct io_uring_cmd *cmd, - unsigned int issue_flags) +static void ublk_uring_cmd_cancel_fn (struct io_uring_cmd *cmd) { struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd); struct ublk_queue *ubq = pdu->ubq;@@ -1464,7 +1462,7 @@ static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,Quoted 8 lines
drivers/block/ublk_drv.c static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd, · line 1465
Quoted 4 lines
drivers/block/ublk_drv.c +1
-1io = &ubq->ios[pdu->tag]; WARN_ON_ONCE(io->cmd != cmd); - ublk_cancel_cmd(ubq, io, issue_flags); + ublk_cancel_cmd(ubq, io);.cancel_fn is always called with .uring_lock held, so this '
issue_flags' can't be removed, otherwise double task run is caused because .cancel_fn can be called multiple times if the request stays inctx->cancelable_uring_cmd.Quoted 4 lines
I see, that's exactly why I was asking whether it can be deferred to tw. Let me see if I can get by without that patch, but honestly it's a horrible abuse of the ring state. Any ideas how that can be cleaned up?
Quoted 5 lines
I assume
io_uring_try_cancel_uring_cmd()can run in parallel with completions, so there can be two parallel calls calls to ->uring_cmd (e.g. io-wq + cancel), which gives me shivers. Also, I'd rather no cancel in place requests of another task,io_submit_flush_completions()but it complicates things.
Quoted 20 lines
I'm wrong though on
flush_completions, the task there cancels only its own requestsio_uring_try_cancel_uring_cmd() { ... if (!cancel_all && req->task != task) continue; }Quoted 10 lines
Is there any argument against removing requests from the cancellation list in
io_uring_try_cancel_uring_cmd()before calling ->uring_cmd?
io_uring_try_cancel_uring_cmd(){
lock();
for_each_req() {
remove_req_from_cancel_list(req);
req->file->uring_cmd();
}
unlock();
Also the req may not be ready to cancel in ->uring_cmd(), so it should be allowed to retry in future if it isn't canceled this time.
Thanks,
Ming
Re: [PATCH v2 05/14] ublk: don't hard code IO_URING_F_UNLOCKED
In reply to Pavel BegunkovQuoted 52 lines · Pavel Begunkov
Quoted 42 lines · Ming Lei
Quoted 41 lines · Pavel Begunkov
uring_cmdimplementations should not try to guessissue_flags, just use a newly addedio_uring_cmd_complete(). We're loosing an optimisation in the cancellation path inublk_uring_cmd_cancel_fn(), but the assumption is that we don't care that much about it.Signed-off-by Pavel Begunkov <[email protected]>Link https://lore.kernel.org/r/2f7bc9fbc98b11412d10b8fd88e58e35614e3147.1710514702.git.asml.silence@gmail.com open hereSigned-off-by Jens Axboe <[email protected]>--- drivers/block/ublk_drv.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)drivers/block/ublk_drv.c +3
-5@@ -1417,8 +1417,7 @@ static bool ublk_abort_requests(struct ublk_device *ub, struct ublk_queue *ubq)return true; } -static void ublk_cancel_cmd (struct ublk_queue *ubq,struct ublk_io *io, -unsigned int issue_flags) +static void ublk_cancel_cmd (struct ublk_queue *ubq,struct ublk_io *io) {bool done;@@ -1432,15 +1431,14 @@ static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io,spin_unlock (&ubq->cancel_lock);if (!done) -io_uring_cmd_done (io->cmd,UBLK_IO_RES_ABORT ,0 , issue_flags); +io_uring_cmd_complete (io->cmd,UBLK_IO_RES_ABORT ,0 ); }/* * The ublk char device won't be closed when calling cancel fn, so both * ublk device and queue are guaranteed to be live */ -static void ublk_uring_cmd_cancel_fn (struct io_uring_cmd *cmd, -unsigned int issue_flags) +static void ublk_uring_cmd_cancel_fn (struct io_uring_cmd *cmd) {struct ublk_uring_cmd_pdu *pdu =ublk_get_uring_cmd_pdu (cmd);struct ublk_queue *ubq = pdu->ubq;@@ -1464,7 +1462,7 @@ static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,Quoted 8 lines
drivers/block/ublk_drv.c static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd, · line 1465
Quoted 4 lines
drivers/block/ublk_drv.c +1
-1io = &ubq->ios[pdu->tag];WARN_ON_ONCE (io->cmd != cmd); -ublk_cancel_cmd (ubq, io, issue_flags); +ublk_cancel_cmd (ubq, io);.cancel_fn is always called with .uring_lock held, so this '
issue_flags' can't be removed, otherwise double task run is caused because .cancel_fn can be called multiple times if the request stays inctx->cancelable_uring_cmd.
Quoted 4 lines
I see, that's exactly why I was asking whether it can be deferred to tw. Let me see if I can get by without that patch, but honestly it's a horrible abuse of the ring state. Any ideas how that can be cleaned up?
Simply deferring io_uring_cmd_done() in ublk_cancel_cmd() to tw still triggers warning in __put_task_struct(), so I'd suggest to add the patch until it is root-cause & fixed.
Thanks,
Ming
Re: [PATCH v2 05/14] ublk: don't hard code IO_URING_F_UNLOCKED
Quoted 53 lines · Ming Lei
Quoted 52 lines · Pavel Begunkov
Quoted 50 lines · Ming Lei
Quoted 41 lines · Pavel Begunkov
uring_cmdimplementations should not try to guessissue_flags, just use a newly addedio_uring_cmd_complete(). We're loosing an optimisation in the cancellation path inublk_uring_cmd_cancel_fn(), but the assumption is that we don't care that much about it.Signed-off-by Pavel Begunkov <[email protected]>Link https://lore.kernel.org/r/2f7bc9fbc98b11412d10b8fd88e58e35614e3147.1710514702.git.asml.silence@gmail.com open hereSigned-off-by Jens Axboe <[email protected]>--- drivers/block/ublk_drv.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)drivers/block/ublk_drv.c +3
-5@@ -1417,8 +1417,7 @@ static bool ublk_abort_requests(struct ublk_device *ub, struct ublk_queue *ubq)return true; } -static void ublk_cancel_cmd (struct ublk_queue *ubq,struct ublk_io *io, -unsigned int issue_flags) +static void ublk_cancel_cmd (struct ublk_queue *ubq,struct ublk_io *io) {bool done;@@ -1432,15 +1431,14 @@ static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io,spin_unlock (&ubq->cancel_lock);if (!done) -io_uring_cmd_done (io->cmd,UBLK_IO_RES_ABORT ,0 , issue_flags); +io_uring_cmd_complete (io->cmd,UBLK_IO_RES_ABORT ,0 ); }/* * The ublk char device won't be closed when calling cancel fn, so both * ublk device and queue are guaranteed to be live */ -static void ublk_uring_cmd_cancel_fn (struct io_uring_cmd *cmd, -unsigned int issue_flags) +static void ublk_uring_cmd_cancel_fn (struct io_uring_cmd *cmd) {struct ublk_uring_cmd_pdu *pdu =ublk_get_uring_cmd_pdu (cmd);struct ublk_queue *ubq = pdu->ubq;drivers/block/ublk_drv.c static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd, · line 1464
Quoted 4 lines
drivers/block/ublk_drv.c +1
-1@@ -1464,7 +1462,7 @@ static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd, io = &ubq->ios[pdu->tag];WARN_ON_ONCE (io->cmd != cmd); -ublk_cancel_cmd (ubq, io, issue_flags); +ublk_cancel_cmd (ubq, io);.cancel_fn is always called with .uring_lock held, so this '
issue_flags' can't be removed, otherwise double task run is caused because .cancel_fn can be called multiple times if the request stays inctx->cancelable_uring_cmd.
Quoted 8 lines
Quoted 4 lines
I see, that's exactly why I was asking whether it can be deferred to tw. Let me see if I can get by without that patch, but honestly it's a horrible abuse of the ring state. Any ideas how that can be cleaned up?
Simply deferring
io_uring_cmd_done()inublk_cancel_cmd()to tw still triggers warning in__put_task_struct(), so I'd suggest to add the patch until it is root-cause & fixed.
I mean drop the patch[es] changing how ublk passes issue_flags around, moving cancellation point and all related, and leave it to later really hoping we'll figure how to do it better.
Signature · 1 line
Pavel Begunkov
Re: [PATCH v2 05/14] ublk: don't hard code IO_URING_F_UNLOCKED
Quoted 59 lines · Pavel Begunkov
Quoted 58 lines · Ming Lei
Quoted 56 lines · Pavel Begunkov
Quoted 42 lines · Ming Lei
Quoted 41 lines · Pavel Begunkov
uring_cmdimplementations should not try to guessissue_flags, just use a newly addedio_uring_cmd_complete(). We're loosing an optimisation in the cancellation path inublk_uring_cmd_cancel_fn(), but the assumption is that we don't care that much about it.Signed-off-by Pavel Begunkov <[email protected]>Link https://lore.kernel.org/r/2f7bc9fbc98b11412d10b8fd88e58e35614e3147.1710514702.git.asml.silence@gmail.com open hereSigned-off-by Jens Axboe <[email protected]>--- drivers/block/ublk_drv.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)drivers/block/ublk_drv.c +3
-5@@ -1417,8 +1417,7 @@ static bool ublk_abort_requests(struct ublk_device *ub, struct ublk_queue *ubq)return true; } -static void ublk_cancel_cmd (struct ublk_queue *ubq,struct ublk_io *io, -unsigned int issue_flags) +static void ublk_cancel_cmd (struct ublk_queue *ubq,struct ublk_io *io) {bool done;@@ -1432,15 +1431,14 @@ static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io,spin_unlock (&ubq->cancel_lock);if (!done) -io_uring_cmd_done (io->cmd,UBLK_IO_RES_ABORT ,0 , issue_flags); +io_uring_cmd_complete (io->cmd,UBLK_IO_RES_ABORT ,0 ); }/* * The ublk char device won't be closed when calling cancel fn, so both * ublk device and queue are guaranteed to be live */ -static void ublk_uring_cmd_cancel_fn (struct io_uring_cmd *cmd, -unsigned int issue_flags) +static void ublk_uring_cmd_cancel_fn (struct io_uring_cmd *cmd) {struct ublk_uring_cmd_pdu *pdu =ublk_get_uring_cmd_pdu (cmd);struct ublk_queue *ubq = pdu->ubq;@@ -1464,7 +1462,7 @@ static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,Quoted 8 lines
drivers/block/ublk_drv.c static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd, · line 1465
Quoted 4 lines
drivers/block/ublk_drv.c +1
-1io = &ubq->ios[pdu->tag];WARN_ON_ONCE (io->cmd != cmd); -ublk_cancel_cmd (ubq, io, issue_flags); +ublk_cancel_cmd (ubq, io);.cancel_fn is always called with .uring_lock held, so this '
issue_flags' can't be removed, otherwise double task run is caused because .cancel_fn can be called multiple times if the request stays inctx->cancelable_uring_cmd.I see, that's exactly why I was asking whether it can be deferred to tw. Let me see if I can get by without that patch, but honestly it's a horrible abuse of the ring state. Any ideas how that can be cleaned up?
Quoted 7 lines
Quoted 3 lines
Simply deferring
io_uring_cmd_done()inublk_cancel_cmd()to tw still triggers warning in__put_task_struct(), so I'd suggest to add the patch until it is root-cause & fixed.I mean drop the patch[es] changing how ublk passes
issue_flagsaround, moving cancellation point and all related, and leave it to later really hoping we'll figure how to do it better.
Looks fine for me.
Thanks,
Ming
PATCH v2 6/14 nvme/io_uring: don't hard code IO_URING_F_UNLOCKED
In reply to Pavel Begunkovuring_cmd implementations should not try to guess issue_flags, use a freshly added helper io_uring_cmd_complete() instead.
--- drivers/nvme/host/ioctl.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/nvme/host/ioctl.c +6 -3
@@ -426,10 +426,13 @@ static enum rq_end_io_ret nvme_uring_cmd_end_io(struct request *req,* For iopoll, complete it directly. * Otherwise, move the completion to task work. */ -if (blk_rq_is_poll (req)) -nvme_uring_task_cb (ioucmd,IO_URING_F_UNLOCKED ); -else +if (blk_rq_is_poll (req)) { +if (pdu->bio) +blk_rq_unmap_user (pdu->bio); +io_uring_cmd_complete (ioucmd, pdu->status, pdu->result); + }else {io_uring_cmd_do_in_task_lazy (ioucmd, nvme_uring_task_cb); + }return RQ_END_IO_FREE ; }
Signature · 1 line
2.44.0
Re: [PATCH v2 06/14] nvme/io_uring: don't hard code IO_URING_F_UNLOCKED
Quoted 2 lines · Pavel Begunkov
uring_cmdimplementations should not try to guessissue_flags, use a freshly added helperio_uring_cmd_complete()instead.
NVMe interactions look/work fine.
Re: [PATCH v2 06/14] nvme/io_uring: don't hard code IO_URING_F_UNLOCKED
Quoted 7 lines · Kanchan Joshi
Quoted 2 lines · Pavel Begunkov
uring_cmdimplementations should not try to guessissue_flags, use a freshly added helperio_uring_cmd_complete()instead.NVMe interactions look/work fine.
Reviewed-by Kanchan Joshi <[email protected]>
Thanks, I'll add it
Signature · 1 line
Pavel Begunkov
PATCH v2 7/14 io_uring/rw: avoid punting to io-wq directly
In reply to Pavel Begunkovkiocb_done() should care to specifically redirecting requests to io-wq. Remove the hopping to tw to then queue an io-wq, return -EAGAIN and let the core code io_uring handle offloading.
--- io_uring/io_uring.c | 8 ++++---- io_uring/io_uring.h | 1 - io_uring/rw.c | 8 +------- 3 files changed, 5 insertions(+), 12 deletions(-)
io_uring/io_uring.c +4 -4
@@ -498,7 +498,7 @@ static void io_prep_async_link(struct io_kiocb *req) } } -void io_queue_iowq (struct io_kiocb *req,struct io_tw_state *ts_dont_use) +static void io_queue_iowq (struct io_kiocb *req) {struct io_kiocb *link =io_prep_linked_timeout (req);struct io_uring_task *tctx = req->task->io_uring;
@@ -1505,7 +1505,7 @@ void io_req_task_submit(struct io_kiocb *req, struct io_tw_state *ts)if (unlikely (req->task->flags &PF_EXITING ))io_req_defer_failed (req, -EFAULT );else if (req->flags &REQ_F_FORCE_ASYNC ) -io_queue_iowq (req, ts); +io_queue_iowq (req);else io_queue_sqe (req); }
@@ -2088,7 +2088,7 @@ static void io_queue_async(struct io_kiocb *req, int ret)break ;case IO_APOLL_ABORTED :io_kbuf_recycle (req,0 ); -io_queue_iowq (req,NULL ); +io_queue_iowq (req);break ;case IO_APOLL_OK :break ;
@@ -2135,7 +2135,7 @@ static void io_queue_sqe_fallback(struct io_kiocb *req)if (unlikely (req->ctx->drain_active))io_drain_req (req);else -io_queue_iowq (req,NULL ); +io_queue_iowq (req); } }
io_uring/io_uring.h +0 -1
@@ -79,7 +79,6 @@ struct file *io_file_get_fixed(struct io_kiocb *req, int fd,void __io_req_task_work_add (struct io_kiocb *req,unsigned flags);bool io_alloc_async_data (struct io_kiocb *req);void io_req_task_queue (struct io_kiocb *req); -void io_queue_iowq (struct io_kiocb *req,struct io_tw_state *ts_dont_use);void io_req_task_complete (struct io_kiocb *req,struct io_tw_state *ts);void io_req_task_queue_fail (struct io_kiocb *req,int ret);void io_req_task_submit (struct io_kiocb *req,struct io_tw_state *ts);
io_uring/rw.c +1 -7
@@ -187,12 +187,6 @@ static inline loff_t *io_kiocb_update_pos(struct io_kiocb *req)return NULL ; } -static void io_req_task_queue_reissue (struct io_kiocb *req) -{ - req->io_task_work.func = io_queue_iowq; -io_req_task_work_add (req); -} -#ifdef CONFIG_BLOCK static bool io_resubmit_prep (struct io_kiocb *req) {
@@ -405,7 +399,7 @@ static int kiocb_done(struct io_kiocb *req, ssize_t ret,if (req->flags &REQ_F_REISSUE ) { req->flags &= ~REQ_F_REISSUE ;if (io_resubmit_prep (req)) -io_req_task_queue_reissue (req); +return -EAGAIN ;else io_req_task_queue_fail (req, final_ret); }
Signature · 1 line
2.44.0
PATCH v2 8/14 io_uring: force tw ctx locking
In reply to Pavel BegunkovWe can run normal task_work without locking the ctx, however we try to lock anyway and most handlers prefer or require it locked. It might have been interesting to multi-submitter ring with high contention completing async read/write requests via task_work, however that will still need to go through io_req_complete_post() and potentially take the lock for rsrc node putting or some other case.
In other words, it's hard to care about it, so alawys force the locking. The case described would also because of various io_uring caches.
--- io_uring/io_uring.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-)
io_uring/io_uring.c +9 -12
@@ -1191,8 +1191,9 @@ struct llist_node *io_handle_tw_list(struct llist_node *node,if (req->ctx != ctx) {ctx_flush_and_put (ctx, &ts); ctx = req->ctx; -/* if not contended, grab and improve batching */ - ts.locked =mutex_trylock (&ctx->uring_lock); + + ts.locked = true; +mutex_lock (&ctx->uring_lock);percpu_ref_get (&ctx->refs); } INDIRECT_CALL_2(req->io_task_work.func,
@@ -1453,11 +1454,9 @@ static int __io_run_local_work(struct io_ring_ctx *ctx, struct io_tw_state *ts,if (io_run_local_work_continue (ctx, ret, min_events)) goto again; -if (ts->locked) { -io_submit_flush_completions (ctx); -if (io_run_local_work_continue (ctx, ret, min_events)) - goto again; - } +io_submit_flush_completions (ctx); +if (io_run_local_work_continue (ctx, ret, min_events)) + goto again;trace_io_uring_local_work_run (ctx, ret, loops);return ret;
@@ -1481,14 +1480,12 @@ static inline int io_run_local_work_locked(struct io_ring_ctx *ctx,static int io_run_local_work (struct io_ring_ctx *ctx,int min_events) { -struct io_tw_state ts = {}; +struct io_tw_state ts = { .locked = true };int ret; - ts.locked =mutex_trylock (&ctx->uring_lock); +mutex_lock (&ctx->uring_lock); ret =__io_run_local_work (ctx, &ts, min_events); -if (ts.locked) -mutex_unlock (&ctx->uring_lock); - +mutex_unlock (&ctx->uring_lock);return ret; }
Signature · 1 line
2.44.0
PATCH v2 9/14 io_uring: remove struct io_tw_state::locked
In reply to Pavel Begunkovctx is always locked for task_work now, so get rid of struct io_tw_state::locked. Note I'm stopping one step before removing io_tw_state altogether, which is not empty, because it still serves the purpose of indicating which function is a tw callback and forcing users not to invoke them carelessly out of a wrong context. The removal can always be done later.
--- include/linux/io_uring_types.h | 2 -- io_uring/io_uring.c | 31 ++++++++----------------------- io_uring/io_uring.h | 5 +---- io_uring/poll.c | 2 +- io_uring/rw.c | 6 ++---- io_uring/timeout.c | 8 ++------ io_uring/uring_cmd.c | 8 ++------ io_uring/waitid.c | 2 +- 8 files changed, 17 insertions(+), 47 deletions(-)
include/linux/io_uring_types.h +0 -2
@@ -439,8 +439,6 @@ struct io_ring_ctx { };struct io_tw_state { -/* ->uring_lock is taken, callbacks can use io_tw_lock to lock it */ -bool locked; };enum {
io_uring/io_uring.c +8 -23
@@ -252,14 +252,12 @@ static __cold void io_fallback_req_func(struct work_struct *work) fallback_work.work);struct llist_node *node =llist_del_all (&ctx->fallback_llist);struct io_kiocb *req, *tmp; -struct io_tw_state ts = { .locked = true, }; +struct io_tw_state ts = {};percpu_ref_get (&ctx->refs);mutex_lock (&ctx->uring_lock);llist_for_each_entry_safe (req, tmp, node, io_task_work.node) req->io_task_work.func (req, &ts); -if (WARN_ON_ONCE (!ts.locked)) -return ;io_submit_flush_completions (ctx);mutex_unlock (&ctx->uring_lock);percpu_ref_put (&ctx->refs);
@@ -1163,11 +1161,9 @@ static void ctx_flush_and_put(struct io_ring_ctx *ctx, struct io_tw_state *ts)return ;if (ctx->flags &IORING_SETUP_TASKRUN_FLAG )atomic_andnot (IORING_SQ_TASKRUN , &ctx->rings->sq_flags); -if (ts->locked) { -io_submit_flush_completions (ctx); -mutex_unlock (&ctx->uring_lock); - ts->locked = false; - } + +io_submit_flush_completions (ctx); +mutex_unlock (&ctx->uring_lock);percpu_ref_put (&ctx->refs); }
@@ -1191,8 +1187,6 @@ struct llist_node *io_handle_tw_list(struct llist_node *node,if (req->ctx != ctx) {ctx_flush_and_put (ctx, &ts); ctx = req->ctx; - - ts.locked = true;mutex_lock (&ctx->uring_lock);percpu_ref_get (&ctx->refs); }
@@ -1465,22 +1459,16 @@ static int __io_run_local_work(struct io_ring_ctx *ctx, struct io_tw_state *ts,static inline int io_run_local_work_locked (struct io_ring_ctx *ctx,int min_events) { -struct io_tw_state ts = { .locked = true, }; -int ret; +struct io_tw_state ts = {};if (llist_empty (&ctx->work_llist))return 0 ; - - ret =__io_run_local_work (ctx, &ts, min_events); -/* shouldn't happen! */ -if (WARN_ON_ONCE (!ts.locked)) -mutex_lock (&ctx->uring_lock); -return ret; +return __io_run_local_work (ctx, &ts, min_events); }static int io_run_local_work (struct io_ring_ctx *ctx,int min_events) { -struct io_tw_state ts = { .locked = true }; +struct io_tw_state ts = {};int ret;mutex_lock (&ctx->uring_lock);
@@ -1708,10 +1696,7 @@ static int io_iopoll_check(struct io_ring_ctx *ctx, long min)void io_req_task_complete (struct io_kiocb *req,struct io_tw_state *ts) { -if (ts->locked) -io_req_complete_defer (req); -else -io_req_complete_post (req,IO_URING_F_UNLOCKED ); +io_req_complete_defer (req); }/*
io_uring/io_uring.h +1 -4
@@ -344,10 +344,7 @@ static inline bool io_task_work_pending(struct io_ring_ctx *ctx)static inline void io_tw_lock (struct io_ring_ctx *ctx,struct io_tw_state *ts) { -if (!ts->locked) { -mutex_lock (&ctx->uring_lock); - ts->locked = true; - } +lockdep_assert_held (&ctx->uring_lock); }/*
io_uring/poll.c +1 -1
@@ -322,7 +322,7 @@ static int io_poll_check_events(struct io_kiocb *req, struct io_tw_state *ts)__poll_t mask =mangle_poll (req->cqe.res & req->apoll_events); -if (!io_fill_cqe_req_aux (req, ts->locked, mask, +if (!io_fill_cqe_req_aux (req, true, mask,IORING_CQE_F_MORE )) {io_req_set_res (req, mask,0 );return IOU_POLL_REMOVE_POLL_USE_RES ;
io_uring/rw.c +2 -4
@@ -305,11 +305,9 @@ void io_req_rw_complete(struct io_kiocb *req, struct io_tw_state *ts)io_req_io_end (req); -if (req->flags & (REQ_F_BUFFER_SELECTED |REQ_F_BUFFER_RING )) { -unsigned issue_flags = ts->locked ?0 :IO_URING_F_UNLOCKED ; +if (req->flags & (REQ_F_BUFFER_SELECTED |REQ_F_BUFFER_RING )) + req->cqe.flags |=io_put_kbuf (req,0 ); - req->cqe.flags |=io_put_kbuf (req, issue_flags); - }io_req_task_complete (req, ts); }
io_uring/timeout.c +2 -6
@@ -72,10 +72,7 @@ static void io_timeout_complete(struct io_kiocb *req, struct io_tw_state *ts)struct io_ring_ctx *ctx = req->ctx;if (!io_timeout_finish (timeout, data)) { -bool filled; - filled =io_fill_cqe_req_aux (req, ts->locked, -ETIME , -IORING_CQE_F_MORE ); -if (filled) { +if (io_fill_cqe_req_aux (req, true, -ETIME ,IORING_CQE_F_MORE )) {/* re-arm timer */ spin_lock_irq (&ctx->timeout_lock);list_add (&timeout->list, ctx->timeout_list.prev);
@@ -301,7 +298,6 @@ int io_timeout_cancel(struct io_ring_ctx *ctx, struct io_cancel_data *cd)static void io_req_task_link_timeout (struct io_kiocb *req,struct io_tw_state *ts) { -unsigned issue_flags = ts->locked ?0 :IO_URING_F_UNLOCKED ; struct io_timeout *timeout = io_kiocb_to_cmd(req, struct io_timeout);struct io_kiocb *prev = timeout->prev;int ret = -ENOENT ;
@@ -313,7 +309,7 @@ static void io_req_task_link_timeout(struct io_kiocb *req, struct io_tw_state *t .data = prev->cqe.user_data, }; - ret =io_try_cancel (req->task->io_uring, &cd, issue_flags); + ret =io_try_cancel (req->task->io_uring, &cd,0 ); }io_req_set_res (req, ret ?: -ETIME ,0 );io_req_task_complete (req, ts);
io_uring/uring_cmd.c +2 -6
@@ -58,13 +58,9 @@ EXPORT_SYMBOL_GPL(io_uring_cmd_mark_cancelable);static void io_uring_cmd_work (struct io_kiocb *req,struct io_tw_state *ts) { struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd); -unsigned issue_flags =IO_URING_F_UNLOCKED ; -/* locked task_work executor checks the deffered list completion */ -if (ts->locked) - issue_flags =IO_URING_F_COMPLETE_DEFER ; - - ioucmd->task_work_cb (ioucmd, issue_flags); +/* task_work executor checks the deffered list completion */ + ioucmd->task_work_cb (ioucmd,IO_URING_F_COMPLETE_DEFER ); } void __io_uring_cmd_do_in_task(struct io_uring_cmd *ioucmd,
io_uring/waitid.c +1 -1
@@ -118,7 +118,7 @@ static int io_waitid_finish(struct io_kiocb *req, int ret)static void io_waitid_complete (struct io_kiocb *req,int ret) { struct io_waitid *iw = io_kiocb_to_cmd(req, struct io_waitid); -struct io_tw_state ts = { .locked = true }; +struct io_tw_state ts = {};/* anyone completing better be holding a reference */ WARN_ON_ONCE (!(atomic_read (&iw->refs) &IO_WAITID_REF_MASK ));
Signature · 1 line
2.44.0
PATCH v2 10/14 io_uring: refactor io_fill_cqe_req_aux
In reply to Pavel BegunkovThe restriction on multishot execution context disallowing io-wq is driven by rules of io_fill_cqe_req_aux(), it should only be called in the master task context, either from the syscall path or in task_work. Since task_work now always takes the ctx lock implying IO_URING_F_COMPLETE_DEFER, we can just assume that the function is always called with its defer argument set to true.
Kill the argument. Also rename the function for more consistency as "fill" in CQE related functions was usually meant for raw interfaces only copying data into the CQ without any locking, waking the user and other accounting "post" functions take care of.
--- io_uring/io_uring.c | 16 +++------------- io_uring/io_uring.h | 2 +- io_uring/net.c | 6 ++---- io_uring/poll.c | 3 +-- io_uring/rw.c | 4 +--- io_uring/timeout.c | 2 +- 6 files changed, 9 insertions(+), 24 deletions(-)
io_uring/io_uring.c +3 -13
@@ -913,40 +913,30 @@ static void __io_flush_post_cqes(struct io_ring_ctx *ctx) state->cqes_count =0 ; } -static bool __io_post_aux_cqe (struct io_ring_ctx *ctx,u64 user_data,s32 res,u32 cflags, -bool allow_overflow) +bool io_post_aux_cqe (struct io_ring_ctx *ctx,u64 user_data,s32 res,u32 cflags) {bool filled;io_cq_lock (ctx); filled =io_fill_cqe_aux (ctx, user_data, res, cflags); -if (!filled && allow_overflow) +if (!filled) filled =io_cqring_event_overflow (ctx, user_data, res, cflags,0 ,0 );io_cq_unlock_post (ctx);return filled; } -bool io_post_aux_cqe (struct io_ring_ctx *ctx,u64 user_data,s32 res,u32 cflags) -{ -return __io_post_aux_cqe (ctx, user_data, res, cflags, true); -} -/* * A helper for multishot requests posting additional CQEs. * Should only be used from a task_work including IO_URING_F_MULTISHOT. */ -bool io_fill_cqe_req_aux (struct io_kiocb *req,bool defer,s32 res,u32 cflags) +bool io_req_post_cqe (struct io_kiocb *req,s32 res,u32 cflags) {struct io_ring_ctx *ctx = req->ctx;u64 user_data = req->cqe.user_data;struct io_uring_cqe *cqe;lockdep_assert (!io_wq_current_is_worker ()); - -if (!defer) -return __io_post_aux_cqe (ctx, user_data, res, cflags, false); -lockdep_assert_held (&ctx->uring_lock);if (ctx->submit_state.cqes_count ==ARRAY_SIZE (ctx->completion_cqes)) {
io_uring/io_uring.h +1 -1
@@ -67,7 +67,7 @@ int io_run_task_work_sig(struct io_ring_ctx *ctx);void io_req_defer_failed (struct io_kiocb *req,s32 res);void io_req_complete_post (struct io_kiocb *req,unsigned issue_flags);bool io_post_aux_cqe (struct io_ring_ctx *ctx,u64 user_data,s32 res,u32 cflags); -bool io_fill_cqe_req_aux (struct io_kiocb *req,bool defer,s32 res,u32 cflags); +bool io_req_post_cqe (struct io_kiocb *req,s32 res,u32 cflags);void __io_commit_cqring_flush (struct io_ring_ctx *ctx);struct page **io_pin_pages (unsigned long ubuf,unsigned long len,int *npages);
io_uring/net.c +2 -4
@@ -706,8 +706,7 @@ static inline bool io_recv_finish(struct io_kiocb *req, int *ret,* receive from this socket. */ if ((req->flags &REQ_F_APOLL_MULTISHOT ) && !mshot_finished && -io_fill_cqe_req_aux (req, issue_flags &IO_URING_F_COMPLETE_DEFER , - *ret, cflags |IORING_CQE_F_MORE )) { +io_req_post_cqe (req, *ret, cflags |IORING_CQE_F_MORE )) { struct io_sr_msg *sr = io_kiocb_to_cmd(req, struct io_sr_msg);int mshot_retry_ret =IOU_ISSUE_SKIP_COMPLETE ;
@@ -1428,8 +1427,7 @@ int io_accept(struct io_kiocb *req, unsigned int issue_flags)if (ret <0 )return ret; -if (io_fill_cqe_req_aux (req, issue_flags &IO_URING_F_COMPLETE_DEFER , - ret,IORING_CQE_F_MORE )) +if (io_req_post_cqe (req, ret,IORING_CQE_F_MORE )) goto retry;io_req_set_res (req, ret,0 );
io_uring/poll.c +1 -2
@@ -322,8 +322,7 @@ static int io_poll_check_events(struct io_kiocb *req, struct io_tw_state *ts)__poll_t mask =mangle_poll (req->cqe.res & req->apoll_events); -if (!io_fill_cqe_req_aux (req, true, mask, -IORING_CQE_F_MORE )) { +if (!io_req_post_cqe (req, mask,IORING_CQE_F_MORE )) {io_req_set_res (req, mask,0 );return IOU_POLL_REMOVE_POLL_USE_RES ; }
io_uring/rw.c +1 -3
@@ -955,9 +955,7 @@ int io_read_mshot(struct io_kiocb *req, unsigned int issue_flags) cflags =io_put_kbuf (req, issue_flags); rw->len =0 ;/* similarly to above, reset len to 0 */ -if (io_fill_cqe_req_aux (req, - issue_flags &IO_URING_F_COMPLETE_DEFER , - ret, cflags |IORING_CQE_F_MORE )) { +if (io_req_post_cqe (req, ret, cflags |IORING_CQE_F_MORE )) {if (issue_flags &IO_URING_F_MULTISHOT ) {/* * Force retry, as we might have more data to
io_uring/timeout.c +1 -1
@@ -72,7 +72,7 @@ static void io_timeout_complete(struct io_kiocb *req, struct io_tw_state *ts)struct io_ring_ctx *ctx = req->ctx;if (!io_timeout_finish (timeout, data)) { -if (io_fill_cqe_req_aux (req, true, -ETIME ,IORING_CQE_F_MORE )) { +if (io_req_post_cqe (req, -ETIME ,IORING_CQE_F_MORE )) {/* re-arm timer */ spin_lock_irq (&ctx->timeout_lock);list_add (&timeout->list, ctx->timeout_list.prev);
Signature · 1 line
2.44.0
PATCH v2 11/14 io_uring: get rid of intermediate aux cqe caches
In reply to Pavel Begunkovio_post_aux_cqe(), which is used for multishot requests, delays completions by putting CQEs into a temporary array for the purpose completion lock/flush batching.
DEFER_TASKRUN doesn't need any locking, so for it we can put completions directly into the CQ and defer post completion handling with a flag. That leaves !DEFER_TASKRUN, which is not that interesting / hot for multishot requests, so have conditional locking with deferred flush for them.
--- include/linux/io_uring_types.h | 3 +- io_uring/io_uring.c | 64 ++++++++-------------------------- 2 files changed, 15 insertions(+), 52 deletions(-)
include/linux/io_uring_types.h +1 -2
@@ -205,6 +205,7 @@ struct io_submit_state {bool plug_started;bool need_plug; +bool cq_flush;unsigned short submit_nr;unsigned int cqes_count;struct blk_plug plug;
@@ -342,8 +343,6 @@ struct io_ring_ctx {unsigned cq_last_tm_flush; }____cacheline_aligned_in_smp ; -struct io_uring_cqe completion_cqes[16 ]; -spinlock_t completion_lock;/* IRQ completion list, under ->completion_lock */
io_uring/io_uring.c +14 -50
104 lines not on this page · raw message
Signature · 1 line
2.44.0
PATCH v2 12/14 io_uring: remove current check from complete_post
In reply to Pavel Begunkovtask_work execution is now always locked, and we shouldn't get into io_req_complete_post() from them. That means that complete_post() is always called out of the original task context and we don't even need to check current.
--- io_uring/io_uring.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
io_uring/io_uring.c +1 -1
@@ -978,7 +978,7 @@ void io_req_complete_post(struct io_kiocb *req, unsigned issue_flags) {struct io_ring_ctx *ctx = req->ctx; -if (ctx->task_complete && ctx->submitter_task != current) { +if (ctx->task_complete) { req->io_task_work.func = io_req_task_complete;io_req_task_work_add (req); } else if (!(issue_flags & IO_URING_F_UNLOCKED) ||
Signature · 1 line
2.44.0
PATCH v2 13/14 io_uring: refactor io_req_complete_post()
In reply to Pavel BegunkovMake io_req_complete_post() to push all IORING_SETUP_IOPOLL requests to task_work, it's much cleaner and should normally happen. We couldn't do it before because there was a possibility of looping in
complete_post() -> tw -> complete_post() -> ...
Also, unexport the function and inline __io_req_complete_post().
--- io_uring/io_uring.c | 29 +++++++++++------------------ io_uring/io_uring.h | 1 - 2 files changed, 11 insertions(+), 19 deletions(-)
io_uring/io_uring.c +11 -18
@@ -926,11 +926,21 @@ bool io_req_post_cqe(struct io_kiocb *req, s32 res, u32 cflags)return posted; } -static void __io_req_complete_post (struct io_kiocb *req,unsigned issue_flags) +static void io_req_complete_post (struct io_kiocb *req,unsigned issue_flags) {struct io_ring_ctx *ctx = req->ctx;struct io_rsrc_node *rsrc_node =NULL ; +/* +* Handle special CQ sync cases via task_work. DEFER_TASKRUN requires +* the submitter task context, IOPOLL protects with uring_lock. +*/ +if (ctx->task_complete || (ctx->flags &IORING_SETUP_IOPOLL )) { + req->io_task_work.func = io_req_task_complete; +io_req_task_work_add (req); +return ; + } +io_cq_lock (ctx);if (!(req->flags &REQ_F_CQE_SKIP )) {if (!io_fill_cqe_req (ctx, req))
@@ -974,23 +984,6 @@ static void __io_req_complete_post(struct io_kiocb *req, unsigned issue_flags) } } -void io_req_complete_post (struct io_kiocb *req,unsigned issue_flags) -{ -struct io_ring_ctx *ctx = req->ctx; - -if (ctx->task_complete) { - req->io_task_work.func = io_req_task_complete; -io_req_task_work_add (req); - }else if (!(issue_flags &IO_URING_F_UNLOCKED ) || - !(ctx->flags &IORING_SETUP_IOPOLL )) { -__io_req_complete_post (req, issue_flags); - }else { -mutex_lock (&ctx->uring_lock); -__io_req_complete_post (req, issue_flags & ~IO_URING_F_UNLOCKED ); -mutex_unlock (&ctx->uring_lock); - } -} -void io_req_defer_failed (struct io_kiocb *req,s32 res) __must_hold(&ctx->uring_lock) {
io_uring/io_uring.h +0 -1
@@ -65,7 +65,6 @@ bool io_cqe_cache_refill(struct io_ring_ctx *ctx, bool overflow);void io_req_cqe_overflow (struct io_kiocb *req);int io_run_task_work_sig (struct io_ring_ctx *ctx);void io_req_defer_failed (struct io_kiocb *req,s32 res); -void io_req_complete_post (struct io_kiocb *req,unsigned issue_flags);bool io_post_aux_cqe (struct io_ring_ctx *ctx,u64 user_data,s32 res,u32 cflags);bool io_req_post_cqe (struct io_kiocb *req,s32 res,u32 cflags);void __io_commit_cqring_flush (struct io_ring_ctx *ctx);
Signature · 1 line
2.44.0
PATCH v2 14/14 io_uring: clean up io_lockdep_assert_cq_locked
In reply to Pavel BegunkovMove CONFIG_PROVE_LOCKING checks inside of io_lockdep_assert_cq_locked() and kill the else branch.
--- io_uring/io_uring.h | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
io_uring/io_uring.h +2 -6
@@ -119,9 +119,9 @@ enum {void io_eventfd_ops (struct rcu_head *rcu);void io_activate_pollwq (struct io_ring_ctx *ctx); -#if defined(CONFIG_PROVE_LOCKING )static inline void io_lockdep_assert_cq_locked (struct io_ring_ctx *ctx) { +#if defined(CONFIG_PROVE_LOCKING )lockdep_assert (in_task ());if (ctx->flags &IORING_SETUP_IOPOLL ) {
@@ -140,12 +140,8 @@ static inline void io_lockdep_assert_cq_locked(struct io_ring_ctx *ctx) else lockdep_assert(current == ctx->submitter_task); } -} -#else -static inline void io_lockdep_assert_cq_locked (struct io_ring_ctx *ctx) -{ -}#endif +}static inline void io_req_task_work_add (struct io_kiocb *req) {
Signature · 1 line
2.44.0