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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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;
> }
>
On Thu, Mar 30, 2017 at 1:26 PM, Anna Schumaker
<[email protected]> 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 <[email protected]>
>> ---
>> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/30/2017 01:37 PM, Olga Kornievskaia wrote:
> On Thu, Mar 30, 2017 at 1:26 PM, Anna Schumaker
> <[email protected]> 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 <[email protected]>
>>> ---
>>> 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 [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <[email protected]>
---
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