Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     "Chuck Lever III" <chuck.lever@oracle.com>
Cc:     "Bruce Fields" <bfields@fieldses.org>,
        "Linux NFS Mailing List" <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v1 1/5] SUNRPC: Fix the calculation of xdr->end in
 xdr_get_next_encode_buffer()
In-reply-to: <EA34A2F2-D80A-44B0-8AF3-2234069DDC29@oracle.com>
References: <165445865736.1664.4497130284832282034.stgit@bazille.1015granger.net>,
 <165445910560.1664.5852151724543272982.stgit@bazille.1015granger.net>,
 <20220606220938.GE15057@fieldses.org>,
 <165456356541.22243.8883363674329684173@noble.neil.brown.name>,
 <D3608A6C-0CA9-434B-BF56-79AB33793AB4@oracle.com>,
 <EA34A2F2-D80A-44B0-8AF3-2234069DDC29@oracle.com>
Date:   Tue, 07 Jun 2022 13:29:56 +1000
Message-id: <165457259601.22243.4068338030298862634@noble.neil.brown.name>
Precedence: bulk

On Tue, 07 Jun 2022, Chuck Lever III wrote:
> > On Jun 6, 2022, at 10:35 PM, Chuck Lever III <chuck.lever@oracle.com> wro=
te:
> >=20
> >> On Jun 6, 2022, at 8:59 PM, NeilBrown <neilb@suse.de> wrote:
> >> We then assign frag{1,2}bytes and have another chunk of code that looks
> >> wrong to me.  I'd like
> >>=20
> >>  if (xdr->iov) {
> >> 	xdr->iov->iov_len +=3D frag1bytes;
> >> 	xdr->iov =3D NULL;
> >>  } else {
> >>       xdr->buf->page_len +=3D frag1bytes;
> >>       xdr->page_ptr++;
> >>  }
> >>=20
> >> Note that this changes the code NOT to increment pagE_ptr if iov was not
> >> NULL.  I cannot see how it would be correct to do that.  Presumably this
> >> code is never called with iov !=3D NULL ???
> >=20
> > That strikes me as a good change. I will add it to this series as
> > another patch.
> >=20
> > Yes, this code is called by the server's READDIR encoder with iov =3D NUL=
L.
> > See nfsd3_init_dirlist_pages().
>=20
> This change breaks READDIR, looks like.

Thanks for testing...

I looked again, and I see that svcxdr_init_encode() initialises
->page_ptr to "buf->page - 1".  So we need a +1 when moving from the
'head' to the pages.

Commit 05638dc73af2 ("nfsd4: simplify server xdr->next_page use")

explains why.  I'm not sure I completely agree with the reasoning (head
really is a special case), but it probably isn't worth "fixing".

Thanks,
NeilBrown

>=20
> --
> Chuck Lever
>=20
>=20
>=20
>=20