On Tue, Sep 25, 2012 at 03:54:57PM +1000, NeilBrown wrote:
> On Mon, 20 Aug 2012 18:49:15 -0400 "J. Bruce Fields" <bfields@xxxxxxxxxxxx>
> wrote:
>
> > On Mon, Aug 20, 2012 at 06:37:47PM -0400, bfields wrote:
> > > From: "J. Bruce Fields" <bfields@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxxxx
> > > Reported-by: Michael Tokarev <mjt@xxxxxxxxxx>
> > > Tested-by: Michael Tokarev <mjt@xxxxxxxxxx>
> > > Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxx>
> 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.
Oops--thanks for spotting that!
All I wish I had now is an idea for a patch that would make that mistake
less likely in the future. Slather on some comments, I guess....
--b.
--
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
