Re: [RFC PATCH] fs: dcache: Delete the associated dentry when deleting a file

[Yafang Shao <laoar.shao@gmail.com>]

On Sat, May 11, 2024 at 11:35 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> On Sat, May 11, 2024 at 10:54 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > On Fri, 10 May 2024 at 19:28, Yafang Shao <laoar.shao@gmail.com> 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