All of lore.kernel.org
 help / color / mirror / Atom feed
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>
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-nfs@vger.kernel.org
Subject: [PATCH 1/3] nfsd: use __fput_sync() to avoid delayed closing of files.
Date: Fri,  8 Dec 2023 14:27:26 +1100	[thread overview]
Message-ID: <20231208033006.5546-2-neilb@suse.de> (raw)
In-Reply-To: <20231208033006.5546-1-neilb@suse.de>

Calling fput() directly or though filp_close() from a kernel thread like
nfsd causes the final __fput() (if necessary) to be called from a
workqueue.  This means that nfsd is not forced to wait for any work to
complete.  If the ->release of ->destroy_inode function is slow for any
reason, this can result in nfsd closing files more quickly than the
workqueue can complete the close and the queue of pending closes can
grow without bounces (30 million has been seen at one customer site,
though this was in part due to a slowness in xfs which has since been
fixed).

nfsd does not need this.  This quite appropriate and safe for nfsd to do
its own close work.  There is now reason that close should ever wait for
nfsd, so no deadlock can occur.

So change all fput() calls to __fput_sync(), and convert filp_close() to
the sequence get_file();filp_close();__fput_sync().

This ensure that no fput work is queued to the workqueue.

Note that this removes the only in-module use of flush_fput_queue().

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/filecache.c   |  3 ++-
 fs/nfsd/lockd.c       |  2 +-
 fs/nfsd/nfs4proc.c    |  4 ++--
 fs/nfsd/nfs4recover.c |  2 +-
 fs/nfsd/vfs.c         | 12 ++++++------
 5 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index ef063f93fde9..e9734c7451b5 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -283,7 +283,9 @@ nfsd_file_free(struct nfsd_file *nf)
 		nfsd_file_mark_put(nf->nf_mark);
 	if (nf->nf_file) {
 		nfsd_file_check_write_error(nf);
+		get_file(nf->nf_file);
 		filp_close(nf->nf_file, NULL);
+		__fput_sync(nf->nf_file);
 	}
 
 	/*
@@ -631,7 +633,6 @@ nfsd_file_close_inode_sync(struct inode *inode)
 		list_del_init(&nf->nf_lru);
 		nfsd_file_free(nf);
 	}
-	flush_delayed_fput();
 }
 
 /**
diff --git a/fs/nfsd/lockd.c b/fs/nfsd/lockd.c
index 46a7f9b813e5..f9d1059096a4 100644
--- a/fs/nfsd/lockd.c
+++ b/fs/nfsd/lockd.c
@@ -60,7 +60,7 @@ nlm_fopen(struct svc_rqst *rqstp, struct nfs_fh *f, struct file **filp,
 static void
 nlm_fclose(struct file *filp)
 {
-	fput(filp);
+	__fput_sync(filp);
 }
 
 static const struct nlmsvc_binding nfsd_nlm_ops = {
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 6f2d4aa4970d..20d60823d530 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -629,7 +629,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		nn->somebody_reclaimed = true;
 out:
 	if (open->op_filp) {
-		fput(open->op_filp);
+		__fput_sync(open->op_filp);
 		open->op_filp = NULL;
 	}
 	if (resfh && resfh != &cstate->current_fh) {
@@ -1546,7 +1546,7 @@ nfsd4_cleanup_inter_ssc(struct nfsd4_ssc_umount_item *nsui, struct file *filp,
 	long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
 
 	nfs42_ssc_close(filp);
-	fput(filp);
+	__fput_sync(filp);
 
 	spin_lock(&nn->nfsd_ssc_lock);
 	list_del(&nsui->nsui_list);
diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index 3509e73abe1f..f8f0112fd9f5 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -561,7 +561,7 @@ nfsd4_shutdown_recdir(struct net *net)
 
 	if (!nn->rec_file)
 		return;
-	fput(nn->rec_file);
+	__fput_sync(nn->rec_file);
 	nn->rec_file = NULL;
 }
 
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index fbbea7498f02..15a811229211 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -879,7 +879,7 @@ __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type,
 
 	host_err = ima_file_check(file, may_flags);
 	if (host_err) {
-		fput(file);
+		__fput_sync(file);
 		goto out;
 	}
 
@@ -1884,10 +1884,10 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
 	fh_drop_write(ffhp);
 
 	/*
-	 * If the target dentry has cached open files, then we need to try to
-	 * close them prior to doing the rename. Flushing delayed fput
-	 * shouldn't be done with locks held however, so we delay it until this
-	 * point and then reattempt the whole shebang.
+	 * If the target dentry has cached open files, then we need to
+	 * try to close them prior to doing the rename.  Final fput
+	 * shouldn't be done with locks held however, so we delay it
+	 * until this point and then reattempt the whole shebang.
 	 */
 	if (close_cached) {
 		close_cached = false;
@@ -2141,7 +2141,7 @@ nfsd_readdir(struct svc_rqst *rqstp, struct svc_fh *fhp, loff_t *offsetp,
 	if (err == nfserr_eof || err == nfserr_toosmall)
 		err = nfs_ok; /* can still be found in ->err */
 out_close:
-	fput(file);
+	__fput_sync(file);
 out:
 	return err;
 }
-- 
2.43.0


  reply	other threads:[~2023-12-08  3:39 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-12-08  3:27 [PATCH 0/3] nfsd: fully close all files in the nfsd threads NeilBrown
2023-12-08  3:27 ` NeilBrown [this message]
2023-12-08 15:01   ` [PATCH 1/3] nfsd: use __fput_sync() to avoid delayed closing of files Chuck Lever
2023-12-10 22:47     ` NeilBrown
2023-12-11 19:01       ` Chuck Lever
2023-12-11 22:04         ` NeilBrown
2023-12-12 16:17           ` Chuck Lever
2023-12-11 19:11       ` Al Viro
2023-12-11 22:23         ` NeilBrown
2023-12-11 23:13           ` Al Viro
2023-12-11 23:21             ` Al Viro
2023-12-13  0:28               ` NeilBrown
2023-12-15 18:27                 ` David Laight
2023-12-15 19:35                   ` Chuck Lever III
2023-12-15 22:36                   ` NeilBrown
2023-12-15 18:28           ` David Laight
2023-12-16  1:50       ` Dave Chinner
2023-12-08  3:27 ` [PATCH 2/3] nfsd: Don't leave work of closing files to a work queue NeilBrown
2023-12-08  3:27 ` [PATCH 3/3] VFS: don't export flush_delayed_fput() NeilBrown
2023-12-08 11:40 ` [PATCH 0/3] nfsd: fully close all files in the nfsd threads Jeff Layton
2023-12-08 14:33 ` Jens Axboe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20231208033006.5546-2-neilb@suse.de \
    --to=neilb@suse.de \
    --cc=axboe@kernel.dk \
    --cc=brauner@kernel.org \
    --cc=chuck.lever@oracle.com \
    --cc=jlayton@kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=oleg@redhat.com \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.