[PATCH v2] io_uring: let IORING_OP_FILES_UPDATE support to choose fixed file slots

[PATCH v2] io_uring: let IORING_OP_FILES_UPDATE support to choose fixed file slots

[Xiaoguang Wang]

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 slots,
which will store picked fixed files slots in fd array and let cqe return
the number of slots allocated.

Suggested-by: Hao Xu <howeyxu@tencent.com>
Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>
---
V2:
  Add illegal flags check in io_close_prep().
---
 fs/io_uring.c                 | 75 +++++++++++++++++++++++++++++++++++++------
 include/uapi/linux/io_uring.h |  1 +
 2 files changed, 66 insertions(+), 10 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 6d91148e9679..18a7459fb6e7 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -574,6 +574,7 @@ struct io_close {
 	struct file			*file;
 	int				fd;
 	u32				file_slot;
+	u32				flags;
 };
 
 struct io_timeout_data {
@@ -1366,7 +1367,9 @@ static int io_req_prep_async(struct io_kiocb *req);
 
 static int io_install_fixed_file(struct io_kiocb *req, struct file *file,
 				 unsigned int issue_flags, u32 slot_index);
-static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags);
+static int __io_close_fixed(struct io_kiocb *req, unsigned int issue_flags,
+			    unsigned int offset);
+static inline int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags);
 
 static enum hrtimer_restart io_link_timeout_fn(struct hrtimer *timer);
 static void io_eventfd_signal(struct io_ring_ctx *ctx);
@@ -5945,16 +5948,22 @@ static int io_statx(struct io_kiocb *req, unsigned int issue_flags)
 	return 0;
 }
 
+#define IORING_CLOSE_FD_AND_FILE_SLOT 1
+
 static int io_close_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 {
-	if (sqe->off || sqe->addr || sqe->len || sqe->rw_flags || sqe->buf_index)
+	if (sqe->off || sqe->addr || sqe->len || sqe->buf_index)
 		return -EINVAL;
 	if (req->flags & REQ_F_FIXED_FILE)
 		return -EBADF;
 
 	req->close.fd = READ_ONCE(sqe->fd);
 	req->close.file_slot = READ_ONCE(sqe->file_index);
-	if (req->close.file_slot && req->close.fd)
+	req->close.flags = READ_ONCE(sqe->close_flags);
+	if (req->close.flags & ~IORING_CLOSE_FD_AND_FILE_SLOT)
+		return -EINVAL;
+	if (!(req->close.flags & IORING_CLOSE_FD_AND_FILE_SLOT) &&
+	    req->close.file_slot && req->close.fd)
 		return -EINVAL;
 
 	return 0;
@@ -5970,7 +5979,8 @@ static int io_close(struct io_kiocb *req, unsigned int issue_flags)
 
 	if (req->close.file_slot) {
 		ret = io_close_fixed(req, issue_flags);
-		goto err;
+		if (ret || !(req->close.flags & IORING_CLOSE_FD_AND_FILE_SLOT))
+			goto err;
 	}
 
 	spin_lock(&files->file_lock);
@@ -8003,6 +8013,42 @@ static int io_files_update_prep(struct io_kiocb *req,
 	return 0;
 }
 
+static int io_files_update_with_index_alloc(struct io_kiocb *req,
+					    unsigned int issue_flags)
+{
+	__s32 __user *fds = u64_to_user_ptr(req->rsrc_update.arg);
+	struct file *file;
+	unsigned int done, nr_fds = req->rsrc_update.nr_args;
+	int ret, fd;
+
+	for (done = 0; done < nr_fds; done++) {
+		if (copy_from_user(&fd, &fds[done], sizeof(fd))) {
+			ret = -EFAULT;
+			break;
+		}
+
+		file = fget(fd);
+		if (!file) {
+			ret = -EBADF;
+			goto out;
+		}
+		ret = io_fixed_fd_install(req, issue_flags, file,
+					  IORING_FILE_INDEX_ALLOC);
+		if (ret < 0)
+			goto out;
+		if (copy_to_user(&fds[done], &ret, sizeof(ret))) {
+			ret = -EFAULT;
+			__io_close_fixed(req, issue_flags, ret);
+			break;
+		}
+	}
+
+out:
+	if (done)
+		return done;
+	return ret;
+}
+
 static int io_files_update(struct io_kiocb *req, unsigned int issue_flags)
 {
 	struct io_ring_ctx *ctx = req->ctx;
@@ -8016,10 +8062,14 @@ static int io_files_update(struct io_kiocb *req, unsigned int issue_flags)
 	up.resv = 0;
 	up.resv2 = 0;
 
-	io_ring_submit_lock(ctx, issue_flags);
-	ret = __io_register_rsrc_update(ctx, IORING_RSRC_FILE,
-					&up, req->rsrc_update.nr_args);
-	io_ring_submit_unlock(ctx, issue_flags);
+	if (req->rsrc_update.offset == IORING_FILE_INDEX_ALLOC) {
+		ret = io_files_update_with_index_alloc(req, issue_flags);
+	} else {
+		io_ring_submit_lock(ctx, issue_flags);
+		ret = __io_register_rsrc_update(ctx, IORING_RSRC_FILE,
+				&up, req->rsrc_update.nr_args);
+		io_ring_submit_unlock(ctx, issue_flags);
+	}
 
 	if (ret < 0)
 		req_set_fail(req);
@@ -10183,9 +10233,9 @@ static int io_install_fixed_file(struct io_kiocb *req, struct file *file,
 	return ret;
 }
 
-static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags)
+static int __io_close_fixed(struct io_kiocb *req, unsigned int issue_flags,
+			    unsigned int offset)
 {
-	unsigned int offset = req->close.file_slot - 1;
 	struct io_ring_ctx *ctx = req->ctx;
 	struct io_fixed_file *file_slot;
 	struct file *file;
@@ -10222,6 +10272,11 @@ static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags)
 	return ret;
 }
 
