Re: [RFC 2/3] nfsd4: hold a reference on nfs4_client when it is used by a session

["J. Bruce Fields" <bfields@citi.umich.edu>]

On Tue, May 04, 2010 at 10:11:40AM +0300, Benny Halevy wrote:
> On May. 04, 2010, 4:12 +0300, " J. Bruce Fields" <bfields@citi.umich.edu> wrote:
> > On Mon, May 03, 2010 at 06:36:20PM -0400,  J. Bruce Fields wrote:
> >> On Mon, May 03, 2010 at 07:31:48PM +0300, Benny Halevy wrote:
> >>> Although expire_client unhashes the session form the session table
> >>> so no new compounds can find it, there's no refcount to keep the
> >>> nfs4_client structure around while it's in use and referenced
> >>> in the compound state via the session structure.
> >> The code in my for-2.6.35 branch already has the cl_count removed (and
> >> doesn't use it for callbacks any more, instead destroying callbacks
> >> before the client is destroyed).
> >>
> >> So we need to add a new usage count.
> > 
> > (Which you do in the next patch, OK!)
> 
> Right :)
> 
> This patch fixes another race in which a client can be expired using
> expire_client(), not from the laundromat path, while it's being referenced
> by the session since we look up the session while holding just the sessionid
> lock and not the state lock.  Therefore we must take a refcount on the
> client while inside the sessionid lock.
> 
> Looks like the new usage count in your for-2.6.35 world (that I _think_
> could just be a kref now) is needed for delegations as well, right?

We should be able to destroy any delegations, recalls, or other
callbacks before we destroy the client, so I don't think we need a
reference count for those.

--b.

> 
> Benny
> 
> > 
> > --b.
> > 
> >> I'd prefer to call it something
> >> different, since it's being used for something different (cl_users?)
> >> since it's being used for something different, and I'd rather avoid
> >> confusion with the previous one.
> >>
> >> --b.
> >>
> >>
> >>
> >>> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> >>> ---
> >>>  fs/nfsd/nfs4callback.c |    2 +-
> >>>  fs/nfsd/nfs4state.c    |    4 +++-
> >>>  fs/nfsd/nfs4xdr.c      |    1 +
> >>>  fs/nfsd/state.h        |    6 ++++++
> >>>  4 files changed, 11 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> >>> index 7e32bd3..fef1dbe 100644
> >>> --- a/fs/nfsd/nfs4callback.c
> >>> +++ b/fs/nfsd/nfs4callback.c
> >>> @@ -571,7 +571,7 @@ nfsd4_probe_callback(struct nfs4_client *clp)
> >>>  	}
> >>>  
> >>>  	/* the task holds a reference to the nfs4_client struct */
> >>> -	atomic_inc(&clp->cl_count);
> >>> +	get_nfs4_client(clp);
> >>>  
> >>>  	do_probe_callback(clp);
> >>>  }
> >>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >>> index 6dbcaf1..50b75af 100644
> >>> --- a/fs/nfsd/nfs4state.c
> >>> +++ b/fs/nfsd/nfs4state.c
> >>> @@ -1458,8 +1458,10 @@ nfsd4_sequence(struct svc_rqst *rqstp,
> >>>  
> >>>  out:
> >>>  	/* Hold a session reference until done processing the compound. */
> >>> -	if (cstate->session)
> >>> +	if (cstate->session) {
> >>>  		nfsd4_get_session(cstate->session);
> >>> +		get_nfs4_client(cstate->session->se_client);
> >>> +	}
> >>>  	spin_unlock(&sessionid_lock);
> >>>  	/* Renew the clientid on success and on replay */
> >>>  	if (cstate->session) {
> >>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> >>> index 19ff5a3..aed733c 100644
> >>> --- a/fs/nfsd/nfs4xdr.c
> >>> +++ b/fs/nfsd/nfs4xdr.c
> >>> @@ -3313,6 +3313,7 @@ nfs4svc_encode_compoundres(struct svc_rqst *rqstp, __be32 *p, struct nfsd4_compo
> >>>  			dprintk("%s: SET SLOT STATE TO AVAILABLE\n", __func__);
> >>>  			cs->slot->sl_inuse = false;
> >>>  		}
> >>> +		put_nfs4_client(cs->session->se_client);
> >>>  		nfsd4_put_session(cs->session);
> >>>  	}
> >>>  	return 1;
> >>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> >>> index fefeae2..e3c002e 100644
> >>> --- a/fs/nfsd/state.h
> >>> +++ b/fs/nfsd/state.h
> >>> @@ -231,6 +231,12 @@ struct nfs4_client {
> >>>  						/* wait here for slots */
> >>>  };
> >>>  
> >>> +static inline void
> >>> +get_nfs4_client(struct nfs4_client *clp)
> >>> +{
> >>> +	atomic_inc(&clp->cl_count);
> >>> +}
> >>> +
> >>>  /* struct nfs4_client_reset
> >>>   * one per old client. Populates reset_str_hashtbl. Filled from conf_id_hashtbl
> >>>   * upon lease reset, or from upcall to state_daemon (to read in state
> >>> -- 
> >>> 1.6.3.3
> >>>
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>