[PATCH 4.4 030/131] Hang/soft lockup in d_invalidate with simultaneous calls

[Greg Kroah-Hartman]

4.4-stable review patch.  If anyone has any objections, please let me know.

--

From: Al Viro 

commit 81be24d263dbeddaba35827036d6f6787a59c2c3 upstream.

It's not hard to trigger a bunch of d_invalidate() on the same
dentry in parallel.  They end up fighting each other - any
dentry picked for removal by one will be skipped by the rest
and we'll go for the next iteration through the entire
subtree, even if everything is being skipped.  Morevoer, we
immediately go back to scanning the subtree.  The only thing
we really need is to dissolve all mounts in the subtree and
as soon as we've nothing left to do, we can just unhash the
dentry and bugger off.

Signed-off-by: Al Viro 
Signed-off-by: Arnd Bergmann 
Signed-off-by: Greg Kroah-Hartman 

---
 fs/dcache.c |   10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

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

[PATCH 4.9 21/30] Hang/soft lockup in d_invalidate with simultaneous calls

[Greg Kroah-Hartman]

4.9-stable review patch.  If anyone has any objections, please let me know.

--

From: Al Viro 

commit 81be24d263dbeddaba35827036d6f6787a59c2c3 upstream.

It's not hard to trigger a bunch of d_invalidate() on the same
dentry in parallel.  They end up fighting each other - any
dentry picked for removal by one will be skipped by the rest
and we'll go for the next iteration through the entire
subtree, even if everything is being skipped.  Morevoer, we
immediately go back to scanning the subtree.  The only thing
we really need is to dissolve all mounts in the subtree and
as soon as we've nothing left to do, we can just unhash the
dentry and bugger off.

Signed-off-by: Al Viro 
Signed-off-by: Arnd Bergmann 
Signed-off-by: Greg Kroah-Hartman 

---
 fs/dcache.c |   10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

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

Re: [BACKPORT 4.4.y 20/25] Hang/soft lockup in d_invalidate with simultaneous calls

[Greg KH]

On Fri, Mar 22, 2019 at 04:44:11PM +0100, Arnd Bergmann wrote:
> From: Al Viro 
> 
> It's not hard to trigger a bunch of d_invalidate() on the same
> dentry in parallel.  They end up fighting each other - any
> dentry picked for removal by one will be skipped by the rest
> and we'll go for the next iteration through the entire
> subtree, even if everything is being skipped.  Morevoer, we
> immediately go back to scanning the subtree.  The only thing
> we really need is to dissolve all mounts in the subtree and
> as soon as we've nothing left to do, we can just unhash the
> dentry and bugger off.
> 
> Signed-off-by: Al Viro 
> (cherry picked from commit 81be24d263dbeddaba35827036d6f6787a59c2c3)
> Signed-off-by: Arnd Bergmann 
> ---
>  fs/dcache.c | 10 --
>  1 file changed, 4 insertions(+), 6 deletions(-)

Also added to 4.9.y

[BACKPORT 4.4.y 20/25] Hang/soft lockup in d_invalidate with simultaneous calls

[Arnd Bergmann]

From: Al Viro 

It's not hard to trigger a bunch of d_invalidate() on the same
dentry in parallel.  They end up fighting each other - any
dentry picked for removal by one will be skipped by the rest
and we'll go for the next iteration through the entire
subtree, even if everything is being skipped.  Morevoer, we
immediately go back to scanning the subtree.  The only thing
we really need is to dissolve all mounts in the subtree and
as soon as we've nothing left to do, we can just unhash the
dentry and bugger off.

Signed-off-by: Al Viro 
(cherry picked from commit 81be24d263dbeddaba35827036d6f6787a59c2c3)
Signed-off-by: Arnd Bergmann 
---
 fs/dcache.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

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

Re: Hang/soft lockup in d_invalidate with simultaneous calls

[Al Viro]

On Mon, Jun 12, 2017 at 04:00:45PM -0700, Khazhismel Kumykov wrote:
> On Fri, Jun 2, 2017 at 11:47 PM, Khazhismel Kumykov  wrote:
> > On Fri, Jun 2, 2017 at 11:20 PM, Al Viro  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.

Re: Hang/soft lockup in d_invalidate with simultaneous calls

[Al Viro]

On Mon, Jun 12, 2017 at 04:00:45PM -0700, Khazhismel Kumykov wrote:
> On Fri, Jun 2, 2017 at 11:47 PM, Khazhismel Kumykov  wrote:
> > On Fri, Jun 2, 2017 at 11:20 PM, Al Viro  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.

Re: Hang/soft lockup in d_invalidate with simultaneous calls

[Khazhismel Kumykov]

On Fri, Jun 2, 2017 at 11:47 PM, Khazhismel Kumykov  wrote:
> On Fri, Jun 2, 2017 at 11:20 PM, Al Viro  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?


smime.p7s
Description: S/MIME Cryptographic Signature

Re: Hang/soft lockup in d_invalidate with simultaneous calls

[Khazhismel Kumykov]

On Fri, Jun 2, 2017 at 11:47 PM, Khazhismel Kumykov  wrote:
> On Fri, Jun 2, 2017 at 11:20 PM, Al Viro  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?


smime.p7s
Description: S/MIME Cryptographic Signature

Re: Hang/soft lockup in d_invalidate with simultaneous calls

[Khazhismel Kumykov]

On Fri, Jun 2, 2017 at 11:20 PM, Al Viro  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.


smime.p7s
Description: S/MIME Cryptographic Signature

Re: Hang/soft lockup in d_invalidate with simultaneous calls

[Khazhismel Kumykov]

On Fri, Jun 2, 2017 at 11:20 PM, Al Viro  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.


