[PATCH 1/2] NFS: Add sequence_priviliged_ops for nfs4_proc_sequence()

[PATCH 1/2] NFS: Add sequence_priviliged_ops for nfs4_proc_sequence()

[bjschuma]

From: Bryan Schumaker <bjschuma@netapp.com>

During recovery the NFS4_SESSION_DRAINING flag might be set on the
client structure.  This can cause lease renewal to abort when
nfs41_setup_sequence sees that we are doing recovery.  As a result, the
client never recovers and all activity with the NFS server halts.

Signed-off-by: Bryan Schumaker <bjschuma@netapp.com>
---
 fs/nfs/nfs4proc.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 5eec442..537181c 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6138,13 +6138,26 @@ static void nfs41_sequence_prepare(struct rpc_task *task, void *data)
 	rpc_call_start(task);
 }
 
+static void nfs41_sequence_prepare_privileged(struct rpc_task *task, void *data)
+{
+	rpc_task_set_priority(task, RPC_PRIORITY_PRIVILEGED);
+	nfs41_sequence_prepare(task, data);
+}
+
 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 struct rpc_task *_nfs41_proc_sequence(struct nfs_client *clp, struct rpc_cred *cred)
+static const struct rpc_call_ops nfs41_sequence_privileged_ops = {
+	.rpc_call_done = nfs41_sequence_call_done,
+	.rpc_call_prepare = nfs41_sequence_prepare_privileged,
+	.rpc_release = nfs41_sequence_release,
+};
+
+static struct rpc_task *_nfs41_proc_sequence(struct nfs_client *clp, struct rpc_cred *cred,
+					     const struct rpc_call_ops *seq_ops)
 {
 	struct nfs4_sequence_data *calldata;
 	struct rpc_message msg = {
@@ -6154,7 +6167,7 @@ static struct rpc_task *_nfs41_proc_sequence(struct nfs_client *clp, struct rpc_
 	struct rpc_task_setup task_setup_data = {
 		.rpc_client = clp->cl_rpcclient,
 		.rpc_message = &msg,
-		.callback_ops = &nfs41_sequence_ops,
+		.callback_ops = seq_ops,
 		.flags = RPC_TASK_ASYNC | RPC_TASK_SOFT,
 	};
 
@@ -6181,7 +6194,7 @@ static int nfs41_proc_async_sequence(struct nfs_client *clp, struct rpc_cred *cr
 
 	if ((renew_flags & NFS4_RENEW_TIMEOUT) == 0)
 		return 0;
-	task = _nfs41_proc_sequence(clp, cred);
+	task = _nfs41_proc_sequence(clp, cred, &nfs41_sequence_ops);
 	if (IS_ERR(task))
 		ret = PTR_ERR(task);
 	else
@@ -6195,7 +6208,7 @@ static int nfs4_proc_sequence(struct nfs_client *clp, struct rpc_cred *cred)
 	struct rpc_task *task;
 	int ret;
 
-	task = _nfs41_proc_sequence(clp, cred);
+	task = _nfs41_proc_sequence(clp, cred, &nfs41_sequence_privileged_ops);
 	if (IS_ERR(task)) {
 		ret = PTR_ERR(task);
 		goto out;
-- 
1.8.0

[PATCH 2/2] NFS: Don't set RPC_TASK_ASYNC for nfs4_proc_sequence()

[bjschuma]

From: Bryan Schumaker <bjschuma@netapp.com>

This should simplify the code a bit since the rpc layer will handle the
task properly, allowing us to remove the rpc_wait_for_completion_task()
call.

Signed-off-by: Bryan Schumaker <bjschuma@netapp.com>
---
 fs/nfs/nfs4proc.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 537181c..a2efcae 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6157,7 +6157,7 @@ static const struct rpc_call_ops nfs41_sequence_privileged_ops = {
 };
 
 static struct rpc_task *_nfs41_proc_sequence(struct nfs_client *clp, struct rpc_cred *cred,
-					     const struct rpc_call_ops *seq_ops)
+					     const struct rpc_call_ops *seq_ops, int flags)
 {
 	struct nfs4_sequence_data *calldata;
 	struct rpc_message msg = {
@@ -6168,7 +6168,7 @@ static struct rpc_task *_nfs41_proc_sequence(struct nfs_client *clp, struct rpc_
 		.rpc_client = clp->cl_rpcclient,
 		.rpc_message = &msg,
 		.callback_ops = seq_ops,
-		.flags = RPC_TASK_ASYNC | RPC_TASK_SOFT,
+		.flags = RPC_TASK_SOFT | flags,
 	};
 
 	if (!atomic_inc_not_zero(&clp->cl_count))
@@ -6194,7 +6194,7 @@ static int nfs41_proc_async_sequence(struct nfs_client *clp, struct rpc_cred *cr
 
 	if ((renew_flags & NFS4_RENEW_TIMEOUT) == 0)
 		return 0;
-	task = _nfs41_proc_sequence(clp, cred, &nfs41_sequence_ops);
+	task = _nfs41_proc_sequence(clp, cred, &nfs41_sequence_ops, RPC_TASK_ASYNC);
 	if (IS_ERR(task))
 		ret = PTR_ERR(task);
 	else
@@ -6208,19 +6208,16 @@ static int nfs4_proc_sequence(struct nfs_client *clp, struct rpc_cred *cred)
 	struct rpc_task *task;
 	int ret;
 
-	task = _nfs41_proc_sequence(clp, cred, &nfs41_sequence_privileged_ops);
+	task = _nfs41_proc_sequence(clp, cred, &nfs41_sequence_privileged_ops, 0);
 	if (IS_ERR(task)) {
 		ret = PTR_ERR(task);
 		goto out;
 	}
-	ret = rpc_wait_for_completion_task(task);
-	if (!ret) {
+	if (task->tk_status == 0) {
 		struct nfs4_sequence_res *res = task->tk_msg.rpc_resp;
-
-		if (task->tk_status == 0)
-			nfs41_handle_sequence_flag_errors(clp, res->sr_status_flags);
-		ret = task->tk_status;
+		nfs41_handle_sequence_flag_errors(clp, res->sr_status_flags);
 	}
+	ret = task->tk_status;
 	rpc_put_task(task);
 out:
 	dprintk("<-- %s status=%d\n", __func__, ret);
-- 
1.8.0

Re: [PATCH 1/2] NFS: Add sequence_priviliged_ops for nfs4_proc_sequence()

[Andy Adamson]

On Mon, Nov 12, 2012 at 1:35 PM,  <bjschuma@netapp.com> wrote:
> From: Bryan Schumaker <bjschuma@netapp.com>
>
> During recovery the NFS4_SESSION_DRAINING flag might be set on the
> client structure.  This can cause lease renewal to abort when
> nfs41_setup_sequence sees that we are doing recovery.  As a result, the
> client never recovers and all activity with the NFS server halts.


When does this happen? Say the session is draining, and an RPC returns
from one of the compounds such as nfs_open, nfs_locku etc whose
rpc_call_done routine calls renew_lease after freeing it's slot.  Like
all calls on a draining session, the renew_lease sleeps on the session
slot_tbl_waitq - is this what you mean by "causes lease renewal to
abort"? How does this cause the client to never recover?

The only other call to renew_lease is from the state manager itself
which runs one function at a time, and will not call renew_lease until
the recovery is over....

What am I missing....?
-->Andy


>
> Signed-off-by: Bryan Schumaker <bjschuma@netapp.com>
> ---
>  fs/nfs/nfs4proc.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 5eec442..537181c 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -6138,13 +6138,26 @@ static void nfs41_sequence_prepare(struct rpc_task *task, void *data)
>         rpc_call_start(task);
>  }
>
> +static void nfs41_sequence_prepare_privileged(struct rpc_task *task, void *data)
> +{
> +       rpc_task_set_priority(task, RPC_PRIORITY_PRIVILEGED);
> +       nfs41_sequence_prepare(task, data);
> +}
> +
>  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 struct rpc_task *_nfs41_proc_sequence(struct nfs_client *clp, struct rpc_cred *cred)
> +static const struct rpc_call_ops nfs41_sequence_privileged_ops = {
> +       .rpc_call_done = nfs41_sequence_call_done,
> +       .rpc_call_prepare = nfs41_sequence_prepare_privileged,
> +       .rpc_release = nfs41_sequence_release,
> +};
> +
> +static struct rpc_task *_nfs41_proc_sequence(struct nfs_client *clp, struct rpc_cred *cred,
> +                                            const struct rpc_call_ops *seq_ops)
>  {
>         struct nfs4_sequence_data *calldata;
>         struct rpc_message msg = {
> @@ -6154,7 +6167,7 @@ static struct rpc_task *_nfs41_proc_sequence(struct nfs_client *clp, struct rpc_
>         struct rpc_task_setup task_setup_data = {
>                 .rpc_client = clp->cl_rpcclient,
>                 .rpc_message = &msg,
> -               .callback_ops = &nfs41_sequence_ops,
> +               .callback_ops = seq_ops,
>                 .flags = RPC_TASK_ASYNC | RPC_TASK_SOFT,
>         };
>
> @@ -6181,7 +6194,7 @@ static int nfs41_proc_async_sequence(struct nfs_client *clp, struct rpc_cred *cr
>
>         if ((renew_flags & NFS4_RENEW_TIMEOUT) == 0)
>                 return 0;
> -       task = _nfs41_proc_sequence(clp, cred);
> +       task = _nfs41_proc_sequence(clp, cred, &nfs41_sequence_ops);
>         if (IS_ERR(task))
>                 ret = PTR_ERR(task);
>         else
> @@ -6195,7 +6208,7 @@ static int nfs4_proc_sequence(struct nfs_client *clp, struct rpc_cred *cred)
>         struct rpc_task *task;
>         int ret;
>
> -       task = _nfs41_proc_sequence(clp, cred);
> +       task = _nfs41_proc_sequence(clp, cred, &nfs41_sequence_privileged_ops);
>         if (IS_ERR(task)) {
>                 ret = PTR_ERR(task);
>                 goto out;
> --
> 1.8.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] NFS: Add sequence_priviliged_ops for nfs4_proc_sequence()

[Bryan Schumaker]

On 11/12/2012 03:22 PM, Andy Adamson wrote:
> On Mon, Nov 12, 2012 at 1:35 PM,  <bjschuma@netapp.com> wrote:
>> From: Bryan Schumaker <bjschuma@netapp.com>
>>
>> During recovery the NFS4_SESSION_DRAINING flag might be set on the
>> client structure.  This can cause lease renewal to abort when
>> nfs41_setup_sequence sees that we are doing recovery.  As a result, the
>> client never recovers and all activity with the NFS server halts.
> 
> 
> When does this happen? Say the session is draining, and an RPC returns
> from one of the compounds such as nfs_open, nfs_locku etc whose
> rpc_call_done routine calls renew_lease after freeing it's slot.  Like
> all calls on a draining session, the renew_lease sleeps on the session
> slot_tbl_waitq - is this what you mean by "causes lease renewal to
> abort"? How does this cause the client to never recover?

I'm not sure exactly what causes it, but I was able to hit this fairly reliably when mounting a server to 4 or 5 mountpoints on a client and then running xfstests against each mountpoint.  At some point during the tests the client gets a cb_path_down sequence error, tries to recover but hangs after the bind connection to session.

> 
> The only other call to renew_lease is from the state manager itself
> which runs one function at a time, and will not call renew_lease until
> the recovery is over....

It's this call that isn't completing.  nfs4_begin_drain_session() was called as part of bind connection to session, but there was never a call to nfs4_end_drain_session() by the time renew_lease was called so the client doesn't know that recovery is over.

> 
> What am I missing....?
> -->Andy
> 
> 
>>
>> Signed-off-by: Bryan Schumaker <bjschuma@netapp.com>
>> ---
>>  fs/nfs/nfs4proc.c | 21 +++++++++++++++++----
>>  1 file changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index 5eec442..537181c 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -6138,13 +6138,26 @@ static void nfs41_sequence_prepare(struct rpc_task *task, void *data)
>>         rpc_call_start(task);
>>  }
>>
>> +static void nfs41_sequence_prepare_privileged(struct rpc_task *task, void *data)
>> +{
>> +       rpc_task_set_priority(task, RPC_PRIORITY_PRIVILEGED);
>> +       nfs41_sequence_prepare(task, data);
>> +}
>> +
>>  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 struct rpc_task *_nfs41_proc_sequence(struct nfs_client *clp, struct rpc_cred *cred)
>> +static const struct rpc_call_ops nfs41_sequence_privileged_ops = {
>> +       .rpc_call_done = nfs41_sequence_call_done,
>> +       .rpc_call_prepare = nfs41_sequence_prepare_privileged,
>> +       .rpc_release = nfs41_sequence_release,
>> +};
>> +
>> +static struct rpc_task *_nfs41_proc_sequence(struct nfs_client *clp, struct rpc_cred *cred,
>> +                                            const struct rpc_call_ops *seq_ops)
>>  {
>>         struct nfs4_sequence_data *calldata;
>>         struct rpc_message msg = {
>> @@ -6154,7 +6167,7 @@ static struct rpc_task *_nfs41_proc_sequence(struct nfs_client *clp, struct rpc_
>>         struct rpc_task_setup task_setup_data = {
>>                 .rpc_client = clp->cl_rpcclient,
>>                 .rpc_message = &msg,
>> -               .callback_ops = &nfs41_sequence_ops,
>> +               .callback_ops = seq_ops,
>>                 .flags = RPC_TASK_ASYNC | RPC_TASK_SOFT,
>>         };
>>
>> @@ -6181,7 +6194,7 @@ static int nfs41_proc_async_sequence(struct nfs_client *clp, struct rpc_cred *cr
>>
>>         if ((renew_flags & NFS4_RENEW_TIMEOUT) == 0)
>>                 return 0;
>> -       task = _nfs41_proc_sequence(clp, cred);
>> +       task = _nfs41_proc_sequence(clp, cred, &nfs41_sequence_ops);
>>         if (IS_ERR(task))
>>                 ret = PTR_ERR(task);
>>         else
>> @@ -6195,7 +6208,7 @@ static int nfs4_proc_sequence(struct nfs_client *clp, struct rpc_cred *cred)
>>         struct rpc_task *task;
>>         int ret;
>>
>> -       task = _nfs41_proc_sequence(clp, cred);
>> +       task = _nfs41_proc_sequence(clp, cred, &nfs41_sequence_privileged_ops);
>>         if (IS_ERR(task)) {
>>                 ret = PTR_ERR(task);
>>                 goto out;
>> --
>> 1.8.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] NFS: Add sequence_priviliged_ops for nfs4_proc_sequence()

[Myklebust, Trond]

T24gTW9uLCAyMDEyLTExLTEyIGF0IDE1OjIyIC0wNTAwLCBBbmR5IEFkYW1zb24gd3JvdGU6DQo+
IE9uIE1vbiwgTm92IDEyLCAyMDEyIGF0IDE6MzUgUE0sICA8YmpzY2h1bWFAbmV0YXBwLmNvbT4g
d3JvdGU6DQo+ID4gRnJvbTogQnJ5YW4gU2NodW1ha2VyIDxianNjaHVtYUBuZXRhcHAuY29tPg0K
PiA+DQo+ID4gRHVyaW5nIHJlY292ZXJ5IHRoZSBORlM0X1NFU1NJT05fRFJBSU5JTkcgZmxhZyBt
aWdodCBiZSBzZXQgb24gdGhlDQo+ID4gY2xpZW50IHN0cnVjdHVyZS4gIFRoaXMgY2FuIGNhdXNl
IGxlYXNlIHJlbmV3YWwgdG8gYWJvcnQgd2hlbg0KDQpOb3QgbGVhc2UgcmVuZXdhbC4gSXQgaXMg
bGVhc2UgdmVyaWZpY2F0aW9uIChpLmUuIGNoZWNraW5nIHRoYXQgd2UgaGF2ZQ0KYSB2YWxpZCBs
ZWFzZSBhbmQgc2Vzc2lvbikgdGhhdCB3aWxsIGRlYWRsb2NrLg0KDQo+ID4gbmZzNDFfc2V0dXBf
c2VxdWVuY2Ugc2VlcyB0aGF0IHdlIGFyZSBkb2luZyByZWNvdmVyeS4gIEFzIGEgcmVzdWx0LCB0
aGUNCj4gPiBjbGllbnQgbmV2ZXIgcmVjb3ZlcnMgYW5kIGFsbCBhY3Rpdml0eSB3aXRoIHRoZSBO
RlMgc2VydmVyIGhhbHRzLg0KPiANCj4gDQo+IFdoZW4gZG9lcyB0aGlzIGhhcHBlbj8gU2F5IHRo
ZSBzZXNzaW9uIGlzIGRyYWluaW5nLCBhbmQgYW4gUlBDIHJldHVybnMNCj4gZnJvbSBvbmUgb2Yg
dGhlIGNvbXBvdW5kcyBzdWNoIGFzIG5mc19vcGVuLCBuZnNfbG9ja3UgZXRjIHdob3NlDQo+IHJw
Y19jYWxsX2RvbmUgcm91dGluZSBjYWxscyByZW5ld19sZWFzZSBhZnRlciBmcmVlaW5nIGl0J3Mg
c2xvdC4gIExpa2UNCj4gYWxsIGNhbGxzIG9uIGEgZHJhaW5pbmcgc2Vzc2lvbiwgdGhlIHJlbmV3
X2xlYXNlIHNsZWVwcyBvbiB0aGUgc2Vzc2lvbg0KPiBzbG90X3RibF93YWl0cSAtIGlzIHRoaXMg
d2hhdCB5b3UgbWVhbiBieSAiY2F1c2VzIGxlYXNlIHJlbmV3YWwgdG8NCj4gYWJvcnQiPyBIb3cg
ZG9lcyB0aGlzIGNhdXNlIHRoZSBjbGllbnQgdG8gbmV2ZXIgcmVjb3Zlcj8NCj4gDQo+IFRoZSBv
bmx5IG90aGVyIGNhbGwgdG8gcmVuZXdfbGVhc2UgaXMgZnJvbSB0aGUgc3RhdGUgbWFuYWdlciBp
dHNlbGYNCj4gd2hpY2ggcnVucyBvbmUgZnVuY3Rpb24gYXQgYSB0aW1lLCBhbmQgd2lsbCBub3Qg
Y2FsbCByZW5ld19sZWFzZSB1bnRpbA0KPiB0aGUgcmVjb3ZlcnkgaXMgb3Zlci4uLi4NCj4gDQo+
IFdoYXQgYW0gSSBtaXNzaW5nLi4uLj8NCg0KbmZzNF9jaGVja19sZWFzZSgpIGlzIHJ1biBleGNs
dXNpdmVseSBieSB0aGUgc3RhdGUgbWFuYWdlciB0aHJlYWQgaW4NCm9yZGVyIHRvIGNoZWNrIHRo
YXQgdGhlIGNsaWVudGlkIChhbmQgc2Vzc2lvbiBpZCBvbiBORlN2NC4xKSBhcmUgdmFsaWQuDQpJ
dCB3aWxsIGRlYWRsb2NrIHRoZSBzdGF0ZSBtYW5hZ2VyIHRocmVhZCBpZiBORlM0X1NFU1NJT05f
RFJBSU5JTkcgaXMNCmFscmVhZHkgc2V0Lg0KDQo+IC0tPkFuZHkNCj4gDQo+IA0KPiA+DQo+ID4g
U2lnbmVkLW9mZi1ieTogQnJ5YW4gU2NodW1ha2VyIDxianNjaHVtYUBuZXRhcHAuY29tPg0KPiA+
IC0tLQ0KPiA+ICBmcy9uZnMvbmZzNHByb2MuYyB8IDIxICsrKysrKysrKysrKysrKysrLS0tLQ0K
PiA+ICAxIGZpbGUgY2hhbmdlZCwgMTcgaW5zZXJ0aW9ucygrKSwgNCBkZWxldGlvbnMoLSkNCj4g
Pg0KPiA+IGRpZmYgLS1naXQgYS9mcy9uZnMvbmZzNHByb2MuYyBiL2ZzL25mcy9uZnM0cHJvYy5j
DQo+ID4gaW5kZXggNWVlYzQ0Mi4uNTM3MTgxYyAxMDA2NDQNCj4gPiAtLS0gYS9mcy9uZnMvbmZz
NHByb2MuYw0KPiA+ICsrKyBiL2ZzL25mcy9uZnM0cHJvYy5jDQo+ID4gQEAgLTYxMzgsMTMgKzYx
MzgsMjYgQEAgc3RhdGljIHZvaWQgbmZzNDFfc2VxdWVuY2VfcHJlcGFyZShzdHJ1Y3QgcnBjX3Rh
c2sgKnRhc2ssIHZvaWQgKmRhdGEpDQo+ID4gICAgICAgICBycGNfY2FsbF9zdGFydCh0YXNrKTsN
Cj4gPiAgfQ0KPiA+DQo+ID4gK3N0YXRpYyB2b2lkIG5mczQxX3NlcXVlbmNlX3ByZXBhcmVfcHJp
dmlsZWdlZChzdHJ1Y3QgcnBjX3Rhc2sgKnRhc2ssIHZvaWQgKmRhdGEpDQo+ID4gK3sNCj4gPiAr
ICAgICAgIHJwY190YXNrX3NldF9wcmlvcml0eSh0YXNrLCBSUENfUFJJT1JJVFlfUFJJVklMRUdF
RCk7DQo+ID4gKyAgICAgICBuZnM0MV9zZXF1ZW5jZV9wcmVwYXJlKHRhc2ssIGRhdGEpOw0KPiA+
ICt9DQo+ID4gKw0KPiA+ICBzdGF0aWMgY29uc3Qgc3RydWN0IHJwY19jYWxsX29wcyBuZnM0MV9z
ZXF1ZW5jZV9vcHMgPSB7DQo+ID4gICAgICAgICAucnBjX2NhbGxfZG9uZSA9IG5mczQxX3NlcXVl
bmNlX2NhbGxfZG9uZSwNCj4gPiAgICAgICAgIC5ycGNfY2FsbF9wcmVwYXJlID0gbmZzNDFfc2Vx
dWVuY2VfcHJlcGFyZSwNCj4gPiAgICAgICAgIC5ycGNfcmVsZWFzZSA9IG5mczQxX3NlcXVlbmNl
X3JlbGVhc2UsDQo+ID4gIH07DQo+ID4NCj4gPiAtc3RhdGljIHN0cnVjdCBycGNfdGFzayAqX25m
czQxX3Byb2Nfc2VxdWVuY2Uoc3RydWN0IG5mc19jbGllbnQgKmNscCwgc3RydWN0IHJwY19jcmVk
ICpjcmVkKQ0KPiA+ICtzdGF0aWMgY29uc3Qgc3RydWN0IHJwY19jYWxsX29wcyBuZnM0MV9zZXF1
ZW5jZV9wcml2aWxlZ2VkX29wcyA9IHsNCj4gPiArICAgICAgIC5ycGNfY2FsbF9kb25lID0gbmZz
NDFfc2VxdWVuY2VfY2FsbF9kb25lLA0KPiA+ICsgICAgICAgLnJwY19jYWxsX3ByZXBhcmUgPSBu
ZnM0MV9zZXF1ZW5jZV9wcmVwYXJlX3ByaXZpbGVnZWQsDQo+ID4gKyAgICAgICAucnBjX3JlbGVh
c2UgPSBuZnM0MV9zZXF1ZW5jZV9yZWxlYXNlLA0KPiA+ICt9Ow0KPiA+ICsNCj4gPiArc3RhdGlj
IHN0cnVjdCBycGNfdGFzayAqX25mczQxX3Byb2Nfc2VxdWVuY2Uoc3RydWN0IG5mc19jbGllbnQg
KmNscCwgc3RydWN0IHJwY19jcmVkICpjcmVkLA0KPiA+ICsgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgIGNvbnN0IHN0cnVjdCBycGNfY2FsbF9vcHMgKnNlcV9vcHMp
DQo+ID4gIHsNCj4gPiAgICAgICAgIHN0cnVjdCBuZnM0X3NlcXVlbmNlX2RhdGEgKmNhbGxkYXRh
Ow0KPiA+ICAgICAgICAgc3RydWN0IHJwY19tZXNzYWdlIG1zZyA9IHsNCj4gPiBAQCAtNjE1NCw3
ICs2MTY3LDcgQEAgc3RhdGljIHN0cnVjdCBycGNfdGFzayAqX25mczQxX3Byb2Nfc2VxdWVuY2Uo
c3RydWN0IG5mc19jbGllbnQgKmNscCwgc3RydWN0IHJwY18NCj4gPiAgICAgICAgIHN0cnVjdCBy
cGNfdGFza19zZXR1cCB0YXNrX3NldHVwX2RhdGEgPSB7DQo+ID4gICAgICAgICAgICAgICAgIC5y
cGNfY2xpZW50ID0gY2xwLT5jbF9ycGNjbGllbnQsDQo+ID4gICAgICAgICAgICAgICAgIC5ycGNf
bWVzc2FnZSA9ICZtc2csDQo+ID4gLSAgICAgICAgICAgICAgIC5jYWxsYmFja19vcHMgPSAmbmZz
NDFfc2VxdWVuY2Vfb3BzLA0KPiA+ICsgICAgICAgICAgICAgICAuY2FsbGJhY2tfb3BzID0gc2Vx
X29wcywNCj4gPiAgICAgICAgICAgICAgICAgLmZsYWdzID0gUlBDX1RBU0tfQVNZTkMgfCBSUENf
VEFTS19TT0ZULA0KPiA+ICAgICAgICAgfTsNCj4gPg0KPiA+IEBAIC02MTgxLDcgKzYxOTQsNyBA
QCBzdGF0aWMgaW50IG5mczQxX3Byb2NfYXN5bmNfc2VxdWVuY2Uoc3RydWN0IG5mc19jbGllbnQg
KmNscCwgc3RydWN0IHJwY19jcmVkICpjcg0KPiA+DQo+ID4gICAgICAgICBpZiAoKHJlbmV3X2Zs
YWdzICYgTkZTNF9SRU5FV19USU1FT1VUKSA9PSAwKQ0KPiA+ICAgICAgICAgICAgICAgICByZXR1
cm4gMDsNCj4gPiAtICAgICAgIHRhc2sgPSBfbmZzNDFfcHJvY19zZXF1ZW5jZShjbHAsIGNyZWQp
Ow0KPiA+ICsgICAgICAgdGFzayA9IF9uZnM0MV9wcm9jX3NlcXVlbmNlKGNscCwgY3JlZCwgJm5m
czQxX3NlcXVlbmNlX29wcyk7DQo+ID4gICAgICAgICBpZiAoSVNfRVJSKHRhc2spKQ0KPiA+ICAg
ICAgICAgICAgICAgICByZXQgPSBQVFJfRVJSKHRhc2spOw0KPiA+ICAgICAgICAgZWxzZQ0KPiA+
IEBAIC02MTk1LDcgKzYyMDgsNyBAQCBzdGF0aWMgaW50IG5mczRfcHJvY19zZXF1ZW5jZShzdHJ1
Y3QgbmZzX2NsaWVudCAqY2xwLCBzdHJ1Y3QgcnBjX2NyZWQgKmNyZWQpDQo+ID4gICAgICAgICBz
dHJ1Y3QgcnBjX3Rhc2sgKnRhc2s7DQo+ID4gICAgICAgICBpbnQgcmV0Ow0KPiA+DQo+ID4gLSAg
ICAgICB0YXNrID0gX25mczQxX3Byb2Nfc2VxdWVuY2UoY2xwLCBjcmVkKTsNCj4gPiArICAgICAg
IHRhc2sgPSBfbmZzNDFfcHJvY19zZXF1ZW5jZShjbHAsIGNyZWQsICZuZnM0MV9zZXF1ZW5jZV9w
cml2aWxlZ2VkX29wcyk7DQo+ID4gICAgICAgICBpZiAoSVNfRVJSKHRhc2spKSB7DQo+ID4gICAg
ICAgICAgICAgICAgIHJldCA9IFBUUl9FUlIodGFzayk7DQo+ID4gICAgICAgICAgICAgICAgIGdv
dG8gb3V0Ow0KPiA+IC0tDQo+ID4gMS44LjANCj4gPg0KPiA+IC0tDQo+ID4gVG8gdW5zdWJzY3Jp
YmUgZnJvbSB0aGlzIGxpc3Q6IHNlbmQgdGhlIGxpbmUgInVuc3Vic2NyaWJlIGxpbnV4LW5mcyIg
aW4NCj4gPiB0aGUgYm9keSBvZiBhIG1lc3NhZ2UgdG8gbWFqb3Jkb21vQHZnZXIua2VybmVsLm9y
Zw0KPiA+IE1vcmUgbWFqb3Jkb21vIGluZm8gYXQgIGh0dHA6Ly92Z2VyLmtlcm5lbC5vcmcvbWFq
b3Jkb21vLWluZm8uaHRtbA0KDQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVu
dCBtYWludGFpbmVyDQoNCk5ldEFwcA0KVHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5u
ZXRhcHAuY29tDQo=

Re: [PATCH 1/2] NFS: Add sequence_priviliged_ops for nfs4_proc_sequence()

[Andy Adamson]

On Mon, Nov 12, 2012 at 3:29 PM, Myklebust, Trond
<Trond.Myklebust@netapp.com> wrote:
> On Mon, 2012-11-12 at 15:22 -0500, Andy Adamson wrote:
>> On Mon, Nov 12, 2012 at 1:35 PM,  <bjschuma@netapp.com> wrote:
>> > From: Bryan Schumaker <bjschuma@netapp.com>
>> >
>> > During recovery the NFS4_SESSION_DRAINING flag might be set on the
>> > client structure.  This can cause lease renewal to abort when
>
> Not lease renewal. It is lease verification (i.e. checking that we have
> a valid lease and session) that will deadlock.
>
>> > nfs41_setup_sequence sees that we are doing recovery.  As a result, the
>> > client never recovers and all activity with the NFS server halts.
>>
>>
>> When does this happen? Say the session is draining, and an RPC returns
>> from one of the compounds such as nfs_open, nfs_locku etc whose
>> rpc_call_done routine calls renew_lease after freeing it's slot.  Like
>> all calls on a draining session, the renew_lease sleeps on the session
>> slot_tbl_waitq - is this what you mean by "causes lease renewal to
>> abort"? How does this cause the client to never recover?
>>
>> The only other call to renew_lease is from the state manager itself
>> which runs one function at a time, and will not call renew_lease until
>> the recovery is over....
>>
>> What am I missing....?
>
> nfs4_check_lease() is run exclusively by the state manager thread in
> order to check that the clientid (and session id on NFSv4.1) are valid.
> It will deadlock the state manager thread if NFS4_SESSION_DRAINING is
> already set.

OK. NFS4_SESSION_DRAINING is also set exclusively by the state manager
thread. Why is the state manager told to check the lease when it's
also draining a session?

No matter what the answer, please update the patch description to
better explain the problem being solved.

-->Andy

>
>> -->Andy
>>
>>
>> >
>> > Signed-off-by: Bryan Schumaker <bjschuma@netapp.com>
>> > ---
>> >  fs/nfs/nfs4proc.c | 21 +++++++++++++++++----
>> >  1 file changed, 17 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> > index 5eec442..537181c 100644
>> > --- a/fs/nfs/nfs4proc.c
>> > +++ b/fs/nfs/nfs4proc.c
>> > @@ -6138,13 +6138,26 @@ static void nfs41_sequence_prepare(struct rpc_task *task, void *data)
>> >         rpc_call_start(task);
>> >  }
>> >
>> > +static void nfs41_sequence_prepare_privileged(struct rpc_task *task, void *data)
>> > +{
>> > +       rpc_task_set_priority(task, RPC_PRIORITY_PRIVILEGED);
>> > +       nfs41_sequence_prepare(task, data);
>> > +}
>> > +
>> >  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 struct rpc_task *_nfs41_proc_sequence(struct nfs_client *clp, struct rpc_cred *cred)
>> > +static const struct rpc_call_ops nfs41_sequence_privileged_ops = {
>> > +       .rpc_call_done = nfs41_sequence_call_done,
>> > +       .rpc_call_prepare = nfs41_sequence_prepare_privileged,
>> > +       .rpc_release = nfs41_sequence_release,
>> > +};
>> > +
>> > +static struct rpc_task *_nfs41_proc_sequence(struct nfs_client *clp, struct rpc_cred *cred,
>> > +                                            const struct rpc_call_ops *seq_ops)
>> >  {
>> >         struct nfs4_sequence_data *calldata;
>> >         struct rpc_message msg = {
>> > @@ -6154,7 +6167,7 @@ static struct rpc_task *_nfs41_proc_sequence(struct nfs_client *clp, struct rpc_
>> >         struct rpc_task_setup task_setup_data = {
>> >                 .rpc_client = clp->cl_rpcclient,
>> >                 .rpc_message = &msg,
>> > -               .callback_ops = &nfs41_sequence_ops,
>> > +               .callback_ops = seq_ops,
>> >                 .flags = RPC_TASK_ASYNC | RPC_TASK_SOFT,
>> >         };
>> >
>> > @@ -6181,7 +6194,7 @@ static int nfs41_proc_async_sequence(struct nfs_client *clp, struct rpc_cred *cr
>> >
>> >         if ((renew_flags & NFS4_RENEW_TIMEOUT) == 0)
>> >                 return 0;
>> > -       task = _nfs41_proc_sequence(clp, cred);
>> > +       task = _nfs41_proc_sequence(clp, cred, &nfs41_sequence_ops);
>> >         if (IS_ERR(task))
>> >                 ret = PTR_ERR(task);
>> >         else
>> > @@ -6195,7 +6208,7 @@ static int nfs4_proc_sequence(struct nfs_client *clp, struct rpc_cred *cred)
>> >         struct rpc_task *task;
>> >         int ret;
>> >
>> > -       task = _nfs41_proc_sequence(clp, cred);
>> > +       task = _nfs41_proc_sequence(clp, cred, &nfs41_sequence_privileged_ops);
>> >         if (IS_ERR(task)) {
>> >                 ret = PTR_ERR(task);
>> >                 goto out;
>> > --
>> > 1.8.0
>> >
>> > --
>> > 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
>
> --
> Trond Myklebust
> Linux NFS client maintainer
>
> NetApp
> Trond.Myklebust@netapp.com
> www.netapp.com

Re: [PATCH 1/2] NFS: Add sequence_priviliged_ops for nfs4_proc_sequence()

[Bryan Schumaker]

On 11/12/2012 03:49 PM, Andy Adamson wrote:
> On Mon, Nov 12, 2012 at 3:29 PM, Myklebust, Trond
> <Trond.Myklebust@netapp.com> wrote:
>> On Mon, 2012-11-12 at 15:22 -0500, Andy Adamson wrote:
>>> On Mon, Nov 12, 2012 at 1:35 PM,  <bjschuma@netapp.com> wrote:
>>>> From: Bryan Schumaker <bjschuma@netapp.com>
>>>>
>>>> During recovery the NFS4_SESSION_DRAINING flag might be set on the
>>>> client structure.  This can cause lease renewal to abort when
>>
>> Not lease renewal. It is lease verification (i.e. checking that we have
>> a valid lease and session) that will deadlock.
>>
>>>> nfs41_setup_sequence sees that we are doing recovery.  As a result, the
>>>> client never recovers and all activity with the NFS server halts.
>>>
>>>
>>> When does this happen? Say the session is draining, and an RPC returns
>>> from one of the compounds such as nfs_open, nfs_locku etc whose
>>> rpc_call_done routine calls renew_lease after freeing it's slot.  Like
>>> all calls on a draining session, the renew_lease sleeps on the session
>>> slot_tbl_waitq - is this what you mean by "causes lease renewal to
>>> abort"? How does this cause the client to never recover?
>>>
>>> The only other call to renew_lease is from the state manager itself
>>> which runs one function at a time, and will not call renew_lease until
>>> the recovery is over....
>>>
>>> What am I missing....?
>>
>> nfs4_check_lease() is run exclusively by the state manager thread in
>> order to check that the clientid (and session id on NFSv4.1) are valid.
>> It will deadlock the state manager thread if NFS4_SESSION_DRAINING is
>> already set.
> 
> OK. NFS4_SESSION_DRAINING is also set exclusively by the state manager
> thread. Why is the state manager told to check the lease when it's
> also draining a session?
> 
> No matter what the answer, please update the patch description to
> better explain the problem being solved.

Yeah, I was just thinking about doing that.  Give me a few minutes...

- Bryan

> 
> -->Andy
> 
>>
>>> -->Andy
>>>
>>>
>>>>
>>>> Signed-off-by: Bryan Schumaker <bjschuma@netapp.com>
>>>> ---
>>>>  fs/nfs/nfs4proc.c | 21 +++++++++++++++++----
>>>>  1 file changed, 17 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>>> index 5eec442..537181c 100644
>>>> --- a/fs/nfs/nfs4proc.c
>>>> +++ b/fs/nfs/nfs4proc.c
>>>> @@ -6138,13 +6138,26 @@ static void nfs41_sequence_prepare(struct rpc_task *task, void *data)
>>>>         rpc_call_start(task);
>>>>  }
>>>>
>>>> +static void nfs41_sequence_prepare_privileged(struct rpc_task *task, void *data)
>>>> +{
>>>> +       rpc_task_set_priority(task, RPC_PRIORITY_PRIVILEGED);
>>>> +       nfs41_sequence_prepare(task, data);
>>>> +}
>>>> +
>>>>  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 struct rpc_task *_nfs41_proc_sequence(struct nfs_client *clp, struct rpc_cred *cred)
>>>> +static const struct rpc_call_ops nfs41_sequence_privileged_ops = {
>>>> +       .rpc_call_done = nfs41_sequence_call_done,
>>>> +       .rpc_call_prepare = nfs41_sequence_prepare_privileged,
>>>> +       .rpc_release = nfs41_sequence_release,
>>>> +};
>>>> +
>>>> +static struct rpc_task *_nfs41_proc_sequence(struct nfs_client *clp, struct rpc_cred *cred,
>>>> +                                            const struct rpc_call_ops *seq_ops)
>>>>  {
>>>>         struct nfs4_sequence_data *calldata;
>>>>         struct rpc_message msg = {
>>>> @@ -6154,7 +6167,7 @@ static struct rpc_task *_nfs41_proc_sequence(struct nfs_client *clp, struct rpc_
>>>>         struct rpc_task_setup task_setup_data = {
>>>>                 .rpc_client = clp->cl_rpcclient,
>>>>                 .rpc_message = &msg,
>>>> -               .callback_ops = &nfs41_sequence_ops,
>>>> +               .callback_ops = seq_ops,
>>>>                 .flags = RPC_TASK_ASYNC | RPC_TASK_SOFT,
>>>>         };
>>>>
>>>> @@ -6181,7 +6194,7 @@ static int nfs41_proc_async_sequence(struct nfs_client *clp, struct rpc_cred *cr
>>>>
>>>>         if ((renew_flags & NFS4_RENEW_TIMEOUT) == 0)
>>>>                 return 0;
>>>> -       task = _nfs41_proc_sequence(clp, cred);
>>>> +       task = _nfs41_proc_sequence(clp, cred, &nfs41_sequence_ops);
>>>>         if (IS_ERR(task))
>>>>                 ret = PTR_ERR(task);
>>>>         else
>>>> @@ -6195,7 +6208,7 @@ static int nfs4_proc_sequence(struct nfs_client *clp, struct rpc_cred *cred)
>>>>         struct rpc_task *task;
>>>>         int ret;
>>>>
>>>> -       task = _nfs41_proc_sequence(clp, cred);
>>>> +       task = _nfs41_proc_sequence(clp, cred, &nfs41_sequence_privileged_ops);
>>>>         if (IS_ERR(task)) {
>>>>                 ret = PTR_ERR(task);
>>>>                 goto out;
>>>> --
>>>> 1.8.0
>>>>
>>>> --
>>>> 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
>>
>> --
>> Trond Myklebust
>> Linux NFS client maintainer
>>
>> NetApp
>> Trond.Myklebust@netapp.com
>> www.netapp.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/2] NFS: Add sequence_priviliged_ops for nfs4_proc_sequence()

[Andy Adamson]

On Mon, Nov 12, 2012 at 3:51 PM, Bryan Schumaker <bjschuma@netapp.com> wrote:
> On 11/12/2012 03:49 PM, Andy Adamson wrote:
>> On Mon, Nov 12, 2012 at 3:29 PM, Myklebust, Trond
>> <Trond.Myklebust@netapp.com> wrote:
>>> On Mon, 2012-11-12 at 15:22 -0500, Andy Adamson wrote:
>>>> On Mon, Nov 12, 2012 at 1:35 PM,  <bjschuma@netapp.com> wrote:
>>>>> From: Bryan Schumaker <bjschuma@netapp.com>
>>>>>
>>>>> During recovery the NFS4_SESSION_DRAINING flag might be set on the
>>>>> client structure.  This can cause lease renewal to abort when
>>>
>>> Not lease renewal. It is lease verification (i.e. checking that we have
>>> a valid lease and session) that will deadlock.
>>>
>>>>> nfs41_setup_sequence sees that we are doing recovery.  As a result, the
>>>>> client never recovers and all activity with the NFS server halts.
>>>>
>>>>
>>>> When does this happen? Say the session is draining, and an RPC returns
>>>> from one of the compounds such as nfs_open, nfs_locku etc whose
>>>> rpc_call_done routine calls renew_lease after freeing it's slot.  Like
>>>> all calls on a draining session, the renew_lease sleeps on the session
>>>> slot_tbl_waitq - is this what you mean by "causes lease renewal to
>>>> abort"? How does this cause the client to never recover?
>>>>
>>>> The only other call to renew_lease is from the state manager itself
>>>> which runs one function at a time, and will not call renew_lease until
>>>> the recovery is over....
>>>>
>>>> What am I missing....?
>>>
>>> nfs4_check_lease() is run exclusively by the state manager thread in
>>> order to check that the clientid (and session id on NFSv4.1) are valid.
>>> It will deadlock the state manager thread if NFS4_SESSION_DRAINING is
>>> already set.
>>
>> OK. NFS4_SESSION_DRAINING is also set exclusively by the state manager
>> thread. Why is the state manager told to check the lease when it's
>> also draining a session?

Here is the call in the state manager.

                if (test_and_clear_bit(NFS4CLNT_BIND_CONN_TO_SESSION,
                                &clp->cl_state)) {
                        section = "bind conn to session";
                        status = nfs4_bind_conn_to_session(clp);
                        if (status < 0)
                                goto out_error;
                        continue;
                }

nfs4_bind_conn_to_session calls nfs4_begin_drain_session. The error
case is fine - nfs4_end_drain_session is called in out_error.

But the continue when nfs4_bind_conn_to_session succeeds can send the
state manager to process other flags (such as NFS4CLNT_CHECK_LEASE)
without a call to nfs4_end_drain_session.

That looks like a bug to me. The same can occur with
NFS4CLNT_RECALL_SLOT. Why not just call nfs4_end_drain_session prior
to the continue, or perhaps remove the continue and hit the
nfs4_end_drain_session call further down in the state manager loop?

-->Andy

>>
>> No matter what the answer, please update the patch description to
>> better explain the problem being solved.
>
> Yeah, I was just thinking about doing that.  Give me a few minutes...


>
> - Bryan
>
>>
>> -->Andy
>>
>>>
>>>> -->Andy
>>>>
>>>>
>>>>>
>>>>> Signed-off-by: Bryan Schumaker <bjschuma@netapp.com>
>>>>> ---
>>>>>  fs/nfs/nfs4proc.c | 21 +++++++++++++++++----
>>>>>  1 file changed, 17 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>>>> index 5eec442..537181c 100644
>>>>> --- a/fs/nfs/nfs4proc.c
>>>>> +++ b/fs/nfs/nfs4proc.c
>>>>> @@ -6138,13 +6138,26 @@ static void nfs41_sequence_prepare(struct rpc_task *task, void *data)
>>>>>         rpc_call_start(task);
>>>>>  }
>>>>>
>>>>> +static void nfs41_sequence_prepare_privileged(struct rpc_task *task, void *data)
>>>>> +{
>>>>> +       rpc_task_set_priority(task, RPC_PRIORITY_PRIVILEGED);
>>>>> +       nfs41_sequence_prepare(task, data);
>>>>> +}
>>>>> +
>>>>>  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 struct rpc_task *_nfs41_proc_sequence(struct nfs_client *clp, struct rpc_cred *cred)
>>>>> +static const struct rpc_call_ops nfs41_sequence_privileged_ops = {
>>>>> +       .rpc_call_done = nfs41_sequence_call_done,
>>>>> +       .rpc_call_prepare = nfs41_sequence_prepare_privileged,
>>>>> +       .rpc_release = nfs41_sequence_release,
>>>>> +};
>>>>> +
>>>>> +static struct rpc_task *_nfs41_proc_sequence(struct nfs_client *clp, struct rpc_cred *cred,
>>>>> +                                            const struct rpc_call_ops *seq_ops)
>>>>>  {
>>>>>         struct nfs4_sequence_data *calldata;
>>>>>         struct rpc_message msg = {
>>>>> @@ -6154,7 +6167,7 @@ static struct rpc_task *_nfs41_proc_sequence(struct nfs_client *clp, struct rpc_
>>>>>         struct rpc_task_setup task_setup_data = {
>>>>>                 .rpc_client = clp->cl_rpcclient,
>>>>>                 .rpc_message = &msg,
>>>>> -               .callback_ops = &nfs41_sequence_ops,
>>>>> +               .callback_ops = seq_ops,
>>>>>                 .flags = RPC_TASK_ASYNC | RPC_TASK_SOFT,
>>>>>         };
>>>>>
>>>>> @@ -6181,7 +6194,7 @@ static int nfs41_proc_async_sequence(struct nfs_client *clp, struct rpc_cred *cr
>>>>>
>>>>>         if ((renew_flags & NFS4_RENEW_TIMEOUT) == 0)
>>>>>                 return 0;
>>>>> -       task = _nfs41_proc_sequence(clp, cred);
>>>>> +       task = _nfs41_proc_sequence(clp, cred, &nfs41_sequence_ops);
>>>>>         if (IS_ERR(task))
>>>>>                 ret = PTR_ERR(task);
>>>>>         else
>>>>> @@ -6195,7 +6208,7 @@ static int nfs4_proc_sequence(struct nfs_client *clp, struct rpc_cred *cred)
>>>>>         struct rpc_task *task;
>>>>>         int ret;
>>>>>
>>>>> -       task = _nfs41_proc_sequence(clp, cred);
>>>>> +       task = _nfs41_proc_sequence(clp, cred, &nfs41_sequence_privileged_ops);
>>>>>         if (IS_ERR(task)) {
>>>>>                 ret = PTR_ERR(task);
>>>>>                 goto out;
>>>>> --
>>>>> 1.8.0
>>>>>
>>>>> --
>>>>> 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
>>>
>>> --
>>> Trond Myklebust
>>> Linux NFS client maintainer
>>>
>>> NetApp
>>> Trond.Myklebust@netapp.com
>>> www.netapp.com
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>

Re: [PATCH 1/2] NFS: Add sequence_priviliged_ops for nfs4_proc_sequence()

[Myklebust, Trond]

T24gTW9uLCAyMDEyLTExLTEyIGF0IDE2OjA4IC0wNTAwLCBBbmR5IEFkYW1zb24gd3JvdGU6DQo+
IE9uIE1vbiwgTm92IDEyLCAyMDEyIGF0IDM6NTEgUE0sIEJyeWFuIFNjaHVtYWtlciA8YmpzY2h1
bWFAbmV0YXBwLmNvbT4gd3JvdGU6DQo+ID4gT24gMTEvMTIvMjAxMiAwMzo0OSBQTSwgQW5keSBB
ZGFtc29uIHdyb3RlOg0KPiA+PiBPbiBNb24sIE5vdiAxMiwgMjAxMiBhdCAzOjI5IFBNLCBNeWts
ZWJ1c3QsIFRyb25kDQo+ID4+IDxUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbT4gd3JvdGU6DQo+
ID4+PiBPbiBNb24sIDIwMTItMTEtMTIgYXQgMTU6MjIgLTA1MDAsIEFuZHkgQWRhbXNvbiB3cm90
ZToNCj4gPj4+PiBPbiBNb24sIE5vdiAxMiwgMjAxMiBhdCAxOjM1IFBNLCAgPGJqc2NodW1hQG5l
dGFwcC5jb20+IHdyb3RlOg0KPiA+Pj4+PiBGcm9tOiBCcnlhbiBTY2h1bWFrZXIgPGJqc2NodW1h
QG5ldGFwcC5jb20+DQo+ID4+Pj4+DQo+ID4+Pj4+IER1cmluZyByZWNvdmVyeSB0aGUgTkZTNF9T
RVNTSU9OX0RSQUlOSU5HIGZsYWcgbWlnaHQgYmUgc2V0IG9uIHRoZQ0KPiA+Pj4+PiBjbGllbnQg
c3RydWN0dXJlLiAgVGhpcyBjYW4gY2F1c2UgbGVhc2UgcmVuZXdhbCB0byBhYm9ydCB3aGVuDQo+
ID4+Pg0KPiA+Pj4gTm90IGxlYXNlIHJlbmV3YWwuIEl0IGlzIGxlYXNlIHZlcmlmaWNhdGlvbiAo
aS5lLiBjaGVja2luZyB0aGF0IHdlIGhhdmUNCj4gPj4+IGEgdmFsaWQgbGVhc2UgYW5kIHNlc3Np
b24pIHRoYXQgd2lsbCBkZWFkbG9jay4NCj4gPj4+DQo+ID4+Pj4+IG5mczQxX3NldHVwX3NlcXVl
bmNlIHNlZXMgdGhhdCB3ZSBhcmUgZG9pbmcgcmVjb3ZlcnkuICBBcyBhIHJlc3VsdCwgdGhlDQo+
ID4+Pj4+IGNsaWVudCBuZXZlciByZWNvdmVycyBhbmQgYWxsIGFjdGl2aXR5IHdpdGggdGhlIE5G
UyBzZXJ2ZXIgaGFsdHMuDQo+ID4+Pj4NCj4gPj4+Pg0KPiA+Pj4+IFdoZW4gZG9lcyB0aGlzIGhh
cHBlbj8gU2F5IHRoZSBzZXNzaW9uIGlzIGRyYWluaW5nLCBhbmQgYW4gUlBDIHJldHVybnMNCj4g
Pj4+PiBmcm9tIG9uZSBvZiB0aGUgY29tcG91bmRzIHN1Y2ggYXMgbmZzX29wZW4sIG5mc19sb2Nr
dSBldGMgd2hvc2UNCj4gPj4+PiBycGNfY2FsbF9kb25lIHJvdXRpbmUgY2FsbHMgcmVuZXdfbGVh
c2UgYWZ0ZXIgZnJlZWluZyBpdCdzIHNsb3QuICBMaWtlDQo+ID4+Pj4gYWxsIGNhbGxzIG9uIGEg
ZHJhaW5pbmcgc2Vzc2lvbiwgdGhlIHJlbmV3X2xlYXNlIHNsZWVwcyBvbiB0aGUgc2Vzc2lvbg0K
PiA+Pj4+IHNsb3RfdGJsX3dhaXRxIC0gaXMgdGhpcyB3aGF0IHlvdSBtZWFuIGJ5ICJjYXVzZXMg
bGVhc2UgcmVuZXdhbCB0bw0KPiA+Pj4+IGFib3J0Ij8gSG93IGRvZXMgdGhpcyBjYXVzZSB0aGUg
Y2xpZW50IHRvIG5ldmVyIHJlY292ZXI/DQo+ID4+Pj4NCj4gPj4+PiBUaGUgb25seSBvdGhlciBj
YWxsIHRvIHJlbmV3X2xlYXNlIGlzIGZyb20gdGhlIHN0YXRlIG1hbmFnZXIgaXRzZWxmDQo+ID4+
Pj4gd2hpY2ggcnVucyBvbmUgZnVuY3Rpb24gYXQgYSB0aW1lLCBhbmQgd2lsbCBub3QgY2FsbCBy
ZW5ld19sZWFzZSB1bnRpbA0KPiA+Pj4+IHRoZSByZWNvdmVyeSBpcyBvdmVyLi4uLg0KPiA+Pj4+
DQo+ID4+Pj4gV2hhdCBhbSBJIG1pc3NpbmcuLi4uPw0KPiA+Pj4NCj4gPj4+IG5mczRfY2hlY2tf
bGVhc2UoKSBpcyBydW4gZXhjbHVzaXZlbHkgYnkgdGhlIHN0YXRlIG1hbmFnZXIgdGhyZWFkIGlu
DQo+ID4+PiBvcmRlciB0byBjaGVjayB0aGF0IHRoZSBjbGllbnRpZCAoYW5kIHNlc3Npb24gaWQg
b24gTkZTdjQuMSkgYXJlIHZhbGlkLg0KPiA+Pj4gSXQgd2lsbCBkZWFkbG9jayB0aGUgc3RhdGUg
bWFuYWdlciB0aHJlYWQgaWYgTkZTNF9TRVNTSU9OX0RSQUlOSU5HIGlzDQo+ID4+PiBhbHJlYWR5
IHNldC4NCj4gPj4NCj4gPj4gT0suIE5GUzRfU0VTU0lPTl9EUkFJTklORyBpcyBhbHNvIHNldCBl
eGNsdXNpdmVseSBieSB0aGUgc3RhdGUgbWFuYWdlcg0KPiA+PiB0aHJlYWQuIFdoeSBpcyB0aGUg
c3RhdGUgbWFuYWdlciB0b2xkIHRvIGNoZWNrIHRoZSBsZWFzZSB3aGVuIGl0J3MNCj4gPj4gYWxz
byBkcmFpbmluZyBhIHNlc3Npb24/DQo+IA0KPiBIZXJlIGlzIHRoZSBjYWxsIGluIHRoZSBzdGF0
ZSBtYW5hZ2VyLg0KPiANCj4gICAgICAgICAgICAgICAgIGlmICh0ZXN0X2FuZF9jbGVhcl9iaXQo
TkZTNENMTlRfQklORF9DT05OX1RPX1NFU1NJT04sDQo+ICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgJmNscC0+Y2xfc3RhdGUpKSB7DQo+ICAgICAgICAgICAgICAgICAgICAgICAgIHNl
Y3Rpb24gPSAiYmluZCBjb25uIHRvIHNlc3Npb24iOw0KPiAgICAgICAgICAgICAgICAgICAgICAg
ICBzdGF0dXMgPSBuZnM0X2JpbmRfY29ubl90b19zZXNzaW9uKGNscCk7DQo+ICAgICAgICAgICAg
ICAgICAgICAgICAgIGlmIChzdGF0dXMgPCAwKQ0KPiAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgIGdvdG8gb3V0X2Vycm9yOw0KPiAgICAgICAgICAgICAgICAgICAgICAgICBjb250aW51
ZTsNCj4gICAgICAgICAgICAgICAgIH0NCj4gDQo+IG5mczRfYmluZF9jb25uX3RvX3Nlc3Npb24g
Y2FsbHMgbmZzNF9iZWdpbl9kcmFpbl9zZXNzaW9uLiBUaGUgZXJyb3INCj4gY2FzZSBpcyBmaW5l
IC0gbmZzNF9lbmRfZHJhaW5fc2Vzc2lvbiBpcyBjYWxsZWQgaW4gb3V0X2Vycm9yLg0KPiANCj4g
QnV0IHRoZSBjb250aW51ZSB3aGVuIG5mczRfYmluZF9jb25uX3RvX3Nlc3Npb24gc3VjY2VlZHMg
Y2FuIHNlbmQgdGhlDQo+IHN0YXRlIG1hbmFnZXIgdG8gcHJvY2VzcyBvdGhlciBmbGFncyAoc3Vj
aCBhcyBORlM0Q0xOVF9DSEVDS19MRUFTRSkNCj4gd2l0aG91dCBhIGNhbGwgdG8gbmZzNF9lbmRf
ZHJhaW5fc2Vzc2lvbi4NCj4gDQo+IFRoYXQgbG9va3MgbGlrZSBhIGJ1ZyB0byBtZS4gVGhlIHNh
bWUgY2FuIG9jY3VyIHdpdGgNCj4gTkZTNENMTlRfUkVDQUxMX1NMT1QuDQoNClNvIHdoYXQ/DQoN
Cj4gIFdoeSBub3QganVzdCBjYWxsIG5mczRfZW5kX2RyYWluX3Nlc3Npb24gcHJpb3INCj4gdG8g
dGhlIGNvbnRpbnVlLCBvciBwZXJoYXBzIHJlbW92ZSB0aGUgY29udGludWUgYW5kIGhpdCB0aGUN
Cj4gbmZzNF9lbmRfZHJhaW5fc2Vzc2lvbiBjYWxsIGZ1cnRoZXIgZG93biBpbiB0aGUgc3RhdGUg
bWFuYWdlciBsb29wPw0KDQpDYWxsaW5nIG5mczRfZW5kX2RyYWluX3Nlc3Npb24gaW4gb3JkZXIg
dG8gYWxsb3cgdW5wcml2aWxlZ2VkIG9wZXJhdGlvbnMNCnRvIHByb2NlZWQsIHdoZW4gd2UncmUg
bm90IGV2ZW4gc3VyZSB0aGF0IHdlIHN0aWxsIGhhdmUgYSBsZWFzZSwgbWFrZXMNCm5vIHNlbnNl
LiBUaGUgd2hvbGUgcG9pbnQgaGVyZSBpcyB0byBibG9jayB0aG9zZSBvcGVyYXRpb25zIHVudGls
IHRoZQ0Kc3RhdGUgbWFuYWdlciB0aHJlYWQgaGFzIHJlY292ZXJlZCB0aGUgbGVhc2UsIHNlc3Np
b24sIG9wZW4gc3RhdGVpZHMgYW5kDQpsb2NrIHN0YXRlaWRzLg0KDQotLSANClRyb25kIE15a2xl
YnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyDQoNCk5ldEFwcA0KVHJvbmQuTXlrbGVi
dXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAuY29tDQo=

Re: [PATCH 1/2] NFS: Add sequence_priviliged_ops for nfs4_proc_sequence()

[Myklebust, Trond]

T24gTW9uLCAyMDEyLTExLTEyIGF0IDE1OjQ5IC0wNTAwLCBBbmR5IEFkYW1zb24gd3JvdGU6DQo+
IE9uIE1vbiwgTm92IDEyLCAyMDEyIGF0IDM6MjkgUE0sIE15a2xlYnVzdCwgVHJvbmQNCj4gPFRy
b25kLk15a2xlYnVzdEBuZXRhcHAuY29tPiB3cm90ZToNCj4gPiBPbiBNb24sIDIwMTItMTEtMTIg
YXQgMTU6MjIgLTA1MDAsIEFuZHkgQWRhbXNvbiB3cm90ZToNCj4gPj4gT24gTW9uLCBOb3YgMTIs
IDIwMTIgYXQgMTozNSBQTSwgIDxianNjaHVtYUBuZXRhcHAuY29tPiB3cm90ZToNCj4gPj4gPiBG
cm9tOiBCcnlhbiBTY2h1bWFrZXIgPGJqc2NodW1hQG5ldGFwcC5jb20+DQo+ID4+ID4NCj4gPj4g
PiBEdXJpbmcgcmVjb3ZlcnkgdGhlIE5GUzRfU0VTU0lPTl9EUkFJTklORyBmbGFnIG1pZ2h0IGJl
IHNldCBvbiB0aGUNCj4gPj4gPiBjbGllbnQgc3RydWN0dXJlLiAgVGhpcyBjYW4gY2F1c2UgbGVh
c2UgcmVuZXdhbCB0byBhYm9ydCB3aGVuDQo+ID4NCj4gPiBOb3QgbGVhc2UgcmVuZXdhbC4gSXQg
aXMgbGVhc2UgdmVyaWZpY2F0aW9uIChpLmUuIGNoZWNraW5nIHRoYXQgd2UgaGF2ZQ0KPiA+IGEg
dmFsaWQgbGVhc2UgYW5kIHNlc3Npb24pIHRoYXQgd2lsbCBkZWFkbG9jay4NCj4gPg0KPiA+PiA+
IG5mczQxX3NldHVwX3NlcXVlbmNlIHNlZXMgdGhhdCB3ZSBhcmUgZG9pbmcgcmVjb3ZlcnkuICBB
cyBhIHJlc3VsdCwgdGhlDQo+ID4+ID4gY2xpZW50IG5ldmVyIHJlY292ZXJzIGFuZCBhbGwgYWN0
aXZpdHkgd2l0aCB0aGUgTkZTIHNlcnZlciBoYWx0cy4NCj4gPj4NCj4gPj4NCj4gPj4gV2hlbiBk
b2VzIHRoaXMgaGFwcGVuPyBTYXkgdGhlIHNlc3Npb24gaXMgZHJhaW5pbmcsIGFuZCBhbiBSUEMg
cmV0dXJucw0KPiA+PiBmcm9tIG9uZSBvZiB0aGUgY29tcG91bmRzIHN1Y2ggYXMgbmZzX29wZW4s
IG5mc19sb2NrdSBldGMgd2hvc2UNCj4gPj4gcnBjX2NhbGxfZG9uZSByb3V0aW5lIGNhbGxzIHJl
bmV3X2xlYXNlIGFmdGVyIGZyZWVpbmcgaXQncyBzbG90LiAgTGlrZQ0KPiA+PiBhbGwgY2FsbHMg
b24gYSBkcmFpbmluZyBzZXNzaW9uLCB0aGUgcmVuZXdfbGVhc2Ugc2xlZXBzIG9uIHRoZSBzZXNz
aW9uDQo+ID4+IHNsb3RfdGJsX3dhaXRxIC0gaXMgdGhpcyB3aGF0IHlvdSBtZWFuIGJ5ICJjYXVz
ZXMgbGVhc2UgcmVuZXdhbCB0bw0KPiA+PiBhYm9ydCI/IEhvdyBkb2VzIHRoaXMgY2F1c2UgdGhl
IGNsaWVudCB0byBuZXZlciByZWNvdmVyPw0KPiA+Pg0KPiA+PiBUaGUgb25seSBvdGhlciBjYWxs
IHRvIHJlbmV3X2xlYXNlIGlzIGZyb20gdGhlIHN0YXRlIG1hbmFnZXIgaXRzZWxmDQo+ID4+IHdo
aWNoIHJ1bnMgb25lIGZ1bmN0aW9uIGF0IGEgdGltZSwgYW5kIHdpbGwgbm90IGNhbGwgcmVuZXdf
bGVhc2UgdW50aWwNCj4gPj4gdGhlIHJlY292ZXJ5IGlzIG92ZXIuLi4uDQo+ID4+DQo+ID4+IFdo
YXQgYW0gSSBtaXNzaW5nLi4uLj8NCj4gPg0KPiA+IG5mczRfY2hlY2tfbGVhc2UoKSBpcyBydW4g
ZXhjbHVzaXZlbHkgYnkgdGhlIHN0YXRlIG1hbmFnZXIgdGhyZWFkIGluDQo+ID4gb3JkZXIgdG8g
Y2hlY2sgdGhhdCB0aGUgY2xpZW50aWQgKGFuZCBzZXNzaW9uIGlkIG9uIE5GU3Y0LjEpIGFyZSB2
YWxpZC4NCj4gPiBJdCB3aWxsIGRlYWRsb2NrIHRoZSBzdGF0ZSBtYW5hZ2VyIHRocmVhZCBpZiBO
RlM0X1NFU1NJT05fRFJBSU5JTkcgaXMNCj4gPiBhbHJlYWR5IHNldC4NCj4gDQo+IE9LLiBORlM0
X1NFU1NJT05fRFJBSU5JTkcgaXMgYWxzbyBzZXQgZXhjbHVzaXZlbHkgYnkgdGhlIHN0YXRlIG1h
bmFnZXINCj4gdGhyZWFkLiBXaHkgaXMgdGhlIHN0YXRlIG1hbmFnZXIgdG9sZCB0byBjaGVjayB0
aGUgbGVhc2Ugd2hlbiBpdCdzDQo+IGFsc28gZHJhaW5pbmcgYSBzZXNzaW9uPw0KDQpORlM0X1NF
U1NJT05fRFJBSU5JTkcgZG9lcyBub3QganVzdCBtZWFuIHRoYXQgdGhlIHNlc3Npb24gaXMgYmVp
bmcNCmRyYWluZWQ7IGl0IHJlbWFpbnMgc2V0IHVudGlsIG9wZW4gYW5kIGxvY2sgc3RhdGUgcmVj
b3ZlcnkgaXMgY29tcGxldGVkLg0KDQpJT1c6IGlmIHNvbWV0aGluZyBoYXBwZW5zIGR1cmluZyBz
dGF0ZSByZWNvdmVyeSB0aGF0IHJlcXVpcmVzIHVzIHRvDQpjaGVjayB0aGUgbGVhc2UgKGUuZy4g
c29tZXRoaW5nIHJldHVybnMgTkZTNEVSUl9FWFBJUkVEKSB0aGVuIHRoZQ0KY3VycmVudCBjb2Rl
IHdpbGwgZGVhZGxvY2suDQoNCj4gTm8gbWF0dGVyIHdoYXQgdGhlIGFuc3dlciwgcGxlYXNlIHVw
ZGF0ZSB0aGUgcGF0Y2ggZGVzY3JpcHRpb24gdG8NCj4gYmV0dGVyIGV4cGxhaW4gdGhlIHByb2Js
ZW0gYmVpbmcgc29sdmVkLg0KDQpBQ0suIEkgYWdyZWUgdGhhdCB0aGUgY2hhbmdlbG9nIGVudHJ5
IGNhbiBiZSBpbXByb3ZlZC4NCg0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGll
bnQgbWFpbnRhaW5lcg0KDQpOZXRBcHANClRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tDQp3d3cu
bmV0YXBwLmNvbQ0K

Re: [PATCH 1/2] NFS: Add sequence_priviliged_ops for nfs4_proc_sequence()

[Bryan Schumaker]

On 11/12/2012 03:54 PM, Myklebust, Trond wrote:
> On Mon, 2012-11-12 at 15:49 -0500, Andy Adamson wrote:
>> On Mon, Nov 12, 2012 at 3:29 PM, Myklebust, Trond
>> <Trond.Myklebust@netapp.com> wrote:
>>> On Mon, 2012-11-12 at 15:22 -0500, Andy Adamson wrote:
>>>> On Mon, Nov 12, 2012 at 1:35 PM,  <bjschuma@netapp.com> wrote:
>>>>> From: Bryan Schumaker <bjschuma@netapp.com>
>>>>>
>>>>> During recovery the NFS4_SESSION_DRAINING flag might be set on the
>>>>> client structure.  This can cause lease renewal to abort when
>>>
>>> Not lease renewal. It is lease verification (i.e. checking that we have
>>> a valid lease and session) that will deadlock.
>>>
>>>>> nfs41_setup_sequence sees that we are doing recovery.  As a result, the
>>>>> client never recovers and all activity with the NFS server halts.
>>>>
>>>>
>>>> When does this happen? Say the session is draining, and an RPC returns
>>>> from one of the compounds such as nfs_open, nfs_locku etc whose
>>>> rpc_call_done routine calls renew_lease after freeing it's slot.  Like
>>>> all calls on a draining session, the renew_lease sleeps on the session
>>>> slot_tbl_waitq - is this what you mean by "causes lease renewal to
>>>> abort"? How does this cause the client to never recover?
>>>>
>>>> The only other call to renew_lease is from the state manager itself
>>>> which runs one function at a time, and will not call renew_lease until
>>>> the recovery is over....
>>>>
>>>> What am I missing....?
>>>
>>> nfs4_check_lease() is run exclusively by the state manager thread in
>>> order to check that the clientid (and session id on NFSv4.1) are valid.
>>> It will deadlock the state manager thread if NFS4_SESSION_DRAINING is
>>> already set.
>>
>> OK. NFS4_SESSION_DRAINING is also set exclusively by the state manager
>> thread. Why is the state manager told to check the lease when it's
>> also draining a session?
> 
> NFS4_SESSION_DRAINING does not just mean that the session is being
> drained; it remains set until open and lock state recovery is completed.
> 
> IOW: if something happens during state recovery that requires us to
> check the lease (e.g. something returns NFS4ERR_EXPIRED) then the
> current code will deadlock.
> 
>> No matter what the answer, please update the patch description to
>> better explain the problem being solved.
> 
> ACK. I agree that the changelog entry can be improved.
> 

Does this read any better (and should I resend the patch)?

NFS: Add sequence_priviliged_ops for nfs4_proc_sequence()

If I mount an NFS v4.1 server to a single client multiple times and then
run xfstests over each mountpoint I usually get the client into a state
where recovery deadlocks.  The server informs the client of a
cb_path_down sequence error, the client then does a
bind_connection_to_session and checks the status of the lease.

I found that bind_connection_to_session sets the NFS4_SESSION_DRAINING
flag on the client, but this flag is never unset before
nfs4_check_lease() reaches nfs4_proc_sequence().  This causes the client
to deadlock, halting all NFS activity to the server.

Re: [PATCH 1/2] NFS: Add sequence_priviliged_ops for nfs4_proc_sequence()

[Myklebust, Trond]

T24gTW9uLCAyMDEyLTExLTEyIGF0IDE2OjAyIC0wNTAwLCBCcnlhbiBTY2h1bWFrZXIgd3JvdGU6
DQo+IE9uIDExLzEyLzIwMTIgMDM6NTQgUE0sIE15a2xlYnVzdCwgVHJvbmQgd3JvdGU6DQo+ID4g
T24gTW9uLCAyMDEyLTExLTEyIGF0IDE1OjQ5IC0wNTAwLCBBbmR5IEFkYW1zb24gd3JvdGU6DQo+
ID4+IE9uIE1vbiwgTm92IDEyLCAyMDEyIGF0IDM6MjkgUE0sIE15a2xlYnVzdCwgVHJvbmQNCj4g
Pj4gPFRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tPiB3cm90ZToNCj4gPj4+IE9uIE1vbiwgMjAx
Mi0xMS0xMiBhdCAxNToyMiAtMDUwMCwgQW5keSBBZGFtc29uIHdyb3RlOg0KPiA+Pj4+IE9uIE1v
biwgTm92IDEyLCAyMDEyIGF0IDE6MzUgUE0sICA8YmpzY2h1bWFAbmV0YXBwLmNvbT4gd3JvdGU6
DQo+ID4+Pj4+IEZyb206IEJyeWFuIFNjaHVtYWtlciA8YmpzY2h1bWFAbmV0YXBwLmNvbT4NCj4g
Pj4+Pj4NCj4gPj4+Pj4gRHVyaW5nIHJlY292ZXJ5IHRoZSBORlM0X1NFU1NJT05fRFJBSU5JTkcg
ZmxhZyBtaWdodCBiZSBzZXQgb24gdGhlDQo+ID4+Pj4+IGNsaWVudCBzdHJ1Y3R1cmUuICBUaGlz
IGNhbiBjYXVzZSBsZWFzZSByZW5ld2FsIHRvIGFib3J0IHdoZW4NCj4gPj4+DQo+ID4+PiBOb3Qg
bGVhc2UgcmVuZXdhbC4gSXQgaXMgbGVhc2UgdmVyaWZpY2F0aW9uIChpLmUuIGNoZWNraW5nIHRo
YXQgd2UgaGF2ZQ0KPiA+Pj4gYSB2YWxpZCBsZWFzZSBhbmQgc2Vzc2lvbikgdGhhdCB3aWxsIGRl
YWRsb2NrLg0KPiA+Pj4NCj4gPj4+Pj4gbmZzNDFfc2V0dXBfc2VxdWVuY2Ugc2VlcyB0aGF0IHdl
IGFyZSBkb2luZyByZWNvdmVyeS4gIEFzIGEgcmVzdWx0LCB0aGUNCj4gPj4+Pj4gY2xpZW50IG5l
dmVyIHJlY292ZXJzIGFuZCBhbGwgYWN0aXZpdHkgd2l0aCB0aGUgTkZTIHNlcnZlciBoYWx0cy4N
Cj4gPj4+Pg0KPiA+Pj4+DQo+ID4+Pj4gV2hlbiBkb2VzIHRoaXMgaGFwcGVuPyBTYXkgdGhlIHNl
c3Npb24gaXMgZHJhaW5pbmcsIGFuZCBhbiBSUEMgcmV0dXJucw0KPiA+Pj4+IGZyb20gb25lIG9m
IHRoZSBjb21wb3VuZHMgc3VjaCBhcyBuZnNfb3BlbiwgbmZzX2xvY2t1IGV0YyB3aG9zZQ0KPiA+
Pj4+IHJwY19jYWxsX2RvbmUgcm91dGluZSBjYWxscyByZW5ld19sZWFzZSBhZnRlciBmcmVlaW5n
IGl0J3Mgc2xvdC4gIExpa2UNCj4gPj4+PiBhbGwgY2FsbHMgb24gYSBkcmFpbmluZyBzZXNzaW9u
LCB0aGUgcmVuZXdfbGVhc2Ugc2xlZXBzIG9uIHRoZSBzZXNzaW9uDQo+ID4+Pj4gc2xvdF90Ymxf
d2FpdHEgLSBpcyB0aGlzIHdoYXQgeW91IG1lYW4gYnkgImNhdXNlcyBsZWFzZSByZW5ld2FsIHRv
DQo+ID4+Pj4gYWJvcnQiPyBIb3cgZG9lcyB0aGlzIGNhdXNlIHRoZSBjbGllbnQgdG8gbmV2ZXIg
cmVjb3Zlcj8NCj4gPj4+Pg0KPiA+Pj4+IFRoZSBvbmx5IG90aGVyIGNhbGwgdG8gcmVuZXdfbGVh
c2UgaXMgZnJvbSB0aGUgc3RhdGUgbWFuYWdlciBpdHNlbGYNCj4gPj4+PiB3aGljaCBydW5zIG9u
ZSBmdW5jdGlvbiBhdCBhIHRpbWUsIGFuZCB3aWxsIG5vdCBjYWxsIHJlbmV3X2xlYXNlIHVudGls
DQo+ID4+Pj4gdGhlIHJlY292ZXJ5IGlzIG92ZXIuLi4uDQo+ID4+Pj4NCj4gPj4+PiBXaGF0IGFt
IEkgbWlzc2luZy4uLi4/DQo+ID4+Pg0KPiA+Pj4gbmZzNF9jaGVja19sZWFzZSgpIGlzIHJ1biBl
eGNsdXNpdmVseSBieSB0aGUgc3RhdGUgbWFuYWdlciB0aHJlYWQgaW4NCj4gPj4+IG9yZGVyIHRv
IGNoZWNrIHRoYXQgdGhlIGNsaWVudGlkIChhbmQgc2Vzc2lvbiBpZCBvbiBORlN2NC4xKSBhcmUg
dmFsaWQuDQo+ID4+PiBJdCB3aWxsIGRlYWRsb2NrIHRoZSBzdGF0ZSBtYW5hZ2VyIHRocmVhZCBp
ZiBORlM0X1NFU1NJT05fRFJBSU5JTkcgaXMNCj4gPj4+IGFscmVhZHkgc2V0Lg0KPiA+Pg0KPiA+
PiBPSy4gTkZTNF9TRVNTSU9OX0RSQUlOSU5HIGlzIGFsc28gc2V0IGV4Y2x1c2l2ZWx5IGJ5IHRo
ZSBzdGF0ZSBtYW5hZ2VyDQo+ID4+IHRocmVhZC4gV2h5IGlzIHRoZSBzdGF0ZSBtYW5hZ2VyIHRv
bGQgdG8gY2hlY2sgdGhlIGxlYXNlIHdoZW4gaXQncw0KPiA+PiBhbHNvIGRyYWluaW5nIGEgc2Vz
c2lvbj8NCj4gPiANCj4gPiBORlM0X1NFU1NJT05fRFJBSU5JTkcgZG9lcyBub3QganVzdCBtZWFu
IHRoYXQgdGhlIHNlc3Npb24gaXMgYmVpbmcNCj4gPiBkcmFpbmVkOyBpdCByZW1haW5zIHNldCB1
bnRpbCBvcGVuIGFuZCBsb2NrIHN0YXRlIHJlY292ZXJ5IGlzIGNvbXBsZXRlZC4NCj4gPiANCj4g
PiBJT1c6IGlmIHNvbWV0aGluZyBoYXBwZW5zIGR1cmluZyBzdGF0ZSByZWNvdmVyeSB0aGF0IHJl
cXVpcmVzIHVzIHRvDQo+ID4gY2hlY2sgdGhlIGxlYXNlIChlLmcuIHNvbWV0aGluZyByZXR1cm5z
IE5GUzRFUlJfRVhQSVJFRCkgdGhlbiB0aGUNCj4gPiBjdXJyZW50IGNvZGUgd2lsbCBkZWFkbG9j
ay4NCj4gPiANCj4gPj4gTm8gbWF0dGVyIHdoYXQgdGhlIGFuc3dlciwgcGxlYXNlIHVwZGF0ZSB0
aGUgcGF0Y2ggZGVzY3JpcHRpb24gdG8NCj4gPj4gYmV0dGVyIGV4cGxhaW4gdGhlIHByb2JsZW0g
YmVpbmcgc29sdmVkLg0KPiA+IA0KPiA+IEFDSy4gSSBhZ3JlZSB0aGF0IHRoZSBjaGFuZ2Vsb2cg
ZW50cnkgY2FuIGJlIGltcHJvdmVkLg0KPiA+IA0KPiANCj4gRG9lcyB0aGlzIHJlYWQgYW55IGJl
dHRlciAoYW5kIHNob3VsZCBJIHJlc2VuZCB0aGUgcGF0Y2gpPw0KPiANCj4gTkZTOiBBZGQgc2Vx
dWVuY2VfcHJpdmlsaWdlZF9vcHMgZm9yIG5mczRfcHJvY19zZXF1ZW5jZSgpDQo+IA0KPiBJZiBJ
IG1vdW50IGFuIE5GUyB2NC4xIHNlcnZlciB0byBhIHNpbmdsZSBjbGllbnQgbXVsdGlwbGUgdGlt
ZXMgYW5kIHRoZW4NCj4gcnVuIHhmc3Rlc3RzIG92ZXIgZWFjaCBtb3VudHBvaW50IEkgdXN1YWxs
eSBnZXQgdGhlIGNsaWVudCBpbnRvIGEgc3RhdGUNCj4gd2hlcmUgcmVjb3ZlcnkgZGVhZGxvY2tz
LiAgVGhlIHNlcnZlciBpbmZvcm1zIHRoZSBjbGllbnQgb2YgYQ0KPiBjYl9wYXRoX2Rvd24gc2Vx
dWVuY2UgZXJyb3IsIHRoZSBjbGllbnQgdGhlbiBkb2VzIGENCj4gYmluZF9jb25uZWN0aW9uX3Rv
X3Nlc3Npb24gYW5kIGNoZWNrcyB0aGUgc3RhdHVzIG9mIHRoZSBsZWFzZS4NCg0KV2h5IGFyZSB5
b3UgZ2V0dGluZyB0aGUgY2JfcGF0aF9kb3duPyBJcyB0aGF0IGEgcmVzdWx0IG9mIGEgZmF1bHQN
CmluamVjdGlvbiwgb3IgaXMgaXQgYSBnZW51aW5lIGVycm9yPw0KDQpXaGlsZSBjYl9wYXRoX2Rv
d24gaXMgYSB2YWxpZCBlcnJvciwgYW5kIHdlIGRvIHdhbnQgdGhlIHJlY292ZXJ5IHRvIHdvcmsN
CmNvcnJlY3RseSwgSSB3b3VsZCBleHBlY3QgaXQgdG8gYmUgcmFyZSBzaW5jZSBpdCBpbXBsaWVz
IHRoYXQgd2UgbG9zdA0KdGhlIFRDUCBjb25uZWN0aW9uIHRvIHRoZSBzZXJ2ZXIgZm9yIHNvbWUg
cmVhc29uLiBGaW5kaW5nIG91dCB3aHkgaXQNCmhhcHBlbnMgaXMgdGhlcmVmb3JlIHdvcnRoIGRv
aW5nLg0KDQo+IEkgZm91bmQgdGhhdCBiaW5kX2Nvbm5lY3Rpb25fdG9fc2Vzc2lvbiBzZXRzIHRo
ZSBORlM0X1NFU1NJT05fRFJBSU5JTkcNCj4gZmxhZyBvbiB0aGUgY2xpZW50LCBidXQgdGhpcyBm
bGFnIGlzIG5ldmVyIHVuc2V0IGJlZm9yZQ0KPiBuZnM0X2NoZWNrX2xlYXNlKCkgcmVhY2hlcyBu
ZnM0X3Byb2Nfc2VxdWVuY2UoKS4gIFRoaXMgY2F1c2VzIHRoZSBjbGllbnQNCj4gdG8gZGVhZGxv
Y2ssIGhhbHRpbmcgYWxsIE5GUyBhY3Rpdml0eSB0byB0aGUgc2VydmVyLg0KDQpPdGhlcndpc2Us
IHRoZSB0ZXh0IGlzIG1vcmUgb3IgbGVzcyBPSy4gVGhlIG9uZSB0aGluZyB0aGF0IEknbSBtaXNz
aW5nDQppcyBhIHN0YXRlbWVudCBhYm91dCB3aHkgbmZzNF9wcm9jX3NlcXVlbmNlKCkgbmVlZHMg
dG8gYmUgYSBwcml2aWxlZ2VkDQpvcGVyYXRpb24uDQoNCi0tIA0KVHJvbmQgTXlrbGVidXN0DQpM
aW51eCBORlMgY2xpZW50IG1haW50YWluZXINCg0KTmV0QXBwDQpUcm9uZC5NeWtsZWJ1c3RAbmV0
YXBwLmNvbQ0Kd3d3Lm5ldGFwcC5jb20NCg==

Re: [PATCH 1/2] NFS: Add sequence_priviliged_ops for nfs4_proc_sequence()

[Bryan Schumaker]

On 11/12/2012 04:10 PM, Myklebust, Trond wrote:
> On Mon, 2012-11-12 at 16:02 -0500, Bryan Schumaker wrote:
>> On 11/12/2012 03:54 PM, Myklebust, Trond wrote:
>>> On Mon, 2012-11-12 at 15:49 -0500, Andy Adamson wrote:
>>>> On Mon, Nov 12, 2012 at 3:29 PM, Myklebust, Trond
>>>> <Trond.Myklebust@netapp.com> wrote:
>>>>> On Mon, 2012-11-12 at 15:22 -0500, Andy Adamson wrote:
>>>>>> On Mon, Nov 12, 2012 at 1:35 PM,  <bjschuma@netapp.com> wrote:
>>>>>>> From: Bryan Schumaker <bjschuma@netapp.com>
>>>>>>>
>>>>>>> During recovery the NFS4_SESSION_DRAINING flag might be set on the
>>>>>>> client structure.  This can cause lease renewal to abort when
>>>>>
>>>>> Not lease renewal. It is lease verification (i.e. checking that we have
>>>>> a valid lease and session) that will deadlock.
>>>>>
>>>>>>> nfs41_setup_sequence sees that we are doing recovery.  As a result, the
>>>>>>> client never recovers and all activity with the NFS server halts.
>>>>>>
>>>>>>
>>>>>> When does this happen? Say the session is draining, and an RPC returns
>>>>>> from one of the compounds such as nfs_open, nfs_locku etc whose
>>>>>> rpc_call_done routine calls renew_lease after freeing it's slot.  Like
>>>>>> all calls on a draining session, the renew_lease sleeps on the session
>>>>>> slot_tbl_waitq - is this what you mean by "causes lease renewal to
>>>>>> abort"? How does this cause the client to never recover?
>>>>>>
>>>>>> The only other call to renew_lease is from the state manager itself
>>>>>> which runs one function at a time, and will not call renew_lease until
>>>>>> the recovery is over....
>>>>>>
>>>>>> What am I missing....?
>>>>>
>>>>> nfs4_check_lease() is run exclusively by the state manager thread in
>>>>> order to check that the clientid (and session id on NFSv4.1) are valid.
>>>>> It will deadlock the state manager thread if NFS4_SESSION_DRAINING is
>>>>> already set.
>>>>
>>>> OK. NFS4_SESSION_DRAINING is also set exclusively by the state manager
>>>> thread. Why is the state manager told to check the lease when it's
>>>> also draining a session?
>>>
>>> NFS4_SESSION_DRAINING does not just mean that the session is being
>>> drained; it remains set until open and lock state recovery is completed.
>>>
>>> IOW: if something happens during state recovery that requires us to
>>> check the lease (e.g. something returns NFS4ERR_EXPIRED) then the
>>> current code will deadlock.
>>>
>>>> No matter what the answer, please update the patch description to
>>>> better explain the problem being solved.
>>>
>>> ACK. I agree that the changelog entry can be improved.
>>>
>>
>> Does this read any better (and should I resend the patch)?
>>
>> NFS: Add sequence_priviliged_ops for nfs4_proc_sequence()
>>
>> If I mount an NFS v4.1 server to a single client multiple times and then
>> run xfstests over each mountpoint I usually get the client into a state
>> where recovery deadlocks.  The server informs the client of a
>> cb_path_down sequence error, the client then does a
>> bind_connection_to_session and checks the status of the lease.
> 
> Why are you getting the cb_path_down? Is that a result of a fault
> injection, or is it a genuine error?
> 
> While cb_path_down is a valid error, and we do want the recovery to work
> correctly, I would expect it to be rare since it implies that we lost
> the TCP connection to the server for some reason. Finding out why it
> happens is therefore worth doing.

I didn't get it with fault injection, it just happened by itself somewhere during testing.  My attempts at capturing it with wireshark usually slow down my system as wireshark tries to display 1,000,000+ packets... I'll try again, though.

> 
>> I found that bind_connection_to_session sets the NFS4_SESSION_DRAINING
>> flag on the client, but this flag is never unset before
>> nfs4_check_lease() reaches nfs4_proc_sequence().  This causes the client
>> to deadlock, halting all NFS activity to the server.
> 
> Otherwise, the text is more or less OK. The one thing that I'm missing
> is a statement about why nfs4_proc_sequence() needs to be a privileged
> operation.
> 

Here is the new last paragraph, I just added the sentence at the end:

I found that bind_connection_to_session sets the NFS4_SESSION_DRAINING
flag on the client, but this flag is never unset before
nfs4_check_lease() reaches nfs4_proc_sequence().  This causes the client
to deadlock, halting all NFS activity to the server.  This patch changes
nfs4_proc_sequence() to run in privileged mode to bypass the
NFS4_SESSION_DRAINING check and continue with recovery.

Re: [PATCH 1/2] NFS: Add sequence_priviliged_ops for nfs4_proc_sequence()

[Myklebust, Trond]

T24gTW9uLCAyMDEyLTExLTEyIGF0IDE2OjI2IC0wNTAwLCBCcnlhbiBTY2h1bWFrZXIgd3JvdGU6
DQo+IE9uIDExLzEyLzIwMTIgMDQ6MTAgUE0sIE15a2xlYnVzdCwgVHJvbmQgd3JvdGU6DQo+ID4g
T24gTW9uLCAyMDEyLTExLTEyIGF0IDE2OjAyIC0wNTAwLCBCcnlhbiBTY2h1bWFrZXIgd3JvdGU6
DQo+ID4+IE9uIDExLzEyLzIwMTIgMDM6NTQgUE0sIE15a2xlYnVzdCwgVHJvbmQgd3JvdGU6DQo+
ID4+PiBPbiBNb24sIDIwMTItMTEtMTIgYXQgMTU6NDkgLTA1MDAsIEFuZHkgQWRhbXNvbiB3cm90
ZToNCj4gPj4+PiBPbiBNb24sIE5vdiAxMiwgMjAxMiBhdCAzOjI5IFBNLCBNeWtsZWJ1c3QsIFRy
b25kDQo+ID4+Pj4gPFRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tPiB3cm90ZToNCj4gPj4+Pj4g
T24gTW9uLCAyMDEyLTExLTEyIGF0IDE1OjIyIC0wNTAwLCBBbmR5IEFkYW1zb24gd3JvdGU6DQo+
ID4+Pj4+PiBPbiBNb24sIE5vdiAxMiwgMjAxMiBhdCAxOjM1IFBNLCAgPGJqc2NodW1hQG5ldGFw
cC5jb20+IHdyb3RlOg0KPiA+Pj4+Pj4+IEZyb206IEJyeWFuIFNjaHVtYWtlciA8YmpzY2h1bWFA
bmV0YXBwLmNvbT4NCj4gPj4+Pj4+Pg0KPiA+Pj4+Pj4+IER1cmluZyByZWNvdmVyeSB0aGUgTkZT
NF9TRVNTSU9OX0RSQUlOSU5HIGZsYWcgbWlnaHQgYmUgc2V0IG9uIHRoZQ0KPiA+Pj4+Pj4+IGNs
aWVudCBzdHJ1Y3R1cmUuICBUaGlzIGNhbiBjYXVzZSBsZWFzZSByZW5ld2FsIHRvIGFib3J0IHdo
ZW4NCj4gPj4+Pj4NCj4gPj4+Pj4gTm90IGxlYXNlIHJlbmV3YWwuIEl0IGlzIGxlYXNlIHZlcmlm
aWNhdGlvbiAoaS5lLiBjaGVja2luZyB0aGF0IHdlIGhhdmUNCj4gPj4+Pj4gYSB2YWxpZCBsZWFz
ZSBhbmQgc2Vzc2lvbikgdGhhdCB3aWxsIGRlYWRsb2NrLg0KPiA+Pj4+Pg0KPiA+Pj4+Pj4+IG5m
czQxX3NldHVwX3NlcXVlbmNlIHNlZXMgdGhhdCB3ZSBhcmUgZG9pbmcgcmVjb3ZlcnkuICBBcyBh
IHJlc3VsdCwgdGhlDQo+ID4+Pj4+Pj4gY2xpZW50IG5ldmVyIHJlY292ZXJzIGFuZCBhbGwgYWN0
aXZpdHkgd2l0aCB0aGUgTkZTIHNlcnZlciBoYWx0cy4NCj4gPj4+Pj4+DQo+ID4+Pj4+Pg0KPiA+
Pj4+Pj4gV2hlbiBkb2VzIHRoaXMgaGFwcGVuPyBTYXkgdGhlIHNlc3Npb24gaXMgZHJhaW5pbmcs
IGFuZCBhbiBSUEMgcmV0dXJucw0KPiA+Pj4+Pj4gZnJvbSBvbmUgb2YgdGhlIGNvbXBvdW5kcyBz
dWNoIGFzIG5mc19vcGVuLCBuZnNfbG9ja3UgZXRjIHdob3NlDQo+ID4+Pj4+PiBycGNfY2FsbF9k
b25lIHJvdXRpbmUgY2FsbHMgcmVuZXdfbGVhc2UgYWZ0ZXIgZnJlZWluZyBpdCdzIHNsb3QuICBM
aWtlDQo+ID4+Pj4+PiBhbGwgY2FsbHMgb24gYSBkcmFpbmluZyBzZXNzaW9uLCB0aGUgcmVuZXdf
bGVhc2Ugc2xlZXBzIG9uIHRoZSBzZXNzaW9uDQo+ID4+Pj4+PiBzbG90X3RibF93YWl0cSAtIGlz
IHRoaXMgd2hhdCB5b3UgbWVhbiBieSAiY2F1c2VzIGxlYXNlIHJlbmV3YWwgdG8NCj4gPj4+Pj4+
IGFib3J0Ij8gSG93IGRvZXMgdGhpcyBjYXVzZSB0aGUgY2xpZW50IHRvIG5ldmVyIHJlY292ZXI/
DQo+ID4+Pj4+Pg0KPiA+Pj4+Pj4gVGhlIG9ubHkgb3RoZXIgY2FsbCB0byByZW5ld19sZWFzZSBp
cyBmcm9tIHRoZSBzdGF0ZSBtYW5hZ2VyIGl0c2VsZg0KPiA+Pj4+Pj4gd2hpY2ggcnVucyBvbmUg
ZnVuY3Rpb24gYXQgYSB0aW1lLCBhbmQgd2lsbCBub3QgY2FsbCByZW5ld19sZWFzZSB1bnRpbA0K
PiA+Pj4+Pj4gdGhlIHJlY292ZXJ5IGlzIG92ZXIuLi4uDQo+ID4+Pj4+Pg0KPiA+Pj4+Pj4gV2hh
dCBhbSBJIG1pc3NpbmcuLi4uPw0KPiA+Pj4+Pg0KPiA+Pj4+PiBuZnM0X2NoZWNrX2xlYXNlKCkg
aXMgcnVuIGV4Y2x1c2l2ZWx5IGJ5IHRoZSBzdGF0ZSBtYW5hZ2VyIHRocmVhZCBpbg0KPiA+Pj4+
PiBvcmRlciB0byBjaGVjayB0aGF0IHRoZSBjbGllbnRpZCAoYW5kIHNlc3Npb24gaWQgb24gTkZT
djQuMSkgYXJlIHZhbGlkLg0KPiA+Pj4+PiBJdCB3aWxsIGRlYWRsb2NrIHRoZSBzdGF0ZSBtYW5h
Z2VyIHRocmVhZCBpZiBORlM0X1NFU1NJT05fRFJBSU5JTkcgaXMNCj4gPj4+Pj4gYWxyZWFkeSBz
ZXQuDQo+ID4+Pj4NCj4gPj4+PiBPSy4gTkZTNF9TRVNTSU9OX0RSQUlOSU5HIGlzIGFsc28gc2V0
IGV4Y2x1c2l2ZWx5IGJ5IHRoZSBzdGF0ZSBtYW5hZ2VyDQo+ID4+Pj4gdGhyZWFkLiBXaHkgaXMg
dGhlIHN0YXRlIG1hbmFnZXIgdG9sZCB0byBjaGVjayB0aGUgbGVhc2Ugd2hlbiBpdCdzDQo+ID4+
Pj4gYWxzbyBkcmFpbmluZyBhIHNlc3Npb24/DQo+ID4+Pg0KPiA+Pj4gTkZTNF9TRVNTSU9OX0RS
QUlOSU5HIGRvZXMgbm90IGp1c3QgbWVhbiB0aGF0IHRoZSBzZXNzaW9uIGlzIGJlaW5nDQo+ID4+
PiBkcmFpbmVkOyBpdCByZW1haW5zIHNldCB1bnRpbCBvcGVuIGFuZCBsb2NrIHN0YXRlIHJlY292
ZXJ5IGlzIGNvbXBsZXRlZC4NCj4gPj4+DQo+ID4+PiBJT1c6IGlmIHNvbWV0aGluZyBoYXBwZW5z
IGR1cmluZyBzdGF0ZSByZWNvdmVyeSB0aGF0IHJlcXVpcmVzIHVzIHRvDQo+ID4+PiBjaGVjayB0
aGUgbGVhc2UgKGUuZy4gc29tZXRoaW5nIHJldHVybnMgTkZTNEVSUl9FWFBJUkVEKSB0aGVuIHRo
ZQ0KPiA+Pj4gY3VycmVudCBjb2RlIHdpbGwgZGVhZGxvY2suDQo+ID4+Pg0KPiA+Pj4+IE5vIG1h
dHRlciB3aGF0IHRoZSBhbnN3ZXIsIHBsZWFzZSB1cGRhdGUgdGhlIHBhdGNoIGRlc2NyaXB0aW9u
IHRvDQo+ID4+Pj4gYmV0dGVyIGV4cGxhaW4gdGhlIHByb2JsZW0gYmVpbmcgc29sdmVkLg0KPiA+
Pj4NCj4gPj4+IEFDSy4gSSBhZ3JlZSB0aGF0IHRoZSBjaGFuZ2Vsb2cgZW50cnkgY2FuIGJlIGlt
cHJvdmVkLg0KPiA+Pj4NCj4gPj4NCj4gPj4gRG9lcyB0aGlzIHJlYWQgYW55IGJldHRlciAoYW5k
IHNob3VsZCBJIHJlc2VuZCB0aGUgcGF0Y2gpPw0KPiA+Pg0KPiA+PiBORlM6IEFkZCBzZXF1ZW5j
ZV9wcml2aWxpZ2VkX29wcyBmb3IgbmZzNF9wcm9jX3NlcXVlbmNlKCkNCj4gPj4NCj4gPj4gSWYg
SSBtb3VudCBhbiBORlMgdjQuMSBzZXJ2ZXIgdG8gYSBzaW5nbGUgY2xpZW50IG11bHRpcGxlIHRp
bWVzIGFuZCB0aGVuDQo+ID4+IHJ1biB4ZnN0ZXN0cyBvdmVyIGVhY2ggbW91bnRwb2ludCBJIHVz
dWFsbHkgZ2V0IHRoZSBjbGllbnQgaW50byBhIHN0YXRlDQo+ID4+IHdoZXJlIHJlY292ZXJ5IGRl
YWRsb2Nrcy4gIFRoZSBzZXJ2ZXIgaW5mb3JtcyB0aGUgY2xpZW50IG9mIGENCj4gPj4gY2JfcGF0
aF9kb3duIHNlcXVlbmNlIGVycm9yLCB0aGUgY2xpZW50IHRoZW4gZG9lcyBhDQo+ID4+IGJpbmRf
Y29ubmVjdGlvbl90b19zZXNzaW9uIGFuZCBjaGVja3MgdGhlIHN0YXR1cyBvZiB0aGUgbGVhc2Uu
DQo+ID4gDQo+ID4gV2h5IGFyZSB5b3UgZ2V0dGluZyB0aGUgY2JfcGF0aF9kb3duPyBJcyB0aGF0
IGEgcmVzdWx0IG9mIGEgZmF1bHQNCj4gPiBpbmplY3Rpb24sIG9yIGlzIGl0IGEgZ2VudWluZSBl
cnJvcj8NCj4gPiANCj4gPiBXaGlsZSBjYl9wYXRoX2Rvd24gaXMgYSB2YWxpZCBlcnJvciwgYW5k
IHdlIGRvIHdhbnQgdGhlIHJlY292ZXJ5IHRvIHdvcmsNCj4gPiBjb3JyZWN0bHksIEkgd291bGQg
ZXhwZWN0IGl0IHRvIGJlIHJhcmUgc2luY2UgaXQgaW1wbGllcyB0aGF0IHdlIGxvc3QNCj4gPiB0
aGUgVENQIGNvbm5lY3Rpb24gdG8gdGhlIHNlcnZlciBmb3Igc29tZSByZWFzb24uIEZpbmRpbmcg
b3V0IHdoeSBpdA0KPiA+IGhhcHBlbnMgaXMgdGhlcmVmb3JlIHdvcnRoIGRvaW5nLg0KPiANCj4g
SSBkaWRuJ3QgZ2V0IGl0IHdpdGggZmF1bHQgaW5qZWN0aW9uLCBpdCBqdXN0IGhhcHBlbmVkIGJ5
IGl0c2VsZiBzb21ld2hlcmUgZHVyaW5nIHRlc3RpbmcuICBNeSBhdHRlbXB0cyBhdCBjYXB0dXJp
bmcgaXQgd2l0aCB3aXJlc2hhcmsgdXN1YWxseSBzbG93IGRvd24gbXkgc3lzdGVtIGFzIHdpcmVz
aGFyayB0cmllcyB0byBkaXNwbGF5IDEsMDAwLDAwMCsgcGFja2V0cy4uLiBJJ2xsIHRyeSBhZ2Fp
biwgdGhvdWdoLg0KDQpTZWUgaWYgeW91IGNhbiBqdXN0IHR1cm4gb24gdGhlIHR3byBkcHJpbnRr
cygpIGluIHhzX3RjcF9zdGF0ZV9jaGFuZ2UoKSwNCmFuZCB0aGVuIGFkZCBhIHByaW50aygpIHRy
aWdnZXIgdG8gdGhlDQpuZnM0MV9oYW5kbGVfc2VxdWVuY2VfZmxhZ19lcnJvcnMoKSB0byBzZWUg
aWYgeW91IGNhbiBjYXRjaCBhDQpjb3JyZWxhdGlvbiBiZXR3ZWVuIGEgVENQIHN0YXRlIGNoYW5n
ZSBhbmQgdGhlIHBhdGhfZG93biBpc3N1ZS4NCg0KPiA+IA0KPiA+PiBJIGZvdW5kIHRoYXQgYmlu
ZF9jb25uZWN0aW9uX3RvX3Nlc3Npb24gc2V0cyB0aGUgTkZTNF9TRVNTSU9OX0RSQUlOSU5HDQo+
ID4+IGZsYWcgb24gdGhlIGNsaWVudCwgYnV0IHRoaXMgZmxhZyBpcyBuZXZlciB1bnNldCBiZWZv
cmUNCj4gPj4gbmZzNF9jaGVja19sZWFzZSgpIHJlYWNoZXMgbmZzNF9wcm9jX3NlcXVlbmNlKCku
ICBUaGlzIGNhdXNlcyB0aGUgY2xpZW50DQo+ID4+IHRvIGRlYWRsb2NrLCBoYWx0aW5nIGFsbCBO
RlMgYWN0aXZpdHkgdG8gdGhlIHNlcnZlci4NCj4gPiANCj4gPiBPdGhlcndpc2UsIHRoZSB0ZXh0
IGlzIG1vcmUgb3IgbGVzcyBPSy4gVGhlIG9uZSB0aGluZyB0aGF0IEknbSBtaXNzaW5nDQo+ID4g
aXMgYSBzdGF0ZW1lbnQgYWJvdXQgd2h5IG5mczRfcHJvY19zZXF1ZW5jZSgpIG5lZWRzIHRvIGJl
IGEgcHJpdmlsZWdlZA0KPiA+IG9wZXJhdGlvbi4NCj4gPiANCj4gDQo+IEhlcmUgaXMgdGhlIG5l
dyBsYXN0IHBhcmFncmFwaCwgSSBqdXN0IGFkZGVkIHRoZSBzZW50ZW5jZSBhdCB0aGUgZW5kOg0K
PiANCj4gSSBmb3VuZCB0aGF0IGJpbmRfY29ubmVjdGlvbl90b19zZXNzaW9uIHNldHMgdGhlIE5G
UzRfU0VTU0lPTl9EUkFJTklORw0KPiBmbGFnIG9uIHRoZSBjbGllbnQsIGJ1dCB0aGlzIGZsYWcg
aXMgbmV2ZXIgdW5zZXQgYmVmb3JlDQo+IG5mczRfY2hlY2tfbGVhc2UoKSByZWFjaGVzIG5mczRf
cHJvY19zZXF1ZW5jZSgpLiAgVGhpcyBjYXVzZXMgdGhlIGNsaWVudA0KPiB0byBkZWFkbG9jaywg
aGFsdGluZyBhbGwgTkZTIGFjdGl2aXR5IHRvIHRoZSBzZXJ2ZXIuICBUaGlzIHBhdGNoIGNoYW5n
ZXMNCj4gbmZzNF9wcm9jX3NlcXVlbmNlKCkgdG8gcnVuIGluIHByaXZpbGVnZWQgbW9kZSB0byBi
eXBhc3MgdGhlDQo+IE5GUzRfU0VTU0lPTl9EUkFJTklORyBjaGVjayBhbmQgY29udGludWUgd2l0
aCByZWNvdmVyeS4NCg0KVGhhbmtzLiBZb3UgbWlnaHQgd2FudCB0byBhZGQgYSBub3RlIHRvIHRo
ZSBmYWN0IHRoYXQNCm5mczRfcHJvY19zZXF1ZW5jZSgpIGlzIGFsd2F5cyBydW4gZnJvbSB0aGUg
c3RhdGUgcmVjb3ZlcnkgdGhyZWFkLCBhbmQNCnRoZXJlZm9yZSBpdCBpcyBjb3JyZWN0IHRvIHJ1
biBpdCBhcyBhIHByaXZpbGVnZWQgb3BlcmF0aW9uLg0KDQotLSANClRyb25kIE15a2xlYnVzdA0K
TGludXggTkZTIGNsaWVudCBtYWludGFpbmVyDQoNCk5ldEFwcA0KVHJvbmQuTXlrbGVidXN0QG5l
dGFwcC5jb20NCnd3dy5uZXRhcHAuY29tDQo=

Re: [PATCH 1/2] NFS: Add sequence_priviliged_ops for nfs4_proc_sequence()

[Bryan Schumaker]

On 11/12/2012 04:37 PM, Myklebust, Trond wrote:
> On Mon, 2012-11-12 at 16:26 -0500, Bryan Schumaker wrote:
>> On 11/12/2012 04:10 PM, Myklebust, Trond wrote:
>>> On Mon, 2012-11-12 at 16:02 -0500, Bryan Schumaker wrote:
>>>> On 11/12/2012 03:54 PM, Myklebust, Trond wrote:
>>>>> On Mon, 2012-11-12 at 15:49 -0500, Andy Adamson wrote:
>>>>>> On Mon, Nov 12, 2012 at 3:29 PM, Myklebust, Trond
>>>>>> <Trond.Myklebust@netapp.com> wrote:
>>>>>>> On Mon, 2012-11-12 at 15:22 -0500, Andy Adamson wrote:
>>>>>>>> On Mon, Nov 12, 2012 at 1:35 PM,  <bjschuma@netapp.com> wrote:
>>>>>>>>> From: Bryan Schumaker <bjschuma@netapp.com>
>>>>>>>>>
>>>>>>>>> During recovery the NFS4_SESSION_DRAINING flag might be set on the
>>>>>>>>> client structure.  This can cause lease renewal to abort when
>>>>>>>
>>>>>>> Not lease renewal. It is lease verification (i.e. checking that we have
>>>>>>> a valid lease and session) that will deadlock.
>>>>>>>
>>>>>>>>> nfs41_setup_sequence sees that we are doing recovery.  As a result, the
>>>>>>>>> client never recovers and all activity with the NFS server halts.
>>>>>>>>
>>>>>>>>
>>>>>>>> When does this happen? Say the session is draining, and an RPC returns
>>>>>>>> from one of the compounds such as nfs_open, nfs_locku etc whose
>>>>>>>> rpc_call_done routine calls renew_lease after freeing it's slot.  Like
>>>>>>>> all calls on a draining session, the renew_lease sleeps on the session
>>>>>>>> slot_tbl_waitq - is this what you mean by "causes lease renewal to
>>>>>>>> abort"? How does this cause the client to never recover?
>>>>>>>>
>>>>>>>> The only other call to renew_lease is from the state manager itself
>>>>>>>> which runs one function at a time, and will not call renew_lease until
>>>>>>>> the recovery is over....
>>>>>>>>
>>>>>>>> What am I missing....?
>>>>>>>
>>>>>>> nfs4_check_lease() is run exclusively by the state manager thread in
>>>>>>> order to check that the clientid (and session id on NFSv4.1) are valid.
>>>>>>> It will deadlock the state manager thread if NFS4_SESSION_DRAINING is
>>>>>>> already set.
>>>>>>
>>>>>> OK. NFS4_SESSION_DRAINING is also set exclusively by the state manager
>>>>>> thread. Why is the state manager told to check the lease when it's
>>>>>> also draining a session?
>>>>>
>>>>> NFS4_SESSION_DRAINING does not just mean that the session is being
>>>>> drained; it remains set until open and lock state recovery is completed.
>>>>>
>>>>> IOW: if something happens during state recovery that requires us to
>>>>> check the lease (e.g. something returns NFS4ERR_EXPIRED) then the
>>>>> current code will deadlock.
>>>>>
>>>>>> No matter what the answer, please update the patch description to
>>>>>> better explain the problem being solved.
>>>>>
>>>>> ACK. I agree that the changelog entry can be improved.
>>>>>
>>>>
>>>> Does this read any better (and should I resend the patch)?
>>>>
>>>> NFS: Add sequence_priviliged_ops for nfs4_proc_sequence()
>>>>
>>>> If I mount an NFS v4.1 server to a single client multiple times and then
>>>> run xfstests over each mountpoint I usually get the client into a state
>>>> where recovery deadlocks.  The server informs the client of a
>>>> cb_path_down sequence error, the client then does a
>>>> bind_connection_to_session and checks the status of the lease.
>>>
>>> Why are you getting the cb_path_down? Is that a result of a fault
>>> injection, or is it a genuine error?
>>>
>>> While cb_path_down is a valid error, and we do want the recovery to work
>>> correctly, I would expect it to be rare since it implies that we lost
>>> the TCP connection to the server for some reason. Finding out why it
>>> happens is therefore worth doing.
>>
>> I didn't get it with fault injection, it just happened by itself somewhere during testing.  My attempts at capturing it with wireshark usually slow down my system as wireshark tries to display 1,000,000+ packets... I'll try again, though.
> 
> See if you can just turn on the two dprintks() in xs_tcp_state_change(),
> and then add a printk() trigger to the
> nfs41_handle_sequence_flag_errors() to see if you can catch a
> correlation between a TCP state change and the path_down issue.

Sounds simple enough.  I'll do that now...

> 
>>>
>>>> I found that bind_connection_to_session sets the NFS4_SESSION_DRAINING
>>>> flag on the client, but this flag is never unset before
>>>> nfs4_check_lease() reaches nfs4_proc_sequence().  This causes the client
>>>> to deadlock, halting all NFS activity to the server.
>>>
>>> Otherwise, the text is more or less OK. The one thing that I'm missing
>>> is a statement about why nfs4_proc_sequence() needs to be a privileged
>>> operation.
>>>
>>
>> Here is the new last paragraph, I just added the sentence at the end:
>>
>> I found that bind_connection_to_session sets the NFS4_SESSION_DRAINING
>> flag on the client, but this flag is never unset before
>> nfs4_check_lease() reaches nfs4_proc_sequence().  This causes the client
>> to deadlock, halting all NFS activity to the server.  This patch changes
>> nfs4_proc_sequence() to run in privileged mode to bypass the
>> NFS4_SESSION_DRAINING check and continue with recovery.
> 
> Thanks. You might want to add a note to the fact that
> nfs4_proc_sequence() is always run from the state recovery thread, and
> therefore it is correct to run it as a privileged operation.
> 

Ok.  Do you want a new patch?  Just the commit text?

Re: [PATCH 1/2] NFS: Add sequence_priviliged_ops for nfs4_proc_sequence()

[Myklebust, Trond]

T24gTW9uLCAyMDEyLTExLTEyIGF0IDE2OjQxIC0wNTAwLCBCcnlhbiBTY2h1bWFrZXIgd3JvdGU6
DQo+IE9uIDExLzEyLzIwMTIgMDQ6MzcgUE0sIE15a2xlYnVzdCwgVHJvbmQgd3JvdGU6DQo+ID4g
T24gTW9uLCAyMDEyLTExLTEyIGF0IDE2OjI2IC0wNTAwLCBCcnlhbiBTY2h1bWFrZXIgd3JvdGU6
DQo+ID4+IE9uIDExLzEyLzIwMTIgMDQ6MTAgUE0sIE15a2xlYnVzdCwgVHJvbmQgd3JvdGU6DQo+
ID4+PiBPbiBNb24sIDIwMTItMTEtMTIgYXQgMTY6MDIgLTA1MDAsIEJyeWFuIFNjaHVtYWtlciB3
cm90ZToNCj4gPj4+PiBPbiAxMS8xMi8yMDEyIDAzOjU0IFBNLCBNeWtsZWJ1c3QsIFRyb25kIHdy
b3RlOg0KPiA+Pj4+PiBPbiBNb24sIDIwMTItMTEtMTIgYXQgMTU6NDkgLTA1MDAsIEFuZHkgQWRh
bXNvbiB3cm90ZToNCj4gPj4+Pj4+IE9uIE1vbiwgTm92IDEyLCAyMDEyIGF0IDM6MjkgUE0sIE15
a2xlYnVzdCwgVHJvbmQNCj4gPj4+Pj4+IDxUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbT4gd3Jv
dGU6DQo+ID4+Pj4+Pj4gT24gTW9uLCAyMDEyLTExLTEyIGF0IDE1OjIyIC0wNTAwLCBBbmR5IEFk
YW1zb24gd3JvdGU6DQo+ID4+Pj4+Pj4+IE9uIE1vbiwgTm92IDEyLCAyMDEyIGF0IDE6MzUgUE0s
ICA8YmpzY2h1bWFAbmV0YXBwLmNvbT4gd3JvdGU6DQo+ID4+Pj4+Pj4+PiBGcm9tOiBCcnlhbiBT
Y2h1bWFrZXIgPGJqc2NodW1hQG5ldGFwcC5jb20+DQo+ID4+Pj4+Pj4+Pg0KPiA+Pj4+Pj4+Pj4g
RHVyaW5nIHJlY292ZXJ5IHRoZSBORlM0X1NFU1NJT05fRFJBSU5JTkcgZmxhZyBtaWdodCBiZSBz
ZXQgb24gdGhlDQo+ID4+Pj4+Pj4+PiBjbGllbnQgc3RydWN0dXJlLiAgVGhpcyBjYW4gY2F1c2Ug
bGVhc2UgcmVuZXdhbCB0byBhYm9ydCB3aGVuDQo+ID4+Pj4+Pj4NCj4gPj4+Pj4+PiBOb3QgbGVh
c2UgcmVuZXdhbC4gSXQgaXMgbGVhc2UgdmVyaWZpY2F0aW9uIChpLmUuIGNoZWNraW5nIHRoYXQg
d2UgaGF2ZQ0KPiA+Pj4+Pj4+IGEgdmFsaWQgbGVhc2UgYW5kIHNlc3Npb24pIHRoYXQgd2lsbCBk
ZWFkbG9jay4NCj4gPj4+Pj4+Pg0KPiA+Pj4+Pj4+Pj4gbmZzNDFfc2V0dXBfc2VxdWVuY2Ugc2Vl
cyB0aGF0IHdlIGFyZSBkb2luZyByZWNvdmVyeS4gIEFzIGEgcmVzdWx0LCB0aGUNCj4gPj4+Pj4+
Pj4+IGNsaWVudCBuZXZlciByZWNvdmVycyBhbmQgYWxsIGFjdGl2aXR5IHdpdGggdGhlIE5GUyBz
ZXJ2ZXIgaGFsdHMuDQo+ID4+Pj4+Pj4+DQo+ID4+Pj4+Pj4+DQo+ID4+Pj4+Pj4+IFdoZW4gZG9l
cyB0aGlzIGhhcHBlbj8gU2F5IHRoZSBzZXNzaW9uIGlzIGRyYWluaW5nLCBhbmQgYW4gUlBDIHJl
dHVybnMNCj4gPj4+Pj4+Pj4gZnJvbSBvbmUgb2YgdGhlIGNvbXBvdW5kcyBzdWNoIGFzIG5mc19v
cGVuLCBuZnNfbG9ja3UgZXRjIHdob3NlDQo+ID4+Pj4+Pj4+IHJwY19jYWxsX2RvbmUgcm91dGlu
ZSBjYWxscyByZW5ld19sZWFzZSBhZnRlciBmcmVlaW5nIGl0J3Mgc2xvdC4gIExpa2UNCj4gPj4+
Pj4+Pj4gYWxsIGNhbGxzIG9uIGEgZHJhaW5pbmcgc2Vzc2lvbiwgdGhlIHJlbmV3X2xlYXNlIHNs
ZWVwcyBvbiB0aGUgc2Vzc2lvbg0KPiA+Pj4+Pj4+PiBzbG90X3RibF93YWl0cSAtIGlzIHRoaXMg
d2hhdCB5b3UgbWVhbiBieSAiY2F1c2VzIGxlYXNlIHJlbmV3YWwgdG8NCj4gPj4+Pj4+Pj4gYWJv
cnQiPyBIb3cgZG9lcyB0aGlzIGNhdXNlIHRoZSBjbGllbnQgdG8gbmV2ZXIgcmVjb3Zlcj8NCj4g
Pj4+Pj4+Pj4NCj4gPj4+Pj4+Pj4gVGhlIG9ubHkgb3RoZXIgY2FsbCB0byByZW5ld19sZWFzZSBp
cyBmcm9tIHRoZSBzdGF0ZSBtYW5hZ2VyIGl0c2VsZg0KPiA+Pj4+Pj4+PiB3aGljaCBydW5zIG9u
ZSBmdW5jdGlvbiBhdCBhIHRpbWUsIGFuZCB3aWxsIG5vdCBjYWxsIHJlbmV3X2xlYXNlIHVudGls
DQo+ID4+Pj4+Pj4+IHRoZSByZWNvdmVyeSBpcyBvdmVyLi4uLg0KPiA+Pj4+Pj4+Pg0KPiA+Pj4+
Pj4+PiBXaGF0IGFtIEkgbWlzc2luZy4uLi4/DQo+ID4+Pj4+Pj4NCj4gPj4+Pj4+PiBuZnM0X2No
ZWNrX2xlYXNlKCkgaXMgcnVuIGV4Y2x1c2l2ZWx5IGJ5IHRoZSBzdGF0ZSBtYW5hZ2VyIHRocmVh
ZCBpbg0KPiA+Pj4+Pj4+IG9yZGVyIHRvIGNoZWNrIHRoYXQgdGhlIGNsaWVudGlkIChhbmQgc2Vz
c2lvbiBpZCBvbiBORlN2NC4xKSBhcmUgdmFsaWQuDQo+ID4+Pj4+Pj4gSXQgd2lsbCBkZWFkbG9j
ayB0aGUgc3RhdGUgbWFuYWdlciB0aHJlYWQgaWYgTkZTNF9TRVNTSU9OX0RSQUlOSU5HIGlzDQo+
ID4+Pj4+Pj4gYWxyZWFkeSBzZXQuDQo+ID4+Pj4+Pg0KPiA+Pj4+Pj4gT0suIE5GUzRfU0VTU0lP
Tl9EUkFJTklORyBpcyBhbHNvIHNldCBleGNsdXNpdmVseSBieSB0aGUgc3RhdGUgbWFuYWdlcg0K
PiA+Pj4+Pj4gdGhyZWFkLiBXaHkgaXMgdGhlIHN0YXRlIG1hbmFnZXIgdG9sZCB0byBjaGVjayB0
aGUgbGVhc2Ugd2hlbiBpdCdzDQo+ID4+Pj4+PiBhbHNvIGRyYWluaW5nIGEgc2Vzc2lvbj8NCj4g
Pj4+Pj4NCj4gPj4+Pj4gTkZTNF9TRVNTSU9OX0RSQUlOSU5HIGRvZXMgbm90IGp1c3QgbWVhbiB0
aGF0IHRoZSBzZXNzaW9uIGlzIGJlaW5nDQo+ID4+Pj4+IGRyYWluZWQ7IGl0IHJlbWFpbnMgc2V0
IHVudGlsIG9wZW4gYW5kIGxvY2sgc3RhdGUgcmVjb3ZlcnkgaXMgY29tcGxldGVkLg0KPiA+Pj4+
Pg0KPiA+Pj4+PiBJT1c6IGlmIHNvbWV0aGluZyBoYXBwZW5zIGR1cmluZyBzdGF0ZSByZWNvdmVy
eSB0aGF0IHJlcXVpcmVzIHVzIHRvDQo+ID4+Pj4+IGNoZWNrIHRoZSBsZWFzZSAoZS5nLiBzb21l
dGhpbmcgcmV0dXJucyBORlM0RVJSX0VYUElSRUQpIHRoZW4gdGhlDQo+ID4+Pj4+IGN1cnJlbnQg
Y29kZSB3aWxsIGRlYWRsb2NrLg0KPiA+Pj4+Pg0KPiA+Pj4+Pj4gTm8gbWF0dGVyIHdoYXQgdGhl
IGFuc3dlciwgcGxlYXNlIHVwZGF0ZSB0aGUgcGF0Y2ggZGVzY3JpcHRpb24gdG8NCj4gPj4+Pj4+
IGJldHRlciBleHBsYWluIHRoZSBwcm9ibGVtIGJlaW5nIHNvbHZlZC4NCj4gPj4+Pj4NCj4gPj4+
Pj4gQUNLLiBJIGFncmVlIHRoYXQgdGhlIGNoYW5nZWxvZyBlbnRyeSBjYW4gYmUgaW1wcm92ZWQu
DQo+ID4+Pj4+DQo+ID4+Pj4NCj4gPj4+PiBEb2VzIHRoaXMgcmVhZCBhbnkgYmV0dGVyIChhbmQg
c2hvdWxkIEkgcmVzZW5kIHRoZSBwYXRjaCk/DQo+ID4+Pj4NCj4gPj4+PiBORlM6IEFkZCBzZXF1
ZW5jZV9wcml2aWxpZ2VkX29wcyBmb3IgbmZzNF9wcm9jX3NlcXVlbmNlKCkNCj4gPj4+Pg0KPiA+
Pj4+IElmIEkgbW91bnQgYW4gTkZTIHY0LjEgc2VydmVyIHRvIGEgc2luZ2xlIGNsaWVudCBtdWx0
aXBsZSB0aW1lcyBhbmQgdGhlbg0KPiA+Pj4+IHJ1biB4ZnN0ZXN0cyBvdmVyIGVhY2ggbW91bnRw
b2ludCBJIHVzdWFsbHkgZ2V0IHRoZSBjbGllbnQgaW50byBhIHN0YXRlDQo+ID4+Pj4gd2hlcmUg
cmVjb3ZlcnkgZGVhZGxvY2tzLiAgVGhlIHNlcnZlciBpbmZvcm1zIHRoZSBjbGllbnQgb2YgYQ0K
PiA+Pj4+IGNiX3BhdGhfZG93biBzZXF1ZW5jZSBlcnJvciwgdGhlIGNsaWVudCB0aGVuIGRvZXMg
YQ0KPiA+Pj4+IGJpbmRfY29ubmVjdGlvbl90b19zZXNzaW9uIGFuZCBjaGVja3MgdGhlIHN0YXR1
cyBvZiB0aGUgbGVhc2UuDQo+ID4+Pg0KPiA+Pj4gV2h5IGFyZSB5b3UgZ2V0dGluZyB0aGUgY2Jf
cGF0aF9kb3duPyBJcyB0aGF0IGEgcmVzdWx0IG9mIGEgZmF1bHQNCj4gPj4+IGluamVjdGlvbiwg
b3IgaXMgaXQgYSBnZW51aW5lIGVycm9yPw0KPiA+Pj4NCj4gPj4+IFdoaWxlIGNiX3BhdGhfZG93
biBpcyBhIHZhbGlkIGVycm9yLCBhbmQgd2UgZG8gd2FudCB0aGUgcmVjb3ZlcnkgdG8gd29yaw0K
PiA+Pj4gY29ycmVjdGx5LCBJIHdvdWxkIGV4cGVjdCBpdCB0byBiZSByYXJlIHNpbmNlIGl0IGlt
cGxpZXMgdGhhdCB3ZSBsb3N0DQo+ID4+PiB0aGUgVENQIGNvbm5lY3Rpb24gdG8gdGhlIHNlcnZl
ciBmb3Igc29tZSByZWFzb24uIEZpbmRpbmcgb3V0IHdoeSBpdA0KPiA+Pj4gaGFwcGVucyBpcyB0
aGVyZWZvcmUgd29ydGggZG9pbmcuDQo+ID4+DQo+ID4+IEkgZGlkbid0IGdldCBpdCB3aXRoIGZh
dWx0IGluamVjdGlvbiwgaXQganVzdCBoYXBwZW5lZCBieSBpdHNlbGYgc29tZXdoZXJlIGR1cmlu
ZyB0ZXN0aW5nLiAgTXkgYXR0ZW1wdHMgYXQgY2FwdHVyaW5nIGl0IHdpdGggd2lyZXNoYXJrIHVz
dWFsbHkgc2xvdyBkb3duIG15IHN5c3RlbSBhcyB3aXJlc2hhcmsgdHJpZXMgdG8gZGlzcGxheSAx
LDAwMCwwMDArIHBhY2tldHMuLi4gSSdsbCB0cnkgYWdhaW4sIHRob3VnaC4NCj4gPiANCj4gPiBT
ZWUgaWYgeW91IGNhbiBqdXN0IHR1cm4gb24gdGhlIHR3byBkcHJpbnRrcygpIGluIHhzX3RjcF9z
dGF0ZV9jaGFuZ2UoKSwNCj4gPiBhbmQgdGhlbiBhZGQgYSBwcmludGsoKSB0cmlnZ2VyIHRvIHRo
ZQ0KPiA+IG5mczQxX2hhbmRsZV9zZXF1ZW5jZV9mbGFnX2Vycm9ycygpIHRvIHNlZSBpZiB5b3Ug
Y2FuIGNhdGNoIGENCj4gPiBjb3JyZWxhdGlvbiBiZXR3ZWVuIGEgVENQIHN0YXRlIGNoYW5nZSBh
bmQgdGhlIHBhdGhfZG93biBpc3N1ZS4NCj4gDQo+IFNvdW5kcyBzaW1wbGUgZW5vdWdoLiAgSSds
bCBkbyB0aGF0IG5vdy4uLg0KPiANCj4gPiANCj4gPj4+DQo+ID4+Pj4gSSBmb3VuZCB0aGF0IGJp
bmRfY29ubmVjdGlvbl90b19zZXNzaW9uIHNldHMgdGhlIE5GUzRfU0VTU0lPTl9EUkFJTklORw0K
PiA+Pj4+IGZsYWcgb24gdGhlIGNsaWVudCwgYnV0IHRoaXMgZmxhZyBpcyBuZXZlciB1bnNldCBi
ZWZvcmUNCj4gPj4+PiBuZnM0X2NoZWNrX2xlYXNlKCkgcmVhY2hlcyBuZnM0X3Byb2Nfc2VxdWVu
Y2UoKS4gIFRoaXMgY2F1c2VzIHRoZSBjbGllbnQNCj4gPj4+PiB0byBkZWFkbG9jaywgaGFsdGlu
ZyBhbGwgTkZTIGFjdGl2aXR5IHRvIHRoZSBzZXJ2ZXIuDQo+ID4+Pg0KPiA+Pj4gT3RoZXJ3aXNl
LCB0aGUgdGV4dCBpcyBtb3JlIG9yIGxlc3MgT0suIFRoZSBvbmUgdGhpbmcgdGhhdCBJJ20gbWlz
c2luZw0KPiA+Pj4gaXMgYSBzdGF0ZW1lbnQgYWJvdXQgd2h5IG5mczRfcHJvY19zZXF1ZW5jZSgp
IG5lZWRzIHRvIGJlIGEgcHJpdmlsZWdlZA0KPiA+Pj4gb3BlcmF0aW9uLg0KPiA+Pj4NCj4gPj4N
Cj4gPj4gSGVyZSBpcyB0aGUgbmV3IGxhc3QgcGFyYWdyYXBoLCBJIGp1c3QgYWRkZWQgdGhlIHNl
bnRlbmNlIGF0IHRoZSBlbmQ6DQo+ID4+DQo+ID4+IEkgZm91bmQgdGhhdCBiaW5kX2Nvbm5lY3Rp
b25fdG9fc2Vzc2lvbiBzZXRzIHRoZSBORlM0X1NFU1NJT05fRFJBSU5JTkcNCj4gPj4gZmxhZyBv
biB0aGUgY2xpZW50LCBidXQgdGhpcyBmbGFnIGlzIG5ldmVyIHVuc2V0IGJlZm9yZQ0KPiA+PiBu
ZnM0X2NoZWNrX2xlYXNlKCkgcmVhY2hlcyBuZnM0X3Byb2Nfc2VxdWVuY2UoKS4gIFRoaXMgY2F1
c2VzIHRoZSBjbGllbnQNCj4gPj4gdG8gZGVhZGxvY2ssIGhhbHRpbmcgYWxsIE5GUyBhY3Rpdml0
eSB0byB0aGUgc2VydmVyLiAgVGhpcyBwYXRjaCBjaGFuZ2VzDQo+ID4+IG5mczRfcHJvY19zZXF1
ZW5jZSgpIHRvIHJ1biBpbiBwcml2aWxlZ2VkIG1vZGUgdG8gYnlwYXNzIHRoZQ0KPiA+PiBORlM0
X1NFU1NJT05fRFJBSU5JTkcgY2hlY2sgYW5kIGNvbnRpbnVlIHdpdGggcmVjb3ZlcnkuDQo+ID4g
DQo+ID4gVGhhbmtzLiBZb3UgbWlnaHQgd2FudCB0byBhZGQgYSBub3RlIHRvIHRoZSBmYWN0IHRo
YXQNCj4gPiBuZnM0X3Byb2Nfc2VxdWVuY2UoKSBpcyBhbHdheXMgcnVuIGZyb20gdGhlIHN0YXRl
IHJlY292ZXJ5IHRocmVhZCwgYW5kDQo+ID4gdGhlcmVmb3JlIGl0IGlzIGNvcnJlY3QgdG8gcnVu
IGl0IGFzIGEgcHJpdmlsZWdlZCBvcGVyYXRpb24uDQo+ID4gDQo+IA0KPiBPay4gIERvIHlvdSB3
YW50IGEgbmV3IHBhdGNoPyAgSnVzdCB0aGUgY29tbWl0IHRleHQ/DQo+IA0KSnVzdCBzZW5kIHRo
ZSB3aG9sZSBwYXRjaCwgcGxlYXNlLg0KDQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZT
IGNsaWVudCBtYWludGFpbmVyDQoNCk5ldEFwcA0KVHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20N
Cnd3dy5uZXRhcHAuY29tDQo=

Re: [PATCH 1/2] NFS: Add sequence_priviliged_ops for nfs4_proc_sequence()

[Bryan Schumaker]

On 11/12/2012 04:37 PM, Myklebust, Trond wrote:
> On Mon, 2012-11-12 at 16:26 -0500, Bryan Schumaker wrote:
>> On 11/12/2012 04:10 PM, Myklebust, Trond wrote:
>>> On Mon, 2012-11-12 at 16:02 -0500, Bryan Schumaker wrote:
>>>> On 11/12/2012 03:54 PM, Myklebust, Trond wrote:
>>>>> On Mon, 2012-11-12 at 15:49 -0500, Andy Adamson wrote:
>>>>>> On Mon, Nov 12, 2012 at 3:29 PM, Myklebust, Trond
>>>>>> <Trond.Myklebust@netapp.com> wrote:
>>>>>>> On Mon, 2012-11-12 at 15:22 -0500, Andy Adamson wrote:
>>>>>>>> On Mon, Nov 12, 2012 at 1:35 PM,  <bjschuma@netapp.com> wrote:
>>>>>>>>> From: Bryan Schumaker <bjschuma@netapp.com>
>>>>>>>>>
>>>>>>>>> During recovery the NFS4_SESSION_DRAINING flag might be set on the
>>>>>>>>> client structure.  This can cause lease renewal to abort when
>>>>>>>
>>>>>>> Not lease renewal. It is lease verification (i.e. checking that we have
>>>>>>> a valid lease and session) that will deadlock.
>>>>>>>
>>>>>>>>> nfs41_setup_sequence sees that we are doing recovery.  As a result, the
>>>>>>>>> client never recovers and all activity with the NFS server halts.
>>>>>>>>
>>>>>>>>
>>>>>>>> When does this happen? Say the session is draining, and an RPC returns
>>>>>>>> from one of the compounds such as nfs_open, nfs_locku etc whose
>>>>>>>> rpc_call_done routine calls renew_lease after freeing it's slot.  Like
>>>>>>>> all calls on a draining session, the renew_lease sleeps on the session
>>>>>>>> slot_tbl_waitq - is this what you mean by "causes lease renewal to
>>>>>>>> abort"? How does this cause the client to never recover?
>>>>>>>>
>>>>>>>> The only other call to renew_lease is from the state manager itself
>>>>>>>> which runs one function at a time, and will not call renew_lease until
>>>>>>>> the recovery is over....
>>>>>>>>
>>>>>>>> What am I missing....?
>>>>>>>
>>>>>>> nfs4_check_lease() is run exclusively by the state manager thread in
>>>>>>> order to check that the clientid (and session id on NFSv4.1) are valid.
>>>>>>> It will deadlock the state manager thread if NFS4_SESSION_DRAINING is
>>>>>>> already set.
>>>>>>
>>>>>> OK. NFS4_SESSION_DRAINING is also set exclusively by the state manager
>>>>>> thread. Why is the state manager told to check the lease when it's
>>>>>> also draining a session?
>>>>>
>>>>> NFS4_SESSION_DRAINING does not just mean that the session is being
>>>>> drained; it remains set until open and lock state recovery is completed.
>>>>>
>>>>> IOW: if something happens during state recovery that requires us to
>>>>> check the lease (e.g. something returns NFS4ERR_EXPIRED) then the
>>>>> current code will deadlock.
>>>>>
>>>>>> No matter what the answer, please update the patch description to
>>>>>> better explain the problem being solved.
>>>>>
>>>>> ACK. I agree that the changelog entry can be improved.
>>>>>
>>>>
>>>> Does this read any better (and should I resend the patch)?
>>>>
>>>> NFS: Add sequence_priviliged_ops for nfs4_proc_sequence()
>>>>
>>>> If I mount an NFS v4.1 server to a single client multiple times and then
>>>> run xfstests over each mountpoint I usually get the client into a state
>>>> where recovery deadlocks.  The server informs the client of a
>>>> cb_path_down sequence error, the client then does a
>>>> bind_connection_to_session and checks the status of the lease.
>>>
>>> Why are you getting the cb_path_down? Is that a result of a fault
>>> injection, or is it a genuine error?
>>>
>>> While cb_path_down is a valid error, and we do want the recovery to work
>>> correctly, I would expect it to be rare since it implies that we lost
>>> the TCP connection to the server for some reason. Finding out why it
>>> happens is therefore worth doing.
>>
>> I didn't get it with fault injection, it just happened by itself somewhere during testing.  My attempts at capturing it with wireshark usually slow down my system as wireshark tries to display 1,000,000+ packets... I'll try again, though.
> 
> See if you can just turn on the two dprintks() in xs_tcp_state_change(),
> and then add a printk() trigger to the
> nfs41_handle_sequence_flag_errors() to see if you can catch a
> correlation between a TCP state change and the path_down issue.

I saw this in the logs, but the printk I added to nfs41_handle_sequence_flag_errors() wasn't hit.  Maybe that was something hit by waiting read / write requests that were stacked up?  I know that a few read_prepare() and write_prepare() requests were stacked up... but I don't remember seeing the "Callback slot table overflowed" message last week.

[  287.951307] Callback slot table overflowed
[  287.951540] RPC:       xs_tcp_state_change client ffff88003d7cc000...
[  287.951542] RPC:       state 4 conn 1 dead 0 zapped 1 sk_shutdown 2
[  287.951550] RPC: Could not send backchannel reply error: -32
[  287.951746] RPC:       xs_tcp_state_change client ffff88003d7cc000...
[  287.951748] RPC:       state 5 conn 0 dead 0 zapped 1 sk_shutdown 2
[  287.951759] RPC:       xs_tcp_state_change client ffff88003d7cc000...
[  287.951761] RPC:       state 7 conn 0 dead 0 zapped 1 sk_shutdown 3
[  287.951762] RPC:       xs_tcp_state_change client ffff88003d7cc000...
[  287.951763] RPC:       state 7 conn 0 dead 0 zapped 1 sk_shutdown 3
[  287.951978] RPC:       xs_tcp_state_change client ffff88003d7cc000...
[  287.951981] RPC:       state 1 conn 0 dead 0 zapped 1 sk_shutdown 0

- Bryan


> 
>>>
>>>> I found that bind_connection_to_session sets the NFS4_SESSION_DRAINING
>>>> flag on the client, but this flag is never unset before
>>>> nfs4_check_lease() reaches nfs4_proc_sequence().  This causes the client
>>>> to deadlock, halting all NFS activity to the server.
>>>
>>> Otherwise, the text is more or less OK. The one thing that I'm missing
>>> is a statement about why nfs4_proc_sequence() needs to be a privileged
>>> operation.
>>>
>>
>> Here is the new last paragraph, I just added the sentence at the end:
>>
>> I found that bind_connection_to_session sets the NFS4_SESSION_DRAINING
>> flag on the client, but this flag is never unset before
>> nfs4_check_lease() reaches nfs4_proc_sequence().  This causes the client
>> to deadlock, halting all NFS activity to the server.  This patch changes
>> nfs4_proc_sequence() to run in privileged mode to bypass the
>> NFS4_SESSION_DRAINING check and continue with recovery.
> 
> Thanks. You might want to add a note to the fact that
> nfs4_proc_sequence() is always run from the state recovery thread, and
> therefore it is correct to run it as a privileged operation.
>