Re: [PATCH v2 1/4] file: add fput and file_ref_put routines optimized for use when closing a fd

[Christian Brauner <brauner@kernel.org>]

On Tue, Mar 04, 2025 at 07:35:03PM +0100, Mateusz Guzik wrote:
> Vast majority of the time closing a file descriptor also operates on the
> last reference, where a regular fput usage will result in 2 atomics.
> This can be changed to only suffer 1.
> 
> See commentary above file_ref_put_close() for more information.
> 
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
> ---

I'm not enthused about the patches tbh because we end up with two new
primivites and I really dislike so many new primitives with slightly
different semantics. But it should at least all be kept private to fs/.

>  fs/file.c                | 75 ++++++++++++++++++++++++++++++----------
>  fs/file_table.c          | 72 +++++++++++++++++++++++++++-----------
>  include/linux/file.h     |  2 ++
>  include/linux/file_ref.h |  1 +
>  4 files changed, 111 insertions(+), 39 deletions(-)
> 
> diff --git a/fs/file.c b/fs/file.c
> index 44efdc8c1e27..ea753f9c8e08 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -26,6 +26,28 @@
>  
>  #include "internal.h"
>  
> +static bool __file_ref_put_badval(file_ref_t *ref, unsigned long cnt)
> +{
> +	/*
> +	 * If the reference count was already in the dead zone, then this
> +	 * put() operation is imbalanced. Warn, put the reference count back to
> +	 * DEAD and tell the caller to not deconstruct the object.
> +	 */
> +	if (WARN_ONCE(cnt >= FILE_REF_RELEASED, "imbalanced put on file reference count")) {
> +		atomic_long_set(&ref->refcnt, FILE_REF_DEAD);
> +		return false;
> +	}
> +
> +	/*
> +	 * This is a put() operation on a saturated refcount. Restore the
> +	 * mean saturation value and tell the caller to not deconstruct the
> +	 * object.
> +	 */
> +	if (cnt > FILE_REF_MAXREF)
> +		atomic_long_set(&ref->refcnt, FILE_REF_SATURATED);
> +	return false;
> +}
> +
>  /**
>   * __file_ref_put - Slowpath of file_ref_put()
>   * @ref:	Pointer to the reference count
> @@ -67,27 +89,44 @@ bool __file_ref_put(file_ref_t *ref, unsigned long cnt)
>  		return true;
>  	}
>  
> -	/*
> -	 * If the reference count was already in the dead zone, then this
> -	 * put() operation is imbalanced. Warn, put the reference count back to
> -	 * DEAD and tell the caller to not deconstruct the object.
> -	 */
> -	if (WARN_ONCE(cnt >= FILE_REF_RELEASED, "imbalanced put on file reference count")) {
> -		atomic_long_set(&ref->refcnt, FILE_REF_DEAD);
> -		return false;
> -	}
> -
> -	/*
> -	 * This is a put() operation on a saturated refcount. Restore the
> -	 * mean saturation value and tell the caller to not deconstruct the
> -	 * object.
> -	 */
> -	if (cnt > FILE_REF_MAXREF)
> -		atomic_long_set(&ref->refcnt, FILE_REF_SATURATED);
> -	return false;
> +	return __file_ref_put_badval(ref, cnt);
>  }
>  EXPORT_SYMBOL_GPL(__file_ref_put);
>  
> +/**
> + * file_ref_put_close - drop a reference expecting it would transition to FILE_REF_NOREF
> + * @ref:	Pointer to the reference count
> + *
> + * Semantically it is equivalent to calling file_ref_put(), but it trades lower
> + * performance in face of other CPUs also modifying the refcount for higher
> + * performance when this happens to be the last reference.
> + *
> + * For the last reference file_ref_put() issues 2 atomics. One to drop the
> + * reference and another to transition it to FILE_REF_DEAD. This routine does
> + * the work in one step, but in order to do it has to pre-read the variable which
> + * decreases scalability.
> + *
> + * Use with close() et al, stick to file_ref_put() by default.
> + */
> +bool file_ref_put_close(file_ref_t *ref)
> +{
> +	long old, new;
> +
> +	old = atomic_long_read(&ref->refcnt);
> +	do {
> +		if (unlikely(old < 0))
> +			return __file_ref_put_badval(ref, old);
> +
> +		if (old == FILE_REF_ONEREF)
> +			new = FILE_REF_DEAD;
> +		else
> +			new = old - 1;
> +	} while (!atomic_long_try_cmpxchg(&ref->refcnt, &old, new));
> +
> +	return new == FILE_REF_DEAD;
> +}
> +EXPORT_SYMBOL_GPL(file_ref_put_close);
> +
>  unsigned int sysctl_nr_open __read_mostly = 1024*1024;
>  unsigned int sysctl_nr_open_min = BITS_PER_LONG;
>  /* our min() is unusable in constant expressions ;-/ */
> diff --git a/fs/file_table.c b/fs/file_table.c
> index 5c00dc38558d..4189c682eb06 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -511,31 +511,37 @@ void flush_delayed_fput(void)
>  }
>  EXPORT_SYMBOL_GPL(flush_delayed_fput);
>  
> -void fput(struct file *file)
> +static void __fput_defer_free(struct file *file)

