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@xxxxxxxxx>
> ---
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
>