+static inline int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags)
+{
+	return __io_close_fixed(req, issue_flags, req->close.file_slot - 1);
+}
+
 static int __io_sqe_files_update(struct io_ring_ctx *ctx,
 				 struct io_uring_rsrc_update2 *up,
 				 unsigned nr_args)
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index 53e7dae92e42..e347b3fea4e4 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -47,6 +47,7 @@ struct io_uring_sqe {
 		__u32		unlink_flags;
 		__u32		hardlink_flags;
 		__u32		xattr_flags;
+		__u32		close_flags;
 	};
 	__u64	user_data;	/* data to be passed back at completion time */
 	/* pack this to avoid bogus arm OABI complaints */
-- 
2.14.4.44.g2045bb6

Re: [PATCH v2] io_uring: let IORING_OP_FILES_UPDATE support to choose fixed file slots

[Jens Axboe]

On 5/30/22 7:15 AM, Xiaoguang Wang wrote:
> @@ -5945,16 +5948,22 @@ static int io_statx(struct io_kiocb *req, unsigned int issue_flags)
>  	return 0;
>  }
>  
> +#define IORING_CLOSE_FD_AND_FILE_SLOT 1
> +

This should go into uapi/linux/io_uring.h - I'll just move it, no need
for a v3 for that. Test case should add it too.


-- 
Jens Axboe

Re: [PATCH v2] io_uring: let IORING_OP_FILES_UPDATE support to choose fixed file slots

[Jens Axboe]

On 5/30/22 7:18 AM, Jens Axboe wrote:
> On 5/30/22 7:15 AM, Xiaoguang Wang wrote:
>> @@ -5945,16 +5948,22 @@ static int io_statx(struct io_kiocb *req, unsigned int issue_flags)
>>  	return 0;
>>  }
>>  
>> +#define IORING_CLOSE_FD_AND_FILE_SLOT 1
>> +
> 
> This should go into uapi/linux/io_uring.h - I'll just move it, no need
> for a v3 for that. Test case should add it too.

Here's what I merged so far:

https://git.kernel.dk/cgit/linux-block/commit/?h=io_uring-5.19&id=f6b0e7c95c20d4889b811ada7fc0061e8cb4e82e

Changes:

- I re-wrote the commit message slightly
- Move flag to header where it belongs
- Get rid of 'goto' in io_files_update_with_index_alloc()
- Drop unneeded variable in io_files_update_with_index_alloc()

-- 
Jens Axboe

Re: [PATCH v2] io_uring: let IORING_OP_FILES_UPDATE support to choose fixed file slots

[Xiaoguang Wang]

hi,

> On 5/30/22 7:18 AM, Jens Axboe wrote:
>> On 5/30/22 7:15 AM, Xiaoguang Wang wrote:
>>> @@ -5945,16 +5948,22 @@ static int io_statx(struct io_kiocb *req, unsigned int issue_flags)
>>>  	return 0;
>>>  }
>>>  
>>> +#define IORING_CLOSE_FD_AND_FILE_SLOT 1
>>> +
>> This should go into uapi/linux/io_uring.h - I'll just move it, no need
>> for a v3 for that. Test case should add it too.
> Here's what I merged so far:
>
> https://git.kernel.dk/cgit/linux-block/commit/?h=io_uring-5.19&id=f6b0e7c95c20d4889b811ada7fc0061e8cb4e82e
>
> Changes:
>
> - I re-wrote the commit message slightly
> - Move flag to header where it belongs
> - Get rid of 'goto' in io_files_update_with_index_alloc()
> - Drop unneeded variable in io_files_update_with_index_alloc()
I think file registration feature is much easier to use now, thanks!

