exchange_id/create_session bugfixes
Subsystems (2)
Bryan noticed a bug in testing (thanks also to Trond for analysis there), and I noticed another bug on reviewing the code. Plus some cleanup. Queueing these up for 3.5.
--b.
PATCH 1/5 nfsd4: fix exchange_id to return confirm flag
From: "J. Bruce Fields" <[email protected]>
Otherwise nfsd4_set_ex_flags writes over the return flags.
--- fs/nfsd/nfs4state.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/nfsd/nfs4state.c +1 -1
@@ -1617,7 +1617,7 @@ nfsd4_exchange_id(struct svc_rqst *rqstp, goto expire_client; }if (verfs_match) {/* case 2 */ - exid->flags |=EXCHGID4_FLAG_CONFIRMED_R ; + conf->cl_exchange_flags |=EXCHGID4_FLAG_CONFIRMED_R ; new = conf; goto out_copy; }
Signature · 1 line
1.7.10.2
PATCH 2/5 nfsd4: return "real" sequence id in confirmed case
In reply to J. Bruce FieldsFrom: "J. Bruce Fields" <[email protected]>
The client should ignore the returned sequence_id in the case where the CONFIRMED flag is set on an exchange_id reply--and in the unconfirmed case "1" is always the right response. So it shouldn't actually matter what we return here.
We could continue returning 1 just to catch clients ignoring the spec here, but I'd rather be generous. Other things equal, returning the existing sequence_id seems more informative.
--- fs/nfsd/nfs4state.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/nfsd/nfs4state.c +1 -1
@@ -1650,7 +1650,7 @@ out_copy: exid->clientid.cl_boot = new->cl_clientid.cl_boot; exid->clientid.cl_id = new->cl_clientid.cl_id; - exid->seqid =1 ; + exid->seqid = new->cl_cs_slot.sl_seqid +1 ;nfsd4_set_ex_flags (new, exid); dprintk("nfsd4_exchange_id seqid %d flags %x\n",
Signature · 1 line
1.7.10.2
PATCH 3/5 nfsd4: remove some dprintk's and a comment
In reply to J. Bruce FieldsFrom: "J. Bruce Fields" <[email protected]>
The comment is redundant, and if we really want dprintk's here they'd probably be better in the common (check-slot_seqid) code.
--- fs/nfsd/nfs4state.c | 8 -------- 1 file changed, 8 deletions(-)
fs/nfsd/nfs4state.c +0 -8
@@ -1750,16 +1750,10 @@ nfsd4_create_session(struct svc_rqst *rqstp, cs_slot = &conf->cl_cs_slot; status =check_slot_seqid (cr_ses->seqid, cs_slot->sl_seqid,0 );if (status == nfserr_replay_cache) { -dprintk ("Got a create_session replay! seqid= %d\n" , - cs_slot->sl_seqid); -/* Return the cached reply status */ status =nfsd4_replay_create_session (cr_ses, cs_slot); goto out; }else if (cr_ses->seqid != cs_slot->sl_seqid +1 ) { status = nfserr_seq_misordered; -dprintk ("Sequence misordered!\n" ); -dprintk ("Expected seqid= %d but got seqid= %d\n" , - cs_slot->sl_seqid, cr_ses->seqid); goto out; } }else if (unconf) {
@@ -1768,7 +1762,6 @@ nfsd4_create_session(struct svc_rqst *rqstp, status = nfserr_clid_inuse; goto out; } - cs_slot = &unconf->cl_cs_slot; status =check_slot_seqid (cr_ses->seqid, cs_slot->sl_seqid,0 );if (status) {
@@ -1776,7 +1769,6 @@ nfsd4_create_session(struct svc_rqst *rqstp, status = nfserr_seq_misordered; goto out; } - confirm_me = true; conf = unconf; }else {
Signature · 1 line
1.7.10.2
PATCH 4/5 nfsd4: don't remove rebooted client record until confirmation
In reply to J. Bruce FieldsFrom: "J. Bruce Fields" <[email protected]>
In the NFSv4.1 client-reboot case we're currently removing the client's previous state in exchange_id. That's wrong--we should be waiting till the confirming create_session.
--- fs/nfsd/nfs4state.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
fs/nfsd/nfs4state.c +9 -4
@@ -1614,7 +1614,8 @@ nfsd4_exchange_id(struct svc_rqst *rqstp, status = nfserr_clid_inuse; goto out; } - goto expire_client; +expire_client (conf); + goto out_new; }if (verfs_match) {/* case 2 */ conf->cl_exchange_flags |=EXCHGID4_FLAG_CONFIRMED_R ;
@@ -1622,8 +1623,6 @@ nfsd4_exchange_id(struct svc_rqst *rqstp, goto out_copy; }/* case 5, client reboot */ -expire_client: -expire_client (conf); goto out_new; }
@@ -1805,8 +1804,14 @@ nfsd4_create_session(struct svc_rqst *rqstp,/* cache solo and embedded create sessions under the state lock */ nfsd4_cache_create_session (cr_ses, cs_slot, status); -if (confirm_me) +if (confirm_me) { +unsigned int hash =clientstr_hashval (unconf->cl_recdir); +struct nfs4_client *old = +find_confirmed_client_by_str (conf->cl_recdir, hash); +if (old) +expire_client (old);move_to_confirmed (conf); + } out:nfs4_unlock_state ();dprintk ("%s returns %d\n" , __func__,ntohl (status));
Signature · 1 line
1.7.10.2
PATCH 5/5 nfsd4: fix, consolidate client_has_state
In reply to J. Bruce FieldsFrom: "J. Bruce Fields" <[email protected]>
Whoops: first, I reimplemented the already-existing has_resources without noticing; second, I got the test backwards. I did pick a better name, though. Combine the two....
--- fs/nfsd/nfs4state.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-)
fs/nfsd/nfs4state.c +4 -11
@@ -1537,9 +1537,9 @@ static bool client_has_state(struct nfs4_client *clp)* * Also note we should probably be using this in 4.0 case too. */ -return list_empty (&clp->cl_openowners) - &&list_empty (&clp->cl_delegations) - &&list_empty (&clp->cl_sessions); +return !list_empty (&clp->cl_openowners) + || !list_empty (&clp->cl_delegations) + || !list_empty (&clp->cl_sessions); } __be32
@@ -2069,13 +2069,6 @@ out:return status; } -static inline bool has_resources (struct nfs4_client *clp) -{ -return !list_empty (&clp->cl_openowners) - || !list_empty (&clp->cl_delegations) - || !list_empty (&clp->cl_sessions); -} -__be32 nfsd4_destroy_clientid (struct svc_rqst *rqstp,struct nfsd4_compound_state *cstate,struct nfsd4_destroy_clientid *dc) {
@@ -2089,7 +2082,7 @@ nfsd4_destroy_clientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstaif (conf) { clp = conf; -if (!is_client_expired (conf) &&has_resources (conf)) { +if (!is_client_expired (conf) &&client_has_state (conf)) { status = nfserr_clientid_busy; goto out; }
Signature · 1 line
1.7.10.2