From: Eric W. Biederman <hidden> Date: 2020-02-12 19:18:42
Linus Torvalds [off-list ref] writes:
On Wed, Feb 12, 2020 at 7:01 AM Eric W. Biederman [off-list ref] wrote:
quoted
Fundamentally proc_flush_task is an optimization. Just getting rid of
dentries earlier. At least at one point it was an important
optimization because the old process dentries would just sit around
doing nothing for anyone.
I'm pretty sure it's still important. It's very easy to generate a
_ton_ of dentries with /proc.
quoted
I wonder if instead of invalidating specific dentries we could instead
fire wake up a shrinker and point it at one or more instances of proc.
It shouldn't be the dentries themselves that are a freeing problem.
They're being RCU-free'd anyway because of lookup. It's the
proc_mounts list that is the problem, isn't it?
So it's just fs_info that needs to be rcu-delayed because it contains
that list. Or is there something else?
The fundamental dcache thing we are playing with is:
dentry = d_hash_and_lookup(proc_root, &name);
if (dentry) {
d_invalidate(dentry);
dput(dentry);
}
As Al pointed out upthread dput and d_invalidate can both sleep.
The dput can potentially go away if we use __d_lookup_rcu instead of
d_lookup.
The challenge is d_invalidate.
It has the fundamentally sleeping detach_mounts loop. Even
shrink_dcache_parent has a cond_sched() in there to ensure it doesn't
live lock the system.
We could and arguabley should set DCACHE_CANT_MOUNT on the proc pid
dentries. Which will prevent having to deal with mounts.
But I don't see an easy way of getting shrink_dcache_parent to run
without sleeping. Ideas?
Eric
On Wed, Feb 12, 2020 at 11:18 AM Eric W. Biederman
[off-list ref] wrote:
quoted
So it's just fs_info that needs to be rcu-delayed because it contains
that list. Or is there something else?
The fundamental dcache thing we are playing with is:
dentry = d_hash_and_lookup(proc_root, &name);
if (dentry) {
d_invalidate(dentry);
dput(dentry);
}
Ahh. And we can't do that part under the RCU read lock. So it's not
the freeing, it's the list traversal itself.
Fair enough.
Hmm.
I wonder if we could split up d_invalidate(). It already ends up being
two phases: first the unhashing under the d_lock, and then the
recursive shrinking of parents and children.
The recursive shrinking of the parent isn't actually interesting for
the proc shrinking case: we just looked up one child, after all. So we
only care about the d_walk of the children.
So if we only did the first part under the RCU lock, and just
collected the dentries (can we perhaps then re-use the hash list to
collect them to another list?) and then did the child d_walk
afterwards?
Linus
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2020-02-12 20:03:44
On Wed, Feb 12, 2020 at 11:49:58AM -0800, Linus Torvalds wrote:
I wonder if we could split up d_invalidate(). It already ends up being
two phases: first the unhashing under the d_lock, and then the
recursive shrinking of parents and children.
The recursive shrinking of the parent isn't actually interesting for
the proc shrinking case: we just looked up one child, after all. So we
only care about the d_walk of the children.
So if we only did the first part under the RCU lock, and just
collected the dentries (can we perhaps then re-use the hash list to
collect them to another list?) and then did the child d_walk
afterwards?
What's to prevent racing with fs shutdown while you are doing the second part?
We could, after all, just have them[*] on procfs-private list (anchored in
task_struct) from the very beginning; evict on ->d_prune(), walk the list
on exit... How do you make sure the fs instance won't go away right under
you while you are doing the real work? Suppose you are looking at one
of those dentries and you've found something blocking to do. You can't
pin that dentry; you can pin ->s_active on its superblock (if it's already
zero, you can skip it - fs shutdown already in progress will take care of
the damn thing), but that will lead to quite a bit of cacheline pingpong...
[*] only /proc/<pid> and /proc/*/task/<pid> dentries, obviously.
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2020-02-12 20:38:44
On Wed, Feb 12, 2020 at 12:35:04PM -0800, Linus Torvalds wrote:
On Wed, Feb 12, 2020 at 12:03 PM Al Viro [off-list ref] wrote:
quoted
What's to prevent racing with fs shutdown while you are doing the second part?
I was thinking that only the proc_flush_task() code would do this.
And that holds a ref to the vfsmount through upid->ns.
So I wasn't suggesting doing this in general - just splitting up the
implementation of d_invalidate() so that proc_flush_task_mnt() could
delay the complex part to after having traversed the RCU-protected
list.
But hey - I missed this part of the problem originally, so maybe I'm
just missing something else this time. Wouldn't be the first time.
Wait, I thought the whole point of that had been to allow multiple
procfs instances for the same userns? Confused...
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2020-02-12 20:41:34
On Wed, Feb 12, 2020 at 08:38:33PM +0000, Al Viro wrote:
On Wed, Feb 12, 2020 at 12:35:04PM -0800, Linus Torvalds wrote:
quoted
On Wed, Feb 12, 2020 at 12:03 PM Al Viro [off-list ref] wrote:
quoted
What's to prevent racing with fs shutdown while you are doing the second part?
I was thinking that only the proc_flush_task() code would do this.
And that holds a ref to the vfsmount through upid->ns.
So I wasn't suggesting doing this in general - just splitting up the
implementation of d_invalidate() so that proc_flush_task_mnt() could
delay the complex part to after having traversed the RCU-protected
list.
But hey - I missed this part of the problem originally, so maybe I'm
just missing something else this time. Wouldn't be the first time.
Wait, I thought the whole point of that had been to allow multiple
procfs instances for the same userns? Confused...
On Wed, Feb 12, 2020 at 12:03 PM Al Viro [off-list ref] wrote:
What's to prevent racing with fs shutdown while you are doing the second part?
I was thinking that only the proc_flush_task() code would do this.
And that holds a ref to the vfsmount through upid->ns.
So I wasn't suggesting doing this in general - just splitting up the
implementation of d_invalidate() so that proc_flush_task_mnt() could
delay the complex part to after having traversed the RCU-protected
list.
But hey - I missed this part of the problem originally, so maybe I'm
just missing something else this time. Wouldn't be the first time.
Linus
On Wed, Feb 12, 2020 at 12:41 PM Al Viro [off-list ref] wrote:
On Wed, Feb 12, 2020 at 08:38:33PM +0000, Al Viro wrote:
quoted
Wait, I thought the whole point of that had been to allow multiple
procfs instances for the same userns? Confused...
s/userns/pidns/, sorry
Right, but we still hold the ref to it here...
[ Looks more ]
Oooh. No we don't. Exactly because we don't hold the lock, only the
rcu lifetime, the ref can go away from under us. I see what your
concern is.
Ouch, this is more painful than I expected - the code flow looked so
simple. I really wanted to avoid a new lock during process shutdown,
because that has always been somewhat painful.
Linus