Re: [PATCH v1 3/3] NFSD: Implement NFSD_IO_DIRECT for NFS READ

[Chuck Lever <cel@kernel.org>]

On 9/9/25 9:52 PM, NeilBrown wrote:
>>>> +	v = 0;
>>>> +	total = dio_end - dio_start;
>>>> +	while (total) {
>>>> +		len = min_t(size_t, total, PAGE_SIZE);
>>>> +		bvec_set_page(&rqstp->rq_bvec[v], *(rqstp->rq_next_page++),
>>>> +			      len, 0);
>>>> +		total -= len;
>>>> +		++v;
>>>> +	}
>>>> +	WARN_ON_ONCE(v > rqstp->rq_maxpages);
>>> I would rather we had an early test rather than a late warn-on.
>>> e.g.
>>>   if (total > (rqstp->rq_maxpages >> PAGE_SHIFT))
>>>      return -EINVAL /* or whatever */;
>>>
>>> Otherwise it seems to be making unstated assumptions about how big the
>>> alignment requirements could be.
>> This is the same warn-on test that nfsd_iter_read does for buffered and
>> dontcache reads. It's done late because the final value of v is computed
>> here, not known before the loop.
> True, but in this case "total" could be larger than "*count" which was
> size-checked in e.g.  nfsd4_encode_read.  So it could now be larger than
> the available space.

Expanding the byte range is constrained to the alignment parameters,
meaning the most the range can increase is by a single page (assuming
the needed alignment is always less than or equal to a page size, or
that we stipulate larger alignments are not yet supported).

Both rq_bvec and rq_pages have that extra page already.


>> I think we might be able to turn this into a short read, for all I/O
>> modes?
> Yes, that could be a clean way to handle the unlikely case that the
> reads doesn't fit any more.

It's probably best to not have the WARN_ON at all. Either convert the
failure to a short read, or prove formally that the condition cannot
happen and simply remove the WARN_ON. I have never seen it fire.

That should be done to nfsd_iter_read() /before/ this series. Then 3/3
can "copy" and use the improved loop logic.


-- 
Chuck Lever