Re: [PATCH 3/2 SQUASH] nfsd: use __fput_sync() to avoid delayed closing of files.

[Jeff Layton <jlayton@kernel.org>]

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@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);
>  	}
>  
>  	/*
> 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@kernel.org>