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

[Mateusz Guzik]

On Mon, Aug 31, 2026 at 12:05 AM Jori Koolstra <jkoolstra@xxxxxxxxx> wrote:
>
> On Mon, Aug 03, 2026 at 02:51:38PM +0200, Mateusz Guzik 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>
>
> So, the legitimize path stuff grabs a ref to the nd path which is
> balanced at terminate_walk(), and you're saying that in the regular open
> path (not O_TMPFILE or O_PATH), since vfs_open() also grabs a ref, why
> not reuse that one. Is that what you are saying?
>
> It seems then that this issue also plagues do_o_path():
>
> static int do_o_path(struct nameidata *nd, unsigned flags, struct file *file)
> {
>         struct path path;
>         int error = path_lookupat(nd, flags, &path);
>         if (!error) {
>                 audit_inode(nd->name, path.dentry, 0);
>                 error = vfs_open(&path, file);
>                 path_put(&path);                        // Not needed?
>         }
>         return error;
>

Indeed. I did not bother looking for other uses. This can be sorted
out separately if/when my patch lands or I can add it.

>
> > ---
> >
> > 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);
>
> Is this needed? The file cannot be closed until after open(2) is done,
> so how can that ref be lost before reaching mnt_drop_write below?
>

do_dentry_open() whacks the ref on failure and I intentionally did not
change that to keep the patch small

> >               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
> >
>
> Thanks,
> Jori.