Hang/soft lockup in d_invalidate with simultaneous calls

Commit Message

Al Viro June 3, 2017, 6:20 a.m. UTC

On Fri, Jun 02, 2017 at 10:22:39PM -0700, Khazhismel Kumykov wrote:
> On Fri, Jun 2, 2017 at 6:12 PM, Al Viro <[e-mail]> wrote:
> > Part of that could be relieved if we turned check_and_drop() into
> > static void check_and_drop(void *_data)
> > {
> >         struct detach_data *data = _data;
> >
> >         if (!data->mountpoint && list_empty(&data->select.dispose))
> >                 __d_drop(data->select.start);
> > }
> 
> So with this change, d_invalidate will drop the starting dentry before
> all it's children are dropped? Would it make sense to just drop it
> right off the bat, and let one task handle shrinking all it's
> children?

We can't - not until we know that there's nothing mounted under it.
The thing is, unlike shrink_dcache_parent() we *can* bugger off as
soon as we'd found no victims, nothing mounted and dentry itself
is unhashed.  We can't do anything in select_collect() (we would've
broken shrink_dcache_parent() that way), but we can do unhashing
in check_and_drop() in "really nothing to do" case and we can return
from d_invalidate() after that.  So how about this:

Comments

Khazhy Kumykov June 3, 2017, 6:47 a.m. UTC | #1

On Fri, Jun 2, 2017 at 11:20 PM, Al Viro <[e-mail]> wrote:
> The thing is, unlike shrink_dcache_parent() we *can* bugger off as
> soon as we'd found no victims, nothing mounted and dentry itself
> is unhashed.  We can't do anything in select_collect() (we would've
> broken shrink_dcache_parent() that way), but we can do unhashing
> in check_and_drop() in "really nothing to do" case and we can return
> from d_invalidate() after that.  So how about this:
That does the trick.

Khazhy Kumykov June 12, 2017, 11 p.m. UTC | #2

On Fri, Jun 2, 2017 at 11:47 PM, Khazhismel Kumykov <[e-mail]> wrote:
> On Fri, Jun 2, 2017 at 11:20 PM, Al Viro <[e-mail]> wrote:
>> The thing is, unlike shrink_dcache_parent() we *can* bugger off as
>> soon as we'd found no victims, nothing mounted and dentry itself
>> is unhashed.  We can't do anything in select_collect() (we would've
>> broken shrink_dcache_parent() that way), but we can do unhashing
>> in check_and_drop() in "really nothing to do" case and we can return
>> from d_invalidate() after that.  So how about this:
> That does the trick.

I'm not entirely familiar the process here, is the above change
committed somewhere, should I propose a patch?

Al Viro June 15, 2017, 10:50 a.m. UTC | #3

On Mon, Jun 12, 2017 at 04:00:45PM -0700, Khazhismel Kumykov wrote:
> On Fri, Jun 2, 2017 at 11:47 PM, Khazhismel Kumykov <[e-mail]> wrote:
> > On Fri, Jun 2, 2017 at 11:20 PM, Al Viro <[e-mail]> wrote:
> >> The thing is, unlike shrink_dcache_parent() we *can* bugger off as
> >> soon as we'd found no victims, nothing mounted and dentry itself
> >> is unhashed.  We can't do anything in select_collect() (we would've
> >> broken shrink_dcache_parent() that way), but we can do unhashing
> >> in check_and_drop() in "really nothing to do" case and we can return
> >> from d_invalidate() after that.  So how about this:
> > That does the trick.
> 
> I'm not entirely familiar the process here, is the above change
> committed somewhere, should I propose a patch?

Sorry, got distracted by other stuff; I'll push that today.

Patch

diff --git a/fs/dcache.c b/fs/dcache.c
index cddf39777835..a9f995f6859e 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1494,7 +1494,7 @@  static void check_and_drop(void *_data)
 {
 	struct detach_data *data = _data;
 
-	if (!data->mountpoint && !data->select.found)
+	if (!data->mountpoint && list_empty(&data->select.dispose))
 		__d_drop(data->select.start);
 }
 
@@ -1536,17 +1536,15 @@  void d_invalidate(struct dentry *dentry)
 
 		d_walk(dentry, &data, detach_and_collect, check_and_drop);
 
-		if (data.select.found)
+		if (!list_empty(&data.select.dispose))
 			shrink_dentry_list(&data.select.dispose);
+		else if (!data.mountpoint)
+			return;
 
 		if (data.mountpoint) {
 			detach_mounts(data.mountpoint);
 			dput(data.mountpoint);
 		}
-
-		if (!data.mountpoint && !data.select.found)
-			break;
-
 		cond_resched();
 	}
 }