Re: [PATCH v4 4/4] NFSD: Implement NFSD_IO_DIRECT for NFS READ

[Chuck Lever <cel@kernel.org>]

On 9/29/25 12:29 AM, Christoph Hellwig wrote:
> On Fri, Sep 26, 2025 at 10:51:51AM -0400, Chuck Lever wrote:
>> From: Chuck Lever <chuck.lever@oracle.com>
>>
>> Add an experimental option that forces NFS READ operations to use
>> direct I/O instead of reading through the NFS server's page cache.
>>
>> There are already other layers of caching:
>>  - The page cache on NFS clients
>>  - The block device underlying the exported file system
> 
> What layer of caching is in the "block device" ?

I wrote this before Damien explained to me that the block devices
generally don't have a very significant cache at all.


>> +nfsd_direct_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
>> +		 struct nfsd_file *nf, loff_t offset, unsigned long *count,
>> +		 u32 *eof)
>> +{
>> +	loff_t dio_start, dio_end;
>> +	unsigned long v, total;
>> +	struct iov_iter iter;
>> +	struct kiocb kiocb;
>> +	ssize_t host_err;
>> +	size_t len;
>> +
>> +	init_sync_kiocb(&kiocb, nf->nf_file);
>> +	kiocb.ki_flags |= IOCB_DIRECT;
>> +
>> +	/* Read a properly-aligned region of bytes into rq_bvec */
>> +	dio_start = round_down(offset, nf->nf_dio_read_offset_align);
>> +	dio_end = round_up(offset + *count, nf->nf_dio_read_offset_align);
>> +
>> +	kiocb.ki_pos = dio_start;
>> +
>> +	v = 0;
>> +	total = dio_end - dio_start;
>> +	while (total && v < rqstp->rq_maxpages &&
>> +	       rqstp->rq_next_page < rqstp->rq_page_end) {
>> +		len = min_t(size_t, total, PAGE_SIZE);
>> +		bvec_set_page(&rqstp->rq_bvec[v], *rqstp->rq_next_page,
>> +			      len, 0);
>> +
>> +		total -= len;
>> +		++rqstp->rq_next_page;
>> +		++v;
>> +	}
>> +
>> +	trace_nfsd_read_direct(rqstp, fhp, offset, *count - total);
>> +	iov_iter_bvec(&iter, ITER_DEST, rqstp->rq_bvec, v,
>> +		      dio_end - dio_start - total);
>> +
>> +	host_err = vfs_iocb_iter_read(nf->nf_file, &kiocb, &iter);
>> +	if (host_err >= 0) {
>> +		unsigned int pad = offset - dio_start;
>> +
>> +		/* The returned payload starts after the pad */
>> +		rqstp->rq_res.page_base = pad;
>> +
>> +		/* Compute the count of bytes to be returned */
>> +		if (host_err > pad + *count) {
>> +			host_err = *count;
>> +		} else if (host_err > pad) {
>> +			host_err -= pad;
>> +		} else {
>> +			host_err = 0;
>> +		}
> 
> No need for the braces here.
> 
>> +	} else if (unlikely(host_err == -EINVAL)) {
>> +		pr_info_ratelimited("nfsd: Unexpected direct I/O alignment failure\n");
>> +		host_err = -ESERVERFAULT;
>> +	}
> 
> You'll probably want to print s_id to identify the file syste for which
> this happened.  What is -ESERVERFAULT supposed to mean, btw?

Note, this is a "should never happen" condition -- so, exceedingly rare.

Clients typically turns NFSERR_SERVERFAULT into -EREMOTEIO.


>> +	case NFSD_IO_DIRECT:
>> +		if (nf->nf_dio_read_offset_align &&
> 
> I guess this is the "is direct I/O actually supported" check?  I guess
> it'll work, but will underreport as very few file system actually
> report the requirement at the moment.  Can you add a comment explaining
> the check?
Well, it's to ensure that the vfs_getattr we did at open time actually
got a valid value. But effectively, yes, it means "is direct I/O
available for this file?"


-- 
Chuck Lever