All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC 0/3] nfsd41: do not expire client while in use by current compound
@ 2010-05-03 16:13 Benny Halevy
  2010-05-03 16:31 ` [RFC 1/3] nfsd4: use local variable in nfs4svc_encode_compoundres Benny Halevy
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Benny Halevy @ 2010-05-03 16:13 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: linux-nfs

Bruce,  the following patchset compiles but is not fully tested yet.
It is based on top of http://marc.info/?l=linux-nfs&m=127288934008791&w=2
Please review.

Thanks,

Benny

[RFC 1/3] nfsd4: use local variable in nfs4svc_encode_compoundres
[RFC 2/3] nfsd4: hold a reference on nfs4_client when it is used by a session
[RFC 3/3] nfsd4: do not expire nfs41 clients while in use

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [RFC 1/3] nfsd4: use local variable in nfs4svc_encode_compoundres
  2010-05-03 16:13 [RFC 0/3] nfsd41: do not expire client while in use by current compound Benny Halevy
@ 2010-05-03 16:31 ` Benny Halevy
  2010-05-03 22:37   `  J. Bruce Fields
  2010-05-03 16:31 ` [RFC 2/3] nfsd4: hold a reference on nfs4_client when it is used by a session Benny Halevy
  2010-05-03 16:31 ` [RFC 3/3] nfsd4: do not expire nfs41 clients while in use Benny Halevy
  2 siblings, 1 reply; 12+ messages in thread
From: Benny Halevy @ 2010-05-03 16:31 UTC (permalink / raw)
  To:  J. Bruce Fields; +Cc: linux-nfs, Benny Halevy

'cs' is already computed, re-use it.

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

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index b04583c..19ff5a3 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -3311,9 +3311,9 @@ nfs4svc_encode_compoundres(struct svc_rqst *rqstp, __be32 *p, struct nfsd4_compo
 		if (cs->status != nfserr_replay_cache) {
 			nfsd4_store_cache_entry(resp);
 			dprintk("%s: SET SLOT STATE TO AVAILABLE\n", __func__);
-			resp->cstate.slot->sl_inuse = false;
+			cs->slot->sl_inuse = false;
 		}
-		nfsd4_put_session(resp->cstate.session);
+		nfsd4_put_session(cs->session);
 	}
 	return 1;
 }
-- 
1.6.3.3


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [RFC 2/3] nfsd4: hold a reference on nfs4_client when it is used by a session
  2010-05-03 16:13 [RFC 0/3] nfsd41: do not expire client while in use by current compound Benny Halevy
  2010-05-03 16:31 ` [RFC 1/3] nfsd4: use local variable in nfs4svc_encode_compoundres Benny Halevy
