[RFC 0/11] state lock reduction prep

[RFC 0/11] state lock reduction prep

[Benny Halevy]

Bruce,

The following patches implement the first phase in preparation
for reducing the scope in we which we hold the state lock mutex.

[RFC 01/11] nfsd: introduce nfs4_client state
[RFC 02/11] nfsd: mark client state as expired in expire_client
[RFC 03/11] nfsd: introduce get_nfs4_client
[RFC 04/11] nfsd: mark client for renew
[RFC 05/11] nfsd: skip clients marked for renewal
[RFC 06/11] nfsd41: no need to hold the state lock around mark_client_for_renew

the patches above use an atomic cl_state field to manage the
nfs4_client expire vs. renew state so this can be done outside
the state lock.

[RFC 07/11] nfsd: rename recall_lock to deleg_lock
[RFC 08/11] nfsd: use deleg_lock to protect dl_perfile and dl_perclnt
[RFC 09/11] nfsd: get a reference count on the delgation while outside of the deleg_lock
[RFC 10/11] nfsd: close delegation only on last reference
[RFC 11/11] nfsd: refactor unhash_delegation

The patches above augment the use of the recall_lock spinlock
(and rename it to deleg_lock) and grab a reference count on
the delegation structure while not protected by the deleg_lock.
The delegation's respective file will be closed only on last
dereference, so the deleg structure could be used safely while
not holding the state_lock and potentially in parallel to
expire_client being called (and unhashing the delegation)

The next steps I see are:

1. use a spinlock for traversing the nfs4_client hash lists
2. use a spinlock for traversing the nfs4_stateowner lists

Eventually, reducing the state_lock scope for complex state changes
requiring the coordination of state across multiple lists
and involving blocking ops (e.g. allocation, manipulating clid_dir)
in particular there should be no need to hold the state lock
for expire_client.

Benny

[RFC 01/11] nfsd: introduce nfs4_client state

[Benny Halevy]

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

diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index fefeae2..7e67eca 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -186,6 +186,13 @@ struct nfsd4_sessionid {
 
 #define HEXDIR_LEN     33 /* hex version of 16 byte md5 of cl_name plus '\0' */
 
+/* nfs4_client states */
+enum nfs4_client_state {
+	CL_STATE_NORMAL,
+	CL_STATE_RENEW,
+	CL_STATE_EXPIRED,
+};
+
 /*
  * struct nfs4_client - one per client.  Clientids live here.
  * 	o Each nfs4_client is hashed by clientid.
@@ -214,6 +221,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_state;	/* renew / expiry state */
 	u32			cl_firststate;	/* recovery dir creation */
 
 	/* for nfs41 */
-- 
1.6.5.1

Re: [pnfs] [RFC 01/11] nfsd: introduce nfs4_client state

[Peter Staubach]

Benny Halevy wrote:
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> ---
>  fs/nfsd/state.h |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index fefeae2..7e67eca 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -186,6 +186,13 @@ struct nfsd4_sessionid {
>  
>  #define HEXDIR_LEN     33 /* hex version of 16 byte md5 of cl_name plus '\0' */
>  
> +/* nfs4_client states */
> +enum nfs4_client_state {
> +	CL_STATE_NORMAL,
> +	CL_STATE_RENEW,
> +	CL_STATE_EXPIRED,
> +};
> +
>  /*
>   * struct nfs4_client - one per client.  Clientids live here.
>   * 	o Each nfs4_client is hashed by clientid.
> @@ -214,6 +221,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_state;	/* renew / expiry state */

The use of an atomic here seems complex and makes the
implementation seem fragile to me.  Is the atomic_cmpxchg()
really going to save much time over just using a spinlock
and normal loads and stores to retrieve and get the value
of cl_state?  A spinlock would make things much more
obvious how they were supposed to work, to me, at least.

	Thanx...

		ps

>  	u32			cl_firststate;	/* recovery dir creation */
>  
>  	/* for nfs41 */

Re: [pnfs] [RFC 01/11] nfsd: introduce nfs4_client state

[Benny Halevy]

On Dec. 17, 2009, 0:33 +0200, Peter Staubach <staubach@redhat.com> wrote:
> Benny Halevy wrote:
>> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
>> ---
>>  fs/nfsd/state.h |    8 ++++++++
>>  1 files changed, 8 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>> index fefeae2..7e67eca 100644
>> --- a/fs/nfsd/state.h
>> +++ b/fs/nfsd/state.h
>> @@ -186,6 +186,13 @@ struct nfsd4_sessionid {
>>  
>>  #define HEXDIR_LEN     33 /* hex version of 16 byte md5 of cl_name plus '\0' */
>>  
>> +/* nfs4_client states */
>> +enum nfs4_client_state {
>> +	CL_STATE_NORMAL,
>> +	CL_STATE_RENEW,
>> +	CL_STATE_EXPIRED,
>> +};
>> +
>>  /*
>>   * struct nfs4_client - one per client.  Clientids live here.
>>   * 	o Each nfs4_client is hashed by clientid.
>> @@ -214,6 +221,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_state;	/* renew / expiry state */
> 
> The use of an atomic here seems complex and makes the
> implementation seem fragile to me.  Is the atomic_cmpxchg()
> really going to save much time over just using a spinlock
> and normal loads and stores to retrieve and get the value
> of cl_state?  A spinlock would make things much more
> obvious how they were supposed to work, to me, at least.
> 

This can be done of course, though logically, I'd still
consider defining the equivalent of cmpxchg as a static
inline helper since it does exactly what I want to to.
As for efficiency the cmpxchg is more as efficient as
an uncontended fine grain lock, with no chance of spinning.
atomic_t also has a smaller memory footprint than a
fine grain lock (per nsf4_client).  Using a coarse grain
lock is possible but will needless increase the chances
for contention.

Benny

> 	Thanx...
> 
> 		ps
> 
>>  	u32			cl_firststate;	/* recovery dir creation */
>>  
>>  	/* for nfs41 */
> 

[RFC 02/11] nfsd: mark client state as expired in expire_client

[Benny Halevy]

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

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index f19ed86..dd76682 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -724,6 +724,7 @@ expire_client(struct nfs4_client *clp)
 	dprintk("NFSD: expire_client cl_count %d\n",
 	                    atomic_read(&clp->cl_count));
 
+	atomic_set(&clp->cl_state, CL_STATE_EXPIRED);
 	INIT_LIST_HEAD(&reaplist);
 	spin_lock(&recall_lock);
 	while (!list_empty(&clp->cl_delegations)) {
-- 
1.6.5.1

[RFC 03/11] nfsd: introduce get_nfs4_client

[Benny Halevy]

define a public interface for increment the nfs4_client reference count.

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

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index c6eed2a..2b4d27b 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -567,7 +567,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 dd76682..c456551 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -708,6 +708,12 @@ free_client(struct nfs4_client *clp)
 }
 
 void
+get_nfs4_client(struct nfs4_client *clp)
+{
+	atomic_inc(&clp->cl_count);
+}
+
+void
 put_nfs4_client(struct nfs4_client *clp)
 {
 	if (atomic_dec_and_test(&clp->cl_count))
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 7e67eca..21109d4 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -385,6 +385,7 @@ extern void nfs4_lock_state(void);
 extern void nfs4_unlock_state(void);
 extern int nfs4_in_grace(void);
 extern __be32 nfs4_check_open_reclaim(clientid_t *clid);
+extern void get_nfs4_client(struct nfs4_client *);
 extern void put_nfs4_client(struct nfs4_client *clp);
 extern void nfs4_free_stateowner(struct kref *kref);
 extern int set_callback_cred(void);
-- 
1.6.5.1

[RFC 04/11] nfsd: mark client for renew

[Benny Halevy]

Mark the nfs4_client for renew on operations holding a stateid (or nfs41
OP_SEQUENCE).
Do the actual lru list update when the compound processing is complete.
This will prevents the client from being expired by the laundromat while
the compound is in progress.

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

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 37514c4..a8e9731 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1010,6 +1010,7 @@ nfsd4_proc_compound(struct svc_rqst *rqstp,
 	resp->opcnt = 0;
 	resp->rqstp = rqstp;
 	resp->cstate.minorversion = args->minorversion;
+	resp->cstate.renew_client = NULL;
 	resp->cstate.replay_owner = NULL;
 	fh_init(&resp->cstate.current_fh, NFS4_FHSIZE);
 	fh_init(&resp->cstate.save_fh, NFS4_FHSIZE);
@@ -1114,6 +1115,10 @@ encode_op:
 	fh_put(&resp->cstate.current_fh);
 	fh_put(&resp->cstate.save_fh);
 	BUG_ON(resp->cstate.replay_owner);
+	if (resp->cstate.renew_client) {
+		renew_client(resp->cstate.renew_client);
+		put_nfs4_client(resp->cstate.renew_client);
+	}
 out:
 	nfsd4_release_compoundargs(args);
 	/* Reset deferral mechanism for RPC deferrals */
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index c456551..16ccab3 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -634,8 +634,39 @@ free_session(struct kref *kref)
 }
 
 static inline void
+mark_client_for_renew(struct nfsd4_compound_state *cstate,
+		      struct nfs4_client *clp)
+{
+	int old_state = atomic_cmpxchg(&clp->cl_state, CL_STATE_NORMAL,
+				       CL_STATE_RENEW);
+
+	if (old_state != CL_STATE_EXPIRED) {
+		cstate->renew_client = clp;
+		get_nfs4_client(clp);
+	}
+
+	dprintk("%s: client (clientid %08x/%08x) %s\n",
+		__func__,
+		clp->cl_clientid.cl_boot,
+		clp->cl_clientid.cl_id,
+		old_state == CL_STATE_EXPIRED ? "already expired" :
+						"marked for renew");
+}
+
+void
 renew_client(struct nfs4_client *clp)
 {
+	int old_state = atomic_cmpxchg(&clp->cl_state, CL_STATE_RENEW,
+				       CL_STATE_NORMAL);
+
+	if (old_state == CL_STATE_EXPIRED) {
+		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.
 	*/
@@ -1471,7 +1502,7 @@ out:
 	/* Renew the clientid on success and on replay */
 	if (cstate->session) {
 		nfs4_lock_state();
-		renew_client(session->se_client);
+		mark_client_for_renew(cstate, session->se_client);
 		nfs4_unlock_state();
 	}
 	dprintk("%s: return %d\n", __func__, ntohl(status));
@@ -2830,7 +2861,7 @@ nfs4_preprocess_stateid_op(struct nfsd4_compound_state *cstate,
 		status = nfs4_check_delegmode(dp, flags);
 		if (status)
 			goto out;
-		renew_client(dp->dl_client);
+		mark_client_for_renew(cstate, dp->dl_client);
 		if (filpp)
 			*filpp = dp->dl_vfs_file;
 	} else { /* open or lock stateid */
@@ -2850,7 +2881,7 @@ nfs4_preprocess_stateid_op(struct nfsd4_compound_state *cstate,
 		status = nfs4_check_openmode(stp, flags);
 		if (status)
 			goto out;
-		renew_client(stp->st_stateowner->so_client);
+		mark_client_for_renew(cstate, stp->st_stateowner->so_client);
 		if (filpp)
 			*filpp = stp->st_vfs_file;
 	}
@@ -2970,7 +3001,7 @@ nfs4_preprocess_seqid_op(struct nfsd4_compound_state *cstate, u32 seqid,
 	status = check_stateid_generation(stateid, &stp->st_stateid, flags);
 	if (status)
 		return status;
-	renew_client(sop->so_client);
+	mark_client_for_renew(cstate, sop->so_client);
 	return nfs_ok;
 
 check_replay:
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 21109d4..b684c84 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -385,6 +385,7 @@ extern void nfs4_lock_state(void);
 extern void nfs4_unlock_state(void);
 extern int nfs4_in_grace(void);
 extern __be32 nfs4_check_open_reclaim(clientid_t *clid);
+extern void renew_client(struct nfs4_client *);
 extern void get_nfs4_client(struct nfs4_client *);
 extern void put_nfs4_client(struct nfs4_client *clp);
 extern void nfs4_free_stateowner(struct kref *kref);
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index efa3377..e48d5c7 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -46,6 +46,7 @@
 struct nfsd4_compound_state {
 	struct svc_fh		current_fh;
 	struct svc_fh		save_fh;
+	struct nfs4_client	*renew_client;
 	struct nfs4_stateowner	*replay_owner;
 	/* For sessions DRC */
 	struct nfsd4_session	*session;
-- 
1.6.5.1

Re: [RFC 04/11] nfsd: mark client for renew

[J. Bruce Fields]

On Wed, Dec 16, 2009 at 07:41:12PM +0200, Benny Halevy wrote:
> Mark the nfs4_client for renew on operations holding a stateid (or nfs41
> OP_SEQUENCE).
> Do the actual lru list update when the compound processing is complete.
> This will prevents the client from being expired by the laundromat while
> the compound is in progress.
> 
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> ---
>  fs/nfsd/nfs4proc.c  |    5 +++++
>  fs/nfsd/nfs4state.c |   39 +++++++++++++++++++++++++++++++++++----
>  fs/nfsd/state.h     |    1 +
>  fs/nfsd/xdr4.h      |    1 +
>  4 files changed, 42 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 37514c4..a8e9731 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1010,6 +1010,7 @@ nfsd4_proc_compound(struct svc_rqst *rqstp,
>  	resp->opcnt = 0;
>  	resp->rqstp = rqstp;
>  	resp->cstate.minorversion = args->minorversion;
> +	resp->cstate.renew_client = NULL;
>  	resp->cstate.replay_owner = NULL;
>  	fh_init(&resp->cstate.current_fh, NFS4_FHSIZE);
>  	fh_init(&resp->cstate.save_fh, NFS4_FHSIZE);
> @@ -1114,6 +1115,10 @@ encode_op:
>  	fh_put(&resp->cstate.current_fh);
>  	fh_put(&resp->cstate.save_fh);
>  	BUG_ON(resp->cstate.replay_owner);
> +	if (resp->cstate.renew_client) {
> +		renew_client(resp->cstate.renew_client);
> +		put_nfs4_client(resp->cstate.renew_client);
> +	}
>  out:
>  	nfsd4_release_compoundargs(args);
>  	/* Reset deferral mechanism for RPC deferrals */
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index c456551..16ccab3 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -634,8 +634,39 @@ free_session(struct kref *kref)
>  }
>  
>  static inline void
> +mark_client_for_renew(struct nfsd4_compound_state *cstate,
> +		      struct nfs4_client *clp)
> +{
> +	int old_state = atomic_cmpxchg(&clp->cl_state, CL_STATE_NORMAL,
> +				       CL_STATE_RENEW);
> +
> +	if (old_state != CL_STATE_EXPIRED) {
> +		cstate->renew_client = clp;
> +		get_nfs4_client(clp);

If cstate->renew_client is already set, then we may leak a reference
count here, right?

Also: at least in the v4 case, I don't think there's anything preventing
cstate->renew_client being already set to a different client.  (With
sessions hopefully that's not possible.)

This patch doesn't appear to stand alone as it removes the client
renewals without yet replacing them by anything.

--b.

> +	}
> +
> +	dprintk("%s: client (clientid %08x/%08x) %s\n",
> +		__func__,
> +		clp->cl_clientid.cl_boot,
> +		clp->cl_clientid.cl_id,
> +		old_state == CL_STATE_EXPIRED ? "already expired" :
> +						"marked for renew");
> +}
> +
> +void
>  renew_client(struct nfs4_client *clp)
>  {
> +	int old_state = atomic_cmpxchg(&clp->cl_state, CL_STATE_RENEW,
> +				       CL_STATE_NORMAL);
> +
> +	if (old_state == CL_STATE_EXPIRED) {
> +		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.
>  	*/
> @@ -1471,7 +1502,7 @@ out:
>  	/* Renew the clientid on success and on replay */
>  	if (cstate->session) {
>  		nfs4_lock_state();
> -		renew_client(session->se_client);
> +		mark_client_for_renew(cstate, session->se_client);
>  		nfs4_unlock_state();
>  	}
>  	dprintk("%s: return %d\n", __func__, ntohl(status));
> @@ -2830,7 +2861,7 @@ nfs4_preprocess_stateid_op(struct nfsd4_compound_state *cstate,
>  		status = nfs4_check_delegmode(dp, flags);
>  		if (status)
>  			goto out;
> -		renew_client(dp->dl_client);
> +		mark_client_for_renew(cstate, dp->dl_client);
>  		if (filpp)
>  			*filpp = dp->dl_vfs_file;
>  	} else { /* open or lock stateid */
> @@ -2850,7 +2881,7 @@ nfs4_preprocess_stateid_op(struct nfsd4_compound_state *cstate,
>  		status = nfs4_check_openmode(stp, flags);
>  		if (status)
>  			goto out;
> -		renew_client(stp->st_stateowner->so_client);
> +		mark_client_for_renew(cstate, stp->st_stateowner->so_client);
>  		if (filpp)
>  			*filpp = stp->st_vfs_file;
>  	}
> @@ -2970,7 +3001,7 @@ nfs4_preprocess_seqid_op(struct nfsd4_compound_state *cstate, u32 seqid,
>  	status = check_stateid_generation(stateid, &stp->st_stateid, flags);
>  	if (status)
>  		return status;
> -	renew_client(sop->so_client);
> +	mark_client_for_renew(cstate, sop->so_client);
>  	return nfs_ok;
>  
>  check_replay:
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 21109d4..b684c84 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -385,6 +385,7 @@ extern void nfs4_lock_state(void);
>  extern void nfs4_unlock_state(void);
>  extern int nfs4_in_grace(void);
>  extern __be32 nfs4_check_open_reclaim(clientid_t *clid);
> +extern void renew_client(struct nfs4_client *);
>  extern void get_nfs4_client(struct nfs4_client *);
>  extern void put_nfs4_client(struct nfs4_client *clp);
>  extern void nfs4_free_stateowner(struct kref *kref);
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index efa3377..e48d5c7 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -46,6 +46,7 @@
>  struct nfsd4_compound_state {
>  	struct svc_fh		current_fh;
>  	struct svc_fh		save_fh;
> +	struct nfs4_client	*renew_client;
>  	struct nfs4_stateowner	*replay_owner;
>  	/* For sessions DRC */
>  	struct nfsd4_session	*session;
> -- 
> 1.6.5.1
> 

Re: [RFC 04/11] nfsd: mark client for renew

[J. Bruce Fields]

On Wed, Dec 16, 2009 at 03:48:39PM -0500,  J. Bruce Fields wrote:
> On Wed, Dec 16, 2009 at 07:41:12PM +0200, Benny Halevy wrote:
> > Mark the nfs4_client for renew on operations holding a stateid (or nfs41
> > OP_SEQUENCE).
> > Do the actual lru list update when the compound processing is complete.
> > This will prevents the client from being expired by the laundromat while
> > the compound is in progress.
> > 
> > Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> > ---
> >  fs/nfsd/nfs4proc.c  |    5 +++++
> >  fs/nfsd/nfs4state.c |   39 +++++++++++++++++++++++++++++++++++----
> >  fs/nfsd/state.h     |    1 +
> >  fs/nfsd/xdr4.h      |    1 +
> >  4 files changed, 42 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 37514c4..a8e9731 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -1010,6 +1010,7 @@ nfsd4_proc_compound(struct svc_rqst *rqstp,
> >  	resp->opcnt = 0;
> >  	resp->rqstp = rqstp;
> >  	resp->cstate.minorversion = args->minorversion;
> > +	resp->cstate.renew_client = NULL;
> >  	resp->cstate.replay_owner = NULL;
> >  	fh_init(&resp->cstate.current_fh, NFS4_FHSIZE);
> >  	fh_init(&resp->cstate.save_fh, NFS4_FHSIZE);
> > @@ -1114,6 +1115,10 @@ encode_op:
> >  	fh_put(&resp->cstate.current_fh);
> >  	fh_put(&resp->cstate.save_fh);
> >  	BUG_ON(resp->cstate.replay_owner);
> > +	if (resp->cstate.renew_client) {
> > +		renew_client(resp->cstate.renew_client);
> > +		put_nfs4_client(resp->cstate.renew_client);
> > +	}
> >  out:
> >  	nfsd4_release_compoundargs(args);
> >  	/* Reset deferral mechanism for RPC deferrals */
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index c456551..16ccab3 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -634,8 +634,39 @@ free_session(struct kref *kref)
> >  }
> >  
> >  static inline void
> > +mark_client_for_renew(struct nfsd4_compound_state *cstate,
> > +		      struct nfs4_client *clp)
> > +{
> > +	int old_state = atomic_cmpxchg(&clp->cl_state, CL_STATE_NORMAL,
> > +				       CL_STATE_RENEW);
> > +
> > +	if (old_state != CL_STATE_EXPIRED) {
> > +		cstate->renew_client = clp;
> > +		get_nfs4_client(clp);
> 
> If cstate->renew_client is already set, then we may leak a reference
> count here, right?
> 
> Also: at least in the v4 case, I don't think there's anything preventing
> cstate->renew_client being already set to a different client.  (With
> sessions hopefully that's not possible.)
> 
> This patch doesn't appear to stand alone as it removes the client
> renewals without yet replacing them by anything.

Sorry, scratch the last sentence, I was overlooking the second
nfs4proc.c chunk above!

--b.

> 
> --b.
> 
> > +	}
> > +
> > +	dprintk("%s: client (clientid %08x/%08x) %s\n",
> > +		__func__,
> > +		clp->cl_clientid.cl_boot,
> > +		clp->cl_clientid.cl_id,
> > +		old_state == CL_STATE_EXPIRED ? "already expired" :
> > +						"marked for renew");
> > +}
> > +
> > +void
> >  renew_client(struct nfs4_client *clp)
> >  {
> > +	int old_state = atomic_cmpxchg(&clp->cl_state, CL_STATE_RENEW,
> > +				       CL_STATE_NORMAL);
> > +
> > +	if (old_state == CL_STATE_EXPIRED) {
> > +		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.
> >  	*/
> > @@ -1471,7 +1502,7 @@ out:
> >  	/* Renew the clientid on success and on replay */
> >  	if (cstate->session) {
> >  		nfs4_lock_state();
> > -		renew_client(session->se_client);
> > +		mark_client_for_renew(cstate, session->se_client);
> >  		nfs4_unlock_state();
> >  	}
> >  	dprintk("%s: return %d\n", __func__, ntohl(status));
> > @@ -2830,7 +2861,7 @@ nfs4_preprocess_stateid_op(struct nfsd4_compound_state *cstate,
> >  		status = nfs4_check_delegmode(dp, flags);
> >  		if (status)
> >  			goto out;
> > -		renew_client(dp->dl_client);
> > +		mark_client_for_renew(cstate, dp->dl_client);
> >  		if (filpp)
> >  			*filpp = dp->dl_vfs_file;
> >  	} else { /* open or lock stateid */
> > @@ -2850,7 +2881,7 @@ nfs4_preprocess_stateid_op(struct nfsd4_compound_state *cstate,
> >  		status = nfs4_check_openmode(stp, flags);
> >  		if (status)
> >  			goto out;
> > -		renew_client(stp->st_stateowner->so_client);
> > +		mark_client_for_renew(cstate, stp->st_stateowner->so_client);
> >  		if (filpp)
> >  			*filpp = stp->st_vfs_file;
> >  	}
> > @@ -2970,7 +3001,7 @@ nfs4_preprocess_seqid_op(struct nfsd4_compound_state *cstate, u32 seqid,
> >  	status = check_stateid_generation(stateid, &stp->st_stateid, flags);
> >  	if (status)
> >  		return status;
> > -	renew_client(sop->so_client);
> > +	mark_client_for_renew(cstate, sop->so_client);
> >  	return nfs_ok;
> >  
> >  check_replay:
> > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> > index 21109d4..b684c84 100644
> > --- a/fs/nfsd/state.h
> > +++ b/fs/nfsd/state.h
> > @@ -385,6 +385,7 @@ extern void nfs4_lock_state(void);
> >  extern void nfs4_unlock_state(void);
> >  extern int nfs4_in_grace(void);
> >  extern __be32 nfs4_check_open_reclaim(clientid_t *clid);
> > +extern void renew_client(struct nfs4_client *);
> >  extern void get_nfs4_client(struct nfs4_client *);
> >  extern void put_nfs4_client(struct nfs4_client *clp);
> >  extern void nfs4_free_stateowner(struct kref *kref);
> > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> > index efa3377..e48d5c7 100644
> > --- a/fs/nfsd/xdr4.h
> > +++ b/fs/nfsd/xdr4.h
> > @@ -46,6 +46,7 @@
> >  struct nfsd4_compound_state {
> >  	struct svc_fh		current_fh;
> >  	struct svc_fh		save_fh;
> > +	struct nfs4_client	*renew_client;
> >  	struct nfs4_stateowner	*replay_owner;
> >  	/* For sessions DRC */
> >  	struct nfsd4_session	*session;
> > -- 
> > 1.6.5.1
> > 

Re: [RFC 04/11] nfsd: mark client for renew

[Benny Halevy]

On Dec. 16, 2009, 22:53 +0200, " J. Bruce Fields" <bfields@citi.umich.edu> wrote:
> On Wed, Dec 16, 2009 at 03:48:39PM -0500,  J. Bruce Fields wrote:
>> On Wed, Dec 16, 2009 at 07:41:12PM +0200, Benny Halevy wrote:
>>> Mark the nfs4_client for renew on operations holding a stateid (or nfs41
>>> OP_SEQUENCE).
>>> Do the actual lru list update when the compound processing is complete.
>>> This will prevents the client from being expired by the laundromat while
>>> the compound is in progress.
>>>
>>> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
>>> ---
>>>  fs/nfsd/nfs4proc.c  |    5 +++++
>>>  fs/nfsd/nfs4state.c |   39 +++++++++++++++++++++++++++++++++++----
>>>  fs/nfsd/state.h     |    1 +
>>>  fs/nfsd/xdr4.h      |    1 +
>>>  4 files changed, 42 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>> index 37514c4..a8e9731 100644
>>> --- a/fs/nfsd/nfs4proc.c
>>> +++ b/fs/nfsd/nfs4proc.c
>>> @@ -1010,6 +1010,7 @@ nfsd4_proc_compound(struct svc_rqst *rqstp,
>>>  	resp->opcnt = 0;
>>>  	resp->rqstp = rqstp;
>>>  	resp->cstate.minorversion = args->minorversion;
>>> +	resp->cstate.renew_client = NULL;
>>>  	resp->cstate.replay_owner = NULL;
>>>  	fh_init(&resp->cstate.current_fh, NFS4_FHSIZE);
>>>  	fh_init(&resp->cstate.save_fh, NFS4_FHSIZE);
>>> @@ -1114,6 +1115,10 @@ encode_op:
>>>  	fh_put(&resp->cstate.current_fh);
>>>  	fh_put(&resp->cstate.save_fh);
>>>  	BUG_ON(resp->cstate.replay_owner);
>>> +	if (resp->cstate.renew_client) {
>>> +		renew_client(resp->cstate.renew_client);
>>> +		put_nfs4_client(resp->cstate.renew_client);
>>> +	}
>>>  out:
>>>  	nfsd4_release_compoundargs(args);
>>>  	/* Reset deferral mechanism for RPC deferrals */
>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>> index c456551..16ccab3 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -634,8 +634,39 @@ free_session(struct kref *kref)
>>>  }
>>>  
>>>  static inline void
>>> +mark_client_for_renew(struct nfsd4_compound_state *cstate,
>>> +		      struct nfs4_client *clp)
>>> +{
>>> +	int old_state = atomic_cmpxchg(&clp->cl_state, CL_STATE_NORMAL,
>>> +				       CL_STATE_RENEW);
>>> +
>>> +	if (old_state != CL_STATE_EXPIRED) {
>>> +		cstate->renew_client = clp;
>>> +		get_nfs4_client(clp);
>> If cstate->renew_client is already set, then we may leak a reference
>> count here, right?
>>
>> Also: at least in the v4 case, I don't think there's anything preventing
>> cstate->renew_client being already set to a different client.  (With
>> sessions hopefully that's not possible.)
>>

Yeah.  That's a valid point.  So if it is already set then
if it is the same client we can do nothing, else we can renew
the previous one here put it and replace cstate->renew_client.

Benny

>> This patch doesn't appear to stand alone as it removes the client
>> renewals without yet replacing them by anything.
> 
> Sorry, scratch the last sentence, I was overlooking the second
> nfs4proc.c chunk above!
> 
> --b.
> 
>> --b.
>>
>>> +	}
>>> +
>>> +	dprintk("%s: client (clientid %08x/%08x) %s\n",
>>> +		__func__,
>>> +		clp->cl_clientid.cl_boot,
>>> +		clp->cl_clientid.cl_id,
>>> +		old_state == CL_STATE_EXPIRED ? "already expired" :
>>> +						"marked for renew");
>>> +}
>>> +
>>> +void
>>>  renew_client(struct nfs4_client *clp)
>>>  {
>>> +	int old_state = atomic_cmpxchg(&clp->cl_state, CL_STATE_RENEW,
>>> +				       CL_STATE_NORMAL);
>>> +
>>> +	if (old_state == CL_STATE_EXPIRED) {
>>> +		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.
>>>  	*/
>>> @@ -1471,7 +1502,7 @@ out:
>>>  	/* Renew the clientid on success and on replay */
>>>  	if (cstate->session) {
>>>  		nfs4_lock_state();
>>> -		renew_client(session->se_client);
>>> +		mark_client_for_renew(cstate, session->se_client);
>>>  		nfs4_unlock_state();
>>>  	}
>>>  	dprintk("%s: return %d\n", __func__, ntohl(status));
>>> @@ -2830,7 +2861,7 @@ nfs4_preprocess_stateid_op(struct nfsd4_compound_state *cstate,
>>>  		status = nfs4_check_delegmode(dp, flags);
>>>  		if (status)
>>>  			goto out;
>>> -		renew_client(dp->dl_client);
>>> +		mark_client_for_renew(cstate, dp->dl_client);
>>>  		if (filpp)
>>>  			*filpp = dp->dl_vfs_file;
>>>  	} else { /* open or lock stateid */
>>> @@ -2850,7 +2881,7 @@ nfs4_preprocess_stateid_op(struct nfsd4_compound_state *cstate,
>>>  		status = nfs4_check_openmode(stp, flags);
>>>  		if (status)
>>>  			goto out;
>>> -		renew_client(stp->st_stateowner->so_client);
>>> +		mark_client_for_renew(cstate, stp->st_stateowner->so_client);
>>>  		if (filpp)
>>>  			*filpp = stp->st_vfs_file;
>>>  	}
>>> @@ -2970,7 +3001,7 @@ nfs4_preprocess_seqid_op(struct nfsd4_compound_state *cstate, u32 seqid,
>>>  	status = check_stateid_generation(stateid, &stp->st_stateid, flags);
>>>  	if (status)
>>>  		return status;
>>> -	renew_client(sop->so_client);
>>> +	mark_client_for_renew(cstate, sop->so_client);
>>>  	return nfs_ok;
>>>  
>>>  check_replay:
>>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>>> index 21109d4..b684c84 100644
>>> --- a/fs/nfsd/state.h
>>> +++ b/fs/nfsd/state.h
>>> @@ -385,6 +385,7 @@ extern void nfs4_lock_state(void);
>>>  extern void nfs4_unlock_state(void);
>>>  extern int nfs4_in_grace(void);
>>>  extern __be32 nfs4_check_open_reclaim(clientid_t *clid);
>>> +extern void renew_client(struct nfs4_client *);
>>>  extern void get_nfs4_client(struct nfs4_client *);
>>>  extern void put_nfs4_client(struct nfs4_client *clp);
>>>  extern void nfs4_free_stateowner(struct kref *kref);
>>> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
>>> index efa3377..e48d5c7 100644
>>> --- a/fs/nfsd/xdr4.h
>>> +++ b/fs/nfsd/xdr4.h
>>> @@ -46,6 +46,7 @@
>>>  struct nfsd4_compound_state {
>>>  	struct svc_fh		current_fh;
>>>  	struct svc_fh		save_fh;
>>> +	struct nfs4_client	*renew_client;
>>>  	struct nfs4_stateowner	*replay_owner;
>>>  	/* For sessions DRC */
>>>  	struct nfsd4_session	*session;
>>> -- 
>>> 1.6.5.1
>>>

Re: [RFC 04/11] nfsd: mark client for renew

[J. Bruce Fields]

On Wed, Dec 16, 2009 at 11:56:05PM +0200, Benny Halevy wrote:
> On Dec. 16, 2009, 22:53 +0200, " J. Bruce Fields" <bfields@citi.umich.edu> wrote:
> > On Wed, Dec 16, 2009 at 03:48:39PM -0500,  J. Bruce Fields wrote:
> >> On Wed, Dec 16, 2009 at 07:41:12PM +0200, Benny Halevy wrote:
> >>> Mark the nfs4_client for renew on operations holding a stateid (or nfs41
> >>> OP_SEQUENCE).
> >>> Do the actual lru list update when the compound processing is complete.
> >>> This will prevents the client from being expired by the laundromat while
> >>> the compound is in progress.
> >>>
> >>> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> >>> ---
> >>>  fs/nfsd/nfs4proc.c  |    5 +++++
> >>>  fs/nfsd/nfs4state.c |   39 +++++++++++++++++++++++++++++++++++----
> >>>  fs/nfsd/state.h     |    1 +
> >>>  fs/nfsd/xdr4.h      |    1 +
> >>>  4 files changed, 42 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> >>> index 37514c4..a8e9731 100644
> >>> --- a/fs/nfsd/nfs4proc.c
> >>> +++ b/fs/nfsd/nfs4proc.c
> >>> @@ -1010,6 +1010,7 @@ nfsd4_proc_compound(struct svc_rqst *rqstp,
> >>>  	resp->opcnt = 0;
> >>>  	resp->rqstp = rqstp;
> >>>  	resp->cstate.minorversion = args->minorversion;
> >>> +	resp->cstate.renew_client = NULL;
> >>>  	resp->cstate.replay_owner = NULL;
> >>>  	fh_init(&resp->cstate.current_fh, NFS4_FHSIZE);
> >>>  	fh_init(&resp->cstate.save_fh, NFS4_FHSIZE);
> >>> @@ -1114,6 +1115,10 @@ encode_op:
> >>>  	fh_put(&resp->cstate.current_fh);
> >>>  	fh_put(&resp->cstate.save_fh);
> >>>  	BUG_ON(resp->cstate.replay_owner);
> >>> +	if (resp->cstate.renew_client) {
> >>> +		renew_client(resp->cstate.renew_client);
> >>> +		put_nfs4_client(resp->cstate.renew_client);
> >>> +	}
> >>>  out:
> >>>  	nfsd4_release_compoundargs(args);
> >>>  	/* Reset deferral mechanism for RPC deferrals */
> >>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >>> index c456551..16ccab3 100644
> >>> --- a/fs/nfsd/nfs4state.c
> >>> +++ b/fs/nfsd/nfs4state.c
> >>> @@ -634,8 +634,39 @@ free_session(struct kref *kref)
> >>>  }
> >>>  
> >>>  static inline void
> >>> +mark_client_for_renew(struct nfsd4_compound_state *cstate,
> >>> +		      struct nfs4_client *clp)
> >>> +{
> >>> +	int old_state = atomic_cmpxchg(&clp->cl_state, CL_STATE_NORMAL,
> >>> +				       CL_STATE_RENEW);
> >>> +
> >>> +	if (old_state != CL_STATE_EXPIRED) {
> >>> +		cstate->renew_client = clp;
> >>> +		get_nfs4_client(clp);
> >> If cstate->renew_client is already set, then we may leak a reference
> >> count here, right?
> >>
> >> Also: at least in the v4 case, I don't think there's anything preventing
> >> cstate->renew_client being already set to a different client.  (With
> >> sessions hopefully that's not possible.)
> >>
> 
> Yeah.  That's a valid point.  So if it is already set then
> if it is the same client we can do nothing, else we can renew
> the previous one here put it and replace cstate->renew_client.

I'm not sure if that can only happen if someone's intentionally messing
with us, or if maybe there's a legitimate case where it can happen
(maybe when a new client is created by some op in the compound?) But,
yes, that sounds like a safe way to handle it.

--b.

Re: [RFC 04/11] nfsd: mark client for renew

[Benny Halevy]

On Dec. 17, 2009, 0:06 +0200, "J. Bruce Fields" <bfields@citi.umich.edu> wrote:
> On Wed, Dec 16, 2009 at 11:56:05PM +0200, Benny Halevy wrote:
>> On Dec. 16, 2009, 22:53 +0200, " J. Bruce Fields" <bfields@citi.umich.edu> wrote:
>>> On Wed, Dec 16, 2009 at 03:48:39PM -0500,  J. Bruce Fields wrote:
>>>> On Wed, Dec 16, 2009 at 07:41:12PM +0200, Benny Halevy wrote:
>>>>> Mark the nfs4_client for renew on operations holding a stateid (or nfs41
>>>>> OP_SEQUENCE).
>>>>> Do the actual lru list update when the compound processing is complete.
>>>>> This will prevents the client from being expired by the laundromat while
>>>>> the compound is in progress.
>>>>>
>>>>> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
>>>>> ---
>>>>>  fs/nfsd/nfs4proc.c  |    5 +++++
>>>>>  fs/nfsd/nfs4state.c |   39 +++++++++++++++++++++++++++++++++++----
>>>>>  fs/nfsd/state.h     |    1 +
>>>>>  fs/nfsd/xdr4.h      |    1 +
>>>>>  4 files changed, 42 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>>>> index 37514c4..a8e9731 100644
>>>>> --- a/fs/nfsd/nfs4proc.c
>>>>> +++ b/fs/nfsd/nfs4proc.c
>>>>> @@ -1010,6 +1010,7 @@ nfsd4_proc_compound(struct svc_rqst *rqstp,
>>>>>  	resp->opcnt = 0;
>>>>>  	resp->rqstp = rqstp;
>>>>>  	resp->cstate.minorversion = args->minorversion;
>>>>> +	resp->cstate.renew_client = NULL;
>>>>>  	resp->cstate.replay_owner = NULL;
>>>>>  	fh_init(&resp->cstate.current_fh, NFS4_FHSIZE);
>>>>>  	fh_init(&resp->cstate.save_fh, NFS4_FHSIZE);
>>>>> @@ -1114,6 +1115,10 @@ encode_op:
>>>>>  	fh_put(&resp->cstate.current_fh);
>>>>>  	fh_put(&resp->cstate.save_fh);
>>>>>  	BUG_ON(resp->cstate.replay_owner);
>>>>> +	if (resp->cstate.renew_client) {
>>>>> +		renew_client(resp->cstate.renew_client);
>>>>> +		put_nfs4_client(resp->cstate.renew_client);
>>>>> +	}
>>>>>  out:
>>>>>  	nfsd4_release_compoundargs(args);
>>>>>  	/* Reset deferral mechanism for RPC deferrals */
>>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>>> index c456551..16ccab3 100644
>>>>> --- a/fs/nfsd/nfs4state.c
>>>>> +++ b/fs/nfsd/nfs4state.c
>>>>> @@ -634,8 +634,39 @@ free_session(struct kref *kref)
>>>>>  }
>>>>>  
>>>>>  static inline void
>>>>> +mark_client_for_renew(struct nfsd4_compound_state *cstate,
>>>>> +		      struct nfs4_client *clp)
>>>>> +{
>>>>> +	int old_state = atomic_cmpxchg(&clp->cl_state, CL_STATE_NORMAL,
>>>>> +				       CL_STATE_RENEW);
>>>>> +
>>>>> +	if (old_state != CL_STATE_EXPIRED) {
>>>>> +		cstate->renew_client = clp;
>>>>> +		get_nfs4_client(clp);
>>>> If cstate->renew_client is already set, then we may leak a reference
>>>> count here, right?
>>>>
>>>> Also: at least in the v4 case, I don't think there's anything preventing
>>>> cstate->renew_client being already set to a different client.  (With
>>>> sessions hopefully that's not possible.)
>>>>
>> Yeah.  That's a valid point.  So if it is already set then
>> if it is the same client we can do nothing, else we can renew
>> the previous one here put it and replace cstate->renew_client.
> 
> I'm not sure if that can only happen if someone's intentionally messing
> with us, or if maybe there's a legitimate case where it can happen
> (maybe when a new client is created by some op in the compound?) But,
> yes, that sounds like a safe way to handle it.
> 

Can the client send a compound with multiple I/Os for different
stateids (and even filehandles) that lead back to different clientids?

Benny

> --b.

Re: [RFC 04/11] nfsd: mark client for renew

[J. Bruce Fields]

On Thu, Dec 17, 2009 at 12:23:41AM +0200, Benny Halevy wrote:
> On Dec. 17, 2009, 0:06 +0200, "J. Bruce Fields" <bfields@citi.umich.edu> wrote:
> > On Wed, Dec 16, 2009 at 11:56:05PM +0200, Benny Halevy wrote:
> >> On Dec. 16, 2009, 22:53 +0200, " J. Bruce Fields" <bfields@citi.umich.edu> wrote:
> >>> On Wed, Dec 16, 2009 at 03:48:39PM -0500,  J. Bruce Fields wrote:
> >>>> On Wed, Dec 16, 2009 at 07:41:12PM +0200, Benny Halevy wrote:
> >>>>>  static inline void
> >>>>> +mark_client_for_renew(struct nfsd4_compound_state *cstate,
> >>>>> +		      struct nfs4_client *clp)
> >>>>> +{
> >>>>> +	int old_state = atomic_cmpxchg(&clp->cl_state, CL_STATE_NORMAL,
> >>>>> +				       CL_STATE_RENEW);
> >>>>> +
> >>>>> +	if (old_state != CL_STATE_EXPIRED) {
> >>>>> +		cstate->renew_client = clp;
> >>>>> +		get_nfs4_client(clp);
> >>>> If cstate->renew_client is already set, then we may leak a reference
> >>>> count here, right?
> >>>>
> >>>> Also: at least in the v4 case, I don't think there's anything preventing
> >>>> cstate->renew_client being already set to a different client.  (With
> >>>> sessions hopefully that's not possible.)
> >>>>
> >> Yeah.  That's a valid point.  So if it is already set then
> >> if it is the same client we can do nothing, else we can renew
> >> the previous one here put it and replace cstate->renew_client.
> > 
> > I'm not sure if that can only happen if someone's intentionally messing
> > with us, or if maybe there's a legitimate case where it can happen
> > (maybe when a new client is created by some op in the compound?) But,
> > yes, that sounds like a safe way to handle it.
> > 
> 
> Can the client send a compound with multiple I/Os for different
> stateids (and even filehandles) that lead back to different clientids?

I don't know of anything in rfc 3530 that forbids it, but I doubt it
would be useful to any legitimate client.

--b.

[RFC 05/11] nfsd: skip clients marked for renewal

[Benny Halevy]

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

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 16ccab3..15f4b63 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2613,6 +2613,9 @@ nfs4_laundromat(void)
 				clientid_val = t;
 			break;
 		}
+		/* skip clients marked for renewal */
+		if (atomic_read(&clp->cl_state) == CL_STATE_RENEW)
+			continue;
 		dprintk("NFSD: purging unused client (clientid %08x)\n",
 			clp->cl_clientid.cl_id);
 		nfsd4_remove_clid_dir(clp);
-- 
1.6.5.1

Re: [RFC 05/11] nfsd: skip clients marked for renewal

[J. Bruce Fields]

On Wed, Dec 16, 2009 at 07:41:25PM +0200, Benny Halevy wrote:
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> ---
>  fs/nfsd/nfs4state.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 16ccab3..15f4b63 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -2613,6 +2613,9 @@ nfs4_laundromat(void)
>  				clientid_val = t;
>  			break;
>  		}
> +		/* skip clients marked for renewal */
> +		if (atomic_read(&clp->cl_state) == CL_STATE_RENEW)
> +			continue;

I may just not understand your locking scheme.

But how are concurrent expiriry and renewal handled?  Couldn't the state
be marked CL_STATE_RENEW after this check but before CL_STATE_EXPIRED is
set in the following expire_client()?  What happens then?

--b.

>  		dprintk("NFSD: purging unused client (clientid %08x)\n",
>  			clp->cl_clientid.cl_id);
>  		nfsd4_remove_clid_dir(clp);
> -- 
> 1.6.5.1
> 

Re: [RFC 05/11] nfsd: skip clients marked for renewal

[Benny Halevy]

On Dec. 16, 2009, 22:58 +0200, " J. Bruce Fields" <bfields@citi.umich.edu> wrote:
> On Wed, Dec 16, 2009 at 07:41:25PM +0200, Benny Halevy wrote:
>> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
>> ---
>>  fs/nfsd/nfs4state.c |    3 +++
>>  1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 16ccab3..15f4b63 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -2613,6 +2613,9 @@ nfs4_laundromat(void)
>>  				clientid_val = t;
>>  			break;
>>  		}
>> +		/* skip clients marked for renewal */
>> +		if (atomic_read(&clp->cl_state) == CL_STATE_RENEW)
>> +			continue;
> 
> I may just not understand your locking scheme.
> 
> But how are concurrent expiriry and renewal handled?  Couldn't the state
> be marked CL_STATE_RENEW after this check but before CL_STATE_EXPIRED is
> set in the following expire_client()?  What happens then?

You're right.  This should be a cmpxchg form CL_STATE_NORMAL to
CL_STATE_EXPIRED, and then looking at the old val.

Benny

> 
> --b.
> 
>>  		dprintk("NFSD: purging unused client (clientid %08x)\n",
>>  			clp->cl_clientid.cl_id);
>>  		nfsd4_remove_clid_dir(clp);
>> -- 
>> 1.6.5.1
>>

[RFC 06/11] nfsd41: no need to hold the state lock around mark_client_for_renew

[Benny Halevy]

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

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 15f4b63..38817c3 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1500,11 +1500,8 @@ nfsd4_sequence(struct svc_rqst *rqstp,
 out:
 	spin_unlock(&sessionid_lock);
 	/* Renew the clientid on success and on replay */
-	if (cstate->session) {
-		nfs4_lock_state();
+	if (cstate->session)
 		mark_client_for_renew(cstate, session->se_client);
-		nfs4_unlock_state();
-	}
 	dprintk("%s: return %d\n", __func__, ntohl(status));
 	return status;
 }
-- 
1.6.5.1

[RFC 07/11] nfsd: rename recall_lock to deleg_lock

[Benny Halevy]

in preparation to expanding its usage to other deleg related lists.

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

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 38817c3..06e6e2e 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -74,7 +74,7 @@ static DEFINE_MUTEX(client_mutex);
  * effort to decrease the scope of the client_mutex, this spinlock may
  * eventually cover more:
  */
-static DEFINE_SPINLOCK(recall_lock);
+static DEFINE_SPINLOCK(deleg_lock);
 
 static struct kmem_cache *stateowner_slab = NULL;
 static struct kmem_cache *file_slab = NULL;
@@ -111,9 +111,9 @@ static struct list_head del_recall_lru;
 static inline void
 put_nfs4_file(struct nfs4_file *fi)
 {
-	if (atomic_dec_and_lock(&fi->fi_ref, &recall_lock)) {
+	if (atomic_dec_and_lock(&fi->fi_ref, &deleg_lock)) {
 		list_del(&fi->fi_hash);
-		spin_unlock(&recall_lock);
+		spin_unlock(&deleg_lock);
 		iput(fi->fi_inode);
 		kmem_cache_free(file_slab, fi);
 	}
@@ -237,9 +237,9 @@ unhash_delegation(struct nfs4_delegation *dp)
 {
 	list_del_init(&dp->dl_perfile);
 	list_del_init(&dp->dl_perclnt);
-	spin_lock(&recall_lock);
+	spin_lock(&deleg_lock);
 	list_del_init(&dp->dl_recall_lru);
-	spin_unlock(&recall_lock);
+	spin_unlock(&deleg_lock);
 	nfs4_close_delegation(dp);
 	nfs4_put_delegation(dp);
 }
@@ -763,7 +763,7 @@ expire_client(struct nfs4_client *clp)
 
 	atomic_set(&clp->cl_state, CL_STATE_EXPIRED);
 	INIT_LIST_HEAD(&reaplist);
-	spin_lock(&recall_lock);
+	spin_lock(&deleg_lock);
 	while (!list_empty(&clp->cl_delegations)) {
 		dp = list_entry(clp->cl_delegations.next, struct nfs4_delegation, dl_perclnt);
 		dprintk("NFSD: expire client. dp %p, fp %p\n", dp,
@@ -771,7 +771,7 @@ expire_client(struct nfs4_client *clp)
 		list_del_init(&dp->dl_perclnt);
 		list_move(&dp->dl_recall_lru, &reaplist);
 	}
-	spin_unlock(&recall_lock);
+	spin_unlock(&deleg_lock);
 	while (!list_empty(&reaplist)) {
 		dp = list_entry(reaplist.next, struct nfs4_delegation, dl_recall_lru);
 		list_del_init(&dp->dl_recall_lru);
@@ -1734,9 +1734,9 @@ alloc_init_file(struct inode *ino)
 		INIT_LIST_HEAD(&fp->fi_hash);
 		INIT_LIST_HEAD(&fp->fi_stateids);
 		INIT_LIST_HEAD(&fp->fi_delegations);
-		spin_lock(&recall_lock);
+		spin_lock(&deleg_lock);
 		list_add(&fp->fi_hash, &file_hashtbl[hashval]);
-		spin_unlock(&recall_lock);
+		spin_unlock(&deleg_lock);
 		fp->fi_inode = igrab(ino);
 		fp->fi_id = current_fileid++;
 		fp->fi_had_conflict = false;
@@ -1910,15 +1910,15 @@ find_file(struct inode *ino)
 	unsigned int hashval = file_hashval(ino);
 	struct nfs4_file *fp;
 
-	spin_lock(&recall_lock);
+	spin_lock(&deleg_lock);
 	list_for_each_entry(fp, &file_hashtbl[hashval], fi_hash) {
 		if (fp->fi_inode == ino) {
 			get_nfs4_file(fp);
-			spin_unlock(&recall_lock);
+			spin_unlock(&deleg_lock);
 			return fp;
 		}
 	}
-	spin_unlock(&recall_lock);
+	spin_unlock(&deleg_lock);
 	return NULL;
 }
 
@@ -2062,9 +2062,9 @@ void nfsd_break_deleg_cb(struct file_lock *fl)
 	atomic_inc(&dp->dl_count);
 	atomic_inc(&dp->dl_client->cl_count);
 
-	spin_lock(&recall_lock);
+	spin_lock(&deleg_lock);
 	list_add_tail(&dp->dl_recall_lru, &del_recall_lru);
-	spin_unlock(&recall_lock);
+	spin_unlock(&deleg_lock);
 
 	/* only place dl_time is set. protected by lock_kernel*/
 	dp->dl_time = get_seconds();
@@ -2619,7 +2619,7 @@ nfs4_laundromat(void)
 		expire_client(clp);
 	}
 	INIT_LIST_HEAD(&reaplist);
-	spin_lock(&recall_lock);
+	spin_lock(&deleg_lock);
 	list_for_each_safe(pos, next, &del_recall_lru) {
 		dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
 		if (time_after((unsigned long)dp->dl_time, (unsigned long)cutoff)) {
@@ -2632,7 +2632,7 @@ nfs4_laundromat(void)
 			            dp, dp->dl_flock);
 		list_move(&dp->dl_recall_lru, &reaplist);
 	}
-	spin_unlock(&recall_lock);
+	spin_unlock(&deleg_lock);
 	list_for_each_safe(pos, next, &reaplist) {
 		dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
 		list_del_init(&dp->dl_recall_lru);
@@ -4097,12 +4097,12 @@ __nfs4_state_shutdown(void)
 		}
 	}
 	INIT_LIST_HEAD(&reaplist);
-	spin_lock(&recall_lock);
+	spin_lock(&deleg_lock);
 	list_for_each_safe(pos, next, &del_recall_lru) {
 		dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
 		list_move(&dp->dl_recall_lru, &reaplist);
 	}
-	spin_unlock(&recall_lock);
+	spin_unlock(&deleg_lock);
 	list_for_each_safe(pos, next, &reaplist) {
 		dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
 		list_del_init(&dp->dl_recall_lru);
-- 
1.6.5.1

[RFC 08/11] nfsd: use deleg_lock to protect dl_perfile and dl_perclnt

[Benny Halevy]

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

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 06e6e2e..ae49d1e 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -196,8 +196,10 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_stateid *stp, struct svc_f
 	fh_copy_shallow(&dp->dl_fh, &current_fh->fh_handle);
 	dp->dl_time = 0;
 	atomic_set(&dp->dl_count, 1);
+	spin_lock(&deleg_lock);
 	list_add(&dp->dl_perfile, &fp->fi_delegations);
 	list_add(&dp->dl_perclnt, &clp->cl_delegations);
+	spin_unlock(&deleg_lock);
 	return dp;
 }
 
@@ -235,9 +237,9 @@ nfs4_close_delegation(struct nfs4_delegation *dp)
 static void
 unhash_delegation(struct nfs4_delegation *dp)
 {
+	spin_lock(&deleg_lock);
 	list_del_init(&dp->dl_perfile);
 	list_del_init(&dp->dl_perclnt);
-	spin_lock(&deleg_lock);
 	list_del_init(&dp->dl_recall_lru);
 	spin_unlock(&deleg_lock);
 	nfs4_close_delegation(dp);
@@ -2222,13 +2224,15 @@ nfs4_check_delegmode(struct nfs4_delegation *dp, int flags)
 static struct nfs4_delegation *
 find_delegation_file(struct nfs4_file *fp, stateid_t *stid)
 {
-	struct nfs4_delegation *dp;
+	struct nfs4_delegation *dp = NULL;
 
+	spin_lock(&deleg_lock);
 	list_for_each_entry(dp, &fp->fi_delegations, dl_perfile) {
 		if (dp->dl_stateid.si_stateownerid == stid->si_stateownerid)
-			return dp;
+			break;
 	}
-	return NULL;
+	spin_unlock(&deleg_lock);
+	return dp;
 }
 
 static __be32
-- 
1.6.5.1

[RFC 09/11] nfsd: get a reference count on the delgation while outside of the deleg_lock

[Benny Halevy]

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

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index ae49d1e..411226a 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2228,8 +2228,10 @@ find_delegation_file(struct nfs4_file *fp, stateid_t *stid)
 
 	spin_lock(&deleg_lock);
 	list_for_each_entry(dp, &fp->fi_delegations, dl_perfile) {
-		if (dp->dl_stateid.si_stateownerid == stid->si_stateownerid)
+		if (dp->dl_stateid.si_stateownerid == stid->si_stateownerid) {
+			atomic_inc(&dp->dl_count);
 			break;
+		}
 	}
 	spin_unlock(&deleg_lock);
 	return dp;
@@ -2533,6 +2535,8 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 	dprintk("%s: stateid=" STATEID_FMT "\n", __func__,
 		STATEID_VAL(&stp->st_stateid));
 out:
+	if (dp)
+		nfs4_put_delegation(dp);
 	if (fp)
 		put_nfs4_file(fp);
 	if (status == 0 && open->op_claim_type == NFS4_OPEN_CLAIM_PREVIOUS)
@@ -2891,6 +2895,8 @@ nfs4_preprocess_stateid_op(struct nfsd4_compound_state *cstate,
 	}
 	status = nfs_ok;
 out:
+	if (dp)
+		nfs4_put_delegation(dp);
 	return status;
 }
 
-- 
1.6.5.1

[RFC 10/11] nfsd: close delegation only on last reference

[Benny Halevy]

as dl_vfs_file may be used while the delegation structure is
being referenced.

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

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 411226a..053fd2b 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -203,29 +203,17 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_stateid *stp, struct svc_f
 	return dp;
 }
 
-void
-nfs4_put_delegation(struct nfs4_delegation *dp)
-{
-	if (atomic_dec_and_test(&dp->dl_count)) {
-		dprintk("NFSD: freeing dp %p\n",dp);
-		put_nfs4_file(dp->dl_file);
-		kmem_cache_free(deleg_slab, dp);
-		num_delegations--;
-	}
-}
-
 /* Remove the associated file_lock first, then remove the delegation.
  * lease_modify() is called to remove the FS_LEASE file_lock from
  * the i_flock list, eventually calling nfsd's lock_manager
  * fl_release_callback.
  */
 static void
-nfs4_close_delegation(struct nfs4_delegation *dp)
+__close_delegation(struct nfs4_delegation *dp)
 {
 	struct file *filp = dp->dl_vfs_file;
 
 	dprintk("NFSD: close_delegation dp %p\n",dp);
-	dp->dl_vfs_file = NULL;
 	/* The following nfsd_close may not actually close the file,
 	 * but we want to remove the lease in any case. */
 	if (dp->dl_flock)
@@ -233,6 +221,18 @@ nfs4_close_delegation(struct nfs4_delegation *dp)
 	nfsd_close(filp);
 }
 
+void
+nfs4_put_delegation(struct nfs4_delegation *dp)
+{
+	if (atomic_dec_and_test(&dp->dl_count)) {
+		dprintk("NFSD: freeing dp %p\n",dp);
+		__close_delegation(dp);
+		put_nfs4_file(dp->dl_file);
+		kmem_cache_free(deleg_slab, dp);
+		num_delegations--;
+	}
+}
+
 /* Called under the state lock. */
 static void
 unhash_delegation(struct nfs4_delegation *dp)
@@ -242,7 +242,6 @@ unhash_delegation(struct nfs4_delegation *dp)
 	list_del_init(&dp->dl_perclnt);
 	list_del_init(&dp->dl_recall_lru);
 	spin_unlock(&deleg_lock);
-	nfs4_close_delegation(dp);
 	nfs4_put_delegation(dp);
 }
 
-- 
1.6.5.1

Re: [RFC 10/11] nfsd: close delegation only on last reference

[J. Bruce Fields]

On Wed, Dec 16, 2009 at 07:42:29PM +0200, Benny Halevy wrote:
> as dl_vfs_file may be used while the delegation structure is
> being referenced.

A callback to the client (which can last a while if the client is
unresponsive) can also hold a reference to the delegation.  It doesn't
need a reference on the file.

I guess I can't see any bug in holding a reference on the file in that
case, but it seems better not to (if only to allow freeing it earlier).

Maybe the limited amount of information used by the callback should be
reference-counted separately.  I'm not sure.

--b.

> 
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> ---
>  fs/nfsd/nfs4state.c |   27 +++++++++++++--------------
>  1 files changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 411226a..053fd2b 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -203,29 +203,17 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_stateid *stp, struct svc_f
>  	return dp;
>  }
>  
> -void
> -nfs4_put_delegation(struct nfs4_delegation *dp)
> -{
> -	if (atomic_dec_and_test(&dp->dl_count)) {
> -		dprintk("NFSD: freeing dp %p\n",dp);
> -		put_nfs4_file(dp->dl_file);
> -		kmem_cache_free(deleg_slab, dp);
> -		num_delegations--;
> -	}
> -}
> -
>  /* Remove the associated file_lock first, then remove the delegation.
>   * lease_modify() is called to remove the FS_LEASE file_lock from
>   * the i_flock list, eventually calling nfsd's lock_manager
>   * fl_release_callback.
>   */
>  static void
> -nfs4_close_delegation(struct nfs4_delegation *dp)
> +__close_delegation(struct nfs4_delegation *dp)
>  {
>  	struct file *filp = dp->dl_vfs_file;
>  
>  	dprintk("NFSD: close_delegation dp %p\n",dp);
> -	dp->dl_vfs_file = NULL;
>  	/* The following nfsd_close may not actually close the file,
>  	 * but we want to remove the lease in any case. */
>  	if (dp->dl_flock)
> @@ -233,6 +221,18 @@ nfs4_close_delegation(struct nfs4_delegation *dp)
>  	nfsd_close(filp);
>  }
>  
> +void
> +nfs4_put_delegation(struct nfs4_delegation *dp)
> +{
> +	if (atomic_dec_and_test(&dp->dl_count)) {
> +		dprintk("NFSD: freeing dp %p\n",dp);
> +		__close_delegation(dp);
> +		put_nfs4_file(dp->dl_file);
> +		kmem_cache_free(deleg_slab, dp);
> +		num_delegations--;
> +	}
> +}
> +
>  /* Called under the state lock. */
>  static void
>  unhash_delegation(struct nfs4_delegation *dp)
> @@ -242,7 +242,6 @@ unhash_delegation(struct nfs4_delegation *dp)
>  	list_del_init(&dp->dl_perclnt);
>  	list_del_init(&dp->dl_recall_lru);
>  	spin_unlock(&deleg_lock);
> -	nfs4_close_delegation(dp);
>  	nfs4_put_delegation(dp);
>  }
>  
> -- 
> 1.6.5.1
> 

Re: [RFC 10/11] nfsd: close delegation only on last reference

[Benny Halevy]

On Dec. 16, 2009, 23:18 +0200, " J. Bruce Fields" <bfields@citi.umich.edu> wrote:
> On Wed, Dec 16, 2009 at 07:42:29PM +0200, Benny Halevy wrote:
>> as dl_vfs_file may be used while the delegation structure is
>> being referenced.
> 
> A callback to the client (which can last a while if the client is
> unresponsive) can also hold a reference to the delegation.  It doesn't
> need a reference on the file.
> 
> I guess I can't see any bug in holding a reference on the file in that
> case, but it seems better not to (if only to allow freeing it earlier).
> 
> Maybe the limited amount of information used by the callback should be
> reference-counted separately.  I'm not sure.
> 

If we lock the delegation, say by grabbing the global deleg_lock
(or a fine-grained per-deleg lock) we can make sure dl_vfs_file
is accessed atomically in nfs4_new_open and nfs4_preprocess_stateid_op
vs. being freed in close_delegation.

That said, it looks like get_file on the *filpp nfs4_preprocess_stateid_op
is setting (on both legs of the is_delegation_stateid() condition)
should happen inside nfs4_preprocess_stateid_op rather while it holds
a ref count on the delegation (and under the lock proposed above)
rather than by its callers, nfsd4_read and nfsd4_write.

Benny

> --b.
> 
>> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
>> ---
>>  fs/nfsd/nfs4state.c |   27 +++++++++++++--------------
>>  1 files changed, 13 insertions(+), 14 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 411226a..053fd2b 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -203,29 +203,17 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_stateid *stp, struct svc_f
>>  	return dp;
>>  }
>>  
>> -void
>> -nfs4_put_delegation(struct nfs4_delegation *dp)
>> -{
>> -	if (atomic_dec_and_test(&dp->dl_count)) {
>> -		dprintk("NFSD: freeing dp %p\n",dp);
>> -		put_nfs4_file(dp->dl_file);
>> -		kmem_cache_free(deleg_slab, dp);
>> -		num_delegations--;
>> -	}
>> -}
>> -
>>  /* Remove the associated file_lock first, then remove the delegation.
>>   * lease_modify() is called to remove the FS_LEASE file_lock from
>>   * the i_flock list, eventually calling nfsd's lock_manager
>>   * fl_release_callback.
>>   */
>>  static void
>> -nfs4_close_delegation(struct nfs4_delegation *dp)
>> +__close_delegation(struct nfs4_delegation *dp)
>>  {
>>  	struct file *filp = dp->dl_vfs_file;
>>  
>>  	dprintk("NFSD: close_delegation dp %p\n",dp);
>> -	dp->dl_vfs_file = NULL;
>>  	/* The following nfsd_close may not actually close the file,
>>  	 * but we want to remove the lease in any case. */
>>  	if (dp->dl_flock)
>> @@ -233,6 +221,18 @@ nfs4_close_delegation(struct nfs4_delegation *dp)
>>  	nfsd_close(filp);
>>  }
>>  
>> +void
>> +nfs4_put_delegation(struct nfs4_delegation *dp)
>> +{
>> +	if (atomic_dec_and_test(&dp->dl_count)) {
>> +		dprintk("NFSD: freeing dp %p\n",dp);
>> +		__close_delegation(dp);
>> +		put_nfs4_file(dp->dl_file);
>> +		kmem_cache_free(deleg_slab, dp);
>> +		num_delegations--;
>> +	}
>> +}
>> +
>>  /* Called under the state lock. */
>>  static void
>>  unhash_delegation(struct nfs4_delegation *dp)
>> @@ -242,7 +242,6 @@ unhash_delegation(struct nfs4_delegation *dp)
>>  	list_del_init(&dp->dl_perclnt);
>>  	list_del_init(&dp->dl_recall_lru);
>>  	spin_unlock(&deleg_lock);
>> -	nfs4_close_delegation(dp);
>>  	nfs4_put_delegation(dp);
>>  }
>>  
>> -- 
>> 1.6.5.1
>>

[RFC 11/11] nfsd: refactor unhash_delegation

[Benny Halevy]

and use in expire_client

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

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 053fd2b..01d79cb 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -233,14 +233,20 @@ nfs4_put_delegation(struct nfs4_delegation *dp)
 	}
 }
 
-/* Called under the state lock. */
-static void
-unhash_delegation(struct nfs4_delegation *dp)
+static inline void
+__unhash_delegation_locked(struct nfs4_delegation *dp)
 {
-	spin_lock(&deleg_lock);
 	list_del_init(&dp->dl_perfile);
 	list_del_init(&dp->dl_perclnt);
 	list_del_init(&dp->dl_recall_lru);
+}
+
+/* Called under the state lock. */
+static inline void
+unhash_delegation(struct nfs4_delegation *dp)
+{
+	spin_lock(&deleg_lock);
+	__unhash_delegation_locked(dp);
 	spin_unlock(&deleg_lock);
 	nfs4_put_delegation(dp);
 }
@@ -769,14 +775,14 @@ expire_client(struct nfs4_client *clp)
 		dp = list_entry(clp->cl_delegations.next, struct nfs4_delegation, dl_perclnt);
 		dprintk("NFSD: expire client. dp %p, fp %p\n", dp,
 				dp->dl_flock);
-		list_del_init(&dp->dl_perclnt);
-		list_move(&dp->dl_recall_lru, &reaplist);
+		__unhash_delegation_locked(dp);
+		list_add(&dp->dl_recall_lru, &reaplist);
 	}
 	spin_unlock(&deleg_lock);
 	while (!list_empty(&reaplist)) {
 		dp = list_entry(reaplist.next, struct nfs4_delegation, dl_recall_lru);
 		list_del_init(&dp->dl_recall_lru);
-		unhash_delegation(dp);
+		nfs4_put_delegation(dp);
 	}
 	list_del(&clp->cl_idhash);
 	list_del(&clp->cl_strhash);
-- 
1.6.5.1