Regards,
Xiaoguang Wang

>

Re: [PATCH v2] io_uring: let IORING_OP_FILES_UPDATE support to choose fixed file slots

[Jens Axboe]

On 5/30/22 10:15 AM, Xiaoguang Wang wrote:
> hi,
> 
>> On 5/30/22 7:18 AM, Jens Axboe wrote:
>>> On 5/30/22 7:15 AM, Xiaoguang Wang wrote:
>>>> @@ -5945,16 +5948,22 @@ static int io_statx(struct io_kiocb *req, unsigned int issue_flags)
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +#define IORING_CLOSE_FD_AND_FILE_SLOT 1
>>>> +
>>> This should go into uapi/linux/io_uring.h - I'll just move it, no need
>>> for a v3 for that. Test case should add it too.
>> Here's what I merged so far:
>>
>> https://git.kernel.dk/cgit/linux-block/commit/?h=io_uring-5.19&id=f6b0e7c95c20d4889b811ada7fc0061e8cb4e82e
>>
>> Changes:
>>
>> - I re-wrote the commit message slightly
>> - Move flag to header where it belongs
>> - Get rid of 'goto' in io_files_update_with_index_alloc()
>> - Drop unneeded variable in io_files_update_with_index_alloc()
> I think file registration feature is much easier to use now, thanks!

Thanks for making the change! Will you send a v2 of the liburing test?
Then I'll get that queued up too.

-- 
Jens Axboe

Re: [PATCH v2] io_uring: let IORING_OP_FILES_UPDATE support to choose fixed file slots

[Xiaoguang Wang]

> On 5/30/22 10:15 AM, Xiaoguang Wang wrote:
>> hi,
>>
>>> On 5/30/22 7:18 AM, Jens Axboe wrote:
>>>> On 5/30/22 7:15 AM, Xiaoguang Wang wrote:
>>>>> @@ -5945,16 +5948,22 @@ static int io_statx(struct io_kiocb *req, unsigned int issue_flags)
>>>>>  	return 0;
>>>>>  }
>>>>>  
>>>>> +#define IORING_CLOSE_FD_AND_FILE_SLOT 1
>>>>> +
>>>> This should go into uapi/linux/io_uring.h - I'll just move it, no need
>>>> for a v3 for that. Test case should add it too.
>>> Here's what I merged so far:
>>>
>>> https://git.kernel.dk/cgit/linux-block/commit/?h=io_uring-5.19&id=f6b0e7c95c20d4889b811ada7fc0061e8cb4e82e
>>>
>>> Changes:
>>>
>>> - I re-wrote the commit message slightly
>>> - Move flag to header where it belongs
>>> - Get rid of 'goto' in io_files_update_with_index_alloc()
>>> - Drop unneeded variable in io_files_update_with_index_alloc()
>> I think file registration feature is much easier to use now, thanks!
> Thanks for making the change! Will you send a v2 of the liburing test?
Yeah, I'll do it, will send it soon.

Regards,
Xiaoguang Wang
> Then I'll get that queued up too.
>

Re: [PATCH v2] io_uring: let IORING_OP_FILES_UPDATE support to choose fixed file slots

[Pavel Begunkov]

On 5/30/22 14:15, 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 slots,
> which will store picked fixed files slots in fd array and let cqe return
> the number of slots allocated.

Why close bits are piggybacked in this patch without any mention
in the commit message? What is error semantics of
IORING_CLOSE_FD_AND_FILE_SLOT. Fail if any errored or both? How
can it be reliably used? Why we do two separate things in one
request with not clear semantics?

There is already one fix pending. Another problem on the surface
is that it may call io_close() twice and the second will fail, e.g.

-> io_close()
---> close_fixed()
---> if (file->flush) return -EAGAIN
-> io_close()
---> close_fixed() // fails


I do think we need to revert the close change, and then remake
properly and only if someone can describe sane semantics for it.



