2010-01-27 02:43:34

by Alexandros Batsakis

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

Signed-off-by: Alexandros Batsakis <[email protected]>
---
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



2010-01-27 02:43:34

by Alexandros Batsakis

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

Signed-off-by: Alexandros Batsakis <[email protected]>
---
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


2010-01-27 02:43:35

by Alexandros Batsakis

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

Signed-off-by: Alexandros Batsakis <[email protected]>
---
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


2010-01-27 02:43:34

by Alexandros Batsakis

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

Signed-off-by: Alexandros Batsakis <[email protected]>
---
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


2010-01-28 20:48:05

by Myklebust, Trond

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

On Tue, 2010-01-26 at 18:43 -0800, Alexandros Batsakis wrote:
> Signed-off-by: Alexandros Batsakis <[email protected]>
> ---
> 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;
> }



2010-01-28 20:48:30

by Myklebust, Trond

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

On Tue, 2010-01-26 at 18:43 -0800, Alexandros Batsakis wrote:
> Signed-off-by: Alexandros Batsakis <[email protected]>
> ---
> 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.