[PATCH 1/1] NFSv4.1 fix infinite loop on IO BAD_STATEID error

[PATCH 1/1] NFSv4.1 fix infinite loop on IO BAD_STATEID error

[Olga Kornievskaia]

Commit 02bfab0414d7 "NFSv4.1: Don't recheck delegations that
have already been checked" introduced a regression where when a
client received BAD_STATEID error it would not send any TEST_STATEID
and instead go into an infinite loop of resending the IO that caused
the BAD_STATEID.

Fixes: 02bfab0414d7 ("NFSv4.1: Don't recheck delegations that have already been checked")
Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfs/nfs4proc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index dfa46e4..fb6d981 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2460,6 +2460,7 @@ static void nfs41_check_delegation_stateid(struct nfs4_state *state)
 	}
 
 	if (!test_and_clear_bit(NFS_DELEGATION_TEST_EXPIRED, &delegation->flags)) {
+		nfs_finish_clear_delegation_stateid(state, &stateid);
 		rcu_read_unlock();
 		return;
 	}
-- 
1.8.3.1

Re: [PATCH 1/1] NFSv4.1 fix infinite loop on IO BAD_STATEID error

[Anna Schumaker]

Hi Olga,

On 03/30/2017 10:00 AM, Olga Kornievskaia wrote:
> Commit 02bfab0414d7 "NFSv4.1: Don't recheck delegations that
> have already been checked" introduced a regression where when a
> client received BAD_STATEID error it would not send any TEST_STATEID
> and instead go into an infinite loop of resending the IO that caused
> the BAD_STATEID.

Can you double check the bad commit?  I found it as 63d63cbf5e03 and not 02bfab0414d7.  

> 
> Fixes: 02bfab0414d7 ("NFSv4.1: Don't recheck delegations that have already been checked")
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfs/nfs4proc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index dfa46e4..fb6d981 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -2460,6 +2460,7 @@ static void nfs41_check_delegation_stateid(struct nfs4_state *state)
>  	}
>  
>  	if (!test_and_clear_bit(NFS_DELEGATION_TEST_EXPIRED, &delegation->flags)) {
> +		nfs_finish_clear_delegation_stateid(state, &stateid);
>  		rcu_read_unlock();

The NFS_DELEGATION_REVOKED case does the rcu_read_unlock() before the call to nfs_finish_clear_delegation_stateid().  I'm not sure how much of a difference it makes, but can you swap the order of the calls here to match?

Thanks,
Anna

>  		return;
>  	}
> 

Re: [PATCH 1/1] NFSv4.1 fix infinite loop on IO BAD_STATEID error

[Olga Kornievskaia]

On Thu, Mar 30, 2017 at 1:26 PM, Anna Schumaker
<Anna.Schumaker@netapp.com> wrote:
> Hi Olga,
>
> On 03/30/2017 10:00 AM, Olga Kornievskaia wrote:
>> Commit 02bfab0414d7 "NFSv4.1: Don't recheck delegations that
>> have already been checked" introduced a regression where when a
>> client received BAD_STATEID error it would not send any TEST_STATEID
>> and instead go into an infinite loop of resending the IO that caused
>> the BAD_STATEID.
>
> Can you double check the bad commit?  I found it as 63d63cbf5e03 and not 02bfab0414d7.

Oops. my bad.

>
>>
>> Fixes: 02bfab0414d7 ("NFSv4.1: Don't recheck delegations that have already been checked")
>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>> ---
>>  fs/nfs/nfs4proc.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index dfa46e4..fb6d981 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -2460,6 +2460,7 @@ static void nfs41_check_delegation_stateid(struct nfs4_state *state)
>>       }
>>
>>       if (!test_and_clear_bit(NFS_DELEGATION_TEST_EXPIRED, &delegation->flags)) {
>> +             nfs_finish_clear_delegation_stateid(state, &stateid);
>>               rcu_read_unlock();
>
> The NFS_DELEGATION_REVOKED case does the rcu_read_unlock() before the call to nfs_finish_clear_delegation_stateid().  I'm not sure how much of a difference it makes, but can you swap the order of the calls here to match?

Ok can do. While I'm at it, both if() do the same thing. Should they
be squashed?

>
> Thanks,
> Anna
>
>>               return;
>>       }
>>
> --
> 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/1] NFSv4.1 fix infinite loop on IO BAD_STATEID error

