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

"Chuck Lever" <[email protected]>
Newsgroups org.kernel.vger.linux-nfs,org.kernel.vger.linux-kernel
Message-ID <[email protected]>
Hi Ameer -

First off, thanks for posting the full series to provide good
context for your problem statement. Contention in the page
allocator is vexing, and is a problem we've been chasing for
years.

There is a lot to unpack. I'm going to focus on patch 1/2 and
set 2/2 aside for the moment. I promise we will come back to it.


On Mon, Aug 17, 2026, at 5:08 PM, Ameer Hamza wrote:
> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> index f0cb184643f2..70bd3c6fc188 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));
> +}

[ ... ]

The iter_read path appears to have two pre-existing problems
that will be exposed to even more file systems when 1/2 is
applied.

1. fsnotify double-counting, which you mention
2. atime is not updated, which shows up in LLM review

LLM explains it this way:

--- cut here ---

nfsd_splice_read() picks one up from the VFS on the way through:

fs/splice.c:splice_direct_to_actor() {
	...
done:
	pipe->tail = pipe->head = 0;
	file_accessed(in);
	return bytes;
	...
}

nfsd_iter_read() has no equivalent, so it leaves atime to ->read_iter.
That works out for most of what moves here -- gfs2 reaches
generic_file_read_iter(), cifs direct I/O reaches
netfs_unbuffered_read_iter(), and ext4_dax_read_iter() and
xfs_file_dax_read() both finish with file_accessed() -- but
fuse_dax_read_iter() leaves it undone:

fs/fuse/dax.c:fuse_dax_read_iter() {
	...
	ret = dax_iomap_rw(iocb, to, &fuse_iomap_ops);
	inode_unlock_shared(inode);

	/* TODO file_accessed(iocb->f_filp) */
	return ret;
}

The second runs the other way.  vfs_iocb_iter_read() emits an fsnotify
access event of its own, and nfsd_finish_read() emits a second one:

fs/nfsd/vfs.c:nfsd_finish_read() {
	...
		*count = host_err;
		fsnotify_access(file);
	...
}

splice_direct_to_actor() emits none, so the event in nfsd_finish_read()
is the only one the splice path gets, and one too many for the iter
path.

Both are reachable today without this patch.  sec=krb5i, sec=krb5p and
nfsd_disable_splice_read all reach nfsd_iter_read(), and NFSD_IO_DIRECT
reaches nfsd_direct_read(), which calls vfs_iocb_iter_read() and
nfsd_finish_read() the same way.

--- cut here ---

So what I'd like you to do is provide a backportable fix that applies
before your series to address the fsnotify and atime accounting issues.
I'm not convinced that stable will want the bulk of the 1/2 changes as
you have them here.

I don't expect this work will be a heavy lift, but let me know if it
turns into one.

-- 
Chuck Lever
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.