Hey Bruce,
On Fri, Jan 25, 2013 at 01:45:22PM -0500, J. Bruce Fields wrote:
> On Thu, Jan 24, 2013 at 06:59:30PM -0600, Ben Myers wrote:
> > At 1020 threads the send buffer size wraps and becomes negative causing
> > the nfs server to grind to a halt.
>
> Actually this appears to be a problem in only one place:
>
> > @@ -525,18 +575,17 @@ static int svc_udp_recvfrom(struct svc_r
> > size_t len;
> > int err;
> >
> > - if (test_and_clear_bit(XPT_CHNGBUF, &svsk->sk_xprt.xpt_flags))
> > - /* udp sockets need large rcvbuf as all pending
> > - * requests are still in that buffer. sndbuf must
> > - * also be large enough that there is enough space
> > - * for one reply per thread. We count all threads
> > - * rather than threads in a particular pool, which
> > - * provides an upper bound on the number of threads
> > - * which will access the socket.
> > - */
> > - svc_sock_setbufsize(svsk->sk_sock,
> > - (serv->sv_nrthreads+3) * serv->sv_max_mesg,
> > - (serv->sv_nrthreads+3) * serv->sv_max_mesg);
>
> Elsewhere it's not dependent on the number of threads:
>
> > svc_sock_setbufsize(svsk->sk_sock,
> > - 3 * svsk->sk_xprt.xpt_server->sv_max_mesg,
> > - 3 * svsk->sk_xprt.xpt_server->sv_max_mesg);
> ...
> > - svc_sock_setbufsize(svsk->sk_sock, 4 * serv->sv_max_mesg,
> > - 4 * serv->sv_max_mesg);
>
> Since sv_max_mesg is at most a megabyte, those won't overflow.
>
> If you saw an overflow in the tcp case, I suspect that was with a kernel
> before 9660439861aa8dbd5e2b8087f33e20760c2c9afc "svcrpc: take advantage
> of tcp autotuning"?
Correct. This is an updated version of an old patch I've been carrying around
awhile.
> Honestly I'm a bit mystified by the udp comment in the code above; do we
> *really* need buffers that large?
I've been assuming that the author was used to running with just a few nfsd
threads, and didn't expect that someone would try a large number.
> And if so, why don't we set them to
> that size at the start in svc_udp_init?
Yeah, that's strange..
> But in the udp case I'm inclined to do just the minimum to fix the
> overflow and otherwise leave the code alone--it doesn't get as much use
> and testing as it once did, so it's probably better to leave it alone.
>
> So something like the below.
It looks reasonable. I'll give it a try.
> This still leaves the issue of tcp buffer size under memory pressure.
Yep. That may be why this patch works for me.
Thanks,
Ben
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index 0a148c9..f3a525c 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -525,7 +525,7 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
> size_t len;
> int err;
>
> - if (test_and_clear_bit(XPT_CHNGBUF, &svsk->sk_xprt.xpt_flags))
> + if (test_and_clear_bit(XPT_CHNGBUF, &svsk->sk_xprt.xpt_flags)) {
> /* udp sockets need large rcvbuf as all pending
> * requests are still in that buffer. sndbuf must
> * also be large enough that there is enough space
> @@ -534,9 +534,13 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
> * provides an upper bound on the number of threads
> * which will access the socket.
> */
> + unsigned int threads = serv->sv_nrthreads;
> + /* arbitrary limit, mainly just to prevent overflow: */
> + threads = min(threads, 128U);
> svc_sock_setbufsize(svsk->sk_sock,
> - (serv->sv_nrthreads+3) * serv->sv_max_mesg,
> - (serv->sv_nrthreads+3) * serv->sv_max_mesg);
> + (threads+3) * serv->sv_max_mesg,
> + (threads+3) * serv->sv_max_mesg);
> + }
>
> clear_bit(XPT_DATA, &svsk->sk_xprt.xpt_flags);
> skb = NULL;
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
