Received-SPF: pass (google.com: domain of linux-nfs+bounces-620-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0
From: "NeilBrown" <neilb@suse.de>
To: "Chuck Lever" <chuck.lever@oracle.com>, "Jeff Layton" <jlayton@kernel.org>
Cc: linux-nfs@vger.kernel.org, "Olga Kornievskaia" <kolga@netapp.com>,
 "Dai Ngo" <Dai.Ngo@oracle.com>, "Tom Talpey" <tom@talpey.com>
Subject:
 [PATCH 3/2 SQUASH] nfsd: use __fput_sync() to avoid delayed closing of files.
In-reply-to: <20231215012059.30857-3-neilb@suse.de>
References: <20231215012059.30857-1-neilb@suse.de>,
 <20231215012059.30857-3-neilb@suse.de>
Date: Fri, 15 Dec 2023 19:27:19 +1100
Message-id: <170262883968.12910.3593954866329935845@noble.neil.brown.name>


After posting I remembered that you suggested a separate function would
be a good place for relevant documentation.

Please squash this into 2/2.

Thanks.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/filecache.c |  4 +---
 fs/nfsd/vfs.c       | 34 +++++++++++++++++++++++++++++++++-
 fs/nfsd/vfs.h       |  2 ++
 3 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index f9da4b0c4d42..9a6ff8fcd12e 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -280,9 +280,7 @@ 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);
+		nfsd_filp_close(nf->nf_file);
 	}
=20
 	/*
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 998f9ba0e168..f365c613e39e 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -2141,11 +2141,43 @@ nfsd_readdir(struct svc_rqst *rqstp, struct svc_fh *f=
hp, loff_t *offsetp,
 	if (err =3D=3D nfserr_eof || err =3D=3D nfserr_toosmall)
 		err =3D nfs_ok; /* can still be found in ->err */
 out_close:
-	__fput_sync(file);
+	nfsd_filp_close(file);
 out:
 	return err;
 }
=20
+/**
+ * nfsd_filp_close: close a file synchronously
+ * @fp: the file to close
+ *
+ * nfsd_filp_close() is similar in behaviour to filp_close().
+ * The difference is that if this is the final close on the
+ * file, the that finalisation happens immediately, rather then
+ * being handed over to a work_queue, as it the case for
+ * filp_close().
+ * When a user-space process closes a file (even when using
+ * filp_close() the finalisation happens before returning to
+ * userspace, so it is effectively synchronous.  When a kernel thread
+ * uses file_close(), on the other hand, the handling is completely
+ * asynchronous.  This means that any cost imposed by that finalisation
+ * is not imposed on the nfsd thread, and nfsd could potentually
+ * close files more quickly than the work queue finalises the close,
+ * which would lead to unbounded growth in the queue.
+ *
+ * In some contexts is it not safe to synchronously wait for
+ * close finalisation (see comment for __fput_sync()), but nfsd
+ * does not match those contexts.  In partcilarly it does not, at the
+ * time that this function is called, hold and locks and no finalisation
+ * of any file, socket, or device driver would have any cause to wait
+ * for nfsd to make progress.
+ */
+void nfsd_filp_close(struct file *fp)
+{
+	get_file(fp);
+	filp_close(fp, NULL);
+	__fput_sync(fp);
+}
+
 /*
  * Get file system stats
  * N.B. After this call fhp needs an fh_put
diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index e3c29596f4df..f76b5c6b4706 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -147,6 +147,8 @@ __be32		nfsd_statfs(struct svc_rqst *, struct svc_fh *,
 __be32		nfsd_permission(struct svc_rqst *, struct svc_export *,
 				struct dentry *, int);
=20
+void		nfsd_filp_close(struct file *fp);
+
 static inline int fh_want_write(struct svc_fh *fh)
 {
 	int ret;
--=20
2.43.0