Re: [PATCH 5/6] NLM: Convert lockd to use kthreads

[Jeff Layton <jlayton@redhat.com>]

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>