Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\))
Subject: Re: [PATCH v1] SUNRPC: Remove XDRBUF_SPARSE_PAGES flag in gss_proxy
 upcall
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <CAN-5tyFP6F1Q+gGBLwfwazuaw7WYu-UGfd=Bb0nOmxgU0e_uxw@mail.gmail.com>
Date:   Mon, 30 Nov 2020 14:37:53 -0500
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Trond Myklebust <trond.myklebust@primarydata.com>,
        Simo Sorce <simo@redhat.com>,
        Bruce Fields <bfields@fieldses.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <3ED91324-7BC3-4A55-87B4-4A21177D6C48@oracle.com>
References: <160625754220.280431.690992380938118353.stgit@klimt.1015granger.net>
 <CAN-5tyFP6F1Q+gGBLwfwazuaw7WYu-UGfd=Bb0nOmxgU0e_uxw@mail.gmail.com>
To:     Olga Kornievskaia <aglo@umich.edu>
Precedence: bulk


> On Nov 30, 2020, at 2:33 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
>=20
> On Tue, Nov 24, 2020 at 7:04 PM Chuck Lever <chuck.lever@oracle.com> =
wrote:
>>=20
>> Commit 9dfd87da1aeb ("rpc: fix huge kmalloc's in gss-proxy") added
>> gssp_alloc_receive_pages() to fully allocate the receive buffer
>> for gss_proxy upcalls.
>>=20
>> However, later, 431f6eb3570f ("SUNRPC: Add a label for RPC calls
>> that require allocation on receive") sets the XDRBUF_SPARSE_PAGES
>> flag for this receive buffer anyway. That doesn't appear to have
>> been necessary, since gssp_alloc_receive_pages() still exists.
>=20
> But the gssp_alloc_receive_pages() only allocates the array of page
> pointers not the actual pages, so I believe the flag is still needed
> to have those pages allocated by something? What is allocating those
> pages if not the SPARSE_PAGES method, what am I missing?

Ugh <face palm>

gssp_alloc_receive_pages() will have to allocate those pages.

Again, I don't see any reason to defer page allocation from a context
in which GFP_KERNEL is allowed to one where it isn't, and where we
know exactly how many pages are needed.

I'll respin. Good catch!


>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> net/sunrpc/auth_gss/gss_rpc_xdr.c |    1 -
>> 1 file changed, 1 deletion(-)
>>=20
>> diff --git a/net/sunrpc/auth_gss/gss_rpc_xdr.c =
b/net/sunrpc/auth_gss/gss_rpc_xdr.c
>> index 2ff7b7083eba..44838f6ea25e 100644
>> --- a/net/sunrpc/auth_gss/gss_rpc_xdr.c
>> +++ b/net/sunrpc/auth_gss/gss_rpc_xdr.c
>> @@ -771,7 +771,6 @@ void gssx_enc_accept_sec_context(struct rpc_rqst =
*req,
>>        xdr_inline_pages(&req->rq_rcv_buf,
>>                PAGE_SIZE/2 /* pretty arbitrary */,
>>                arg->pages, 0 /* page base */, arg->npages * =
PAGE_SIZE);
>> -       req->rq_rcv_buf.flags |=3D XDRBUF_SPARSE_PAGES;
>> done:
>>        if (err)
>>                dprintk("RPC:       gssx_enc_accept_sec_context: =
%d\n", err);
>>=20
>>=20

--
Chuck Lever