> Suggested-by: Hao Xu <howeyxu@tencent.com>
> Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>
> ---
> V2:
>    Add illegal flags check in io_close_prep().
> ---
>   fs/io_uring.c                 | 75 +++++++++++++++++++++++++++++++++++++------
>   include/uapi/linux/io_uring.h |  1 +
>   2 files changed, 66 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 6d91148e9679..18a7459fb6e7 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -574,6 +574,7 @@ struct io_close {
>   	struct file			*file;
>   	int				fd;
>   	u32				file_slot;
> +	u32				flags;
>   };
>   
>   struct io_timeout_data {
> @@ -1366,7 +1367,9 @@ static int io_req_prep_async(struct io_kiocb *req);
>   
>   static int io_install_fixed_file(struct io_kiocb *req, struct file *file,
>   				 unsigned int issue_flags, u32 slot_index);
> -static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags);
> +static int __io_close_fixed(struct io_kiocb *req, unsigned int issue_flags,
> +			    unsigned int offset);
> +static inline int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags);
>   
>   static enum hrtimer_restart io_link_timeout_fn(struct hrtimer *timer);
>   static void io_eventfd_signal(struct io_ring_ctx *ctx);
> @@ -5945,16 +5948,22 @@ static int io_statx(struct io_kiocb *req, unsigned int issue_flags)
>   	return 0;
>   }
>   
> +#define IORING_CLOSE_FD_AND_FILE_SLOT 1
> +
>   static int io_close_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>   {
> -	if (sqe->off || sqe->addr || sqe->len || sqe->rw_flags || sqe->buf_index)
> +	if (sqe->off || sqe->addr || sqe->len || sqe->buf_index)
>   		return -EINVAL;
>   	if (req->flags & REQ_F_FIXED_FILE)
>   		return -EBADF;
>   
>   	req->close.fd = READ_ONCE(sqe->fd);
>   	req->close.file_slot = READ_ONCE(sqe->file_index);
> -	if (req->close.file_slot && req->close.fd)
> +	req->close.flags = READ_ONCE(sqe->close_flags);
> +	if (req->close.flags & ~IORING_CLOSE_FD_AND_FILE_SLOT)
> +		return -EINVAL;
> +	if (!(req->close.flags & IORING_CLOSE_FD_AND_FILE_SLOT) &&
> +	    req->close.file_slot && req->close.fd)
>   		return -EINVAL;
>   
>   	return 0;
> @@ -5970,7 +5979,8 @@ static int io_close(struct io_kiocb *req, unsigned int issue_flags)
>   
>   	if (req->close.file_slot) {
>   		ret = io_close_fixed(req, issue_flags);
> -		goto err;
> +		if (ret || !(req->close.flags & IORING_CLOSE_FD_AND_FILE_SLOT))
> +			goto err;
>   	}
>   
>   	spin_lock(&files->file_lock);
> @@ -8003,6 +8013,42 @@ static int io_files_update_prep(struct io_kiocb *req,
>   	return 0;
>   }
>   
> +static int io_files_update_with_index_alloc(struct io_kiocb *req,
> +					    unsigned int issue_flags)
> +{
> +	__s32 __user *fds = u64_to_user_ptr(req->rsrc_update.arg);
> +	struct file *file;
> +	unsigned int done, nr_fds = req->rsrc_update.nr_args;
> +	int ret, fd;
> +
> +	for (done = 0; done < nr_fds; done++) {
> +		if (copy_from_user(&fd, &fds[done], sizeof(fd))) {
> +			ret = -EFAULT;
> +			break;
> +		}
> +
> +		file = fget(fd);
> +		if (!file) {
> +			ret = -EBADF;
> +			goto out;
> +		}
> +		ret = io_fixed_fd_install(req, issue_flags, file,
> +					  IORING_FILE_INDEX_ALLOC);
> +		if (ret < 0)
> +			goto out;
> +		if (copy_to_user(&fds[done], &ret, sizeof(ret))) {
> +			ret = -EFAULT;
> +			__io_close_fixed(req, issue_flags, ret);
> +			break;
> +		}
> +	}
> +
> +out:
> +	if (done)
> +		return done;
> +	return ret;
> +}
> +
>   static int io_files_update(struct io_kiocb *req, unsigned int issue_flags)
>   {
>   	struct io_ring_ctx *ctx = req->ctx;
> @@ -8016,10 +8062,14 @@ static int io_files_update(struct io_kiocb *req, unsigned int issue_flags)
>   	up.resv = 0;
>   	up.resv2 = 0;
>   
> -	io_ring_submit_lock(ctx, issue_flags);
> -	ret = __io_register_rsrc_update(ctx, IORING_RSRC_FILE,
> -					&up, req->rsrc_update.nr_args);
> -	io_ring_submit_unlock(ctx, issue_flags);
> +	if (req->rsrc_update.offset == IORING_FILE_INDEX_ALLOC) {
> +		ret = io_files_update_with_index_alloc(req, issue_flags);
> +	} else {
> +		io_ring_submit_lock(ctx, issue_flags);
> +		ret = __io_register_rsrc_update(ctx, IORING_RSRC_FILE,
> +				&up, req->rsrc_update.nr_args);
> +		io_ring_submit_unlock(ctx, issue_flags);
> +	}
>   
>   	if (ret < 0)
>   		req_set_fail(req);
> @@ -10183,9 +10233,9 @@ static int io_install_fixed_file(struct io_kiocb *req, struct file *file,
>   	return ret;
>   }
>   
> -static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags)
> +static int __io_close_fixed(struct io_kiocb *req, unsigned int issue_flags,
> +			    unsigned int offset)
>   {
> -	unsigned int offset = req->close.file_slot - 1;
>   	struct io_ring_ctx *ctx = req->ctx;
>   	struct io_fixed_file *file_slot;
>   	struct file *file;
> @@ -10222,6 +10272,11 @@ static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags)
>   	return ret;
>   }
>   
> +static inline int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags)
> +{
> +	return __io_close_fixed(req, issue_flags, req->close.file_slot - 1);
> +}
> +
>   static int __io_sqe_files_update(struct io_ring_ctx *ctx,
>   				 struct io_uring_rsrc_update2 *up,
>   				 unsigned nr_args)
> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> index 53e7dae92e42..e347b3fea4e4 100644
> --- a/include/uapi/linux/io_uring.h
> +++ b/include/uapi/linux/io_uring.h
> @@ -47,6 +47,7 @@ struct io_uring_sqe {
>   		__u32		unlink_flags;
>   		__u32		hardlink_flags;
>   		__u32		xattr_flags;
> +		__u32		close_flags;
>   	};
>   	__u64	user_data;	/* data to be passed back at completion time */
>   	/* pack this to avoid bogus arm OABI complaints */

