RE: [PATCH 1/1] nfs: fix race between renewd, umount, and the state manager in V4.1

[Trond Myklebust <Trond.Myklebust@netapp.com>]

On Fri, 2010-01-22 at 12:01 -0800, Batsakis, Alexandros wrote: 
> 
> > -----Original Message-----
> > From: Myklebust, Trond
> > Sent: Friday, January 22, 2010 11:42 AM
> > To: Batsakis, Alexandros
> > Cc: linux-nfs@vger.kernel.org
> > Subject: RE: [PATCH 1/1] nfs: fix race between renewd, umount, and the
> > state manager in V4.1
> > 
> > On Fri, 2010-01-22 at 11:30 -0800, Batsakis, Alexandros wrote:
> > >
> > > > -----Original Message-----
> > > > From: Myklebust, Trond
> > > > Sent: Friday, January 22, 2010 10:47 AM
> > > > To: Batsakis, Alexandros
> > > > Cc: linux-nfs@vger.kernel.org; Myklebust, Trond
> > > > Subject: Re: [PATCH 1/1] nfs: fix race between renewd, umount, and
> > the
> > > > state manager in V4.1
> > > >
> > > > On Thu, 2010-01-21 at 20:51 -0800, Alexandros Batsakis wrote:
> > > >
> > > > > struct rpc_cred *nfs4_get_setclientid_cred(struct nfs_client
> > *clp)
> > > > > @@ -878,7 +888,11 @@ void nfs4_schedule_state_manager(struct
> > > > nfs_client *clp)
> > > > >  	if (test_and_set_bit(NFS4CLNT_MANAGER_RUNNING, &clp-
> > >cl_state) !=
> > > > 0)
> > > > >  		return;
> > > > >  	__module_get(THIS_MODULE);
> > > > > -	atomic_inc(&clp->cl_count);
> > > > > +	if (!atomic_inc_not_zero(&clp->cl_count)) {
> > > > > +		nfs4_clear_state_manager_bit(clp);
> > > > > +		module_put(THIS_MODULE);
> > > > > +		return;
> > > > > +	}
> > > >
> > > > The use of atomic_inc_not_zero() should not be necessary here.
> > Anybody
> > > > who is calling this function without holding a reference to clp is
> > > > doing
> > > > something fundamentally wrong.
> > >
> > > Agreed, but how can we enforce this (especially with asynchronous
> > RPC) ?
> > > For example what will happen if _during_ umount (after
> > nfs_put_client()), an NFS call that was sleeping e.g. waiting for the
> > server to come up, reaches the server and the response causes the state
> > manager to start ?
> > > Any ideas ?
> > 
> > What would you expect to see running during umount? Nobody can be
> > holding a reference to the vfsmount at that point, so there should be
> > no
> > active processes around to make NFS calls.
> > 
> 
> renewd is still running and even if we kill the daemon, maybe there is a sequence_done pending

Then the correct thing to do is to fix the renewd kill process. Once
that is done, there should be no reason for any sequence_done calls to
be pending (see reason above).

Trond