Skip to contentSkip to conversation

NFSv4.1 fix infinite loop on IO BAD_STATEID error

PATCH In review Olga Kornievskaia 3 participants 5 messages Mar 30, 2017

Subsystems (2)
NFS client NFS server
lore
Share & tools
lore

Olga Kornievskaia

author
#0 lore raw

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

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(+)
fs/nfs/nfs4proc.c +1 -0
index dfa46e4..fb6d981 100644
@@ -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;
 	}
Signature · 1 line
1.8.3.1
#1 lore raw

Hi Olga,

Quoted 5 lines · 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.

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

Quoted 12 lines
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(+)
fs/nfs/nfs4proc.c +0 -0
index dfa46e4..fb6d981 100644

fs/nfs/nfs4proc.c static void nfs41_check_delegation_stateid(struct nfs4_state *state) · line 2464

Quoted 5 lines
fs/nfs/nfs4proc.c +1 -0
@@ -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

Quoted 3 lines
fs/nfs/nfs4proc.c +0 -0
 		return;
 	}
#2 lore raw
Quoted 10 lines · Anna Schumaker

Hi Olga,

Quoted 5 lines · 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.

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

Oops. my bad.

Quoted 15 lines
Quoted 15 lines
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(+)
fs/nfs/nfs4proc.c +0 -0
index dfa46e4..fb6d981 100644
@@ -2460,6 +2460,7 @@ static void nfs41_check_delegation_stateid(struct nfs4_state *state)
      }
Quoted 5 lines

fs/nfs/nfs4proc.c static void nfs41_check_delegation_stateid(struct nfs4_state *state) · line 2464

Quoted 3 lines
fs/nfs/nfs4proc.c +1 -0
      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?

Quoted 11 lines

Thanks,
Anna

fs/nfs/nfs4proc.c static void nfs41_check_delegation_stateid(struct nfs4_state *state) · line 2466

Quoted 3 lines
fs/nfs/nfs4proc.c +0 -0
              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

#3 lore raw
Quoted 30 lines · Olga Kornievskaia
Quoted 10 lines · Anna Schumaker

Hi Olga,

Quoted 5 lines · 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.

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

Oops. my bad.

Quoted 15 lines
Quoted 15 lines
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(+)
fs/nfs/nfs4proc.c +0 -0
index dfa46e4..fb6d981 100644
@@ -2460,6 +2460,7 @@ static void nfs41_check_delegation_stateid(struct nfs4_state *state)
      }
Quoted 8 lines
Quoted 5 lines

fs/nfs/nfs4proc.c static void nfs41_check_delegation_stateid(struct nfs4_state *state) · line 2464

Quoted 3 lines
fs/nfs/nfs4proc.c +1 -0
      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! :)

Quoted 12 lines
Quoted 12 lines

Thanks,
Anna

fs/nfs/nfs4proc.c static void nfs41_check_delegation_stateid(struct nfs4_state *state) · line 2466

Quoted 3 lines
fs/nfs/nfs4proc.c +0 -0
              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

author
#4 lore raw

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

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(-)
fs/nfs/nfs4proc.c +3 -6
index dfa46e4..bd1e4f6 100644
@@ -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);
Signature · 1 line
1.8.3.1