From 4329490a78b66ae44a9c93e433da375284162e3d Mon Sep 17 00:00:00 2001 From: Al Viro Date: Wed, 11 May 2022 20:30:20 -0400 Subject: io_uring_enter(): don't leave f.flags uninitialized simplifies logics on cleanup, as well... Reviewed-by: Jens Axboe Signed-off-by: Al Viro --- fs/io_uring.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/fs/io_uring.c b/fs/io_uring.c index a8413f006417..dc580a30723d 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -10961,14 +10961,14 @@ SYSCALL_DEFINE6(io_uring_enter, unsigned int, fd, u32, to_submit, return -EINVAL; fd = array_index_nospec(fd, IO_RINGFD_REG_MAX); f.file = tctx->registered_rings[fd]; - if (unlikely(!f.file)) - return -EBADF; + f.flags = 0; } else { f = fdget(fd); - if (unlikely(!f.file)) - return -EBADF; } + if (unlikely(!f.file)) + return -EBADF; + ret = -EOPNOTSUPP; if (unlikely(f.file->f_op != &io_uring_fops)) goto out_fput; @@ -11041,8 +11041,7 @@ SYSCALL_DEFINE6(io_uring_enter, unsigned int, fd, u32, to_submit, out: percpu_ref_put(&ctx->refs); out_fput: - if (!(flags & IORING_ENTER_REGISTERED_RING)) - fdput(f); + fdput(f); return submitted ? submitted : ret; } -- cgit v1.2.3-58-ga151 From 81132a39c152ca09832b9e4cb748129cee5f55ec Mon Sep 17 00:00:00 2001 From: Gou Hao Date: Tue, 2 Nov 2021 10:46:48 +0800 Subject: fs: remove fget_many and fput_many interface These two interface were added in 091141a42 commit, but now there is no place to call them. The only user of fput/fget_many() was removed in commit 62906e89e63b ("io_uring: remove file batch-get optimisation"). A user of get_file_rcu_many() were removed in commit f073531070d2 ("init: add an init_dup helper"). And replace atomic_long_sub/add to atomic_long_dec/inc can improve performance. Here are the test results of unixbench: Cmd: ./Run -c 64 context1 Without patch: System Benchmarks Partial Index BASELINE RESULT INDEX Pipe-based Context Switching 4000.0 2798407.0 6996.0 ======== System Benchmarks Index Score (Partial Only) 6996.0 With patch: System Benchmarks Partial Index BASELINE RESULT INDEX Pipe-based Context Switching 4000.0 3486268.8 8715.7 ======== System Benchmarks Index Score (Partial Only) 8715.7 Signed-off-by: Gou Hao Signed-off-by: Al Viro --- fs/file.c | 33 +++++++++++++-------------------- fs/file_table.c | 9 ++------- include/linux/file.h | 2 -- include/linux/fs.h | 4 +--- 4 files changed, 16 insertions(+), 32 deletions(-) diff --git a/fs/file.c b/fs/file.c index ee9317346702..4c59f410a48d 100644 --- a/fs/file.c +++ b/fs/file.c @@ -871,7 +871,7 @@ void do_close_on_exec(struct files_struct *files) } static inline struct file *__fget_files_rcu(struct files_struct *files, - unsigned int fd, fmode_t mask, unsigned int refs) + unsigned int fd, fmode_t mask) { for (;;) { struct file *file; @@ -897,10 +897,9 @@ static inline struct file *__fget_files_rcu(struct files_struct *files, * Such a race can take two forms: * * (a) the file ref already went down to zero, - * and get_file_rcu_many() fails. Just try - * again: + * and get_file_rcu() fails. Just try again: */ - if (unlikely(!get_file_rcu_many(file, refs))) + if (unlikely(!get_file_rcu(file))) continue; /* @@ -909,11 +908,11 @@ static inline struct file *__fget_files_rcu(struct files_struct *files, * pointer having changed, because it always goes * hand-in-hand with 'fdt'. * - * If so, we need to put our refs and try again. + * If so, we need to put our ref and try again. */ if (unlikely(rcu_dereference_raw(files->fdt) != fdt) || unlikely(rcu_dereference_raw(*fdentry) != file)) { - fput_many(file, refs); + fput(file); continue; } @@ -926,37 +925,31 @@ static inline struct file *__fget_files_rcu(struct files_struct *files, } static struct file *__fget_files(struct files_struct *files, unsigned int fd, - fmode_t mask, unsigned int refs) + fmode_t mask) { struct file *file; rcu_read_lock(); - file = __fget_files_rcu(files, fd, mask, refs); + file = __fget_files_rcu(files, fd, mask); rcu_read_unlock(); return file; } -static inline struct file *__fget(unsigned int fd, fmode_t mask, - unsigned int refs) -{ - return __fget_files(current->files, fd, mask, refs); -} - -struct file *fget_many(unsigned int fd, unsigned int refs) +static inline struct file *__fget(unsigned int fd, fmode_t mask) { - return __fget(fd, FMODE_PATH, refs); + return __fget_files(current->files, fd, mask); } struct file *fget(unsigned int fd) { - return __fget(fd, FMODE_PATH, 1); + return __fget(fd, FMODE_PATH); } EXPORT_SYMBOL(fget); struct file *fget_raw(unsigned int fd) { - return __fget(fd, 0, 1); + return __fget(fd, 0); } EXPORT_SYMBOL(fget_raw); @@ -966,7 +959,7 @@ struct file *fget_task(struct task_struct *task, unsigned int fd) task_lock(task); if (task->files) - file = __fget_files(task->files, fd, 0, 1); + file = __fget_files(task->files, fd, 0); task_unlock(task); return file; @@ -1035,7 +1028,7 @@ static unsigned long __fget_light(unsigned int fd, fmode_t mask) return 0; return (unsigned long)file; } else { - file = __fget(fd, mask, 1); + file = __fget(fd, mask); if (!file) return 0; return FDPUT_FPUT | (unsigned long)file; diff --git a/fs/file_table.c b/fs/file_table.c index 7d2e692b66a9..1ffd74bbbed6 100644 --- a/fs/file_table.c +++ b/fs/file_table.c @@ -368,9 +368,9 @@ EXPORT_SYMBOL_GPL(flush_delayed_fput); static DECLARE_DELAYED_WORK(delayed_fput_work, delayed_fput); -void fput_many(struct file *file, unsigned int refs) +void fput(struct file *file) { - if (atomic_long_sub_and_test(refs, &file->f_count)) { + if (atomic_long_dec_and_test(&file->f_count)) { struct task_struct *task = current; if (likely(!in_interrupt() && !(task->flags & PF_KTHREAD))) { @@ -389,11 +389,6 @@ void fput_many(struct file *file, unsigned int refs) } } -void fput(struct file *file) -{ - fput_many(file, 1); -} - /* * synchronous analog of fput(); for kernel threads that might be needed * in some umount() (and thus can't use flush_delayed_fput() without diff --git a/include/linux/file.h b/include/linux/file.h index 51e830b4fe3a..39704eae83e2 100644 --- a/include/linux/file.h +++ b/include/linux/file.h @@ -14,7 +14,6 @@ struct file; extern void fput(struct file *); -extern void fput_many(struct file *, unsigned int); struct file_operations; struct task_struct; @@ -47,7 +46,6 @@ static inline void fdput(struct fd fd) } extern struct file *fget(unsigned int fd); -extern struct file *fget_many(unsigned int fd, unsigned int refs); extern struct file *fget_raw(unsigned int fd); extern struct file *fget_task(struct task_struct *task, unsigned int fd); extern unsigned long __fdget(unsigned int fd); diff --git a/include/linux/fs.h b/include/linux/fs.h index bbde95387a23..3660c338bb16 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -981,9 +981,7 @@ static inline struct file *get_file(struct file *f) atomic_long_inc(&f->f_count); return f; } -#define get_file_rcu_many(x, cnt) \ - atomic_long_add_unless(&(x)->f_count, (cnt), 0) -#define get_file_rcu(x) get_file_rcu_many((x), 1) +#define get_file_rcu(x) atomic_long_inc_not_zero(&(x)->f_count) #define file_count(x) atomic_long_read(&(x)->f_count) #define MAX_NON_LFS ((1UL<<31) - 1) -- cgit v1.2.3-58-ga151 From 6319194ec57b0452dcda4589d24c4e7db299c5bf Mon Sep 17 00:00:00 2001 From: Al Viro Date: Thu, 12 May 2022 17:08:03 -0400 Subject: Unify the primitives for file descriptor closing Currently we have 3 primitives for removing an opened file from descriptor table - pick_file(), __close_fd_get_file() and close_fd_get_file(). Their calling conventions are rather odd and there's a code duplication for no good reason. They can be unified - 1) have __range_close() cap max_fd in the very beginning; that way we don't need separate way for pick_file() to report being past the end of descriptor table. 2) make {__,}close_fd_get_file() return file (or NULL) directly, rather than returning it via struct file ** argument. Don't bother with (bogus) return value - nobody wants that -ENOENT. 3) make pick_file() return NULL on unopened descriptor - the only caller that used to care about the distinction between descriptor past the end of descriptor table and finding NULL in descriptor table doesn't give a damn after (1). 4) lift ->files_lock out of pick_file() That actually simplifies the callers, as well as the primitives themselves. Code duplication is also gone... Reviewed-by: Christian Brauner (Microsoft) Signed-off-by: Al Viro --- drivers/android/binder.c | 2 +- fs/file.c | 77 ++++++++++++++++++------------------------------ fs/internal.h | 2 +- fs/io_uring.c | 7 ++--- include/linux/fdtable.h | 2 +- 5 files changed, 34 insertions(+), 56 deletions(-) diff --git a/drivers/android/binder.c b/drivers/android/binder.c index 8351c5638880..27c9b004823a 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -1855,7 +1855,7 @@ static void binder_deferred_fd_close(int fd) if (!twcb) return; init_task_work(&twcb->twork, binder_do_fd_close); - close_fd_get_file(fd, &twcb->file); + twcb->file = close_fd_get_file(fd); if (twcb->file) { filp_close(twcb->file, current->files); task_work_add(current, &twcb->twork, TWA_RESUME); diff --git a/fs/file.c b/fs/file.c index 4c59f410a48d..dd6692048f4f 100644 --- a/fs/file.c +++ b/fs/file.c @@ -630,32 +630,23 @@ EXPORT_SYMBOL(fd_install); * @files: file struct to retrieve file from * @fd: file descriptor to retrieve file for * - * If this functions returns an EINVAL error pointer the fd was beyond the - * current maximum number of file descriptors for that fdtable. + * Context: files_lock must be held. * - * Returns: The file associated with @fd, on error returns an error pointer. + * Returns: The file associated with @fd (NULL if @fd is not open) */ static struct file *pick_file(struct files_struct *files, unsigned fd) { + struct fdtable *fdt = files_fdtable(files); struct file *file; - struct fdtable *fdt; - spin_lock(&files->file_lock); - fdt = files_fdtable(files); - if (fd >= fdt->max_fds) { - file = ERR_PTR(-EINVAL); - goto out_unlock; - } + if (fd >= fdt->max_fds) + return NULL; + file = fdt->fd[fd]; - if (!file) { - file = ERR_PTR(-EBADF); - goto out_unlock; + if (file) { + rcu_assign_pointer(fdt->fd[fd], NULL); + __put_unused_fd(files, fd); } - rcu_assign_pointer(fdt->fd[fd], NULL); - __put_unused_fd(files, fd); - -out_unlock: - spin_unlock(&files->file_lock); return file; } @@ -664,8 +655,10 @@ int close_fd(unsigned fd) struct files_struct *files = current->files; struct file *file; + spin_lock(&files->file_lock); file = pick_file(files, fd); - if (IS_ERR(file)) + spin_unlock(&files->file_lock); + if (!file) return -EBADF; return filp_close(file, files); @@ -702,20 +695,25 @@ static inline void __range_cloexec(struct files_struct *cur_fds, static inline void __range_close(struct files_struct *cur_fds, unsigned int fd, unsigned int max_fd) { + unsigned n; + + rcu_read_lock(); + n = last_fd(files_fdtable(cur_fds)); + rcu_read_unlock(); + max_fd = min(max_fd, n); + while (fd <= max_fd) { struct file *file; + spin_lock(&cur_fds->file_lock); file = pick_file(cur_fds, fd++); - if (!IS_ERR(file)) { + spin_unlock(&cur_fds->file_lock); + + if (file) { /* found a valid file to close */ filp_close(file, cur_fds); cond_resched(); - continue; } - - /* beyond the last fd in that table */ - if (PTR_ERR(file) == -EINVAL) - return; } } @@ -795,26 +793,9 @@ int __close_range(unsigned fd, unsigned max_fd, unsigned int flags) * See close_fd_get_file() below, this variant assumes current->files->file_lock * is held. */ -int __close_fd_get_file(unsigned int fd, struct file **res) +struct file *__close_fd_get_file(unsigned int fd) { - struct files_struct *files = current->files; - struct file *file; - struct fdtable *fdt; - - fdt = files_fdtable(files); - if (fd >= fdt->max_fds) - goto out_err; - file = fdt->fd[fd]; - if (!file) - goto out_err; - rcu_assign_pointer(fdt->fd[fd], NULL); - __put_unused_fd(files, fd); - get_file(file); - *res = file; - return 0; -out_err: - *res = NULL; - return -ENOENT; + return pick_file(current->files, fd); } /* @@ -822,16 +803,16 @@ out_err: * The caller must ensure that filp_close() called on the file, and then * an fput(). */ -int close_fd_get_file(unsigned int fd, struct file **res) +struct file *close_fd_get_file(unsigned int fd) { struct files_struct *files = current->files; - int ret; + struct file *file; spin_lock(&files->file_lock); - ret = __close_fd_get_file(fd, res); + file = pick_file(files, fd); spin_unlock(&files->file_lock); - return ret; + return file; } void do_close_on_exec(struct files_struct *files) diff --git a/fs/internal.h b/fs/internal.h index 08503dc68d2b..4065e2679103 100644 --- a/fs/internal.h +++ b/fs/internal.h @@ -125,7 +125,7 @@ extern struct file *do_file_open_root(const struct path *, const char *, const struct open_flags *); extern struct open_how build_open_how(int flags, umode_t mode); extern int build_open_flags(const struct open_how *how, struct open_flags *op); -extern int __close_fd_get_file(unsigned int fd, struct file **res); +extern struct file *__close_fd_get_file(unsigned int fd); long do_sys_ftruncate(unsigned int fd, loff_t length, int small); int chmod_common(const struct path *path, umode_t mode); diff --git a/fs/io_uring.c b/fs/io_uring.c index dc580a30723d..7257b0870353 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -5137,13 +5137,10 @@ static int io_close(struct io_kiocb *req, unsigned int issue_flags) return -EAGAIN; } - ret = __close_fd_get_file(close->fd, &file); + file = __close_fd_get_file(close->fd); spin_unlock(&files->file_lock); - if (ret < 0) { - if (ret == -ENOENT) - ret = -EBADF; + if (!file) goto err; - } /* No ->flush() or already async, safely close from here */ ret = filp_close(file, current->files); diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h index d0e78174874a..e066816f3519 100644 --- a/include/linux/fdtable.h +++ b/include/linux/fdtable.h @@ -125,7 +125,7 @@ int iterate_fd(struct files_struct *, unsigned, extern int close_fd(unsigned int fd); extern int __close_range(unsigned int fd, unsigned int max_fd, unsigned int flags); -extern int close_fd_get_file(unsigned int fd, struct file **res); +extern struct file *close_fd_get_file(unsigned int fd); extern int unshare_fd(unsigned long unshare_flags, unsigned int max_fds, struct files_struct **new_fdp); -- cgit v1.2.3-58-ga151