| Message ID | 20220531153708.3449446-1-chengzhihao1@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] proc: Fix a dentry lock race between release_task and lookup | expand |
Zhihao Cheng <chengzhihao1@huawei.com> writes: > 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/pid) > 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/pid) > shrink_dcache_parent(/proc/pid) > d_walk(/proc/pid) > spin_lock_nested(/proc/pid/fd) > // iterating opened fd > proc_flush_pid | > d_invalidate (/proc/pid/fd) | > shrink_dcache_parent(/proc/pid/fd) | > shrink_dentry_list(subdirs) ↓ > shrink_lock_dentry(/proc/pid/fd) ---> race on dentry lock > > Function d_invalidate() will remove dentry from hash firstly, but why does > proc_flush_pid() process dentry '/proc/pid/fd' before dentry '/proc/pid'? > That's because proc_pid_make_inode() adds proc inode in reverse order by > invoking hlist_add_head_rcu(). But proc should not add any inodes under > '/proc/pid' except '/proc/pid/task/pid', fix it by adding inode into > 'pid->inodes' only if the inode is /proc/pid or /proc/pid/task/pid. If I understand correctly you are saying that under some circumstances this code runs slow, and you are proposing an optimization. That optimization is to change the content of the pid->inodes list from all directories under that pid, to just the /proc/<tgid> and /proc/<tgid>/task/<pid>. The justification being that d_invalidate on the parent directory will invalidate all children. So only those two directories are interesting from a d_invalidate point of view. That seems like a valid optimization. This could also count as a regression fix if you can show how the performance changed poorly when the pid->inodes change was introduced and how the performance improves with your change. I currently only see that you hit a pathological case and you are correcting it. As for the actual code change I think it would be better to remove the code from proc_pid_make_inode and make a helper proc_pid_make_base_inode that performs the extra work of adding to the pid->list. Not adding a flag makes the code easier to follow. Something like the code below. diff --git a/fs/proc/base.c b/fs/proc/base.c index d654ce7150fd..9d025e70ddc3 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -1915,11 +1915,6 @@ struct inode *proc_pid_make_inode(struct super_block * sb, /* Let the pid remember us for quick removal */ ei->pid = pid; - if (S_ISDIR(mode)) { - spin_lock(&pid->lock); - hlist_add_head_rcu(&ei->sibling_inodes, &pid->inodes); - spin_unlock(&pid->lock); - } task_dump_owner(task, 0, &inode->i_uid, &inode->i_gid); security_task_to_inode(task, inode); @@ -1932,6 +1927,27 @@ struct inode *proc_pid_make_inode(struct super_block * sb, return NULL; } +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; +} + int pid_getattr(struct user_namespace *mnt_userns, const struct path *path, struct kstat *stat, u32 request_mask, unsigned int query_flags) { @@ -3351,7 +3367,7 @@ 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); @@ -3650,7 +3666,7 @@ 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); Eric
Hi, > If I understand correctly you are saying that under some circumstances > this code runs slow, and you are proposing an optimization. > > That optimization is to change the content of the pid->inodes list > from all directories under that pid, to just the /proc/<tgid> and > /proc/<tgid>/task/<pid>. > Yes and yes. > The justification being that d_invalidate on the parent directory will > invalidate all children. So only those two directories are interesting > from a d_invalidate point of view. > Absolutely right. > That seems like a valid optimization. > > This could also count as a regression fix if you can show how the > performance changed poorly when the pid->inodes change was introduced > and how the performance improves with your change. I currently only > see that you hit a pathological case and you are correcting it. There is a reproducer attached in commit msg: https://bugzilla.kernel.org/show_bug.cgi?id=216054 Create 200 tasks, each task open one file for 50,000 times. Kill all tasks when opened files exceed 10,000,000 (cat /proc/sys/fs/file-nr). Before fix: $ time killall -wq aa real 4m40.946s # During this period, we can see 'ps' and 'systemd' taking high cpu usage. After fix: $ time killall -wq aa real 1min~ # During this period, we can see 'systemd' taking high cpu usage. > > As for the actual code change I think it would be better to > remove the code from proc_pid_make_inode and make a helper > proc_pid_make_base_inode that performs the extra work of > adding to the pid->list. Not adding a flag makes the code > easier to follow. > Agree, will send a v2. > Something like the code below. > > diff --git a/fs/proc/base.c b/fs/proc/base.c > index d654ce7150fd..9d025e70ddc3 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -1915,11 +1915,6 @@ struct inode *proc_pid_make_inode(struct super_block * sb, > > /* Let the pid remember us for quick removal */ > ei->pid = pid; > - if (S_ISDIR(mode)) { > - spin_lock(&pid->lock); > - hlist_add_head_rcu(&ei->sibling_inodes, &pid->inodes); > - spin_unlock(&pid->lock); > - } > > task_dump_owner(task, 0, &inode->i_uid, &inode->i_gid); > security_task_to_inode(task, inode); > @@ -1932,6 +1927,27 @@ struct inode *proc_pid_make_inode(struct super_block * sb, > return NULL; > } > > +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; > +} > + > int pid_getattr(struct user_namespace *mnt_userns, const struct path *path, > struct kstat *stat, u32 request_mask, unsigned int query_flags) > { > @@ -3351,7 +3367,7 @@ 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); > > @@ -3650,7 +3666,7 @@ 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); > > > Eric > . >
diff --git a/fs/proc/base.c b/fs/proc/base.c index c1031843cc6a..29c0f1175766 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -1886,7 +1886,8 @@ void proc_pid_evict_inode(struct proc_inode *ei) } struct inode *proc_pid_make_inode(struct super_block * sb, - struct task_struct *task, umode_t mode) + struct task_struct *task, + umode_t mode, bool add_inode) { struct inode * inode; struct proc_inode *ei; @@ -1914,7 +1915,7 @@ struct inode *proc_pid_make_inode(struct super_block * sb, /* Let the pid remember us for quick removal */ ei->pid = pid; - if (S_ISDIR(mode)) { + if (add_inode) { spin_lock(&pid->lock); hlist_add_head_rcu(&ei->sibling_inodes, &pid->inodes); spin_unlock(&pid->lock); @@ -2243,7 +2244,7 @@ proc_map_files_instantiate(struct dentry *dentry, inode = proc_pid_make_inode(dentry->d_sb, task, S_IFLNK | ((mode & FMODE_READ ) ? S_IRUSR : 0) | - ((mode & FMODE_WRITE) ? S_IWUSR : 0)); + ((mode & FMODE_WRITE) ? S_IWUSR : 0), false); if (!inode) return ERR_PTR(-ENOENT); @@ -2609,7 +2610,7 @@ static struct dentry *proc_pident_instantiate(struct dentry *dentry, struct inode *inode; struct proc_inode *ei; - inode = proc_pid_make_inode(dentry->d_sb, task, p->mode); + inode = proc_pid_make_inode(dentry->d_sb, task, p->mode, false); if (!inode) return ERR_PTR(-ENOENT); @@ -3350,7 +3351,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_inode(dentry->d_sb, task, + S_IFDIR | S_IRUGO | S_IXUGO, true); if (!inode) return ERR_PTR(-ENOENT); @@ -3649,7 +3651,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_inode(dentry->d_sb, task, + S_IFDIR | S_IRUGO | S_IXUGO, true); if (!inode) return ERR_PTR(-ENOENT); diff --git a/fs/proc/fd.c b/fs/proc/fd.c index 913bef0d2a36..53365ebb4567 100644 --- a/fs/proc/fd.c +++ b/fs/proc/fd.c @@ -199,7 +199,7 @@ static struct dentry *proc_fd_instantiate(struct dentry *dentry, struct proc_inode *ei; struct inode *inode; - inode = proc_pid_make_inode(dentry->d_sb, task, S_IFLNK); + inode = proc_pid_make_inode(dentry->d_sb, task, S_IFLNK, false); if (!inode) return ERR_PTR(-ENOENT); @@ -332,7 +332,7 @@ static struct dentry *proc_fdinfo_instantiate(struct dentry *dentry, struct proc_inode *ei; struct inode *inode; - inode = proc_pid_make_inode(dentry->d_sb, task, S_IFREG | S_IRUSR); + inode = proc_pid_make_inode(dentry->d_sb, task, S_IFREG | S_IRUSR, false); if (!inode) return ERR_PTR(-ENOENT); diff --git a/fs/proc/internal.h b/fs/proc/internal.h index 06a80f78433d..9894a591a38c 100644 --- a/fs/proc/internal.h +++ b/fs/proc/internal.h @@ -162,7 +162,8 @@ extern int pid_getattr(struct user_namespace *, const struct path *, extern int pid_getattr(const struct path *, struct kstat *, u32, unsigned int); extern int proc_setattr(struct dentry *, struct iattr *); extern void proc_pid_evict_inode(struct proc_inode *); -extern struct inode *proc_pid_make_inode(struct super_block *, struct task_struct *, umode_t); +extern struct inode *proc_pid_make_inode(struct super_block *, + struct task_struct *, umode_t, bool); extern void pid_update_inode(struct task_struct *, struct inode *); extern int pid_delete_dentry(const struct dentry *); extern int proc_pid_readdir(struct file *, struct dir_context *); diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c index 8e159fc78c0a..c9006e3b36fc 100644 --- a/fs/proc/namespaces.c +++ b/fs/proc/namespaces.c @@ -102,7 +102,8 @@ static struct dentry *proc_ns_instantiate(struct dentry *dentry, struct inode *inode; struct proc_inode *ei; - inode = proc_pid_make_inode(dentry->d_sb, task, S_IFLNK | S_IRWXUGO); + inode = proc_pid_make_inode(dentry->d_sb, task, S_IFLNK | S_IRWXUGO, + false); if (!inode) return ERR_PTR(-ENOENT);
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/pid) 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/pid) shrink_dcache_parent(/proc/pid) d_walk(/proc/pid) spin_lock_nested(/proc/pid/fd) // iterating opened fd proc_flush_pid | d_invalidate (/proc/pid/fd) | shrink_dcache_parent(/proc/pid/fd) | shrink_dentry_list(subdirs) ↓ shrink_lock_dentry(/proc/pid/fd) ---> race on dentry lock Function d_invalidate() will remove dentry from hash firstly, but why does proc_flush_pid() process dentry '/proc/pid/fd' before dentry '/proc/pid'? That's because proc_pid_make_inode() adds proc inode in reverse order by invoking hlist_add_head_rcu(). But proc should not add any inodes under '/proc/pid' except '/proc/pid/task/pid', fix it by adding inode into 'pid->inodes' only if the inode is /proc/pid or /proc/pid/task/pid. 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@huawei.com> Signed-off-by: Zhang Yi <yi.zhang@huawei.com> --- fs/proc/base.c | 15 +++++++++------ fs/proc/fd.c | 4 ++-- fs/proc/internal.h | 3 ++- fs/proc/namespaces.c | 3 ++- 4 files changed, 15 insertions(+), 10 deletions(-)