@ 2010-05-03 16:31 ` Benny Halevy
  2010-05-03 22:36   `  J. Bruce Fields
  2010-05-03 16:31 ` [RFC 3/3] nfsd4: do not expire nfs41 clients while in use Benny Halevy
  2 siblings, 1 reply; 12+ messages in thread
From: Benny Halevy @ 2010-05-03 16:31 UTC (permalink / raw)
  To:  J. Bruce Fields; +Cc: linux-nfs, Benny Halevy

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.

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [RFC 3/3] nfsd4: do not expire nfs41 clients while in use
  2010-05-03 16:13 [RFC 0/3] nfsd41: do not expire client while in use by current compound Benny Halevy
  2010-05-03 16:31 ` [RFC 1/3] nfsd4: use local variable in nfs4svc_encode_compoundres Benny Halevy
  2010-05-03 16:31 ` [RFC 2/3] nfsd4: hold a reference on nfs4_client when it is used by a session Benny Halevy
@ 2010-05-03 16:31 ` Benny Halevy
  2010-05-04  5:39   ` Benny Halevy
  2 siblings, 1 reply; 12+ messages in thread
From: Benny Halevy @ 2010-05-03 16:31 UTC (permalink / raw)
  To:  J. Bruce Fields; +Cc: linux-nfs, Benny Halevy

Add a cl_use_count atomic counter to struct nfs4_client.
Hold a use count while the client is in use by a compound that begins
with SEQUENCE.
Renew the client when the comound completes.
The laundromat skips clients that have a positive use count.
Otherwise, the laundromat marks the client as expired by setting
cl_use_count to -1.

Note that we don't want to use the state lock to synchronize with nfsd4_sequence
as we want to diminish the state lock scope.
An alternative to using atomic_cmpxchg is to grab the sessionid_lock spin lock
while accessing cl_use_count, but that would cause some contention in the
laundromat if it needs to lock and unlock it for every client.

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 fs/nfsd/nfs4state.c |   40 ++++++++++++++++++++++++++++++++++------
 fs/nfsd/nfs4xdr.c   |    2 ++
 fs/nfsd/state.h     |    9 +++++++++
 3 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 50b75af..c95ddca 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -647,6 +647,14 @@ renew_client(struct nfs4_client *clp)
 	clp->cl_time = get_seconds();
 }
 
+void
+renew_nfs4_client(struct nfs4_client *clp)
+{
+	nfs4_lock_state();
+	renew_client(clp);
+	nfs4_unlock_state();
+}
+
 /* SETCLIENTID and SETCLIENTID_CONFIRM Helper functions */
 static int
 STALE_CLIENTID(clientid_t *clid)
@@ -715,6 +723,24 @@ put_nfs4_client(struct nfs4_client *clp)
 		free_client(clp);
 }
 
+/*
+ * atomically mark client as used, as long as it's not already expired.
+ * return 0 on success, or a negative value if client already expired.
+ */
+int
+use_nfs4_client(struct nfs4_client *clp)
+{
+	int orig;
+
+	do {
+		orig = atomic_read(&clp->cl_use_count);
+		if (orig < 0)
+			return orig;
+	} while (atomic_cmpxchg(&clp->cl_use_count, orig, orig + 1) != orig);
+
+	return 0;
+}
+
 static void
 expire_client(struct nfs4_client *clp)
 {
@@ -840,6 +866,7 @@ static struct nfs4_client *create_client(struct xdr_netobj name, char *recdir,
 
 	memcpy(clp->cl_recdir, recdir, HEXDIR_LEN);
 	atomic_set(&clp->cl_count, 1);
+	atomic_set(&clp->cl_use_count, 0);
 	atomic_set(&clp->cl_cb_conn.cb_set, 0);
 	INIT_LIST_HEAD(&clp->cl_idhash);
 	INIT_LIST_HEAD(&clp->cl_strhash);
@@ -1423,6 +1450,10 @@ nfsd4_sequence(struct svc_rqst *rqstp,
 	if (!session)
 		goto out;
 
+	/* keep the client from expiring */
+	if (use_nfs4_client(cstate->session->se_client) < 0)
+		goto out;
+
 	status = nfserr_badslot;
 	if (seq->slotid >= session->se_fchannel.maxreqs)
 		goto out;
@@ -1463,12 +1494,6 @@ out:
 		get_nfs4_client(cstate->session->se_client);
 	}
 	spin_unlock(&sessionid_lock);
-	/* Renew the clientid on success and on replay */
-	if (cstate->session) {
-		nfs4_lock_state();
-		renew_client(session->se_client);
-		nfs4_unlock_state();
-	}
 	dprintk("%s: return %d\n", __func__, ntohl(status));
 	return status;
 }
@@ -2575,6 +2600,9 @@ nfs4_laundromat(void)
 		nfsd4_end_grace();
 	list_for_each_safe(pos, next, &client_lru) {
 		clp = list_entry(pos, struct nfs4_client, cl_lru);
+		/* skip client if in use (cl_use_count is greater than zero) */
+		if (atomic_cmpxchg(&clp->cl_use_count, 0, -1) > 0)
+			continue;
 		if (time_after((unsigned long)clp->cl_time, (unsigned long)cutoff)) {
 			t = clp->cl_time - cutoff;
 			if (clientid_val > t)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index aed733c..0f0b857 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -3313,6 +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;
 		}
+		renew_nfs4_client(cs->session->se_client);
+		unuse_nfs4_client(cs->session->se_client);
 		put_nfs4_client(cs->session->se_client);
 		nfsd4_put_session(cs->session);
 	}
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index e3c002e..806d9b8 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -214,6 +214,7 @@ struct nfs4_client {
 	nfs4_verifier		cl_confirm;	/* generated by server */
 	struct nfs4_cb_conn	cl_cb_conn;     /* callback info */
 	atomic_t		cl_count;	/* ref count */
+	atomic_t		cl_use_count;	/* session use count */
 	u32			cl_firststate;	/* recovery dir creation */
 
 	/* for nfs41 */
@@ -237,6 +238,12 @@ get_nfs4_client(struct nfs4_client *clp)
 	atomic_inc(&clp->cl_count);
 }
 
+static inline void
+unuse_nfs4_client(struct nfs4_client *clp)
+{
+	atomic_dec(&clp->cl_use_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
@@ -384,6 +391,8 @@ extern void nfs4_unlock_state(void);
 extern int nfs4_in_grace(void);
 extern __be32 nfs4_check_open_reclaim(clientid_t *clid);
 extern void put_nfs4_client(struct nfs4_client *clp);
+extern int use_nfs4_client(struct nfs4_client *clp);
+extern void renew_nfs4_client(struct nfs4_client *clp);
 extern void nfs4_free_stateowner(struct kref *kref);
 extern int set_callback_cred(void);
 extern void nfsd4_probe_callback(struct nfs4_client *clp);
-- 
1.6.3.3


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [RFC 2/3] nfsd4: hold a reference on nfs4_client when it is used by a session
  2010-05-03 16:31 ` [RFC 2/3] nfsd4: hold a reference on nfs4_client when it is used by a session Benny Halevy
