Re: [PATCH v5] fs: avoid spurious dentry ref/unref cycle on open

[Mateusz Guzik]

any flames on this? it is ridiculous the problem remains imo

On Mon, Aug 3, 2026 at 2:51 PM Mateusz Guzik <mjguzik@xxxxxxxxx> wrote:
>
> Opening a file grabs a reference on the terminal dentry in
> __legitimize_path(), then another one in do_dentry_open() and finally
> drops the initial reference in terminate_walk().
>
> That's 2 modifications which don't need to be there -- do_dentry_open()
> can consume the already held reference instead.
>
> When benchmarking on a 20-core vm using will-it-scale to open the same
> file read-only, the results are (ops/s):
> before: 4043375
> after:  5629378 (+39%)
>
> Signed-off-by: Mateusz Guzik <mjguzik@xxxxxxxxx>
> ---
>
> The spurious ref cycle remains an issue and it is trivially avoidable,
> for the most common case anyway.
>
> Al Viro had a more involved patchset which got stalled, see:
> https://lore.kernel.org/linux-fsdevel/20240822003359.GO504335@ZenIV/
>
> I already pointed this out over a year ago when sending v3.
>
> Given lack of traffic on the more involved variant, the nice win from my
> simple patch and its overall triviality, I think it should go in. Worst
> case, if the more involved work ever gets off the ground it can be
> trivially reverted later.
>
> bench is:
> $ cat tests/openro3.c
>
> #include <stdlib.h>
> #include <unistd.h>
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <assert.h>
>
> static char tmpfile[] = "/tmp/willitscale.XXXXXX";
>
> char *testcase_description = "Same file open/close read-only";
>
> void testcase_prepare(unsigned long nr_tasks)
> {
>         int fd = mkstemp(tmpfile);
>
>         assert(fd >= 0);
>         close(fd);
> }
>
> void testcase(unsigned long long *iterations, unsigned long nr)
> {
>         while (1) {
>                 int fd = open(tmpfile, O_RDONLY);
>                 assert(fd >= 0);
>                 close(fd);
>
>                 (*iterations)++;
>         }
> }
>
> void testcase_cleanup(void)
> {
>         unlink(tmpfile);
> }
>
> v5:
> - the extra ref is of course needed, i blame the heatwave for thinking
>   it is not this time around
>
> v4:
> - rebase
> - don't grab the extra ref on mnt for truncate
> - bench opening things r/o. note perf improved from last year thanks to
>   other changes
>
>
>  fs/internal.h |  1 +
>  fs/namei.c    | 15 ++++++++++++---
>  fs/open.c     | 27 ++++++++++++++++++++++++++-
>  3 files changed, 39 insertions(+), 4 deletions(-)
>
> diff --git a/fs/internal.h b/fs/internal.h
> index c658c8a5ebd5..9632239036ac 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -205,6 +205,7 @@ int do_fchownat(int dfd, const char __user *filename, uid_t user, gid_t group,
>                 int flag);
>  int chown_common(const struct path *path, uid_t user, gid_t group);
>  extern int vfs_open(const struct path *, struct file *);
> +int vfs_open_consume(struct path *, struct file *);
>
>  /*
>   * inode.c
> diff --git a/fs/namei.c b/fs/namei.c
> index 3f9bf103ba12..f71481b9bf8f 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4789,6 +4789,7 @@ static const char *open_last_lookups(struct nameidata *nd,
>  static int do_open(struct nameidata *nd,
>                    struct file *file, const struct open_flags *op)
>  {
> +       struct vfsmount *mnt;
>         struct mnt_idmap *idmap;
>         int open_flag = op->open_flag;
>         bool do_truncate;
> @@ -4830,11 +4831,17 @@ static int do_open(struct nameidata *nd,
>                 error = mnt_want_write(nd->path.mnt);
>                 if (error)
>                         return error;
> +               /*
> +                * A dedicated reference is needed because after the call to
> +                * vfs_open_consume() we no longer own the reference in nd->path.mnt
> +                * while we need to undo write acess below.
> +                */
> +               mnt = mntget(nd->path.mnt);
>                 do_truncate = true;
>         }
>         error = may_open(idmap, &nd->path, acc_mode, open_flag);
>         if (!error && !(file->f_mode & FMODE_OPENED))
> -               error = vfs_open(&nd->path, file);
> +               error = vfs_open_consume(&nd->path, file);
>         if (!error)
>                 error = security_file_post_open(file, op->acc_mode);
>         if (!error && do_truncate)
> @@ -4843,8 +4850,10 @@ static int do_open(struct nameidata *nd,
>                 WARN_ON(1);
>                 error = -EINVAL;
>         }
> -       if (do_truncate)
> -               mnt_drop_write(nd->path.mnt);
> +       if (do_truncate) {
> +               mnt_drop_write(mnt);
> +               mntput(mnt);
> +       }
>         return error;
>  }
>
> diff --git a/fs/open.c b/fs/open.c
> index 6b1c14e684a9..2a7697cee00b 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -931,6 +931,11 @@ static inline int file_get_write_access(struct file *f)
>         return error;
>  }
>
> +/*
> + * Populate struct file
> + *
> + * NOTE: it assumes f_path is populated and consumes the caller's reference.
> + */
>  static int do_dentry_open(struct file *f,
>                           int (*open)(struct inode *, struct file *))
>  {
> @@ -938,7 +943,6 @@ static int do_dentry_open(struct file *f,
>         struct inode *inode = f->f_path.dentry->d_inode;
>         int error;
>
> -       path_get(&f->f_path);
>         f->f_inode = inode;
>         f->f_mapping = inode->i_mapping;
>         f->f_wb_err = filemap_sample_wb_err(f->f_mapping);
> @@ -1055,6 +1059,7 @@ int finish_open(struct file *file, struct dentry *dentry,
>         BUG_ON(file->f_mode & FMODE_OPENED); /* once it's opened, it's opened */
>
>         file->__f_path.dentry = dentry;
> +       path_get(&file->f_path);
>         return do_dentry_open(file, open);
>  }
>  EXPORT_SYMBOL(finish_open);
> @@ -1098,6 +1103,7 @@ int vfs_open(const struct path *path, struct file *file)
>         int ret;
>
>         file->__f_path = *path;
> +       path_get(&file->f_path);
>         ret = do_dentry_open(file, NULL);
>         if (!ret) {
>                 /*
> @@ -1110,6 +1116,25 @@ int vfs_open(const struct path *path, struct file *file)
>         return ret;
>  }
>
> +/**
> + * vfs_open_consume - open the file at the given path and consume the reference
> + * @path: path to open
> + * @file: newly allocated file with f_flag initialized
> + */
> +int vfs_open_consume(struct path *path, struct file *file)
> +{
> +       int ret;
> +
> +       file->__f_path = *path;
> +       path->mnt = NULL;
> +       path->dentry = NULL;
> +       ret = do_dentry_open(file, NULL);
> +       if (!ret) {
> +               fsnotify_open(file);
> +       }
> +       return ret;
> +}
> +
>  struct file *dentry_open(const struct path *path, int flags,
>                          const struct cred *cred)
>  {
> --
> 2.53.0
>