Re: [PATCH v2 01/10] sunrpc: Remove backchannel check in svc_init_buffer()

[Chuck Lever <cel@kernel.org>]

On 4/21/25 8:16 AM, Jeff Layton wrote:
> On Sat, 2025-04-19 at 13:28 -0400, cel@kernel.org wrote:
>> From: Chuck Lever <chuck.lever@oracle.com>
>>
>> A backchannel service might use forechannel send and receive
>> buffers, but svc_recv() still calls svc_alloc_arg() on backchannel
>> svc_rqsts, so it will populate the svc_rqst's rq_pages[] array
>> anyway. The "is backchannel" check in svc_init_buffer() saves us
>> nothing.
>>
>> In a moment, I plan to replace the rq_pages[] array with a
>> dynamically-allocated piece of memory, and svc_init_buffer() is
>> where that memory will get allocated. Backchannel requests actually
>> do use that array, so it has to be available to handle those
>> requests without a segfault.
>>
>> XXX: Or, make svc_alloc_arg() ignore backchannel requests too?
>>      Could set rqstp->rq_maxpages to zero.
>>
> 
> Maybe I'm confused here, but the backchannel still needs some pages to
> do its thing? I guess the main change here is that the pages are
> allocated at svc_create time instead of waiting until later?

The backchannel does not use the pages in svc_rqst::rq_pages, it's
rq_arg::pages comes from the RPC client's page allocator. Currently,
svc_init_buffer() skips allocating pages in rq_pages for that reason.

Except that, svc_rqst::rq_pages is filled anyway when a backchannel
svc_rqst is passed to svc_recv() -> and then to svc_alloc_arg().

This isn't really a problem at the moment, except that these pages are
allocated but then never used AFAICT.

The problem is that in 03/10, in addition to populating rq_pages[],
svc_init_buffer() also now allocates the rq_pages array itself. If
that is skipped, then svc_alloc_args() chases a NULL pointer.

As I mentioned in the patch description, there are two or three ways to
handle this. I'm not sure the way proposed here is best, but it does
avoid introducing extra conditional logic in svc_alloc_args(), which is
a hot path.

Let me know if you have a better idea. I will try to bolster the patch
description here as well.


>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>  net/sunrpc/svc.c | 4 ----
>>  1 file changed, 4 deletions(-)
>>
>> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
>> index e7f9c295d13c..8ce3e6b3df6a 100644
>> --- a/net/sunrpc/svc.c
>> +++ b/net/sunrpc/svc.c
>> @@ -640,10 +640,6 @@ svc_init_buffer(struct svc_rqst *rqstp, unsigned int size, int node)
>>  {
>>  	unsigned long pages, ret;
>>  
>> -	/* bc_xprt uses fore channel allocated buffers */
>> -	if (svc_is_backchannel(rqstp))
>> -		return true;
>> -
>>  	pages = size / PAGE_SIZE + 1; /* extra page as we hold both request and reply.
>>  				       * We assume one is at most one page
>>  				       */
> 
> Acked-by: Jeff Layton <jlayton@kernel.org>


-- 
Chuck Lever