nfs: Only update callback sequnce id when CB_SEQUENCE success

PATCH In review Kinglong Mee 3 participants 5 messages Jun 2, 2015 – Jun 12, 2015

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Kinglong Mee

author

2015-06-02 10:58 UTC #0 lore raw

PATCH 1/2 nfs: Only update callback sequnce id when CB_SEQUENCE success

When testing pnfs layout, nfsd got error NFS4ERR_SEQ_MISORDERED. It is caused by nfs return NFS4ERR_DELAY before validate_seqid(), don't update the sequnce id, but nfsd updates the sequnce id !!!

According to RFC5661 20.9.3,
" If CB_SEQUENCE returns an error, then the state of the slot
(sequence ID, cached reply) MUST NOT change. "

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfs/callback_proc.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

fs/nfs/callback_proc.c +12 -4

index 197806f..7e9653a 100644
@@ -327,10 +327,8 @@ validate_seqid(struct nfs4_slot_table *tbl, struct cb_sequenceargs * args)
 	dprintk("%s slot table seqid: %u\n", __func__, slot->seq_nr);
 
 	/* Normal */
-	if (likely(args->csa_sequenceid == slot->seq_nr + 1)) {
-		slot->seq_nr++;
+	if (likely(args->csa_sequenceid == slot->seq_nr + 1))
 		goto out_ok;
-	}
 
 	/* Replay */
 	if (args->csa_sequenceid == slot->seq_nr) {

@@ -418,6 +416,7 @@ __be32 nfs4_callback_sequence(struct cb_sequenceargs *args,
 			      struct cb_process_state *cps)
 {
 	struct nfs4_slot_table *tbl;
+	struct nfs4_slot *slot;
 	struct nfs_client *clp;
 	int i;
 	__be32 status = htonl(NFS4ERR_BADSESSION);

@@ -429,7 +428,9 @@ __be32 nfs4_callback_sequence(struct cb_sequenceargs *args,
 
 	if (!(clp->cl_session->flags & SESSION4_BACK_CHAN))
 		goto out;
+
 	tbl = &clp->cl_session->bc_slot_table;
+	slot = tbl->slots + args->csa_slotid;
 
 	spin_lock(&tbl->slot_tbl_lock);
 	/* state manager is resetting the session */

@@ -444,7 +445,7 @@ __be32 nfs4_callback_sequence(struct cb_sequenceargs *args,
 		goto out;
 	}
 
-	status = validate_seqid(&clp->cl_session->bc_slot_table, args);
+	status = validate_seqid(tbl, args);
 	spin_unlock(&tbl->slot_tbl_lock);
 	if (status)
 		goto out;

@@ -468,6 +469,13 @@ __be32 nfs4_callback_sequence(struct cb_sequenceargs *args,
 	res->csr_highestslotid = NFS41_BC_MAX_CALLBACKS - 1;
 	res->csr_target_highestslotid = NFS41_BC_MAX_CALLBACKS - 1;
 
+	/*
+	 * RFC5661 20.9.3
+	 * If CB_SEQUENCE returns an error, then the state of the slot
+	 * (sequence ID, cached reply) MUST NOT change.
+	 */
+	slot->seq_nr++;
+
 out:
 	cps->clp = clp; /* put in nfs4_callback_compound */
 	for (i = 0; i < args->csa_nrclists; i++)

Signature · 1 line

2.4.2

Kinglong Mee

author

2015-06-11 01:38 UTC #1 lore raw

ping ....

Quoted 71 lines · Kinglong Mee

According to RFC5661 20.9.3,
" If CB_SEQUENCE returns an error, then the state of the slot
(sequence ID, cached reply) MUST NOT change. "

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfs/callback_proc.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

fs/nfs/callback_proc.c +12 -4

index 197806f..7e9653a 100644
@@ -327,10 +327,8 @@ validate_seqid(struct nfs4_slot_table *tbl, struct cb_sequenceargs * args)
 	dprintk("%s slot table seqid: %u\n", __func__, slot->seq_nr);
 
 	/* Normal */
-	if (likely(args->csa_sequenceid == slot->seq_nr + 1)) {
-		slot->seq_nr++;
+	if (likely(args->csa_sequenceid == slot->seq_nr + 1))
 		goto out_ok;
-	}
 
 	/* Replay */
 	if (args->csa_sequenceid == slot->seq_nr) {

@@ -418,6 +416,7 @@ __be32 nfs4_callback_sequence(struct cb_sequenceargs *args,
 			      struct cb_process_state *cps)
 {
 	struct nfs4_slot_table *tbl;
+	struct nfs4_slot *slot;
 	struct nfs_client *clp;
 	int i;
 	__be32 status = htonl(NFS4ERR_BADSESSION);

@@ -429,7 +428,9 @@ __be32 nfs4_callback_sequence(struct cb_sequenceargs *args,
 
 	if (!(clp->cl_session->flags & SESSION4_BACK_CHAN))
 		goto out;
+
 	tbl = &clp->cl_session->bc_slot_table;
+	slot = tbl->slots + args->csa_slotid;
 
 	spin_lock(&tbl->slot_tbl_lock);
 	/* state manager is resetting the session */

@@ -444,7 +445,7 @@ __be32 nfs4_callback_sequence(struct cb_sequenceargs *args,
 		goto out;
 	}
 
-	status = validate_seqid(&clp->cl_session->bc_slot_table, args);
+	status = validate_seqid(tbl, args);
 	spin_unlock(&tbl->slot_tbl_lock);
 	if (status)
 		goto out;

@@ -468,6 +469,13 @@ __be32 nfs4_callback_sequence(struct cb_sequenceargs *args,
 	res->csr_highestslotid = NFS41_BC_MAX_CALLBACKS - 1;
 	res->csr_target_highestslotid = NFS41_BC_MAX_CALLBACKS - 1;
 
+	/*
+	 * RFC5661 20.9.3
+	 * If CB_SEQUENCE returns an error, then the state of the slot
+	 * (sequence ID, cached reply) MUST NOT change.
+	 */
+	slot->seq_nr++;
+
 out:
 	cps->clp = clp; /* put in nfs4_callback_compound */
 	for (i = 0; i < args->csa_nrclists; i++)

Christoph Hellwig

2015-06-11 08:20 UTC #2 lore raw

Hi Kinglong,

when we last brought up this issue the recommendation was to follow the model in the clients nfs41_sequence_done() which handles even more corner cases. Given that my previous fixes in that area were just a bandaid I probably shouldn't complain about piece mail handling, but if you have a little spare time available implementing the full set of checks might be worthwhile.

Kinglong Mee

author

2015-06-11 09:13 UTC #3 lore raw

Quoted 8 lines · Christoph Hellwig

Hi Kinglong,
when we last brought up this issue the recommendation was to follow the model in the clients nfs41_sequence_done() which handles even more corner cases. Given that my previous fixes in that area were just a bandaid I probably shouldn't complain about piece mail handling, but if you have a little spare time available implementing the full set of checks might be worthwhile.

Thank you for your comments.

I have see your patches sits "nfsd: callback fixes". You said process as nfs41_sequence_done() for handling error cases is server's logical.

Those two patches for NFS client are fix two bugs.

The first one is fix that between validate_seqid() nfs can return error to nfs server. NFS server cannot make sure whether NFS client has update the sequence ID.

Also, according to RFC, "If CB_SEQUENCE returns an error, then the state of the slot (sequence ID, cached reply) MUST NOT change.", so that this this patch is out.

The second for nfs is fix the initialize problem of cb_sequenceres when meeting NFS4ERR_RETRY_UNCACHED_REP.

And, other two patches for nfsd,
The first is fix a infinite loop caused by your last patch I meet. The second is as you said following to nfs41_sequence_done() maybe better.

thanks
Kinglong Mee

Trond Myklebust

2015-06-12 01:35 UTC #4 lore raw

PATCH 1/1 NFS: Ensure that we update the sequence id under the slot table lock

In reply to Kinglong Mee

Fix a callback slot table regression.

Fixes e937ee714b2d ("nfs: Only update callback sequnce id when CB_SEQUENCE success")

Cc Kinglong Mee <[email protected]>

Signed-off-by Trond Myklebust <[email protected]>

---
 fs/nfs/callback_proc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

fs/nfs/callback_proc.c +5 -5

index 17a5c571a006..29e3c1b011b7 100644
@@ -435,14 +435,13 @@ __be32 nfs4_callback_sequence(struct cb_sequenceargs *args,
 	spin_lock(&tbl->slot_tbl_lock);
 	/* state manager is resetting the session */
 	if (test_bit(NFS4_SLOT_TBL_DRAINING, &tbl->slot_tbl_state)) {
-		spin_unlock(&tbl->slot_tbl_lock);
 		status = htonl(NFS4ERR_DELAY);
 		/* Return NFS4ERR_BADSESSION if we're draining the session
 		 * in order to reset it.
 		 */
 		if (test_bit(NFS4CLNT_SESSION_RESET, &clp->cl_state))
 			status = htonl(NFS4ERR_BADSESSION);
-		goto out;
+		goto out_unlock;
 	}
 
 	memcpy(&res->csr_sessionid, &args->csa_sessionid,

@@ -453,9 +452,8 @@ __be32 nfs4_callback_sequence(struct cb_sequenceargs *args,
 	res->csr_target_highestslotid = NFS41_BC_MAX_CALLBACKS - 1;
 
 	status = validate_seqid(tbl, args);
-	spin_unlock(&tbl->slot_tbl_lock);
 	if (status)
-		goto out;
+		goto out_unlock;
 
 	cps->slotid = args->csa_slotid;

@@ -466,7 +464,7 @@ __be32 nfs4_callback_sequence(struct cb_sequenceargs *args,
 	 */
 	if (referring_call_exists(clp, args->csa_nrclists, args->csa_rclists)) {
 		status = htonl(NFS4ERR_DELAY);
-		goto out;
+		goto out_unlock;
 	}
 
 	/*

@@ -475,6 +473,8 @@ __be32 nfs4_callback_sequence(struct cb_sequenceargs *args,
 	 * (sequence ID, cached reply) MUST NOT change.
 	 */
 	slot->seq_nr++;
+out_unlock:
+	spin_unlock(&tbl->slot_tbl_lock);
 
 out:
 	cps->clp = clp; /* put in nfs4_callback_compound */

Signature · 1 line

2.4.2