[PATCH 1/4] nfs: kill renewd before clearing the client session

[PATCH 1/4] nfs: kill renewd before clearing the client session

[Alexandros Batsakis]

Signed-off-by: Alexandros Batsakis <batsakis@netapp.com>
---
 fs/nfs/client.c |   47 +++++++++++++++++++++++------------------------
 1 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index d0b060a..07e4b56 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -164,30 +164,6 @@ error_0:
 	return ERR_PTR(err);
 }
 
-static void nfs4_shutdown_client(struct nfs_client *clp)
-{
-#ifdef CONFIG_NFS_V4
-	if (__test_and_clear_bit(NFS_CS_RENEWD, &clp->cl_res_state))
-		nfs4_kill_renewd(clp);
-	BUG_ON(!RB_EMPTY_ROOT(&clp->cl_state_owners));
-	if (__test_and_clear_bit(NFS_CS_IDMAP, &clp->cl_res_state))
-		nfs_idmap_delete(clp);
-
-	rpc_destroy_wait_queue(&clp->cl_rpcwaitq);
-#endif
-}
-
-/*
- * Destroy the NFS4 callback service
- */
-static void nfs4_destroy_callback(struct nfs_client *clp)
-{
-#ifdef CONFIG_NFS_V4
-	if (__test_and_clear_bit(NFS_CS_CALLBACK, &clp->cl_res_state))
-		nfs_callback_down(clp->cl_minorversion);
-#endif /* CONFIG_NFS_V4 */
-}
-
 /*
  * Clears/puts all minor version specific parts from an nfs_client struct
  * reverting it to minorversion 0.
@@ -202,9 +178,31 @@ static void nfs4_clear_client_minor_version(struct nfs_client *clp)
 
 	clp->cl_call_sync = _nfs4_call_sync;
 #endif /* CONFIG_NFS_V4_1 */
+}
+
+/*
+ * Destroy the NFS4 callback service
+ */
+static void nfs4_destroy_callback(struct nfs_client *clp)
+{
+#ifdef CONFIG_NFS_V4
+	if (__test_and_clear_bit(NFS_CS_CALLBACK, &clp->cl_res_state))
+		nfs_callback_down(clp->cl_minorversion);
+#endif /* CONFIG_NFS_V4 */
+}
 
+static void nfs4_shutdown_client(struct nfs_client *clp)
+{
+#ifdef CONFIG_NFS_V4
+	if (__test_and_clear_bit(NFS_CS_RENEWD, &clp->cl_res_state))
+		nfs4_kill_renewd(clp);
+	nfs4_clear_client_minor_version(clp);
 	nfs4_destroy_callback(clp);
+	if (__test_and_clear_bit(NFS_CS_IDMAP, &clp->cl_res_state))
+		nfs_idmap_delete(clp);
+
+	rpc_destroy_wait_queue(&clp->cl_rpcwaitq);
+#endif
 }
 
 /*
-- 
1.6.2.5

[PATCH 2/4] nfs4: prevent backlogging of renewd requests

[Alexandros Batsakis]

Signed-off-by: Alexandros Batsakis <batsakis@netapp.com>
---
 fs/nfs/nfs4proc.c   |    3 +++
 fs/nfs/nfs4renewd.c |   13 -------------
 2 files changed, 3 insertions(+), 13 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 3644c35..9fc99e9 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3162,6 +3162,7 @@ static void nfs4_renew_done(struct rpc_task *task, void *data)
 	if (time_before(clp->cl_last_renewal,timestamp))
 		clp->cl_last_renewal = timestamp;
 	spin_unlock(&clp->cl_lock);
+	nfs4_schedule_state_renewal(clp);
 }
 
 static const struct rpc_call_ops nfs4_renew_ops = {
@@ -5049,6 +5050,8 @@ void nfs41_sequence_call_done(struct rpc_task *task, void *data)
 	}
 	dprintk("%s rpc_cred %p\n", __func__, task->tk_msg.rpc_cred);
 
+	nfs4_schedule_state_renewal(clp);
+
 	kfree(task->tk_msg.rpc_argp);
 	kfree(task->tk_msg.rpc_resp);
 
diff --git a/fs/nfs/nfs4renewd.c b/fs/nfs/nfs4renewd.c
index 0156c01..f514902 100644
--- a/fs/nfs/nfs4renewd.c
+++ b/fs/nfs/nfs4renewd.c
@@ -36,11 +36,6 @@
  * as an rpc_task, not a real kernel thread, so it always runs in rpciod's
  * context.  There is one renewd per nfs_server.
  *
- * TODO: If the send queue gets backlogged (e.g., if the server goes down),
- * we will keep filling the queue with periodic RENEW requests.  We need a
- * mechanism for ensuring that if renewd successfully sends off a request,
- * then it only wakes up when the request is finished.  Maybe use the
- * child task framework of the RPC layer?
  */
 
 #include <linux/mm.h>
