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()...
