Question on nfs40_discover_server_trunking.

Question on nfs40_discover_server_trunking.

[Ben Greear]

Any chance the STALE_CLIENTID case needs a 'break'?

Twice I've seen kernel crashes after the nfs40_walk_client_list
failed (though code comments say it should never fail).



int nfs40_discover_server_trunking(struct nfs_client *clp,
				   struct nfs_client **result,
				   struct rpc_cred *cred)
{
	struct nfs4_setclientid_res clid = {
		.clientid = clp->cl_clientid,
		.confirm = clp->cl_confirm,
	};
	struct nfs_net *nn = net_generic(clp->cl_net, nfs_net_id);
	unsigned short port;
	int status;

	port = nn->nfs_callback_tcpport;
	if (clp->cl_addr.ss_family == AF_INET6)
		port = nn->nfs_callback_tcpport6;

	status = nfs4_proc_setclientid(clp, NFS4_CALLBACK, port, cred, &clid);
	if (status != 0)
		goto out;
	clp->cl_clientid = clid.clientid;
	clp->cl_confirm = clid.confirm;

	status = nfs40_walk_client_list(clp, result, cred);
	switch (status) {
	case -NFS4ERR_STALE_CLIENTID:
		set_bit(NFS4CLNT_LEASE_CONFIRM, &clp->cl_state);
	case 0:
		/* Sustain the lease, even if it's empty.  If the clientid4
		 * goes stale it's of no use for trunking discovery. */
		nfs4_schedule_state_renewal(*result);
		break;
	}

out:
	return status;
}

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Question on nfs40_discover_server_trunking.

[Chuck Lever]

On Jan 18, 2013, at 4:28 PM, Ben Greear <greearb@candelatech.com> wrote:

> Any chance the STALE_CLIENTID case needs a 'break'?

I don't think so.  LEASE_CONFIRM is set, and we want to wake the state renewal thread.

> 
> Twice I've seen kernel crashes after the nfs40_walk_client_list
> failed (though code comments say it should never fail).

nfs40_walk_client_list() is looking for an nfs_client that is supposed to already be in the nfs_client list.  If the search fails, that's a bug.

Eyeball the contents of your nfs_client list.  You should find an appropriate nfs_client in there, and then figure out why the search doesn't find it.


> int nfs40_discover_server_trunking(struct nfs_client *clp,
> 				   struct nfs_client **result,
> 				   struct rpc_cred *cred)
> {
> 	struct nfs4_setclientid_res clid = {
> 		.clientid = clp->cl_clientid,
> 		.confirm = clp->cl_confirm,
> 	};
> 	struct nfs_net *nn = net_generic(clp->cl_net, nfs_net_id);
> 	unsigned short port;
> 	int status;
> 
> 	port = nn->nfs_callback_tcpport;
> 	if (clp->cl_addr.ss_family == AF_INET6)
> 		port = nn->nfs_callback_tcpport6;
> 
> 	status = nfs4_proc_setclientid(clp, NFS4_CALLBACK, port, cred, &clid);
> 	if (status != 0)
> 		goto out;
> 	clp->cl_clientid = clid.clientid;
> 	clp->cl_confirm = clid.confirm;
> 
> 	status = nfs40_walk_client_list(clp, result, cred);
> 	switch (status) {
> 	case -NFS4ERR_STALE_CLIENTID:
> 		set_bit(NFS4CLNT_LEASE_CONFIRM, &clp->cl_state);
> 	case 0:
> 		/* Sustain the lease, even if it's empty.  If the clientid4
> 		 * goes stale it's of no use for trunking discovery. */
> 		nfs4_schedule_state_renewal(*result);
> 		break;
> 	}
> 
> out:
> 	return status;
> }
> 
> -- 
> Ben Greear <greearb@candelatech.com>
> Candela Technologies Inc  http://www.candelatech.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

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: Question on nfs40_discover_server_trunking.

[Ben Greear]

On 01/18/2013 01:33 PM, Chuck Lever wrote:
>
> On Jan 18, 2013, at 4:28 PM, Ben Greear <greearb@candelatech.com> wrote:
>
>> Any chance the STALE_CLIENTID case needs a 'break'?
>
> I don't think so.  LEASE_CONFIRM is set, and we want to wake the state renewal thread.
>
>>
>> Twice I've seen kernel crashes after the nfs40_walk_client_list
>> failed (though code comments say it should never fail).
>
> nfs40_walk_client_list() is looking for an nfs_client that is supposed to already be in the nfs_client list.  If the search fails, that's a bug.
>
> Eyeball the contents of your nfs_client list.  You should find an appropriate nfs_client in there, and then figure out why the search doesn't find it.

Ok, I'll keep poking at the code.  My test case is 3000 individual mounts,
and the system is working very hard, so not sure eyeballing printk's is
going to help a whole lot :)

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Question on nfs40_discover_server_trunking.

[Ben Greear]

On 01/18/2013 01:33 PM, Chuck Lever wrote:
>
> On Jan 18, 2013, at 4:28 PM, Ben Greear <greearb@candelatech.com> wrote:
>
>> Any chance the STALE_CLIENTID case needs a 'break'?
>
> I don't think so.  LEASE_CONFIRM is set, and we want to wake the state renewal thread.
>
>>
>> Twice I've seen kernel crashes after the nfs40_walk_client_list
>> failed (though code comments say it should never fail).
>
> nfs40_walk_client_list() is looking for an nfs_client that is supposed to already be in the nfs_client list.  If the search fails, that's a bug.
>
> Eyeball the contents of your nfs_client list.  You should find an appropriate nfs_client in there, and then figure out why the search doesn't find it.

Ok, I think I found another issue.

nfs4_client_init does not initialize 'old', but passes it to nfs4_discover_server_trunking.

That gets passed to the detect_trunking operation, which is nfs40_discover_server_trunking
or nfs41_discover_server_trunking.

This will call walk_client_list, which also may not ever assign a value to
'result'.

The code in walk_client_list always dereferences result, however.

So, that is probably why my system blows up shortly after the 'impossible'
error message...

Maybe initialize result to NULL in nfs4[10]_walk_client_list and properly check
for null result in calling code?

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Question on nfs40_discover_server_trunking.

[Chuck Lever]

On Jan 18, 2013, at 4:59 PM, Ben Greear <greearb@candelatech.com> wrote:

> On 01/18/2013 01:33 PM, Chuck Lever wrote:
>> 
>> On Jan 18, 2013, at 4:28 PM, Ben Greear <greearb@candelatech.com> wrote:
>> 
>>> Any chance the STALE_CLIENTID case needs a 'break'?
>> 
>> I don't think so.  LEASE_CONFIRM is set, and we want to wake the state renewal thread.
>> 
>>> 
>>> Twice I've seen kernel crashes after the nfs40_walk_client_list
>>> failed (though code comments say it should never fail).
>> 
>> nfs40_walk_client_list() is looking for an nfs_client that is supposed to already be in the nfs_client list.  If the search fails, that's a bug.
>> 
>> Eyeball the contents of your nfs_client list.  You should find an appropriate nfs_client in there, and then figure out why the search doesn't find it.
> 
> Ok, I think I found another issue.
> 
> nfs4_client_init does not initialize 'old', but passes it to nfs4_discover_server_trunking.
> 
> That gets passed to the detect_trunking operation, which is nfs40_discover_server_trunking
> or nfs41_discover_server_trunking.

Yes, *result is an output parameter, not an input parameter.

> This will call walk_client_list, which also may not ever assign a value to
> 'result'.

It should always assign *result in the SUCCESS case.

> The code in walk_client_list always dereferences result, however.
> 
> So, that is probably why my system blows up shortly after the 'impossible'
> error message...

