[PATCH v2 0/9] nfsd4: keep the client from expiring while in use by nfs41 compounds

[PATCH v2 0/9] nfsd4: keep the client from expiring while in use by nfs41 compounds

[Benny Halevy]

Changes since first version:

- move clients to be expired by laundromat to temporary reaplist
  under the client lock.  Then release the lock and expire them
  without holding the client lock.

- mark clients as expired under the client lock, do not renew
  clients mark as expired

- fix an existing bug in nfsd4_destroy_session that must take the state lock
  for clearing the callback client.

[PATCH v2 1/9] nfsd4: rename sessionid_lock to client_lock
[PATCH v2 2/9] nfsd4: fold release_session into expire_client
[PATCH v2 3/9] nfsd4: use list_move in move_to_confirmed
[PATCH v2 4/9] nfsd4: extend the client_lock to cover cl_lru
[PATCH v2 5/9] nfsd4: refactor expire_client
[PATCH v2 6/9] nfsd4: introduce nfs4_client.cl_refcount
[PATCH v2 7/9] nfsd4: mark_client_expired
[PATCH v2 8/9] nfsd4: keep a reference count on client while in use
[PATCH v2 9/9] nfsd4: nfsd4_destroy_session must set callback client under the state lock

[PATCH v2 1/9] nfsd4: rename sessionid_lock to client_lock

[Benny Halevy]

In preparation to share the lock's scope to both client
and session hash tables.

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 fs/nfsd/nfs4state.c |   26 ++++++++++++++------------
 1 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 835d6ce..2313dbf 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -250,6 +250,9 @@ unhash_delegation(struct nfs4_delegation *dp)
  * SETCLIENTID state 
  */
 
+/* client_lock protects the session hash table */
+static DEFINE_SPINLOCK(client_lock);
+
 /* Hash tables for nfs4_clientid state */
 #define CLIENT_HASH_BITS                 4
 #define CLIENT_HASH_SIZE                (1 << CLIENT_HASH_BITS)
@@ -368,7 +371,6 @@ static void release_openowner(struct nfs4_stateowner *sop)
 	nfs4_put_stateowner(sop);
 }
 
-static DEFINE_SPINLOCK(sessionid_lock);
 #define SESSION_HASH_SIZE	512
 static struct list_head sessionid_hashtbl[SESSION_HASH_SIZE];
 
@@ -566,10 +568,10 @@ alloc_init_session(struct svc_rqst *rqstp, struct nfs4_client *clp,
 
 	new->se_flags = cses->flags;
 	kref_init(&new->se_ref);
-	spin_lock(&sessionid_lock);
+	spin_lock(&client_lock);
 	list_add(&new->se_hash, &sessionid_hashtbl[idx]);
 	list_add(&new->se_perclnt, &clp->cl_sessions);
-	spin_unlock(&sessionid_lock);
+	spin_unlock(&client_lock);
 
 	status = nfs_ok;
 out:
@@ -580,7 +582,7 @@ out_free:
 	goto out;
 }
 
-/* caller must hold sessionid_lock */
+/* caller must hold client_lock */
 static struct nfsd4_session *
 find_in_sessionid_hashtbl(struct nfs4_sessionid *sessionid)
 {
@@ -603,7 +605,7 @@ find_in_sessionid_hashtbl(struct nfs4_sessionid *sessionid)
 	return NULL;
 }
 
