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 <[email protected]>
Signed-off-by: Zhang Yi <[email protected]>
---
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(-)
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);
--
2.31.1
Zhihao Cheng <[email protected]> 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