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

From: Luis Chamberlain <mcgrof@kernel.org>
To: Kanchan Joshi <joshi.k@samsung.com>
Cc: io-uring@vger.kernel.org, linux-nvme@lists.infradead.org,
	linux-block@vger.kernel.org, axboe@kernel.dk, hch@lst.de,
	kbusch@kernel.org, javier@javigon.com, anuj20.g@samsung.com,
	joshiiitr@gmail.com, pankydev8@gmail.com
Subject: Re: [RFC 01/13] io_uring: add infra for uring_cmd completion in submitter-task
Date: Wed, 16 Feb 2022 18:13:19 -0800	[thread overview]
Message-ID: <Yg2vP7lo3hGLGakx@bombadil.infradead.org> (raw)
In-Reply-To: <20211220141734.12206-2-joshi.k@samsung.com>

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