On Sat, 27 Jan 2024 13:47:32 -0800 Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> 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
|