Re: [RFC] io_uring: let IORING_OP_FILES_UPDATE support to choose fixed file slot

[Hao Xu <haoxu.linux@icloud.com>]

Hi Xiaoguang,

On 5/26/22 20:38, Xiaoguang Wang wrote:
> One big issue with file registration feature is that it needs user
> space apps to maintain free slot info about io_uring's fixed file
> table, which really is a burden for development. Now since io_uring
> starts to choose free file slot for user space apps by using
> IORING_FILE_INDEX_ALLOC flag in accept or open operations, but they
> need app to uses direct accept or direct open, which as far as I know,
> some apps are not prepared to use direct accept or open yet.
> 
> To support apps, who still need real fds, use registration feature
> easier, let IORING_OP_FILES_UPDATE support to choose fixed file slot,
> which will return free file slot in cqe->res.
> 
> TODO list:
>      Need to prepare liburing corresponding helpers.
> 
> Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>
> ---
>   fs/io_uring.c                 | 50 ++++++++++++++++++++++++++++++++++---------
>   include/uapi/linux/io_uring.h |  1 +
>   2 files changed, 41 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 9f1c682d7caf..d77e6bbec81c 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -680,6 +680,7 @@ struct io_rsrc_update {
>   	u64				arg;
>   	u32				nr_args;
>   	u32				offset;
> +	u32				flags;
>   };
>   
>   struct io_fadvise {
> @@ -7970,14 +7971,23 @@ static int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags)
>   	return 0;
>   }
>   
> +#define IORING_FILES_UPDATE_INDEX_ALLOC 1
> +
>   static int io_rsrc_update_prep(struct io_kiocb *req,
>   				const struct io_uring_sqe *sqe)
>   {
> +	u32 flags = READ_ONCE(sqe->files_update_flags);
> +
>   	if (unlikely(req->flags & (REQ_F_FIXED_FILE | REQ_F_BUFFER_SELECT)))
>   		return -EINVAL;
> -	if (sqe->rw_flags || sqe->splice_fd_in)
> +	if (sqe->splice_fd_in)
> +		return -EINVAL;
> +	if (flags & ~IORING_FILES_UPDATE_INDEX_ALLOC)
> +		return -EINVAL;
> +	if ((flags & IORING_FILES_UPDATE_INDEX_ALLOC) && READ_ONCE(sqe->len) != 1)

How about allowing multiple fd update in IORING_FILES_UPDATE_INDEX_ALLOC
case? For example, using the sqe->addr(the fd array) to store the slots 
we allocated, and let cqe return the number of slots allocated.
By the way, another way, we can levarage up->offset == 
IORING_FILE_INDEX_ALLOC
to do the mode check since seems it is not used in this mode. Though
I'm not sure that is better..

>   		return -EINVAL;
>   
> +	req->rsrc_update.flags = flags;
>   	req->rsrc_update.offset = READ_ONCE(sqe->off);
>   	req->rsrc_update.nr_args = READ_ONCE(sqe->len);
>   	if (!req->rsrc_update.nr_args)
> @@ -7990,18 +8000,38 @@ static int io_files_update(struct io_kiocb *req, unsigned int issue_flags)
>   {
>   	struct io_ring_ctx *ctx = req->ctx;
>   	struct io_uring_rsrc_update2 up;
> +	struct file *file;
>   	int ret;
>   
> -	up.offset = req->rsrc_update.offset;
> -	up.data = req->rsrc_update.arg;
> -	up.nr = 0;
> -	up.tags = 0;
> -	up.resv = 0;
> -	up.resv2 = 0;
> +	if (req->rsrc_update.flags & IORING_FILES_UPDATE_INDEX_ALLOC) {
> +		int fd;
>   
> -	io_ring_submit_lock(ctx, issue_flags);
> -	ret = __io_register_rsrc_update(ctx, IORING_RSRC_FILE,
> -					&up, req->rsrc_update.nr_args);
> +		if (copy_from_user(&fd, (int *)req->rsrc_update.arg, sizeof(fd))) {

                                           ^ (void __user *) ?

Regards,
Hao