On 2018-02-25, Al Viro <v...@zeniv.linux.org.uk> wrote:
> I'll play with cleaning up and reordering tomorrow after I get some
> sleep. In the meanwhile, the current state of that set is at
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #work.dcache
I have one comment on your new dentry_kill()...
> From e77db95d1ad189c76bcda7b90ac2225e98d776a2 Mon Sep 17 00:00:00 2001
> From: Al Viro <v...@zeniv.linux.org.uk>
> Date: Fri, 23 Feb 2018 21:25:42 -0500
> Subject: [PATCH] get rid of trylock loop around dentry_kill()
>
> In case when trylock in there fails, deal with it directly in
> dentry_kill(). Note that in cases when we drop and retake
> ->d_lock, we need to recheck whether to retain the dentry.
> Another thing is that dropping/retaking ->d_lock might have
> ended up with negative dentry turning into positive; that,
> of course, can happen only once...
>
> Signed-off-by: Al Viro <v...@zeniv.linux.org.uk>
> ---
> fs/dcache.c | 34 +++++++++++++++++++++++++++-------
> 1 file changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 449c1a5..c1f082d 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -657,23 +657,43 @@ static struct dentry *dentry_kill(struct dentry *dentry)
> struct dentry *parent = NULL;
>
> if (inode && unlikely(!spin_trylock(&inode->i_lock)))
> - goto failed;
> + goto slow_positive;
>
> if (!IS_ROOT(dentry)) {
> parent = dentry->d_parent;
> if (unlikely(!spin_trylock(&parent->d_lock))) {
> - if (inode)
> - spin_unlock(&inode->i_lock);
> - goto failed;
> + 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;
> }
> }
> -
> __dentry_kill(dentry);
> return parent;
>
> -failed:
> +slow_positive:
> + spin_unlock(&dentry->d_lock);
> + spin_lock(&inode->i_lock);
> + spin_lock(&dentry->d_lock);
> + parent = lock_parent(dentry);
> +got_locks:
> + if (likely(dentry->d_lockref.count == 1 && !retain_dentry(dentry))) {
> + __dentry_kill(dentry);
> + return parent;
> + }
> + /* we are keeping it, after all */
> + if (inode)
> + spin_unlock(&inode->i_lock);
> + if (parent)
> + spin_unlock(&parent->d_lock);
If someone else has grabbed a reference, it shouldn't be added to the
lru list. Only decremented.
if (entry->d_lockref.count == 1)
> + dentry_lru_add(dentry);
> + dentry->d_lockref.count--;
> spin_unlock(&dentry->d_lock);
> - return dentry; /* try again with same dentry */
> + return NULL;
> }
>
> /*
