On Sat, May 11, 2024 at 11:35 AM Yafang Shao <laoar.shao@xxxxxxxxx> wrote:
>
> On Sat, May 11, 2024 at 10:54 AM Linus Torvalds
> <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > 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:
>
> It's simpler. I used to contemplate handling it that way, but lack the
> knowledge and courage to proceed, hence I opted for the d_flags
> solution.
> I'll conduct tests on the revised change. Appreciate your suggestion.
>
We have successfully applied a hotfix to a subset of our production
servers, totaling several thousand. The hotfix is as follows:
diff --git a/fs/dcache.c b/fs/dcache.c
index 52e6d5f..30eb733 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2557,14 +2557,14 @@ void d_delete(struct dentry * dentry)
spin_lock(&inode->i_lock);
spin_lock(&dentry->d_lock);
+ __d_drop(dentry);
+
/*
* Are we the only user?
*/
if (dentry->d_lockref.count == 1) {
- dentry->d_flags &= ~DCACHE_CANT_MOUNT;
dentry_unlink_inode(dentry);
} else {
- __d_drop(dentry);
spin_unlock(&dentry->d_lock);
spin_unlock(&inode->i_lock);
}
So far, it has been functioning well without any regressions. We are
planning to roll this update out to our entire fleet, which consists
of hundreds of thousands of servers.
I believe this change is still necessary. Would you prefer to commit
it directly, or should I send an official patch?
If the "unlink-create" issue is a concern, perhaps we can address it
by adding a /sys/kernel/debug/vfs/delete_file_legacy entry?
> >
> > --- 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").
>
> This poses a substantial challenge. Despite recurrent discussions
> within the community about improving negative dentry over and over,
> there hasn't been a consensus on how to address it.
>
> >
> > 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
>
>
> --
> Regards
> Yafang
--
Regards
Yafang
