On Tue, Jul 02, 2019 at 05:21:26PM +0800, Hillf Danton wrote:
> Hello,
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -673,14 +673,11 @@ static struct dentry *dentry_kill(struct dentry *dentry)
> if (!IS_ROOT(dentry)) {
> parent = dentry->d_parent;
> if (unlikely(!spin_trylock(&parent->d_lock))) {
> - parent = __lock_parent(dentry);
> - if (likely(inode || !dentry->d_inode))
> - goto got_locks;
> - /* negative that became positive */
> - if (parent)
> - spin_unlock(&parent->d_lock);
> - inode = dentry->d_inode;
> - goto slow_positive;
> + /*
> + * fine if peer is busy either populating or
> + * cleaning up parent
> + */
> + parent = NULL;
> }
> }
> __dentry_kill(dentry);
This is very much *NOT* fine.
1) trylock can fail from any number of reasons, starting
with "somebody is going through the hash chain doing a lookup on
something completely unrelated"
2) whoever had been holding the lock and whatever they'd
been doing might be over right after we get the return value from
spin_trylock().
3) even had that been really somebody adding children in
the same parent *AND* even if they really kept doing that, rather
than unlocking and buggering off, would you care to explain why
dentry_unlist() called by __dentry_kill() and removing the victim
from the list of children would be safe to do in parallel with that?
NAK, in case it's not obvious from the above.
