diff mbox series

[RFC,2/5] file: remove pointless wrapper

Message ID 20231130-vfs-files-fixes-v1-2-e73ca6f4ea83@kernel.org (mailing list archive)
State New, archived
Headers show
Series file: minor fixes | expand

Commit Message

Christian Brauner Nov. 30, 2023, 12:49 p.m. UTC 
Only io_uring uses __close_fd_get_file(). All it does is hide
current->files but io_uring accesses files_struct directly right now
anyway so it's a bit pointless. Just rename pick_file() to
file_close_fd_locked() and let io_uring use it. Add a lockdep assert in
there that we expect the caller to hold file_lock while we're at it.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/file.c            | 23 +++++++++--------------
 fs/internal.h        |  2 +-
 io_uring/openclose.c |  2 +-
 3 files changed, 11 insertions(+), 16 deletions(-)

Comments

Jens Axboe Nov. 30, 2023, 2:22 p.m. UTC | #1 
On 11/30/23 5:49 AM, Christian Brauner wrote:
> Only io_uring uses __close_fd_get_file(). All it does is hide
> current->files but io_uring accesses files_struct directly right now
> anyway so it's a bit pointless. Just rename pick_file() to
> file_close_fd_locked() and let io_uring use it. Add a lockdep assert in
> there that we expect the caller to hold file_lock while we're at it.

LGTM.