@@ -92,17 +87,9 @@ nfs4_renew_state(struct work_struct *work)
 			put_rpccred(cred);
 		}
 		timeout = (2 * lease) / 3;
-		spin_lock(&clp->cl_lock);
 	} else
 		dprintk("%s: failed to call renewd. Reason: lease not expired \n",
 				__func__);
-	if (timeout < 5 * HZ)    /* safeguard */
-		timeout = 5 * HZ;
-	dprintk("%s: requeueing work. Lease period = %ld\n",
-			__func__, (timeout + HZ - 1) / HZ);
-	cancel_delayed_work(&clp->cl_renewd);
-	schedule_delayed_work(&clp->cl_renewd, timeout);
-	spin_unlock(&clp->cl_lock);
 	nfs_expire_unreferenced_delegations(clp);
 out:
 	dprintk("%s: done\n", __func__);
-- 
1.6.2.5

[PATCH 3/4] nfs41: fix race between umount and renewd sequence operations

[Alexandros Batsakis]

Signed-off-by: Alexandros Batsakis <batsakis@netapp.com>
---
 fs/nfs/nfs4proc.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 9fc99e9..cd523df 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5041,6 +5041,8 @@ void nfs41_sequence_call_done(struct rpc_task *task, void *data)
 
 	if (task->tk_status < 0) {
 		dprintk("%s ERROR %d\n", __func__, task->tk_status);
+		if (atomic_read(&clp->cl_count) == 1)
+			return;
 
 		if (_nfs4_async_handle_error(task, NULL, clp, NULL)
 								== -EAGAIN) {
@@ -5050,7 +5052,8 @@ void nfs41_sequence_call_done(struct rpc_task *task, void *data)
 	}
 	dprintk("%s rpc_cred %p\n", __func__, task->tk_msg.rpc_cred);
 
-	nfs4_schedule_state_renewal(clp);
+	if (atomic_read(&clp->cl_count) > 1)
+		nfs4_schedule_state_renewal(clp);
 
 	kfree(task->tk_msg.rpc_argp);
 	kfree(task->tk_msg.rpc_resp);
@@ -5073,9 +5076,15 @@ static void nfs41_sequence_prepare(struct rpc_task *task, void *data)
 	rpc_call_start(task);
 }
 
+static void nfs41_sequence_release(void *calldata)
+{
+	nfs_put_client((struct nfs_client *) calldata);
+}
+
 static const struct rpc_call_ops nfs41_sequence_ops = {
 	.rpc_call_done = nfs41_sequence_call_done,
 	.rpc_call_prepare = nfs41_sequence_prepare,
+	.rpc_release = nfs41_sequence_release,
 };
 
 static int nfs41_proc_async_sequence(struct nfs_client *clp,
@@ -5088,11 +5097,16 @@ static int nfs41_proc_async_sequence(struct nfs_client *clp,
 		.rpc_cred = cred,
 	};
 
+	if (!atomic_inc_not_zero(&clp->cl_count))
+		return -EIO;
 	args = kzalloc(sizeof(*args), GFP_KERNEL);
-	if (!args)
+	if (!args) {
+		nfs_put_client(clp);
 		return -ENOMEM;
+	}
 	res = kzalloc(sizeof(*res), GFP_KERNEL);
 	if (!res) {
+		nfs_put_client(clp);
 		kfree(args);
 		return -ENOMEM;
 	}
-- 
1.6.2.5

[PATCH 4/4] nfs4: fix race between umount and renew operations

[Alexandros Batsakis]

Signed-off-by: Alexandros Batsakis <batsakis@netapp.com>
---
 fs/nfs/nfs4proc.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index cd523df..cc8f059 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3162,11 +3162,18 @@ static void nfs4_renew_done(struct rpc_task *task, void *data)
 	if (time_before(clp->cl_last_renewal,timestamp))
 		clp->cl_last_renewal = timestamp;
 	spin_unlock(&clp->cl_lock);
-	nfs4_schedule_state_renewal(clp);
+	if (atomic_read(clp->cl_count) > 1)
+		nfs4_schedule_state_renewal(clp);
+}
+
+static void nfs4_renew_release(void *calldata)
+{
+	nfs_put_client((struct nfs_client *) calldata);
 }
 
 static const struct rpc_call_ops nfs4_renew_ops = {
 	.rpc_call_done = nfs4_renew_done,
+	.rpc_release = nfs4_renew_release,
 };
 
 int nfs4_proc_async_renew(struct nfs_client *clp, struct rpc_cred *cred)
@@ -3177,6 +3184,9 @@ int nfs4_proc_async_renew(struct nfs_client *clp, struct rpc_cred *cred)
 		.rpc_cred	= cred,
 	};
 
+	if (!atomic_inc_not_zero(clp->cl_count))
+		return -EIO;
+
 	return rpc_call_async(clp->cl_rpcclient, &msg, RPC_TASK_SOFT,
 			&nfs4_renew_ops, (void *)jiffies);
 }