-- 
Pavel Begunkov

Re: [PATCH v2] io_uring: let IORING_OP_FILES_UPDATE support to choose fixed file slots

[Xiaoguang Wang]

hello Pavel,

> On 5/30/22 14:15, 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 slots,
>> which will store picked fixed files slots in fd array and let cqe return
>> the number of slots allocated.
>
> Why close bits are piggybacked in this patch without any mention
> in the commit message? 
Sorry, it actually should be in one separate patch.

> What is error semantics of
> IORING_CLOSE_FD_AND_FILE_SLOT. Fail if any errored or both? How
> can it be reliably used? Why we do two separate things in one
> request with not clear semantics?
Yeah, indeed I know this issue when writing this patch, either normal
fd close or file slot close may fail independently.  I put them together
just for efficiency, one CLOSE op can do two jobs.

>
> There is already one fix pending. Another problem on the surface
> is that it may call io_close() twice and the second will fail, e.g.
>
> -> io_close()
> ---> close_fixed()
> ---> if (file->flush) return -EAGAIN
> -> io_close()
> ---> close_fixed() // fails
Thanks for pointing this issue.

>
>
> I do think we need to revert the close change, and then remake
> properly and only if someone can describe sane semantics for it.
Revert this close change seems reasonable, I thought for a while,
didn't figure out better solutions yet.
Sorry for the trouble again.

