| Message ID | 20220530124654.22349-1-xiaoguang.wang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | io_uring: let IORING_OP_FILES_UPDATE support to choose fixed file slots | expand |
On 5/30/22 6:46 AM, Xiaoguang Wang wrote: > diff --git a/fs/io_uring.c b/fs/io_uring.c > index 6d91148e9679..58514b8048da 100644 > --- a/fs/io_uring.c > +++ b/fs/io_uring.c > @@ -5945,16 +5948,20 @@ 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); This needs a: if (req->closeflags & ~IORING_CLOSE_FD_AND_FILE_SLOT) return -EINVAL; to be future proof in terms of new flags. > + if (!(req->close.flags & IORING_CLOSE_FD_AND_FILE_SLOT) && > + req->close.file_slot && req->close.fd) > return -EINVAL; > > return 0; > @@ -5970,7 +5977,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,23 +8011,63 @@ 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; > struct io_uring_rsrc_update2 up; > 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.offset == IORING_FILE_INDEX_ALLOC) { > + ret = io_files_update_with_index_alloc(req, issue_flags); > + } else { > + up.offset = req->rsrc_update.offset; > + up.data = req->rsrc_update.arg; > + up.nr = 0; > + up.tags = 0; > + up.resv = 0; > + up.resv2 = 0; Move 'up' into this branch? Do you have a liburing test case for this as well?
hi, Forgot to add "Suggested-by: Hao Xu <howeyxu@tencent.com>". If this patch can be merged directly, please kindly add this, thanks. Regards, 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. > > Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com> > --- > fs/io_uring.c | 85 +++++++++++++++++++++++++++++++++++-------- > include/uapi/linux/io_uring.h | 1 + > 2 files changed, 70 insertions(+), 16 deletions(-) > > diff --git a/fs/io_uring.c b/fs/io_uring.c > index 6d91148e9679..58514b8048da 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,20 @@ 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) && > + req->close.file_slot && req->close.fd) > return -EINVAL; > > return 0; > @@ -5970,7 +5977,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,23 +8011,63 @@ 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; > struct io_uring_rsrc_update2 up; > 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.offset == IORING_FILE_INDEX_ALLOC) { > + ret = io_files_update_with_index_alloc(req, issue_flags); > + } else { > + up.offset = req->rsrc_update.offset; > + up.data = req->rsrc_update.arg; > + up.nr = 0; > + up.tags = 0; > + 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); > + 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 +10231,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 +10270,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 */
On 5/30/22 6:54 AM, Xiaoguang Wang wrote: > hi, > > Forgot to add "Suggested-by: Hao Xu <howeyxu@tencent.com>". If this > patch can be merged directly, please kindly add this, thanks. Please do a v2 with the flags check I mentioned, that's definitely required.
hi, Thanks for the very quick review :) > On 5/30/22 6:46 AM, Xiaoguang Wang wrote: >> diff --git a/fs/io_uring.c b/fs/io_uring.c >> index 6d91148e9679..58514b8048da 100644 >> --- a/fs/io_uring.c >> +++ b/fs/io_uring.c >> @@ -5945,16 +5948,20 @@ 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); > This needs a: > > if (req->closeflags & ~IORING_CLOSE_FD_AND_FILE_SLOT) > return -EINVAL; > > to be future proof in terms of new flags. OK. > >> + if (!(req->close.flags & IORING_CLOSE_FD_AND_FILE_SLOT) && >> + req->close.file_slot && req->close.fd) >> return -EINVAL; >> >> return 0; >> @@ -5970,7 +5977,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,23 +8011,63 @@ 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; >> struct io_uring_rsrc_update2 up; >> 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.offset == IORING_FILE_INDEX_ALLOC) { >> + ret = io_files_update_with_index_alloc(req, issue_flags); >> + } else { >> + up.offset = req->rsrc_update.offset; >> + up.data = req->rsrc_update.arg; >> + up.nr = 0; >> + up.tags = 0; >> + up.resv = 0; >> + up.resv2 = 0; > Move 'up' into this branch? OK. > > Do you have a liburing test case for this as well? Yeah, just sent it. Regards, Xiaoguang Wang >
diff --git a/fs/io_uring.c b/fs/io_uring.c index 6d91148e9679..58514b8048da 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,20 @@ 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) && + req->close.file_slot && req->close.fd) return -EINVAL; return 0; @@ -5970,7 +5977,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,23 +8011,63 @@ 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; struct io_uring_rsrc_update2 up; 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.offset == IORING_FILE_INDEX_ALLOC) { + ret = io_files_update_with_index_alloc(req, issue_flags); + } else { + up.offset = req->rsrc_update.offset; + up.data = req->rsrc_update.arg; + up.nr = 0; + up.tags = 0; + 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); + 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 +10231,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 +10270,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 */
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. Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com> --- fs/io_uring.c | 85 +++++++++++++++++++++++++++++++++++-------- include/uapi/linux/io_uring.h | 1 + 2 files changed, 70 insertions(+), 16 deletions(-)