Reviewed-by: Jens Axboe <axboe@kernel.dk>
Jan Kara Nov. 30, 2023, 4:40 p.m. UTC | #2 
On Thu 30-11-23 13:49:08, Christian Brauner wrote:
> Only io_uring uses __close_fd_get_file(). All it does is hide
> current->files but io_uring accesses files_struct directly right now
> anyway so it's a bit pointless. Just rename pick_file() to
> file_close_fd_locked() and let io_uring use it. Add a lockdep assert in
> there that we expect the caller to hold file_lock while we're at it.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/file.c            | 23 +++++++++--------------
>  fs/internal.h        |  2 +-
>  io_uring/openclose.c |  2 +-
>  3 files changed, 11 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/file.c b/fs/file.c
> index 66f04442a384..c8eaa0b29a08 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -629,19 +629,23 @@ void fd_install(unsigned int fd, struct file *file)
>  EXPORT_SYMBOL(fd_install);
>  
>  /**
> - * pick_file - return file associatd with fd
> + * file_close_fd_locked - return file associated with fd
>   * @files: file struct to retrieve file from
>   * @fd: file descriptor to retrieve file for
>   *
> + * Doesn't take a separate reference count.
> + *
>   * Context: files_lock must be held.
>   *
>   * Returns: The file associated with @fd (NULL if @fd is not open)
>   */
> -static struct file *pick_file(struct files_struct *files, unsigned fd)
> +struct file *file_close_fd_locked(struct files_struct *files, unsigned fd)
>  {
>  	struct fdtable *fdt = files_fdtable(files);
>  	struct file *file;
>  
> +	lockdep_assert_held(&files->file_lock);
> +
>  	if (fd >= fdt->max_fds)
>  		return NULL;
>  
> @@ -660,7 +664,7 @@ int close_fd(unsigned fd)
>  	struct file *file;
>  
>  	spin_lock(&files->file_lock);
> -	file = pick_file(files, fd);
> +	file = file_close_fd_locked(files, fd);
>  	spin_unlock(&files->file_lock);
>  	if (!file)
>  		return -EBADF;
> @@ -707,7 +711,7 @@ static inline void __range_close(struct files_struct *files, unsigned int fd,
>  	max_fd = min(max_fd, n);
>  
>  	for (; fd <= max_fd; fd++) {
> -		file = pick_file(files, fd);
> +		file = file_close_fd_locked(files, fd);
>  		if (file) {
>  			spin_unlock(&files->file_lock);
>  			filp_close(file, files);
> @@ -795,15 +799,6 @@ int __close_range(unsigned fd, unsigned max_fd, unsigned int flags)
>  	return 0;
>  }
>  
> -/*
> - * See file_close_fd() below, this variant assumes current->files->file_lock
> - * is held.
> - */
> -struct file *__close_fd_get_file(unsigned int fd)
> -{
> -	return pick_file(current->files, fd);
> -}
> -
>  /**
>   * file_close_fd - return file associated with fd
>   * @fd: file descriptor to retrieve file for
> @@ -818,7 +813,7 @@ struct file *file_close_fd(unsigned int fd)
>  	struct file *file;
>  
>  	spin_lock(&files->file_lock);
> -	file = pick_file(files, fd);
> +	file = file_close_fd_locked(files, fd);
>  	spin_unlock(&files->file_lock);
>  
>  	return file;
> diff --git a/fs/internal.h b/fs/internal.h
> index 273e6fd40d1b..a7469ddba9b6 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -179,7 +179,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 struct file *__close_fd_get_file(unsigned int fd);
> +struct file *file_close_fd_locked(struct files_struct *files, unsigned 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/io_uring/openclose.c b/io_uring/openclose.c
> index fb73adb89067..74fc22461f48 100644
> --- a/io_uring/openclose.c
> +++ b/io_uring/openclose.c
> @@ -241,7 +241,7 @@ int io_close(struct io_kiocb *req, unsigned int issue_flags)
>  		return -EAGAIN;
>  	}
>  
> -	file = __close_fd_get_file(close->fd);
> +	file = file_close_fd_locked(files, close->fd);
>  	spin_unlock(&files->file_lock);
>  	if (!file)
>  		goto err;
> 
> -- 
> 2.42.0
>
diff mbox series

Patch

diff --git a/fs/file.c b/fs/file.c
index 66f04442a384..c8eaa0b29a08 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -629,19 +629,23 @@  void fd_install(unsigned int fd, struct file *file)
 EXPORT_SYMBOL(fd_install);
 
 /**
- * pick_file - return file associatd with fd
+ * file_close_fd_locked - return file associated with fd
  * @files: file struct to retrieve file from
  * @fd: file descriptor to retrieve file for
  *
+ * Doesn't take a separate reference count.
+ *
  * Context: files_lock must be held.
  *
  * Returns: The file associated with @fd (NULL if @fd is not open)
  */
-static struct file *pick_file(struct files_struct *files, unsigned fd)
+struct file *file_close_fd_locked(struct files_struct *files, unsigned fd)
 {
 	struct fdtable *fdt = files_fdtable(files);
 	struct file *file;
 
+	lockdep_assert_held(&files->file_lock);
+
 	if (fd >= fdt->max_fds)
 		return NULL;
 
@@ -660,7 +664,7 @@  int close_fd(unsigned fd)
 	struct file *file;
 
 	spin_lock(&files->file_lock);
-	file = pick_file(files, fd);
+	file = file_close_fd_locked(files, fd);
 	spin_unlock(&files->file_lock);
 	if (!file)
 		return -EBADF;
@@ -707,7 +711,7 @@  static inline void __range_close(struct files_struct *files, unsigned int fd,
 	max_fd = min(max_fd, n);
 
 	for (; fd <= max_fd; fd++) {
-		file = pick_file(files, fd);
+		file = file_close_fd_locked(files, fd);
 		if (file) {
 			spin_unlock(&files->file_lock);
 			filp_close(file, files);
@@ -795,15 +799,6 @@  int __close_range(unsigned fd, unsigned max_fd, unsigned int flags)
 	return 0;
 }
 
-/*
- * See file_close_fd() below, this variant assumes current->files->file_lock
- * is held.
- */
-struct file *__close_fd_get_file(unsigned int fd)
-{
-	return pick_file(current->files, fd);
-}
-
 /**
  * file_close_fd - return file associated with fd
  * @fd: file descriptor to retrieve file for
@@ -818,7 +813,7 @@  struct file *file_close_fd(unsigned int fd)
 	struct file *file;
 
 	spin_lock(&files->file_lock);
-	file = pick_file(files, fd);
+	file = file_close_fd_locked(files, fd);
 	spin_unlock(&files->file_lock);
 
 	return file;
diff --git a/fs/internal.h b/fs/internal.h
index 273e6fd40d1b..a7469ddba9b6 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -179,7 +179,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 struct file *__close_fd_get_file(unsigned int fd);
+struct file *file_close_fd_locked(struct files_struct *files, unsigned 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/io_uring/openclose.c b/io_uring/openclose.c
index fb73adb89067..74fc22461f48 100644
--- a/io_uring/openclose.c
+++ b/io_uring/openclose.c
@@ -241,7 +241,7 @@  int io_close(struct io_kiocb *req, unsigned int issue_flags)
 		return -EAGAIN;
 	}
 
-	file = __close_fd_get_file(close->fd);
+	file = file_close_fd_locked(files, close->fd);
 	spin_unlock(&files->file_lock);
 	if (!file)
 		goto err;