From: Pavel Begunkov <[email protected]>
To: Xiaoguang Wang <[email protected]>,
[email protected]
Cc: [email protected]
Subject: Re: [PATCH v2] io_uring: let IORING_OP_FILES_UPDATE support to choose fixed file slots
Date: Tue, 14 Jun 2022 17:41:53 +0100 [thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
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 <[email protected]>
>>> Signed-off-by: Xiaoguang Wang <[email protected]>
>>> ---
>>> 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
prev parent reply other threads:[~2022-06-14 16:42 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-30 13:15 [PATCH v2] io_uring: let IORING_OP_FILES_UPDATE support to choose fixed file slots Xiaoguang Wang
2022-05-30 13:18 ` Jens Axboe
2022-05-30 13:45 ` Jens Axboe
2022-05-30 16:15 ` Xiaoguang Wang
2022-05-30 16:28 ` Jens Axboe
2022-05-30 16:34 ` Xiaoguang Wang
2022-06-12 17:41 ` Pavel Begunkov
2022-06-13 3:35 ` Xiaoguang Wang
2022-06-14 16:41 ` Pavel Begunkov [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
[email protected] \
[email protected] \
[email protected] \
[email protected] \
[email protected] \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox