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(&dentry->d_lock);
if (parent)
spin_unlock(&parent->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(&data->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(&data->select.dispose))
__d_drop(data->select.start);
}
On Fri, Jun 2, 2017 at 6:12 PM, Al Viro <[email protected]> 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?
>
> 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, &data->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(&data->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.dispose);
data.start = parent;
data.found = 0;
+ data.actually_found = 0;
d_walk(parent, &data, select_collect, NULL);
if (!data.found)
@@ -1536,6 +1542,7 @@ void d_invalidate(struct dentry *dentry)
INIT_LIST_HEAD(&data.select.dispose);
data.select.start = dentry;
data.select.found = 0;
+ data.select.actually_found = 0;
d_walk(dentry, &data, detach_and_collect, check_and_drop);
On Fri, Jun 02, 2017 at 10:22:39PM -0700, Khazhismel Kumykov wrote:
> On Fri, Jun 2, 2017 at 6:12 PM, Al Viro <[email protected]> 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:
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();
}
}
On Fri, Jun 2, 2017 at 11:20 PM, Al Viro <[email protected]> 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.
On Fri, Jun 2, 2017 at 11:47 PM, Khazhismel Kumykov <[email protected]> wrote:
> On Fri, Jun 2, 2017 at 11:20 PM, Al Viro <[email protected]> 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?
On Mon, Jun 12, 2017 at 04:00:45PM -0700, Khazhismel Kumykov wrote:
> On Fri, Jun 2, 2017 at 11:47 PM, Khazhismel Kumykov <[email protected]> wrote:
> > On Fri, Jun 2, 2017 at 11:20 PM, Al Viro <[email protected]> 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.