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