eventfs: Process deletion of dentry more thoroughly
Commit Message
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
Looking at how dentry is removed via the tracefs system, I found that
eventfs does not do everything that it did under tracefs. The tracefs
removal of a dentry calls simple_recursive_removal() that does a lot more
than a simple d_invalidate().
Have the same done on eventfs dentry:
1. Set S_DEAD for directories
2. Call clear_nlink() on the dentry inode
3. Call any notifiers about the dentry removal
Cc: stable@vger.kernel.org
Fixes: 5bdcd5f5331a2 ("eventfs: Implement removal of meta data from eventfs")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
fs/tracefs/event_inode.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
Comments
On Tue, Oct 31, 2023 at 02:47:03PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
>
> Looking at how dentry is removed via the tracefs system, I found that
> eventfs does not do everything that it did under tracefs. The tracefs
> removal of a dentry calls simple_recursive_removal() that does a lot more
> than a simple d_invalidate().
Umm... Is there any reason not to use simple_recursive_removal() there?
On Wed, 1 Nov 2023 02:25:53 +0000
Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Tue, Oct 31, 2023 at 02:47:03PM -0400, Steven Rostedt wrote:
> > From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> >
> > Looking at how dentry is removed via the tracefs system, I found that
> > eventfs does not do everything that it did under tracefs. The tracefs
> > removal of a dentry calls simple_recursive_removal() that does a lot more
> > than a simple d_invalidate().
>
> Umm... Is there any reason not to use simple_recursive_removal() there?
Hmm, I may be able to (I'm still a newbie with understanding of the vfs).
I did it this way thinking that a dentry may exist in the children but not
at a higher level, but I don't think that can be the case. This creates
dentries and inodes dynamically when they are referenced. The eventfs_inode
maps to each directory (the files of a directory are created from the
information from the eventfs_inode).
My thought process for doing it this way was if a child created a dentry
but the parent did not. But I don't think that can happen, right? So all I
may need to do is to check if the ei->dentry exists for the ei that is
being deleted, and after marking it and all its children as "freed", I can
then call simple_recursive_removal() on the top ei->dentry if it exists, as
that will guarantee to get all the dentries of any of the children that
exist. Right?
-- Steve
On Wed, 1 Nov 2023 00:16:59 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Wed, 1 Nov 2023 02:25:53 +0000
> Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> > Umm... Is there any reason not to use simple_recursive_removal() there?
>
> Hmm, I may be able to (I'm still a newbie with understanding of the vfs).
>
> I did it this way thinking that a dentry may exist in the children but not
> at a higher level, but I don't think that can be the case. This creates
> dentries and inodes dynamically when they are referenced. The eventfs_inode
> maps to each directory (the files of a directory are created from the
> information from the eventfs_inode).
OK, as I tried to use the simple_recursive_remove() and I failed miserably!
I think I know why. What happened was the last child would get one extra
"dput" than it needed. That's because dentry's exist without any reference
on them and they don't disappear until a reclaim happens. What I mean is,
when a file is "open()'d" a dentry is created on the fly so that the user
can access the file. When it is "close()'d" the dentry count goes to zero.
Then on memory reclaim, the dentries may be removed. If another open
happens, the dentry is created again, or the one that is still cached can
be reinstated.
It looks like the simple_recursive_remove() expects all dentries to have at
least a 1 when entering, which is not the case here.
But!
Now what I could do is to do a dget() when removing the eventfs_inodes (ei)
on any dentry that is attached to them.
/me goes and tries that...
OK, that actually seems to work. With the assumption that there will never
be dentry without a parent I think I can go this approach.
Thanks!
-- Steve
@@ -856,6 +856,7 @@ static void unhook_dentry(struct dentry **dentry, struct dentry **list)
*dentry = NULL;
}
}
+
/**
* eventfs_remove_dir - remove eventfs dir or file from list
* @ei: eventfs_inode to be removed.
@@ -868,6 +869,7 @@ void eventfs_remove_dir(struct eventfs_inode *ei)
LIST_HEAD(ei_del_list);
struct dentry *dentry_list = NULL;
struct dentry *dentry;
+ struct inode *inode;
int i;
if (!ei)
@@ -891,7 +893,28 @@ void eventfs_remove_dir(struct eventfs_inode *ei)
ptr = (unsigned long)dentry->d_fsdata & ~1UL;
dentry_list = (struct dentry *)ptr;
dentry->d_fsdata = NULL;
+
+ inode = dentry->d_inode;
+ inode_lock(inode);
+ if (d_is_dir(dentry))
+ inode->i_flags |= S_DEAD;
+ clear_nlink(inode);
+ inode_unlock(inode);
+
+ inode = dentry->d_parent->d_inode;
+ inode_lock(inode);
+
+ /* Remove its visibility */
d_invalidate(dentry);
+ if (d_is_dir(dentry))
+ fsnotify_rmdir(inode, dentry);
+ else
+ fsnotify_unlink(inode, dentry);
+
+ if (d_is_dir(dentry))
+ drop_nlink(inode);
+ inode_unlock(inode);
+
mutex_lock(&eventfs_mutex);
/* dentry should now have at least a single reference */
WARN_ONCE((int)d_count(dentry) < 1,