Re: [PATCH V6 7/8] io_uring/uring_cmd: support provide group kernel buffer

[Pavel Begunkov <asml.silence@gmail.com>]

On 9/12/24 11:49, Ming Lei wrote:
> Allow uring command to be group leader for providing kernel buffer,
> and this way can support generic device zero copy over device buffer.
> 
> The following patch will use the way to support zero copy for ublk.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   include/linux/io_uring/cmd.h  |  7 +++++++
>   include/uapi/linux/io_uring.h |  7 ++++++-
>   io_uring/uring_cmd.c          | 28 ++++++++++++++++++++++++++++
>   3 files changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
> index 447fbfd32215..fde3a2ec7d9a 100644
> --- a/include/linux/io_uring/cmd.h
> +++ b/include/linux/io_uring/cmd.h
> @@ -48,6 +48,8 @@ void __io_uring_cmd_do_in_task(struct io_uring_cmd *ioucmd,
>   void io_uring_cmd_mark_cancelable(struct io_uring_cmd *cmd,
>   		unsigned int issue_flags);
>   
> +int io_uring_cmd_provide_kbuf(struct io_uring_cmd *ioucmd,
> +		const struct io_uring_kernel_buf *grp_kbuf);
>   #else
>   static inline int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw,
>   			      struct iov_iter *iter, void *ioucmd)
> @@ -67,6 +69,11 @@ static inline void io_uring_cmd_mark_cancelable(struct io_uring_cmd *cmd,
>   		unsigned int issue_flags)
>   {
>   }
> +static inline int io_uring_cmd_provide_kbuf(struct io_uring_cmd *ioucmd,
> +		const struct io_uring_kernel_buf *grp_kbuf)
> +{
> +	return -EOPNOTSUPP;
> +}
>   #endif
>   
>   /*
> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> index 2af32745ebd3..11985eeac10e 100644
> --- a/include/uapi/linux/io_uring.h
> +++ b/include/uapi/linux/io_uring.h
> @@ -271,9 +271,14 @@ enum io_uring_op {
>    * sqe->uring_cmd_flags		top 8bits aren't available for userspace
>    * IORING_URING_CMD_FIXED	use registered buffer; pass this flag
>    *				along with setting sqe->buf_index.
> + * IORING_PROVIDE_GROUP_KBUF	this command provides group kernel buffer
> + *				for member requests which can retrieve
> + *				any sub-buffer with offset(sqe->addr) and
> + *				len(sqe->len)

Is there a good reason it needs to be a cmd generic flag instead of
ublk specific?

1. Extra overhead for files / cmds that don't even care about the
feature.

2. As it stands with this patch, the flag is ignored by all other
cmd implementations, which might be quite confusing as an api,
especially so since if we don't set that REQ_F_GROUP_KBUF memeber
requests will silently try to import a buffer the "normal way",
i.e. interpret sqe->addr or such as the target buffer.

3. We can't even put some nice semantics on top since it's
still cmd specific and not generic to all other io_uring
requests.

I'd even think that it'd make sense to implement it as a
new cmd opcode, but that's the business of the file implementing
it, i.e. ublk.

>    */
>   #define IORING_URING_CMD_FIXED	(1U << 0)
> -#define IORING_URING_CMD_MASK	IORING_URING_CMD_FIXED
> +#define IORING_PROVIDE_GROUP_KBUF	(1U << 1)
> +#define IORING_URING_CMD_MASK	(IORING_URING_CMD_FIXED | IORING_PROVIDE_GROUP_KBUF)
>   
>   
>   /*
-- 
Pavel Begunkov