Re: [PATCH v2 8/9] nfsd4: keep a reference count on client while in use

[Benny Halevy <bhalevy@panasas.com>]

On May. 12, 2010, 5:40 +0300, " J. Bruce Fields" <bfields@citi.umich.edu> wrote:
> Something still doesn't look quite right: a sequence operation
> increments cl_count from 1 to 2; then, say, an exchangeid in another
> thread expires the client, dropping cl_count from 2 to 1; then the
> laundromat runs, sees cl_count 1, and decides it can expire the
> client.  Meanwhile, the original compound is still running.
> 
> Am I missing something?

Yeah.  exchange_id doesn't touch cl_refcount.  It may call expire_client
explicitly which will unhash the client but will not destroy it if cl_refcount > 0
the laundromat won't the client either after that since it's not on the lru list
any more (and even if it would, it's refcount is still great than zero so it would
have been ignored)

Benny

> 
> --b.
> 
> On Wed, May 12, 2010 at 12:13:54AM +0300, Benny Halevy wrote:
>> Get a refcount on the client on SEQUENCE,
>> Release the refcount and renew the client when all respective compounds completed.
>> Do not expire the client by the laundromat while in use.
>> If the client was expired via another path, free it when the compounds
>> complete and the refcount reaches 0.
>>
>> Note that unhash_client_locked must call list_del_init on cl_lru as
>> it may be called twice for the same client (once from nfs4_laundromat
>> and then from expire_client)
>>
>> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
>> ---
>>  fs/nfsd/nfs4state.c |   27 ++++++++++++++++++++++++---
>>  fs/nfsd/nfs4xdr.c   |    3 ++-
>>  fs/nfsd/state.h     |    1 +
>>  3 files changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 378ee86..1a8fb39 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -701,6 +701,22 @@ free_client(struct nfs4_client *clp)
>>  	kfree(clp);
>>  }
>>  
>> +void
>> +release_session_client(struct nfsd4_session *session)
>> +{
>> +	struct nfs4_client *clp = session->se_client;
>> +
>> +	spin_lock(&client_lock);
>> +	BUG_ON(clp->cl_refcount <= 0);
>> +	if (--clp->cl_refcount <= 0) {
>> +		free_client(clp);
>> +		session->se_client = NULL;
>> +	} else if (clp->cl_refcount == 1)
>> +		renew_client_locked(clp);
>> +	spin_unlock(&client_lock);
>> +	nfsd4_put_session(session);
>> +}
>> +
>>  /* must be called under the client_lock */
>>  static inline void
>>  unhash_client_locked(struct nfs4_client *clp)
>> @@ -1477,8 +1493,7 @@ out:
>>  	/* Hold a session reference until done processing the compound. */
>>  	if (cstate->session) {
>>  		nfsd4_get_session(cstate->session);
>> -		/* Renew the clientid on success and on replay */
>> -		renew_client_locked(session->se_client);
>> +		session->se_client->cl_refcount++;
>>  	}
>>  	spin_unlock(&client_lock);
>>  	dprintk("%s: return %d\n", __func__, ntohl(status));
>> @@ -2599,7 +2614,13 @@ nfs4_laundromat(void)
>>  				clientid_val = t;
>>  			break;
>>  		}
>> -		list_move(&clp->cl_lru, &reaplist);
>> +		if (clp->cl_refcount > 1) {
>> +			dprintk("NFSD: client in use (clientid %08x)\n",
>> +				clp->cl_clientid.cl_id);
>> +			continue;
>> +		}
>> +		unhash_client_locked(clp);
>> +		list_add(&clp->cl_lru, &reaplist);
>>  	}
>>  	spin_unlock(&client_lock);
>>  	list_for_each_safe(pos, next, &reaplist) {
>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> index 5c2de47..126d0ca 100644
>> --- a/fs/nfsd/nfs4xdr.c
>> +++ b/fs/nfsd/nfs4xdr.c
>> @@ -3313,7 +3313,8 @@ 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;
>>  		}
>> -		nfsd4_put_session(cs->session);
>> +		/* Renew the clientid on success and on replay */
>> +		release_session_client(cs->session);
>>  	}
>>  	return 1;
>>  }
>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>> index 37e1dff..f263174 100644
>> --- a/fs/nfsd/state.h
>> +++ b/fs/nfsd/state.h
>> @@ -419,6 +419,7 @@ extern int nfs4_has_reclaimed_state(const char *name, bool use_exchange_id);
>>  extern void nfsd4_recdir_purge_old(void);
>>  extern int nfsd4_create_clid_dir(struct nfs4_client *clp);
>>  extern void nfsd4_remove_clid_dir(struct nfs4_client *clp);
>> +extern void release_session_client(struct nfsd4_session *);
>>  
>>  static inline void
>>  nfs4_put_stateowner(struct nfs4_stateowner *so)
>> -- 
>> 1.6.5.1
>>
> --
> 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