On Wed, Aug 7, 2024 at 9:05 AM Al Viro <viro@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx> 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>