Messages in this thread | | | Date | Wed, 9 Jan 2008 13:08:11 -0500 | From | Jeff Layton <> | Subject | Re: [PATCH 5/6] NLM: Convert lockd to use kthreads |
| |
On Wed, 9 Jan 2008 17:45:06 +0000 Christoph Hellwig <hch@infradead.org> wrote:
> On Tue, Jan 08, 2008 at 02:33:17PM -0500, Jeff Layton wrote: > > - struct svc_serv * serv; > > - int error = 0; > > + struct svc_serv *serv; > > + struct svc_rqst *rqstp; > > + int error = 0; > > > > mutex_lock(&nlmsvc_mutex); > > /* > > * Check whether we're already up and running. > > */ > > - if (nlmsvc_pid) { > > + if (nlmsvc_task) { > > if (proto) > > error = make_socks(nlmsvc_serv, proto); > > While equivalent I think it would be clener to check for nlmsvc_serv > above as that'swhat we're passing to make_socks. But I think the > whole of lockd_up could use a little makeover, but that's for later. >
Probably so. If I respin, I'll plan to fix that too.
> > void > > lockd_down(void) > > { > > mutex_lock(&nlmsvc_mutex); > > if (nlmsvc_users) { > > if (--nlmsvc_users) > > goto out; > > + } else { > > + printk(KERN_ERR "lockd_down: no users! task=%p\n", > > + nlmsvc_task); > > + BUG(); > > } > > + if (!nlmsvc_task) { > > + printk(KERN_ERR "lockd_down: no lockd running.\n"); > > + BUG(); > > } > > + kthread_stop(nlmsvc_task); > > I think all this user/foo checking here should be BUG_ONs as it's > quite fatal errors. > > e.g. > > void > lockd_down(void) > { > mutex_lock(&nlmsvc_mutex); > > BUG_ON(!nlmsvc_task); > BUG_ON(!nlmsvc_users); > > if (!--nlmsvc_users) > kthread_stop(nlmsvc_task); > mutex_unlock(&nlmsvc_mutex); > } > > > same applies for similar checks in lockd_up aswell. >
With this patch the lockd_down checks should now be BUGs. I decided not to do that in lockd_up. If there's an error within the main lockd loop, it can exit without being requested to do so. If someone then calls lockd_up then the counts will be off and the check will fire.
It seems like if we're going to make the check in lockd_up be a BUG, then we should also BUG rather than letting lockd exit prematurely.
-- Jeff Layton <jlayton@redhat.com>
| |