@ 2010-05-03 22:36   `  J. Bruce Fields
  2010-05-04  1:12     `  J. Bruce Fields
  0 siblings, 1 reply; 12+ messages in thread
From:  J. Bruce Fields @ 2010-05-03 22:36 UTC (permalink / raw)
  To: Benny Halevy; +Cc: linux-nfs

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC 1/3] nfsd4: use local variable in nfs4svc_encode_compoundres
  2010-05-03 16:31 ` [RFC 1/3] nfsd4: use local variable in nfs4svc_encode_compoundres Benny Halevy
@ 2010-05-03 22:37   `  J. Bruce Fields
  0 siblings, 0 replies; 12+ messages in thread
From:  J. Bruce Fields @ 2010-05-03 22:37 UTC (permalink / raw)
  To: Benny Halevy; +Cc: linux-nfs

On Mon, May 03, 2010 at 07:31:33PM +0300, Benny Halevy wrote:
> 'cs' is already computed, re-use it.

Thanks, applied.

--b.

> 
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> ---
>  fs/nfsd/nfs4xdr.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index b04583c..19ff5a3 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -3311,9 +3311,9 @@ nfs4svc_encode_compoundres(struct svc_rqst *rqstp, __be32 *p, struct nfsd4_compo
>  		if (cs->status != nfserr_replay_cache) {
>  			nfsd4_store_cache_entry(resp);
>  			dprintk("%s: SET SLOT STATE TO AVAILABLE\n", __func__);
> -			resp->cstate.slot->sl_inuse = false;
> +			cs->slot->sl_inuse = false;
>  		}
> -		nfsd4_put_session(resp->cstate.session);
> +		nfsd4_put_session(cs->session);
>  	}
>  	return 1;
>  }
> -- 
> 1.6.3.3
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC 2/3] nfsd4: hold a reference on nfs4_client when it is used by a session
  2010-05-03 22:36   `  J. Bruce Fields
@ 2010-05-04  1:12     `  J. Bruce Fields
  2010-05-04  7:11       ` Benny Halevy
  0 siblings, 1 reply; 12+ messages in thread
From:  J. Bruce Fields @ 2010-05-04  1:12 UTC (permalink / raw)
  To: Benny Halevy; +Cc: linux-nfs

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

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC 3/3] nfsd4: do not expire nfs41 clients while in use
  2010-05-03 16:31 ` [RFC 3/3] nfsd4: do not expire nfs41 clients while in use Benny Halevy
