Re: [PATCH v3 07/25] io_uring/kbuf: add recycling for kernel managed buffer rings

[Gabriel Krisman Bertazi <krisman@suse.de>]

Gabriel Krisman Bertazi <krisman@suse.de> writes:

> Joanne Koong <joannelkoong@gmail.com> writes:
>
>> Add an interface for buffers to be recycled back into a kernel-managed
>> buffer ring.
>>
>> This is a preparatory patch for fuse over io-uring.
>>
>> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
>> ---
>>  include/linux/io_uring/cmd.h | 13 +++++++++++
>>  io_uring/kbuf.c              | 42 ++++++++++++++++++++++++++++++++++++
>>  io_uring/kbuf.h              |  3 +++
>>  io_uring/uring_cmd.c         | 11 ++++++++++
>>  4 files changed, 69 insertions(+)
>>
>> diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
>> index 424f071f42e5..7169a2a9a744 100644
>> --- a/include/linux/io_uring/cmd.h
>> +++ b/include/linux/io_uring/cmd.h
>> @@ -88,6 +88,11 @@ int io_uring_cmd_buf_ring_pin(struct io_uring_cmd *ioucmd, unsigned buf_group,
>>  			      unsigned issue_flags, struct io_buffer_list **bl);
>>  int io_uring_cmd_buf_ring_unpin(struct io_uring_cmd *ioucmd, unsigned buf_group,
>>  				unsigned issue_flags);
>> +
>> +int io_uring_cmd_kmbuffer_recycle(struct io_uring_cmd *cmd,
>> +				  unsigned int buf_group, u64 addr,
>> +				  unsigned int len, unsigned int bid,
>> +				  unsigned int issue_flags);
>>  #else
>>  static inline int
>>  io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw,
>> @@ -143,6 +148,14 @@ static inline int io_uring_cmd_buf_ring_unpin(struct io_uring_cmd *ioucmd,
>>  {
>>  	return -EOPNOTSUPP;
>>  }
>> +static inline int io_uring_cmd_kmbuffer_recycle(struct io_uring_cmd *cmd,
>> +						unsigned int buf_group,
>> +						u64 addr, unsigned int len,
>> +						unsigned int bid,
>> +						unsigned int issue_flags)
>> +{
>> +	return -EOPNOTSUPP;
>> +}
>>  #endif
>>  
>>  static inline struct io_uring_cmd *io_uring_cmd_from_tw(struct io_tw_req tw_req)
>> diff --git a/io_uring/kbuf.c b/io_uring/kbuf.c
>> index 03e05bab023a..f12d000b71c5 100644
>> --- a/io_uring/kbuf.c
>> +++ b/io_uring/kbuf.c
>> @@ -101,6 +101,48 @@ void io_kbuf_drop_legacy(struct io_kiocb *req)
>>  	req->kbuf = NULL;
>>  }
>>  
>> +int io_kmbuf_recycle(struct io_kiocb *req, unsigned int bgid, u64 addr,
>> +		     unsigned int len, unsigned int bid,
>> +		     unsigned int issue_flags)
>> +{
>> +	struct io_ring_ctx *ctx = req->ctx;
>> +	struct io_uring_buf_ring *br;
>> +	struct io_uring_buf *buf;
>> +	struct io_buffer_list *bl;
>> +	int ret = -EINVAL;
>> +
>> +	if (WARN_ON_ONCE(req->flags & REQ_F_BUFFERS_COMMIT))
>> +		return ret;
>> +
>> +	io_ring_submit_lock(ctx, issue_flags);
>> +
>> +	bl = io_buffer_get_list(ctx, bgid);
>> +
>> +	if (WARN_ON_ONCE(!(bl->flags & IOBL_BUF_RING)) ||
>> +	    WARN_ON_ONCE(!(bl->flags & IOBL_KERNEL_MANAGED)))
>> +		goto done;
>
> Hi Joanne,
>
> WARN_ONs are not supposed to be reached by the user, but I think that is
> possible here, i.e. by passing the bgid of legacy provided buffers.

But now I see this is never exposed to userspace as an io_uring_cmd
command itself, it is only used internally by other fuse operations.
Nevertheless, it's implemented as an io_uring_cmd by
io_uring_cmd_kmbuffer_recycle.

Is it eventually going to be exposed as operations to userspace? If not,
I'd suggest to stay out of the io_uring_cmd namespace (perhaps call
io_kmbuf_recycle directly from fs/fuse).  Do we need to have this
io_uring_cmd abstraction for some reason I'm missing?

Thanks,

-- 
Gabriel Krisman Bertazi