diff mbox series

[v1] SUNRPC: Remove XDRBUF_SPARSE_PAGES flag in gss_proxy upcall

Message ID 160625754220.280431.690992380938118353.stgit@klimt.1015granger.net (mailing list archive)
State New
Headers show
Series [v1] SUNRPC: Remove XDRBUF_SPARSE_PAGES flag in gss_proxy upcall | expand

Commit Message

Chuck Lever Nov. 24, 2020, 10:39 p.m. UTC
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.

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.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/auth_gss/gss_rpc_xdr.c |    1 -
 1 file changed, 1 deletion(-)

Comments

Olga Kornievskaia Nov. 30, 2020, 7:33 p.m. UTC | #1
On Tue, Nov 24, 2020 at 7:04 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> 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.
>
> 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.

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?


> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  net/sunrpc/auth_gss/gss_rpc_xdr.c |    1 -
>  1 file changed, 1 deletion(-)
>
> 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 |= XDRBUF_SPARSE_PAGES;
>  done:
>         if (err)
>                 dprintk("RPC:       gssx_enc_accept_sec_context: %d\n", err);
>
>
Chuck Lever Nov. 30, 2020, 7:37 p.m. UTC | #2
> On Nov 30, 2020, at 2:33 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> 
> On Tue, Nov 24, 2020 at 7:04 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>> 
>> 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.
>> 
>> 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.
> 
> 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(-)
>> 
>> 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 |= XDRBUF_SPARSE_PAGES;
>> done:
>>        if (err)
>>                dprintk("RPC:       gssx_enc_accept_sec_context: %d\n", err);
>> 
>> 

--
Chuck Lever
diff mbox series

Patch

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 |= XDRBUF_SPARSE_PAGES;
 done:
 	if (err)
 		dprintk("RPC:       gssx_enc_accept_sec_context: %d\n", err);