From: Eric W. Biederman <hidden> Date: 2020-02-13 04:39:52
Linus Torvalds [off-list ref] writes:
On Wed, Feb 12, 2020 at 1:48 PM Eric W. Biederman [off-list ref] wrote:
quoted
The good news is proc_flush_task isn't exactly called from process exit.
proc_flush_task is called during zombie clean up. AKA release_task.
Yeah, that at least avoids some of the nasty locking while dying debug problems.
But the one I was more worried about was actually the lock contention
issue with lots of processes. The lock is basically a single global
lock in many situations - yes, it's technically per-ns, but in a lot
of cases you really only have one namespace anyway.
And we've had problems with global locks in this area before, notably
the one you call out:
quoted
Further after proc_flush_task does it's thing the code goes
and does "write_lock_irq(&task_list_lock);"
Yeah, so it's not introducing a new issue, but it is potentially
making something we already know is bad even worse.
quoted
What would be downside of having a mutex for a list of proc superblocks?
A mutex that is taken for both reading and writing the list.
That's what the original patch actually was, and I was hoping we could
avoid that thing.
An rwsem would be possibly better, since most cases by far are likely
about reading.
And yes, I'm very aware of the task_list_lock, but it's literally why
I don't want to make a new one.
I'm _hoping_ we can some day come up with something better than
task_list_lock.
Yes. I understand that.
I occassionally play with ideas, and converted all of proc to rcu
to help with situation but I haven't come up with anything clearly
better.
All of this is why I was really hoping we could have a change in
strategy and see if we can make the shrinker be able to better prune
proc inodes.
I think I have an alternate idea that could work. Add some extra code
into proc_task_readdir, that would look for dentries that no longer
point to tasks and d_invalidate them. With the same logic probably
being called from a few more places as well like proc_pid_readdir,
proc_task_lookup, and proc_pid_lookup.
We could even optimize it and have a process died flag we set in the
superblock.
That would would batch up the freeing work until the next time someone
reads from proc in a way that would create more dentries. So it would
prevent dentries from reaped zombies from growing without bound.
Hmm. Given the existence of proc_fill_cache it would really be a good
idea if readdir and lookup performed some of the freeing work as well.
As on readdir we always populate the dcache for all of the directory
entries.
I am booked solid for the next little while but if no one beats me to it
I will try and code something like that up where at least readdir
looks for and invalidates stale dentries.
Eric
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2020-02-13 05:55:40
On Wed, Feb 12, 2020 at 10:37:52PM -0600, Eric W. Biederman wrote:
I think I have an alternate idea that could work. Add some extra code
into proc_task_readdir, that would look for dentries that no longer
point to tasks and d_invalidate them. With the same logic probably
being called from a few more places as well like proc_pid_readdir,
proc_task_lookup, and proc_pid_lookup.
We could even optimize it and have a process died flag we set in the
superblock.
That would would batch up the freeing work until the next time someone
reads from proc in a way that would create more dentries. So it would
prevent dentries from reaped zombies from growing without bound.
Hmm. Given the existence of proc_fill_cache it would really be a good
idea if readdir and lookup performed some of the freeing work as well.
As on readdir we always populate the dcache for all of the directory
entries.
First of all, that won't do a damn thing when nobody is accessing
given superblock. What's more, readdir in root of that procfs instance
is not enough - you need it in task/ of group leader.
What I don't understand is the insistence on getting those dentries
via dcache lookups. _IF_ we are willing to live with cacheline
contention (on ->d_lock of root dentry, if nothing else), why not
do the following:
* put all dentries of such directories ([0-9]* and [0-9]*/task/*)
into a list anchored in task_struct; have non-counting reference to
task_struct stored in them (might simplify part of get_proc_task() users,
BTW - avoids pid-to-task_struct lookups if we have a dentry and not just
the inode; many callers do)
* have ->d_release() remove from it (protecting per-task_struct lock
nested outside of all ->d_lock)
* on exit:
lock the (per-task_struct) list
while list is non-empty
pick the first dentry
remove from the list
sb = dentry->d_sb
try to bump sb->s_active (if non-zero, that is).
if failed
continue // move on to the next one - nothing to do here
grab ->d_lock
res = handle_it(dentry, &temp_list)
drop ->d_lock
unlock the list
if (!list_empty(&temp_list))
shrink_dentry_list(&temp_list)
if (res)
d_invalidate(dentry)
dput(dentry)
deactivate_super(sb)
lock the list
unlock the list
handle_it(dentry, temp_list) // ->d_lock held; that one should be in dcache.c
if ->d_count is negative // unlikely
return 0;
if ->d_count is positive,
increment ->d_count
return 1;
// OK, it's still alive, but ->d_count is 0
__d_drop // equivalent of d_invalidate in this case
if not on a shrink list // otherwise it's not our headache
if on lru list
d_lru_del
d_shrink_add dentry to temp_list
return 0;
And yeah, that'll dirty ->s_active for each procfs superblock that
has dentry for our process present in dcache. On exit()...
On Wed, Feb 12, 2020 at 9:55 PM Al Viro [off-list ref] wrote:
What I don't understand is the insistence on getting those dentries
via dcache lookups.
I don't think that's an "insistence", it's more of a "historical
behavior" together with "several changes over the years to deal with
dentry-level cleanups and updates".
_IF_ we are willing to live with cacheline
contention (on ->d_lock of root dentry, if nothing else), why not
do the following:
* put all dentries of such directories ([0-9]* and [0-9]*/task/*)
into a list anchored in task_struct; have non-counting reference to
task_struct stored in them (might simplify part of get_proc_task() users,
Hmm.
Right now I don't think we actually create any dentries at all for the
short-lived process case.
Wouldn't your suggestion make fork/exit rather worse?
Or would you create the dentries dynamically still at lookup time, and
then attach them to the process at that point?
What list would you use for the dentry chaining? Would you play games
with the dentry hashing, and "hash" them off the process, and never
hit in the lookup cache?
Am I misunderstanding what you suggest?
Linus
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2020-02-13 22:24:03
On Thu, Feb 13, 2020 at 01:30:11PM -0800, Linus Torvalds wrote:
On Wed, Feb 12, 2020 at 9:55 PM Al Viro [off-list ref] wrote:
quoted
What I don't understand is the insistence on getting those dentries
via dcache lookups.
I don't think that's an "insistence", it's more of a "historical
behavior" together with "several changes over the years to deal with
dentry-level cleanups and updates".
quoted
_IF_ we are willing to live with cacheline
contention (on ->d_lock of root dentry, if nothing else), why not
do the following:
* put all dentries of such directories ([0-9]* and [0-9]*/task/*)
into a list anchored in task_struct; have non-counting reference to
task_struct stored in them (might simplify part of get_proc_task() users,
Hmm.
Right now I don't think we actually create any dentries at all for the
short-lived process case.
Wouldn't your suggestion make fork/exit rather worse?
Or would you create the dentries dynamically still at lookup time, and
then attach them to the process at that point?
What list would you use for the dentry chaining? Would you play games
with the dentry hashing, and "hash" them off the process, and never
hit in the lookup cache?
I'd been thinking of ->d_fsdata pointing to a structure with list_head
and a (non-counting) task_struct pointer for those guys. Allocated
on lookup, of course (as well as readdir ;-/) and put on the list
at the same time.
IOW, for short-lived process we simply have an empty (h)list anchored
in task_struct and that's it.
On Thu, Feb 13, 2020 at 2:23 PM Al Viro [off-list ref] wrote:
I'd been thinking of ->d_fsdata pointing to a structure with list_head
and a (non-counting) task_struct pointer for those guys. Allocated
on lookup, of course (as well as readdir ;-/) and put on the list
at the same time.
Hmm. That smells like potentially a lot of small allocations, and
making readdir() even nastier.
Do we really want to create the dentries at readdir time? We do now
(with proc_fill_cache()) but do we actually _need_ to?
I guess a lot of readdir users end up doing a stat on it immediately
afterwards. I think right now we do it to get the inode number, and
maybe that is a basic requirement (even if I don't think it's really
stable - an inode could be evicted and then the ino changes, no?)
Ho humm. This all doesn't make me happy. But I guess the proof is in
the pudding - and if you come up with a good patch, I won't complain.
Linus