[RFC 01/13] io_uring: add infra for uring_cmd completion in submitter-task

[Kanchan Joshi]

On Thu, Feb 17, 2022 at 7:43 AM Luis Chamberlain <mcgrof at kernel.org> wrote:
>
> On Mon, Dec 20, 2021 at 07:47:22PM +0530, Kanchan Joshi wrote:
> > Completion of a uring_cmd ioctl may involve referencing certain
> > ioctl-specific fields, requiring original submitter context.
> > Export an API that driver can use for this purpose.
> > The API facilitates reusing task-work infra of io_uring, while driver
> > gets to implement cmd-specific handling in a callback.
> >
> > Signed-off-by: Kanchan Joshi <joshi.k at samsung.com>
> > Signed-off-by: Anuj Gupta <anuj20.g at samsung.com>
> > ---
> >  fs/io_uring.c            | 16 ++++++++++++++++
> >  include/linux/io_uring.h |  8 ++++++++
> >  2 files changed, 24 insertions(+)
> >
> > diff --git a/fs/io_uring.c b/fs/io_uring.c
> > index e96ed3d0385e..246f1085404d 100644
> > --- a/fs/io_uring.c
> > +++ b/fs/io_uring.c
> > @@ -2450,6 +2450,22 @@ static void io_req_task_submit(struct io_kiocb *req, bool *locked)
> >               io_req_complete_failed(req, -EFAULT);
> >  }
> >
> > +static void io_uring_cmd_work(struct io_kiocb *req, bool *locked)
> > +{
> > +     req->uring_cmd.driver_cb(&req->uring_cmd);
>
> If the callback memory area is gone, boom.

Why will the memory area be gone?
Module removal is protected because try_module_get is done anyway when
the namespace was opened.

> > +}
> > +
> > +void io_uring_cmd_complete_in_task(struct io_uring_cmd *ioucmd,
> > +                     void (*driver_cb)(struct io_uring_cmd *))
>
> Adding kdoc style comment for this would be nice. Please document
> the context that is allowed.

Sure, for all kdoc style comments. Will add that in the next version.

> > +{
> > +     struct io_kiocb *req = container_of(ioucmd, struct io_kiocb, uring_cmd);
> > +
> > +     req->uring_cmd.driver_cb = driver_cb;
> > +     req->io_task_work.func = io_uring_cmd_work;
> > +     io_req_task_work_add(req, !!(req->ctx->flags & IORING_SETUP_SQPOLL));
>
> This can schedules, and so the callback may go fishing in the meantime.

io_req_task_work_add is safe to be called in atomic context.
FWIW, io_uring uses this for regular (i.e. direct block) io completion too.

> > +}
> > +EXPORT_SYMBOL_GPL(io_uring_cmd_complete_in_task);
> > +
> >  static void io_req_task_queue_fail(struct io_kiocb *req, int ret)
> >  {
> >       req->result = ret;
> > diff --git a/include/linux/io_uring.h b/include/linux/io_uring.h
> > index 64e788b39a86..f4b4990a3b62 100644
> > --- a/include/linux/io_uring.h
> > +++ b/include/linux/io_uring.h
> > @@ -14,11 +14,15 @@ struct io_uring_cmd {
> >       __u16           op;
> >       __u16           unused;
> >       __u32           len;
> > +     /* used if driver requires update in task context*/
>
> By using kdoc above youcan remove this comment.
>
> > +     void (*driver_cb)(struct io_uring_cmd *cmd);
>
> So we'd need a struct module here I think if we're going to
> defer this into memory which can be removed.
>
Same as the previous module-removal comment.Do we really need that?

Thanks,
--