[pnfs] [PATCH 4/12] use kref for nfsd nfs4_client
Benny Halevy
bhalevy at panasas.com
Tue May 8 15:26:12 EDT 2007
J. Bruce Fields wrote:
> If this would also apply to mainline, I'd be happy to submit it, if it
> came with a comment justifying the changes.... (Which are actually
> two--it switches to kref, but also seems to make the reference counting
> a little finer-grained?)
Cool. While working on a patch over mainline (nfs-2.6.git)
I realized I have a couple of bugs in the patch below.
[I can't test the patches over mainline as it doesn't boot
on our test machines (I wonder if other folks see breakage in
head-of-line git trees, cause we currently have the same problem
also with scsi-misc-2.6 and linux-block :-/)]
I completely agree with your analysis that it can logically be
separated into two patches, one that replaces the current mechanism
to using kref and the other making ref counting finer-grained.
So the first step can be justified in mainline as in nfsd4_probe_callback's
out_rpciod error case atomic_dec(&clp->cl_count) won't free up the client
structure if it was expired concurrently. expire_client shuts down the
callback rpc path so that may wait for nfsd4_probe_callback to finish
but I'm not sure. At any rate, calling put_nfs4_client() there is
more robust.
Doing finer grained ref counting seem to make sense more in the pnfs
context when we do that in the layout* functions. Let me know if you
want that part too in the mainline (it's cleaner IMO, but I don't think
it's necessary)
Benny
>
> --b.
>
> On Mon, May 07, 2007 at 10:34:34PM +0300, Benny Halevy wrote:
>
>> >From 514cd510ad1aa4bd7930eecb64cc731c1396e6e9 Mon Sep 17 00:00:00 2001
>> From: Benny Halevy <bhalevy at ns1.bhalevy.com>
>> Date: Mon, 7 May 2007 16:36:49 +0300
>> Subject: [PATCH] use kref for nfsd nfs4_client
>>
>> ---
>> fs/nfsd/nfs4callback.c | 8 +------
>> fs/nfsd/nfs4state.c | 50 +++++++++++++++++++++++++++++--------------
>> include/linux/nfsd/state.h | 3 +-
>> 3 files changed, 36 insertions(+), 25 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
>> index cd88b1e..c2f4732 100644
>> --- a/fs/nfsd/nfs4callback.c
>> +++ b/fs/nfsd/nfs4callback.c
>> @@ -520,9 +520,6 @@ nfsd4_probe_callback(struct nfs4_client *clp)
>>
>> cb->cb_client = clnt;
>>
>> - /* the task holds a reference to the nfs4_client struct */
>> - atomic_inc(&clp->cl_count);
>> -
>> msg.rpc_cred = nfsd4_lookupcred(clp,0);
>> if (IS_ERR(msg.rpc_cred))
>> goto out_rpciod;
>> @@ -536,7 +533,6 @@ nfsd4_probe_callback(struct nfs4_client *clp)
>> return;
>>
>> out_rpciod:
>> - atomic_dec(&clp->cl_count);
>> rpciod_down();
>> cb->cb_client = NULL;
>> out_clnt:
>> @@ -558,12 +554,10 @@ nfs4_cb_null(struct rpc_task *task, void *dummy)
>> if (task->tk_status < 0) {
>> dprintk("NFSD: callback establishment to client %.*s failed\n",
>> (int)clp->cl_name.len, clp->cl_name.data);
>> - goto out;
>> + return;
>> }
>> atomic_set(&cb->cb_set, 1);
>> dprintk("NFSD: callback set to client %u.%u.%u.%u\n", NIPQUAD(addr));
>> -out:
>> - put_nfs4_client(clp);
>> }
>>
>> static const struct rpc_call_ops nfs4_cb_null_ops = {
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index e33faa9..d1355b2 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -535,20 +535,28 @@ alloc_client(struct xdr_netobj name)
>> return clp;
>> }
>>
>> -static inline void
>> -free_client(struct nfs4_client *clp)
>> +static inline struct nfs4_client *
>> +get_nfs4_client(struct nfs4_client *clp)
>> {
>> + kref_get(&clp->cl_ref);
>> + return clp;
>> +}
>> +
>> +static void
>> +free_nfs4_client(struct kref *kref)
>> +{
>> + struct nfs4_client *clp = container_of(kref, struct nfs4_client, cl_ref);
>> if (clp->cl_cred.cr_group_info)
>> put_group_info(clp->cl_cred.cr_group_info);
>> kfree(clp->cl_name.data);
>> kfree(clp);
>> }
>>
>> -void
>> +static inline void
>> put_nfs4_client(struct nfs4_client *clp)
>> {
>> - if (atomic_dec_and_test(&clp->cl_count))
>> - free_client(clp);
>> + if (clp)
>> + kref_put(&clp->cl_ref, free_nfs4_client);
>> }
>>
>> static void
>> @@ -573,8 +581,8 @@ expire_client(struct nfs4_client *clp)
>> struct nfs41_session *ses;
>> struct list_head reaplist;
>>
>> - dprintk("NFSD: expire_client cl_count %d\n",
>> - atomic_read(&clp->cl_count));
>> + dprintk("NFSD: expire_client refcount %d\n",
>> + atomic_read(&clp->cl_ref.refcount));
>>
>> shutdown_callback_client(clp);
>>
>> @@ -632,7 +640,7 @@ create_client(struct xdr_netobj name, char *recdir) {
>> if (!(clp = alloc_client(name)))
>> goto out;
>> memcpy(clp->cl_recdir, recdir, HEXDIR_LEN);
>> - atomic_set(&clp->cl_count, 1);
>> + kref_init(&clp->cl_ref);
>> atomic_set(&clp->cl_callback.cb_set, 0);
>> INIT_LIST_HEAD(&clp->cl_idhash);
>> INIT_LIST_HEAD(&clp->cl_strhash);
>> @@ -751,7 +759,7 @@ find_confirmed_client(clientid_t *clid)
>>
>> list_for_each_entry(clp, &conf_id_hashtbl[idhashval], cl_idhash) {
>> if (cmp_clid(&clp->cl_clientid, clid))
>> - return clp;
>> + return get_nfs4_client(clp);
>> }
>> return NULL;
>> }
>> @@ -764,7 +772,7 @@ find_unconfirmed_client(clientid_t *clid)
>>
>> list_for_each_entry(clp, &unconf_id_hashtbl[idhashval], cl_idhash) {
>> if (cmp_clid(&clp->cl_clientid, clid))
>> - return clp;
>> + return get_nfs4_client(clp);
>> }
>> return NULL;
>> }
>> @@ -776,7 +784,7 @@ find_confirmed_client_by_str(const char *dname, unsigned int hashval)
>>
>> list_for_each_entry(clp, &conf_str_hashtbl[hashval], cl_strhash) {
>> if (same_name(clp->cl_recdir, dname))
>> - return clp;
>> + return get_nfs4_client(clp);
>> }
>> return NULL;
>> }
>> @@ -788,7 +796,7 @@ find_unconfirmed_client_by_str(const char *dname, unsigned int hashval)
>>
>> list_for_each_entry(clp, &unconf_str_hashtbl[hashval], cl_strhash) {
>> if (same_name(clp->cl_recdir, dname))
>> - return clp;
>> + return get_nfs4_client(clp);
>> }
>> return NULL;
>> }
>> @@ -929,7 +937,7 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_setclientid *setclid)
>> };
>> nfs4_verifier clverifier = setclid->se_verf;
>> unsigned int strhashval;
>> - struct nfs4_client *conf, *unconf, *new;
>> + struct nfs4_client *conf = NULL, *unconf = NULL, *new;
>> int status;
>> char dname[HEXDIR_LEN];
>>
>> @@ -1073,6 +1081,8 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_setclientid *setclid)
>> status = nfs_ok;
>> out:
>> nfs4_unlock_state();
>> + put_nfs4_client(conf);
>> + put_nfs4_client(unconf);
>> return status;
>> }
>>
>> @@ -1083,7 +1093,7 @@ void nfsd4_setup_callback_channel(void)
>>
>> int nfsd4_exchange_id(struct svc_rqst *rqstp, struct nfsd4_exchange_id *clid)
>> {
>> - struct nfs4_client *unconf, *conf, *new;
>> + struct nfs4_client *unconf = NULL, *conf = NULL, *new;
>> int status;
>> unsigned int strhashval;
>> char dname[HEXDIR_LEN];
>> @@ -1175,7 +1185,8 @@ out_copy:
>>
>> out:
>> nfs4_unlock_state();
>> -
>> + put_nfs4_client(conf);
>> + put_nfs4_client(conf);
>> dprintk("nfsd4_exchange_id returns %d\n", status);
>> return status;
>> }
>> @@ -1267,9 +1278,11 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp, struct nfsd4_setclientid_confi
>> if (conf) {
>> nfsd4_remove_clid_dir(conf);
>> expire_client(conf);
>> + put_nfs4_client(conf);
>> }
>> move_to_confirmed(unconf);
>> conf = unconf;
>> + unconf = NULL;
>> status = nfs_ok;
>> }
>> } else if ((!conf || (conf && !cmp_verf(&conf->cl_confirm, &confirm)))
>> @@ -1290,6 +1303,8 @@ out:
>> if (!status)
>> nfsd4_probe_callback(conf);
>> nfs4_unlock_state();
>> + put_nfs4_client(conf);
>> + put_nfs4_client(unconf);
>> return status;
>> }
>>
>> @@ -1343,7 +1358,6 @@ int nfsd4_create_session(struct svc_rqst *rqstp, struct nfsd4_create_session *se
>> dprintk("Got a create_session replay!\n");
>> goto out_replay;
>> }
>> -
>> }
>> else if (unconf) {
>> if (!cmp_creds(&unconf->cl_cred, &rqstp->rq_cred) || (ip_addr != unconf->cl_addr)) {
>> @@ -1359,7 +1373,9 @@ int nfsd4_create_session(struct svc_rqst *rqstp, struct nfsd4_create_session *se
>> unconf->cl_seqid++;
>> move_to_confirmed(unconf);
>> nfsd4_probe_callback(unconf);
>> + put_nfs4_client(conf);
>> conf = unconf;
>> + unconf = NULL;
>> }
>> status = alloc_init_session(conf, session);
>>
>> @@ -1375,6 +1391,8 @@ out_replay:
>>
>> out:
>> nfs4_unlock_state();
>> + put_nfs4_client(conf);
>> + put_nfs4_client(unconf);
>> dprintk("%s returns %d %d\n", __FUNCTION__, status, ntohl(status));
>> return status;
>> }
>> diff --git a/include/linux/nfsd/state.h b/include/linux/nfsd/state.h
>> index 92feae0..c23ae98 100644
>> --- a/include/linux/nfsd/state.h
>> +++ b/include/linux/nfsd/state.h
>> @@ -193,6 +193,7 @@ struct current_session {
>> * when we expire the nfs4_client
>> */
>> struct nfs4_client {
>> + struct kref cl_ref;
>> struct list_head cl_idhash; /* hash by cl_clientid.id */
>> struct list_head cl_strhash; /* hash by cl_name */
>> struct list_head cl_openowners;
>> @@ -211,7 +212,6 @@ struct nfs4_client {
>> clientid_t cl_clientid; /* generated by server */
>> nfs4_verifier cl_confirm; /* generated by server */
>> struct nfs4_callback cl_callback; /* callback info */
>> - atomic_t cl_count; /* ref count */
>> u32 cl_firststate; /* recovery dir creation */
>> /* NFSv4.1 create_session slot */
>> u32 cl_seqid; /* seqid for create_session */
>> @@ -428,7 +428,6 @@ extern void nfs4_lock_state(void);
>> extern void nfs4_unlock_state(void);
>> extern int nfs4_in_grace(void);
>> extern int nfs4_check_open_reclaim(clientid_t *clid);
>> -extern void put_nfs4_client(struct nfs4_client *clp);
>> extern void nfs4_free_stateowner(struct kref *kref);
>> extern void nfsd4_probe_callback(struct nfs4_client *clp);
>> extern void nfsd4_cb_recall(struct nfs4_delegation *dp);
>> --
>> 1.5.1
>>
>
>> _______________________________________________
>> pNFS mailing list
>> pNFS at linux-nfs.org
>> http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
--
Benny Halevy
Panasas Inc.
Accelerating Time to Results(TM) with Clustered Storage
www.panasas.com
bhalevy at panasas.com
Tel/Fax: +972-3-647-8340
Mobile: +972-54-802-8340
More information about the pNFS
mailing list