@ 2010-05-04  5:39   ` Benny Halevy
  0 siblings, 0 replies; 12+ messages in thread
From: Benny Halevy @ 2010-05-04  5:39 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: linux-nfs

Benny Halevy wrote:
> Add a cl_use_count atomic counter to struct nfs4_client.
> Hold a use count while the client is in use by a compound that begins
> with SEQUENCE.
> Renew the client when the comound completes.
> The laundromat skips clients that have a positive use count.
> Otherwise, the laundromat marks the client as expired by setting
> cl_use_count to -1.
> 
> Note that we don't want to use the state lock to synchronize with nfsd4_sequence
> as we want to diminish the state lock scope.
> An alternative to using atomic_cmpxchg is to grab the sessionid_lock spin lock
> while accessing cl_use_count, but that would cause some contention in the
> laundromat if it needs to lock and unlock it for every client.
> 
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> ---
>  fs/nfsd/nfs4state.c |   40 ++++++++++++++++++++++++++++++++++------
>  fs/nfsd/nfs4xdr.c   |    2 ++
>  fs/nfsd/state.h     |    9 +++++++++
>  3 files changed, 45 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 50b75af..c95ddca 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -647,6 +647,14 @@ renew_client(struct nfs4_client *clp)
>  	clp->cl_time = get_seconds();
>  }
>  
> +void
> +renew_nfs4_client(struct nfs4_client *clp)
> +{
> +	nfs4_lock_state();
> +	renew_client(clp);
> +	nfs4_unlock_state();
> +}
> +
>  /* SETCLIENTID and SETCLIENTID_CONFIRM Helper functions */
>  static int
>  STALE_CLIENTID(clientid_t *clid)
> @@ -715,6 +723,24 @@ put_nfs4_client(struct nfs4_client *clp)
>  		free_client(clp);
>  }
>  
> +/*
> + * atomically mark client as used, as long as it's not already expired.
> + * return 0 on success, or a negative value if client already expired.
> + */
> +int
> +use_nfs4_client(struct nfs4_client *clp)
> +{
> +	int orig;
> +
> +	do {
> +		orig = atomic_read(&clp->cl_use_count);
> +		if (orig < 0)
> +			return orig;
> +	} while (atomic_cmpxchg(&clp->cl_use_count, orig, orig + 1) != orig);
> +
> +	return 0;
> +}
> +
>  static void
>  expire_client(struct nfs4_client *clp)
>  {
> @@ -840,6 +866,7 @@ static struct nfs4_client *create_client(struct xdr_netobj name, char *recdir,
>  
>  	memcpy(clp->cl_recdir, recdir, HEXDIR_LEN);
>  	atomic_set(&clp->cl_count, 1);
> +	atomic_set(&clp->cl_use_count, 0);
>  	atomic_set(&clp->cl_cb_conn.cb_set, 0);
>  	INIT_LIST_HEAD(&clp->cl_idhash);
>  	INIT_LIST_HEAD(&clp->cl_strhash);
> @@ -1423,6 +1450,10 @@ nfsd4_sequence(struct svc_rqst *rqstp,
>  	if (!session)
>  		goto out;
>  
> +	/* keep the client from expiring */
> +	if (use_nfs4_client(cstate->session->se_client) < 0)
> +		goto out;
> +
>  	status = nfserr_badslot;
>  	if (seq->slotid >= session->se_fchannel.maxreqs)
>  		goto out;
> @@ -1463,12 +1494,6 @@ out:
>  		get_nfs4_client(cstate->session->se_client);
>  	}
>  	spin_unlock(&sessionid_lock);
> -	/* Renew the clientid on success and on replay */
> -	if (cstate->session) {
> -		nfs4_lock_state();
> -		renew_client(session->se_client);
> -		nfs4_unlock_state();
> -	}
>  	dprintk("%s: return %d\n", __func__, ntohl(status));
>  	return status;
>  }
> @@ -2575,6 +2600,9 @@ nfs4_laundromat(void)
>  		nfsd4_end_grace();
>  	list_for_each_safe(pos, next, &client_lru) {
>  		clp = list_entry(pos, struct nfs4_client, cl_lru);
> +		/* skip client if in use (cl_use_count is greater than zero) */
> +		if (atomic_cmpxchg(&clp->cl_use_count, 0, -1) > 0)
> +			continue;

I'll move that check after the time comparison
since there's no need for the atomic instruction if the client's
time out hasn't expired yet.

Benny

>  		if (time_after((unsigned long)clp->cl_time, (unsigned long)cutoff)) {
>  			t = clp->cl_time - cutoff;
>  			if (clientid_val > t)
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index aed733c..0f0b857 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -3313,6 +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;
>  		}
> +		renew_nfs4_client(cs->session->se_client);
> +		unuse_nfs4_client(cs->session->se_client);
>  		put_nfs4_client(cs->session->se_client);
>  		nfsd4_put_session(cs->session);
>  	}
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index e3c002e..806d9b8 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -214,6 +214,7 @@ struct nfs4_client {
>  	nfs4_verifier		cl_confirm;	/* generated by server */
>  	struct nfs4_cb_conn	cl_cb_conn;     /* callback info */
>  	atomic_t		cl_count;	/* ref count */
> +	atomic_t		cl_use_count;	/* session use count */
>  	u32			cl_firststate;	/* recovery dir creation */
>  
>  	/* for nfs41 */
> @@ -237,6 +238,12 @@ get_nfs4_client(struct nfs4_client *clp)
>  	atomic_inc(&clp->cl_count);
>  }
>  
> +static inline void
> +unuse_nfs4_client(struct nfs4_client *clp)
> +{
> +	atomic_dec(&clp->cl_use_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
> @@ -384,6 +391,8 @@ extern void nfs4_unlock_state(void);
>  extern int nfs4_in_grace(void);
>  extern __be32 nfs4_check_open_reclaim(clientid_t *clid);
>  extern void put_nfs4_client(struct nfs4_client *clp);
> +extern int use_nfs4_client(struct nfs4_client *clp);
> +extern void renew_nfs4_client(struct nfs4_client *clp);
>  extern void nfs4_free_stateowner(struct kref *kref);
>  extern int set_callback_cred(void);
>  extern void nfsd4_probe_callback(struct nfs4_client *clp);


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC 2/3] nfsd4: hold a reference on nfs4_client when it is used by a session
  2010-05-04  1:12     `  J. Bruce Fields
