Message-ID: <20240128151542.6efa2118@rorschach.local.home>
Date: Sun, 28 Jan 2024 15:15:42 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Masami Hiramatsu <mhiramat@...nel.org>, Mathieu Desnoyers
 <mathieu.desnoyers@...icios.com>, LKML <linux-kernel@...r.kernel.org>,
 Linux Trace Devel <linux-trace-devel@...r.kernel.org>, Christian Brauner
 <brauner@...nel.org>, Ajay Kaher <ajay.kaher@...adcom.com>, Geert
 Uytterhoeven <geert@...ux-m68k.org>, linux-fsdevel
 <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH] eventfs: Have inodes have unique inode numbers

On Sat, 27 Jan 2024 13:47:32 -0800
Linus Torvalds <torvalds@...ux-foundation.org> wrote:
> 
> So here's an attempt at some fairly trivial but entirely untested cleanup.
> 
> I have *not* tested this at all, and I assume you have some extensive
> test-suite that you run. So these are "signed-off' in the sense that
> the patch looks fine, it builds in one configuration for me, but maybe
> there's something really odd going on.
> 
> The first patch is trivial dead code removal.

Ah yes. That was leftover code from the mount dentry walk that I
removed. I missed that code removal.

> 
> The second patch is because I really do not understand the reason for
> the 'ei->dentry' pointer, and it just looks messy.

I have to understand how the dentry lookup works. Basically, when the
ei gets deleted, it can't be freed until all dentries it references
(including its children) are no longer being accessed. Does that lookup
get called only when a dentry with the name doesn't already exist?

That is, can I assume that eventfs_root_lookup() is only called when
the VFS file system could not find an existing dentry and it has to
create one?

If that's the case, then I can remove the ei->dentry and just add a ref
counter that it was accessed. Then the final dput() should call
eventfs_set_ei_status_free() (I hate that name and need to change it),
and if the ei->is_freed is set, it can free the ei.

The eventfs_remove_dir() can free the ei (after SRCU) if it has no
references, otherwise it needs to wait for the final dput() to do the
free.

Again, if the ->lookup() only gets called if no dentry exists (where
ei->dentry would already be NULL), then I can do this.

I think the ei->dentry was required for the dir wrapper logic that we
removed.

> 
> It looks _particularly_ messy when it is mixed up in operations that
> really do not need it and really shouldn't use it.
> 
> The eventfs_find_events() code was using the dentry parent pointer to
> find the parent (fine, and simple), then looking up the tracefs inode
> using that (fine so far), but then it looked up the dentry using
> *that*. But it already *had* the dentry - it's that parent dentry it
> just used to find the tracefs inode. The code looked nonsensical.

That was probably from rewriting that function a few different ways.

> 
> Similarly, it then (in the set_top_events_ownership() helper) used
> 'ei->dentry' to update the events attr, but all that really wants is
> the superblock root. So instead of passing a dentry, just pass the
> superblock pointer, which you can find in either the dentry or in the
> VFS inode, depending on which level you're working at.
> 
> There are tons of other 'ei->dentry' uses, and I didn't look at those.
> Baby steps. But this *seems* like an obvious cleanup, and many small
> obvious cleanups later and perhaps the 'ei->dentry' pointer (and the
> '->d_children[]' array) can eventually go away. They should all be
> entirely useless - there's really no reason for a filesystem to hold
> on to back-pointers of dentries.
> 
> Anybody willing to run the test-suite on this?

It passed the ftrace selftests that are in the kernel, although the
ownership test fails if you run it a second time. That fails before
this patch too. It's because the test assumes that there's been no
chgrp done on any of the files/directories, which then permanently
assigns owership and ignores the default. The test needs to be fixed to
handle this case, as it calls chgrp and chown so itself is permanently
assigning ownership.

I'll have to run this on my entire test suit.

-- Steve

Powered by blists - more mailing lists