Message-ID: <CAGudoHGMF=nt=Dr+0UDVOsd4nfGRr4xC8=oeQqs=Av9s0tXXXA@mail.gmail.com>
Date: Wed, 7 Aug 2024 09:22:59 +0200
From: Mateusz Guzik <mjguzik@...il.com>
To: Al Viro <viro@...iv.linux.org.uk>
Cc: brauner@...nel.org, jack@...e.cz, linux-kernel@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] vfs: avoid spurious dentry ref/unref cycle on open

On Wed, Aug 7, 2024 at 9:05 AM Al Viro <viro@...iv.linux.org.uk> wrote:
>
> On Wed, Aug 07, 2024 at 08:40:28AM +0200, Mateusz Guzik wrote:
> > On Wed, Aug 7, 2024 at 8:33 AM Al Viro <viro@...iv.linux.org.uk> wrote:
> > >
> > > On Wed, Aug 07, 2024 at 07:23:00AM +0100, Al Viro wrote:
> > > >       After having looked at the problem, how about the following
> > > > series:
> > > >
> > > > 1/5) lift path_get() *AND* path_put() out of do_dentry_open()
> > > > into the callers.  The latter - conditional upon "do_dentry_open()
> > > > has not set FMODE_OPENED".  Equivalent transformation.
> > > >
> > > > 2/5) move path_get() we'd lifted into the callers past the
> > > > call of do_dentry_open(), conditionally collapse it with path_put().
> > > > You'd get e.g.
> > > > int vfs_open(const struct path *path, struct file *file)
> > > > {
> > > >         int ret;
> > > >
> > > >         file->f_path = *path;
> > > >         ret = do_dentry_open(file, NULL);
> > > >         if (!ret) {
> > > >                 /*
> > > >                  * Once we return a file with FMODE_OPENED, __fput() will call
> > > >                  * fsnotify_close(), so we need fsnotify_open() here for
> > > >                  * symmetry.
> > > >                  */
> > > >                 fsnotify_open(file);
> > > >         }
> > > >       if (file->f_mode & FMODE_OPENED)
> > > >               path_get(path);
> > > >         return ret;
> > > > }
> > > >
> > > > Equivalent transformation, provided that nobody is playing silly
> > > > buggers with reassigning ->f_path in their ->open() instances.
> > > > They *really* should not - if anyone does, we'd better catch them
> > > > and fix them^Wtheir code.  Incidentally, if we find any such,
> > > > we have a damn good reason to add asserts in the callers.  As
> > > > in, "if do_dentry_open() has set FMODE_OPENED, it would bloody
> > > > better *not* modify ->f_path".  <greps> Nope, nobody is that
> > > > insane.
> > > >
> > > > 3/5) split vfs_open_consume() out of vfs_open() (possibly
> > > > named vfs_open_borrow()), replace the call in do_open() with
> > > > calling the new function.
> > > >
> > > > Trivially equivalent transformation.
> > > >
> > > > 4/5) Remove conditional path_get() from vfs_open_consume()
> > > > and finish_open().  Add
> > > >               if (file->f_mode & FMODE_OPENED)
> > > >                       path_get(&nd->path);
> > > > before terminate_walk(nd); in path_openat().
> > > >
> > > > Equivalent transformation - see
> > > >         if (file->f_mode & (FMODE_OPENED | FMODE_CREATED)) {
> > > >                 dput(nd->path.dentry);
> > > >                 nd->path.dentry = dentry;
> > > >                 return NULL;
> > > >         }
> > > > in lookup_open() (which is where nd->path gets in sync with what
> > > > had been given to do_dentry_open() in finish_open()); in case
> > > > of vfs_open_consume() in do_open() it's in sync from the very
> > > > beginning.  And we never modify nd->path after those points.
> > > > So we can move grabbing it downstream, keeping it under the
> > > > same condition (which also happens to be true only if we'd
> > > > called do_dentry_open(), so for all other paths through the
> > > > whole thing it's a no-op.
> > > >
> > > > 5/5) replace
> > > >               if (file->f_mode & FMODE_OPENED)
> > > >                       path_get(&nd->path);
> > > >               terminate_walk(nd);
> > > > with
> > > >               if (file->f_mode & FMODE_OPENED) {
> > > >                       nd->path.mnt = NULL;
> > > >                       nd->path.dentry = NULL;
> > > >               }
> > > >               terminate_walk(nd);
> > > > Again, an obvious equivalent transformation.
> > >
> > > BTW, similar to that, with that we could turn do_o_path()
> > > into
> > >
> > >         struct path path;
> > >         int error = path_lookupat(nd, flags, &path);
> > >         if (!error) {
> > >                 audit_inode(nd->name, path.dentry, 0);
> > >                 error = vfs_open_borrow(&path, file);
> > >                 if (!(file->f_mode & FMODE_OPENED))
> > >                         path_put(&path);
> > >         }
> > >         return error;
> > > }
> > >
> > > and perhaps do something similar in the vicinity of
> > > vfs_tmpfile() / do_o_tmpfile().
> >
> > That's quite a bit of churn, but if you insist I can take a stab.
>
> What I have in mind is something along the lines of COMPLETELY UNTESTED
> git.kernel.org:/pub/scm/linux/kernel/git/viro/vfs.git #experimental-for-mateusz
>
> It needs saner commit messages, references to your analysis of the
> overhead, quite possibly a finer carve-up, etc.  And it's really
> completely untested - it builds, but I hadn't even tried to boot
> the sucker, let alone give it any kind of beating, so consider that
> as a quick illustration (slapped together at 3am, on top of 5 hours of
> sleep yesterday) to what I'd been talking about and no more than that.

Well it's your call, you wrote the thing and I need the problem out of
the way, so I'm not going to argue about the patchset.

I verified it boots and provides the expected perf win [I have to
repeat it is highly variable between re-runs because of ever-changing
offsets between different inode allocations resulting in different
false-sharing problems; i'm going to separately mail about that]

I think it will be fine to copy the result from my commit message and
denote it's from a different variant achieving the same goal.

That said feel free to use my commit message in whatever capacity,
there is no need to mention me.

Thanks for sorting this out.
-- 
Mateusz Guzik <mjguzik gmail.com>

Powered by blists - more mailing lists