smime.p7s
Description: S/MIME Cryptographic Signature

Re: Hang/soft lockup in d_invalidate with simultaneous calls

[Al Viro]

On Fri, Jun 02, 2017 at 10:22:39PM -0700, Khazhismel Kumykov wrote:
> On Fri, Jun 2, 2017 at 6:12 PM, Al Viro  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(>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:

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(>select.dispose))
__d_drop(data->select.start);
 }
 
@@ -1536,17 +1536,15 @@ void d_invalidate(struct dentry *dentry)
 
d_walk(dentry, , detach_and_collect, check_and_drop);
 
-   if (data.select.found)
+   if (!list_empty())
shrink_dentry_list();
+   else if (!data.mountpoint)
+   return;
 
if (data.mountpoint) {
detach_mounts(data.mountpoint);
dput(data.mountpoint);
}
-
-   if (!data.mountpoint && !data.select.found)
-   break;
-
cond_resched();
}
 }

Re: Hang/soft lockup in d_invalidate with simultaneous calls

[Al Viro]

On Fri, Jun 02, 2017 at 10:22:39PM -0700, Khazhismel Kumykov wrote:
> On Fri, Jun 2, 2017 at 6:12 PM, Al Viro  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(>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:

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(>select.dispose))
__d_drop(data->select.start);
 }
 
@@ -1536,17 +1536,15 @@ void d_invalidate(struct dentry *dentry)
 
d_walk(dentry, , detach_and_collect, check_and_drop);
 
-   if (data.select.found)
+   if (!list_empty())
shrink_dentry_list();
+   else if (!data.mountpoint)
+   return;
 
if (data.mountpoint) {
detach_mounts(data.mountpoint);
dput(data.mountpoint);
}
-
-   if (!data.mountpoint && !data.select.found)
-   break;
-
cond_resched();
}
 }

Re: Hang/soft lockup in d_invalidate with simultaneous calls

[Khazhismel Kumykov]

On Fri, Jun 2, 2017 at 6:12 PM, Al Viro  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(>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?
>
> It doesn't solve the entire problem, though - we still could get too
> many threads into that thing before any of them gets to that __d_drop().

Yes, the change isn't sufficient for my repro, as many threads get to
the loop before the drop, although new tasks don't get stuck in the
same loop after the dentry is dropped.

> I wonder if we should try and collect more victims; that could lead
> to contention of its own, but...

>From my understanding, the contention is the worst when one task is
shrinking everything, and several other tasks are busily looping
walking the dentry until everything is done. In this case, the tasks
busily looping d_walk hold the d_lock for a dentry while walking over
all it's children, then soon after it finishes the d_walk, it queues
again to walk again, while shrink_dentry_list releases and re-grabs
for each entry.

If we limit the number of items we pass to shrink_dentry_list at one
time things actually look quite a bit better. e.g., in testing
arbitrarily limiting select.dispose to 1000 elements does fix my
repro.

e.g.
diff --git a/fs/dcache.c b/fs/dcache.c
index 22af360ceca3..3892e0eb7ec2 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1367,6 +1367,7 @@ struct select_data {
struct dentry *start;
struct list_head dispose;
int found;
+   int actually_found;
 };

 static enum d_walk_ret select_collect(void *_data, struct dentry *dentry)
@@ -1388,6 +1389,7 @@ static enum d_walk_ret select_collect(void
*_data, struct dentry *dentry)
if (!dentry->d_lockref.count) {
d_shrink_add(dentry, >dispose);
data->found++;
+   data->actually_found++;
}
}
/*
@@ -1397,6 +1399,9 @@ static enum d_walk_ret select_collect(void
*_data, struct dentry *dentry)
 */
if (!list_empty(>dispose))
ret = need_resched() ? D_WALK_QUIT : D_WALK_NORETRY;
+
+   if (data->actually_found > 1000)
+   ret = D_WALK_QUIT;
 out:
return ret;
 }
@@ -1415,6 +1420,7 @@ void shrink_dcache_parent(struct dentry *parent)
INIT_LIST_HEAD();
data.start = parent;
data.found = 0;
+   data.actually_found = 0;

d_walk(parent, , select_collect, NULL);
if (!data.found)
@@ -1536,6 +1542,7 @@ void d_invalidate(struct dentry *dentry)
INIT_LIST_HEAD();
data.select.start = dentry;
data.select.found = 0;
+   data.select.actually_found = 0;

d_walk(dentry, , detach_and_collect, check_and_drop);


smime.p7s
Description: S/MIME Cryptographic Signature

Re: Hang/soft lockup in d_invalidate with simultaneous calls

[Khazhismel Kumykov]

On Fri, Jun 2, 2017 at 6:12 PM, Al Viro  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(>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?
>
> It doesn't solve the entire problem, though - we still could get too
> many threads into that thing before any of them gets to that __d_drop().

Yes, the change isn't sufficient for my repro, as many threads get to
the loop before the drop, although new tasks don't get stuck in the
same loop after the dentry is dropped.

> I wonder if we should try and collect more victims; that could lead
> to contention of its own, but...

>From my understanding, the contention is the worst when one task is
shrinking everything, and several other tasks are busily looping
walking the dentry until everything is done. In this case, the tasks
busily looping d_walk hold the d_lock for a dentry while walking over
all it's children, then soon after it finishes the d_walk, it queues
again to walk again, while shrink_dentry_list releases and re-grabs
for each entry.

If we limit the number of items we pass to shrink_dentry_list at one
time things actually look quite a bit better. e.g., in testing
arbitrarily limiting select.dispose to 1000 elements does fix my
repro.

e.g.
diff --git a/fs/dcache.c b/fs/dcache.c
index 22af360ceca3..3892e0eb7ec2 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1367,6 +1367,7 @@ struct select_data {
struct dentry *start;
struct list_head dispose;
int found;
+   int actually_found;
 };

 static enum d_walk_ret select_collect(void *_data, struct dentry *dentry)
@@ -1388,6 +1389,7 @@ static enum d_walk_ret select_collect(void
*_data, struct dentry *dentry)
if (!dentry->d_lockref.count) {
d_shrink_add(dentry, >dispose);
data->found++;
+   data->actually_found++;
}
}
/*
@@ -1397,6 +1399,9 @@ static enum d_walk_ret select_collect(void
*_data, struct dentry *dentry)
 */
