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

[Chuck Lever <cel@kernel.org>]

On 10/3/25 3:19 AM, Christoph Hellwig wrote:
>> +/*
>> + * The byte range of the client's READ request is expanded on both
>> + * ends until it meets the underlying file system's direct I/O
>> + * alignment requirements. After the internal read is complete, the
>> + * byte range of the NFS READ payload is reduced to the byte range
>> + * that was originally requested.
>> + *
>> + * Note that a direct read can be done only when the xdr_buf
>> + * containing the NFS READ reply does not already have contents in
>> + * its .pages array. This is due to potentially restrictive
>> + * alignment requirements on the read buffer. When .page_len and
>> + * @base are zero, the .pages array is guaranteed to be page-
>> + * aligned.
>> + */
> 
> Any reason you are not using the full 80 characters available for the
> comment?

Yep: I find comment blocks that fill the whole line more difficult
to read because my eyes are gray and bent. :-)


>> +	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;
> 
> I find this loop style very hard to follow.  Why not do something like:
> 
> Why not something like:
> 
> 	for (v = 0; v < rqstp->rq_maxpages; v++) {
> 		size_t len = min_t(size_t, total, PAGE_SIZE);
> 
> 		bvec_set_page(&rqstp->rq_bvec[v], *rqstp->rq_next_page, len, 0);
> 		rqstp->rq_next_page++;
> 		total -= len;
> 		if (!total)
> 			break;
> 	}
> 
> ?

We copied this loop from nfsd_iter_read(), where we just reworked it
because there are several conditions that need to terminate the loop:

- There are no more bytes to process
- There are no more pages available in rq_pages
- There are no more entries in rq_bvec

Your proposed rewrite drops the array bounds checking that we just
added, IIUC. Neil doesn't like adding termination conditions at the end
of a loop.

I'm open to utilizing for() instead of while() but I didn't find that
writing it as for() improved legibility.

(Yes this is duplicate code, which I'm OK with until we have optimized
direct reads properly and have nailed down exactly when NFSD will employ
them).


>> +	case NFSD_IO_DIRECT:
>> +		/* When dio_read_offset_align is zero, dio is not supported */
>> +		if (nf->nf_dio_read_offset_align &&
>> +		    !rqstp->rq_res.page_len)
> 
> Nit: the entire if clause fits into a single line.
> 

I'll fix that; an additional check was removed during the last
revision, and that shortened the second line.


-- 
Chuck Lever