On Fri, 10 May 2024 at 19:28, Yafang Shao <laoar.shao@xxxxxxxxx> wrote:
>
> We've devised a solution to address both issues by deleting associated
> dentry when removing a file.
This patch is buggy. You are modifying d_flags outside the locked region.
So at a minimum, the DCACHE_FILE_DELETED bit setting would need to
just go into the
if (dentry->d_lockref.count == 1) {
side of the conditional, since the other side of that conditional
already unhashes the dentry which makes this all moot anyway.
That said, I think it's buggy in another way too: what if somebody
else looks up the dentry before it actually gets unhashed? Then you
have another ref to it, and the dentry might live long enough that it
then gets re-used for a newly created file (which is why we have those
negative dentries in the first place).
So you'd have to clear the DCACHE_FILE_DELETED if the dentry is then
made live by a file creation or rename or whatever.
So that d_flags thing is actually pretty complicated.
But since you made all this unconditional anyway, I think having a new
dentry flag is unnecessary in the first place, and I suspect you are
better off just unhashing the dentry unconditionally instead.
IOW, I think the simpler patch is likely just something like this:
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2381,6 +2381,7 @@ void d_delete(struct dentry * dentry)
spin_lock(&inode->i_lock);
spin_lock(&dentry->d_lock);
+ __d_drop(dentry);
/*
* Are we the only user?
*/
@@ -2388,7 +2389,6 @@ void d_delete(struct dentry * dentry)
dentry->d_flags &= ~DCACHE_CANT_MOUNT;
dentry_unlink_inode(dentry);
} else {
- __d_drop(dentry);
spin_unlock(&dentry->d_lock);
spin_unlock(&inode->i_lock);
}
although I think Al needs to ACK this, and I suspect that unhashing
the dentry also makes that
dentry->d_flags &= ~DCACHE_CANT_MOUNT;
pointless (because the dentry won't be reused, so DCACHE_CANT_MOUNT
just won't matter).
I do worry that there are loads that actually love our current
behavior, but maybe it's worth doing the simple unconditional "make
d_delete() always unhash" and only worry about whether that causes
performance problems for people who commonly create a new file in its
place when we get such a report.
IOW, the more complex thing might be to actually take other behavior
into account (eg "do we have so many negative dentries that we really
don't want to create new ones").
Al - can you please step in and tell us what else I've missed, and why
my suggested version of the patch is also broken garbage?
Linus