if (!list_empty(>dispose))
ret = need_resched() ? D_WALK_QUIT : D_WALK_NORETRY;
+
+   if (data->actually_found > 1000)
+   ret = D_WALK_QUIT;
 out:
return ret;
 }
@@ -1415,6 +1420,7 @@ void shrink_dcache_parent(struct dentry *parent)
INIT_LIST_HEAD();
data.start = parent;
data.found = 0;
+   data.actually_found = 0;

d_walk(parent, , select_collect, NULL);
if (!data.found)
@@ -1536,6 +1542,7 @@ void d_invalidate(struct dentry *dentry)
INIT_LIST_HEAD();
data.select.start = dentry;
data.select.found = 0;
+   data.select.actually_found = 0;

d_walk(dentry, , detach_and_collect, check_and_drop);


smime.p7s
Description: S/MIME Cryptographic Signature

Re: Hang/soft lockup in d_invalidate with simultaneous calls

[Al Viro]

On Wed, May 17, 2017 at 02:58:11PM -0700, Khazhismel Kumykov wrote:

> Once the dentry is on a shrink list would
> it be unreachable anyways,

Why would it be?  Suppose e.g. shrink_dcache_parent() finds a dentry with
zero refcount; to the shrink list it goes, right?  Then, before we actually
get around to evicting it, somebody goes and looks it up and incrementes
refcount.  It's still on the shrink list at that point; whoever had done
that lookup has no way to safely remove the sucker from that - only the
owner of shrink list could do that.  And that's precisely what happens
once that owner gets around to shrink_dentry_list():
d_shrink_del(dentry);

/*
 * We found an inuse dentry which was not removed from
 * the LRU because of laziness during lookup. Do not free it.
 */
if (dentry->d_lockref.count > 0) {
spin_unlock(>d_lock);
if (parent)
spin_unlock(>d_lock);
continue;
}
and we are done with it.

dentry being on a shrink list is *NOT* unreachable; it might have been already
looked up since it had been placed there and it might be looked up at any point
up until shrink_dentry_list() gets to it.

We really can't quit d_invalidate() until all those dentries are done with.
The shit hits the fan when you get a bunch of threads hitting d_invalidate()
in parallel on the same directory.  "Let's just go away and expect that
eventually they'll get evicted" is not a fix.  Each of them picks one
victim (after having skipped everything claimed by others), then proudly
disposes of it and repeats everything from scratch.  Worse, if everything
_is_ claimed, we'll just keep rescanning again and again until they go
away.

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(>select.dispose))
__d_drop(data->select.start);
}

It doesn't solve the entire problem, though - we still could get too
many threads into that thing before any of them gets to that __d_drop().
I wonder if we should try and collect more victims; that could lead
to contention of its own, but...

Anyway, could you try the one-liner as above and see what it does to your
reproducer?  I.e.
diff --git a/fs/dcache.c b/fs/dcache.c
index cddf39777835..21f8dd0002a6 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(>select.dispose))
__d_drop(data->select.start);
 }
 

Re: Hang/soft lockup in d_invalidate with simultaneous calls

[Al Viro]

On Wed, May 17, 2017 at 02:58:11PM -0700, Khazhismel Kumykov wrote:

> Once the dentry is on a shrink list would
> it be unreachable anyways,

Why would it be?  Suppose e.g. shrink_dcache_parent() finds a dentry with
zero refcount; to the shrink list it goes, right?  Then, before we actually
get around to evicting it, somebody goes and looks it up and incrementes
refcount.  It's still on the shrink list at that point; whoever had done
that lookup has no way to safely remove the sucker from that - only the
owner of shrink list could do that.  And that's precisely what happens
once that owner gets around to shrink_dentry_list():
d_shrink_del(dentry);

/*
 * We found an inuse dentry which was not removed from
 * the LRU because of laziness during lookup. Do not free it.
 */
if (dentry->d_lockref.count > 0) {
spin_unlock(>d_lock);
if (parent)
spin_unlock(>d_lock);
continue;
}
and we are done with it.

dentry being on a shrink list is *NOT* unreachable; it might have been already
looked up since it had been placed there and it might be looked up at any point
up until shrink_dentry_list() gets to it.

We really can't quit d_invalidate() until all those dentries are done with.
The shit hits the fan when you get a bunch of threads hitting d_invalidate()
in parallel on the same directory.  "Let's just go away and expect that
eventually they'll get evicted" is not a fix.  Each of them picks one
victim (after having skipped everything claimed by others), then proudly
disposes of it and repeats everything from scratch.  Worse, if everything
_is_ claimed, we'll just keep rescanning again and again until they go
away.

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(>select.dispose))
__d_drop(data->select.start);
}

It doesn't solve the entire problem, though - we still could get too
many threads into that thing before any of them gets to that __d_drop().
I wonder if we should try and collect more victims; that could lead
to contention of its own, but...

Anyway, could you try the one-liner as above and see what it does to your
reproducer?  I.e.
diff --git a/fs/dcache.c b/fs/dcache.c
index cddf39777835..21f8dd0002a6 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(>select.dispose))
__d_drop(data->select.start);
 }
 

