linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Hang/soft lockup in d_invalidate with simultaneous calls
@ 2017-05-16  0:05 Khazhismel Kumykov
  2017-05-17 21:58 ` Khazhismel Kumykov
  0 siblings, 1 reply; 10+ messages in thread
From: Khazhismel Kumykov @ 2017-05-16  0:05 UTC (permalink / raw)
  To: linux-kernel, linux-fsdevel

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

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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4843 bytes --]

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: Hang/soft lockup in d_invalidate with simultaneous calls
  2017-05-16  0:05 Hang/soft lockup in d_invalidate with simultaneous calls Khazhismel Kumykov
@ 2017-05-17 21:58 ` Khazhismel Kumykov
  2017-05-22 18:18   ` Khazhismel Kumykov
  2017-06-03  1:12   ` Al Viro
  0 siblings, 2 replies; 10+ messages in thread
From: Khazhismel Kumykov @ 2017-05-17 21:58 UTC (permalink / raw)
  To: linux-kernel, linux-fsdevel, viro

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

On Mon, May 15, 2017 at 5:05 PM, Khazhismel Kumykov <khazhy@google.com> 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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4843 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Hang/soft lockup in d_invalidate with simultaneous calls
  2017-05-17 21:58 ` Khazhismel Kumykov
@ 2017-05-22 18:18   ` Khazhismel Kumykov
  2017-05-25 22:31     ` Khazhismel Kumykov
  2017-06-03  1:12   ` Al Viro
  1 sibling, 1 reply; 10+ messages in thread
From: Khazhismel Kumykov @ 2017-05-22 18:18 UTC (permalink / raw)
  To: linux-kernel, linux-fsdevel, viro, ebiederm, miklos

On Wed, May 17, 2017 at 2:58 PM Khazhismel Kumykov <khazhy@google.com> wrote:
>
> On Mon, May 15, 2017 at 5:05 PM, Khazhismel Kumykov <khazhy@google.com> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Hang/soft lockup in d_invalidate with simultaneous calls
  2017-05-22 18:18   ` Khazhismel Kumykov
@ 2017-05-25 22:31     ` Khazhismel Kumykov
  0 siblings, 0 replies; 10+ messages in thread
From: Khazhismel Kumykov @ 2017-05-25 22:31 UTC (permalink / raw)
  To: linux-kernel, linux-fsdevel, viro, ebiederm, miklos, akpm

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

On Mon, May 22, 2017 at 11:18 AM, Khazhismel Kumykov <khazhy@google.com> wrote:
> On Wed, May 17, 2017 at 2:58 PM Khazhismel Kumykov <khazhy@google.com> wrote:
>>
>> On Mon, May 15, 2017 at 5:05 PM, Khazhismel Kumykov <khazhy@google.com> 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 10000x longer.

dir/
  dir/file-0
  ...
  dir/file-160000

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 the queue.

One proposed solution is in select_collect, do not increment the
number of found unused descendants when we find a dentry in a
shrink list. This proposal will result in d_invalidate and shrink_dcache_parent
returning before necessarily all unused children are pruned, but they will
be pruned at some time soon by the other task currently shrinking, which
I am not sure if that is OK or not.

A quick and dirty fix would be adding a mutex around the shrink loops,
so simultaneous tasks don't thrash with each other.

There are probably also other solutions here, I would appreciate a look
from someone more familiar with this area.

khazhy

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4843 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Hang/soft lockup in d_invalidate with simultaneous calls
  2017-05-17 21:58 ` Khazhismel Kumykov
  2017-05-22 18:18   ` Khazhismel Kumykov
@ 2017-06-03  1:12   ` Al Viro
  2017-06-03  5:22     ` Khazhismel Kumykov
  1 sibling, 1 reply; 10+ messages in thread
