On Wed, Jun 01, 2022 at 02:23:32PM +0800, Zhihao Cheng wrote:
> Commit 7bc3e6e55acf06 ("proc: Use a list of inodes to flush from proc")
> moved proc_flush_task() behind __exit_signal(). Then, process systemd
> can take long period high cpu usage during releasing task in following
> concurrent processes:
>
> systemd ps
> kernel_waitid stat(/proc/tgid)
> do_wait filename_lookup
> wait_consider_task lookup_fast
> release_task
> __exit_signal
> __unhash_process
> detach_pid
> __change_pid // remove task->pid_links
> d_revalidate -> pid_revalidate // 0
> d_invalidate(/proc/tgid)
> shrink_dcache_parent(/proc/tgid)
> d_walk(/proc/tgid)
> spin_lock_nested(/proc/tgid/fd)
> // iterating opened fd
> proc_flush_pid |
> d_invalidate (/proc/tgid/fd) |
> shrink_dcache_parent(/proc/tgid/fd) |
> shrink_dentry_list(subdirs) ↓
> shrink_lock_dentry(/proc/tgid/fd) --> race on dentry lock
>
Curious... can this same sort of thing happen with /proc/<tgid>/task if
that dir similarly has a lot of dentries?
...
> Fixes: 7bc3e6e55acf06 ("proc: Use a list of inodes to flush from proc")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216054
> Signed-off-by: Zhihao Cheng <chengzhihao1@xxxxxxxxxx>
> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>
> ---
> v1->v2: Add new helper proc_pid_make_base_inode that performs the extra
> work of adding to the pid->list.
> v2->v3: Add performance regression in commit message.
> v3->v4: Make proc_pid_make_base_inode() static
> fs/proc/base.c | 34 ++++++++++++++++++++++++++--------
> 1 file changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index c1031843cc6a..d884933950fd 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
...
> @@ -1931,6 +1926,27 @@ struct inode *proc_pid_make_inode(struct super_block * sb,
> return NULL;
> }
>
> +static struct inode *proc_pid_make_base_inode(struct super_block *sb,
> + struct task_struct *task, umode_t mode)
> +{
> + struct inode *inode;
> + struct proc_inode *ei;
> + struct pid *pid;
> +
> + inode = proc_pid_make_inode(sb, task, mode);
> + if (!inode)
> + return NULL;
> +
> + /* Let proc_flush_pid find this directory inode */
> + ei = PROC_I(inode);
> + pid = ei->pid;
> + spin_lock(&pid->lock);
> + hlist_add_head_rcu(&ei->sibling_inodes, &pid->inodes);
> + spin_unlock(&pid->lock);
> +
> + return inode;
> +}
> +
Somewhat related to the question above.. it would be nice if this
wrapper had a line or two comment above it that explained when it should
or shouldn't be used over the underlying function (for example, why or
why not include /proc/<tgid>/task?). Otherwise the patch overall seems
reasonable to me..
Brian
> int pid_getattr(struct user_namespace *mnt_userns, const struct path *path,
> struct kstat *stat, u32 request_mask, unsigned int query_flags)
> {
> @@ -3350,7 +3366,8 @@ static struct dentry *proc_pid_instantiate(struct dentry * dentry,
> {
> struct inode *inode;
>
> - inode = proc_pid_make_inode(dentry->d_sb, task, S_IFDIR | S_IRUGO | S_IXUGO);
> + inode = proc_pid_make_base_inode(dentry->d_sb, task,
> + S_IFDIR | S_IRUGO | S_IXUGO);
> if (!inode)
> return ERR_PTR(-ENOENT);
>
> @@ -3649,7 +3666,8 @@ static struct dentry *proc_task_instantiate(struct dentry *dentry,
> struct task_struct *task, const void *ptr)
> {
> struct inode *inode;
> - inode = proc_pid_make_inode(dentry->d_sb, task, S_IFDIR | S_IRUGO | S_IXUGO);
> + inode = proc_pid_make_base_inode(dentry->d_sb, task,
> + S_IFDIR | S_IRUGO | S_IXUGO);
> if (!inode)
> return ERR_PTR(-ENOENT);
>
> --
> 2.31.1
>
