| Message ID | 20211220141734.12206-2-joshi.k@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | uring-passthru for nvme | expand |
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@samsung.com> > Signed-off-by: Anuj Gupta <anuj20.g@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. > +} > + > +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. > +{ > + 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. > +} > +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. Luis
On Thu, Feb 17, 2022 at 7:43 AM Luis Chamberlain <mcgrof@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@samsung.com> > > Signed-off-by: Anuj Gupta <anuj20.g@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, --
On 2/17/22 8:39 AM, Kanchan Joshi wrote: > On Thu, Feb 17, 2022 at 7:43 AM Luis Chamberlain <mcgrof@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@samsung.com> >>> Signed-off-by: Anuj Gupta <anuj20.g@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. And the req isn't going away before it's completed. >>> +{ >>> + 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. Correct, it doesn't schedule and is safe from irq context as long as the task is pinned (which it is, via the req itself).
On Thu, Feb 17, 2022 at 08:50:59AM -0700, Jens Axboe wrote: > On 2/17/22 8:39 AM, Kanchan Joshi wrote: > > On Thu, Feb 17, 2022 at 7:43 AM Luis Chamberlain <mcgrof@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@samsung.com> > >>> Signed-off-by: Anuj Gupta <anuj20.g@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. > > And the req isn't going away before it's completed. Groovy, it would be nice to add a little /* comment */ to just remind the reader? > >>> +{ > >>> + 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. > > Correct, it doesn't schedule and is safe from irq context as long as the > task is pinned (which it is, via the req itself). Great, a kdoc explaining the routine and that it can be called from atomic context and the rationale would be very useful to users. And .. so the callback *must* be safe in atomic context too or can it sleep? Luis
On Thu, Feb 17, 2022 at 08:50:59AM -0700, Jens Axboe wrote: > On 2/17/22 8:39 AM, Kanchan Joshi wrote: > > On Thu, Feb 17, 2022 at 7:43 AM Luis Chamberlain <mcgrof@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@samsung.com> > >>> Signed-off-by: Anuj Gupta <anuj20.g@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. > > And the req isn't going away before it's completed. Just to be clear, I was thinking outside of the block layer context too. Does this still hold true? Luis
On 2/17/22 11:46 AM, Luis Chamberlain wrote: > On Thu, Feb 17, 2022 at 08:50:59AM -0700, Jens Axboe wrote: >> On 2/17/22 8:39 AM, Kanchan Joshi wrote: >>> On Thu, Feb 17, 2022 at 7:43 AM Luis Chamberlain <mcgrof@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@samsung.com> >>>>> Signed-off-by: Anuj Gupta <anuj20.g@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. >> >> And the req isn't going away before it's completed. > > Just to be clear, I was thinking outside of the block layer context too. > Does this still hold true? It has nothing to do with the block layer, the req belongs to io_uring context. io_uring has ownership of it.
On Thu, Feb 17, 2022 at 11:26 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > On Thu, Feb 17, 2022 at 08:50:59AM -0700, Jens Axboe wrote: > > On 2/17/22 8:39 AM, Kanchan Joshi wrote: > > > On Thu, Feb 17, 2022 at 7:43 AM Luis Chamberlain <mcgrof@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@samsung.com> > > >>> Signed-off-by: Anuj Gupta <anuj20.g@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. > > > > And the req isn't going away before it's completed. > > Groovy, it would be nice to add a little /* comment */ to just remind > the reader? > > > >>> +{ > > >>> + 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. > > > > Correct, it doesn't schedule and is safe from irq context as long as the > > task is pinned (which it is, via the req itself). > > Great, a kdoc explaining the routine and that it can be called from > atomic context and the rationale would be very useful to users. And .. > so the callback *must* be safe in atomic context too or can it sleep? Callback will be invoked in task/process context. Allowing much more to do than we can in atomic context, including sleep if necessary.
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); +} + +void io_uring_cmd_complete_in_task(struct io_uring_cmd *ioucmd, + void (*driver_cb)(struct io_uring_cmd *)) +{ + 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)); +} +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*/ + void (*driver_cb)(struct io_uring_cmd *cmd); __u64 pdu[5]; /* 40 bytes available inline for free use */ }; #if defined(CONFIG_IO_URING) void io_uring_cmd_done(struct io_uring_cmd *cmd, ssize_t ret); +void io_uring_cmd_complete_in_task(struct io_uring_cmd *ioucmd, + void (*driver_cb)(struct io_uring_cmd *)); struct sock *io_uring_get_socket(struct file *file); void __io_uring_cancel(bool cancel_all); void __io_uring_free(struct task_struct *tsk); @@ -42,6 +46,10 @@ static inline void io_uring_free(struct task_struct *tsk) static inline void io_uring_cmd_done(struct io_uring_cmd *cmd, ssize_t ret) { } +static inline void io_uring_cmd_complete_in_task(struct io_uring_cmd *ioucmd, + void (*driver_cb)(struct io_uring_cmd *)) +{ +} static inline struct sock *io_uring_get_socket(struct file *file) { return NULL;