Imho just call it __fput_deferred().

>  {
> -	if (file_ref_put(&file->f_ref)) {
> -		struct task_struct *task = current;
> +	struct task_struct *task = current;
>  
> -		if (unlikely(!(file->f_mode & (FMODE_BACKING | FMODE_OPENED)))) {
> -			file_free(file);
> +	if (unlikely(!(file->f_mode & (FMODE_BACKING | FMODE_OPENED)))) {
> +		file_free(file);
> +		return;
> +	}
> +	if (likely(!in_interrupt() && !(task->flags & PF_KTHREAD))) {
> +		init_task_work(&file->f_task_work, ____fput);
> +		if (!task_work_add(task, &file->f_task_work, TWA_RESUME))
>  			return;
> -		}
> -		if (likely(!in_interrupt() && !(task->flags & PF_KTHREAD))) {
> -			init_task_work(&file->f_task_work, ____fput);
> -			if (!task_work_add(task, &file->f_task_work, TWA_RESUME))
> -				return;
> -			/*
> -			 * After this task has run exit_task_work(),
> -			 * task_work_add() will fail.  Fall through to delayed
> -			 * fput to avoid leaking *file.
> -			 */
> -		}
> -
> -		if (llist_add(&file->f_llist, &delayed_fput_list))
> -			schedule_delayed_work(&delayed_fput_work, 1);
> +		/*
> +		 * After this task has run exit_task_work(),
> +		 * task_work_add() will fail.  Fall through to delayed
> +		 * fput to avoid leaking *file.
> +		 */
>  	}
> +
> +	if (llist_add(&file->f_llist, &delayed_fput_list))
> +		schedule_delayed_work(&delayed_fput_work, 1);
>  }
>  
> +void fput(struct file *file)
> +{
> +	if (unlikely(file_ref_put(&file->f_ref))) {
> +		__fput_defer_free(file);
> +	}
> +}
> +EXPORT_SYMBOL(fput);
> +
>  /*
>   * synchronous analog of fput(); for kernel threads that might be needed
>   * in some umount() (and thus can't use flush_delayed_fput() without
> @@ -549,10 +555,34 @@ void __fput_sync(struct file *file)
>  	if (file_ref_put(&file->f_ref))
>  		__fput(file);
>  }
> -
> -EXPORT_SYMBOL(fput);
>  EXPORT_SYMBOL(__fput_sync);
>  
> +/*
> + * Equivalent to __fput_sync(), but optimized for being called with the last
> + * reference.
> + *
> + * See file_ref_put_close() for details.
> + */
> +void fput_close_sync(struct file *file)
> +{
> +	if (unlikely(file_ref_put_close(&file->f_ref)))
> +		__fput(file);
> +}
> +EXPORT_SYMBOL(fput_close_sync);

Shouldn't be exported to modules, please.

> +
> +/*
> + * Equivalent to fput(), but optimized for being called with the last
> + * reference.
> + *
> + * See file_ref_put_close() for details.
> + */
> +void fput_close(struct file *file)
> +{
> +	if (file_ref_put_close(&file->f_ref))
> +		__fput_defer_free(file);
> +}
> +EXPORT_SYMBOL(fput_close);

Shouldn't be exported to modules, please.

> +
>  void __init files_init(void)
>  {
>  	struct kmem_cache_args args = {
> diff --git a/include/linux/file.h b/include/linux/file.h
> index 302f11355b10..7b04e87cbde6 100644
> --- a/include/linux/file.h
> +++ b/include/linux/file.h
> @@ -124,6 +124,8 @@ int receive_fd_replace(int new_fd, struct file *file, unsigned int o_flags);
>  
>  extern void flush_delayed_fput(void);
>  extern void __fput_sync(struct file *);
> +void fput_close_sync(struct file *);
> +void fput_close(struct file *);

Should go into internal.h.

>  
>  extern unsigned int sysctl_nr_open_min, sysctl_nr_open_max;
>  
> diff --git a/include/linux/file_ref.h b/include/linux/file_ref.h
> index 9b3a8d9b17ab..f269299941aa 100644
> --- a/include/linux/file_ref.h
> +++ b/include/linux/file_ref.h
> @@ -62,6 +62,7 @@ static inline void file_ref_init(file_ref_t *ref, unsigned long cnt)
>  }
>  
>  bool __file_ref_put(file_ref_t *ref, unsigned long cnt);
> +bool file_ref_put_close(file_ref_t *ref);
>  
>  /**
>   * file_ref_get - Acquire one reference on a file
> -- 
> 2.43.0
>