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