From: Al Viro @ 2017-06-03  1:12 UTC (permalink / raw)
  To: Khazhismel Kumykov; +Cc: linux-kernel, linux-fsdevel, Miklos Szeredi

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);
 }
 

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: Hang/soft lockup in d_invalidate with simultaneous calls
  2017-06-03  1:12   ` Al Viro
@ 2017-06-03  5:22     ` Khazhismel Kumykov
  2017-06-03  6:20       ` Al Viro
  0 siblings, 1 reply; 10+ messages in thread
From: Khazhismel Kumykov @ 2017-06-03  5:22 UTC (permalink / raw)
  To: Al Viro; +Cc: linux-kernel, linux-fsdevel, Miklos Szeredi

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

On Fri, Jun 2, 2017 at 6:12 PM, Al Viro <viro@zeniv.linux.org.uk> 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);

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4843 bytes --]

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: Hang/soft lockup in d_invalidate with simultaneous calls
  2017-06-03  5:22     ` Khazhismel Kumykov
@ 2017-06-03  6:20       ` Al Viro
  2017-06-03  6:47         ` Khazhismel Kumykov
  0 siblings, 1 reply; 10+ messages in thread
From: Al Viro @ 2017-06-03  6:20 UTC (permalink / raw)
  To: Khazhismel Kumykov; +Cc: linux-kernel, linux-fsdevel, Miklos Szeredi

On Fri, Jun 02, 2017 at 10:22:39PM -0700, Khazhismel Kumykov wrote:
> On Fri, Jun 2, 2017 at 6:12 PM, Al Viro <viro@zeniv.linux.org.uk> 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();
 	}
 }

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: Hang/soft lockup in d_invalidate with simultaneous calls
  2017-06-03  6:20       ` Al Viro
@ 2017-06-03  6:47         ` Khazhismel Kumykov
  2017-06-12 23:00           ` Khazhismel Kumykov
  0 siblings, 1 reply; 10+ messages in thread
From: Khazhismel Kumykov @ 2017-06-03  6:47 UTC (permalink / raw)
  To: Al Viro; +Cc: linux-kernel, linux-fsdevel, Miklos Szeredi

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

On Fri, Jun 2, 2017 at 11:20 PM, Al Viro <viro@zeniv.linux.org.uk> 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.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4843 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Hang/soft lockup in d_invalidate with simultaneous calls
  2017-06-03  6:47         ` Khazhismel Kumykov
@ 2017-06-12 23:00           ` Khazhismel Kumykov
  2017-06-15 10:50             ` Al Viro
  0 siblings, 1 reply; 10+ messages in thread
From: Khazhismel Kumykov @ 2017-06-12 23:00 UTC (permalink / raw)
  To: Al Viro; +Cc: linux-kernel, linux-fsdevel, Miklos Szeredi

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

On Fri, Jun 2, 2017 at 11:47 PM, Khazhismel Kumykov <khazhy@google.com> wrote:
> On Fri, Jun 2, 2017 at 11:20 PM, Al Viro <viro@zeniv.linux.org.uk> 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?

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4843 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Hang/soft lockup in d_invalidate with simultaneous calls
  2017-06-12 23:00           ` Khazhismel Kumykov
@ 2017-06-15 10:50             ` Al Viro
  0 siblings, 0 replies; 10+ messages in thread
From: Al Viro @ 2017-06-15 10:50 UTC (permalink / raw)
  To: Khazhismel Kumykov; +Cc: linux-kernel, linux-fsdevel, Miklos Szeredi

On Mon, Jun 12, 2017 at 04:00:45PM -0700, Khazhismel Kumykov wrote:
> On Fri, Jun 2, 2017 at 11:47 PM, Khazhismel Kumykov <khazhy@google.com> wrote:
> > On Fri, Jun 2, 2017 at 11:20 PM, Al Viro <viro@zeniv.linux.org.uk> 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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2017-06-15 10:51 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-16  0:05 Hang/soft lockup in d_invalidate with simultaneous calls Khazhismel Kumykov
2017-05-17 21:58 ` Khazhismel Kumykov
2017-05-22 18:18   ` Khazhismel Kumykov
2017-05-25 22:31     ` Khazhismel Kumykov
2017-06-03  1:12   ` Al Viro
2017-06-03  5:22     ` Khazhismel Kumykov
2017-06-03  6:20       ` Al Viro
2017-06-03  6:47         ` Khazhismel Kumykov
2017-06-12 23:00           ` Khazhismel Kumykov
2017-06-15 10:50             ` Al Viro

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).