Re: [PATCH 16/26] fs: add fget_many() and fput_many()

[Christoph Hellwig <hch@lst.de>]

[adding Al, I think we really want him to review anything fget
 related]

On Thu, Dec 13, 2018 at 10:56:35AM -0700, Jens Axboe wrote:
> Some uses cases repeatedly get and put references to the same file, but
> the only exposed interface is doing these one at the time. As each of
> these entail an atomic inc or dec on a shared structure, that cost can
> add up.
> 
> Add fget_many(), which works just like fget(), except it takes an
> argument for how many references to get on the file. Ditto fput_many(),
> which can drop an arbitrary number of references to a file.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>  fs/file.c            | 15 ++++++++++-----
>  fs/file_table.c      | 10 ++++++++--
>  include/linux/file.h |  2 ++
>  include/linux/fs.h   |  3 ++-
>  4 files changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/file.c b/fs/file.c
> index 7ffd6e9d103d..ad9870edfd51 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -676,7 +676,7 @@ void do_close_on_exec(struct files_struct *files)
>  	spin_unlock(&files->file_lock);
>  }
>  
> -static struct file *__fget(unsigned int fd, fmode_t mask)
> +static struct file *__fget(unsigned int fd, fmode_t mask, unsigned int refs)
>  {
>  	struct files_struct *files = current->files;
>  	struct file *file;
> @@ -691,7 +691,7 @@ static struct file *__fget(unsigned int fd, fmode_t mask)
>  		 */
>  		if (file->f_mode & mask)
>  			file = NULL;
> -		else if (!get_file_rcu(file))
> +		else if (!get_file_rcu_many(file, refs))
>  			goto loop;
>  	}
>  	rcu_read_unlock();
> @@ -699,15 +699,20 @@ static struct file *__fget(unsigned int fd, fmode_t mask)
>  	return file;
>  }
>  
> +struct file *fget_many(unsigned int fd, unsigned int refs)
> +{
> +	return __fget(fd, FMODE_PATH, refs);
> +}
> +
>  struct file *fget(unsigned int fd)
>  {
> -	return __fget(fd, FMODE_PATH);
> +	return fget_many(fd, 1);

Can we just call __fget directly here?  That is a little easier
to follow, and might actually generate better code if the compiler
is inliner challenged (which they often seem to be).

> +void fput(struct file *file)
> +{
> +	fput_many(file, 1);
> +}
> +
> +

Double empty line here.

> +#define get_file_rcu_many(x, cnt) atomic_long_add_unless(&(x)->f_count, (cnt), 0)

This could use a line break to be easier readable and not spill over
80 chars.  Otherwise this looks fine to me.