@ 2010-05-04  7:11       ` Benny Halevy
  2010-05-04 15:40         ` J. Bruce Fields
  2010-05-04 17:45         ` J. Bruce Fields
  0 siblings, 2 replies; 12+ messages in thread
From: Benny Halevy @ 2010-05-04  7:11 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: linux-nfs

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?

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC 2/3] nfsd4: hold a reference on nfs4_client when it is used by a session
  2010-05-04  7:11       ` Benny Halevy
@ 2010-05-04 15:40         ` J. Bruce Fields
  2010-05-04 17:45         ` J. Bruce Fields
  1 sibling, 0 replies; 12+ messages in thread
From: J. Bruce Fields @ 2010-05-04 15:40 UTC (permalink / raw)
  To: Benny Halevy; +Cc: linux-nfs

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
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC 2/3] nfsd4: hold a reference on nfs4_client when it is used by a session
  2010-05-04  7:11       ` Benny Halevy
  2010-05-04 15:40         ` J. Bruce Fields
@ 2010-05-04 17:45         ` J. Bruce Fields
  2010-05-04 21:40           ` Benny Halevy
  1 sibling, 1 reply; 12+ messages in thread
From: J. Bruce Fields @ 2010-05-04 17:45 UTC (permalink / raw)
  To: Benny Halevy; +Cc: linux-nfs

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,

Ouch, OK, I'd forgotten that case.

--b.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC 2/3] nfsd4: hold a reference on nfs4_client when it is used by a session
  2010-05-04 17:45         ` J. Bruce Fields
@ 2010-05-04 21:40           ` Benny Halevy
  0 siblings, 0 replies; 12+ messages in thread
From: Benny Halevy @ 2010-05-04 21:40 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: linux-nfs

On May. 04, 2010, 20:45 +0300, "J. Bruce Fields" <bfields@citi.umich.edu> wrote:
> 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,
> 
> Ouch, OK, I'd forgotten that case.

I'm working on a new version on top of your for-2.6.35 branch
that will solve both cases using the new ref count.

Benny

> 
> --b.
> 
>> 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?
>>
>> 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
>>

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2010-05-04 21:40 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-05-03 16:13 [RFC 0/3] nfsd41: do not expire client while in use by current compound Benny Halevy
2010-05-03 16:31 ` [RFC 1/3] nfsd4: use local variable in nfs4svc_encode_compoundres Benny Halevy
2010-05-03 22:37   `  J. Bruce Fields
2010-05-03 16:31 ` [RFC 2/3] nfsd4: hold a reference on nfs4_client when it is used by a session Benny Halevy
2010-05-03 22:36   `  J. Bruce Fields
2010-05-04  1:12     `  J. Bruce Fields
2010-05-04  7:11       ` Benny Halevy
2010-05-04 15:40         ` J. Bruce Fields
2010-05-04 17:45         ` J. Bruce Fields
2010-05-04 21:40           ` Benny Halevy
2010-05-03 16:31 ` [RFC 3/3] nfsd4: do not expire nfs41 clients while in use Benny Halevy
2010-05-04  5:39   ` Benny Halevy

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.