Re: [PATCH] sunrpc: raise kernel RPC channel buffer size

Re: [PATCH] sunrpc: raise kernel RPC channel buffer size

[Geert Uytterhoeven]

 	Hi Bruce, Roberto,

On Mon, 19 Oct 2020, J. Bruce Fields wrote:
> On Mon, Oct 19, 2020 at 11:33:56AM +0200, Roberto Bergantinos Corpas wrote:
>> Its possible that using AUTH_SYS and mountd manage-gids option a
>> user may hit the 8k RPC channel buffer limit. This have been observed
>> on field, causing unanswered RPCs on clients after mountd fails to
>> write on channel :
>>
>> rpc.mountd[11231]: auth_unix_gid: error writing reply
>>
>> Userland nfs-utils uses a buffer size of 32k (RPC_CHAN_BUF_SIZE), so
>> lets match those two.
>
> Thanks, applying.
>
> That should allow about 4000 group memberships.  If that doesn't do it
> then maybe it's time to rethink....
>
> --b.
>
>>
>> Signed-off-by: Roberto Bergantinos Corpas <rbergant@redhat.com>
>> ---
>>  net/sunrpc/cache.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
>> index baef5ee43dbb..08df4c599ab3 100644
>> --- a/net/sunrpc/cache.c
>> +++ b/net/sunrpc/cache.c
>> @@ -908,7 +908,7 @@ static ssize_t cache_do_downcall(char *kaddr, const char __user *buf,
>>  static ssize_t cache_slow_downcall(const char __user *buf,
>>  				   size_t count, struct cache_detail *cd)
>>  {
>> -	static char write_buf[8192]; /* protected by queue_io_mutex */
>> +	static char write_buf[32768]; /* protected by queue_io_mutex */
>>  	ssize_t ret = -EINVAL;
>>
>>  	if (count >= sizeof(write_buf))

This is now commit 27a1e8a0f79e643d ("sunrpc: raise kernel RPC channel
buffer size") upstream, and increases kernel size by 24 KiB, even if
RPC is not used.

Can this buffer allocated dynamically instead? This code path seems to
be a slow path anyway. If it's critical, perhaps this buffer can be
allocated on first use?

Thanks!

Gr{oetje,eeting}s,

 						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
 							    -- Linus Torvalds

Re: [PATCH] sunrpc: raise kernel RPC channel buffer size

[J. Bruce Fields]

On Fri, Oct 23, 2020 at 11:44:38AM +0200, Geert Uytterhoeven wrote:
> 	Hi Bruce, Roberto,
> 
> On Mon, 19 Oct 2020, J. Bruce Fields wrote:
> >On Mon, Oct 19, 2020 at 11:33:56AM +0200, Roberto Bergantinos Corpas wrote:
> >>Its possible that using AUTH_SYS and mountd manage-gids option a
> >>user may hit the 8k RPC channel buffer limit. This have been observed
> >>on field, causing unanswered RPCs on clients after mountd fails to
> >>write on channel :
> >>
> >>rpc.mountd[11231]: auth_unix_gid: error writing reply
> >>
> >>Userland nfs-utils uses a buffer size of 32k (RPC_CHAN_BUF_SIZE), so
> >>lets match those two.
> >
> >Thanks, applying.
> >
> >That should allow about 4000 group memberships.  If that doesn't do it
> >then maybe it's time to rethink....
> >
> >--b.
> >
> >>
> >>Signed-off-by: Roberto Bergantinos Corpas <rbergant@redhat.com>
> >>---
> >> net/sunrpc/cache.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >>diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
> >>index baef5ee43dbb..08df4c599ab3 100644
> >>--- a/net/sunrpc/cache.c
> >>+++ b/net/sunrpc/cache.c
> >>@@ -908,7 +908,7 @@ static ssize_t cache_do_downcall(char *kaddr, const char __user *buf,
> >> static ssize_t cache_slow_downcall(const char __user *buf,
> >> 				   size_t count, struct cache_detail *cd)
> >> {
> >>-	static char write_buf[8192]; /* protected by queue_io_mutex */
> >>+	static char write_buf[32768]; /* protected by queue_io_mutex */
> >> 	ssize_t ret = -EINVAL;
> >>
> >> 	if (count >= sizeof(write_buf))
> 
> This is now commit 27a1e8a0f79e643d ("sunrpc: raise kernel RPC channel
> buffer size") upstream, and increases kernel size by 24 KiB, even if
> RPC is not used.
> 
> Can this buffer allocated dynamically instead? This code path seems to
> be a slow path anyway. If it's critical, perhaps this buffer can be
> allocated on first use?

Sure.  Actually it is using an allocated buffer typically, see
cache_downcall().

Looking back at the history....  That was added by Trond in 2009
(da77005f0d64 "SUNRPC: Remove the global temporary write buffer in
net/sunrpc/cache.c").  

Before that there's a pre-git change from 2004 which replaced a simple
kmalloc(PAGE_SIZE).

So I guess the point was to be careful about higher-order allocations,
but probably it was overkill.

How about making it a kvmalloc?

--b.

Re: [PATCH] sunrpc: raise kernel RPC channel buffer size

[Roberto Bergantinos Corpas]

Good point Geert !

> How about making it a kvmalloc?

I can post a new patch using kvmalloc, Bruce looks we can also
prescind of queue_io_mutex, what do you think ?

>
> --b.
>

Re: [PATCH] sunrpc: raise kernel RPC channel buffer size

[J. Bruce Fields]

On Sat, Oct 24, 2020 at 03:29:25AM +0200, Roberto Bergantinos Corpas wrote:
> Good point Geert !
> 
> > How about making it a kvmalloc?
> 
> I can post a new patch using kvmalloc, Bruce looks we can also
> prescind of queue_io_mutex, what do you think ?

And revert da77005f0d64, I think. 

Maybe there's something I'm missing, but I don't think we need all that
complexity.

--b.