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@xxxxxxxxxxx>
>> Signed-off-by: Xiaoguang Wang <xiaoguang.wang@xxxxxxxxxxxxxxxxx>
>> ---
>> 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 */
>
