Re: The recent kref_put warning (was: [PATCH] sunrpc: remove unnecessary svc_xprt_put)

["J. Bruce Fields" <bfields@fieldses.org>]

On Mon, Mar 01, 2010 at 06:19:56PM -0500, J. Bruce Fields wrote:
> On Mon, Mar 01, 2010 at 09:50:15AM -0500, J. Bruce Fields wrote:
> > On Mon, Mar 01, 2010 at 04:51:14PM +1100, Neil Brown wrote:
> > > On Sun, 28 Feb 2010 22:46:47 -0500
> > > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > > 
> > > > On Mon, Mar 01, 2010 at 10:57:34AM +1100, Neil Brown wrote:
> > > > > No, you are correct.  "return 0" is wrong, it should be "return -EAGAIN",
> > > > > both in the XPT_CLOSE case and the XPT_LISTENER case.
> > > > > 
> > > > > I observed that in both those cases, 'len' remained at 0 and we didn't do
> > > > > much else but 'return len', so I optimised.
> > > > > I forgot to factor in:
> > > > > 
> > > > > 	if (len == 0 || len == -EAGAIN) {
> > > > > 		rqstp->rq_res.len = 0;
> > > > > 		svc_xprt_release(rqstp);
> > > > > 		return -EAGAIN;
> > > > > 	}
> > > > > 
> > > > > So the svc_xprt_release needs to be moved in there as well, I'm not sure
> > > > > about the rq_res.len = 0.
> > > > > Maybe that was a bad case of premature-optimisation :-)
> > > > > 
> > > > > We should probably leave that last else clause as it is and just have a
> > > > > single return from the function.
> > > > 
> > > > OK, so the below is what I'm thinking of sending, after some testing;
> > > > really just a split-up version of your patches (uh, so credits may be
> > > > wrong) with the final cleanup removed:
> > > 
> > > Credits and code look OK the me, thanks.
> 
> And, by the way, this is all ready to submit--but I'd like to avoid
> having to revert anything more, and as part of that I'd greatly
> appreciate any testing results, however partial.

(I've run my basic regression tests, but they were never enough to
reproduce the refcnt warning others were seeing.)

--b.