[PATCH] nfs4_client ref count for cb_recall

Benny Halevy bhalevy at ns1.bhalevy.com
Fri May 11 05:39:16 EDT 2007


This patch changes the client reference counting technique
to struct kref, moves shutting down of the callback rpc_client
from expire_client time to free_nfs4_client time that's called
when the nfs4_client refcount goes to zero, adds a refcount
to the nfs4_client around nfs4_cb_recall to keep the rcp_client
around throughout the recall (otherwise it could have been used
after free), and finally it adds some BUG_ONs to assert that the
state lock is taken when taking or releasing reference for nfs4_file,
nfs4_client, and nfs4_delegation to ensure atomicity.
---
 fs/nfsd/nfs4callback.c     |   11 +++++-
 fs/nfsd/nfs4state.c        |   80 ++++++++++++++++++++++++++++++++-----------
 include/linux/nfsd/state.h |    3 +-
 3 files changed, 70 insertions(+), 24 deletions(-)

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 32ffea0..ae60a7d 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -433,7 +433,7 @@ nfsd4_probe_callback(struct nfs4_client *clp)
 		goto out_clnt;
 
 	/* the task holds a reference to the nfs4_client struct */
-	atomic_inc(&clp->cl_count);
+	get_nfs4_client(clp);
 
 	msg.rpc_cred = nfsd4_lookupcred(clp,0);
 	if (IS_ERR(msg.rpc_cred))
@@ -448,7 +448,9 @@ nfsd4_probe_callback(struct nfs4_client *clp)
 	return;
 
 out_rpciod:
-	atomic_dec(&clp->cl_count);
+	nfs4_lock_state();
+	put_nfs4_client(clp);
+	nfs4_unlock_state();
 	rpciod_down();
 out_clnt:
 	rpc_shutdown_client(cb->cb_client);
@@ -475,7 +477,9 @@ nfs4_cb_null(struct rpc_task *task, void *dummy)
 	atomic_set(&cb->cb_set, 1);
 	dprintk("NFSD: callback set to client %u.%u.%u.%u\n", NIPQUAD(addr));
 out:
+	nfs4_lock_state();
 	put_nfs4_client(clp);
+	nfs4_unlock_state();
 }
 
 static const struct rpc_call_ops nfs4_cb_null_ops = {
@@ -533,6 +537,9 @@ out:
 	/* Success or failure, now we're either waiting for lease expiration
 	 * or deleg_return. */
 	dprintk("NFSD: nfs4_cb_recall: dp %p dl_flock %p dl_count %d\n",dp, dp->dl_flock, atomic_read(&dp->dl_count));
+	nfs4_lock_state();
+	put_nfs4_client(dp->dl_client);
 	nfs4_put_delegation(dp);
+	nfs4_unlock_state();
 	return;
 }
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 3cc8ce4..2a2eb97 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -90,6 +90,8 @@ static struct kmem_cache *file_slab = NULL;
 static struct kmem_cache *stateid_slab = NULL;
 static struct kmem_cache *deleg_slab = NULL;
 