-- 
1.6.2.5

Re: [PATCH 3/4] nfs41: fix race between umount and renewd sequence operations

[Trond Myklebust]

On Tue, 2010-01-26 at 18:43 -0800, Alexandros Batsakis wrote: 
> Signed-off-by: Alexandros Batsakis <batsakis@netapp.com>
> ---
>  fs/nfs/nfs4proc.c |   18 ++++++++++++++++--
>  1 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 9fc99e9..cd523df 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -5041,6 +5041,8 @@ void nfs41_sequence_call_done(struct rpc_task *task, void *data)
>  
>  	if (task->tk_status < 0) {
>  		dprintk("%s ERROR %d\n", __func__, task->tk_status);
> +		if (atomic_read(&clp->cl_count) == 1)
> +			return;
>  
>  		if (_nfs4_async_handle_error(task, NULL, clp, NULL)
>  								== -EAGAIN) {
> @@ -5050,7 +5052,8 @@ void nfs41_sequence_call_done(struct rpc_task *task, void *data)
>  	}
>  	dprintk("%s rpc_cred %p\n", __func__, task->tk_msg.rpc_cred);
>  
> -	nfs4_schedule_state_renewal(clp);
> +	if (atomic_read(&clp->cl_count) > 1)

Why not just have a single
         if (atomic_read(&clp->cl_count) == 1)
return;

above

> +		nfs4_schedule_state_renewal(clp);
>  
>  	kfree(task->tk_msg.rpc_argp);
>  	kfree(task->tk_msg.rpc_resp);

These belong in the 'sequence_release' function regardless. Otherwise
you have a memory leak if rpc_call_async() failed, and also if your test
for clp->cl_count == 1 in the error case above.

> @@ -5073,9 +5076,15 @@ static void nfs41_sequence_prepare(struct rpc_task *task, void *data)
>  	rpc_call_start(task);
>  }
>  
> +static void nfs41_sequence_release(void *calldata)
> +{
> +	nfs_put_client((struct nfs_client *) calldata);
> +}
> +
>  static const struct rpc_call_ops nfs41_sequence_ops = {
>  	.rpc_call_done = nfs41_sequence_call_done,
>  	.rpc_call_prepare = nfs41_sequence_prepare,
> +	.rpc_release = nfs41_sequence_release,
>  };
>  
>  static int nfs41_proc_async_sequence(struct nfs_client *clp,
> @@ -5088,11 +5097,16 @@ static int nfs41_proc_async_sequence(struct nfs_client *clp,
>  		.rpc_cred = cred,
>  	};
>  
> +	if (!atomic_inc_not_zero(&clp->cl_count))
> +		return -EIO;
>  	args = kzalloc(sizeof(*args), GFP_KERNEL);
> -	if (!args)
> +	if (!args) {
> +		nfs_put_client(clp);
>  		return -ENOMEM;
> +	}
>  	res = kzalloc(sizeof(*res), GFP_KERNEL);
>  	if (!res) {
> +		nfs_put_client(clp);
>  		kfree(args);
>  		return -ENOMEM;
>  	}

Re: [PATCH 4/4] nfs4: fix race between umount and renew operations

[Trond Myklebust]

On Tue, 2010-01-26 at 18:43 -0800, Alexandros Batsakis wrote: 
> Signed-off-by: Alexandros Batsakis <batsakis@netapp.com>
> ---
>  fs/nfs/nfs4proc.c |   12 +++++++++++-
>  1 files changed, 11 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index cd523df..cc8f059 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -3162,11 +3162,18 @@ static void nfs4_renew_done(struct rpc_task *task, void *data)
>  	if (time_before(clp->cl_last_renewal,timestamp))
>  		clp->cl_last_renewal = timestamp;
>  	spin_unlock(&clp->cl_lock);
> -	nfs4_schedule_state_renewal(clp);
> +	if (atomic_read(clp->cl_count) > 1)
> +		nfs4_schedule_state_renewal(clp);
> +}
> +
> +static void nfs4_renew_release(void *calldata)
> +{
> +	nfs_put_client((struct nfs_client *) calldata);
>  }
>  
>  static const struct rpc_call_ops nfs4_renew_ops = {
>  	.rpc_call_done = nfs4_renew_done,
> +	.rpc_release = nfs4_renew_release,
>  };
>  
>  int nfs4_proc_async_renew(struct nfs_client *clp, struct rpc_cred *cred)
> @@ -3177,6 +3184,9 @@ int nfs4_proc_async_renew(struct nfs_client *clp, struct rpc_cred *cred)
>  		.rpc_cred	= cred,
>  	};
>  
> +	if (!atomic_inc_not_zero(clp->cl_count))
> +		return -EIO;
> +
>  	return rpc_call_async(clp->cl_rpcclient, &msg, RPC_TASK_SOFT,
>  			&nfs4_renew_ops, (void *)jiffies);
>  }

Thanks! This looks good.