In particular, nfs4_walk_client_list() does not set *result when it returns NFS4ERR_STALE_CLIENTID.  In nfs4_discover_server_trunking(), should we therefore have this:

     status = nfs40_walk_client_list(clp, result, cred);
     switch (status) {
     case -NFS4ERR_STALE_CLIENTID:
             set_bit(NFS4CLNT_LEASE_CONFIRM, &clp->cl_state);
>>           nfs4_schedule_state_renewal(clp);
>>           break;
     case 0:

I'm not sure the nfs4_schedule_state_renewal() call is necessary here.

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: Question on nfs40_discover_server_trunking.

[Ben Greear]

On 01/18/2013 02:29 PM, Chuck Lever wrote:
>
> On Jan 18, 2013, at 4:59 PM, Ben Greear <greearb@candelatech.com> wrote:
>
>> On 01/18/2013 01:33 PM, Chuck Lever wrote:
>>>
>>> On Jan 18, 2013, at 4:28 PM, Ben Greear <greearb@candelatech.com> wrote:
>>>
>>>> Any chance the STALE_CLIENTID case needs a 'break'?
>>>
>>> I don't think so.  LEASE_CONFIRM is set, and we want to wake the state renewal thread.
>>>
>>>>
>>>> Twice I've seen kernel crashes after the nfs40_walk_client_list
>>>> failed (though code comments say it should never fail).
>>>
>>> nfs40_walk_client_list() is looking for an nfs_client that is supposed to already be in the nfs_client list.  If the search fails, that's a bug.
>>>
>>> Eyeball the contents of your nfs_client list.  You should find an appropriate nfs_client in there, and then figure out why the search doesn't find it.
>>
>> Ok, I think I found another issue.
>>
>> nfs4_client_init does not initialize 'old', but passes it to nfs4_discover_server_trunking.
>>
>> That gets passed to the detect_trunking operation, which is nfs40_discover_server_trunking
>> or nfs41_discover_server_trunking.
>
> Yes, *result is an output parameter, not an input parameter.
>
>> This will call walk_client_list, which also may not ever assign a value to
>> 'result'.
>
> It should always assign *result in the SUCCESS case.
>
>> The code in walk_client_list always dereferences result, however.
>>
>> So, that is probably why my system blows up shortly after the 'impossible'
>> error message...
>
> In particular, nfs4_walk_client_list() does not set *result when it returns NFS4ERR_STALE_CLIENTID.  In nfs4_discover_server_trunking(), should we therefore have this:
>
>       status = nfs40_walk_client_list(clp, result, cred);
>       switch (status) {
>       case -NFS4ERR_STALE_CLIENTID:
>               set_bit(NFS4CLNT_LEASE_CONFIRM, &clp->cl_state);
>>>            nfs4_schedule_state_renewal(clp);
>>>            break;
>       case 0:
>
> I'm not sure the nfs4_schedule_state_renewal() call is necessary here.


I'm not sure I follow you entirely..but I'm going to start testing this patch in
a minute.  Looks like it should fix my crash, and maybe give clues about
why we get to the error case in the first place.


diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
index d6b39a9..3188283 100644
--- a/fs/nfs/nfs4client.c
+++ b/fs/nfs/nfs4client.c
@@ -307,6 +307,8 @@ int nfs40_walk_client_list(struct nfs_client *new,
         };
         int status;

+       *result = NULL;
+
         spin_lock(&nn->nfs_client_lock);
         list_for_each_entry_safe(pos, n, &nn->nfs_client_list, cl_share_link) {
                 /* If "pos" isn't marked ready, we can't trust the
@@ -364,6 +366,7 @@ int nfs40_walk_client_list(struct nfs_client *new,
                 nfs_put_client(prev);
         spin_unlock(&nn->nfs_client_lock);
         pr_err("NFS: %s Error: no matching nfs_client found\n", __func__);
+       WARN_ON(1);
         return -NFS4ERR_STALE_CLIENTID;
  }

@@ -433,6 +436,8 @@ int nfs41_walk_client_list(struct nfs_client *new,
         struct nfs_client *pos, *n, *prev = NULL;
         int error;

+       *result = NULL;
+
         spin_lock(&nn->nfs_client_lock);
         list_for_each_entry_safe(pos, n, &nn->nfs_client_list, cl_share_link) {
                 /* If "pos" isn't marked ready, we can't trust the
@@ -489,6 +494,7 @@ int nfs41_walk_client_list(struct nfs_client *new,
          */
         spin_unlock(&nn->nfs_client_lock);
         pr_err("NFS: %s Error: no matching nfs_client found\n", __func__);
+       WARN_ON(1);
         return -NFS4ERR_STALE_CLIENTID;
  }
  #endif /* CONFIG_NFS_V4_1 */
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index c351e6b..54d29e2 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -142,7 +142,8 @@ int nfs40_discover_server_trunking(struct nfs_client *clp,
         case 0:
                 /* Sustain the lease, even if it's empty.  If the clientid4
                  * goes stale it's of no use for trunking discovery. */
-               nfs4_schedule_state_renewal(*result);
+               if (*result)
+                       nfs4_schedule_state_renewal(*result);
                 break;
         }




-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Question on nfs40_discover_server_trunking.

[Chuck Lever]

On Jan 18, 2013, at 5:34 PM, Ben Greear <greearb@candelatech.com> wrote:

> On 01/18/2013 02:29 PM, Chuck Lever wrote:
>> 
>> On Jan 18, 2013, at 4:59 PM, Ben Greear <greearb@candelatech.com> wrote:
>> 
>>> On 01/18/2013 01:33 PM, Chuck Lever wrote:
>>>> 
>>>> On Jan 18, 2013, at 4:28 PM, Ben Greear <greearb@candelatech.com> wrote:
>>>> 
>>>>> Any chance the STALE_CLIENTID case needs a 'break'?
>>>> 
>>>> I don't think so.  LEASE_CONFIRM is set, and we want to wake the state renewal thread.
>>>> 
>>>>> 
>>>>> Twice I've seen kernel crashes after the nfs40_walk_client_list
>>>>> failed (though code comments say it should never fail).
>>>> 
>>>> nfs40_walk_client_list() is looking for an nfs_client that is supposed to already be in the nfs_client list.  If the search fails, that's a bug.
>>>> 
>>>> Eyeball the contents of your nfs_client list.  You should find an appropriate nfs_client in there, and then figure out why the search doesn't find it.
>>> 
>>> Ok, I think I found another issue.
>>> 
>>> nfs4_client_init does not initialize 'old', but passes it to nfs4_discover_server_trunking.
>>> 
>>> That gets passed to the detect_trunking operation, which is nfs40_discover_server_trunking
>>> or nfs41_discover_server_trunking.
>> 
>> Yes, *result is an output parameter, not an input parameter.
>> 
>>> This will call walk_client_list, which also may not ever assign a value to
>>> 'result'.
>> 
>> It should always assign *result in the SUCCESS case.
>> 
>>> The code in walk_client_list always dereferences result, however.
>>> 
>>> So, that is probably why my system blows up shortly after the 'impossible'
>>> error message...
>> 
>> In particular, nfs4_walk_client_list() does not set *result when it returns NFS4ERR_STALE_CLIENTID.  In nfs4_discover_server_trunking(), should we therefore have this:
>> 
>>      status = nfs40_walk_client_list(clp, result, cred);
>>      switch (status) {
>>      case -NFS4ERR_STALE_CLIENTID:
>>              set_bit(NFS4CLNT_LEASE_CONFIRM, &clp->cl_state);
>>>>           nfs4_schedule_state_renewal(clp);
>>>>           break;
>>      case 0:
>> 
>> I'm not sure the nfs4_schedule_state_renewal() call is necessary here.
> 
> 
> I'm not sure I follow you entirely..but I'm going to start testing this patch in
> a minute.  Looks like it should fix my crash, and maybe give clues about
> why we get to the error case in the first place.
> 
> 
> diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
> index d6b39a9..3188283 100644
> --- a/fs/nfs/nfs4client.c
> +++ b/fs/nfs/nfs4client.c
> @@ -307,6 +307,8 @@ int nfs40_walk_client_list(struct nfs_client *new,
>        };
>        int status;
> 
> +       *result = NULL;
> +
>        spin_lock(&nn->nfs_client_lock);
>        list_for_each_entry_safe(pos, n, &nn->nfs_client_list, cl_share_link) {
>                /* If "pos" isn't marked ready, we can't trust the
> @@ -364,6 +366,7 @@ int nfs40_walk_client_list(struct nfs_client *new,
>                nfs_put_client(prev);
>        spin_unlock(&nn->nfs_client_lock);
>        pr_err("NFS: %s Error: no matching nfs_client found\n", __func__);
> +       WARN_ON(1);
>        return -NFS4ERR_STALE_CLIENTID;
> }
> 
> @@ -433,6 +436,8 @@ int nfs41_walk_client_list(struct nfs_client *new,
>        struct nfs_client *pos, *n, *prev = NULL;
>        int error;
> 
> +       *result = NULL;
> +

The design of this code is that output parameters should never be touched except in the "success" case.

>        spin_lock(&nn->nfs_client_lock);
>        list_for_each_entry_safe(pos, n, &nn->nfs_client_list, cl_share_link) {
>                /* If "pos" isn't marked ready, we can't trust the
> @@ -489,6 +494,7 @@ int nfs41_walk_client_list(struct nfs_client *new,
>         */
>        spin_unlock(&nn->nfs_client_lock);
>        pr_err("NFS: %s Error: no matching nfs_client found\n", __func__);
> +       WARN_ON(1);
>        return -NFS4ERR_STALE_CLIENTID;
> }
> #endif /* CONFIG_NFS_V4_1 */
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index c351e6b..54d29e2 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -142,7 +142,8 @@ int nfs40_discover_server_trunking(struct nfs_client *clp,
>        case 0:
>                /* Sustain the lease, even if it's empty.  If the clientid4
>                 * goes stale it's of no use for trunking discovery. */
> -               nfs4_schedule_state_renewal(*result);
> +               if (*result)
> +                       nfs4_schedule_state_renewal(*result);

In the STALE_CLIENTID case, this would do state renewal on the wrong nfs_client.

>                break;
>        }

Let's look at this again:

 138         status = nfs40_walk_client_list(clp, result, cred);
 139         switch (status) {
 140         case -NFS4ERR_STALE_CLIENTID:
 141                 set_bit(NFS4CLNT_LEASE_CONFIRM, &clp->cl_state);

This arm of the switch operates on "clp" not on "*result".  So we want to add:

       nfs4_schedule_state_renewal(clp);
       break;

That makes initializing *result above completely unnecessary.

 142         case 0:
 143                 /* Sustain the lease, even if it's empty.  If the clientid4
 144                  * goes stale it's of no use for trunking discovery. */
 145                 nfs4_schedule_state_renewal(*result);
 146                 break;
 147         }

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: Question on nfs40_discover_server_trunking.

[Ben Greear]

On 01/18/2013 02:43 PM, Chuck Lever wrote:
>
> On Jan 18, 2013, at 5:34 PM, Ben Greear <greearb@candelatech.com> wrote:
>
>> On 01/18/2013 02:29 PM, Chuck Lever wrote:
>>>
>>> On Jan 18, 2013, at 4:59 PM, Ben Greear <greearb@candelatech.com> wrote:
>>>
>>>> On 01/18/2013 01:33 PM, Chuck Lever wrote:
>>>>>
>>>>> On Jan 18, 2013, at 4:28 PM, Ben Greear <greearb@candelatech.com> wrote:
>>>>>
>>>>>> Any chance the STALE_CLIENTID case needs a 'break'?
>>>>>
>>>>> I don't think so.  LEASE_CONFIRM is set, and we want to wake the state renewal thread.
>>>>>
>>>>>>
>>>>>> Twice I've seen kernel crashes after the nfs40_walk_client_list
>>>>>> failed (though code comments say it should never fail).
>>>>>
>>>>> nfs40_walk_client_list() is looking for an nfs_client that is supposed to already be in the nfs_client list.  If the search fails, that's a bug.
>>>>>
>>>>> Eyeball the contents of your nfs_client list.  You should find an appropriate nfs_client in there, and then figure out why the search doesn't find it.
>>>>
>>>> Ok, I think I found another issue.
>>>>
>>>> nfs4_client_init does not initialize 'old', but passes it to nfs4_discover_server_trunking.
>>>>
>>>> That gets passed to the detect_trunking operation, which is nfs40_discover_server_trunking
>>>> or nfs41_discover_server_trunking.
>>>
>>> Yes, *result is an output parameter, not an input parameter.
>>>
>>>> This will call walk_client_list, which also may not ever assign a value to
>>>> 'result'.
>>>
>>> It should always assign *result in the SUCCESS case.
>>>
>>>> The code in walk_client_list always dereferences result, however.
>>>>
>>>> So, that is probably why my system blows up shortly after the 'impossible'
>>>> error message...
>>>
>>> In particular, nfs4_walk_client_list() does not set *result when it returns NFS4ERR_STALE_CLIENTID.  In nfs4_discover_server_trunking(), should we therefore have this:
>>>
>>>       status = nfs40_walk_client_list(clp, result, cred);
>>>       switch (status) {
>>>       case -NFS4ERR_STALE_CLIENTID:
>>>               set_bit(NFS4CLNT_LEASE_CONFIRM, &clp->cl_state);
>>>>>            nfs4_schedule_state_renewal(clp);
>>>>>            break;
>>>       case 0:
>>>
>>> I'm not sure the nfs4_schedule_state_renewal() call is necessary here.
>>
>>
>> I'm not sure I follow you entirely..but I'm going to start testing this patch in
>> a minute.  Looks like it should fix my crash, and maybe give clues about
>> why we get to the error case in the first place.
>>
>>
>> diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
>> index d6b39a9..3188283 100644
>> --- a/fs/nfs/nfs4client.c
>> +++ b/fs/nfs/nfs4client.c
>> @@ -307,6 +307,8 @@ int nfs40_walk_client_list(struct nfs_client *new,
>>         };
>>         int status;
>>
>> +       *result = NULL;
>> +
>>         spin_lock(&nn->nfs_client_lock);
>>         list_for_each_entry_safe(pos, n, &nn->nfs_client_list, cl_share_link) {
>>                 /* If "pos" isn't marked ready, we can't trust the
>> @@ -364,6 +366,7 @@ int nfs40_walk_client_list(struct nfs_client *new,
>>                 nfs_put_client(prev);
>>         spin_unlock(&nn->nfs_client_lock);
>>         pr_err("NFS: %s Error: no matching nfs_client found\n", __func__);
>> +       WARN_ON(1);
>>         return -NFS4ERR_STALE_CLIENTID;
>> }
>>
>> @@ -433,6 +436,8 @@ int nfs41_walk_client_list(struct nfs_client *new,
>>         struct nfs_client *pos, *n, *prev = NULL;
>>         int error;
>>
>> +       *result = NULL;
>> +
>
> The design of this code is that output parameters should never be touched except in the "success" case.
>
>>         spin_lock(&nn->nfs_client_lock);
>>         list_for_each_entry_safe(pos, n, &nn->nfs_client_list, cl_share_link) {
>>                 /* If "pos" isn't marked ready, we can't trust the
>> @@ -489,6 +494,7 @@ int nfs41_walk_client_list(struct nfs_client *new,
>>          */
>>         spin_unlock(&nn->nfs_client_lock);
>>         pr_err("NFS: %s Error: no matching nfs_client found\n", __func__);
>> +       WARN_ON(1);
>>         return -NFS4ERR_STALE_CLIENTID;
>> }
>> #endif /* CONFIG_NFS_V4_1 */
>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>> index c351e6b..54d29e2 100644
>> --- a/fs/nfs/nfs4state.c
>> +++ b/fs/nfs/nfs4state.c
>> @@ -142,7 +142,8 @@ int nfs40_discover_server_trunking(struct nfs_client *clp,
>>         case 0:
>>                 /* Sustain the lease, even if it's empty.  If the clientid4
>>                  * goes stale it's of no use for trunking discovery. */
>> -               nfs4_schedule_state_renewal(*result);
>> +               if (*result)
>> +                       nfs4_schedule_state_renewal(*result);
>
> In the STALE_CLIENTID case, this would do state renewal on the wrong nfs_client.
>
>>                 break;
>>         }
>
> Let's look at this again:
>
>   138         status = nfs40_walk_client_list(clp, result, cred);
>   139         switch (status) {
>   140         case -NFS4ERR_STALE_CLIENTID:
>   141                 set_bit(NFS4CLNT_LEASE_CONFIRM, &clp->cl_state);
>
> This arm of the switch operates on "clp" not on "*result".  So we want to add:
>
>         nfs4_schedule_state_renewal(clp);
>         break;
>
> That makes initializing *result above completely unnecessary.

Ok, I'll test that, but I still think the result pointer should be initialized
somewhere.  If a bug like this ever creeps in again, dereferencing a NULL
pointer should be a lot more obvious than dereferencing some random thing.


>
>   142         case 0:
>   143                 /* Sustain the lease, even if it's empty.  If the clientid4
>   144                  * goes stale it's of no use for trunking discovery. */
>   145                 nfs4_schedule_state_renewal(*result);
>   146                 break;
>   147         }
>


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Question on nfs40_discover_server_trunking.

[Ben Greear]

Is this OK, or is the NULL assignment just not acceptable?  Due to the
complexity of this code, I think it is a good idea to initialize
the variable...



[greearb@fs3 nfs]$ git diff
diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
index d6b39a9..cdc99bd 100644
--- a/fs/nfs/nfs4client.c
+++ b/fs/nfs/nfs4client.c
@@ -185,7 +185,7 @@ struct nfs_client *nfs4_init_client(struct nfs_client *clp,
                                     rpc_authflavor_t authflavour)
  {
         char buf[INET6_ADDRSTRLEN + 1];
-       struct nfs_client *old;
+       struct nfs_client *old = NULL;
         int error;

         if (clp->cl_cons_state == NFS_CS_READY) {
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index c351e6b..7103617 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -139,6 +139,8 @@ int nfs40_discover_server_trunking(struct nfs_client *clp,
         switch (status) {
         case -NFS4ERR_STALE_CLIENTID:
                 set_bit(NFS4CLNT_LEASE_CONFIRM, &clp->cl_state);
+               nfs4_schedule_state_renewal(clp);
+               break;
         case 0:
                 /* Sustain the lease, even if it's empty.  If the clientid4
                  * goes stale it's of no use for trunking discovery. */
[greearb@fs3 nfs]$


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Question on nfs40_discover_server_trunking.

[Myklebust, Trond]

T24gRnJpLCAyMDEzLTAxLTE4IGF0IDE2OjMzIC0wNTAwLCBDaHVjayBMZXZlciB3cm90ZToNCj4g
T24gSmFuIDE4LCAyMDEzLCBhdCA0OjI4IFBNLCBCZW4gR3JlZWFyIDxncmVlYXJiQGNhbmRlbGF0
ZWNoLmNvbT4gd3JvdGU6DQo+IA0KPiA+IEFueSBjaGFuY2UgdGhlIFNUQUxFX0NMSUVOVElEIGNh
c2UgbmVlZHMgYSAnYnJlYWsnPw0KPiANCj4gSSBkb24ndCB0aGluayBzby4gIExFQVNFX0NPTkZJ
Uk0gaXMgc2V0LCBhbmQgd2Ugd2FudCB0byB3YWtlIHRoZSBzdGF0ZSByZW5ld2FsIHRocmVhZC4N
Cj4gDQo+ID4gDQo+ID4gVHdpY2UgSSd2ZSBzZWVuIGtlcm5lbCBjcmFzaGVzIGFmdGVyIHRoZSBu
ZnM0MF93YWxrX2NsaWVudF9saXN0DQo+ID4gZmFpbGVkICh0aG91Z2ggY29kZSBjb21tZW50cyBz
YXkgaXQgc2hvdWxkIG5ldmVyIGZhaWwpLg0KPiANCj4gbmZzNDBfd2Fsa19jbGllbnRfbGlzdCgp
IGlzIGxvb2tpbmcgZm9yIGFuIG5mc19jbGllbnQgdGhhdCBpcyBzdXBwb3NlZCB0byBhbHJlYWR5
IGJlIGluIHRoZSBuZnNfY2xpZW50IGxpc3QuICBJZiB0aGUgc2VhcmNoIGZhaWxzLCB0aGF0J3Mg
YSBidWcuDQo+IA0KPiBFeWViYWxsIHRoZSBjb250ZW50cyBvZiB5b3VyIG5mc19jbGllbnQgbGlz
dC4gIFlvdSBzaG91bGQgZmluZCBhbiBhcHByb3ByaWF0ZSBuZnNfY2xpZW50IGluIHRoZXJlLCBh
bmQgdGhlbiBmaWd1cmUgb3V0IHdoeSB0aGUgc2VhcmNoIGRvZXNuJ3QgZmluZCBpdC4NCg0KWW91
IGhhdmUgY29uc2lkZXJlZCB0aGUgZmFjdCB0aGF0IHRoZSBjYWxsIHRvDQpuZnM0X3Byb2Nfc2V0
Y2xpZW50aWRfY29uZmlybSBjYW4gcG90ZW50aWFsbHkgcmV0dXJuDQpORlM0RVJSX1NUQUxFX0NM
SUVOVElEIGlmIHRoZSBzZXJ2ZXIgcmVib290ZWQgd2hpbGUgdGhlIGNsaWVudCB3YXMNCndhbGtp
bmcgdGhlIGxpc3Q/DQoNCkVpdGhlciB3YXksIHdlIHNob3VsZG4ndCBiZSBkZWxpYmVyYXRlbHkg
cGFzc2luZyBhbiB1bmluaXRpYWxpc2VkDQp2YXJpYWJsZSBhcyBhbiBhcmd1bWVudCB0byBuZnM0
X3NjaGVkdWxlX3N0YXRlX3JlbmV3YWwoKS4NCg0KDQotLSANClRyb25kIE15a2xlYnVzdA0KTGlu
dXggTkZTIGNsaWVudCBtYWludGFpbmVyDQoNCk5ldEFwcA0KVHJvbmQuTXlrbGVidXN0QG5ldGFw
cC5jb20NCnd3dy5uZXRhcHAuY29tDQo=

Re: Question on nfs40_discover_server_trunking.

[Myklebust, Trond]

[-- Attachment #1: Type: text/plain, Size: 1397 bytes --]

On Fri, 2013-01-18 at 17:03 -0500, an unknown sender wrote:
> On Fri, 2013-01-18 at 16:33 -0500, Chuck Lever wrote:
> > On Jan 18, 2013, at 4:28 PM, Ben Greear <greearb@candelatech.com> wrote:
> > 
> > > Any chance the STALE_CLIENTID case needs a 'break'?
> > 
> > I don't think so.  LEASE_CONFIRM is set, and we want to wake the state renewal thread.
> > 
> > > 
> > > Twice I've seen kernel crashes after the nfs40_walk_client_list
> > > failed (though code comments say it should never fail).
> > 
> > nfs40_walk_client_list() is looking for an nfs_client that is supposed to already be in the nfs_client list.  If the search fails, that's a bug.
> > 
> > Eyeball the contents of your nfs_client list.  You should find an appropriate nfs_client in there, and then figure out why the search doesn't find it.
> 
> You have considered the fact that the call to
> nfs4_proc_setclientid_confirm can potentially return
> NFS4ERR_STALE_CLIENTID if the server rebooted while the client was
> walking the list?

In fact, as far as I can see, the correct behaviour in
nfs40_discover_server_trunking() should be to re-issue the setclientid
call, and then walk the list again if nfs40_walk_client_list() returns
NFS4ERR_STALE_CLIENTID.

Something like the attached patch:

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-NFSv4-Fix-NFSv4.0-trunking-discovery.patch --]
[-- Type: text/x-patch; name="0001-NFSv4-Fix-NFSv4.0-trunking-discovery.patch", Size: 4018 bytes --]

From 8ae0dc0ae64c9913c90f550d3c6b724313883fff Mon Sep 17 00:00:00 2001
From: Trond Myklebust <Trond.Myklebust@netapp.com>
Date: Fri, 18 Jan 2013 17:54:34 -0500
Subject: [PATCH] NFSv4: Fix NFSv4.0 trunking discovery

If walking the list in nfs40_walk_client_list fails, then the most
likely explanation is that the server rebooted. Ensure that we catch
that case by testing our new nfs_client last.

Reported-by: Ben Greear <greearb@candelatech.com>
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
Cc: Chuck Lever <chuck.lever@oracle.com>
Cc: stable@vger.kernel.org [>=3.7]
---
 fs/nfs/nfs4client.c | 35 +++++++++++++++++------------------
 fs/nfs/nfs4state.c  | 22 ++++++++++------------
 2 files changed, 27 insertions(+), 30 deletions(-)

diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
index acc3472..596b303 100644
--- a/fs/nfs/nfs4client.c
+++ b/fs/nfs/nfs4client.c
@@ -310,6 +310,9 @@ int nfs40_walk_client_list(struct nfs_client *new,
 
 	spin_lock(&nn->nfs_client_lock);
 	list_for_each_entry_safe(pos, n, &nn->nfs_client_list, cl_share_link) {
+		/* Skip the new entry */
+		if (pos == new)
+			continue;
 		/* If "pos" isn't marked ready, we can't trust the
 		 * remaining fields in "pos" */
 		if (pos->cl_cons_state < NFS_CS_READY)
@@ -332,40 +335,36 @@ int nfs40_walk_client_list(struct nfs_client *new,
 
 		if (prev)
 			nfs_put_client(prev);
+		prev = pos;
 
 		status = nfs4_proc_setclientid_confirm(pos, &clid, cred);
-		if (status == 0) {
+		switch (status) {
+		case 0:
 			nfs4_swap_callback_idents(pos, new);
 
-			nfs_put_client(pos);
+			prev = NULL;
 			*result = pos;
 			dprintk("NFS: <-- %s using nfs_client = %p ({%d})\n",
 				__func__, pos, atomic_read(&pos->cl_count));
-			return 0;
-		}
-		if (status != -NFS4ERR_STALE_CLIENTID) {
-			nfs_put_client(pos);
-			dprintk("NFS: <-- %s status = %d, no result\n",
-				__func__, status);
-			return status;
+		default:
+			goto out;
+		case -NFS4ERR_STALE_CLIENTID:
 		}
 
 		spin_lock(&nn->nfs_client_lock);
-		prev = pos;
 	}
+	spin_unlock(&nn->nfs_client_lock);
 
 	/*
-	 * No matching nfs_client found.  This should be impossible,
-	 * because the new nfs_client has already been added to
-	 * nfs_client_list by nfs_get_client().
-	 *
-	 * Don't BUG(), since the caller is holding a mutex.
+	 * No match found. Try to confirm the clientid on "new".
 	 */
+	*result = new;
+	status = nfs4_proc_setclientid_confirm(new, &clid, cred);
+out:
 	if (prev)
 		nfs_put_client(prev);
-	spin_unlock(&nn->nfs_client_lock);
-	pr_err("NFS: %s Error: no matching nfs_client found\n", __func__);
-	return -NFS4ERR_STALE_CLIENTID;
+	dprintk("NFS: <-- %s status = %d\n", __func__, status);
+	return status;
 }
 
 #ifdef CONFIG_NFS_V4_1
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 9448c57..102885d 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -129,24 +129,22 @@ int nfs40_discover_server_trunking(struct nfs_client *clp,
 	if (clp->cl_addr.ss_family == AF_INET6)
 		port = nn->nfs_callback_tcpport6;
 
-	status = nfs4_proc_setclientid(clp, NFS4_CALLBACK, port, cred, &clid);
-	if (status != 0)
-		goto out;
-	clp->cl_clientid = clid.clientid;
-	clp->cl_confirm = clid.confirm;
+	do {
+		status = nfs4_proc_setclientid(clp, NFS4_CALLBACK, port, cred, &clid);
+		if (status != 0)
+			break;
+		clp->cl_clientid = clid.clientid;
+		clp->cl_confirm = clid.confirm;
 
-	status = nfs40_walk_client_list(clp, result, cred);
-	switch (status) {
-	case -NFS4ERR_STALE_CLIENTID:
-		set_bit(NFS4CLNT_LEASE_CONFIRM, &clp->cl_state);
-	case 0:
+		status = nfs40_walk_client_list(clp, result, cred);
+	} while (status == -NFS4ERR_STALE_CLIENTID);
+
+	if (status == 0) {
 		/* Sustain the lease, even if it's empty.  If the clientid4
 		 * goes stale it's of no use for trunking discovery. */
 		nfs4_schedule_state_renewal(*result);
-		break;
 	}
 
-out:
 	return status;
 }
 
-- 
1.8.1

Re: Question on nfs40_discover_server_trunking.

[Ben Greear]

On 01/18/2013 02:59 PM, Myklebust, Trond wrote:
> On Fri, 2013-01-18 at 17:03 -0500, an unknown sender wrote:
>> On Fri, 2013-01-18 at 16:33 -0500, Chuck Lever wrote:
>>> On Jan 18, 2013, at 4:28 PM, Ben Greear <greearb@candelatech.com> wrote:
>>>
>>>> Any chance the STALE_CLIENTID case needs a 'break'?
>>>
>>> I don't think so.  LEASE_CONFIRM is set, and we want to wake the state renewal thread.
>>>
>>>>
>>>> Twice I've seen kernel crashes after the nfs40_walk_client_list
>>>> failed (though code comments say it should never fail).
>>>
>>> nfs40_walk_client_list() is looking for an nfs_client that is supposed to already be in the nfs_client list.  If the search fails, that's a bug.
>>>
>>> Eyeball the contents of your nfs_client list.  You should find an appropriate nfs_client in there, and then figure out why the search doesn't find it.
>>
>> You have considered the fact that the call to
>> nfs4_proc_setclientid_confirm can potentially return
>> NFS4ERR_STALE_CLIENTID if the server rebooted while the client was
>> walking the list?
>
> In fact, as far as I can see, the correct behaviour in
> nfs40_discover_server_trunking() should be to re-issue the setclientid
> call, and then walk the list again if nfs40_walk_client_list() returns
> NFS4ERR_STALE_CLIENTID.

Thanks for the patch.  I'll add that to my tree as well.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Question on nfs40_discover_server_trunking.

[Chuck Lever]

On Jan 18, 2013, at 5:59 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:

> On Fri, 2013-01-18 at 17:03 -0500, an unknown sender wrote:
>> On Fri, 2013-01-18 at 16:33 -0500, Chuck Lever wrote:
>>> On Jan 18, 2013, at 4:28 PM, Ben Greear <greearb@candelatech.com> wrote:
>>> 
>>>> Any chance the STALE_CLIENTID case needs a 'break'?
>>> 
>>> I don't think so.  LEASE_CONFIRM is set, and we want to wake the state renewal thread.
>>> 
>>>> 
>>>> Twice I've seen kernel crashes after the nfs40_walk_client_list
>>>> failed (though code comments say it should never fail).
>>> 
>>> nfs40_walk_client_list() is looking for an nfs_client that is supposed to already be in the nfs_client list.  If the search fails, that's a bug.
>>> 
>>> Eyeball the contents of your nfs_client list.  You should find an appropriate nfs_client in there, and then figure out why the search doesn't find it.
>> 
>> You have considered the fact that the call to
>> nfs4_proc_setclientid_confirm can potentially return
>> NFS4ERR_STALE_CLIENTID if the server rebooted while the client was
>> walking the list?
> 
> In fact, as far as I can see, the correct behaviour in
> nfs40_discover_server_trunking() should be to re-issue the setclientid
> call, and then walk the list again if nfs40_walk_client_list() returns
> NFS4ERR_STALE_CLIENTID.

When I wrote the server trunking detection logic, I think we hadn't clearly decided what needed to be done in the STALE_CLIENTID case.

> Something like the attached patch:

A couple of comments:

 o  nfs_get_client() already sticks the new client on the tail of the nfs_client list

 o  We don't want to get stuck in a loop here.  Should the "do {}" loop in nfs40_discover_server_trunking() be bounded by a retry count?

However, I haven't heard Ben say "oh, yes, my server had rebooted."  I'd like some confirmation that the match failed for an explainable and expected reason.

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: Question on nfs40_discover_server_trunking.

[Ben Greear]

On 01/18/2013 03:14 PM, Chuck Lever wrote:
>
> On Jan 18, 2013, at 5:59 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:
>
>> On Fri, 2013-01-18 at 17:03 -0500, an unknown sender wrote:
>>> On Fri, 2013-01-18 at 16:33 -0500, Chuck Lever wrote:
>>>> On Jan 18, 2013, at 4:28 PM, Ben Greear <greearb@candelatech.com> wrote:
>>>>
>>>>> Any chance the STALE_CLIENTID case needs a 'break'?
>>>>
>>>> I don't think so.  LEASE_CONFIRM is set, and we want to wake the state renewal thread.
>>>>
>>>>>
>>>>> Twice I've seen kernel crashes after the nfs40_walk_client_list
>>>>> failed (though code comments say it should never fail).
>>>>
>>>> nfs40_walk_client_list() is looking for an nfs_client that is supposed to already be in the nfs_client list.  If the search fails, that's a bug.
>>>>
>>>> Eyeball the contents of your nfs_client list.  You should find an appropriate nfs_client in there, and then figure out why the search doesn't find it.
>>>
>>> You have considered the fact that the call to
>>> nfs4_proc_setclientid_confirm can potentially return
>>> NFS4ERR_STALE_CLIENTID if the server rebooted while the client was
>>> walking the list?
>>
>> In fact, as far as I can see, the correct behaviour in
>> nfs40_discover_server_trunking() should be to re-issue the setclientid
>> call, and then walk the list again if nfs40_walk_client_list() returns
>> NFS4ERR_STALE_CLIENTID.
>
> When I wrote the server trunking detection logic, I think we hadn't clearly decided what needed to be done in the STALE_CLIENTID case.
>
>> Something like the attached patch:
>
> A couple of comments:
>
>   o  nfs_get_client() already sticks the new client on the tail of the nfs_client list
>
>   o  We don't want to get stuck in a loop here.  Should the "do {}" loop in nfs40_discover_server_trunking() be bounded by a retry count?
>
> However, I haven't heard Ben say "oh, yes, my server had rebooted."  I'd like some confirmation that the match failed for an explainable and expected reason.

The server machine did not reboot, but it's badly overloaded,
trying to serve 3000 mount points that are
constantly being brought up and torn down while
NFS write traffic is going on.

Even with all this, I've seen this particular problem only twice in
around 2 days of solid testing (I've been optimizing my user-space
app, and the better it gets, the more kernel bugs I find!)

If you have some particular debug info you want printed in
the failure cause, I'll be happy to run with that.

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Question on nfs40_discover_server_trunking.

[Myklebust, Trond]

T24gRnJpLCAyMDEzLTAxLTE4IGF0IDE4OjE0IC0wNTAwLCBDaHVjayBMZXZlciB3cm90ZToNCj4g
T24gSmFuIDE4LCAyMDEzLCBhdCA1OjU5IFBNLCAiTXlrbGVidXN0LCBUcm9uZCIgPFRyb25kLk15
a2xlYnVzdEBuZXRhcHAuY29tPiB3cm90ZToNCj4gDQo+ID4gT24gRnJpLCAyMDEzLTAxLTE4IGF0
IDE3OjAzIC0wNTAwLCBhbiB1bmtub3duIHNlbmRlciB3cm90ZToNCj4gPj4gT24gRnJpLCAyMDEz
LTAxLTE4IGF0IDE2OjMzIC0wNTAwLCBDaHVjayBMZXZlciB3cm90ZToNCj4gPj4+IE9uIEphbiAx
OCwgMjAxMywgYXQgNDoyOCBQTSwgQmVuIEdyZWVhciA8Z3JlZWFyYkBjYW5kZWxhdGVjaC5jb20+
IHdyb3RlOg0KPiA+Pj4gDQo+ID4+Pj4gQW55IGNoYW5jZSB0aGUgU1RBTEVfQ0xJRU5USUQgY2Fz
ZSBuZWVkcyBhICdicmVhayc/DQo+ID4+PiANCj4gPj4+IEkgZG9uJ3QgdGhpbmsgc28uICBMRUFT
RV9DT05GSVJNIGlzIHNldCwgYW5kIHdlIHdhbnQgdG8gd2FrZSB0aGUgc3RhdGUgcmVuZXdhbCB0
aHJlYWQuDQo+ID4+PiANCj4gPj4+PiANCj4gPj4+PiBUd2ljZSBJJ3ZlIHNlZW4ga2VybmVsIGNy
YXNoZXMgYWZ0ZXIgdGhlIG5mczQwX3dhbGtfY2xpZW50X2xpc3QNCj4gPj4+PiBmYWlsZWQgKHRo
b3VnaCBjb2RlIGNvbW1lbnRzIHNheSBpdCBzaG91bGQgbmV2ZXIgZmFpbCkuDQo+ID4+PiANCj4g
Pj4+IG5mczQwX3dhbGtfY2xpZW50X2xpc3QoKSBpcyBsb29raW5nIGZvciBhbiBuZnNfY2xpZW50
IHRoYXQgaXMgc3VwcG9zZWQgdG8gYWxyZWFkeSBiZSBpbiB0aGUgbmZzX2NsaWVudCBsaXN0LiAg
SWYgdGhlIHNlYXJjaCBmYWlscywgdGhhdCdzIGEgYnVnLg0KPiA+Pj4gDQo+ID4+PiBFeWViYWxs
IHRoZSBjb250ZW50cyBvZiB5b3VyIG5mc19jbGllbnQgbGlzdC4gIFlvdSBzaG91bGQgZmluZCBh
biBhcHByb3ByaWF0ZSBuZnNfY2xpZW50IGluIHRoZXJlLCBhbmQgdGhlbiBmaWd1cmUgb3V0IHdo
eSB0aGUgc2VhcmNoIGRvZXNuJ3QgZmluZCBpdC4NCj4gPj4gDQo+ID4+IFlvdSBoYXZlIGNvbnNp
ZGVyZWQgdGhlIGZhY3QgdGhhdCB0aGUgY2FsbCB0bw0KPiA+PiBuZnM0X3Byb2Nfc2V0Y2xpZW50
aWRfY29uZmlybSBjYW4gcG90ZW50aWFsbHkgcmV0dXJuDQo+ID4+IE5GUzRFUlJfU1RBTEVfQ0xJ
RU5USUQgaWYgdGhlIHNlcnZlciByZWJvb3RlZCB3aGlsZSB0aGUgY2xpZW50IHdhcw0KPiA+PiB3
YWxraW5nIHRoZSBsaXN0Pw0KPiA+IA0KPiA+IEluIGZhY3QsIGFzIGZhciBhcyBJIGNhbiBzZWUs
IHRoZSBjb3JyZWN0IGJlaGF2aW91ciBpbg0KPiA+IG5mczQwX2Rpc2NvdmVyX3NlcnZlcl90cnVu
a2luZygpIHNob3VsZCBiZSB0byByZS1pc3N1ZSB0aGUgc2V0Y2xpZW50aWQNCj4gPiBjYWxsLCBh
bmQgdGhlbiB3YWxrIHRoZSBsaXN0IGFnYWluIGlmIG5mczQwX3dhbGtfY2xpZW50X2xpc3QoKSBy
ZXR1cm5zDQo+ID4gTkZTNEVSUl9TVEFMRV9DTElFTlRJRC4NCj4gDQo+IFdoZW4gSSB3cm90ZSB0
aGUgc2VydmVyIHRydW5raW5nIGRldGVjdGlvbiBsb2dpYywgSSB0aGluayB3ZSBoYWRuJ3QgY2xl
YXJseSBkZWNpZGVkIHdoYXQgbmVlZGVkIHRvIGJlIGRvbmUgaW4gdGhlIFNUQUxFX0NMSUVOVElE
IGNhc2UuDQo+IA0KPiA+IFNvbWV0aGluZyBsaWtlIHRoZSBhdHRhY2hlZCBwYXRjaDoNCj4gDQo+
IEEgY291cGxlIG9mIGNvbW1lbnRzOg0KPiANCj4gIG8gIG5mc19nZXRfY2xpZW50KCkgYWxyZWFk
eSBzdGlja3MgdGhlIG5ldyBjbGllbnQgb24gdGhlIHRhaWwgb2YgdGhlIG5mc19jbGllbnQgbGlz
dA0KDQpPSy4gVGhhdCBhbGxvd3MgdXMgdG8gZ2V0IHJpZCBvZiA1LTYgbGluZXMgb2YgY29kZS4N
Cg0KPiAgbyAgV2UgZG9uJ3Qgd2FudCB0byBnZXQgc3R1Y2sgaW4gYSBsb29wIGhlcmUuICBTaG91
bGQgdGhlICJkbyB7fSIgbG9vcCBpbiBuZnM0MF9kaXNjb3Zlcl9zZXJ2ZXJfdHJ1bmtpbmcoKSBi
ZSBib3VuZGVkIGJ5IGEgcmV0cnkgY291bnQ/DQoNCk5vdCBhIG51bWJlciBvZiByZXRyaWVzLCBi
dXQgcG9zc2libHkgYSB0aW1lb3V0IHZhbHVlLg0KDQo+IEhvd2V2ZXIsIEkgaGF2ZW4ndCBoZWFy
ZCBCZW4gc2F5ICJvaCwgeWVzLCBteSBzZXJ2ZXIgaGFkIHJlYm9vdGVkLiIgIEknZCBsaWtlIHNv
bWUgY29uZmlybWF0aW9uIHRoYXQgdGhlIG1hdGNoIGZhaWxlZCBmb3IgYW4gZXhwbGFpbmFibGUg
YW5kIGV4cGVjdGVkIHJlYXNvbi4NCg0KVGhlIHNwZWMgYWxsb3dzIHRoZSBzZXJ2ZXIgdG8gZGlz
Y2FyZCB1bmNvbmZpcm1lZCBjbGllbnRpZHMsIHNvIG91ciBjb2RlDQpzaG91bGQgdGFrZSB0aGF0
IGludG8gYWNjb3VudC4NCg0KDQpCVFc6IEknbSBnb2luZyBpbnNhbmUgdHJ5aW5nIHRvIGZpZ3Vy
ZSBvdXQgaG93IHRoZSByZWZjb3VudGluZyBpbiB0aGF0DQpjb2RlIGlzIHN1cHBvc2VkIHRvIHdv
cmsuIFRoZSBhc3N1bXB0aW9uIHRoYXQgd2UgY2FuIHNvbWVob3cgc2FmZWx5DQpyZXR1cm4gYSBu
b24tcmVmZXJlbmNlZCBwb2ludGVyIGZyb20gbmZzNF9kaXNjb3Zlcl9zZXJ2ZXJfdHJ1bmtpbmcg
YW5kDQp0aGVuIGJ1bXAgdGhlIHJlZmVyZW5jZSBjb3VudCBpbiBuZnM0X2luaXRfY2xpZW50KCkg
ZHJpdmVzIG1lIG51dHMuLi4NCg0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGll
bnQgbWFpbnRhaW5lcg0KDQpOZXRBcHANClRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tDQp3d3cu
bmV0YXBwLmNvbQ0K

Re: Question on nfs40_discover_server_trunking.

[Myklebust, Trond]

[-- Attachment #1: Type: text/plain, Size: 2920 bytes --]

On Fri, 2013-01-18 at 19:44 -0500, Trond Myklebust wrote:
> On Fri, 2013-01-18 at 18:14 -0500, Chuck Lever wrote:
> > On Jan 18, 2013, at 5:59 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:
> > 
> > > On Fri, 2013-01-18 at 17:03 -0500, an unknown sender wrote:
> > >> On Fri, 2013-01-18 at 16:33 -0500, Chuck Lever wrote:
> > >>> On Jan 18, 2013, at 4:28 PM, Ben Greear <greearb@candelatech.com> wrote:
> > >>> 
> > >>>> Any chance the STALE_CLIENTID case needs a 'break'?
> > >>> 
> > >>> I don't think so.  LEASE_CONFIRM is set, and we want to wake the state renewal thread.
> > >>> 
> > >>>> 
> > >>>> Twice I've seen kernel crashes after the nfs40_walk_client_list
> > >>>> failed (though code comments say it should never fail).
> > >>> 
> > >>> nfs40_walk_client_list() is looking for an nfs_client that is supposed to already be in the nfs_client list.  If the search fails, that's a bug.
> > >>> 
> > >>> Eyeball the contents of your nfs_client list.  You should find an appropriate nfs_client in there, and then figure out why the search doesn't find it.
> > >> 
> > >> You have considered the fact that the call to
> > >> nfs4_proc_setclientid_confirm can potentially return
> > >> NFS4ERR_STALE_CLIENTID if the server rebooted while the client was
> > >> walking the list?
> > > 
> > > In fact, as far as I can see, the correct behaviour in
> > > nfs40_discover_server_trunking() should be to re-issue the setclientid
> > > call, and then walk the list again if nfs40_walk_client_list() returns
> > > NFS4ERR_STALE_CLIENTID.
> > 
> > When I wrote the server trunking detection logic, I think we hadn't clearly decided what needed to be done in the STALE_CLIENTID case.
> > 
> > > Something like the attached patch:
> > 
> > A couple of comments:
> > 
> >  o  nfs_get_client() already sticks the new client on the tail of the nfs_client list
> 
> OK. That allows us to get rid of 5-6 lines of code.
> 
> >  o  We don't want to get stuck in a loop here.  Should the "do {}" loop in nfs40_discover_server_trunking() be bounded by a retry count?
> 
> Not a number of retries, but possibly a timeout value.
> 
> > However, I haven't heard Ben say "oh, yes, my server had rebooted."  I'd like some confirmation that the match failed for an explainable and expected reason.
> 
> The spec allows the server to discard unconfirmed clientids, so our code
> should take that into account.
> 
> 
> BTW: I'm going insane trying to figure out how the refcounting in that
> code is supposed to work. The assumption that we can somehow safely
> return a non-referenced pointer from nfs4_discover_server_trunking and
> then bump the reference count in nfs4_init_client() drives me nuts...

Attached is v2 of the patch that addresses all of the above.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-NFSv4-Fix-NFSv4.0-trunking-discovery.patch --]
[-- Type: text/x-patch; name="0001-NFSv4-Fix-NFSv4.0-trunking-discovery.patch", Size: 4371 bytes --]

From 5fdbe6129cc767b4d8926d4a53ce7ce075411156 Mon Sep 17 00:00:00 2001
From: Trond Myklebust <Trond.Myklebust@netapp.com>
Date: Fri, 18 Jan 2013 17:54:34 -0500
Subject: [PATCH v2] NFSv4: Fix NFSv4.0 trunking discovery

If walking the list in nfs40_walk_client_list fails, then the most
likely explanation is that the server rebooted. Ensure that we catch
that case by testing our new nfs_client last.

Also fix reference counting issues that could lead to a memory leak
for nfs_client structures.

Reported-by: Ben Greear <greearb@candelatech.com>
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
Cc: Chuck Lever <chuck.lever@oracle.com>
Cc: stable@vger.kernel.org [>=3.7]
---
 fs/nfs/nfs4client.c | 37 +++++++++++++++----------------------
 fs/nfs/nfs4state.c  | 22 ++++++++++------------
 2 files changed, 25 insertions(+), 34 deletions(-)

diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
index acc3472..bd6efe8 100644
--- a/fs/nfs/nfs4client.c
+++ b/fs/nfs/nfs4client.c
@@ -234,13 +234,12 @@ struct nfs_client *nfs4_init_client(struct nfs_client *clp,
 		nfs_mark_client_ready(clp, NFS_CS_READY);
 
 	error = nfs4_discover_server_trunking(clp, &old);
+	nfs_put_client(clp);
 	if (error < 0)
 		goto error;
 	if (clp != old) {
 		clp->cl_preserve_clid = true;
-		nfs_put_client(clp);
 		clp = old;
-		atomic_inc(&clp->cl_count);
 	}
 
 	return clp;
@@ -332,40 +331,33 @@ int nfs40_walk_client_list(struct nfs_client *new,
 
 		if (prev)
 			nfs_put_client(prev);
+		prev = pos;
 
 		status = nfs4_proc_setclientid_confirm(pos, &clid, cred);
-		if (status == 0) {
+		switch (status) {
+		case -NFS4ERR_STALE_CLIENTID:
+			break;
+		case 0:
 			nfs4_swap_callback_idents(pos, new);
 
-			nfs_put_client(pos);
+			prev = NULL;
 			*result = pos;
 			dprintk("NFS: <-- %s using nfs_client = %p ({%d})\n",
 				__func__, pos, atomic_read(&pos->cl_count));
-			return 0;
-		}
-		if (status != -NFS4ERR_STALE_CLIENTID) {
-			nfs_put_client(pos);
-			dprintk("NFS: <-- %s status = %d, no result\n",
-				__func__, status);
-			return status;
+		default:
+			goto out;
 		}
 
 		spin_lock(&nn->nfs_client_lock);
-		prev = pos;
 	}
+	spin_unlock(&nn->nfs_client_lock);
 
-	/*
-	 * No matching nfs_client found.  This should be impossible,
-	 * because the new nfs_client has already been added to
-	 * nfs_client_list by nfs_get_client().
-	 *
-	 * Don't BUG(), since the caller is holding a mutex.
-	 */
+	/* No match found. The server lost our clientid */
+out:
 	if (prev)
 		nfs_put_client(prev);
-	spin_unlock(&nn->nfs_client_lock);
-	pr_err("NFS: %s Error: no matching nfs_client found\n", __func__);
-	return -NFS4ERR_STALE_CLIENTID;
+	dprintk("NFS: <-- %s status = %d\n", __func__, status);
+	return status;
 }
 
 #ifdef CONFIG_NFS_V4_1
@@ -473,6 +465,7 @@ int nfs41_walk_client_list(struct nfs_client *new,
 		if (!nfs4_match_serverowners(pos, new))
 			continue;
 
+		atomic_inc(&pos->cl_count);
 		spin_unlock(&nn->nfs_client_lock);
 		dprintk("NFS: <-- %s using nfs_client = %p ({%d})\n",
 			__func__, pos, atomic_read(&pos->cl_count));
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 9448c57..102885d 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -129,24 +129,22 @@ int nfs40_discover_server_trunking(struct nfs_client *clp,
 	if (clp->cl_addr.ss_family == AF_INET6)
 		port = nn->nfs_callback_tcpport6;
 
-	status = nfs4_proc_setclientid(clp, NFS4_CALLBACK, port, cred, &clid);
-	if (status != 0)
-		goto out;
-	clp->cl_clientid = clid.clientid;
-	clp->cl_confirm = clid.confirm;
+	do {
+		status = nfs4_proc_setclientid(clp, NFS4_CALLBACK, port, cred, &clid);
+		if (status != 0)
+			break;
+		clp->cl_clientid = clid.clientid;
+		clp->cl_confirm = clid.confirm;
 
-	status = nfs40_walk_client_list(clp, result, cred);
-	switch (status) {
-	case -NFS4ERR_STALE_CLIENTID:
-		set_bit(NFS4CLNT_LEASE_CONFIRM, &clp->cl_state);
-	case 0:
+		status = nfs40_walk_client_list(clp, result, cred);
+	} while (status == -NFS4ERR_STALE_CLIENTID);
+
+	if (status == 0) {
 		/* Sustain the lease, even if it's empty.  If the clientid4
 		 * goes stale it's of no use for trunking discovery. */
 		nfs4_schedule_state_renewal(*result);
-		break;
 	}
 
-out:
 	return status;
 }
 
-- 
1.8.1

Re: Question on nfs40_discover_server_trunking.

[Chuck Lever]

On Jan 18, 2013, at 8:01 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:

> On Fri, 2013-01-18 at 19:44 -0500, Trond Myklebust wrote:
>> On Fri, 2013-01-18 at 18:14 -0500, Chuck Lever wrote:
>>> On Jan 18, 2013, at 5:59 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:
>>> 
>>>> On Fri, 2013-01-18 at 17:03 -0500, an unknown sender wrote:
>>>>> On Fri, 2013-01-18 at 16:33 -0500, Chuck Lever wrote:
>>>>>> On Jan 18, 2013, at 4:28 PM, Ben Greear <greearb@candelatech.com> wrote:
>>>>>> 
>>>>>>> Any chance the STALE_CLIENTID case needs a 'break'?
>>>>>> 
>>>>>> I don't think so.  LEASE_CONFIRM is set, and we want to wake the state renewal thread.
>>>>>> 
>>>>>>> 
>>>>>>> Twice I've seen kernel crashes after the nfs40_walk_client_list
>>>>>>> failed (though code comments say it should never fail).
>>>>>> 
>>>>>> nfs40_walk_client_list() is looking for an nfs_client that is supposed to already be in the nfs_client list.  If the search fails, that's a bug.
>>>>>> 
>>>>>> Eyeball the contents of your nfs_client list.  You should find an appropriate nfs_client in there, and then figure out why the search doesn't find it.
>>>>> 
>>>>> You have considered the fact that the call to
>>>>> nfs4_proc_setclientid_confirm can potentially return
>>>>> NFS4ERR_STALE_CLIENTID if the server rebooted while the client was
>>>>> walking the list?
>>>> 
>>>> In fact, as far as I can see, the correct behaviour in
>>>> nfs40_discover_server_trunking() should be to re-issue the setclientid
>>>> call, and then walk the list again if nfs40_walk_client_list() returns
>>>> NFS4ERR_STALE_CLIENTID.
>>> 
>>> When I wrote the server trunking detection logic, I think we hadn't clearly decided what needed to be done in the STALE_CLIENTID case.
>>> 
>>>> Something like the attached patch:
>>> 
>>> A couple of comments:
>>> 
>>> o  nfs_get_client() already sticks the new client on the tail of the nfs_client list
>> 
>> OK. That allows us to get rid of 5-6 lines of code.
>> 
>>> o  We don't want to get stuck in a loop here.  Should the "do {}" loop in nfs40_discover_server_trunking() be bounded by a retry count?
>> 
>> Not a number of retries, but possibly a timeout value.
>> 
>>> However, I haven't heard Ben say "oh, yes, my server had rebooted."  I'd like some confirmation that the match failed for an explainable and expected reason.
>> 
>> The spec allows the server to discard unconfirmed clientids, so our code
>> should take that into account.
>> 
>> 
>> BTW: I'm going insane trying to figure out how the refcounting in that
>> code is supposed to work. The assumption that we can somehow safely
>> return a non-referenced pointer from nfs4_discover_server_trunking and
>> then bump the reference count in nfs4_init_client() drives me nuts...
> 
> Attached is v2 of the patch that addresses all of the above.

At first glance, it looks plausible.  Well done.  Perhaps the reference counting fix should be split into a separate patch.

This:

"If walking the list in nfs40_walk_client_list fails, then the most
likely explanation is that the server rebooted."

I think in Ben's case the server may be dropping the unconfirmed client ID because it is so busy and the client is probably dealing with a thousand or more mount points, which causes trunking detection to take a lot of time.  I'd still like to see real evidence of this, but I can't think of any easy way for Ben to provide it.

The "No match found." comment is good.

Then:

"Ensure that we catch that case by testing our new nfs_client last."

"testing the new nfs_client last" is the way it's supposed to work -- that's why nfs_get_client() uses list_add_tail() now to insert the new client.  Adding the new client to the client list is supposed to guarantee that walk_client_list always finds an appropriate nfs_client on the list.

So you're not actually changing the order of the tests.  What you're really changing is the recovery behavior in the "our new clientid is already stale" case.  The patch description is probably inaccurate in this regard.

Do we need a similar change for STALE_CLIENTID in the NFSv4.1 path?

Finally, because you are replacing that bogus switch statement in nfs40_discover_server_trunking(), you are fixing Ben's crash.  His GPF back trace should be included in your patch description if you are sending this to stable.

I can test this a little more deeply in a week or two when I get back to the migration work.

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: Question on nfs40_discover_server_trunking.

[Myklebust, Trond]

[-- Attachment #1: Type: text/plain, Size: 4884 bytes --]

On Fri, 2013-01-18 at 20:27 -0500, Chuck Lever wrote:
> On Jan 18, 2013, at 8:01 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:
> 
> > On Fri, 2013-01-18 at 19:44 -0500, Trond Myklebust wrote:
> >> On Fri, 2013-01-18 at 18:14 -0500, Chuck Lever wrote:
> >>> On Jan 18, 2013, at 5:59 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:
> >>> 
> >>>> On Fri, 2013-01-18 at 17:03 -0500, an unknown sender wrote:
> >>>>> On Fri, 2013-01-18 at 16:33 -0500, Chuck Lever wrote:
> >>>>>> On Jan 18, 2013, at 4:28 PM, Ben Greear <greearb@candelatech.com> wrote:
> >>>>>> 
> >>>>>>> Any chance the STALE_CLIENTID case needs a 'break'?
> >>>>>> 
> >>>>>> I don't think so.  LEASE_CONFIRM is set, and we want to wake the state renewal thread.
> >>>>>> 
> >>>>>>> 
> >>>>>>> Twice I've seen kernel crashes after the nfs40_walk_client_list
> >>>>>>> failed (though code comments say it should never fail).
> >>>>>> 
> >>>>>> nfs40_walk_client_list() is looking for an nfs_client that is supposed to already be in the nfs_client list.  If the search fails, that's a bug.
> >>>>>> 
> >>>>>> Eyeball the contents of your nfs_client list.  You should find an appropriate nfs_client in there, and then figure out why the search doesn't find it.
> >>>>> 
> >>>>> You have considered the fact that the call to
> >>>>> nfs4_proc_setclientid_confirm can potentially return
> >>>>> NFS4ERR_STALE_CLIENTID if the server rebooted while the client was
> >>>>> walking the list?
> >>>> 
> >>>> In fact, as far as I can see, the correct behaviour in
> >>>> nfs40_discover_server_trunking() should be to re-issue the setclientid
> >>>> call, and then walk the list again if nfs40_walk_client_list() returns
> >>>> NFS4ERR_STALE_CLIENTID.
> >>> 
> >>> When I wrote the server trunking detection logic, I think we hadn't clearly decided what needed to be done in the STALE_CLIENTID case.
> >>> 
> >>>> Something like the attached patch:
> >>> 
> >>> A couple of comments:
> >>> 
> >>> o  nfs_get_client() already sticks the new client on the tail of the nfs_client list
> >> 
> >> OK. That allows us to get rid of 5-6 lines of code.
> >> 
> >>> o  We don't want to get stuck in a loop here.  Should the "do {}" loop in nfs40_discover_server_trunking() be bounded by a retry count?
> >> 
> >> Not a number of retries, but possibly a timeout value.
> >> 
> >>> However, I haven't heard Ben say "oh, yes, my server had rebooted."  I'd like some confirmation that the match failed for an explainable and expected reason.
> >> 
> >> The spec allows the server to discard unconfirmed clientids, so our code
> >> should take that into account.
> >> 
> >> 
> >> BTW: I'm going insane trying to figure out how the refcounting in that
> >> code is supposed to work. The assumption that we can somehow safely
> >> return a non-referenced pointer from nfs4_discover_server_trunking and
> >> then bump the reference count in nfs4_init_client() drives me nuts...
> > 
> > Attached is v2 of the patch that addresses all of the above.
> 
> At first glance, it looks plausible.  Well done.  Perhaps the reference counting fix should be split into a separate patch.
> 
> This:
> 
> "If walking the list in nfs40_walk_client_list fails, then the most
> likely explanation is that the server rebooted."
> 
> I think in Ben's case the server may be dropping the unconfirmed client ID because it is so busy and the client is probably dealing with a thousand or more mount points, which causes trunking detection to take a lot of time.  I'd still like to see real evidence of this, but I can't think of any easy way for Ben to provide it.
> 
> The "No match found." comment is good.
> 
> Then:
> 
> "Ensure that we catch that case by testing our new nfs_client last."
> 
> "testing the new nfs_client last" is the way it's supposed to work -- that's why nfs_get_client() uses list_add_tail() now to insert the new client.  Adding the new client to the client list is supposed to guarantee that walk_client_list always finds an appropriate nfs_client on the list.
> 
> So you're not actually changing the order of the tests.  What you're really changing is the recovery behavior in the "our new clientid is already stale" case.  The patch description is probably inaccurate in this regard.
> 
> Do we need a similar change for STALE_CLIENTID in the NFSv4.1 path?
> 
> Finally, because you are replacing that bogus switch statement in nfs40_discover_server_trunking(), you are fixing Ben's crash.  His GPF back trace should be included in your patch description if you are sending this to stable.

The Oops is just a symptom. The underlying problem here is the client
side spec violation.

See attachments for the v3 patches...

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-NFSv4-Fix-NFSv4-reference-counting-for-trunked-sessi.patch --]
[-- Type: text/x-patch; name="0001-NFSv4-Fix-NFSv4-reference-counting-for-trunked-sessi.patch", Size: 3216 bytes --]

From 79e6b226aa8da2fe37974c4d0915ddc78b6cdd11 Mon Sep 17 00:00:00 2001
From: Trond Myklebust <Trond.Myklebust@netapp.com>
Date: Fri, 18 Jan 2013 22:41:53 -0500
Subject: [PATCH v3 1/3] NFSv4: Fix NFSv4 reference counting for trunked
 sessions

The reference counting in nfs4_init_client assumes wongly that it
is safe for nfs4_discover_server_trunking() to return a pointer to a
nfs_client prior to bumping the reference count.

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
Cc: Chuck Lever <chuck.lever@oracle.com>
Cc: Ben Greear <greearb@candelatech.com>
Cc: stable@vger.kernel.org [>=3.7]
---
 fs/nfs/nfs4client.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
index acc3472..65a290a 100644
--- a/fs/nfs/nfs4client.c
+++ b/fs/nfs/nfs4client.c
@@ -236,11 +236,10 @@ struct nfs_client *nfs4_init_client(struct nfs_client *clp,
 	error = nfs4_discover_server_trunking(clp, &old);
 	if (error < 0)
 		goto error;
+	nfs_put_client(clp);
 	if (clp != old) {
 		clp->cl_preserve_clid = true;
-		nfs_put_client(clp);
 		clp = old;
-		atomic_inc(&clp->cl_count);
 	}
 
 	return clp;
@@ -306,7 +305,7 @@ int nfs40_walk_client_list(struct nfs_client *new,
 		.clientid	= new->cl_clientid,
 		.confirm	= new->cl_confirm,
 	};
-	int status;
+	int status = -NFS4ERR_STALE_CLIENTID;
 
 	spin_lock(&nn->nfs_client_lock);
 	list_for_each_entry_safe(pos, n, &nn->nfs_client_list, cl_share_link) {
@@ -332,28 +331,28 @@ int nfs40_walk_client_list(struct nfs_client *new,
 
 		if (prev)
 			nfs_put_client(prev);
+		prev = pos;
 
 		status = nfs4_proc_setclientid_confirm(pos, &clid, cred);
-		if (status == 0) {
+		switch (status) {
+		case -NFS4ERR_STALE_CLIENTID:
+			break;
+		case 0:
 			nfs4_swap_callback_idents(pos, new);
 
-			nfs_put_client(pos);
+			prev = NULL;
 			*result = pos;
 			dprintk("NFS: <-- %s using nfs_client = %p ({%d})\n",
 				__func__, pos, atomic_read(&pos->cl_count));
-			return 0;
-		}
-		if (status != -NFS4ERR_STALE_CLIENTID) {
-			nfs_put_client(pos);
-			dprintk("NFS: <-- %s status = %d, no result\n",
-				__func__, status);
-			return status;
+		default:
+			goto out;
 		}
 
 		spin_lock(&nn->nfs_client_lock);
-		prev = pos;
 	}
+	spin_unlock(&nn->nfs_client_lock);
 
+out:
 	/*
 	 * No matching nfs_client found.  This should be impossible,
 	 * because the new nfs_client has already been added to
@@ -363,9 +362,8 @@ int nfs40_walk_client_list(struct nfs_client *new,
 	 */
 	if (prev)
 		nfs_put_client(prev);
-	spin_unlock(&nn->nfs_client_lock);
-	pr_err("NFS: %s Error: no matching nfs_client found\n", __func__);
-	return -NFS4ERR_STALE_CLIENTID;
+	dprintk("NFS: <-- %s status = %d\n", __func__, status);
+	return status;
 }
 
 #ifdef CONFIG_NFS_V4_1
@@ -473,6 +471,7 @@ int nfs41_walk_client_list(struct nfs_client *new,
 		if (!nfs4_match_serverowners(pos, new))
 			continue;
 
+		atomic_inc(&pos->cl_count);
 		spin_unlock(&nn->nfs_client_lock);
 		dprintk("NFS: <-- %s using nfs_client = %p ({%d})\n",
 			__func__, pos, atomic_read(&pos->cl_count));
-- 
1.8.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-NFSv4-Fix-NFSv4-trunking-discovery.patch --]
[-- Type: text/x-patch; name="0002-NFSv4-Fix-NFSv4-trunking-discovery.patch", Size: 3798 bytes --]

From 0c66c02db14b5dbeec5206dafcb45f38f9d4b43c Mon Sep 17 00:00:00 2001
From: Trond Myklebust <Trond.Myklebust@netapp.com>
Date: Fri, 18 Jan 2013 22:56:23 -0500
Subject: [PATCH v3 2/3] NFSv4: Fix NFSv4 trunking discovery

If walking the list in nfs4[01]_walk_client_list fails, then the most
likely explanation is that the server dropped the clientid before we
actually managed to confirm it. As long as our nfs_client is the very
last one in the list to be tested, the caller can be assured that this
is the case when the final return value is NFS4ERR_STALE_CLIENTID.

Reported-by: Ben Greear <greearb@candelatech.com>
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
Cc: Chuck Lever <chuck.lever@oracle.com>
Cc: stable@vger.kernel.org [>=3.7]
---
 fs/nfs/nfs4client.c | 26 +++++++-------------------
 fs/nfs/nfs4state.c  |  8 ++------
 2 files changed, 9 insertions(+), 25 deletions(-)

diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
index 65a290a..2f21f17 100644
--- a/fs/nfs/nfs4client.c
+++ b/fs/nfs/nfs4client.c
@@ -352,14 +352,8 @@ int nfs40_walk_client_list(struct nfs_client *new,
 	}
 	spin_unlock(&nn->nfs_client_lock);
 
+	/* No match found. The server lost our clientid */
 out:
-	/*
-	 * No matching nfs_client found.  This should be impossible,
-	 * because the new nfs_client has already been added to
-	 * nfs_client_list by nfs_get_client().
-	 *
-	 * Don't BUG(), since the caller is holding a mutex.
-	 */
 	if (prev)
 		nfs_put_client(prev);
 	dprintk("NFS: <-- %s status = %d\n", __func__, status);
@@ -430,7 +424,7 @@ int nfs41_walk_client_list(struct nfs_client *new,
 {
 	struct nfs_net *nn = net_generic(new->cl_net, nfs_net_id);
 	struct nfs_client *pos, *n, *prev = NULL;
-	int error;
+	int status = -NFS4ERR_STALE_CLIENTID;
 
 	spin_lock(&nn->nfs_client_lock);
 	list_for_each_entry_safe(pos, n, &nn->nfs_client_list, cl_share_link) {
@@ -446,8 +440,8 @@ int nfs41_walk_client_list(struct nfs_client *new,
 				nfs_put_client(prev);
 			prev = pos;
 
-			error = nfs_wait_client_init_complete(pos);
-			if (error < 0) {
+			status = nfs_wait_client_init_complete(pos);
+			if (status < 0) {
 				nfs_put_client(pos);
 				spin_lock(&nn->nfs_client_lock);
 				continue;
@@ -480,16 +474,10 @@ int nfs41_walk_client_list(struct nfs_client *new,
 		return 0;
 	}
 
-	/*
-	 * No matching nfs_client found.  This should be impossible,
-	 * because the new nfs_client has already been added to
-	 * nfs_client_list by nfs_get_client().
-	 *
-	 * Don't BUG(), since the caller is holding a mutex.
-	 */
+	/* No matching nfs_client found. */
 	spin_unlock(&nn->nfs_client_lock);
-	pr_err("NFS: %s Error: no matching nfs_client found\n", __func__);
-	return -NFS4ERR_STALE_CLIENTID;
+	dprintk("NFS: <-- %s status = %d\n", __func__, status);
+	return status;
 }
 #endif	/* CONFIG_NFS_V4_1 */
 
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 9448c57..f72561ca 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -136,16 +136,11 @@ int nfs40_discover_server_trunking(struct nfs_client *clp,
 	clp->cl_confirm = clid.confirm;
 
 	status = nfs40_walk_client_list(clp, result, cred);
-	switch (status) {
-	case -NFS4ERR_STALE_CLIENTID:
-		set_bit(NFS4CLNT_LEASE_CONFIRM, &clp->cl_state);
-	case 0:
+	if (status == 0) {
 		/* Sustain the lease, even if it's empty.  If the clientid4
 		 * goes stale it's of no use for trunking discovery. */
 		nfs4_schedule_state_renewal(*result);
-		break;
 	}
-
 out:
 	return status;
 }
@@ -1863,6 +1858,7 @@ again:
 	case -ETIMEDOUT:
 	case -EAGAIN:
 		ssleep(1);
+	case -NFS4ERR_STALE_CLIENTID:
 		dprintk("NFS: %s after status %d, retrying\n",
 			__func__, status);
 		goto again;
-- 
1.8.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-NFSv4.1-Ensure-that-nfs41_walk_client_list-does-star.patch --]
[-- Type: text/x-patch; name="0003-NFSv4.1-Ensure-that-nfs41_walk_client_list-does-star.patch", Size: 1292 bytes --]

From 8102b6146af9a071457e371da4e9516156694880 Mon Sep 17 00:00:00 2001
From: Trond Myklebust <Trond.Myklebust@netapp.com>
Date: Fri, 18 Jan 2013 23:01:43 -0500
Subject: [PATCH v3 3/3] NFSv4.1: Ensure that nfs41_walk_client_list() does
 start lease recovery

We do need to start the lease recovery thread prior to waiting for the
client initialisation to complete in NFSv4.1.

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
Cc: Chuck Lever <chuck.lever@oracle.com>
Cc: Ben Greear <greearb@candelatech.com>
Cc: stable@vger.kernel.org [>=3.7]
---
 fs/nfs/nfs4client.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
index 2f21f17..2e9779b 100644
--- a/fs/nfs/nfs4client.c
+++ b/fs/nfs/nfs4client.c
@@ -440,14 +440,17 @@ int nfs41_walk_client_list(struct nfs_client *new,
 				nfs_put_client(prev);
 			prev = pos;
 
+			nfs4_schedule_lease_recovery(pos);
 			status = nfs_wait_client_init_complete(pos);
 			if (status < 0) {
 				nfs_put_client(pos);
 				spin_lock(&nn->nfs_client_lock);
 				continue;
 			}
-
+			status = pos->cl_cons_state;
 			spin_lock(&nn->nfs_client_lock);
+			if (status < 0)
+				continue;
 		}
 
 		if (pos->rpc_ops != new->rpc_ops)
-- 
1.8.1

Re: Question on nfs40_discover_server_trunking.

[Ben Greear]

On 01/18/2013 08:11 PM, Myklebust, Trond wrote:

> The Oops is just a symptom. The underlying problem here is the client
> side spec violation.
>
> See attachments for the v3 patches...
>

I have been testing with these, and have not seen any NFS crashes since applying
them.

I still do see some crashes on tcp-recv, but these were there before, and may
not be related to NFS anyway.  I'll continue to debug the TCP crashes,
but as for these three NFS patches, feel free to add:

Tested-By:  Ben Greear <greearb@candelatech.com>

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com