-/* caller must hold sessionid_lock */
+/* caller must hold client_lock */
 static void
 unhash_session(struct nfsd4_session *ses)
 {
@@ -614,9 +616,9 @@ unhash_session(struct nfsd4_session *ses)
 static void
 release_session(struct nfsd4_session *ses)
 {
-	spin_lock(&sessionid_lock);
+	spin_lock(&client_lock);
 	unhash_session(ses);
-	spin_unlock(&sessionid_lock);
+	spin_unlock(&client_lock);
 	nfsd4_put_session(ses);
 }
 
@@ -1379,15 +1381,15 @@ nfsd4_destroy_session(struct svc_rqst *r,
 			return nfserr_not_only_op;
 	}
 	dump_sessionid(__func__, &sessionid->sessionid);
-	spin_lock(&sessionid_lock);
+	spin_lock(&client_lock);
 	ses = find_in_sessionid_hashtbl(&sessionid->sessionid);
 	if (!ses) {
-		spin_unlock(&sessionid_lock);
+		spin_unlock(&client_lock);
 		goto out;
 	}
 
 	unhash_session(ses);
-	spin_unlock(&sessionid_lock);
+	spin_unlock(&client_lock);
 
 	/* wait for callbacks */
 	nfsd4_set_callback_client(ses->se_client, NULL);
@@ -1411,7 +1413,7 @@ nfsd4_sequence(struct svc_rqst *rqstp,
 	if (resp->opcnt != 1)
 		return nfserr_sequence_pos;
 
-	spin_lock(&sessionid_lock);
+	spin_lock(&client_lock);
 	status = nfserr_badsession;
 	session = find_in_sessionid_hashtbl(&seq->sessionid);
 	if (!session)
@@ -1454,7 +1456,7 @@ out:
 	/* Hold a session reference until done processing the compound. */
 	if (cstate->session)
 		nfsd4_get_session(cstate->session);
-	spin_unlock(&sessionid_lock);
+	spin_unlock(&client_lock);
 	/* Renew the clientid on success and on replay */
 	if (cstate->session) {
 		nfs4_lock_state();
-- 
1.6.5.1

[PATCH v2 2/9] nfsd4: fold release_session into expire_client

[Benny Halevy]

and grab the client lock once for all the client's sessions.

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 fs/nfsd/nfs4state.c |   14 ++++----------
 1 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 2313dbf..f8bf619 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -613,15 +613,6 @@ unhash_session(struct nfsd4_session *ses)
 	list_del(&ses->se_perclnt);
 }
 
-static void
-release_session(struct nfsd4_session *ses)
-{
-	spin_lock(&client_lock);
-	unhash_session(ses);
-	spin_unlock(&client_lock);
-	nfsd4_put_session(ses);
-}
-
 void
 free_session(struct kref *kref)
 {
@@ -722,12 +713,15 @@ expire_client(struct nfs4_client *clp)
 		sop = list_entry(clp->cl_openowners.next, struct nfs4_stateowner, so_perclient);
 		release_openowner(sop);
 	}
+	spin_lock(&client_lock);
 	while (!list_empty(&clp->cl_sessions)) {
 		struct nfsd4_session  *ses;
 		ses = list_entry(clp->cl_sessions.next, struct nfsd4_session,
 				 se_perclnt);
-		release_session(ses);
+		unhash_session(ses);
+		nfsd4_put_session(ses);
 	}
+	spin_unlock(&client_lock);
 	nfsd4_set_callback_client(clp, NULL);
 	if (clp->cl_cb_conn.cb_xprt)
 		svc_xprt_put(clp->cl_cb_conn.cb_xprt);
-- 
1.6.5.1

[PATCH v2 3/9] nfsd4: use list_move in move_to_confirmed

[Benny Halevy]

rather than list_del_init, list_add

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 fs/nfsd/nfs4state.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index f8bf619..aecafb2 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -859,10 +859,9 @@ move_to_confirmed(struct nfs4_client *clp)
 	unsigned int strhashval;
 
 	dprintk("NFSD: move_to_confirm nfs4_client %p\n", clp);
-	list_del_init(&clp->cl_strhash);
 	list_move(&clp->cl_idhash, &conf_id_hashtbl[idhashval]);
 	strhashval = clientstr_hashval(clp->cl_recdir);
-	list_add(&clp->cl_strhash, &conf_str_hashtbl[strhashval]);
+	list_move(&clp->cl_strhash, &conf_str_hashtbl[strhashval]);
 	renew_client(clp);
 }
 
-- 
1.6.5.1

[PATCH v2 4/9] nfsd4: extend the client_lock to cover cl_lru

[Benny Halevy]

To be used later on to hold a reference count on the client while in use by a
nfsv4.1 compound.

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 fs/nfsd/nfs4state.c |   41 ++++++++++++++++++++++++++---------------
 1 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index aecafb2..3f572cb 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -250,7 +250,7 @@ unhash_delegation(struct nfs4_delegation *dp)
  * SETCLIENTID state 
  */
 
-/* client_lock protects the session hash table */
+/* client_lock protects the client lru list and session hash table */
 static DEFINE_SPINLOCK(client_lock);
 
 /* Hash tables for nfs4_clientid state */
@@ -628,8 +628,9 @@ free_session(struct kref *kref)
 	kfree(ses);
 }
 
+/* must be called under the client_lock */
 static inline void
-renew_client(struct nfs4_client *clp)
+renew_client_locked(struct nfs4_client *clp)
 {
 	/*
 	* Move client to the end to the LRU list.
@@ -641,6 +642,14 @@ renew_client(struct nfs4_client *clp)
 	clp->cl_time = get_seconds();
 }
 
+static inline void
+renew_client(struct nfs4_client *clp)
+{
+	spin_lock(&client_lock);
+	renew_client_locked(clp);
+	spin_unlock(&client_lock);
+}
+
 /* SETCLIENTID and SETCLIENTID_CONFIRM Helper functions */
 static int
 STALE_CLIENTID(clientid_t *clid)
@@ -706,14 +715,14 @@ 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);
 	while (!list_empty(&clp->cl_openowners)) {
 		sop = list_entry(clp->cl_openowners.next, struct nfs4_stateowner, so_perclient);
 		release_openowner(sop);
 	}
+	list_del(&clp->cl_idhash);
+	list_del(&clp->cl_strhash);
 	spin_lock(&client_lock);
