Received-SPF: pass (google.com: domain of linux-nfs+bounces-111-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0
From: "NeilBrown" <neilb@suse.de>
To: Al Viro <viro@zeniv.linux.org.uk>, Christian Brauner <brauner@kernel.org>,
 Jens Axboe <axboe@kernel.dk>, Oleg Nesterov <oleg@redhat.com>,
 Chuck Lever <chuck.lever@oracle.com>, Jeff Layton <jlayton@kernel.org>,
 Ingo Molnar <mingo@redhat.com>, Peter Zijlstra <peterz@infradead.org>,
 Juri Lelli <juri.lelli@redhat.com>,
 Vincent Guittot <vincent.guittot@linaro.org>
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
 linux-nfs@vger.kernel.org
Subject: [PATCH/RFC] core/nfsd: allow kernel threads to use task_work.
Date: Tue, 28 Nov 2023 09:05:21 +1100
Message-id: <170112272125.7109.6245462722883333440@noble.neil.brown.name>


I have evidence from a customer site of 256 nfsd threads adding files to
delayed_fput_lists nearly twice as fast they are retired by a single
work-queue thread running delayed_fput().  As you might imagine this
does not end well (20 million files in the queue at the time a snapshot
was taken for analysis).

While this might point to a problem with the filesystem not handling the
final close efficiently, such problems should only hurt throughput, not
lead to memory exhaustion.

For normal threads, the thread that closes the file also calls the
final fput so there is natural rate limiting preventing excessive growth
in the list of delayed fputs.  For kernel threads, and particularly for
nfsd, delayed in the final fput do not impose any throttling to prevent
the thread from closing more files.

A simple way to fix this is to treat nfsd threads like normal processes
for task_work.  Thus the pending files are queued for the thread, and
the same thread finishes the work.

Currently KTHREADs are assumed never to call task_work_run().  With this
patch that it still the default but it is implemented by storing the
magic value TASK_WORKS_DISABLED in ->task_works.  If a kthread, such as
nfsd, will call task_work_run() periodically, it sets ->task_works
to NULL to indicate this.

Signed-off-by: NeilBrown <neilb@suse.de>
---

I wonder which tree this should go through assuming everyone likes it.
VFS maybe??

Thanks.

 fs/file_table.c           | 2 +-
 fs/nfsd/nfssvc.c          | 4 ++++
 include/linux/sched.h     | 1 +
 include/linux/task_work.h | 4 +++-
 kernel/fork.c             | 2 +-
 kernel/task_work.c        | 7 ++++---
 6 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/fs/file_table.c b/fs/file_table.c
index de4a2915bfd4..e79351df22be 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -445,7 +445,7 @@ void fput(struct file *file)
 	if (atomic_long_dec_and_test(&file->f_count)) {
 		struct task_struct *task =3D current;
=20
-		if (likely(!in_interrupt() && !(task->flags & PF_KTHREAD))) {
+		if (likely(!in_interrupt())) {
 			init_task_work(&file->f_rcuhead, ____fput);
 			if (!task_work_add(task, &file->f_rcuhead, TWA_RESUME))
 				return;
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 66ca50b38b27..c047961262ca 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -13,6 +13,7 @@
 #include <linux/fs_struct.h>
 #include <linux/swap.h>
 #include <linux/siphash.h>
+#include <linux/task_work.h>
=20
 #include <linux/sunrpc/stats.h>
 #include <linux/sunrpc/svcsock.h>
@@ -941,6 +942,7 @@ nfsd(void *vrqstp)
 	}
=20
 	current->fs->umask =3D 0;
+	current->task_works =3D NULL; /* Declare that I will call task_work_run() */
=20
 	atomic_inc(&nfsdstats.th_cnt);
=20
@@ -955,6 +957,8 @@ nfsd(void *vrqstp)
=20
 		svc_recv(rqstp);
 		validate_process_creds();
+		if (task_work_pending(current))
+			task_work_run();
 	}
=20
 	atomic_dec(&nfsdstats.th_cnt);
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 292c31697248..c63c2bedbf71 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1117,6 +1117,7 @@ struct task_struct {
 	unsigned int			sas_ss_flags;
=20
 	struct callback_head		*task_works;
+#define	TASK_WORKS_DISABLED	((void*)1)
=20
 #ifdef CONFIG_AUDIT
 #ifdef CONFIG_AUDITSYSCALL
diff --git a/include/linux/task_work.h b/include/linux/task_work.h
index 795ef5a68429..3c74e3de81ed 100644
--- a/include/linux/task_work.h
+++ b/include/linux/task_work.h
@@ -22,7 +22,9 @@ enum task_work_notify_mode {
=20
 static inline bool task_work_pending(struct task_struct *task)
 {
-	return READ_ONCE(task->task_works);
+	struct callback_head *works =3D READ_ONCE(task->task_works);
+
+	return works && works !=3D TASK_WORKS_DISABLED;
 }
=20
 int task_work_add(struct task_struct *task, struct callback_head *twork,
diff --git a/kernel/fork.c b/kernel/fork.c
index 10917c3e1f03..903b29804fe1 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2577,7 +2577,7 @@ __latent_entropy struct task_struct *copy_process(
 	p->dirty_paused_when =3D 0;
=20
 	p->pdeath_signal =3D 0;
-	p->task_works =3D NULL;
+	p->task_works =3D args->kthread ? TASK_WORKS_DISABLED : NULL;
 	clear_posix_cputimers_work(p);
=20
 #ifdef CONFIG_KRETPROBES
diff --git a/kernel/task_work.c b/kernel/task_work.c
index 95a7e1b7f1da..ffdf4b0d7a0e 100644
--- a/kernel/task_work.c
+++ b/kernel/task_work.c
@@ -49,7 +49,8 @@ int task_work_add(struct task_struct *task, struct callback=
_head *work,
=20
 	head =3D READ_ONCE(task->task_works);
 	do {
-		if (unlikely(head =3D=3D &work_exited))
+		if (unlikely(head =3D=3D &work_exited ||
+			     head =3D=3D TASK_WORKS_DISABLED))
 			return -ESRCH;
 		work->next =3D head;
 	} while (!try_cmpxchg(&task->task_works, &head, work));
@@ -157,7 +158,7 @@ void task_work_run(void)
 		work =3D READ_ONCE(task->task_works);
 		do {
 			head =3D NULL;
-			if (!work) {
+			if (!work || work =3D=3D TASK_WORKS_DISABLED) {
 				if (task->flags & PF_EXITING)
 					head =3D &work_exited;
 				else
@@ -165,7 +166,7 @@ void task_work_run(void)
 			}
 		} while (!try_cmpxchg(&task->task_works, &work, head));
=20
-		if (!work)
+		if (!work || work =3D=3D TASK_WORKS_DISABLED)
 			break;
 		/*
 		 * Synchronize with task_work_cancel(). It can not remove
--=20
2.42.1