Re: [PATCH 2/2] deal with deadlock in d_walk()

Re: [PATCH 2/2] deal with deadlock in d_walk()

[Greg KH]

On Thu, Feb 05, 2015 at 03:33:41PM +0800, hujianyang wrote:
> 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>
> ---
>  fs/dcache.c |  102 ++++++++++++++++++++++++++++++++++++-----------------------
>  1 files changed, 62 insertions(+), 40 deletions(-)

Can you provide a version for 3.14-stable as well?

thanks,

greg k-h

Re: [PATCH 2/2] deal with deadlock in d_walk()

[hujianyang]

On 2015/4/26 18:21, Greg KH wrote:
> On Thu, Feb 05, 2015 at 03:33:41PM +0800, hujianyang wrote:
>> 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>
>> ---
>>  fs/dcache.c |  102 ++++++++++++++++++++++++++++++++++++-----------------------
>>  1 files changed, 62 insertions(+), 40 deletions(-)
> 
> Can you provide a version for 3.14-stable as well?
> 
> thanks,
> 
> greg k-h
> 
> .
> 

Hi Greg,

Sorry for my delay.

Here is the patch for 3.14-stable. Since we have 'd_walk()' in 3.14-stable, we
could just apply changes as same as original patch and do not need to apply Ben's
fix, commit 20defcec264c from 3.2.y.

Thanks!
Also thanks for the help from Ben!

Hu



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.14 refer to the work of Ben Hutchings in 3.2:
 - Adjust context to make __dentry_kill() apply to d_kill()]
Signed-off-by: hujianyang <hujianyang@huawei.com>
---
 fs/dcache.c |   31 ++++++++++++++++---------------
 1 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index c345f5f..a9231c8 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -435,7 +435,7 @@ static struct dentry *d_kill(struct dentry *dentry, struct dentry *parent)
 	__releases(parent->d_lock)
 	__releases(dentry->d_inode->i_lock)
 {
-	list_del(&dentry->d_child);
+	__list_del_entry(&dentry->d_child);
 	/*
 	 * Inform d_walk() that we are no longer attached to the
 	 * dentry tree
@@ -1123,33 +1123,31 @@ 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 = child->d_parent;

-		rcu_read_lock();
 		spin_unlock(&child->d_lock);
 		spin_lock(&this_parent->d_lock);

-		/*
-		 * might go back up the wrong parent if we have had a rename
-		 * or deletion
-		 */
-		if (this_parent != child->d_parent ||
-			 (child->d_flags & DCACHE_DENTRY_KILLED) ||
-			 need_seqretry(&rename_lock, seq)) {
-			spin_unlock(&this_parent->d_lock);
-			rcu_read_unlock();
+		/* might go back up the wrong parent if we have had a rename. */
+		if (need_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();
-		next = child->d_child.next;
 		goto resume;
 	}
-	if (need_seqretry(&rename_lock, seq)) {
-		spin_unlock(&this_parent->d_lock);
+	if (need_seqretry(&rename_lock, seq))
 		goto rename_retry;
-	}
+	rcu_read_unlock();
 	if (finish)
 		finish(data);

@@ -1159,6 +1157,9 @@ out_unlock:
 	return;

 rename_retry:
+	spin_unlock(&this_parent->d_lock);
+	rcu_read_unlock();
+	BUG_ON(seq & 1);
 	if (!retry)
 		return;
 	seq = 1;
-- 
1.6.0.2

Re: [PATCH 2/2] deal with deadlock in d_walk()

[Greg KH]

On Wed, May 06, 2015 at 03:02:59PM +0800, hujianyang wrote:
> On 2015/4/26 18:21, Greg KH wrote:
> > On Thu, Feb 05, 2015 at 03:33:41PM +0800, hujianyang wrote:
> >> 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>
> >> ---
> >>  fs/dcache.c |  102 ++++++++++++++++++++++++++++++++++++-----------------------
> >>  1 files changed, 62 insertions(+), 40 deletions(-)
> > 
> > Can you provide a version for 3.14-stable as well?
> > 
> > thanks,
> > 
> > greg k-h
> > 
> > .
> > 
> 
> Hi Greg,
> 
> Sorry for my delay.
> 
> Here is the patch for 3.14-stable. Since we have 'd_walk()' in 3.14-stable, we
> could just apply changes as same as original patch and do not need to apply Ben's
> fix, commit 20defcec264c from 3.2.y.
> 
> Thanks!
> Also thanks for the help from Ben!
> 
> Hu

Great, thanks for this, now queued up.

greg k-h