+	list_del(&clp->cl_lru);
 	while (!list_empty(&clp->cl_sessions)) {
 		struct nfsd4_session  *ses;
 		ses = list_entry(clp->cl_sessions.next, struct nfsd4_session,
@@ -848,8 +857,7 @@ add_to_unconfirmed(struct nfs4_client *clp, unsigned int strhashval)
 	list_add(&clp->cl_strhash, &unconf_str_hashtbl[strhashval]);
 	idhashval = clientid_hashval(clp->cl_clientid.cl_id);
 	list_add(&clp->cl_idhash, &unconf_id_hashtbl[idhashval]);
-	list_add_tail(&clp->cl_lru, &client_lru);
-	clp->cl_time = get_seconds();
+	renew_client(clp);
 }
 
 static void
@@ -1447,15 +1455,12 @@ nfsd4_sequence(struct svc_rqst *rqstp,
 
 out:
 	/* Hold a session reference until done processing the compound. */
-	if (cstate->session)
-		nfsd4_get_session(cstate->session);
-	spin_unlock(&client_lock);
-	/* Renew the clientid on success and on replay */
 	if (cstate->session) {
-		nfs4_lock_state();
-		renew_client(session->se_client);
-		nfs4_unlock_state();
+		nfsd4_get_session(cstate->session);
+		/* Renew the clientid on success and on replay */
+		renew_client_locked(session->se_client);
 	}
+	spin_unlock(&client_lock);
 	dprintk("%s: return %d\n", __func__, ntohl(status));
 	return status;
 }
@@ -2564,6 +2569,8 @@ nfs4_laundromat(void)
 	dprintk("NFSD: laundromat service - starting\n");
 	if (locks_in_grace())
 		nfsd4_end_grace();
+	INIT_LIST_HEAD(&reaplist);
+	spin_lock(&client_lock);
 	list_for_each_safe(pos, next, &client_lru) {
 		clp = list_entry(pos, struct nfs4_client, cl_lru);
 		if (time_after((unsigned long)clp->cl_time, (unsigned long)cutoff)) {
@@ -2572,12 +2579,16 @@ nfs4_laundromat(void)
 				clientid_val = t;
 			break;
 		}
+		list_move(&clp->cl_lru, &reaplist);
+	}
+	spin_unlock(&client_lock);
+	list_for_each_safe(pos, next, &reaplist) {
+		clp = list_entry(pos, struct nfs4_client, cl_lru);
 		dprintk("NFSD: purging unused client (clientid %08x)\n",
 			clp->cl_clientid.cl_id);
 		nfsd4_remove_clid_dir(clp);
 		expire_client(clp);
 	}
-	INIT_LIST_HEAD(&reaplist);
 	spin_lock(&recall_lock);
 	list_for_each_safe(pos, next, &del_recall_lru) {
 		dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
-- 
1.6.5.1

[PATCH v2 5/9] nfsd4: refactor expire_client

[Benny Halevy]

Separate out unhashing of the client and session.
To be used later by the laundromat.

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 fs/nfsd/nfs4state.c |   29 ++++++++++++++++++-----------
 1 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 3f572cb..dede43c 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -693,6 +693,20 @@ free_client(struct nfs4_client *clp)
 	kfree(clp);
 }
 
+/* must be called under the client_lock */
+static inline void
+unhash_client_locked(struct nfs4_client *clp)
+{
+	list_del(&clp->cl_lru);
+	while (!list_empty(&clp->cl_sessions)) {
+		struct nfsd4_session  *ses;
+		ses = list_entry(clp->cl_sessions.next, struct nfsd4_session,
+				 se_perclnt);
+		unhash_session(ses);
+		nfsd4_put_session(ses);
+	}
+}
+
 static void
 expire_client(struct nfs4_client *clp)
 {
@@ -719,21 +733,14 @@ expire_client(struct nfs4_client *clp)
 		sop = list_entry(clp->cl_openowners.next, struct nfs4_stateowner, so_perclient);
 		release_openowner(sop);
 	}
+	nfsd4_set_callback_client(clp, NULL);
+	if (clp->cl_cb_conn.cb_xprt)
+		svc_xprt_put(clp->cl_cb_conn.cb_xprt);
 	list_del(&clp->cl_idhash);
 	list_del(&clp->cl_strhash);
 	spin_lock(&client_lock);
-	list_del(&clp->cl_lru);
-	while (!list_empty(&clp->cl_sessions)) {
-		struct nfsd4_session  *ses;
-		ses = list_entry(clp->cl_sessions.next, struct nfsd4_session,
-				 se_perclnt);
-		unhash_session(ses);
-		nfsd4_put_session(ses);
-	}
+	unhash_client_locked(clp);
 	spin_unlock(&client_lock);
-	nfsd4_set_callback_client(clp, NULL);
-	if (clp->cl_cb_conn.cb_xprt)
-		svc_xprt_put(clp->cl_cb_conn.cb_xprt);
 	free_client(clp);
 }
 
-- 
1.6.5.1

[PATCH v2 6/9] nfsd4: introduce nfs4_client.cl_refcount

[Benny Halevy]

Currently just initialize the cl_refcount to 1
and decrement in expire_client(), conditionally freeing the
client when the refcount reaches 0.

