| Message ID | faeec0d1e7c740a582f51f80626f61c745ed9a52.1710720150.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | remove aux CQE caches | expand |
On Mon, Mar 18, 2024 at 12:41:48AM +0000, Pavel Begunkov wrote: > 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 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. > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > --- > io_uring/uring_cmd.c | 24 +++++++++++++++++------- > 1 file changed, 17 insertions(+), 7 deletions(-) > > diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c > index ec38a8d4836d..9590081feb2d 100644 > --- a/io_uring/uring_cmd.c > +++ b/io_uring/uring_cmd.c > @@ -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; > + 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
On 3/18/24 08:10, Ming Lei wrote: > On Mon, Mar 18, 2024 at 12:41:48AM +0000, Pavel Begunkov wrote: >> 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 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. >> >> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >> --- >> io_uring/uring_cmd.c | 24 +++++++++++++++++------- >> 1 file changed, 17 insertions(+), 7 deletions(-) >> >> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c >> index ec38a8d4836d..9590081feb2d 100644 >> --- a/io_uring/uring_cmd.c >> +++ b/io_uring/uring_cmd.c >> @@ -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; >> >> + 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.
On Mon, Mar 18, 2024 at 11:50:32AM +0000, Pavel Begunkov wrote: > On 3/18/24 08:10, Ming Lei wrote: > > On Mon, Mar 18, 2024 at 12:41:48AM +0000, Pavel Begunkov wrote: > > > 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 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. > > > > > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > > > --- > > > io_uring/uring_cmd.c | 24 +++++++++++++++++------- > > > 1 file changed, 17 insertions(+), 7 deletions(-) > > > > > > diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c > > > index ec38a8d4836d..9590081feb2d 100644 > > > --- a/io_uring/uring_cmd.c > > > +++ b/io_uring/uring_cmd.c > > > @@ -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; > > > + 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. 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
On 3/18/24 11:59, Ming Lei wrote: > On Mon, Mar 18, 2024 at 11:50:32AM +0000, Pavel Begunkov wrote: >> On 3/18/24 08:10, Ming Lei wrote: >>> On Mon, Mar 18, 2024 at 12:41:48AM +0000, Pavel Begunkov wrote: >>>> 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 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. >>>> >>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >>>> --- >>>> io_uring/uring_cmd.c | 24 +++++++++++++++++------- >>>> 1 file changed, 17 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c >>>> index ec38a8d4836d..9590081feb2d 100644 >>>> --- a/io_uring/uring_cmd.c >>>> +++ b/io_uring/uring_cmd.c >>>> @@ -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; >>>> + 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. > > 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?
On Mon, Mar 18, 2024 at 12:46:25PM +0000, Pavel Begunkov wrote: > On 3/18/24 11:59, Ming Lei wrote: > > On Mon, Mar 18, 2024 at 11:50:32AM +0000, Pavel Begunkov wrote: > > > On 3/18/24 08:10, Ming Lei wrote: > > > > On Mon, Mar 18, 2024 at 12:41:48AM +0000, Pavel Begunkov wrote: > > > > > 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 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. > > > > > > > > > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > > > > > --- > > > > > io_uring/uring_cmd.c | 24 +++++++++++++++++------- > > > > > 1 file changed, 17 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c > > > > > index ec38a8d4836d..9590081feb2d 100644 > > > > > --- a/io_uring/uring_cmd.c > > > > > +++ b/io_uring/uring_cmd.c > > > > > @@ -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; > > > > > + 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. > > > > 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? Looks I missed the point, now the WARN() is fine. Thanks, Ming
diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c index ec38a8d4836d..9590081feb2d 100644 --- a/io_uring/uring_cmd.c +++ b/io_uring/uring_cmd.c @@ -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); } }
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 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. Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- io_uring/uring_cmd.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)