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.
>
> --- 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
