Re: [PATCH 3.10 27/31] deal with deadlock in d_walk()

[Ben Hutchings <ben@decadent.org.uk>]

[-- Attachment #1: Type: text/plain, Size: 7044 bytes --]

On Sun, 2015-04-26 at 15:49 +0200, Greg Kroah-Hartman wrote:
> 3.10-stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> commit ca5358ef75fc69fee5322a38a340f5739d997c10 upstream.
> 
> ... by not hitting rename_retry for reasons other than rename having
> happened.  In other words, do _not_ restart when finding that
> between unlocking the child and locking the parent the former got
> into __dentry_kill().  Skip the killed siblings instead...
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Ben Hutchings <ben@decadent.org.uk>
> [hujianyang: Backported to 3.10 refer to the work of Ben Hutchings in 3.2:
>  - As we only have try_to_ascend() and not d_walk(), apply this
>    change to all callers of try_to_ascend()
>  - Adjust context to make __dentry_kill() apply to d_kill()]
> Signed-off-by: hujianyang <hujianyang@huawei.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

This is broken; you need to fold in commit 20defcec264c from 3.2.y
("dcache: Fix locking bugs in backported "deal with deadlock in
d_walk()"").

Ben.

> ---
>  fs/dcache.c |  102 ++++++++++++++++++++++++++++++++++++------------------------
>  1 file changed, 62 insertions(+), 40 deletions(-)
> 
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -364,9 +364,9 @@ static struct dentry *d_kill(struct dent
>  	__releases(parent->d_lock)
>  	__releases(dentry->d_inode->i_lock)
>  {
> -	list_del(&dentry->d_child);
> +	__list_del_entry(&dentry->d_child);
>  	/*
> -	 * Inform try_to_ascend() that we are no longer attached to the
> +	 * Inform ascending readers that we are no longer attached to the
>  	 * dentry tree
>  	 */
>  	dentry->d_flags |= DCACHE_DENTRY_KILLED;
> @@ -988,35 +988,6 @@ void shrink_dcache_for_umount(struct sup
>  }
>  
>  /*
> - * This tries to ascend one level of parenthood, but
> - * we can race with renaming, so we need to re-check
> - * the parenthood after dropping the lock and check
> - * that the sequence number still matches.
> - */
> -static struct dentry *try_to_ascend(struct dentry *old, int locked, unsigned seq)
> -{
> -	struct dentry *new = old->d_parent;
> -
> -	rcu_read_lock();
> -	spin_unlock(&old->d_lock);
> -	spin_lock(&new->d_lock);
> -
> -	/*
> -	 * might go back up the wrong parent if we have had a rename
> -	 * or deletion
> -	 */
> -	if (new != old->d_parent ||
> -		 (old->d_flags & DCACHE_DENTRY_KILLED) ||
> -		 (!locked && read_seqretry(&rename_lock, seq))) {
> -		spin_unlock(&new->d_lock);
> -		new = NULL;
> -	}
> -	rcu_read_unlock();
> -	return new;
> -}
> -
> -
> -/*
>   * Search for at least 1 mount point in the dentry's subdirs.
>   * We descend to the next level whenever the d_subdirs
>   * list is non-empty and continue searching.
> @@ -1070,17 +1041,32 @@ resume:
>  	/*
>  	 * All done at this level ... ascend and resume the search.
>  	 */
> +	rcu_read_lock();
> +ascend:
>  	if (this_parent != parent) {
>  		struct dentry *child = this_parent;
> -		this_parent = try_to_ascend(this_parent, locked, seq);
> -		if (!this_parent)
> +		this_parent = child->d_parent;
> +
> +		spin_unlock(&child->d_lock);
> +		spin_lock(&this_parent->d_lock);
> +
> +		/* might go back up the wrong parent if we have had a rename. */
> +		if (!locked && read_seqretry(&rename_lock, seq))
>  			goto rename_retry;
>  		next = child->d_child.next;
> +		while (unlikely(child->d_flags & DCACHE_DENTRY_KILLED)) {
> +			if (next == &this_parent->d_subdirs)
> +				goto ascend;
> +			child = list_entry(next, struct dentry, d_child);
> +			next = next->next;
> +		}
> +		rcu_read_unlock();
>  		goto resume;
>  	}
> -	spin_unlock(&this_parent->d_lock);
>  	if (!locked && read_seqretry(&rename_lock, seq))
>  		goto rename_retry;
> +	spin_unlock(&this_parent->d_lock);
> +	rcu_read_unlock();
>  	if (locked)
>  		write_sequnlock(&rename_lock);
>  	return 0; /* No mount points found in tree */
> @@ -1092,6 +1078,8 @@ positive:
>  	return 1;
>  
>  rename_retry:
> +	spin_unlock(&this_parent->d_lock);
> +	rcu_read_unlock();
>  	if (locked)
>  		goto again;
>  	locked = 1;
> @@ -1177,23 +1165,40 @@ resume:
>  	/*
>  	 * All done at this level ... ascend and resume the search.
>  	 */
> +	rcu_read_lock();
> +ascend:
>  	if (this_parent != parent) {
>  		struct dentry *child = this_parent;
> -		this_parent = try_to_ascend(this_parent, locked, seq);
> -		if (!this_parent)
> +		this_parent = child->d_parent;
> +
> +		spin_unlock(&child->d_lock);
> +		spin_lock(&this_parent->d_lock);
> +
> +		/* might go back up the wrong parent if we have had a rename. */
> +		if (!locked && read_seqretry(&rename_lock, seq))
>  			goto rename_retry;
>  		next = child->d_child.next;
> +		while (unlikely(child->d_flags & DCACHE_DENTRY_KILLED)) {
> +			if (next == &this_parent->d_subdirs)
> +				goto ascend;
> +			child = list_entry(next, struct dentry, d_child);
> +			next = next->next;
> +		}
> +		rcu_read_unlock();
>  		goto resume;
>  	}
>  out:
> -	spin_unlock(&this_parent->d_lock);
>  	if (!locked && read_seqretry(&rename_lock, seq))
>  		goto rename_retry;
> +	spin_unlock(&this_parent->d_lock);
> +	rcu_read_unlock();
>  	if (locked)
>  		write_sequnlock(&rename_lock);
>  	return found;
>  
>  rename_retry:
> +	spin_unlock(&this_parent->d_lock);
> +	rcu_read_unlock();
>  	if (found)
>  		return found;
>  	if (locked)
> @@ -2954,26 +2959,43 @@ resume:
>  		}
>  		spin_unlock(&dentry->d_lock);
>  	}
> +	rcu_read_lock();
> +ascend:
>  	if (this_parent != root) {
>  		struct dentry *child = this_parent;
>  		if (!(this_parent->d_flags & DCACHE_GENOCIDE)) {
>  			this_parent->d_flags |= DCACHE_GENOCIDE;
>  			this_parent->d_count--;
>  		}
> -		this_parent = try_to_ascend(this_parent, locked, seq);
> -		if (!this_parent)
> +		this_parent = child->d_parent;
> +
> +		spin_unlock(&child->d_lock);
> +		spin_lock(&this_parent->d_lock);
> +
> +		/* might go back up the wrong parent if we have had a rename. */
> +		if (!locked && read_seqretry(&rename_lock, seq))
>  			goto rename_retry;
>  		next = child->d_child.next;
> +		while (unlikely(child->d_flags & DCACHE_DENTRY_KILLED)) {
> +			if (next == &this_parent->d_subdirs)
> +				goto ascend;
> +			child = list_entry(next, struct dentry, d_child);
> +			next = next->next;
> +		}
> +		rcu_read_unlock();
>  		goto resume;
>  	}
> -	spin_unlock(&this_parent->d_lock);
>  	if (!locked && read_seqretry(&rename_lock, seq))
>  		goto rename_retry;
> +	spin_unlock(&this_parent->d_lock);
> +	rcu_read_unlock();
>  	if (locked)
>  		write_sequnlock(&rename_lock);
>  	return;
>  
>  rename_retry:
> +	spin_unlock(&this_parent->d_lock);
> +	rcu_read_unlock();
>  	if (locked)
>  		goto again;
>  	locked = 1;
> 
> 

-- 
Ben Hutchings
I'm not a reverse psychological virus.  Please don't copy me into your sig.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]