Regards,
Xiaoguang Wang
>
>
>
>> Suggested-by: Hao Xu <howeyxu@tencent.com>
>> Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>
>> ---
>> V2:
>>    Add illegal flags check in io_close_prep().
>> ---
>>   fs/io_uring.c                 | 75 +++++++++++++++++++++++++++++++++++++------
>>   include/uapi/linux/io_uring.h |  1 +
>>   2 files changed, 66 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 6d91148e9679..18a7459fb6e7 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -574,6 +574,7 @@ struct io_close {
>>       struct file            *file;
>>       int                fd;
>>       u32                file_slot;
>> +    u32                flags;
>>   };
>>     struct io_timeout_data {
>> @@ -1366,7 +1367,9 @@ static int io_req_prep_async(struct io_kiocb *req);
>>     static int io_install_fixed_file(struct io_kiocb *req, struct file *file,
>>                    unsigned int issue_flags, u32 slot_index);
>> -static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags);
>> +static int __io_close_fixed(struct io_kiocb *req, unsigned int issue_flags,
>> +                unsigned int offset);
>> +static inline int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags);
>>     static enum hrtimer_restart io_link_timeout_fn(struct hrtimer *timer);
>>   static void io_eventfd_signal(struct io_ring_ctx *ctx);
>> @@ -5945,16 +5948,22 @@ static int io_statx(struct io_kiocb *req, unsigned int issue_flags)
>>       return 0;
>>   }
>>   +#define IORING_CLOSE_FD_AND_FILE_SLOT 1
>> +
>>   static int io_close_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>>   {
>> -    if (sqe->off || sqe->addr || sqe->len || sqe->rw_flags || sqe->buf_index)
>> +    if (sqe->off || sqe->addr || sqe->len || sqe->buf_index)
>>           return -EINVAL;
>>       if (req->flags & REQ_F_FIXED_FILE)
>>           return -EBADF;
>>         req->close.fd = READ_ONCE(sqe->fd);
>>       req->close.file_slot = READ_ONCE(sqe->file_index);
>> -    if (req->close.file_slot && req->close.fd)
>> +    req->close.flags = READ_ONCE(sqe->close_flags);
>> +    if (req->close.flags & ~IORING_CLOSE_FD_AND_FILE_SLOT)
>> +        return -EINVAL;
>> +    if (!(req->close.flags & IORING_CLOSE_FD_AND_FILE_SLOT) &&
>> +        req->close.file_slot && req->close.fd)
>>           return -EINVAL;
>>         return 0;
>> @@ -5970,7 +5979,8 @@ static int io_close(struct io_kiocb *req, unsigned int issue_flags)
>>         if (req->close.file_slot) {
>>           ret = io_close_fixed(req, issue_flags);
>> -        goto err;
>> +        if (ret || !(req->close.flags & IORING_CLOSE_FD_AND_FILE_SLOT))
>> +            goto err;
>>       }
>>         spin_lock(&files->file_lock);
>> @@ -8003,6 +8013,42 @@ static int io_files_update_prep(struct io_kiocb *req,
>>       return 0;
>>   }
>>   +static int io_files_update_with_index_alloc(struct io_kiocb *req,
>> +                        unsigned int issue_flags)
>> +{
>> +    __s32 __user *fds = u64_to_user_ptr(req->rsrc_update.arg);
>> +    struct file *file;
>> +    unsigned int done, nr_fds = req->rsrc_update.nr_args;
>> +    int ret, fd;
>> +
>> +    for (done = 0; done < nr_fds; done++) {
>> +        if (copy_from_user(&fd, &fds[done], sizeof(fd))) {
>> +            ret = -EFAULT;
>> +            break;
>> +        }
>> +
>> +        file = fget(fd);
>> +        if (!file) {
>> +            ret = -EBADF;
>> +            goto out;
>> +        }
>> +        ret = io_fixed_fd_install(req, issue_flags, file,
>> +                      IORING_FILE_INDEX_ALLOC);
>> +        if (ret < 0)
>> +            goto out;
>> +        if (copy_to_user(&fds[done], &ret, sizeof(ret))) {
>> +            ret = -EFAULT;
>> +            __io_close_fixed(req, issue_flags, ret);
>> +            break;
>> +        }
>> +    }
>> +
>> +out:
>> +    if (done)
>> +        return done;
>> +    return ret;
>> +}
>> +
>>   static int io_files_update(struct io_kiocb *req, unsigned int issue_flags)
>>   {
>>       struct io_ring_ctx *ctx = req->ctx;
>> @@ -8016,10 +8062,14 @@ static int io_files_update(struct io_kiocb *req, unsigned int issue_flags)
>>       up.resv = 0;
>>       up.resv2 = 0;
>>   -    io_ring_submit_lock(ctx, issue_flags);
>> -    ret = __io_register_rsrc_update(ctx, IORING_RSRC_FILE,
>> -                    &up, req->rsrc_update.nr_args);
>> -    io_ring_submit_unlock(ctx, issue_flags);
>> +    if (req->rsrc_update.offset == IORING_FILE_INDEX_ALLOC) {
>> +        ret = io_files_update_with_index_alloc(req, issue_flags);
>> +    } else {
>> +        io_ring_submit_lock(ctx, issue_flags);
>> +        ret = __io_register_rsrc_update(ctx, IORING_RSRC_FILE,
>> +                &up, req->rsrc_update.nr_args);
>> +        io_ring_submit_unlock(ctx, issue_flags);
>> +    }
>>         if (ret < 0)
>>           req_set_fail(req);
>> @@ -10183,9 +10233,9 @@ static int io_install_fixed_file(struct io_kiocb *req, struct file *file,
>>       return ret;
>>   }
>>   -static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags)
>> +static int __io_close_fixed(struct io_kiocb *req, unsigned int issue_flags,
>> +                unsigned int offset)
>>   {
>> -    unsigned int offset = req->close.file_slot - 1;
>>       struct io_ring_ctx *ctx = req->ctx;
>>       struct io_fixed_file *file_slot;
>>       struct file *file;
>> @@ -10222,6 +10272,11 @@ static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags)
>>       return ret;
>>   }
>>   +static inline int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags)
>> +{
>> +    return __io_close_fixed(req, issue_flags, req->close.file_slot - 1);
>> +}
>> +
>>   static int __io_sqe_files_update(struct io_ring_ctx *ctx,
>>                    struct io_uring_rsrc_update2 *up,
>>                    unsigned nr_args)
>> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
>> index 53e7dae92e42..e347b3fea4e4 100644
>> --- a/include/uapi/linux/io_uring.h
>> +++ b/include/uapi/linux/io_uring.h
>> @@ -47,6 +47,7 @@ struct io_uring_sqe {
>>           __u32        unlink_flags;
>>           __u32        hardlink_flags;
>>           __u32        xattr_flags;
>> +        __u32        close_flags;
>>       };
>>       __u64    user_data;    /* data to be passed back at completion time */
>>       /* pack this to avoid bogus arm OABI complaints */
>

