On Sat, Feb 27, 2010 at 09:33:40AM +1100, Neil Brown wrote: > > [I found this while looking for the current refcount problem > that triggers a warning in svc_recv. This isn't that bug > but is a different refcount bug - NB] And thanks very much for looking into that, I'm worried.... Seems to have appeared some time between v2.6.31 and v2.6.32.2. On a quick skim commits in that range that struck me as worth a second look included 8f55f3c0a013, b0401d725334, and the 4.1 backchannel patches (3ddc8bf5f3 and preceding). Oh, and I also have some very rough notes from when I looked at this before, in case there's anything useful. --b. Re: 2.6.32.2 - WARNING: at lib/kref.c:43 kref_get+0x,23/0x2b() Seen on: 2.6.32.2, 2.6.32.6, 2.6.32.8; probably was OK on 2.6.29.6 and 2.6.31. Is the warning actually warning about anything that's a problem, or can that counter by zero by design? Yes, it's actually a problem. Is probably svc_xprt_get(xprt) in svc_recv() (only obvious kref_get I found on a quick glance through svc_recv). Double-check: svc_recv+0x305/0x7e6 Note next bug is on putting a socket (that we probably shouldn't have!?): - BUG_ON(inode->i_state == I_CLEAR). - Implies clear_inode() was previously called on it. - stack includes kref_put() call in svc_xprt_release, which is indeed put of same xpt_ref field that svc_xprt_get() gets. So, most probably explanation: - We still had a dangling reference to an xprt after putting one. So we ended up doing another get/put pair on it later and trying to free the same socket twice. So, plan: look for svc_xprt_puts (after checking for other stray uses of xpt_ref) and verify that they're all legit. And gets while we're at it: Ignore svc_rdma for now. Those reporters that answered weren't using rdma. Most puts outside of rdma are in svc_xprt.c: - svc_xprt_release (unconditional): 0 to caller (put matched with removal from rq_xprt) - svc_check_conn_limits(): 0 to caller - takes an xprt off a sv_tempsocks list, gets it (and sets XPT_CLOSE) before dropping sv_lock, then enqueues and puts. (Note: enqueue will get, and assign to rq_xprt, if thread found.) - svc_age_temp_xprts: 0 to caller - same pattern as svc_check_conn_limits(). - svc_delete_xprt: 0 to caller (put matched with removal from xpt_list) - if test_and_set_bit of XPT_DEAD succeeds, will svc_xprt_put(), after calling xpo_detach, then (under sv_lock) removing xpt_list. - ALSO unconditionally puts once for each deferred request it finds associated with this request. Is that right? Yup: svc_defer() gets on success, when assigning dr->xprt. - svc_close_xprt: - sets XPT_CLOSE, then if test_and_set_bit of XPT_BUSY succeeds, gets xprt, deletes, clears BUSY, puts. - revisit: - puts associated xprt unconditionally. Also some puts are in fs/nfsd/nfsctl.c, fs/nfsd/nfs4state.c, fs/lockd/svc.c: nfsctl.c: ifs/nfsd/nfsctl.c:__write_ports_delxprt(): - svc_find_xprt() gets a reference; if found: svc_close_xprt, svc_xprt_put. OK. nfs4state.c: free_client: svc_xprt_put(clp->cl_cb_xprt); Looks basically correct: we take reference when we assign that in nfsd4_create_session. Hm. Note we copy pointer to clp->cl_cb_xprt without taking reference? The client holds a reference, though. Looking at cb_xprt use in client xprt code, I can't see any references taken or dropped. This all looks fine. lockd/svc.c: create_lock_listener() looks innocuous. -- 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