+#define BUG_ON_UNLOCKED_STATE() BUG_ON(mutex_trylock(&client_mutex))
+
 void
 nfs4_lock_state(void)
 {
@@ -139,6 +141,7 @@ free_nfs4_file(struct kref *kref)
 static inline void
 put_nfs4_file(struct nfs4_file *fi)
 {
+	BUG_ON_UNLOCKED_STATE();
 	kref_put(&fi->fi_ref, free_nfs4_file);
 }
 
@@ -228,6 +231,7 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_stateid *stp, struct svc_f
 void
 nfs4_put_delegation(struct nfs4_delegation *dp)
 {
+	BUG_ON_UNLOCKED_STATE();
 	if (atomic_dec_and_test(&dp->dl_count)) {
 		dprintk("NFSD: freeing dp %p\n",dp);
 		put_nfs4_file(dp->dl_file);
@@ -353,20 +357,11 @@ alloc_client(struct xdr_netobj name)
 	return clp;
 }
 
-static inline void
-free_client(struct nfs4_client *clp)
-{
-	if (clp->cl_cred.cr_group_info)
-		put_group_info(clp->cl_cred.cr_group_info);
-	kfree(clp->cl_name.data);
-	kfree(clp);
-}
-
-void
-put_nfs4_client(struct nfs4_client *clp)
+struct nfs4_client *
+get_nfs4_client(struct nfs4_client *clp)
 {
-	if (atomic_dec_and_test(&clp->cl_count))
-		free_client(clp);
+	kref_get(&clp->cl_ref);
+	return clp;
 }
 
 static void
@@ -383,17 +378,43 @@ shutdown_callback_client(struct nfs4_client *clp)
 }
 
 static void
+free_nfs4_client(struct kref *kref)
+{
+	struct nfs4_client *clp = container_of(kref, struct nfs4_client, cl_ref);
+	BUG_ON(!list_empty(&clp->cl_idhash));
+	BUG_ON(!list_empty(&clp->cl_strhash));
+	BUG_ON(!list_empty(&clp->cl_lru));
+	BUG_ON(!list_empty(&clp->cl_delegations));
+	BUG_ON(!list_empty(&clp->cl_openowners));
+	shutdown_callback_client(clp);
+	if (clp->cl_cred.cr_group_info)
+		put_group_info(clp->cl_cred.cr_group_info);
+	kfree(clp->cl_name.data);
+	kfree(clp);
+}
+
+void
+put_nfs4_client(struct nfs4_client *clp)
+{
+	BUG_ON_UNLOCKED_STATE();
+	if (clp)
+		kref_put(&clp->cl_ref, free_nfs4_client);
+}
+
+static void
 expire_client(struct nfs4_client *clp)
 {
 	struct nfs4_stateowner *sop;
 	struct nfs4_delegation *dp;
 	struct list_head reaplist;
 
-	dprintk("NFSD: expire_client cl_count %d\n",
-	                    atomic_read(&clp->cl_count));
-
-	shutdown_callback_client(clp);
+	dprintk("NFSD: expire_client refcount %d\n",
+	                    atomic_read(&clp->cl_ref.refcount));
 
+	BUG_ON_UNLOCKED_STATE();
+	atomic_set(&clp->cl_callback.cb_set, 0);
+	if (clp->cl_callback.cb_client != NULL)
+		rpc_killall_tasks(clp->cl_callback.cb_client);
 	INIT_LIST_HEAD(&reaplist);
 	spin_lock(&recall_lock);
 	while (!list_empty(&clp->cl_delegations)) {
@@ -409,9 +430,9 @@ expire_client(struct nfs4_client *clp)
 		list_del_init(&dp->dl_recall_lru);
 		unhash_delegation(dp);
 	}
-	list_del(&clp->cl_idhash);
-	list_del(&clp->cl_strhash);
-	list_del(&clp->cl_lru);
+	list_del_init(&clp->cl_idhash);
+	list_del_init(&clp->cl_strhash);
+	list_del_init(&clp->cl_lru);
 	while (!list_empty(&clp->cl_openowners)) {
 		sop = list_entry(clp->cl_openowners.next, struct nfs4_stateowner, so_perclient);
 		release_stateowner(sop);
@@ -426,7 +447,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);
@@ -1213,6 +1234,7 @@ find_openstateowner_str(unsigned int hashval, struct nfsd4_open *open)
 {
 	struct nfs4_stateowner *so = NULL;
 
+	BUG_ON_UNLOCKED_STATE();
 	list_for_each_entry(so, &ownerstr_hashtbl[hashval], so_strhash) {
 		if (cmp_owner_str(so, &open->op_owner, &open->op_clientid))
 			return so;
@@ -1227,6 +1249,7 @@ find_file(struct inode *ino)
 	unsigned int hashval = file_hashval(ino);
 	struct nfs4_file *fp;
 
+	BUG_ON_UNLOCKED_STATE();
 	list_for_each_entry(fp, &file_hashtbl[hashval], fi_hash) {
 		if (fp->fi_inode == ino) {
 			get_nfs4_file(fp);
@@ -1342,12 +1365,24 @@ static
 void nfsd_break_deleg_cb(struct file_lock *fl)
 {
 	struct nfs4_delegation *dp=  (struct nfs4_delegation *)fl->fl_owner;
+	struct nfs4_client *clp;
+	struct rpc_clnt *clnt;
 	struct task_struct *t;
 
 	dprintk("NFSD nfsd_break_deleg_cb: dp %p fl %p\n",dp,fl);
 	if (!dp)
 		return;
 
+	nfs4_lock_state();
+	clp = dp->dl_client;
+	clnt = clp->cl_callback.cb_client;
+	if (!atomic_read(&clp->cl_callback.cb_set) || !clnt) {
+		nfs4_lock_state();
+		return;
+	}
+	get_nfs4_client(clp);
+	nfs4_lock_state();
+
 	/* We're assuming the state code never drops its reference
 	 * without first removing the lease.  Since we're in this lease
 	 * callback (and since the lease code is serialized by the kernel
@@ -1372,7 +1407,10 @@ void nfsd_break_deleg_cb(struct file_lock *fl)
 		printk(KERN_INFO "NFSD: Callback thread failed for "
 			"for client (clientid %08x/%08x)\n",
 			clp->cl_clientid.cl_boot, clp->cl_clientid.cl_id);
+		nfs4_lock_state();
+		put_nfs4_client(clp);
 		nfs4_put_delegation(dp);
+		nfs4_unlock_state();
 	}
 }
 
diff --git a/include/linux/nfsd/state.h b/include/linux/nfsd/state.h
index ab5c236..84463a3 100644
--- a/include/linux/nfsd/state.h
+++ b/include/linux/nfsd/state.h
@@ -116,6 +116,7 @@ struct nfs4_callback {
  *	  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;
@@ -130,7 +131,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 */
 };
 
@@ -279,6 +279,7 @@ extern void nfs4_lock_state(void);
 extern void nfs4_unlock_state(void);
 extern int nfs4_in_grace(void);
 extern __be32 nfs4_check_open_reclaim(clientid_t *clid);
+extern struct nfs4_client *get_nfs4_client(struct nfs4_client *clp);
 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);
-- 
1.5.1


--------------070300090107050408030000--


More information about the pNFS mailing list