To be used later by nfsv4.1 compounds to keep the client from
timing out while in use.

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 fs/nfsd/nfs4state.c |    5 ++++-
 fs/nfsd/state.h     |    1 +
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index dede43c..295e8d9 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -740,8 +740,10 @@ expire_client(struct nfs4_client *clp)
 	list_del(&clp->cl_strhash);
 	spin_lock(&client_lock);
 	unhash_client_locked(clp);
+	BUG_ON(clp->cl_refcount <= 0);
+	if (--clp->cl_refcount <= 0)
+		free_client(clp);
 	spin_unlock(&client_lock);
-	free_client(clp);
 }
 
 static void copy_verf(struct nfs4_client *target, nfs4_verifier *source)
@@ -827,6 +829,7 @@ static struct nfs4_client *create_client(struct xdr_netobj name, char *recdir,
 	}
 
 	memcpy(clp->cl_recdir, recdir, HEXDIR_LEN);
+	clp->cl_refcount = 1;
 	atomic_set(&clp->cl_cb_set, 0);
 	INIT_LIST_HEAD(&clp->cl_idhash);
 	INIT_LIST_HEAD(&clp->cl_strhash);
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 98836fd..09c827d 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -233,6 +233,7 @@ struct nfs4_client {
 	struct nfsd4_clid_slot	cl_cs_slot;	/* create_session slot */
 	u32			cl_exchange_flags;
 	struct nfs4_sessionid	cl_sessionid;
+	int			cl_refcount;	/* use under client_lock */
 
 	/* for nfs41 callbacks */
 	/* We currently support a single back channel with a single slot */
-- 
1.6.5.1

[PATCH v2 7/9] nfsd4: mark_client_expired

[Benny Halevy]

Mark the client as expired under the client_lock so it won't be renewed
when an nfsv4.1 session is done, after it was explicitly expired
during processing of the compound.

Do not renew a client mark as expired (in particular, it is not
on the lru list anymore)

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 fs/nfsd/nfs4state.c |   10 ++++++++++
 fs/nfsd/state.h     |   14 +++++++++++++-
 2 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 295e8d9..378ee86 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -632,6 +632,14 @@ free_session(struct kref *kref)
 static inline void
 renew_client_locked(struct nfs4_client *clp)
 {
+	if (is_client_expired(clp)) {
+		dprintk("%s: client (clientid %08x/%08x) already expired\n",
+			__func__,
+			clp->cl_clientid.cl_boot,
+			clp->cl_clientid.cl_id);
+		return;
+	}
+
 	/*
 	* Move client to the end to the LRU list.
 	*/
@@ -697,6 +705,7 @@ free_client(struct nfs4_client *clp)
 static inline void
 unhash_client_locked(struct nfs4_client *clp)
 {
+	mark_client_expired(clp);
 	list_del(&clp->cl_lru);
 	while (!list_empty(&clp->cl_sessions)) {
 		struct nfsd4_session  *ses;
@@ -837,6 +846,7 @@ static struct nfs4_client *create_client(struct xdr_netobj name, char *recdir,
 	INIT_LIST_HEAD(&clp->cl_delegations);
 	INIT_LIST_HEAD(&clp->cl_sessions);
 	INIT_LIST_HEAD(&clp->cl_lru);
+	clp->cl_time = get_seconds();
 	clear_bit(0, &clp->cl_cb_slot_busy);
 	rpc_init_wait_queue(&clp->cl_cb_waitq, "Backchannel slot table");
 	copy_verf(clp, verf);
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 09c827d..37e1dff 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -166,7 +166,7 @@ struct nfsd4_session {
 	struct list_head	se_hash;	/* hash by sessionid */
 	struct list_head	se_perclnt;
 	u32			se_flags;
-	struct nfs4_client	*se_client;	/* for expire_client */
+	struct nfs4_client	*se_client;
 	struct nfs4_sessionid	se_sessionid;
 	struct nfsd4_channel_attrs se_fchannel;
 	struct nfsd4_channel_attrs se_bchannel;
@@ -243,6 +243,18 @@ struct nfs4_client {
 						/* wait here for slots */
 };
 
+static inline void
+mark_client_expired(struct nfs4_client *clp)
+{
+	clp->cl_time = 0;
+}
+
+static inline bool
+is_client_expired(struct nfs4_client *clp)
+{
+	return clp->cl_time == 0;
+}
+
 /* 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.5.1

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

[Benny Halevy]

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

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

[J. Bruce Fields]

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?

--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
> 

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

[Benny Halevy]

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

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

[Benny Halevy]

On May. 12, 2010, 7:26 +0300, Benny Halevy <bhalevy@panasas.com> wrote:
> 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)

Sorry, I misspoken.  exchange_id may decrement cl_refcount via expire_client()
but still the laundromat won't see it as expire_client will have already removed the
client from client_lru.

Benny


> 
> 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
> 

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

[J. Bruce Fields]

On Wed, May 12, 2010 at 09:19:34AM +0300, Benny Halevy wrote:
> On May. 12, 2010, 7:26 +0300, Benny Halevy <bhalevy@panasas.com> wrote:
> > 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)
> 
> Sorry, I misspoken.  exchange_id may decrement cl_refcount via expire_client()
> but still the laundromat won't see it as expire_client will have already removed the
> client from client_lru.

So the only potential problems are with operations that reach the client
through the pointer in the session, so such operations may need to do
something special.  OK.

--b.

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

[J. Bruce Fields]

On Wed, May 12, 2010 at 09:19:34AM +0300, Benny Halevy wrote:
> On May. 12, 2010, 7:26 +0300, Benny Halevy <bhalevy@panasas.com> wrote:
> > 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)
> 
> Sorry, I misspoken.  exchange_id may decrement cl_refcount via expire_client()
> but still the laundromat won't see it as expire_client will have already removed the
> client from client_lru.

Yep, sorry for my confusion!

The special treatment of refcount == 1 still seems odd to me; with
expiry==0 trick it no longer seems necessary.

Another case:

	- Two 4.1 compounds arrive, both their sequence operations are
	  processed.
	- Independently, an exchange_id expires the client.
	- At this point, the reference count is 2.
	- One of the original compounds completes.  It renews the client
	  (because it hits reference count 1).
	- The second of the two original compounds completes.  It frees
	  the client.

I guess there's nothing fundamentally wrong with that, but it's a little
odd.

Wouldn't it be more straightforward to let cl_refcount be a count of the
number of outstanding compounds, and make release_session_client do:

	if cl_refcount >= 0
		return;
	if (client_is_expired(clp))
		free client;
	else
		renew client;

?

The following works for me. If you don't see any objection, I'll squash
this in and push out the result.

--b.

commit 9d1b08f5cd9f2367f76f8350595c2bc00876d57f
Author: J. Bruce Fields <bfields@citi.umich.edu>
Date:   Wed May 12 18:17:58 2010 -0400

    tmp-squashme: count only session users in cl_refcount

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 89b77fe..05ad0ae 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -706,12 +706,12 @@ 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) {
+	if (!atomic_dec_and_lock(&clp->cl_refcount, &client_lock))
+		return;
+	if (is_client_expired(clp)) {
 		free_client(clp);
 		session->se_client = NULL;
-	} else if (clp->cl_refcount == 1)
+	} else
 		renew_client_locked(clp);
 	spin_unlock(&client_lock);
 	nfsd4_put_session(session);
@@ -765,8 +765,7 @@ expire_client(struct nfs4_client *clp)
 	list_del(&clp->cl_strhash);
 	spin_lock(&client_lock);
 	unhash_client_locked(clp);
-	BUG_ON(clp->cl_refcount <= 0);
-	if (--clp->cl_refcount <= 0)
+	if (atomic_dec_and_test(&clp->cl_refcount))
 		free_client(clp);
 	spin_unlock(&client_lock);
 }
@@ -854,7 +853,7 @@ static struct nfs4_client *create_client(struct xdr_netobj name, char *recdir,
 	}
 
 	memcpy(clp->cl_recdir, recdir, HEXDIR_LEN);
-	clp->cl_refcount = 1;
+	atomic_set(&clp->cl_refcount, 1);
 	atomic_set(&clp->cl_cb_set, 0);
 	INIT_LIST_HEAD(&clp->cl_idhash);
 	INIT_LIST_HEAD(&clp->cl_strhash);
@@ -1495,7 +1494,7 @@ out:
 	/* Hold a session reference until done processing the compound. */
 	if (cstate->session) {
 		nfsd4_get_session(cstate->session);
-		session->se_client->cl_refcount++;
+		atomic_inc(&session->se_client->cl_refcount);
 	}
 	spin_unlock(&client_lock);
 	dprintk("%s: return %d\n", __func__, ntohl(status));
@@ -2643,7 +2642,7 @@ nfs4_laundromat(void)
 				clientid_val = t;
 			break;
 		}
-		if (clp->cl_refcount > 1) {
+		if (atomic_read(&clp->cl_refcount)) {
 			dprintk("NFSD: client in use (clientid %08x)\n",
 				clp->cl_clientid.cl_id);
 			continue;
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index f263174..006c842 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -233,7 +233,8 @@ struct nfs4_client {
 	struct nfsd4_clid_slot	cl_cs_slot;	/* create_session slot */
 	u32			cl_exchange_flags;
 	struct nfs4_sessionid	cl_sessionid;
-	int			cl_refcount;	/* use under client_lock */
+	/* number of rpc's in progress over an associated session: */
+	atomic_t		cl_refcount;
 
 	/* for nfs41 callbacks */
 	/* We currently support a single back channel with a single slot */

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

[J. Bruce Fields]

On Wed, May 12, 2010 at 06:29:46PM -0400, J. Bruce Fields wrote:
> The following works for me. If you don't see any objection, I'll squash
> this in and push out the result.

And then this will also go on top.

(With the nfsd4_reclaim_complete() code being the one part mildly
relevant to your patch series.)

--b.

commit 3746094a09914b8e380bf6273f7ceb478de79157
Author: J. Bruce Fields <bfields@citi.umich.edu>
Date:   Mon Apr 19 15:11:28 2010 -0400

    nfsd4: implement reclaim_complete
    
    This is a mandatory operation.  Also, here (not in open) is where we
    should be committing the reboot recovery information.
    
    Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>

diff --git a/Documentation/filesystems/nfs/nfs41-server.txt b/Documentation/filesystems/nfs/nfs41-server.txt
index 6a53a84..0488491 100644
--- a/Documentation/filesystems/nfs/nfs41-server.txt
+++ b/Documentation/filesystems/nfs/nfs41-server.txt
@@ -137,7 +137,7 @@ NS*| OPENATTR             | OPT        |              | Section 18.17  |
    | READ                 | REQ        |              | Section 18.22  |
    | READDIR              | REQ        |              | Section 18.23  |
    | READLINK             | OPT        |              | Section 18.24  |
-NS | RECLAIM_COMPLETE     | REQ        |              | Section 18.51  |
+   | RECLAIM_COMPLETE     | REQ        |              | Section 18.51  |
    | RELEASE_LOCKOWNER    | MNI        |              | N/A            |
    | REMOVE               | REQ        |              | Section 18.25  |
    | RENAME               | REQ        |              | Section 18.26  |
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index e2dc960..59ec449 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1312,6 +1312,11 @@ static struct nfsd4_operation nfsd4_ops[] = {
 		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP,
 		.op_name = "OP_SEQUENCE",
 	},
+	[OP_RECLAIM_COMPLETE] = {
+		.op_func = (nfsd4op_func)nfsd4_reclaim_complete,
+		.op_flags = ALLOWED_WITHOUT_FH,
+		.op_name = "OP_RECLAIM_COMPLETE",
+	},
 };
 
 static const char *nfsd4_op_name(unsigned opnum)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index e563597..89b77fe 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1503,6 +1503,35 @@ out:
 }
 
 __be32
+nfsd4_reclaim_complete(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, struct nfsd4_reclaim_complete *rc)
+{
+	if (rc->rca_one_fs) {
+		if (!cstate->current_fh.fh_dentry)
+			return nfserr_nofilehandle;
+		/*
+		 * We don't take advantage of the rca_one_fs case.
+		 * That's OK, it's optional, we can safely ignore it.
+		 */
+		 return nfs_ok;
+	}
+	nfs4_lock_state();
+	if (is_client_expired(cstate->session->se_client)) {
+		nfs4_unlock_state();
+		/*
+		 * The following error isn't really legal.
+		 * But we only get here if the client just explicitly
+		 * destroyed the client.  Surely it no longer cares what
+		 * error it gets back on an operation for the dead
+		 * client.
+		 */
+		return nfserr_stale_clientid;
+	}
+	nfsd4_create_clid_dir(cstate->session->se_client);
+	nfs4_unlock_state();
+	return nfs_ok;
+}
+
+__be32
 nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		  struct nfsd4_setclientid *setclid)
 {
@@ -2511,10 +2540,8 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 	}
 	memcpy(&open->op_stateid, &stp->st_stateid, sizeof(stateid_t));
 
-	if (nfsd4_has_session(&resp->cstate)) {
+	if (nfsd4_has_session(&resp->cstate))
 		open->op_stateowner->so_confirmed = 1;
-		nfsd4_create_clid_dir(open->op_stateowner->so_client);
-	}
 
 	/*
 	* Attempt to hand out a delegation. No error return, because the
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 126d0ca..ac17a70 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1234,6 +1234,16 @@ nfsd4_decode_sequence(struct nfsd4_compoundargs *argp,
 	DECODE_TAIL;
 }
 
+static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, struct nfsd4_reclaim_complete *rc)
+{
+	DECODE_HEAD;
+
+	READ_BUF(4);
+	READ32(rc->rca_one_fs);
+
+	DECODE_TAIL;
+}
+
 static __be32
 nfsd4_decode_noop(struct nfsd4_compoundargs *argp, void *p)
 {
@@ -1346,7 +1356,7 @@ static nfsd4_dec nfsd41_dec_ops[] = {
 	[OP_TEST_STATEID]	= (nfsd4_dec)nfsd4_decode_notsupp,
 	[OP_WANT_DELEGATION]	= (nfsd4_dec)nfsd4_decode_notsupp,
 	[OP_DESTROY_CLIENTID]	= (nfsd4_dec)nfsd4_decode_notsupp,
-	[OP_RECLAIM_COMPLETE]	= (nfsd4_dec)nfsd4_decode_notsupp,
+	[OP_RECLAIM_COMPLETE]	= (nfsd4_dec)nfsd4_decode_reclaim_complete,
 };
 
 struct nfsd4_minorversion_ops {
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index c28958e..4d476ff 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -381,6 +381,10 @@ struct nfsd4_destroy_session {
 	struct nfs4_sessionid	sessionid;
 };
 
+struct nfsd4_reclaim_complete {
+	u32 rca_one_fs;
+};
+
 struct nfsd4_op {
 	int					opnum;
 	__be32					status;
@@ -421,6 +425,7 @@ struct nfsd4_op {
 		struct nfsd4_create_session	create_session;
 		struct nfsd4_destroy_session	destroy_session;
 		struct nfsd4_sequence		sequence;
+		struct nfsd4_reclaim_complete	reclaim_complete;
 	} u;
 	struct nfs4_replay *			replay;
 };
@@ -523,6 +528,7 @@ extern __be32 nfsd4_sequence(struct svc_rqst *,
 extern __be32 nfsd4_destroy_session(struct svc_rqst *,
 		struct nfsd4_compound_state *,
 		struct nfsd4_destroy_session *);
+__be32 nfsd4_reclaim_complete(struct svc_rqst *, struct nfsd4_compound_state *, struct nfsd4_reclaim_complete *);
 extern __be32 nfsd4_process_open1(struct nfsd4_compound_state *,
 		struct nfsd4_open *open);
 extern __be32 nfsd4_process_open2(struct svc_rqst *rqstp,

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

[Benny Halevy]

On May. 13, 2010, 1:29 +0300, "J. Bruce Fields" <bfields@citi.umich.edu> wrote:
> On Wed, May 12, 2010 at 09:19:34AM +0300, Benny Halevy wrote:
>> On May. 12, 2010, 7:26 +0300, Benny Halevy <bhalevy@panasas.com> wrote:
>>> 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)
>>
>> Sorry, I misspoken.  exchange_id may decrement cl_refcount via expire_client()
>> but still the laundromat won't see it as expire_client will have already removed the
>> client from client_lru.
> 
> Yep, sorry for my confusion!
> 
> The special treatment of refcount == 1 still seems odd to me; with
> expiry==0 trick it no longer seems necessary.
> 
> Another case:
> 
> 	- Two 4.1 compounds arrive, both their sequence operations are
> 	  processed.
> 	- Independently, an exchange_id expires the client.
> 	- At this point, the reference count is 2.
> 	- One of the original compounds completes.  It renews the client
> 	  (because it hits reference count 1).

and that will be a no-op as the client is marked as expired.

> 	- The second of the two original compounds completes.  It frees
> 	  the client.

right

> 
> I guess there's nothing fundamentally wrong with that, but it's a little
> odd.
> 
> Wouldn't it be more straightforward to let cl_refcount be a count of the
> number of outstanding compounds, and make release_session_client do:
> 
> 	if cl_refcount >= 0
> 		return;
> 	if (client_is_expired(clp))
> 		free client;
> 	else
> 		renew client;
> 
> ?
> 
> The following works for me. If you don't see any objection, I'll squash
> this in and push out the result.

No objection, looks good. Thanks!

Benny

> 
> --b.
> 
> commit 9d1b08f5cd9f2367f76f8350595c2bc00876d57f
> Author: J. Bruce Fields <bfields@citi.umich.edu>
> Date:   Wed May 12 18:17:58 2010 -0400
> 
>     tmp-squashme: count only session users in cl_refcount
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 89b77fe..05ad0ae 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -706,12 +706,12 @@ 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) {
> +	if (!atomic_dec_and_lock(&clp->cl_refcount, &client_lock))
> +		return;
> +	if (is_client_expired(clp)) {
>  		free_client(clp);
>  		session->se_client = NULL;
> -	} else if (clp->cl_refcount == 1)
> +	} else
>  		renew_client_locked(clp);
>  	spin_unlock(&client_lock);
>  	nfsd4_put_session(session);
> @@ -765,8 +765,7 @@ expire_client(struct nfs4_client *clp)
>  	list_del(&clp->cl_strhash);
>  	spin_lock(&client_lock);
>  	unhash_client_locked(clp);
> -	BUG_ON(clp->cl_refcount <= 0);
> -	if (--clp->cl_refcount <= 0)
> +	if (atomic_dec_and_test(&clp->cl_refcount))
>  		free_client(clp);
>  	spin_unlock(&client_lock);
>  }
> @@ -854,7 +853,7 @@ static struct nfs4_client *create_client(struct xdr_netobj name, char *recdir,
>  	}
>  
>  	memcpy(clp->cl_recdir, recdir, HEXDIR_LEN);
> -	clp->cl_refcount = 1;
> +	atomic_set(&clp->cl_refcount, 1);
>  	atomic_set(&clp->cl_cb_set, 0);
>  	INIT_LIST_HEAD(&clp->cl_idhash);
>  	INIT_LIST_HEAD(&clp->cl_strhash);
> @@ -1495,7 +1494,7 @@ out:
>  	/* Hold a session reference until done processing the compound. */
>  	if (cstate->session) {
>  		nfsd4_get_session(cstate->session);
> -		session->se_client->cl_refcount++;
> +		atomic_inc(&session->se_client->cl_refcount);
>  	}
>  	spin_unlock(&client_lock);
>  	dprintk("%s: return %d\n", __func__, ntohl(status));
> @@ -2643,7 +2642,7 @@ nfs4_laundromat(void)
>  				clientid_val = t;
>  			break;
>  		}
> -		if (clp->cl_refcount > 1) {
> +		if (atomic_read(&clp->cl_refcount)) {
>  			dprintk("NFSD: client in use (clientid %08x)\n",
>  				clp->cl_clientid.cl_id);
>  			continue;
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index f263174..006c842 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -233,7 +233,8 @@ struct nfs4_client {
>  	struct nfsd4_clid_slot	cl_cs_slot;	/* create_session slot */
>  	u32			cl_exchange_flags;
>  	struct nfs4_sessionid	cl_sessionid;
> -	int			cl_refcount;	/* use under client_lock */
> +	/* number of rpc's in progress over an associated session: */
> +	atomic_t		cl_refcount;
>  
>  	/* for nfs41 callbacks */
>  	/* We currently support a single back channel with a single slot */

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

[J. Bruce Fields]

On Thu, May 13, 2010 at 05:36:15PM +0300, Benny Halevy wrote:
> On May. 13, 2010, 1:29 +0300, "J. Bruce Fields" <bfields@citi.umich.edu> wrote:
> > Another case:
> > 
> > 	- Two 4.1 compounds arrive, both their sequence operations are
> > 	  processed.
> > 	- Independently, an exchange_id expires the client.
> > 	- At this point, the reference count is 2.
> > 	- One of the original compounds completes.  It renews the client
> > 	  (because it hits reference count 1).
> 
> and that will be a no-op as the client is marked as expired.

Pfft, apologies, and thanks for setting me straight again.  Still, the
existing scheme seems slightly more complicated than necessary.

> 
> > 	- The second of the two original compounds completes.  It frees
> > 	  the client.
> 
> right
> 
> > 
> > I guess there's nothing fundamentally wrong with that, but it's a little
> > odd.
> > 
> > Wouldn't it be more straightforward to let cl_refcount be a count of the
> > number of outstanding compounds, and make release_session_client do:
> > 
> > 	if cl_refcount >= 0
> > 		return;
> > 	if (client_is_expired(clp))
> > 		free client;
> > 	else
> > 		renew client;
> > 
> > ?
> > 
> > The following works for me. If you don't see any objection, I'll squash
> > this in and push out the result.
> 
> No objection, looks good. Thanks!

OK, thanks!

I notice I didn't do exactly what I said I would, though; with the
following change on top I think it's right.

I'll test that and then push out the result.

--b.

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 05ad0ae..84b0fe9 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -765,7 +765,7 @@ expire_client(struct nfs4_client *clp)
 	list_del(&clp->cl_strhash);
 	spin_lock(&client_lock);
 	unhash_client_locked(clp);
-	if (atomic_dec_and_test(&clp->cl_refcount))
+	if (atomic_read(&clp->cl_refcount) == 0)
 		free_client(clp);
 	spin_unlock(&client_lock);
 }
@@ -853,7 +853,7 @@ static struct nfs4_client *create_client(struct xdr_netobj name, char *recdir,
 	}
 
 	memcpy(clp->cl_recdir, recdir, HEXDIR_LEN);
-	atomic_set(&clp->cl_refcount, 1);
+	atomic_set(&clp->cl_refcount, 0);
 	atomic_set(&clp->cl_cb_set, 0);
 	INIT_LIST_HEAD(&clp->cl_idhash);
 	INIT_LIST_HEAD(&clp->cl_strhash);

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

[J. Bruce Fields]

On Wed, May 12, 2010 at 07:26:46AM +0300, Benny Halevy wrote:
> 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)

Right, that was the whole point.  Thanks!

--b.

> 
> 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
> 

[PATCH v2 9/9] nfsd4: nfsd4_destroy_session must set callback client under the state lock

[Benny Halevy]

nfsd4_set_callback_client must be called under the state lock to atomically
set or unset the callback client and shutting down the previous one.

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 fs/nfsd/nfs4callback.c |    1 +
 fs/nfsd/nfs4state.c    |    2 ++
 2 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 1d5051d..77bc9d3 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -718,6 +718,7 @@ void nfsd4_destroy_callback_queue(void)
 	destroy_workqueue(callback_wq);
 }
 
+/* must be called under the state lock */
 void nfsd4_set_callback_client(struct nfs4_client *clp, struct rpc_clnt *new)
 {
 	struct rpc_clnt *old = clp->cl_cb_client;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 1a8fb39..e563597 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1428,8 +1428,10 @@ nfsd4_destroy_session(struct svc_rqst *r,
 	unhash_session(ses);
 	spin_unlock(&client_lock);
 
+	nfs4_lock_state();
 	/* wait for callbacks */
 	nfsd4_set_callback_client(ses->se_client, NULL);
+	nfs4_unlock_state();
 	nfsd4_put_session(ses);
 	status = nfs_ok;
 out:
-- 
1.6.5.1