在 2022/7/12 22:16, Brian Foster 写道:
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?
Yes. It could happend too. There will be many dentries under
/proc/<tgid>/task when there are many tasks under same thread group.
We must put /proc/<tgid>/task into pid->inodes, because we have to
handle single thread exiting situation: Any one of threads should
invalidate its /proc/<tgid>/task/<pid> dentry before begin released. You
may refer to the function proc_flush_task_mnt() before commit
7bc3e6e55acf06 ("proc: Use a list of inodes to flush from proc").
...
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..
Thanks for advice, I will add some notes in v5.
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
.
