Re: [PATCH/RFC] core/nfsd: allow kernel threads to use task_work.

From: "NeilBrown" <neilb@suse.de>
To: "Christian Brauner" <brauner@kernel.org>
Cc: "Al Viro" <viro@zeniv.linux.org.uk>,
	"Jeff Layton" <jlayton@kernel.org>,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-nfs@vger.kernel.org
Subject: Re: [PATCH/RFC] core/nfsd: allow kernel threads to use task_work.
Date: Mon, 04 Dec 2023 12:30:34 +1100	[thread overview]
Message-ID: <170165343439.7109.3106838865273118254@noble.neil.brown.name> (raw)
In-Reply-To: <20231129-querschnitt-urfassung-3ebd703c345a@brauner>

On Wed, 29 Nov 2023, Christian Brauner wrote:
> > If an nfsd thread only completes the close that it initiated the close
> > on (which is what I am currently proposing) then there would be at most
> > one, or maybe 2, fds to close after handling each request.  While that
> > is certainly a non-zero burden, I can't see how it can realistically be
> > called a DOS.
> 
> The 10s of millions of files is what makes me curious. Because that's
> the workload that'd be interesting.
> 

I think the main effect of the 10s of millions of files is to bloat the
icache which causes it to be cleaned and this results is synchronous
reads from storage is situations that you wouldn't usually expect them.

It appears from examining a memory-snapshot that some files being closed
have already been unlinked.  This is quite unusual with NFS but can
happen if they are unlinked from one client while open on another
client.  (The directory containing the file in one case is called
"Cache".  Maybe cleaning of that cache by the applications often gets
files that are in use).

This pattern means that the final __dput calls __dentry_kill and
eventually xfs_fs_destroy_inode.  This sometimes needs to read
synchronously from storage - if the required info isn't cached.  This
causes the delays.

I've modelled the delay with

diff --git a/fs/file_table.c b/fs/file_table.c
index d36cade6e366..51563f79385a 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -29,6 +29,7 @@
 #include <linux/ima.h>
 #include <linux/swap.h>
 #include <linux/kmemleak.h>
+#include <linux/delay.h>
 
 #include <linux/atomic.h>
 
@@ -375,6 +376,9 @@ static void __fput(struct file *file)
 	eventpoll_release(file);
 	locks_remove_file(file);
 
+	if ((file->f_mode & FMODE_WRITE) &&
+	    (current->flags & PF_KTHREAD))
+		msleep(25);
 	ima_file_free(file);
 	if (unlikely(file->f_flags & FASYNC)) {
 		if (file->f_op->fasync)


I loop-back mount a filesystem with NFS on the test machine.
The PF_KTHREAD test ensures that when "cp -r" writes to the NFS
filesystem there is no delay, but when nfsd writes to the local
filesystem there is a delay.

With this patch I can easily demonstrate the number of open files
growing without bound.  With my patch as well (I'll send new version
shortly) the growth is bounded.

Thanks,
NeilBrown
