Re: [PATCH 1/2] NFSD: Use nfsd_iter_read() when ->splice_read is not zero-copy

[NeilBrown]

On Tue, 18 Aug 2026, Ameer Hamza wrote:
> For some files splicing a READ cannot avoid a copy: gfs2, kernfs
> and the cifs direct-I/O modes use copy_splice_read() as their
> ->splice_read, and the VFS substitutes it for DAX files.
> copy_splice_read() allocates a fresh page for every page of
> payload and reads into it; nfsd_splice_actor() then installs
> those pages in rq_respages, displacing Reply pages the thread
> already owns. Both sets of pages are then freed.
> 
> Route these READs through nfsd_iter_read() instead. It performs
> the same single copy, but into the thread's own Reply pages, so
> the per-READ allocation and the displacement both disappear. On
> its own this is not expected to raise throughput; it changes
> which pages a Reply is built from so that the next patch can
> recycle them.
> 
> 9p and ceph fall back to copy_splice_read() only inside their
> own ->splice_read methods, which nfsd_splice_read_is_zero_copy()
> cannot detect,

I think that if we are going to do this then we should do it properly
and make it easy to detect these cases.

Could we add an FMODE flags FMODE_DONT_COPY_FOR_SPLICE which causes
copy_splice_read() to return -ENOTSUP or similar.
Then nfsd can call splice_read if it appear to exist, but set that flag.
If it fails with -ENOTSUP, fall back to iter_read.

I really don't like the approach of explicitly testing whether
f_op->splice_read is a particular value.

Thanks,
NeilBrown



>                 so they keep the splice path. nfsd_iter_read() is
> the path sec=krb5i, sec=krb5p and nfsd_disable_splice_read READs
> already take, and is unchanged here; the one visible difference
> is that an fsnotify watcher now sees two access events per READ
> instead of one, the extra one from vfs_iocb_iter_read().
> 
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Ameer Hamza <ameer.hamza@xxxxxxxxxxx>
> ---
>  fs/nfsd/nfs4xdr.c |  4 ++--
>  fs/nfsd/vfs.c     |  5 ++++-
>  fs/nfsd/vfs.h     | 29 +++++++++++++++++++++++++++++
>  3 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 7d1b2d6f57f20..07a4bd8feb764 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -5369,7 +5369,7 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr,
>  	maxcount = min_t(unsigned long, read->rd_length,
>  			 (xdr->buf->buflen - xdr->buf->len));
>  
> -	if (file->f_op->splice_read && splice_ok)
> +	if (nfsd_splice_read_is_zero_copy(file) && splice_ok)
>  		nfserr = nfsd4_encode_splice_read(resp, read, file, maxcount);
>  	else
>  		nfserr = nfsd4_encode_readv(resp, read, maxcount);
> @@ -6267,7 +6267,7 @@ nfsd4_encode_read_plus_data(struct nfsd4_compoundres *resp,
>  	maxcount = min_t(unsigned long, read->rd_length,
>  			 (xdr->buf->buflen - xdr->buf->len));
>  
> -	if (file->f_op->splice_read && splice_ok)
> +	if (nfsd_splice_read_is_zero_copy(file) && splice_ok)
>  		nfserr = nfsd4_encode_splice_read(resp, read, file, maxcount);
>  	else
>  		nfserr = nfsd4_encode_readv(resp, read, maxcount);
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index f9131827d391e..1a5fec4cf73d3 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1176,6 +1176,9 @@ nfsd_direct_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
>   *
>   * Some filesystems or situations cannot use nfsd_splice_read. This
>   * function is the slightly less-performant fallback for those cases.
> + * It is also the preferred path where splicing would copy anyway
> + * (see nfsd_splice_read_is_zero_copy()), because the copy then
> + * lands directly in Reply pages nfsd already owns.
>   *
>   * Returns nfs_ok on success, otherwise an nfserr stat value is
>   * returned.
> @@ -1576,7 +1579,7 @@ __be32 nfsd_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		return err;
>  
>  	file = nf->nf_file;
> -	if (file->f_op->splice_read && nfsd_read_splice_ok(rqstp))
> +	if (nfsd_splice_read_is_zero_copy(file) && nfsd_read_splice_ok(rqstp))
>  		err = nfsd_splice_read(rqstp, fhp, file, offset, count, eof);
>  	else
>  		err = nfsd_iter_read(rqstp, fhp, nf, offset, count, 0, eof);
> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> index f0cb184643f2f..70bd3c6fc1880 100644
> --- a/fs/nfsd/vfs.h
> +++ b/fs/nfsd/vfs.h
> @@ -149,6 +149,35 @@ __be32		nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  				unsigned long *count, unsigned int base,
>  				u32 *eof);
>  bool		nfsd_read_splice_ok(struct svc_rqst *rqstp);
> +
> +/**
> + * nfsd_splice_read_is_zero_copy - check whether splice can avoid a data copy
> + * @file: file to be read from
> + *
> + * copy_splice_read() reads via ->read_iter into freshly allocated
> + * pages, exactly as nfsd_iter_read() does into pages nfsd already
> + * holds. Filesystems that implement ->splice_read with it gain
> + * nothing from the splice path, and neither do DAX files, for
> + * which the VFS substitutes copy_splice_read() no matter what
> + * the filesystem registered. The VFS substitutes it for O_DIRECT
> + * files as well, but nfsd never opens files O_DIRECT.
> + *
> + * The test is one-sided: a filesystem's own ->splice_read method
> + * may fall back to copy_splice_read() internally, as ceph and 9p
> + * do, and that cannot be detected here.
> + *
> + * Return values:
> + *   %true: splicing from @file is not known to copy
> + *   %false: splicing from @file would copy, or is not supported
> + *           at all; use nfsd_iter_read()
> + */
> +static inline bool nfsd_splice_read_is_zero_copy(const struct file *file)
> +{
> +	return file->f_op->splice_read &&
> +	       file->f_op->splice_read != copy_splice_read &&
> +	       !IS_DAX(file_inode(file));
> +}
> +
>  __be32		nfsd_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  				loff_t offset, unsigned long *count,
>  				u32 *eof);
> -- 
> 2.53.0
> 
>