Re: [PATCH v2] io_uring: let IORING_OP_FILES_UPDATE support to choose fixed file slots

[Pavel Begunkov]

On 6/13/22 04:35, Xiaoguang Wang wrote:
> hello Pavel,
> 
>> On 5/30/22 14:15, 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 slots,
>>> which will store picked fixed files slots in fd array and let cqe return
>>> the number of slots allocated.
>>
>> Why close bits are piggybacked in this patch without any mention
>> in the commit message?
> Sorry, it actually should be in one separate patch.
> 
>> What is error semantics of
>> IORING_CLOSE_FD_AND_FILE_SLOT. Fail if any errored or both? How
>> can it be reliably used? Why we do two separate things in one
>> request with not clear semantics?
> Yeah, indeed I know this issue when writing this patch, either normal
> fd close or file slot close may fail independently.  I put them together
> just for efficiency, one CLOSE op can do two jobs.
> 
>>
>> There is already one fix pending. Another problem on the surface
>> is that it may call io_close() twice and the second will fail, e.g.
>>
>> -> io_close()
>> ---> close_fixed()
>> ---> if (file->flush) return -EAGAIN
>> -> io_close()
>> ---> close_fixed() // fails
> Thanks for pointing this issue.
> 
>>
>>
>> I do think we need to revert the close change, and then remake
>> properly and only if someone can describe sane semantics for it.
> Revert this close change seems reasonable, I thought for a while,
> didn't figure out better solutions yet.
> Sorry for the trouble again.

No worries, let's kill it off then and return it back if and when
someone comes up with a good semantaics for error handling.



