* mainly setclientid/confirm cleanup
@ 2012-05-23 21:40 J. Bruce Fields
2012-05-23 21:40 ` [PATCH 01/10] nfsd4: setclientid remove unnecessary terms from a logical expression J. Bruce Fields
` (9 more replies)
0 siblings, 10 replies; 11+ messages in thread
From: J. Bruce Fields @ 2012-05-23 21:40 UTC (permalink / raw)
To: linux-nfs
While working on some locking fixes I ran into trouble understanding the
setclientid and setclientid_confirm code. So, here's some cleanup and
minor bugfixes.
The result is a lot simpler.
--b.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 01/10] nfsd4: setclientid remove unnecessary terms from a logical expression
2012-05-23 21:40 mainly setclientid/confirm cleanup J. Bruce Fields
@ 2012-05-23 21:40 ` J. Bruce Fields
2012-05-23 21:40 ` [PATCH 02/10] nfsd4: setclientid/confirm comment cleanup J. Bruce Fields
` (8 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: J. Bruce Fields @ 2012-05-23 21:40 UTC (permalink / raw)
To: linux-nfs; +Cc: J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/nfsd/nfs4state.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 21266c7..3dc0289 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2344,9 +2344,8 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
nfsd4_probe_callback(conf);
status = nfs_ok;
}
- } else if ((!conf || (conf && !same_verf(&conf->cl_confirm, &confirm)))
- && (!unconf || (unconf && !same_verf(&unconf->cl_confirm,
- &confirm)))) {
+ } else if ((!conf || !same_verf(&conf->cl_confirm, &confirm))
+ && (!unconf || !same_verf(&unconf->cl_confirm, &confirm))) {
/*
* RFC 3530 14.2.34 CASE 4:
* Client probably hasn't noticed that we rebooted yet.
--
1.7.9.5
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 02/10] nfsd4: setclientid/confirm comment cleanup
2012-05-23 21:40 mainly setclientid/confirm cleanup J. Bruce Fields
2012-05-23 21:40 ` [PATCH 01/10] nfsd4: setclientid remove unnecessary terms from a logical expression J. Bruce Fields
@ 2012-05-23 21:40 ` J. Bruce Fields
2012-05-23 21:40 ` [PATCH 03/10] nfsd4: merge last two setclientid cases J. Bruce Fields
` (7 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: J. Bruce Fields @ 2012-05-23 21:40 UTC (permalink / raw)
To: linux-nfs; +Cc: J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
Be a little more concise.
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/nfsd/nfs4state.c | 67 +++++++++------------------------------------------
1 file changed, 11 insertions(+), 56 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 3dc0289..4ce1045 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2169,17 +2169,13 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
if (status)
return status;
- /*
- * XXX The Duplicate Request Cache (DRC) has been checked (??)
- * We get here on a DRC miss.
- */
-
strhashval = clientstr_hashval(dname);
+ /* Cases below refer to rfc 3530 section 14.2.33: */
nfs4_lock_state();
conf = find_confirmed_client_by_str(dname, strhashval);
if (conf) {
- /* RFC 3530 14.2.33 CASE 0: */
+ /* case 0: */
status = nfserr_clid_inuse;
if (clp_used_exchangeid(conf))
goto out;
@@ -2192,18 +2188,10 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
goto out;
}
}
- /*
- * section 14.2.33 of RFC 3530 (under the heading "IMPLEMENTATION")
- * has a description of SETCLIENTID request processing consisting
- * of 5 bullet points, labeled as CASE0 - CASE4 below.
- */
unconf = find_unconfirmed_client_by_str(dname, strhashval);
status = nfserr_jukebox;
if (!conf) {
- /*
- * RFC 3530 14.2.33 CASE 4:
- * placed first, because it is the normal case
- */
+ /* case 4: placed first, because it's the normal case */
if (unconf)
expire_client(unconf);
new = create_client(clname, dname, rqstp, &clverifier);
@@ -2211,10 +2199,7 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
goto out;
gen_clid(new);
} else if (same_verf(&conf->cl_verifier, &clverifier)) {
- /*
- * RFC 3530 14.2.33 CASE 1:
- * probable callback update
- */
+ /* case 1: probable callback update */
if (unconf) {
/* Note this is removing unconfirmed {*x***},
* which is stronger than RFC recommended {vxc**}.
@@ -2228,21 +2213,13 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
goto out;
copy_clid(new, conf);
} else if (!unconf) {
- /*
- * RFC 3530 14.2.33 CASE 2:
- * probable client reboot; state will be removed if
- * confirmed.
- */
+ /* case 2: probable client reboot: */
new = create_client(clname, dname, rqstp, &clverifier);
if (new == NULL)
goto out;
gen_clid(new);
} else {
- /*
- * RFC 3530 14.2.33 CASE 3:
- * probable client reboot; state will be removed if
- * confirmed.
- */
+ /* case 3: probable client reboot: */
expire_client(unconf);
new = create_client(clname, dname, rqstp, &clverifier);
if (new == NULL)
@@ -2266,11 +2243,6 @@ out:
}
-/*
- * Section 14.2.34 of RFC 3530 (under the heading "IMPLEMENTATION") has
- * a description of SETCLIENTID_CONFIRM request processing consisting of 4
- * bullets, labeled as CASE1 - CASE4 below.
- */
__be32
nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
struct nfsd4_compound_state *cstate,
@@ -2293,17 +2265,10 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
conf = find_confirmed_client(clid);
unconf = find_unconfirmed_client(clid);
- /*
- * section 14.2.34 of RFC 3530 has a description of
- * SETCLIENTID_CONFIRM request processing consisting
- * of 4 bullet points, labeled as CASE1 - CASE4 below.
- */
+ /* cases below refer to rfc 3530 section 14.2.34: */
status = nfserr_clid_inuse;
if (conf && unconf && same_verf(&confirm, &unconf->cl_confirm)) {
- /*
- * RFC 3530 14.2.34 CASE 1:
- * callback update
- */
+ /* case 1: callback update */
if (!same_creds(&conf->cl_cred, &unconf->cl_cred))
status = nfserr_clid_inuse;
else {
@@ -2313,21 +2278,14 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
status = nfs_ok;
}
} else if (conf && !unconf) {
- /*
- * RFC 3530 14.2.34 CASE 2:
- * probable retransmitted request; play it safe and
- * do nothing.
- */
+ /* case 2: probable retransmit: */
if (!same_creds(&conf->cl_cred, &rqstp->rq_cred))
status = nfserr_clid_inuse;
else
status = nfs_ok;
} else if (!conf && unconf
&& same_verf(&unconf->cl_confirm, &confirm)) {
- /*
- * RFC 3530 14.2.34 CASE 3:
- * Normal case; new or rebooted client:
- */
+ /* case 3: normal case; new or rebooted client */
if (!same_creds(&unconf->cl_cred, &rqstp->rq_cred)) {
status = nfserr_clid_inuse;
} else {
@@ -2346,10 +2304,7 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
}
} else if ((!conf || !same_verf(&conf->cl_confirm, &confirm))
&& (!unconf || !same_verf(&unconf->cl_confirm, &confirm))) {
- /*
- * RFC 3530 14.2.34 CASE 4:
- * Client probably hasn't noticed that we rebooted yet.
- */
+ /* case 4: client hasn't noticed we rebooted yet? */
status = nfserr_stale_clientid;
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 03/10] nfsd4: merge last two setclientid cases
2012-05-23 21:40 mainly setclientid/confirm cleanup J. Bruce Fields
2012-05-23 21:40 ` [PATCH 01/10] nfsd4: setclientid remove unnecessary terms from a logical expression J. Bruce Fields
2012-05-23 21:40 ` [PATCH 02/10] nfsd4: setclientid/confirm comment cleanup J. Bruce Fields
@ 2012-05-23 21:40 ` J. Bruce Fields
2012-05-23 21:40 ` [PATCH 04/10] nfsd4: pull out common code from " J. Bruce Fields
` (6 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: J. Bruce Fields @ 2012-05-23 21:40 UTC (permalink / raw)
To: linux-nfs; +Cc: J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
The code here is mostly the same.
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/nfsd/nfs4state.c | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 4ce1045..b4e9c35 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2212,15 +2212,10 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
if (new == NULL)
goto out;
copy_clid(new, conf);
- } else if (!unconf) {
- /* case 2: probable client reboot: */
- new = create_client(clname, dname, rqstp, &clverifier);
- if (new == NULL)
- goto out;
- gen_clid(new);
- } else {
- /* case 3: probable client reboot: */
- expire_client(unconf);
+ } else { /* conf && !same_verf(): */
+ /* cases 2, 3: probable client reboot: */
+ if (unconf)
+ expire_client(unconf);
new = create_client(clname, dname, rqstp, &clverifier);
if (new == NULL)
goto out;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 04/10] nfsd4: pull out common code from setclientid cases
2012-05-23 21:40 mainly setclientid/confirm cleanup J. Bruce Fields
` (2 preceding siblings ...)
2012-05-23 21:40 ` [PATCH 03/10] nfsd4: merge last two setclientid cases J. Bruce Fields
@ 2012-05-23 21:40 ` J. Bruce Fields
2012-05-23 21:40 ` [PATCH 05/10] nfsd4: merge 3 setclientid cases to 2 J. Bruce Fields
` (5 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: J. Bruce Fields @ 2012-05-23 21:40 UTC (permalink / raw)
To: linux-nfs; +Cc: J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/nfsd/nfs4state.c | 26 +++++---------------------
1 file changed, 5 insertions(+), 21 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index b4e9c35..af916a7 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2189,36 +2189,20 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
}
}
unconf = find_unconfirmed_client_by_str(dname, strhashval);
+ if (unconf)
+ expire_client(unconf);
status = nfserr_jukebox;
+ new = create_client(clname, dname, rqstp, &clverifier);
+ if (new == NULL)
+ goto out;
if (!conf) {
/* case 4: placed first, because it's the normal case */
- if (unconf)
- expire_client(unconf);
- new = create_client(clname, dname, rqstp, &clverifier);
- if (new == NULL)
- goto out;
gen_clid(new);
} else if (same_verf(&conf->cl_verifier, &clverifier)) {
/* case 1: probable callback update */
- if (unconf) {
- /* Note this is removing unconfirmed {*x***},
- * which is stronger than RFC recommended {vxc**}.
- * This has the advantage that there is at most
- * one {*x***} in either list at any time.
- */
- expire_client(unconf);
- }
- new = create_client(clname, dname, rqstp, &clverifier);
- if (new == NULL)
- goto out;
copy_clid(new, conf);
} else { /* conf && !same_verf(): */
/* cases 2, 3: probable client reboot: */
- if (unconf)
- expire_client(unconf);
- new = create_client(clname, dname, rqstp, &clverifier);
- if (new == NULL)
- goto out;
gen_clid(new);
}
/*
--
1.7.9.5
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 05/10] nfsd4: merge 3 setclientid cases to 2
2012-05-23 21:40 mainly setclientid/confirm cleanup J. Bruce Fields
` (3 preceding siblings ...)
2012-05-23 21:40 ` [PATCH 04/10] nfsd4: pull out common code from " J. Bruce Fields
@ 2012-05-23 21:40 ` J. Bruce Fields
2012-05-23 21:40 ` [PATCH 06/10] nfsd4: fix setclientid_confirm same_cred check J. Bruce Fields
` (4 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: J. Bruce Fields @ 2012-05-23 21:40 UTC (permalink / raw)
To: linux-nfs; +Cc: J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
Boy, is this simpler.
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/nfsd/nfs4state.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index af916a7..2d28abf 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2195,16 +2195,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
new = create_client(clname, dname, rqstp, &clverifier);
if (new == NULL)
goto out;
- if (!conf) {
- /* case 4: placed first, because it's the normal case */
- gen_clid(new);
- } else if (same_verf(&conf->cl_verifier, &clverifier)) {
+ if (conf && same_verf(&conf->cl_verifier, &clverifier))
/* case 1: probable callback update */
copy_clid(new, conf);
- } else { /* conf && !same_verf(): */
- /* cases 2, 3: probable client reboot: */
+ else /* case 4 (new client) or cases 2, 3 (client reboot): */
gen_clid(new);
- }
/*
* XXX: we should probably set this at creation time, and check
* for consistent minorversion use throughout:
--
1.7.9.5
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 06/10] nfsd4: fix setclientid_confirm same_cred check
2012-05-23 21:40 mainly setclientid/confirm cleanup J. Bruce Fields
` (4 preceding siblings ...)
2012-05-23 21:40 ` [PATCH 05/10] nfsd4: merge 3 setclientid cases to 2 J. Bruce Fields
@ 2012-05-23 21:40 ` J. Bruce Fields
2012-05-23 21:40 ` [PATCH 07/10] nfsd4: fix error return in non-matching-creds case J. Bruce Fields
` (3 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: J. Bruce Fields @ 2012-05-23 21:40 UTC (permalink / raw)
To: linux-nfs; +Cc: J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
New clients are created only by nfsd4_setclientid(), which always gives
any new client a unique clientid. The only exception is in the
"callback update" case, in which case it may create an unconfirmed
client with the same clientid as a confirmed client. In that case it
also checks that the confirmed client has the same credential.
Therefore, it is pointless for setclientid_confirm to check whether a
confirmed and unconfirmed client with the same clientid have matching
credentials--they're guaranteed to.
Instead, it should be checking whether the credential on the
setclientid_confirm matches either of those. Otherwise, it could be
anyone sending the setclientid_confirm. Granted, I can't see why anyone
would, but still it's probalby safer to check.
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/nfsd/nfs4state.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 2d28abf..286710d 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2243,7 +2243,7 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
status = nfserr_clid_inuse;
if (conf && unconf && same_verf(&confirm, &unconf->cl_confirm)) {
/* case 1: callback update */
- if (!same_creds(&conf->cl_cred, &unconf->cl_cred))
+ if (!same_creds(&conf->cl_cred, &rqstp->rq_cred))
status = nfserr_clid_inuse;
else {
nfsd4_change_callback(conf, &unconf->cl_cb_conn);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 07/10] nfsd4: fix error return in non-matching-creds case
2012-05-23 21:40 mainly setclientid/confirm cleanup J. Bruce Fields
` (5 preceding siblings ...)
2012-05-23 21:40 ` [PATCH 06/10] nfsd4: fix setclientid_confirm same_cred check J. Bruce Fields
@ 2012-05-23 21:40 ` J. Bruce Fields
2012-05-23 21:40 ` [PATCH 08/10] nfsd4: setclientid: remove pointless assignment J. Bruce Fields
` (2 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: J. Bruce Fields @ 2012-05-23 21:40 UTC (permalink / raw)
To: linux-nfs; +Cc: J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
Note CLID_INUSE is for the case where two clients are trying to use the
same client-provided long-form client identifiers. But what we're
looking at here is the server-returned shorthand client id--if those
clash there's a bug somewhere.
Fix the error return, pull the check out into common code, and do the
check unconditionally in all cases.
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/nfsd/nfs4state.c | 62 +++++++++++++++++++++------------------------------
1 file changed, 26 insertions(+), 36 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 286710d..8e77782 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2229,59 +2229,49 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
if (STALE_CLIENTID(clid))
return nfserr_stale_clientid;
- /*
- * XXX The Duplicate Request Cache (DRC) has been checked (??)
- * We get here on a DRC miss.
- */
-
nfs4_lock_state();
conf = find_confirmed_client(clid);
unconf = find_unconfirmed_client(clid);
-
+ /*
+ * We try hard to give out unique clientid's, so if we get an
+ * attempt to confirm the same clientid with a different cred,
+ * there's a bug somewhere. Let's charitably assume it's our
+ * bug.
+ */
+ status = nfserr_serverfault;
+ if (unconf && !same_creds(&unconf->cl_cred, &rqstp->rq_cred))
+ goto out;
+ if (conf && !same_creds(&conf->cl_cred, &rqstp->rq_cred))
+ goto out;
/* cases below refer to rfc 3530 section 14.2.34: */
- status = nfserr_clid_inuse;
if (conf && unconf && same_verf(&confirm, &unconf->cl_confirm)) {
/* case 1: callback update */
- if (!same_creds(&conf->cl_cred, &rqstp->rq_cred))
- status = nfserr_clid_inuse;
- else {
- nfsd4_change_callback(conf, &unconf->cl_cb_conn);
- nfsd4_probe_callback(conf);
- expire_client(unconf);
- status = nfs_ok;
- }
+ nfsd4_change_callback(conf, &unconf->cl_cb_conn);
+ nfsd4_probe_callback(conf);
+ expire_client(unconf);
+ status = nfs_ok;
} else if (conf && !unconf) {
- /* case 2: probable retransmit: */
- if (!same_creds(&conf->cl_cred, &rqstp->rq_cred))
- status = nfserr_clid_inuse;
- else
- status = nfs_ok;
+ status = nfs_ok;
} else if (!conf && unconf
&& same_verf(&unconf->cl_confirm, &confirm)) {
/* case 3: normal case; new or rebooted client */
- if (!same_creds(&unconf->cl_cred, &rqstp->rq_cred)) {
- status = nfserr_clid_inuse;
- } else {
- unsigned int hash =
- clientstr_hashval(unconf->cl_recdir);
- conf = find_confirmed_client_by_str(unconf->cl_recdir,
- hash);
- if (conf) {
- nfsd4_client_record_remove(conf);
- expire_client(conf);
- }
- move_to_confirmed(unconf);
- conf = unconf;
- nfsd4_probe_callback(conf);
- status = nfs_ok;
+ unsigned int hash = clientstr_hashval(unconf->cl_recdir);
+ conf = find_confirmed_client_by_str(unconf->cl_recdir, hash);
+ if (conf) {
+ nfsd4_client_record_remove(conf);
+ expire_client(conf);
}
+ move_to_confirmed(unconf);
+ conf = unconf;
+ nfsd4_probe_callback(conf);
+ status = nfs_ok;
} else if ((!conf || !same_verf(&conf->cl_confirm, &confirm))
&& (!unconf || !same_verf(&unconf->cl_confirm, &confirm))) {
/* case 4: client hasn't noticed we rebooted yet? */
status = nfserr_stale_clientid;
}
-
+out:
nfs4_unlock_state();
return status;
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 08/10] nfsd4: setclientid: remove pointless assignment
2012-05-23 21:40 mainly setclientid/confirm cleanup J. Bruce Fields
` (6 preceding siblings ...)
2012-05-23 21:40 ` [PATCH 07/10] nfsd4: fix error return in non-matching-creds case J. Bruce Fields
@ 2012-05-23 21:40 ` J. Bruce Fields
2012-05-23 21:40 ` [PATCH 09/10] nfsd4: simpler ordering of setclientid_confirm checks J. Bruce Fields
2012-05-23 21:40 ` [PATCH 10/10] nfsd4: clarify that renewing expired client is a bug J. Bruce Fields
9 siblings, 0 replies; 11+ messages in thread
From: J. Bruce Fields @ 2012-05-23 21:40 UTC (permalink / raw)
To: linux-nfs; +Cc: J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/nfsd/nfs4state.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 8e77782..e8e3ce0 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2263,8 +2263,7 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
expire_client(conf);
}
move_to_confirmed(unconf);
- conf = unconf;
- nfsd4_probe_callback(conf);
+ nfsd4_probe_callback(unconf);
status = nfs_ok;
} else if ((!conf || !same_verf(&conf->cl_confirm, &confirm))
&& (!unconf || !same_verf(&unconf->cl_confirm, &confirm))) {
--
1.7.9.5
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 09/10] nfsd4: simpler ordering of setclientid_confirm checks
2012-05-23 21:40 mainly setclientid/confirm cleanup J. Bruce Fields
` (7 preceding siblings ...)
2012-05-23 21:40 ` [PATCH 08/10] nfsd4: setclientid: remove pointless assignment J. Bruce Fields
@ 2012-05-23 21:40 ` J. Bruce Fields
2012-05-23 21:40 ` [PATCH 10/10] nfsd4: clarify that renewing expired client is a bug J. Bruce Fields
9 siblings, 0 replies; 11+ messages in thread
From: J. Bruce Fields @ 2012-05-23 21:40 UTC (permalink / raw)
To: linux-nfs; +Cc: J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
The cases here divide into two main categories:
- if there's an uncomfirmed record with a matching verifier,
then this is a "normal", succesful case: we're either creating
a new client, or updating an existing one.
- otherwise, this is a weird case: a replay, or a server reboot.
Reordering to reflect that makes the code a bit more concise and the
logic a lot easier to understand.
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/nfsd/nfs4state.c | 24 +++++++++++-------------
1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index e8e3ce0..72bc277 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2245,18 +2245,21 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
if (conf && !same_creds(&conf->cl_cred, &rqstp->rq_cred))
goto out;
/* cases below refer to rfc 3530 section 14.2.34: */
- if (conf && unconf && same_verf(&confirm, &unconf->cl_confirm)) {
- /* case 1: callback update */
+ if (!unconf || !same_verf(&confirm, &unconf->cl_confirm)) {
+ if (conf && !unconf) /* case 2: probable retransmit */
+ status = nfs_ok;
+ else /* case 4: client hasn't noticed we rebooted yet? */
+ status = nfserr_stale_clientid;
+ goto out;
+ }
+ status = nfs_ok;
+ if (conf) { /* case 1: callback update */
nfsd4_change_callback(conf, &unconf->cl_cb_conn);
nfsd4_probe_callback(conf);
expire_client(unconf);
- status = nfs_ok;
- } else if (conf && !unconf) {
- status = nfs_ok;
- } else if (!conf && unconf
- && same_verf(&unconf->cl_confirm, &confirm)) {
- /* case 3: normal case; new or rebooted client */
+ } else { /* case 3: normal case; new or rebooted client */
unsigned int hash = clientstr_hashval(unconf->cl_recdir);
+
conf = find_confirmed_client_by_str(unconf->cl_recdir, hash);
if (conf) {
nfsd4_client_record_remove(conf);
@@ -2264,11 +2267,6 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
}
move_to_confirmed(unconf);
nfsd4_probe_callback(unconf);
- status = nfs_ok;
- } else if ((!conf || !same_verf(&conf->cl_confirm, &confirm))
- && (!unconf || !same_verf(&unconf->cl_confirm, &confirm))) {
- /* case 4: client hasn't noticed we rebooted yet? */
- status = nfserr_stale_clientid;
}
out:
nfs4_unlock_state();
--
1.7.9.5
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 10/10] nfsd4: clarify that renewing expired client is a bug
2012-05-23 21:40 mainly setclientid/confirm cleanup J. Bruce Fields
` (8 preceding siblings ...)
2012-05-23 21:40 ` [PATCH 09/10] nfsd4: simpler ordering of setclientid_confirm checks J. Bruce Fields
@ 2012-05-23 21:40 ` J. Bruce Fields
9 siblings, 0 replies; 11+ messages in thread
From: J. Bruce Fields @ 2012-05-23 21:40 UTC (permalink / raw)
To: linux-nfs; +Cc: J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
This can't happen:
- cl_time is zeroed only by unhash_client_locked, which is only
ever called under both the state lock and the client lock.
- every caller of renew_client() should have looked up a
(non-expired) client and then called renew_client() all
without dropping the state lock.
- the only other caller of renew_client_locked() is
release_session_client(), which first checks under the
client_lock that the cl_time is nonzero.
So make it clear that this is a bug, not something we handle. I can't
quite bring myself to make this a BUG(), though, as there are a lot of
renew_client() callers, and returning here is probably safer than a
BUG().
We'll consider making it a BUG() after some more cleanup.
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/nfsd/nfs4state.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 72bc277..be1fc97 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1022,7 +1022,8 @@ static inline void
renew_client_locked(struct nfs4_client *clp)
{
if (is_client_expired(clp)) {
- dprintk("%s: client (clientid %08x/%08x) already expired\n",
+ WARN_ON(1);
+ printk("%s: client (clientid %08x/%08x) already expired\n",
__func__,
clp->cl_clientid.cl_boot,
clp->cl_clientid.cl_id);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2012-05-23 21:40 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-05-23 21:40 mainly setclientid/confirm cleanup J. Bruce Fields
2012-05-23 21:40 ` [PATCH 01/10] nfsd4: setclientid remove unnecessary terms from a logical expression J. Bruce Fields
2012-05-23 21:40 ` [PATCH 02/10] nfsd4: setclientid/confirm comment cleanup J. Bruce Fields
2012-05-23 21:40 ` [PATCH 03/10] nfsd4: merge last two setclientid cases J. Bruce Fields
2012-05-23 21:40 ` [PATCH 04/10] nfsd4: pull out common code from " J. Bruce Fields
2012-05-23 21:40 ` [PATCH 05/10] nfsd4: merge 3 setclientid cases to 2 J. Bruce Fields
2012-05-23 21:40 ` [PATCH 06/10] nfsd4: fix setclientid_confirm same_cred check J. Bruce Fields
2012-05-23 21:40 ` [PATCH 07/10] nfsd4: fix error return in non-matching-creds case J. Bruce Fields
2012-05-23 21:40 ` [PATCH 08/10] nfsd4: setclientid: remove pointless assignment J. Bruce Fields
2012-05-23 21:40 ` [PATCH 09/10] nfsd4: simpler ordering of setclientid_confirm checks J. Bruce Fields
2012-05-23 21:40 ` [PATCH 10/10] nfsd4: clarify that renewing expired client is a bug J. Bruce Fields
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.