From 8e60029f403781b8a63b7ffdb7dc1faff6ca651e Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Mon, 11 Feb 2008 10:00:20 -0500 Subject: NFS: fix reference counting for NFSv4 callback thread The reference counting for the NFSv4 callback thread stays artificially high. When this thread comes down, it doesn't properly tear down the svc_serv, causing a memory leak. In my testing on an older kernel on x86_64, memory would leak out of the 8k kmalloc slab. So, we're leaking at least a page of memory every time the thread comes down. svc_create() creates the svc_serv with a sv_nrthreads count of 1, and then svc_create_thread() increments that count. Whenever the callback thread is started it has a sv_nrthreads count of 2. When coming down, it calls svc_exit_thread() which decrements that count and if it hits 0, it tears everything down. That never happens here since the count is always at 2 when the thread exits. The problem is that nfs_callback_up() should be calling svc_destroy() on the svc_serv on both success and failure. This is how lockd_up_proto() handles the reference counting, and doing that here fixes the leak. Signed-off-by: Jeff Layton Signed-off-by: Trond Myklebust --- fs/nfs/callback.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) (limited to 'fs/nfs/callback.c') diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c index bd185a572a23..ecc06c619494 100644 --- a/fs/nfs/callback.c +++ b/fs/nfs/callback.c @@ -105,7 +105,7 @@ static void nfs_callback_svc(struct svc_rqst *rqstp) */ int nfs_callback_up(void) { - struct svc_serv *serv; + struct svc_serv *serv = NULL; int ret = 0; lock_kernel(); @@ -122,24 +122,30 @@ int nfs_callback_up(void) ret = svc_create_xprt(serv, "tcp", nfs_callback_set_tcpport, SVC_SOCK_ANONYMOUS); if (ret <= 0) - goto out_destroy; + goto out_err; nfs_callback_tcpport = ret; dprintk("Callback port = 0x%x\n", nfs_callback_tcpport); ret = svc_create_thread(nfs_callback_svc, serv); if (ret < 0) - goto out_destroy; + goto out_err; nfs_callback_info.serv = serv; wait_for_completion(&nfs_callback_info.started); out: + /* + * svc_create creates the svc_serv with sv_nrthreads == 1, and then + * svc_create_thread increments that. So we need to call svc_destroy + * on both success and failure so that the refcount is 1 when the + * thread exits. + */ + if (serv) + svc_destroy(serv); mutex_unlock(&nfs_callback_mutex); unlock_kernel(); return ret; -out_destroy: +out_err: dprintk("Couldn't create callback socket or server thread; err = %d\n", ret); - svc_destroy(serv); -out_err: nfs_callback_info.users--; goto out; } -- cgit From 1227a74e2e0217a4ca155d1677bdbf5f69e32bed Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Tue, 19 Feb 2008 12:51:35 -0500 Subject: NFS: flush signals before taking down callback thread Now that the reference counting on the callback thread is working as expected, it uncovers another problem. Peter Staubach noticed while testing that patch on an older kernel that he would occasionally see this printk in rpc_register fire: "RPC: failed to contact portmap (errno -512). The NFSv4 callback thread is signaled by nfs_callback_down(), but never flushes that signal. All of the shutdown processing is done with that signal pending. This makes it fail the call to unregister the port with the portmapper. In actuality, this rpc_register call isn't necessary at all since the port isn't actually registered with the portmapper anymore. Regardless, there doesn't seem to be any reason to leave the signal pending while the thread is being shut down and flushing it should generally silence that printk. Signed-off-by: Jeff Layton Signed-off-by: Trond Myklebust --- fs/nfs/callback.c | 1 + 1 file changed, 1 insertion(+) (limited to 'fs/nfs/callback.c') diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c index ecc06c619494..0d784f6f609f 100644 --- a/fs/nfs/callback.c +++ b/fs/nfs/callback.c @@ -93,6 +93,7 @@ static void nfs_callback_svc(struct svc_rqst *rqstp) svc_process(rqstp); } + flush_signals(current); svc_exit_thread(rqstp); nfs_callback_info.pid = 0; complete(&nfs_callback_info.stopped); -- cgit From 5216a8e70e25b01cbd2915cd0442fb96deb2c262 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Thu, 21 Feb 2008 10:57:45 +0300 Subject: Wrap buffers used for rpc debug printks into RPC_IFDEBUG Sorry for the noise, but here's the v3 of this compilation fix :) There are some places, which declare the char buf[...] on the stack to push it later into dprintk(). Since the dprintk sometimes (if the CONFIG_SYSCTL=n) becomes an empty do { } while (0) stub, these buffers cause gcc to produce appropriate warnings. Wrap these buffers with RPC_IFDEBUG macro, as Trond proposed, to compile them out when not needed. Signed-off-by: Pavel Emelyanov Acked-by: J. Bruce Fields Signed-off-by: Trond Myklebust --- fs/nfs/callback.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'fs/nfs/callback.c') diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c index 0d784f6f609f..66648dd92d97 100644 --- a/fs/nfs/callback.c +++ b/fs/nfs/callback.c @@ -172,7 +172,7 @@ void nfs_callback_down(void) static int nfs_callback_authenticate(struct svc_rqst *rqstp) { struct nfs_client *clp; - char buf[RPC_MAX_ADDRBUFLEN]; + RPC_IFDEBUG(char buf[RPC_MAX_ADDRBUFLEN]); /* Don't talk to strangers */ clp = nfs_find_client(svc_addr(rqstp), 4); -- cgit