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

[Chuck Lever <chuck.lever@oracle.com>]

On 9/29/25 6:03 AM, Chuck Lever wrote:
> 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?

It means, there was an unexpected fault on the server. Software bug.


> 
> 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