[Anna Schumaker]

On 03/30/2017 01:37 PM, Olga Kornievskaia wrote:
> On Thu, Mar 30, 2017 at 1:26 PM, Anna Schumaker
> <Anna.Schumaker@netapp.com> wrote:
>> Hi Olga,
>>
>> On 03/30/2017 10:00 AM, Olga Kornievskaia wrote:
>>> Commit 02bfab0414d7 "NFSv4.1: Don't recheck delegations that
>>> have already been checked" introduced a regression where when a
>>> client received BAD_STATEID error it would not send any TEST_STATEID
>>> and instead go into an infinite loop of resending the IO that caused
>>> the BAD_STATEID.
>>
>> Can you double check the bad commit?  I found it as 63d63cbf5e03 and not 02bfab0414d7.
> 
> Oops. my bad.
> 
>>
>>>
>>> Fixes: 02bfab0414d7 ("NFSv4.1: Don't recheck delegations that have already been checked")
>>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>>> ---
>>>  fs/nfs/nfs4proc.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>> index dfa46e4..fb6d981 100644
>>> --- a/fs/nfs/nfs4proc.c
>>> +++ b/fs/nfs/nfs4proc.c
>>> @@ -2460,6 +2460,7 @@ static void nfs41_check_delegation_stateid(struct nfs4_state *state)
>>>       }
>>>
>>>       if (!test_and_clear_bit(NFS_DELEGATION_TEST_EXPIRED, &delegation->flags)) {
>>> +             nfs_finish_clear_delegation_stateid(state, &stateid);
>>>               rcu_read_unlock();
>>
>> The NFS_DELEGATION_REVOKED case does the rcu_read_unlock() before the call to nfs_finish_clear_delegation_stateid().  I'm not sure how much of a difference it makes, but can you swap the order of the calls here to match?
> 
> Ok can do. While I'm at it, both if() do the same thing. Should they
> be squashed?

I like the sound of that even better, thanks! :)

> 
>>
>> Thanks,
>> Anna
>>
>>>               return;
>>>       }
>>>
>> --
>> 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

[PATCH 1/1] NFSv4.1 fix infinite loop on IO BAD_STATEID error

[Olga Kornievskaia]

Commit 63d63cbf5e03 "NFSv4.1: Don't recheck delegations that
have already been checked" introduced a regression where when a
client received BAD_STATEID error it would not send any TEST_STATEID
and instead go into an infinite loop of resending the IO that caused
the BAD_STATEID.

Fixes: 63d63cbf5e03 ("NFSv4.1: Don't recheck delegations that have already been checked")
Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfs/nfs4proc.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index dfa46e4..bd1e4f6 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2453,17 +2453,14 @@ static void nfs41_check_delegation_stateid(struct nfs4_state *state)
 	}
 
 	nfs4_stateid_copy(&stateid, &delegation->stateid);
-	if (test_bit(NFS_DELEGATION_REVOKED, &delegation->flags)) {
+	if (test_bit(NFS_DELEGATION_REVOKED, &delegation->flags) ||
+		!test_and_clear_bit(NFS_DELEGATION_TEST_EXPIRED,
+			&delegation->flags)) {
 		rcu_read_unlock();
 		nfs_finish_clear_delegation_stateid(state, &stateid);
 		return;
 	}
 
-	if (!test_and_clear_bit(NFS_DELEGATION_TEST_EXPIRED, &delegation->flags)) {
-		rcu_read_unlock();
-		return;
-	}
-
 	cred = get_rpccred(delegation->cred);
 	rcu_read_unlock();
 	status = nfs41_test_and_free_expired_stateid(server, &stateid, cred);
-- 
1.8.3.1