Re: Hang/soft lockup in d_invalidate with simultaneous calls

[Khazhismel Kumykov]

On Mon, May 22, 2017 at 11:18 AM, Khazhismel Kumykov  wrote:
> On Wed, May 17, 2017 at 2:58 PM Khazhismel Kumykov  wrote:
>>
>> On Mon, May 15, 2017 at 5:05 PM, Khazhismel Kumykov  
>> wrote:
>> > Hi,
>> >
>> > I'm seeing behavior in d_invalidate, if multiple threads call d_invalidate 
>> > on
>> > the same tree at the same, behavior time blows up and all the calls hang 
>> > with
>> > large enough trees/enough simultaneous callers. (e.g. a directory w/ 100k
>> > entries in d_subdir, and 5 or so threads calling d_invalidate was able to 
>> > hang
>> > my test VMs)
>> >
>> > This seems to be due to thrashing on the dentry locks in multiple threads
>> > tightly looping calling d_walk waiting for a shrink_dentry_list call (also
>> > thrashing the locks) to complete. (d_invalidate loops calling d_walk so 
>> > long as
>> > any dentry in the tree is in a dcache shrink list).
>> >
>> > There was a similar report recently "soft lockup in d_invalidate()" that
>> > proposed in the d_invalidate d_walk to ignore dentries marked as in a 
>> > shrink
>> > list already, which does prevent this hang/lockup in this case as well, 
>> > although
>> > I'm not sure it doesn't violate the contract for d_invalidate. (Although 
>> > the
>> > entries in a shrink list should be dropped eventually, not necessarily by 
>> > the
>> > time d_invalidate returns).
>> >
>> > If someone more familiar with the dcache could provide input I would 
>> > appreciate.
>> >
>> > A reliable repro on mainline is:
>> >  - create a mountpoint with DCACHE_OP_REVALIDATE, e.g. fuse passthrough
>> >  - create a directory and populate with ~100k files with content to
>> > populate dcache
>> >  - create some background processes opening/reading files in this folder (5
>> >   while true; cat $file was enough to get an indefinite hang for me)
>> >  - cause the directory to need to be invalidated (e.g., make_bad_inode on 
>> > the
>> > directory)
>> >
>> > This results in the background processes all entering d_invalidate and 
>> > hanging,
>> > while with just one process in d_invalidate (e.g., stat'ing a file in the 
>> > dir)
>> > things go pretty quickly as expected.
>> >
>> >
>> > (The proposed patch from other thread)
>> >
>> > diff --git a/fs/dcache.c b/fs/dcache.c
>> > index 7b8feb6..3a3b0f37 100644
>> > --- a/fs/dcache.c
>> > +++ b/fs/dcache.c
>> > @@ -1364,7 +1364,7 @@ static enum d_walk_ret select_collect(void *_data,
>> > struct dentry *dentry)
>> >  goto out;
>> >
>> >  if (dentry->d_flags & DCACHE_SHRINK_LIST) {
>> > -   data->found++;
>> > +   goto out;
>> >  } else {
>> >  if (dentry->d_flags & DCACHE_LRU_LIST)
>> >  d_lru_del(dentry);
>> >
>> >
>> > khazhy
>>
>> Would this change actually violate any guarantees? select_collect
>> looks like it used to ignore unused dentries that were part of a
>> shrink list before fe91522a7ba82ca1a51b07e19954b3825e4aaa22 (found
>> would not be incremented). Once the dentry is on a shrink list would
>> it be unreachable anyways, so returning from d_invalidate before all
>> the shrink lists finish processing would be ok?
>>
>> khazhy
>
> Pinging in case this got missed, would appreciate thoughts from someone more
> familiar with the dcache.
>
> My previous email wasn't completely correct, while before fe91522a7ba8
> ("don't remove from shrink list in select_collect()") d_invalidate
> would not busy
> wait for other workers calling shrink_list to compete, it would return -EBUSY,
> rather than success, so a change to return success without waiting would not
> be equivalent behavior before. Currently, we will loop calling d_walk 
> repeatedly
> causing the extreme slowdown observed.
>
> I still want to understand better, in d_invalidate if we can return
> without pruning
> in-use dentries safely, would returning before unused dentries are
> pruned, so long
> as we know they will be pruned by another task (are on a shrink list),
> be safe as well?
> If not, would it make more sense to have have a mutual exclusion on calling
> d_invalidate on the same dentries simultaneously?
>
> khazhy

ping

Again to summarize:
With a directory with large number of children, which makes a dentry with a
large number dentries in d_subdir, simultaneous calls to d_invalidate on that
dentry take a very long time. As an example, a directory with 160k files,
where a single d_invalidate call took ~1.4s, having 6 tasks call d_invalidate
simultaneously took ~6.5 hours to resolve, about 1x longer.

dir/
  dir/file-0
  ...
  dir/file-16

This seems to be due to contention between d_walk and shrink_dentry_list,
and particularly d_invalidate tightly looping d_walk so long as any dentry
it finds is in a shrink list. With this particular directory
structure, d_walk will
hold d_lock for "dir" while walking over d_subdir, meanwhile
shrink_dentry_list will release and regrab "dir"s 

Re: Hang/soft lockup in d_invalidate with simultaneous calls

[Khazhismel Kumykov]

On Mon, May 22, 2017 at 11:18 AM, Khazhismel Kumykov  wrote:
> On Wed, May 17, 2017 at 2:58 PM Khazhismel Kumykov  wrote:
>>
>> On Mon, May 15, 2017 at 5:05 PM, Khazhismel Kumykov  
>> wrote:
>> > Hi,
>> >
>> > I'm seeing behavior in d_invalidate, if multiple threads call d_invalidate 
>> > on
>> > the same tree at the same, behavior time blows up and all the calls hang 
>> > with
>> > large enough trees/enough simultaneous callers. (e.g. a directory w/ 100k
>> > entries in d_subdir, and 5 or so threads calling d_invalidate was able to 
>> > hang
>> > my test VMs)
>> >
>> > This seems to be due to thrashing on the dentry locks in multiple threads
>> > tightly looping calling d_walk waiting for a shrink_dentry_list call (also
>> > thrashing the locks) to complete. (d_invalidate loops calling d_walk so 
>> > long as
>> > any dentry in the tree is in a dcache shrink list).
>> >
>> > There was a similar report recently "soft lockup in d_invalidate()" that
>> > proposed in the d_invalidate d_walk to ignore dentries marked as in a 
>> > shrink
>> > list already, which does prevent this hang/lockup in this case as well, 
>> > although
>> > I'm not sure it doesn't violate the contract for d_invalidate. (Although 
>> > the
>> > entries in a shrink list should be dropped eventually, not necessarily by 
>> > the
>> > time d_invalidate returns).
>> >
>> > If someone more familiar with the dcache could provide input I would 
>> > appreciate.
>> >
>> > A reliable repro on mainline is:
>> >  - create a mountpoint with DCACHE_OP_REVALIDATE, e.g. fuse passthrough
>> >  - create a directory and populate with ~100k files with content to
>> > populate dcache
>> >  - create some background processes opening/reading files in this folder (5
>> >   while true; cat $file was enough to get an indefinite hang for me)
>> >  - cause the directory to need to be invalidated (e.g., make_bad_inode on 
>> > the
>> > directory)
>> >
>> > This results in the background processes all entering d_invalidate and 
>> > hanging,
>> > while with just one process in d_invalidate (e.g., stat'ing a file in the 
>> > dir)
>> > things go pretty quickly as expected.
>> >
>> >
>> > (The proposed patch from other thread)
>> >
>> > diff --git a/fs/dcache.c b/fs/dcache.c
>> > index 7b8feb6..3a3b0f37 100644
>> > --- a/fs/dcache.c
>> > +++ b/fs/dcache.c
>> > @@ -1364,7 +1364,7 @@ static enum d_walk_ret select_collect(void *_data,
>> > struct dentry *dentry)
>> >  goto out;
>> >
>> >  if (dentry->d_flags & DCACHE_SHRINK_LIST) {
>> > -   data->found++;
>> > +   goto out;
>> >  } else {
>> >  if (dentry->d_flags & DCACHE_LRU_LIST)
>> >  d_lru_del(dentry);
>> >
>> >
>> > khazhy
>>
>> Would this change actually violate any guarantees? select_collect
>> looks like it used to ignore unused dentries that were part of a
>> shrink list before fe91522a7ba82ca1a51b07e19954b3825e4aaa22 (found
>> would not be incremented). Once the dentry is on a shrink list would
>> it be unreachable anyways, so returning from d_invalidate before all
>> the shrink lists finish processing would be ok?
>>
>> khazhy
>
> Pinging in case this got missed, would appreciate thoughts from someone more
> familiar with the dcache.
>
> My previous email wasn't completely correct, while before fe91522a7ba8
> ("don't remove from shrink list in select_collect()") d_invalidate
> would not busy
> wait for other workers calling shrink_list to compete, it would return -EBUSY,
> rather than success, so a change to return success without waiting would not
> be equivalent behavior before. Currently, we will loop calling d_walk 
> repeatedly
> causing the extreme slowdown observed.
>
> I still want to understand better, in d_invalidate if we can return
> without pruning
> in-use dentries safely, would returning before unused dentries are
> pruned, so long
> as we know they will be pruned by another task (are on a shrink list),
> be safe as well?
> If not, would it make more sense to have have a mutual exclusion on calling
> d_invalidate on the same dentries simultaneously?
>
> khazhy

ping

Again to summarize:
With a directory with large number of children, which makes a dentry with a
large number dentries in d_subdir, simultaneous calls to d_invalidate on that
dentry take a very long time. As an example, a directory with 160k files,
where a single d_invalidate call took ~1.4s, having 6 tasks call d_invalidate
simultaneously took ~6.5 hours to resolve, about 1x longer.

dir/
  dir/file-0
  ...
  dir/file-16

This seems to be due to contention between d_walk and shrink_dentry_list,
and particularly d_invalidate tightly looping d_walk so long as any dentry
it finds is in a shrink list. With this particular directory
structure, d_walk will
hold d_lock for "dir" while walking over d_subdir, meanwhile
shrink_dentry_list will release and regrab "dir"s d_lock every iteration,
also throwing it at the back of 

Re: Hang/soft lockup in d_invalidate with simultaneous calls

[Khazhismel Kumykov]

On Wed, May 17, 2017 at 2:58 PM Khazhismel Kumykov  wrote:
>
> On Mon, May 15, 2017 at 5:05 PM, Khazhismel Kumykov  wrote:
> > Hi,
> >
> > I'm seeing behavior in d_invalidate, if multiple threads call d_invalidate 
> > on
> > the same tree at the same, behavior time blows up and all the calls hang 
> > with
> > large enough trees/enough simultaneous callers. (e.g. a directory w/ 100k
> > entries in d_subdir, and 5 or so threads calling d_invalidate was able to 
> > hang
> > my test VMs)
> >
> > This seems to be due to thrashing on the dentry locks in multiple threads
> > tightly looping calling d_walk waiting for a shrink_dentry_list call (also
> > thrashing the locks) to complete. (d_invalidate loops calling d_walk so 
> > long as
> > any dentry in the tree is in a dcache shrink list).
> >
> > There was a similar report recently "soft lockup in d_invalidate()" that
> > proposed in the d_invalidate d_walk to ignore dentries marked as in a shrink
> > list already, which does prevent this hang/lockup in this case as well, 
> > although
> > I'm not sure it doesn't violate the contract for d_invalidate. (Although the
> > entries in a shrink list should be dropped eventually, not necessarily by 
> > the
> > time d_invalidate returns).
> >
> > If someone more familiar with the dcache could provide input I would 
> > appreciate.
> >
> > A reliable repro on mainline is:
> >  - create a mountpoint with DCACHE_OP_REVALIDATE, e.g. fuse passthrough
> >  - create a directory and populate with ~100k files with content to
> > populate dcache
> >  - create some background processes opening/reading files in this folder (5
> >   while true; cat $file was enough to get an indefinite hang for me)
> >  - cause the directory to need to be invalidated (e.g., make_bad_inode on 
> > the
> > directory)
> >
> > This results in the background processes all entering d_invalidate and 
> > hanging,
> > while with just one process in d_invalidate (e.g., stat'ing a file in the 
> > dir)
> > things go pretty quickly as expected.
> >
> >
> > (The proposed patch from other thread)
> >
> > diff --git a/fs/dcache.c b/fs/dcache.c
> > index 7b8feb6..3a3b0f37 100644
> > --- a/fs/dcache.c
> > +++ b/fs/dcache.c
> > @@ -1364,7 +1364,7 @@ static enum d_walk_ret select_collect(void *_data,
> > struct dentry *dentry)
> >  goto out;
> >
> >  if (dentry->d_flags & DCACHE_SHRINK_LIST) {
> > -   data->found++;
> > +   goto out;
> >  } else {
> >  if (dentry->d_flags & DCACHE_LRU_LIST)
> >  d_lru_del(dentry);
> >
> >
> > khazhy
>
> Would this change actually violate any guarantees? select_collect
> looks like it used to ignore unused dentries that were part of a
> shrink list before fe91522a7ba82ca1a51b07e19954b3825e4aaa22 (found
> would not be incremented). Once the dentry is on a shrink list would
> it be unreachable anyways, so returning from d_invalidate before all
> the shrink lists finish processing would be ok?
>
> khazhy

Pinging in case this got missed, would appreciate thoughts from someone more
familiar with the dcache.

My previous email wasn't completely correct, while before fe91522a7ba8
("don't remove from shrink list in select_collect()") d_invalidate
would not busy
wait for other workers calling shrink_list to compete, it would return -EBUSY,
rather than success, so a change to return success without waiting would not
be equivalent behavior before. Currently, we will loop calling d_walk repeatedly
causing the extreme slowdown observed.

I still want to understand better, in d_invalidate if we can return
without pruning
in-use dentries safely, would returning before unused dentries are
pruned, so long
as we know they will be pruned by another task (are on a shrink list),
be safe as well?
If not, would it make more sense to have have a mutual exclusion on calling
d_invalidate on the same dentries simultaneously?

khazhy

Re: Hang/soft lockup in d_invalidate with simultaneous calls

[Khazhismel Kumykov]

On Wed, May 17, 2017 at 2:58 PM Khazhismel Kumykov  wrote:
>
> On Mon, May 15, 2017 at 5:05 PM, Khazhismel Kumykov  wrote:
> > Hi,
> >
> > I'm seeing behavior in d_invalidate, if multiple threads call d_invalidate 
> > on
> > the same tree at the same, behavior time blows up and all the calls hang 
> > with
> > large enough trees/enough simultaneous callers. (e.g. a directory w/ 100k
> > entries in d_subdir, and 5 or so threads calling d_invalidate was able to 
> > hang
> > my test VMs)
> >
> > This seems to be due to thrashing on the dentry locks in multiple threads
> > tightly looping calling d_walk waiting for a shrink_dentry_list call (also
> > thrashing the locks) to complete. (d_invalidate loops calling d_walk so 
> > long as
> > any dentry in the tree is in a dcache shrink list).
> >
> > There was a similar report recently "soft lockup in d_invalidate()" that
> > proposed in the d_invalidate d_walk to ignore dentries marked as in a shrink
> > list already, which does prevent this hang/lockup in this case as well, 
> > although
> > I'm not sure it doesn't violate the contract for d_invalidate. (Although the
> > entries in a shrink list should be dropped eventually, not necessarily by 
> > the
> > time d_invalidate returns).
> >
> > If someone more familiar with the dcache could provide input I would 
> > appreciate.
> >
> > A reliable repro on mainline is:
> >  - create a mountpoint with DCACHE_OP_REVALIDATE, e.g. fuse passthrough
> >  - create a directory and populate with ~100k files with content to
> > populate dcache
> >  - create some background processes opening/reading files in this folder (5
> >   while true; cat $file was enough to get an indefinite hang for me)
> >  - cause the directory to need to be invalidated (e.g., make_bad_inode on 
> > the
> > directory)
> >
> > This results in the background processes all entering d_invalidate and 
> > hanging,
> > while with just one process in d_invalidate (e.g., stat'ing a file in the 
> > dir)
> > things go pretty quickly as expected.
> >
> >
> > (The proposed patch from other thread)
> >
> > diff --git a/fs/dcache.c b/fs/dcache.c
> > index 7b8feb6..3a3b0f37 100644
> > --- a/fs/dcache.c
> > +++ b/fs/dcache.c
> > @@ -1364,7 +1364,7 @@ static enum d_walk_ret select_collect(void *_data,
> > struct dentry *dentry)
> >  goto out;
> >
> >  if (dentry->d_flags & DCACHE_SHRINK_LIST) {
> > -   data->found++;
> > +   goto out;
> >  } else {
> >  if (dentry->d_flags & DCACHE_LRU_LIST)
> >  d_lru_del(dentry);
> >
> >
> > khazhy
>
> Would this change actually violate any guarantees? select_collect
> looks like it used to ignore unused dentries that were part of a
> shrink list before fe91522a7ba82ca1a51b07e19954b3825e4aaa22 (found
> would not be incremented). Once the dentry is on a shrink list would
> it be unreachable anyways, so returning from d_invalidate before all
> the shrink lists finish processing would be ok?
>
> khazhy

Pinging in case this got missed, would appreciate thoughts from someone more
familiar with the dcache.

My previous email wasn't completely correct, while before fe91522a7ba8
("don't remove from shrink list in select_collect()") d_invalidate
would not busy
wait for other workers calling shrink_list to compete, it would return -EBUSY,
rather than success, so a change to return success without waiting would not
be equivalent behavior before. Currently, we will loop calling d_walk repeatedly
causing the extreme slowdown observed.

I still want to understand better, in d_invalidate if we can return
without pruning
in-use dentries safely, would returning before unused dentries are
pruned, so long
as we know they will be pruned by another task (are on a shrink list),
be safe as well?
If not, would it make more sense to have have a mutual exclusion on calling
d_invalidate on the same dentries simultaneously?

khazhy

Re: Hang/soft lockup in d_invalidate with simultaneous calls

[Khazhismel Kumykov]

On Mon, May 15, 2017 at 5:05 PM, Khazhismel Kumykov  wrote:
> Hi,
>
> I'm seeing behavior in d_invalidate, if multiple threads call d_invalidate on
> the same tree at the same, behavior time blows up and all the calls hang with
> large enough trees/enough simultaneous callers. (e.g. a directory w/ 100k
> entries in d_subdir, and 5 or so threads calling d_invalidate was able to hang
> my test VMs)
>
> This seems to be due to thrashing on the dentry locks in multiple threads
> tightly looping calling d_walk waiting for a shrink_dentry_list call (also
> thrashing the locks) to complete. (d_invalidate loops calling d_walk so long 
> as
> any dentry in the tree is in a dcache shrink list).
>
> There was a similar report recently "soft lockup in d_invalidate()" that
> proposed in the d_invalidate d_walk to ignore dentries marked as in a shrink
> list already, which does prevent this hang/lockup in this case as well, 
> although
> I'm not sure it doesn't violate the contract for d_invalidate. (Although the
> entries in a shrink list should be dropped eventually, not necessarily by the
> time d_invalidate returns).
>
> If someone more familiar with the dcache could provide input I would 
> appreciate.
>
> A reliable repro on mainline is:
>  - create a mountpoint with DCACHE_OP_REVALIDATE, e.g. fuse passthrough
>  - create a directory and populate with ~100k files with content to
> populate dcache
>  - create some background processes opening/reading files in this folder (5
>   while true; cat $file was enough to get an indefinite hang for me)
>  - cause the directory to need to be invalidated (e.g., make_bad_inode on the
> directory)
>
> This results in the background processes all entering d_invalidate and 
> hanging,
> while with just one process in d_invalidate (e.g., stat'ing a file in the dir)
> things go pretty quickly as expected.
>
>
> (The proposed patch from other thread)
>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 7b8feb6..3a3b0f37 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -1364,7 +1364,7 @@ static enum d_walk_ret select_collect(void *_data,
> struct dentry *dentry)
>  goto out;
>
>  if (dentry->d_flags & DCACHE_SHRINK_LIST) {
> -   data->found++;
> +   goto out;
>  } else {
>  if (dentry->d_flags & DCACHE_LRU_LIST)
>  d_lru_del(dentry);
>
>
> khazhy

Would this change actually violate any guarantees? select_collect
looks like it used to ignore unused dentries that were part of a
shrink list before fe91522a7ba82ca1a51b07e19954b3825e4aaa22 (found
would not be incremented). Once the dentry is on a shrink list would
it be unreachable anyways, so returning from d_invalidate before all
the shrink lists finish processing would be ok?

khazhy


smime.p7s
Description: S/MIME Cryptographic Signature

Re: Hang/soft lockup in d_invalidate with simultaneous calls

[Khazhismel Kumykov]

On Mon, May 15, 2017 at 5:05 PM, Khazhismel Kumykov  wrote:
> Hi,
>
> I'm seeing behavior in d_invalidate, if multiple threads call d_invalidate on
> the same tree at the same, behavior time blows up and all the calls hang with
> large enough trees/enough simultaneous callers. (e.g. a directory w/ 100k
> entries in d_subdir, and 5 or so threads calling d_invalidate was able to hang
> my test VMs)
>
> This seems to be due to thrashing on the dentry locks in multiple threads
> tightly looping calling d_walk waiting for a shrink_dentry_list call (also
> thrashing the locks) to complete. (d_invalidate loops calling d_walk so long 
> as
> any dentry in the tree is in a dcache shrink list).
>
> There was a similar report recently "soft lockup in d_invalidate()" that
> proposed in the d_invalidate d_walk to ignore dentries marked as in a shrink
> list already, which does prevent this hang/lockup in this case as well, 
> although
> I'm not sure it doesn't violate the contract for d_invalidate. (Although the
> entries in a shrink list should be dropped eventually, not necessarily by the
> time d_invalidate returns).
>
> If someone more familiar with the dcache could provide input I would 
> appreciate.
>
> A reliable repro on mainline is:
>  - create a mountpoint with DCACHE_OP_REVALIDATE, e.g. fuse passthrough
>  - create a directory and populate with ~100k files with content to
> populate dcache
>  - create some background processes opening/reading files in this folder (5
>   while true; cat $file was enough to get an indefinite hang for me)
>  - cause the directory to need to be invalidated (e.g., make_bad_inode on the
> directory)
>
> This results in the background processes all entering d_invalidate and 
> hanging,
> while with just one process in d_invalidate (e.g., stat'ing a file in the dir)
> things go pretty quickly as expected.
>
>
> (The proposed patch from other thread)
>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 7b8feb6..3a3b0f37 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -1364,7 +1364,7 @@ static enum d_walk_ret select_collect(void *_data,
> struct dentry *dentry)
>  goto out;
>
>  if (dentry->d_flags & DCACHE_SHRINK_LIST) {
> -   data->found++;
> +   goto out;
>  } else {
>  if (dentry->d_flags & DCACHE_LRU_LIST)
>  d_lru_del(dentry);
>
>
> khazhy

Would this change actually violate any guarantees? select_collect
looks like it used to ignore unused dentries that were part of a
shrink list before fe91522a7ba82ca1a51b07e19954b3825e4aaa22 (found
would not be incremented). Once the dentry is on a shrink list would
it be unreachable anyways, so returning from d_invalidate before all
the shrink lists finish processing would be ok?

khazhy


smime.p7s
Description: S/MIME Cryptographic Signature

Hang/soft lockup in d_invalidate with simultaneous calls

[Khazhismel Kumykov]

Hi,

I'm seeing behavior in d_invalidate, if multiple threads call d_invalidate on
the same tree at the same, behavior time blows up and all the calls hang with
large enough trees/enough simultaneous callers. (e.g. a directory w/ 100k
entries in d_subdir, and 5 or so threads calling d_invalidate was able to hang
my test VMs)

This seems to be due to thrashing on the dentry locks in multiple threads
tightly looping calling d_walk waiting for a shrink_dentry_list call (also
thrashing the locks) to complete. (d_invalidate loops calling d_walk so long as
any dentry in the tree is in a dcache shrink list).

There was a similar report recently "soft lockup in d_invalidate()" that
proposed in the d_invalidate d_walk to ignore dentries marked as in a shrink
list already, which does prevent this hang/lockup in this case as well, although
I'm not sure it doesn't violate the contract for d_invalidate. (Although the
entries in a shrink list should be dropped eventually, not necessarily by the
time d_invalidate returns).

If someone more familiar with the dcache could provide input I would appreciate.

A reliable repro on mainline is:
 - create a mountpoint with DCACHE_OP_REVALIDATE, e.g. fuse passthrough
 - create a directory and populate with ~100k files with content to
populate dcache
 - create some background processes opening/reading files in this folder (5
  while true; cat $file was enough to get an indefinite hang for me)
 - cause the directory to need to be invalidated (e.g., make_bad_inode on the
directory)

This results in the background processes all entering d_invalidate and hanging,
while with just one process in d_invalidate (e.g., stat'ing a file in the dir)
things go pretty quickly as expected.


(The proposed patch from other thread)

diff --git a/fs/dcache.c b/fs/dcache.c
index 7b8feb6..3a3b0f37 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1364,7 +1364,7 @@ static enum d_walk_ret select_collect(void *_data,
struct dentry *dentry)
 goto out;

 if (dentry->d_flags & DCACHE_SHRINK_LIST) {
-   data->found++;
+   goto out;
 } else {
 if (dentry->d_flags & DCACHE_LRU_LIST)
 d_lru_del(dentry);


khazhy


smime.p7s
Description: S/MIME Cryptographic Signature

Hang/soft lockup in d_invalidate with simultaneous calls

[Khazhismel Kumykov]

Hi,

I'm seeing behavior in d_invalidate, if multiple threads call d_invalidate on
the same tree at the same, behavior time blows up and all the calls hang with
large enough trees/enough simultaneous callers. (e.g. a directory w/ 100k
entries in d_subdir, and 5 or so threads calling d_invalidate was able to hang
my test VMs)

This seems to be due to thrashing on the dentry locks in multiple threads
tightly looping calling d_walk waiting for a shrink_dentry_list call (also
thrashing the locks) to complete. (d_invalidate loops calling d_walk so long as
any dentry in the tree is in a dcache shrink list).

There was a similar report recently "soft lockup in d_invalidate()" that
proposed in the d_invalidate d_walk to ignore dentries marked as in a shrink
list already, which does prevent this hang/lockup in this case as well, although
I'm not sure it doesn't violate the contract for d_invalidate. (Although the
entries in a shrink list should be dropped eventually, not necessarily by the
time d_invalidate returns).

If someone more familiar with the dcache could provide input I would appreciate.

A reliable repro on mainline is:
 - create a mountpoint with DCACHE_OP_REVALIDATE, e.g. fuse passthrough
 - create a directory and populate with ~100k files with content to
populate dcache
 - create some background processes opening/reading files in this folder (5
  while true; cat $file was enough to get an indefinite hang for me)
 - cause the directory to need to be invalidated (e.g., make_bad_inode on the
directory)

This results in the background processes all entering d_invalidate and hanging,
while with just one process in d_invalidate (e.g., stat'ing a file in the dir)
things go pretty quickly as expected.


(The proposed patch from other thread)

diff --git a/fs/dcache.c b/fs/dcache.c
index 7b8feb6..3a3b0f37 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1364,7 +1364,7 @@ static enum d_walk_ret select_collect(void *_data,
struct dentry *dentry)
 goto out;

 if (dentry->d_flags & DCACHE_SHRINK_LIST) {
-   data->found++;
+   goto out;
 } else {
 if (dentry->d_flags & DCACHE_LRU_LIST)
 d_lru_del(dentry);


khazhy


smime.p7s
Description: S/MIME Cryptographic Signature