From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755699AbYKDSm4 (ORCPT ); Tue, 4 Nov 2008 13:42:56 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752441AbYKDSmq (ORCPT ); Tue, 4 Nov 2008 13:42:46 -0500 Received: from mx2.redhat.com ([66.187.237.31]:34539 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751599AbYKDSmp (ORCPT ); Tue, 4 Nov 2008 13:42:45 -0500 Date: Tue, 4 Nov 2008 13:42:35 -0500 From: Jeff Layton To: Trond Myklebust Cc: Andrew Morton , linux-nfs@vger.kernel.org, linux-kernel@vger.kernel.org, hch@infradead.org Subject: Re: [PATCH] lockd: convert reclaimer thread to kthread interface Message-ID: <20081104134235.0382c873@tleilax.poochiereds.net> In-Reply-To: <1225802508.19864.2.camel@heimdal.trondhjem.org> References: <1225278945-15407-1-git-send-email-jlayton@redhat.com> <20081103131215.75a83236.akpm@linux-foundation.org> <20081103191924.16528bfd@tleilax.poochiereds.net> <1225802508.19864.2.camel@heimdal.trondhjem.org> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 04 Nov 2008 07:41:47 -0500 Trond Myklebust wrote: > On Mon, 2008-11-03 at 19:19 -0500, Jeff Layton wrote: > > On Mon, 3 Nov 2008 13:12:15 -0800 > > Andrew Morton wrote: > > > > > On Wed, 29 Oct 2008 07:15:45 -0400 > > > Jeff Layton wrote: > > > > > > > My understanding is that there is a push to turn the kernel_thread > > > > interface into a non-exported symbol and move all kernel threads to use > > > > the kthread API. This patch changes lockd to use kthread_run to spawn > > > > the reclaimer thread. > > > > > > > > I've made the assumption here that the extra module references taken > > > > when we spawn this thread are unnecessary and removed them. I've also > > > > added a KERN_ERR printk that pops if the thread can't be spawned to warn > > > > the admin that the locks won't be reclaimed. > > > > > > > > I consider this patch 2.6.29 material. > > > > > > > > Signed-off-by: Jeff Layton > > > > --- > > > > fs/lockd/clntlock.c | 14 +++++++++----- > > > > 1 files changed, 9 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/fs/lockd/clntlock.c b/fs/lockd/clntlock.c > > > > index 8307dd6..fcc2378 100644 > > > > --- a/fs/lockd/clntlock.c > > > > +++ b/fs/lockd/clntlock.c > > > > @@ -14,6 +14,7 @@ > > > > #include > > > > #include > > > > #include > > > > +#include > > > > > > > > #define NLMDBG_FACILITY NLMDBG_CLIENT > > > > > > > > @@ -191,11 +192,15 @@ __be32 nlmclnt_grant(const struct sockaddr *addr, const struct nlm_lock *lock) > > > > void > > > > nlmclnt_recovery(struct nlm_host *host) > > > > { > > > > + struct task_struct *task; > > > > + > > > > if (!host->h_reclaiming++) { > > > > nlm_get_host(host); > > > > - __module_get(THIS_MODULE); > > > > - if (kernel_thread(reclaimer, host, CLONE_FS | CLONE_FILES) < 0) > > > > - module_put(THIS_MODULE); > > > > + task = kthread_run(reclaimer, host, "%s-reclaim", host->h_name); > > > > + if (IS_ERR(task)) > > > > + printk(KERN_ERR "lockd: unable to spawn reclaimer " > > > > + "thread. Locks for %s won't be reclaimed! " > > > > + "(%ld)\n", host->h_name, PTR_ERR(task)); > > > > } > > > > } > > > > > > > > @@ -207,7 +212,6 @@ reclaimer(void *ptr) > > > > struct file_lock *fl, *next; > > > > u32 nsmstate; > > > > > > > > - daemonize("%s-reclaim", host->h_name); > > > > allow_signal(SIGKILL); > > > > > > > > down_write(&host->h_rwsem); > > > > @@ -261,5 +265,5 @@ restart: > > > > nlm_release_host(host); > > > > lockd_down(); > > > > unlock_kernel(); > > > > - module_put_and_exit(0); > > > > + return 0; > > > > } > > > > > > Looks OK to me. I assume the SIGKILL handling has been carefully tested? > > > > > > > > > Is it correct to emit a warning and keep going if the thread didn't > > > start? Or would it be safer&saner to fail the whole mount (or whatever > > > syscall we're doing here..) > > > > > > > Forgot to answer this part... > > > > This thread gets kicked off when the server has rebooted and we need to > > reclaim our locks. There isn't a syscall on which we can return an > > error to the user. > > > > Aside from just warning the admin, I'm not sure what we can do here. We > > might be able to start making all syscalls on the mount fail somehow, > > but I don't think we have infrastructure for that and that may be > > overkill anyway. I suppose we could also go to sleep and try to spawn the > > thread again, but there's no guarantee of success there. > > We should consider implementing SIGLOST. That is the closest thing that > we have to a *NIX standard for signalling that remote filesystem state > has been lost. > Very interesting. I hadn't heard of SIGLOST before, but it does seem like something we should implement. CIFS might also be able to use it too. CIFS doesn't have a grace period, so lock reclaims are always iffy... While we're on the subject of signals... Do you have any thoughts/objections to just making the reclaimer thread ignore them altogether? That would simplify the code a bit. I think I may have been wrong before as well. Now that I look closer, I'm not sure that we're actually leaking memory if the reclaimer is signaled. The file_locks do end up not being on the h_granted list anymore, but I think that just keeps the kernel from attempting to reclaim them again (for instance, if a new reclaimer thread is spawned after this one exits). -- Jeff Layton