Re: [PATCH] svcrpc: fix svc_xprt_enqueue/svc_recv busy-looping

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 4353 bytes --]

On Mon, 20 Aug 2012 18:49:15 -0400 "J. Bruce Fields" <bfields@fieldses.org>
wrote:

> On Mon, Aug 20, 2012 at 06:37:47PM -0400, bfields wrote:
> > From: "J. Bruce Fields" <bfields@redhat.com>
> > 
> > The rpc server tries to ensure that there will be room to send a reply
> > before it receives a request.
> > 
> > It does this by tracking, in xpt_reserved, an upper bound on the total
> > size of the replies that is has already committed to for the socket.
> > 
> > Currently it is adding in the estimate for a new reply *before* it
> > checks whether there is space available.  If it finds that there is not
> > space, it then subtracts the estimate back out.
> > 
> > This may lead the subsequent svc_xprt_enqueue to decide that there is
> > space after all.
> > 
> > The results is a svc_recv() that will repeatedly return -EAGAIN, causing
> > server threads to loop without doing any actual work.
> > 
> > Cc: stable@vger.kernel.org
> > Reported-by: Michael Tokarev <mjt@tls.msk.ru>
> > Tested-by: Michael Tokarev <mjt@tls.msk.ru>
> > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > ---
> >  net/sunrpc/svc_xprt.c |    7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > Queuing up for 3.6 absent any objections.--b.
> 
> By the way, one thing I'm still curious about is how this got
> introduced.  mjt bisected it to f03d78db65085609938fdb686238867e65003181
> "net: refine {udp|tcp|sctp}_mem limits", which looks like it just made
> the problem a little more likely.
> 
> The last substantive change to has_wspace logic was Trond's
> 47fcb03fefee2501e79176932a4184fc24d6f8ec, but I have a tough time
> figuring out whether that would have affected it one way or the other.
> 
> As far as I can tell we've always added to xpt_reserved in this way, so
> that svc_recv and svc_xprt_enqueue are comparing different things, and
> surely this was always wrong even if the problem must have been harder
> to trigger before.
> 
> But some of the wspace logic I don't understand, so cc'ing Neil and
> Trond in case they see any other problem I missed.

Hi Bruce et al.

My guess is that 
commit 9c335c0b8daf56b9f73479d00b1dd726e1fcca09
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Tue Oct 26 11:32:03 2010 -0400

    svcrpc: fix wspace-checking race

introduced this problem.  It moved the test for 'do we have enough space'
from before we add in the requirements of a new request, to after.

But I think your patch looks good.

Thanks,
NeilBrown


> 
> --b.
> 
> > 
> > diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> > index 0d693a8..bac973a 100644
> > --- a/net/sunrpc/svc_xprt.c
> > +++ b/net/sunrpc/svc_xprt.c
> > @@ -316,7 +316,6 @@ static bool svc_xprt_has_something_to_do(struct svc_xprt *xprt)
> >   */
> >  void svc_xprt_enqueue(struct svc_xprt *xprt)
> >  {
> > -	struct svc_serv	*serv = xprt->xpt_server;
> >  	struct svc_pool *pool;
> >  	struct svc_rqst	*rqstp;
> >  	int cpu;
> > @@ -362,8 +361,6 @@ void svc_xprt_enqueue(struct svc_xprt *xprt)
> >  				rqstp, rqstp->rq_xprt);
> >  		rqstp->rq_xprt = xprt;
> >  		svc_xprt_get(xprt);
> > -		rqstp->rq_reserved = serv->sv_max_mesg;
> > -		atomic_add(rqstp->rq_reserved, &xprt->xpt_reserved);
> >  		pool->sp_stats.threads_woken++;
> >  		wake_up(&rqstp->rq_wait);
> >  	} else {
> > @@ -640,8 +637,6 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
> >  	if (xprt) {
> >  		rqstp->rq_xprt = xprt;
> >  		svc_xprt_get(xprt);
> > -		rqstp->rq_reserved = serv->sv_max_mesg;
> > -		atomic_add(rqstp->rq_reserved, &xprt->xpt_reserved);
> >  
> >  		/* As there is a shortage of threads and this request
> >  		 * had to be queued, don't allow the thread to wait so
> > @@ -738,6 +733,8 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
> >  		else
> >  			len = xprt->xpt_ops->xpo_recvfrom(rqstp);
> >  		dprintk("svc: got len=%d\n", len);
> > +		rqstp->rq_reserved = serv->sv_max_mesg;
> > +		atomic_add(rqstp->rq_reserved, &xprt->xpt_reserved);
> >  	}
> >  	svc_xprt_received(xprt);
> >  
> > -- 
> > 1.7.9.5
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]