On Fri, 2023-12-15 at 19:27 +1100, NeilBrown wrote:
> 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@xxxxxxx>
> ---
> 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);
> }
>
> /*
> 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 *fhp, loff_t *offsetp,
> if (err == nfserr_eof || err == nfserr_toosmall)
> err = nfs_ok; /* can still be found in ->err */
> out_close:
> - __fput_sync(file);
> + nfsd_filp_close(file);
> out:
> return err;
> }
>
> +/**
> + * 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);
>
> +void nfsd_filp_close(struct file *fp);
> +
> static inline int fh_want_write(struct svc_fh *fh)
> {
> int ret;
Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
