On Sun, 14 Sep 2025, Chuck Lever wrote:
> On 9/13/25 1:37 AM, NeilBrown wrote:
> >> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> >> index 714777c221ed..e2f0fe3f82c0 100644
> >> --- a/fs/nfsd/vfs.c
> >> +++ b/fs/nfsd/vfs.c
> >> @@ -1120,13 +1120,13 @@ __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >> bvec_set_page(&rqstp->rq_bvec[v], *(rqstp->rq_next_page++),
> >> len, base);
> >> total -= len;
> >> - ++v;
> >> base = 0;
> >> + if (++v >= rqstp->rq_maxpages)
> >> + break;
> > I would have changed the head of the while loop to be
> >
> > while (total > 0 && rqstp->rq_next_page < ....)
> >
> > and then realised that I don't know what to put there.
> > I don't think that counting up to rq_maxpages is safe when there could
> > be two READ ops in an NFSv4 Compound.
> >
> > And if we are cleaning up this function I woul put the increments of v
> > and rq_next_page next to each other..
> > </bikeshed>
> >
> > The patch is an improvement. I wonder if it is enough.
>
> I'm planning a second patch that adds protection against overrunning
> the rq_pages array. Is there anything else you feel is needed?
It might be easier to review the two patches together, but if we decide
that we aren't worrying about multiple users of the page list I would
rather the code looked like:
while (total > 0 && v < rqstp->rq_maxpages) {
len = min_t(size_t, total, PAGE_SIZE - base);
bvec_set_page(&rqstp->rq_bvec[v], *(rqstp->rq_next_page),
len, base);
total -= len;
v += 1;
rqstp->rq_nextpage += 1
base = 0;
}
So all the tests are together, all the increments are together.
I don't mind if you keep "++" but as I personally don't much like it
I changed it here.
A possible alternative would be
bvec_set_page(&rqstp->rq_bvec[v++], *(rqstp->rq_next_page++),
len, base);
then again all the increments would be together - I don't mind ++
in this context, I just don't much like it stand-alone.
But if you disagree, leave it as it is. Probably the part I like the
least is the "break" and the end of the while(). Having multiple exits
from a loop is sometimes needed, but should be avoided when not needed.
Thanks,
NeilBrown