> 
> Regards,
> Xiaoguang Wang
>>
>>
>>
>>> Suggested-by: Hao Xu <howeyxu@tencent.com>
>>> Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>
>>> ---
>>> V2:
>>>     Add illegal flags check in io_close_prep().
>>> ---
>>>    fs/io_uring.c                 | 75 +++++++++++++++++++++++++++++++++++++------
>>>    include/uapi/linux/io_uring.h |  1 +
>>>    2 files changed, 66 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>> index 6d91148e9679..18a7459fb6e7 100644
>>> --- a/fs/io_uring.c
>>> +++ b/fs/io_uring.c
>>> @@ -574,6 +574,7 @@ struct io_close {
>>>        struct file            *file;
>>>        int                fd;
>>>        u32                file_slot;
>>> +    u32                flags;
>>>    };
>>>      struct io_timeout_data {
>>> @@ -1366,7 +1367,9 @@ static int io_req_prep_async(struct io_kiocb *req);
>>>      static int io_install_fixed_file(struct io_kiocb *req, struct file *file,
>>>                     unsigned int issue_flags, u32 slot_index);
>>> -static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags);
>>> +static int __io_close_fixed(struct io_kiocb *req, unsigned int issue_flags,
>>> +                unsigned int offset);
>>> +static inline int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags);
>>>      static enum hrtimer_restart io_link_timeout_fn(struct hrtimer *timer);
>>>    static void io_eventfd_signal(struct io_ring_ctx *ctx);
>>> @@ -5945,16 +5948,22 @@ static int io_statx(struct io_kiocb *req, unsigned int issue_flags)
>>>        return 0;
>>>    }
>>>    +#define IORING_CLOSE_FD_AND_FILE_SLOT 1
>>> +
>>>    static int io_close_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>>>    {
>>> -    if (sqe->off || sqe->addr || sqe->len || sqe->rw_flags || sqe->buf_index)
>>> +    if (sqe->off || sqe->addr || sqe->len || sqe->buf_index)
>>>            return -EINVAL;
>>>        if (req->flags & REQ_F_FIXED_FILE)
>>>            return -EBADF;
>>>          req->close.fd = READ_ONCE(sqe->fd);
>>>        req->close.file_slot = READ_ONCE(sqe->file_index);
>>> -    if (req->close.file_slot && req->close.fd)
>>> +    req->close.flags = READ_ONCE(sqe->close_flags);
>>> +    if (req->close.flags & ~IORING_CLOSE_FD_AND_FILE_SLOT)
>>> +        return -EINVAL;
>>> +    if (!(req->close.flags & IORING_CLOSE_FD_AND_FILE_SLOT) &&
>>> +        req->close.file_slot && req->close.fd)
>>>            return -EINVAL;
>>>          return 0;
>>> @@ -5970,7 +5979,8 @@ static int io_close(struct io_kiocb *req, unsigned int issue_flags)
>>>          if (req->close.file_slot) {
>>>            ret = io_close_fixed(req, issue_flags);
>>> -        goto err;
>>> +        if (ret || !(req->close.flags & IORING_CLOSE_FD_AND_FILE_SLOT))
>>> +            goto err;
>>>        }
>>>          spin_lock(&files->file_lock);
>>> @@ -8003,6 +8013,42 @@ static int io_files_update_prep(struct io_kiocb *req,
>>>        return 0;
>>>    }
>>>    +static int io_files_update_with_index_alloc(struct io_kiocb *req,
>>> +                        unsigned int issue_flags)
>>> +{
>>> +    __s32 __user *fds = u64_to_user_ptr(req->rsrc_update.arg);
>>> +    struct file *file;
>>> +    unsigned int done, nr_fds = req->rsrc_update.nr_args;
>>> +    int ret, fd;
>>> +
>>> +    for (done = 0; done < nr_fds; done++) {
>>> +        if (copy_from_user(&fd, &fds[done], sizeof(fd))) {
>>> +            ret = -EFAULT;
>>> +            break;
>>> +        }
>>> +
>>> +        file = fget(fd);
>>> +        if (!file) {
>>> +            ret = -EBADF;
>>> +            goto out;
>>> +        }
>>> +        ret = io_fixed_fd_install(req, issue_flags, file,
>>> +                      IORING_FILE_INDEX_ALLOC);
>>> +        if (ret < 0)
>>> +            goto out;
>>> +        if (copy_to_user(&fds[done], &ret, sizeof(ret))) {
>>> +            ret = -EFAULT;
>>> +            __io_close_fixed(req, issue_flags, ret);
>>> +            break;
>>> +        }
>>> +    }
>>> +
>>> +out:
>>> +    if (done)
>>> +        return done;
>>> +    return ret;
>>> +}
>>> +
>>>    static int io_files_update(struct io_kiocb *req, unsigned int issue_flags)
>>>    {
>>>        struct io_ring_ctx *ctx = req->ctx;
>>> @@ -8016,10 +8062,14 @@ static int io_files_update(struct io_kiocb *req, unsigned int issue_flags)
>>>        up.resv = 0;
>>>        up.resv2 = 0;
>>>    -    io_ring_submit_lock(ctx, issue_flags);
>>> -    ret = __io_register_rsrc_update(ctx, IORING_RSRC_FILE,
>>> -                    &up, req->rsrc_update.nr_args);
>>> -    io_ring_submit_unlock(ctx, issue_flags);
>>> +    if (req->rsrc_update.offset == IORING_FILE_INDEX_ALLOC) {
>>> +        ret = io_files_update_with_index_alloc(req, issue_flags);
>>> +    } else {
>>> +        io_ring_submit_lock(ctx, issue_flags);
>>> +        ret = __io_register_rsrc_update(ctx, IORING_RSRC_FILE,
>>> +                &up, req->rsrc_update.nr_args);
>>> +        io_ring_submit_unlock(ctx, issue_flags);
>>> +    }
>>>          if (ret < 0)
>>>            req_set_fail(req);
>>> @@ -10183,9 +10233,9 @@ static int io_install_fixed_file(struct io_kiocb *req, struct file *file,
>>>        return ret;
>>>    }
>>>    -static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags)
>>> +static int __io_close_fixed(struct io_kiocb *req, unsigned int issue_flags,
>>> +                unsigned int offset)
>>>    {
>>> -    unsigned int offset = req->close.file_slot - 1;
>>>        struct io_ring_ctx *ctx = req->ctx;
>>>        struct io_fixed_file *file_slot;
>>>        struct file *file;
>>> @@ -10222,6 +10272,11 @@ static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags)
>>>        return ret;
>>>    }
>>>    +static inline int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags)
>>> +{
>>> +    return __io_close_fixed(req, issue_flags, req->close.file_slot - 1);
>>> +}
>>> +
>>>    static int __io_sqe_files_update(struct io_ring_ctx *ctx,
>>>                     struct io_uring_rsrc_update2 *up,
>>>                     unsigned nr_args)
>>> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
>>> index 53e7dae92e42..e347b3fea4e4 100644
>>> --- a/include/uapi/linux/io_uring.h
>>> +++ b/include/uapi/linux/io_uring.h
>>> @@ -47,6 +47,7 @@ struct io_uring_sqe {
>>>            __u32        unlink_flags;
>>>            __u32        hardlink_flags;
>>>            __u32        xattr_flags;
>>> +        __u32        close_flags;
>>>        };
>>>        __u64    user_data;    /* data to be passed back at completion time */
>>>        /* pack this to avoid bogus arm OABI complaints */
>>
> 

-- 
Pavel Begunkov