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>
---
 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)
 {
-	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);
+
+/*
+ * 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);
+
 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 *);
 
 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

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
>

On Wed, Mar 5, 2025 at 11:47 AM Christian Brauner <brauner@kernel.org> wrote:
>
> 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/.
>

Caller-observable behavior is the same, so I don't think this is a big
deal. The intent was to keep this very much for vfs-internal usage.
The EXPORT thing was copy-pasto.

Anyhow, everyone interested can still just fput so this does not put
any burden on other people afaics. The few patched spots are very much
vfs-internal.

I sent v3 with the feedback addressed.

-- 
Mateusz Guzik <mjguzik gmail.com>

On Tue, Mar 4, 2025 at 7:35 PM Mateusz Guzik <mjguzik@gmail.com> wrote:

> +void fput_close_sync(struct file *file)
> +{
> +       if (unlikely(file_ref_put_close(&file->f_ref)))
> +               __fput(file);
> +}

this of course should be: *likely*

-- 
Mateusz Guzik <mjguzik gmail.com>