Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\))
Subject: Re: [PATCH v1] NFS: Fix handling of reply page vector
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <aa8a63d9bfc6d76800ef00bfa3b6921e312cfd44.camel@hammerspace.com>
Date:   Mon, 8 Apr 2019 17:29:43 -0400
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Olga Kornievskaia <kolga@netapp.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <09085DBD-FFBD-4ACD-86CE-428F49916F61@oracle.com>
References: <20190408200030.16366.22026.stgit@manet.1015granger.net>
 <bede7559be11f283e9f37db2da0b47ddf869a468.camel@hammerspace.com>
 <B28ACEA4-BB35-4137-A81F-50A1D2974F6A@oracle.com>
 <aa8a63d9bfc6d76800ef00bfa3b6921e312cfd44.camel@hammerspace.com>
To:     Trond Myklebust <trondmy@hammerspace.com>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk


> On Apr 8, 2019, at 5:23 PM, Trond Myklebust <trondmy@hammerspace.com> =
wrote:
>=20
> On Mon, 2019-04-08 at 16:58 -0400, Chuck Lever wrote:
>>> On Apr 8, 2019, at 4:38 PM, Trond Myklebust <
>>> trondmy@hammerspace.com> wrote:
>>>=20
>>> On Mon, 2019-04-08 at 16:00 -0400, Chuck Lever wrote:
>>>> NFSv4 GETACL and FS_LOCATIONS requests stopped working in v5.1-
>>>> rc.
>>>>=20
>>>> These two need the extra padding to be added directly to the
>>>> reply
>>>> length.
>>>>=20
>>>> Reported-by: Olga Kornievskaia <aglo@umich.edu>
>>>> Fixes: 02ef04e432ba ("NFS: Account for XDR pad of buf->pages")
>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>> ---
>>>> fs/nfs/nfs4xdr.c |    4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>=20
>>>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
>>>> index cfcabc3..6024461 100644
>>>> --- a/fs/nfs/nfs4xdr.c
>>>> +++ b/fs/nfs/nfs4xdr.c
>>>> @@ -2589,7 +2589,7 @@ static void nfs4_xdr_enc_getacl(struct
>>>> rpc_rqst
>>>> *req, struct xdr_stream *xdr,
>>>> 			ARRAY_SIZE(nfs4_acl_bitmap), &hdr);
>>>>=20
>>>> 	rpc_prepare_reply_pages(req, args->acl_pages, 0,
>>>> -				args->acl_len, replen);
>>>> +				args->acl_len, replen + 1);
>>>> 	encode_nops(&hdr);
>>>> }
>>>>=20
>>>> @@ -2811,7 +2811,7 @@ static void
>>>> nfs4_xdr_enc_fs_locations(struct
>>>> rpc_rqst *req,
>>>> 	}
>>>>=20
>>>> 	rpc_prepare_reply_pages(req, (struct page **)&args->page, 0,
>>>> -				PAGE_SIZE, replen);
>>>> +				PAGE_SIZE, replen + 1);
>>>> 	encode_nops(&hdr);
>>>> }
>>>>=20
>>>=20
>>> I'm having trouble with the math here. Why are we pre-emptively
>>> subtracting a word from the tail? The header constants are always
>>> 4-bit=20
>>> aligned because they are calculated as a word count, so I'm not
>>> understanding why we need commit 02ef04e432ba at all.
>>>=20
>>> Can you please explain, Chuck?
>>=20
>> The goal is to allocate a reply buffer that is just large enough
>> to fit the expected reply, and ensure that the variable-length
>> payload will start exactly where the xdr_buf's pages begin.
>>=20
>> In cases where the payload length is not aligned to four bytes,
>> an extra quad has to be allocated in the tail. So, the total
>> reply length is increased by one quad so there is enough space
>> for the XDR pad bytes in the tail.
>>=20
> Right, but we should never hit that problem because the proc->p_arglen
> and proc->p_replen are always in units of 32-bit words.

But buf->pages aren't. The point is to accommodate XDR padding
of the data payload, which goes into the page cache. You want
just the data to go into the page cache, and the padding to
land in the tail.


> IOW: the functions that allocate memory, will always do so in full
> words, hence it should not be necessary for xdr_inline_pages() to
> adjust that allocation.
>=20
> The one thing that we _might_ want to do if we're to do anything at =
all
> is to perhaps adjust tail->iov_base by (xdr->page_len & 3) bytes to
> ensure that we have word-aligned data in the tail.
>=20
> i.e. capture the padding in the remaining bytes in that first word, so
> that xdr_read_pages() sets word aligned values for xdr->p and =
xdr->end.

I'm in favor of hiding the details of that in the generic
XDR code, and I probably should have taken that step in
02ef04e432ba.

So I think we should add a quad to the tail whenever the
expected size of the data payload (ie, payload that goes into
the page cache) is going to be non-word aligned. Otherwise
the tail doesn't need that space. In those cases, the tail
can be eliminated.

The reason to do that is that then the RDMA transport does
not need to register a 4-byte memory region for the XDR
padding.


--
Chuck Lever