From: "J. Bruce Fields" <[email protected]>
This is some miscellaneous cleanup of NFSv4 state code that I ran into
while working on another project.
I think it's all pretty routine. The only behavior difference should be
changing the error returned in some situations where there are multiple
reasons an operation could fail; none look like a problem to me.
--b.
J. Bruce Fields (8):
nfsd4: simplify process_lookup1
nfsd: simplify process_lock
nfsd: simplify nfsd_renew
nfsd: refactor lookup_clientid
nfsd: find_cpntf_state cleanup
nfsd: remove unused set_clientid argument
nfsd: simplify nfsd4_check_open_reclaim
nfsd: cstate->session->se_client -> cstate->clp
fs/nfsd/nfs4proc.c | 8 ++-
fs/nfsd/nfs4state.c | 125 ++++++++++++++++++--------------------------
fs/nfsd/state.h | 3 +-
3 files changed, 56 insertions(+), 80 deletions(-)
--
2.29.2
From: "J. Bruce Fields" <[email protected]>
Similarly, this STALE_CLIENTID check is already handled inside
preprocess_confirmed_seqid_op().
(This may cause it to return a different error in some cases where
there are multiple things wrong; pynfs test SEQ10 regressed on this
commit because of that, but I think that's the test's fault, and I've
fixed it separately.)
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4state.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index f9f89229dba6..7ea63d7cec4d 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -6697,10 +6697,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
&cstate->session->se_client->cl_clientid,
sizeof(clientid_t));
- status = nfserr_stale_clientid;
- if (STALE_CLIENTID(&lock->lk_new_clientid, nn))
- goto out;
-
/* validate and update open stateid and open seqid */
status = nfs4_preprocess_confirmed_seqid_op(cstate,
lock->lk_new_open_seqid,
--
2.29.2
From: "J. Bruce Fields" <[email protected]>
This STALE_CLIENTID check is redundant with the one in
lookup_clientid().
There's a difference in behavior is in case of memory allocation
failure, which I think isn't a big deal.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4state.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 1d2cd6a88f61..f9f89229dba6 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4680,8 +4680,6 @@ nfsd4_process_open1(struct nfsd4_compound_state *cstate,
struct nfs4_openowner *oo = NULL;
__be32 status;
- if (STALE_CLIENTID(&open->op_clientid, nn))
- return nfserr_stale_clientid;
/*
* In case we need it later, after we've already created the
* file and don't want to risk a further failure:
--
2.29.2
From: "J. Bruce Fields" <[email protected]>
I'm not sure why we're writing this out the hard way in so many places.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4proc.c | 5 ++---
fs/nfsd/nfs4state.c | 16 ++++++++--------
2 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 567af1f10d2c..f63a12a5278a 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -373,8 +373,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
* Before RECLAIM_COMPLETE done, server should deny new lock
*/
if (nfsd4_has_session(cstate) &&
- !test_bit(NFSD4_CLIENT_RECLAIM_COMPLETE,
- &cstate->session->se_client->cl_flags) &&
+ !test_bit(NFSD4_CLIENT_RECLAIM_COMPLETE, &cstate->clp->cl_flags) &&
open->op_claim_type != NFS4_OPEN_CLAIM_PREVIOUS)
return nfserr_grace;
@@ -1882,7 +1881,7 @@ nfsd4_getdeviceinfo(struct svc_rqst *rqstp,
nfserr = nfs_ok;
if (gdp->gd_maxcount != 0) {
nfserr = ops->proc_getdeviceinfo(exp->ex_path.mnt->mnt_sb,
- rqstp, cstate->session->se_client, gdp);
+ rqstp, cstate->clp, gdp);
}
gdp->gd_notify_types &= ops->notify_types;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 860805ffde1a..7b865ed7c9d7 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3891,6 +3891,7 @@ nfsd4_reclaim_complete(struct svc_rqst *rqstp,
struct nfsd4_compound_state *cstate, union nfsd4_op_u *u)
{
struct nfsd4_reclaim_complete *rc = &u->reclaim_complete;
+ struct nfs4_client *clp = cstate->clp;
__be32 status = 0;
if (rc->rca_one_fs) {
@@ -3904,12 +3905,11 @@ nfsd4_reclaim_complete(struct svc_rqst *rqstp,
}
status = nfserr_complete_already;
- if (test_and_set_bit(NFSD4_CLIENT_RECLAIM_COMPLETE,
- &cstate->session->se_client->cl_flags))
+ if (test_and_set_bit(NFSD4_CLIENT_RECLAIM_COMPLETE, &clp->cl_flags))
goto out;
status = nfserr_stale_clientid;
- if (is_client_expired(cstate->session->se_client))
+ if (is_client_expired(clp))
/*
* The following error isn't really legal.
* But we only get here if the client just explicitly
@@ -3920,8 +3920,8 @@ nfsd4_reclaim_complete(struct svc_rqst *rqstp,
goto out;
status = nfs_ok;
- nfsd4_client_record_create(cstate->session->se_client);
- inc_reclaim_complete(cstate->session->se_client);
+ nfsd4_client_record_create(clp);
+ inc_reclaim_complete(clp);
out:
return status;
}
@@ -5917,7 +5917,7 @@ nfsd4_test_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
{
struct nfsd4_test_stateid *test_stateid = &u->test_stateid;
struct nfsd4_test_stateid_id *stateid;
- struct nfs4_client *cl = cstate->session->se_client;
+ struct nfs4_client *cl = cstate->clp;
list_for_each_entry(stateid, &test_stateid->ts_stateid_list, ts_id_list)
stateid->ts_id_status =
@@ -5963,7 +5963,7 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
stateid_t *stateid = &free_stateid->fr_stateid;
struct nfs4_stid *s;
struct nfs4_delegation *dp;
- struct nfs4_client *cl = cstate->session->se_client;
+ struct nfs4_client *cl = cstate->clp;
__be32 ret = nfserr_bad_stateid;
spin_lock(&cl->cl_lock);
@@ -6692,7 +6692,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
if (nfsd4_has_session(cstate))
/* See rfc 5661 18.10.3: given clientid is ignored: */
memcpy(&lock->lk_new_clientid,
- &cstate->session->se_client->cl_clientid,
+ &cstate->clp->cl_clientid,
sizeof(clientid_t));
/* validate and update open stateid and open seqid */
--
2.29.2
From: "J. Bruce Fields" <[email protected]>
Every caller is setting this argument to false, so we don't need it.
Also clarify comments a little.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4state.c | 24 ++++++++++--------------
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 01191d2dddc8..2c580ef6e337 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4648,8 +4648,7 @@ static struct nfs4_client *lookup_clientid(clientid_t *clid, bool sessions,
static __be32 set_client(clientid_t *clid,
struct nfsd4_compound_state *cstate,
- struct nfsd_net *nn,
- bool sessions)
+ struct nfsd_net *nn)
{
if (cstate->clp) {
if (!same_clid(&cstate->clp->cl_clientid, clid))
@@ -4658,13 +4657,10 @@ static __be32 set_client(clientid_t *clid,
}
if (STALE_CLIENTID(clid, nn))
return nfserr_stale_clientid;
- /*
- * For v4.1+ we get the client in the SEQUENCE op. If we don't have one
- * cached already then we know this is for is for v4.0 and "sessions"
- * will be false.
- */
+ /* For v4.1+ we should have gotten the client in the SEQUENCE op: */
WARN_ON_ONCE(cstate->session);
- cstate->clp = lookup_clientid(clid, sessions, nn);
+ /* So we're looking for a 4.0 client (sessions = false): */
+ cstate->clp = lookup_clientid(clid, false, nn);
if (!cstate->clp)
return nfserr_expired;
return nfs_ok;
@@ -4688,7 +4684,7 @@ nfsd4_process_open1(struct nfsd4_compound_state *cstate,
if (open->op_file == NULL)
return nfserr_jukebox;
- status = set_client(clientid, cstate, nn, false);
+ status = set_client(clientid, cstate, nn);
if (status)
return status;
clp = cstate->clp;
@@ -5298,7 +5294,7 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
trace_nfsd_clid_renew(clid);
- status = set_client(clid, cstate, nn, false);
+ status = set_client(clid, cstate, nn);
if (status)
return status;
clp = cstate->clp;
@@ -5681,7 +5677,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
CLOSE_STATEID(stateid))
return nfserr_bad_stateid;
- status = set_client(&stateid->si_opaque.so_clid, cstate, nn, false);
+ status = set_client(&stateid->si_opaque.so_clid, cstate, nn);
if (status == nfserr_stale_clientid) {
if (cstate->session)
return nfserr_bad_stateid;
@@ -6905,7 +6901,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
return nfserr_inval;
if (!nfsd4_has_session(cstate)) {
- status = set_client(&lockt->lt_clientid, cstate, nn, false);
+ status = set_client(&lockt->lt_clientid, cstate, nn);
if (status)
goto out;
}
@@ -7089,7 +7085,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
dprintk("nfsd4_release_lockowner clientid: (%08x/%08x):\n",
clid->cl_boot, clid->cl_id);
- status = set_client(clid, cstate, nn, false);
+ status = set_client(clid, cstate, nn);
if (status)
return status;
@@ -7236,7 +7232,7 @@ nfs4_check_open_reclaim(clientid_t *clid,
__be32 status;
/* find clientid in conf_id_hashtbl */
- status = set_clientid(clid, cstate, nn, false);
+ status = set_clientid(clid, cstate, nn);
if (status)
return nfserr_reclaim_bad;
--
2.29.2
From: "J. Bruce Fields" <[email protected]>
The set_client() was already taken care of by process_open1().
The comments here are mostly redundant with the code.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4proc.c | 3 +--
fs/nfsd/nfs4state.c | 18 +++---------------
fs/nfsd/state.h | 3 +--
3 files changed, 5 insertions(+), 19 deletions(-)
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 4727b7f03c5b..567af1f10d2c 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -423,8 +423,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
goto out;
break;
case NFS4_OPEN_CLAIM_PREVIOUS:
- status = nfs4_check_open_reclaim(&open->op_clientid,
- cstate, nn);
+ status = nfs4_check_open_reclaim(cstate->clp);
if (status)
goto out;
open->op_openowner->oo_flags |= NFS4_OO_CONFIRMED;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 2c580ef6e337..860805ffde1a 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -7221,25 +7221,13 @@ nfsd4_find_reclaim_client(struct xdr_netobj name, struct nfsd_net *nn)
return NULL;
}
-/*
-* Called from OPEN. Look for clientid in reclaim list.
-*/
__be32
-nfs4_check_open_reclaim(clientid_t *clid,
- struct nfsd4_compound_state *cstate,
- struct nfsd_net *nn)
+nfs4_check_open_reclaim(struct nfs4_client *clp)
{
- __be32 status;
-
- /* find clientid in conf_id_hashtbl */
- status = set_clientid(clid, cstate, nn);
- if (status)
- return nfserr_reclaim_bad;
-
- if (test_bit(NFSD4_CLIENT_RECLAIM_COMPLETE, &cstate->clp->cl_flags))
+ if (test_bit(NFSD4_CLIENT_RECLAIM_COMPLETE, &clp->cl_flags))
return nfserr_no_grace;
- if (nfsd4_client_record_check(cstate->clp))
+ if (nfsd4_client_record_check(clp))
return nfserr_reclaim_bad;
return nfs_ok;
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 9eae11a9d21c..73deea353169 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -649,8 +649,7 @@ void nfs4_remove_reclaim_record(struct nfs4_client_reclaim *, struct nfsd_net *)
extern void nfs4_release_reclaim(struct nfsd_net *);
extern struct nfs4_client_reclaim *nfsd4_find_reclaim_client(struct xdr_netobj name,
struct nfsd_net *nn);
-extern __be32 nfs4_check_open_reclaim(clientid_t *clid,
- struct nfsd4_compound_state *cstate, struct nfsd_net *nn);
+extern __be32 nfs4_check_open_reclaim(struct nfs4_client *);
extern void nfsd4_probe_callback(struct nfs4_client *clp);
extern void nfsd4_probe_callback_sync(struct nfs4_client *clp);
extern void nfsd4_change_callback(struct nfs4_client *clp, struct nfs4_cb_conn *);
--
2.29.2
From: "J. Bruce Fields" <[email protected]>
I think this unusual use of struct compound_state could cause confusion.
It's not that much more complicated just to open-code this stateid
lookup.
The only change in behavior should be a different error return in the
case the copy is using a source stateid that is a revoked delegation,
but I doubt that matters.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4state.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index d7128e16c86e..01191d2dddc8 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5812,21 +5812,27 @@ static __be32 find_cpntf_state(struct nfsd_net *nn, stateid_t *st,
{
__be32 status;
struct nfs4_cpntf_state *cps = NULL;
- struct nfsd4_compound_state cstate;
+ struct nfs4_client *found;
status = manage_cpntf_state(nn, st, NULL, &cps);
if (status)
return status;
cps->cpntf_time = ktime_get_boottime_seconds();
- memset(&cstate, 0, sizeof(cstate));
- status = set_clientid(&cps->cp_p_clid, &cstate, nn, true);
- if (status)
+
+ status = nfserr_expired;
+ found = lookup_clientid(&cps->cp_p_clid, true, nn);
+ if (!found)
goto out;
- status = nfsd4_lookup_stateid(&cstate, &cps->cp_p_stateid,
- NFS4_DELEG_STID|NFS4_OPEN_STID|NFS4_LOCK_STID,
- stid, nn);
- put_client_renew(cstate.clp);
+
+ *stid = find_stateid_by_type(found, &cps->cp_p_stateid,
+ NFS4_DELEG_STID|NFS4_OPEN_STID|NFS4_LOCK_STID);
+ if (stid)
+ status = nfs_ok;
+ else
+ status = nfserr_bad_stateid;
+
+ put_client_renew(found);
out:
nfs4_put_cpntf_state(nn, cps);
return status;
--
2.29.2
From: "J. Bruce Fields" <[email protected]>
You can take the single-exit thing too far, I think.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4state.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 7ea63d7cec4d..ba955bbf21df 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5300,15 +5300,12 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
trace_nfsd_clid_renew(clid);
status = lookup_clientid(clid, cstate, nn, false);
if (status)
- goto out;
+ return status;
clp = cstate->clp;
- status = nfserr_cb_path_down;
if (!list_empty(&clp->cl_delegations)
&& clp->cl_cb_state != NFSD4_CB_UP)
- goto out;
- status = nfs_ok;
-out:
- return status;
+ return nfserr_cb_path_down;
+ return nfs_ok;
}
void
--
2.29.2
From: "J. Bruce Fields" <[email protected]>
I think set_client is a better name, and the lookup itself I want to
use somewhere else.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4state.c | 50 ++++++++++++++++++++++-----------------------
1 file changed, 24 insertions(+), 26 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index ba955bbf21df..d7128e16c86e 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4633,40 +4633,40 @@ static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate, struct nfs4
return nfserr_bad_seqid;
}
-static __be32 lookup_clientid(clientid_t *clid,
+static struct nfs4_client *lookup_clientid(clientid_t *clid, bool sessions,
+ struct nfsd_net *nn)
+{
+ struct nfs4_client *found;
+
+ spin_lock(&nn->client_lock);
+ found = find_confirmed_client(clid, sessions, nn);
+ if (found)
+ atomic_inc(&found->cl_rpc_users);
+ spin_unlock(&nn->client_lock);
+ return found;
+}
+
+static __be32 set_client(clientid_t *clid,
struct nfsd4_compound_state *cstate,
struct nfsd_net *nn,
bool sessions)
{
- struct nfs4_client *found;
-
if (cstate->clp) {
- found = cstate->clp;
- if (!same_clid(&found->cl_clientid, clid))
+ if (!same_clid(&cstate->clp->cl_clientid, clid))
return nfserr_stale_clientid;
return nfs_ok;
}
-
if (STALE_CLIENTID(clid, nn))
return nfserr_stale_clientid;
-
/*
* For v4.1+ we get the client in the SEQUENCE op. If we don't have one
* cached already then we know this is for is for v4.0 and "sessions"
* will be false.
*/
WARN_ON_ONCE(cstate->session);
- spin_lock(&nn->client_lock);
- found = find_confirmed_client(clid, sessions, nn);
- if (!found) {
- spin_unlock(&nn->client_lock);
+ cstate->clp = lookup_clientid(clid, sessions, nn);
+ if (!cstate->clp)
return nfserr_expired;
- }
- atomic_inc(&found->cl_rpc_users);
- spin_unlock(&nn->client_lock);
-
- /* Cache the nfs4_client in cstate! */
- cstate->clp = found;
return nfs_ok;
}
@@ -4688,7 +4688,7 @@ nfsd4_process_open1(struct nfsd4_compound_state *cstate,
if (open->op_file == NULL)
return nfserr_jukebox;
- status = lookup_clientid(clientid, cstate, nn, false);
+ status = set_client(clientid, cstate, nn, false);
if (status)
return status;
clp = cstate->clp;
@@ -5298,7 +5298,7 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
trace_nfsd_clid_renew(clid);
- status = lookup_clientid(clid, cstate, nn, false);
+ status = set_client(clid, cstate, nn, false);
if (status)
return status;
clp = cstate->clp;
@@ -5681,8 +5681,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
CLOSE_STATEID(stateid))
return nfserr_bad_stateid;
- status = lookup_clientid(&stateid->si_opaque.so_clid, cstate, nn,
- false);
+ status = set_client(&stateid->si_opaque.so_clid, cstate, nn, false);
if (status == nfserr_stale_clientid) {
if (cstate->session)
return nfserr_bad_stateid;
@@ -5821,7 +5820,7 @@ static __be32 find_cpntf_state(struct nfsd_net *nn, stateid_t *st,
cps->cpntf_time = ktime_get_boottime_seconds();
memset(&cstate, 0, sizeof(cstate));
- status = lookup_clientid(&cps->cp_p_clid, &cstate, nn, true);
+ status = set_clientid(&cps->cp_p_clid, &cstate, nn, true);
if (status)
goto out;
status = nfsd4_lookup_stateid(&cstate, &cps->cp_p_stateid,
@@ -6900,8 +6899,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
return nfserr_inval;
if (!nfsd4_has_session(cstate)) {
- status = lookup_clientid(&lockt->lt_clientid, cstate, nn,
- false);
+ status = set_client(&lockt->lt_clientid, cstate, nn, false);
if (status)
goto out;
}
@@ -7085,7 +7083,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
dprintk("nfsd4_release_lockowner clientid: (%08x/%08x):\n",
clid->cl_boot, clid->cl_id);
- status = lookup_clientid(clid, cstate, nn, false);
+ status = set_client(clid, cstate, nn, false);
if (status)
return status;
@@ -7232,7 +7230,7 @@ nfs4_check_open_reclaim(clientid_t *clid,
__be32 status;
/* find clientid in conf_id_hashtbl */
- status = lookup_clientid(clid, cstate, nn, false);
+ status = set_clientid(clid, cstate, nn, false);
if (status)
return nfserr_reclaim_bad;
--
2.29.2
> On Jan 19, 2021, at 5:35 PM, J. Bruce Fields <[email protected]> wrote:
>
> From: "J. Bruce Fields" <[email protected]>
>
> Similarly, this STALE_CLIENTID check is already handled inside
> preprocess_confirmed_seqid_op().
I can't confirm this claim. Where is clid->cl_boot checked?
Did you mean nfs4_preprocess_confirmed_seqid_op() here?
> (This may cause it to return a different error in some cases where
> there are multiple things wrong; pynfs test SEQ10 regressed on this
> commit because of that, but I think that's the test's fault, and I've
> fixed it separately.)
>
> Signed-off-by: J. Bruce Fields <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index f9f89229dba6..7ea63d7cec4d 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -6697,10 +6697,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> &cstate->session->se_client->cl_clientid,
> sizeof(clientid_t));
>
> - status = nfserr_stale_clientid;
> - if (STALE_CLIENTID(&lock->lk_new_clientid, nn))
> - goto out;
> -
> /* validate and update open stateid and open seqid */
> status = nfs4_preprocess_confirmed_seqid_op(cstate,
> lock->lk_new_open_seqid,
> --
> 2.29.2
>
--
Chuck Lever
> On Jan 19, 2021, at 5:35 PM, J. Bruce Fields <[email protected]> wrote:
>
> From: "J. Bruce Fields" <[email protected]>
>
> I think set_client is a better name, and the lookup itself I want to
> use somewhere else.
Should this be two patches?
- Rename lookup_clientid() to set_client()
- Refactor the lookup_clientid() helper
nfs4state.c stops compiling with this patch. See below.
> Signed-off-by: J. Bruce Fields <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 50 ++++++++++++++++++++++-----------------------
> 1 file changed, 24 insertions(+), 26 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index ba955bbf21df..d7128e16c86e 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4633,40 +4633,40 @@ static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate, struct nfs4
> return nfserr_bad_seqid;
> }
>
> -static __be32 lookup_clientid(clientid_t *clid,
> +static struct nfs4_client *lookup_clientid(clientid_t *clid, bool sessions,
> + struct nfsd_net *nn)
> +{
> + struct nfs4_client *found;
> +
> + spin_lock(&nn->client_lock);
> + found = find_confirmed_client(clid, sessions, nn);
> + if (found)
> + atomic_inc(&found->cl_rpc_users);
> + spin_unlock(&nn->client_lock);
> + return found;
> +}
> +
> +static __be32 set_client(clientid_t *clid,
> struct nfsd4_compound_state *cstate,
> struct nfsd_net *nn,
> bool sessions)
> {
> - struct nfs4_client *found;
> -
> if (cstate->clp) {
> - found = cstate->clp;
> - if (!same_clid(&found->cl_clientid, clid))
> + if (!same_clid(&cstate->clp->cl_clientid, clid))
> return nfserr_stale_clientid;
> return nfs_ok;
> }
> -
> if (STALE_CLIENTID(clid, nn))
> return nfserr_stale_clientid;
> -
> /*
> * For v4.1+ we get the client in the SEQUENCE op. If we don't have one
> * cached already then we know this is for is for v4.0 and "sessions"
> * will be false.
> */
> WARN_ON_ONCE(cstate->session);
> - spin_lock(&nn->client_lock);
> - found = find_confirmed_client(clid, sessions, nn);
> - if (!found) {
> - spin_unlock(&nn->client_lock);
> + cstate->clp = lookup_clientid(clid, sessions, nn);
> + if (!cstate->clp)
> return nfserr_expired;
> - }
> - atomic_inc(&found->cl_rpc_users);
> - spin_unlock(&nn->client_lock);
> -
> - /* Cache the nfs4_client in cstate! */
> - cstate->clp = found;
> return nfs_ok;
> }
>
> @@ -4688,7 +4688,7 @@ nfsd4_process_open1(struct nfsd4_compound_state *cstate,
> if (open->op_file == NULL)
> return nfserr_jukebox;
>
> - status = lookup_clientid(clientid, cstate, nn, false);
> + status = set_client(clientid, cstate, nn, false);
> if (status)
> return status;
> clp = cstate->clp;
> @@ -5298,7 +5298,7 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
>
> trace_nfsd_clid_renew(clid);
> - status = lookup_clientid(clid, cstate, nn, false);
> + status = set_client(clid, cstate, nn, false);
> if (status)
> return status;
> clp = cstate->clp;
> @@ -5681,8 +5681,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
> if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
> CLOSE_STATEID(stateid))
> return nfserr_bad_stateid;
> - status = lookup_clientid(&stateid->si_opaque.so_clid, cstate, nn,
> - false);
> + status = set_client(&stateid->si_opaque.so_clid, cstate, nn, false);
> if (status == nfserr_stale_clientid) {
> if (cstate->session)
> return nfserr_bad_stateid;
> @@ -5821,7 +5820,7 @@ static __be32 find_cpntf_state(struct nfsd_net *nn, stateid_t *st,
>
> cps->cpntf_time = ktime_get_boottime_seconds();
> memset(&cstate, 0, sizeof(cstate));
> - status = lookup_clientid(&cps->cp_p_clid, &cstate, nn, true);
> + status = set_clientid(&cps->cp_p_clid, &cstate, nn, true);
This doesn't compile. I think you meant set_client() here.
> if (status)
> goto out;
> status = nfsd4_lookup_stateid(&cstate, &cps->cp_p_stateid,
> @@ -6900,8 +6899,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> return nfserr_inval;
>
> if (!nfsd4_has_session(cstate)) {
> - status = lookup_clientid(&lockt->lt_clientid, cstate, nn,
> - false);
> + status = set_client(&lockt->lt_clientid, cstate, nn, false);
> if (status)
> goto out;
> }
> @@ -7085,7 +7083,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
> dprintk("nfsd4_release_lockowner clientid: (%08x/%08x):\n",
> clid->cl_boot, clid->cl_id);
>
> - status = lookup_clientid(clid, cstate, nn, false);
> + status = set_client(clid, cstate, nn, false);
> if (status)
> return status;
>
> @@ -7232,7 +7230,7 @@ nfs4_check_open_reclaim(clientid_t *clid,
> __be32 status;
>
> /* find clientid in conf_id_hashtbl */
> - status = lookup_clientid(clid, cstate, nn, false);
> + status = set_clientid(clid, cstate, nn, false);
Ditto.
> if (status)
> return nfserr_reclaim_bad;
>
> --
> 2.29.2
>
--
Chuck Lever
On Wed, Jan 20, 2021 at 09:01:53PM +0000, Chuck Lever wrote:
>
>
> > On Jan 19, 2021, at 5:35 PM, J. Bruce Fields <[email protected]> wrote:
> >
> > From: "J. Bruce Fields" <[email protected]>
> >
> > Similarly, this STALE_CLIENTID check is already handled inside
> > preprocess_confirmed_seqid_op().
>
> I can't confirm this claim. Where is clid->cl_boot checked?
nfs4_preprocess_confirmed_seqid_op()->
nfs4_preprocess_seqid_op()->
nfsd4_lookup_stateid()->
set_client()->
STALE_CLIENTID()
> Did you mean nfs4_preprocess_confirmed_seqid_op() here?
Yeah. But maybe it'd be better just to include that whole call stack in
the changelog.
--b.
>
>
> > (This may cause it to return a different error in some cases where
> > there are multiple things wrong; pynfs test SEQ10 regressed on this
> > commit because of that, but I think that's the test's fault, and I've
> > fixed it separately.)
> >
> > Signed-off-by: J. Bruce Fields <[email protected]>
> > ---
> > fs/nfsd/nfs4state.c | 4 ----
> > 1 file changed, 4 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index f9f89229dba6..7ea63d7cec4d 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -6697,10 +6697,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > &cstate->session->se_client->cl_clientid,
> > sizeof(clientid_t));
> >
> > - status = nfserr_stale_clientid;
> > - if (STALE_CLIENTID(&lock->lk_new_clientid, nn))
> > - goto out;
> > -
> > /* validate and update open stateid and open seqid */
> > status = nfs4_preprocess_confirmed_seqid_op(cstate,
> > lock->lk_new_open_seqid,
> > --
> > 2.29.2
> >
>
> --
> Chuck Lever
>
>
>
On Wed, Jan 20, 2021 at 09:02:25PM +0000, Chuck Lever wrote:
>
>
> > On Jan 19, 2021, at 5:35 PM, J. Bruce Fields <[email protected]> wrote:
> >
> > From: "J. Bruce Fields" <[email protected]>
> >
> > I think set_client is a better name, and the lookup itself I want to
> > use somewhere else.
>
> Should this be two patches?
> - Rename lookup_clientid() to set_client()
> - Refactor the lookup_clientid() helper
Sure.
> nfs4state.c stops compiling with this patch. See below.
Oops, thanks.
If these look OK otherwise, I can resend.
--b.
>
>
> > Signed-off-by: J. Bruce Fields <[email protected]>
> > ---
> > fs/nfsd/nfs4state.c | 50 ++++++++++++++++++++++-----------------------
> > 1 file changed, 24 insertions(+), 26 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index ba955bbf21df..d7128e16c86e 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -4633,40 +4633,40 @@ static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate, struct nfs4
> > return nfserr_bad_seqid;
> > }
> >
> > -static __be32 lookup_clientid(clientid_t *clid,
> > +static struct nfs4_client *lookup_clientid(clientid_t *clid, bool sessions,
> > + struct nfsd_net *nn)
> > +{
> > + struct nfs4_client *found;
> > +
> > + spin_lock(&nn->client_lock);
> > + found = find_confirmed_client(clid, sessions, nn);
> > + if (found)
> > + atomic_inc(&found->cl_rpc_users);
> > + spin_unlock(&nn->client_lock);
> > + return found;
> > +}
> > +
> > +static __be32 set_client(clientid_t *clid,
> > struct nfsd4_compound_state *cstate,
> > struct nfsd_net *nn,
> > bool sessions)
> > {
> > - struct nfs4_client *found;
> > -
> > if (cstate->clp) {
> > - found = cstate->clp;
> > - if (!same_clid(&found->cl_clientid, clid))
> > + if (!same_clid(&cstate->clp->cl_clientid, clid))
> > return nfserr_stale_clientid;
> > return nfs_ok;
> > }
> > -
> > if (STALE_CLIENTID(clid, nn))
> > return nfserr_stale_clientid;
> > -
> > /*
> > * For v4.1+ we get the client in the SEQUENCE op. If we don't have one
> > * cached already then we know this is for is for v4.0 and "sessions"
> > * will be false.
> > */
> > WARN_ON_ONCE(cstate->session);
> > - spin_lock(&nn->client_lock);
> > - found = find_confirmed_client(clid, sessions, nn);
> > - if (!found) {
> > - spin_unlock(&nn->client_lock);
> > + cstate->clp = lookup_clientid(clid, sessions, nn);
> > + if (!cstate->clp)
> > return nfserr_expired;
> > - }
> > - atomic_inc(&found->cl_rpc_users);
> > - spin_unlock(&nn->client_lock);
> > -
> > - /* Cache the nfs4_client in cstate! */
> > - cstate->clp = found;
> > return nfs_ok;
> > }
> >
> > @@ -4688,7 +4688,7 @@ nfsd4_process_open1(struct nfsd4_compound_state *cstate,
> > if (open->op_file == NULL)
> > return nfserr_jukebox;
> >
> > - status = lookup_clientid(clientid, cstate, nn, false);
> > + status = set_client(clientid, cstate, nn, false);
> > if (status)
> > return status;
> > clp = cstate->clp;
> > @@ -5298,7 +5298,7 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
> >
> > trace_nfsd_clid_renew(clid);
> > - status = lookup_clientid(clid, cstate, nn, false);
> > + status = set_client(clid, cstate, nn, false);
> > if (status)
> > return status;
> > clp = cstate->clp;
> > @@ -5681,8 +5681,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
> > if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
> > CLOSE_STATEID(stateid))
> > return nfserr_bad_stateid;
> > - status = lookup_clientid(&stateid->si_opaque.so_clid, cstate, nn,
> > - false);
> > + status = set_client(&stateid->si_opaque.so_clid, cstate, nn, false);
> > if (status == nfserr_stale_clientid) {
> > if (cstate->session)
> > return nfserr_bad_stateid;
> > @@ -5821,7 +5820,7 @@ static __be32 find_cpntf_state(struct nfsd_net *nn, stateid_t *st,
> >
> > cps->cpntf_time = ktime_get_boottime_seconds();
> > memset(&cstate, 0, sizeof(cstate));
> > - status = lookup_clientid(&cps->cp_p_clid, &cstate, nn, true);
> > + status = set_clientid(&cps->cp_p_clid, &cstate, nn, true);
>
> This doesn't compile. I think you meant set_client() here.
>
>
> > if (status)
> > goto out;
> > status = nfsd4_lookup_stateid(&cstate, &cps->cp_p_stateid,
> > @@ -6900,8 +6899,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > return nfserr_inval;
> >
> > if (!nfsd4_has_session(cstate)) {
> > - status = lookup_clientid(&lockt->lt_clientid, cstate, nn,
> > - false);
> > + status = set_client(&lockt->lt_clientid, cstate, nn, false);
> > if (status)
> > goto out;
> > }
> > @@ -7085,7 +7083,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
> > dprintk("nfsd4_release_lockowner clientid: (%08x/%08x):\n",
> > clid->cl_boot, clid->cl_id);
> >
> > - status = lookup_clientid(clid, cstate, nn, false);
> > + status = set_client(clid, cstate, nn, false);
> > if (status)
> > return status;
> >
> > @@ -7232,7 +7230,7 @@ nfs4_check_open_reclaim(clientid_t *clid,
> > __be32 status;
> >
> > /* find clientid in conf_id_hashtbl */
> > - status = lookup_clientid(clid, cstate, nn, false);
> > + status = set_clientid(clid, cstate, nn, false);
>
> Ditto.
>
>
> > if (status)
> > return nfserr_reclaim_bad;
> >
> > --
> > 2.29.2
> >
>
> --
> Chuck Lever
>
>
>
> On Jan 20, 2021, at 6:01 PM, J. Bruce Fields <[email protected]> wrote:
>
> On Wed, Jan 20, 2021 at 09:02:25PM +0000, Chuck Lever wrote:
>>
>>
>>>> On Jan 19, 2021, at 5:35 PM, J. Bruce Fields <[email protected]> wrote:
>>>
>>> From: "J. Bruce Fields" <[email protected]>
>>>
>>> I think set_client is a better name, and the lookup itself I want to
>>> use somewhere else.
>>
>> Should this be two patches?
>> - Rename lookup_clientid() to set_client()
>> - Refactor the lookup_clientid() helper
>
> Sure.
>
>> nfs4state.c stops compiling with this patch. See below.
>
> Oops, thanks.
>
> If these look OK otherwise, I can resend.
Yes, please resend.
>
> --b.
>
>>
>>
>>> Signed-off-by: J. Bruce Fields <[email protected]>
>>> ---
>>> fs/nfsd/nfs4state.c | 50 ++++++++++++++++++++++-----------------------
>>> 1 file changed, 24 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>> index ba955bbf21df..d7128e16c86e 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -4633,40 +4633,40 @@ static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate, struct nfs4
>>> return nfserr_bad_seqid;
>>> }
>>>
>>> -static __be32 lookup_clientid(clientid_t *clid,
>>> +static struct nfs4_client *lookup_clientid(clientid_t *clid, bool sessions,
>>> + struct nfsd_net *nn)
>>> +{
>>> + struct nfs4_client *found;
>>> +
>>> + spin_lock(&nn->client_lock);
>>> + found = find_confirmed_client(clid, sessions, nn);
>>> + if (found)
>>> + atomic_inc(&found->cl_rpc_users);
>>> + spin_unlock(&nn->client_lock);
>>> + return found;
>>> +}
>>> +
>>> +static __be32 set_client(clientid_t *clid,
>>> struct nfsd4_compound_state *cstate,
>>> struct nfsd_net *nn,
>>> bool sessions)
>>> {
>>> - struct nfs4_client *found;
>>> -
>>> if (cstate->clp) {
>>> - found = cstate->clp;
>>> - if (!same_clid(&found->cl_clientid, clid))
>>> + if (!same_clid(&cstate->clp->cl_clientid, clid))
>>> return nfserr_stale_clientid;
>>> return nfs_ok;
>>> }
>>> -
>>> if (STALE_CLIENTID(clid, nn))
>>> return nfserr_stale_clientid;
>>> -
>>> /*
>>> * For v4.1+ we get the client in the SEQUENCE op. If we don't have one
>>> * cached already then we know this is for is for v4.0 and "sessions"
>>> * will be false.
>>> */
>>> WARN_ON_ONCE(cstate->session);
>>> - spin_lock(&nn->client_lock);
>>> - found = find_confirmed_client(clid, sessions, nn);
>>> - if (!found) {
>>> - spin_unlock(&nn->client_lock);
>>> + cstate->clp = lookup_clientid(clid, sessions, nn);
>>> + if (!cstate->clp)
>>> return nfserr_expired;
>>> - }
>>> - atomic_inc(&found->cl_rpc_users);
>>> - spin_unlock(&nn->client_lock);
>>> -
>>> - /* Cache the nfs4_client in cstate! */
>>> - cstate->clp = found;
>>> return nfs_ok;
>>> }
>>>
>>> @@ -4688,7 +4688,7 @@ nfsd4_process_open1(struct nfsd4_compound_state *cstate,
>>> if (open->op_file == NULL)
>>> return nfserr_jukebox;
>>>
>>> - status = lookup_clientid(clientid, cstate, nn, false);
>>> + status = set_client(clientid, cstate, nn, false);
>>> if (status)
>>> return status;
>>> clp = cstate->clp;
>>> @@ -5298,7 +5298,7 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>> struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
>>>
>>> trace_nfsd_clid_renew(clid);
>>> - status = lookup_clientid(clid, cstate, nn, false);
>>> + status = set_client(clid, cstate, nn, false);
>>> if (status)
>>> return status;
>>> clp = cstate->clp;
>>> @@ -5681,8 +5681,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
>>> if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
>>> CLOSE_STATEID(stateid))
>>> return nfserr_bad_stateid;
>>> - status = lookup_clientid(&stateid->si_opaque.so_clid, cstate, nn,
>>> - false);
>>> + status = set_client(&stateid->si_opaque.so_clid, cstate, nn, false);
>>> if (status == nfserr_stale_clientid) {
>>> if (cstate->session)
>>> return nfserr_bad_stateid;
>>> @@ -5821,7 +5820,7 @@ static __be32 find_cpntf_state(struct nfsd_net *nn, stateid_t *st,
>>>
>>> cps->cpntf_time = ktime_get_boottime_seconds();
>>> memset(&cstate, 0, sizeof(cstate));
>>> - status = lookup_clientid(&cps->cp_p_clid, &cstate, nn, true);
>>> + status = set_clientid(&cps->cp_p_clid, &cstate, nn, true);
>>
>> This doesn't compile. I think you meant set_client() here.
>>
>>
>>> if (status)
>>> goto out;
>>> status = nfsd4_lookup_stateid(&cstate, &cps->cp_p_stateid,
>>> @@ -6900,8 +6899,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>> return nfserr_inval;
>>>
>>> if (!nfsd4_has_session(cstate)) {
>>> - status = lookup_clientid(&lockt->lt_clientid, cstate, nn,
>>> - false);
>>> + status = set_client(&lockt->lt_clientid, cstate, nn, false);
>>> if (status)
>>> goto out;
>>> }
>>> @@ -7085,7 +7083,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
>>> dprintk("nfsd4_release_lockowner clientid: (%08x/%08x):\n",
>>> clid->cl_boot, clid->cl_id);
>>>
>>> - status = lookup_clientid(clid, cstate, nn, false);
>>> + status = set_client(clid, cstate, nn, false);
>>> if (status)
>>> return status;
>>>
>>> @@ -7232,7 +7230,7 @@ nfs4_check_open_reclaim(clientid_t *clid,
>>> __be32 status;
>>>
>>> /* find clientid in conf_id_hashtbl */
>>> - status = lookup_clientid(clid, cstate, nn, false);
>>> + status = set_clientid(clid, cstate, nn, false);
>>
>> Ditto.
>>
>>
>>> if (status)
>>> return nfserr_reclaim_bad;
>>>
>>> --
>>> 2.29.2
>>>
>>
>> --
>> Chuck Lever
>>
>>
>>
>
From: "J. Bruce Fields" <[email protected]>
This STALE_CLIENTID check is redundant with the one in
lookup_clientid().
There's a difference in behavior is in case of memory allocation
failure, which I think isn't a big deal.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4state.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 1d2cd6a88f61..f9f89229dba6 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4680,8 +4680,6 @@ nfsd4_process_open1(struct nfsd4_compound_state *cstate,
struct nfs4_openowner *oo = NULL;
__be32 status;
- if (STALE_CLIENTID(&open->op_clientid, nn))
- return nfserr_stale_clientid;
/*
* In case we need it later, after we've already created the
* file and don't want to risk a further failure:
--
2.29.2
From: "J. Bruce Fields" <[email protected]>
This'll be useful elsewhere.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4state.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 4bdd90074e24..c74bf3b5b0de 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4633,40 +4633,40 @@ static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate, struct nfs4
return nfserr_bad_seqid;
}
+static struct nfs4_client *lookup_clientid(clientid_t *clid, bool sessions,
+ struct nfsd_net *nn)
+{
+ struct nfs4_client *found;
+
+ spin_lock(&nn->client_lock);
+ found = find_confirmed_client(clid, sessions, nn);
+ if (found)
+ atomic_inc(&found->cl_rpc_users);
+ spin_unlock(&nn->client_lock);
+ return found;
+}
+
static __be32 set_client(clientid_t *clid,
struct nfsd4_compound_state *cstate,
struct nfsd_net *nn,
bool sessions)
{
- struct nfs4_client *found;
-
if (cstate->clp) {
- found = cstate->clp;
- if (!same_clid(&found->cl_clientid, clid))
+ if (!same_clid(&cstate->clp->cl_clientid, clid))
return nfserr_stale_clientid;
return nfs_ok;
}
-
if (STALE_CLIENTID(clid, nn))
return nfserr_stale_clientid;
-
/*
* For v4.1+ we get the client in the SEQUENCE op. If we don't have one
* cached already then we know this is for is for v4.0 and "sessions"
* will be false.
*/
WARN_ON_ONCE(cstate->session);
- spin_lock(&nn->client_lock);
- found = find_confirmed_client(clid, sessions, nn);
- if (!found) {
- spin_unlock(&nn->client_lock);
+ cstate->clp = lookup_clientid(clid, sessions, nn);
+ if (!cstate->clp)
return nfserr_expired;
- }
- atomic_inc(&found->cl_rpc_users);
- spin_unlock(&nn->client_lock);
-
- /* Cache the nfs4_client in cstate! */
- cstate->clp = found;
return nfs_ok;
}
--
2.29.2
From: "J. Bruce Fields" <[email protected]>
I think this unusual use of struct compound_state could cause confusion.
It's not that much more complicated just to open-code this stateid
lookup.
The only change in behavior should be a different error return in the
case the copy is using a source stateid that is a revoked delegation,
but I doubt that matters.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4state.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index c74bf3b5b0de..db10fef1c1d2 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5812,21 +5812,27 @@ static __be32 find_cpntf_state(struct nfsd_net *nn, stateid_t *st,
{
__be32 status;
struct nfs4_cpntf_state *cps = NULL;
- struct nfsd4_compound_state cstate;
+ struct nfs4_client *found;
status = manage_cpntf_state(nn, st, NULL, &cps);
if (status)
return status;
cps->cpntf_time = ktime_get_boottime_seconds();
- memset(&cstate, 0, sizeof(cstate));
- status = set_client(&cps->cp_p_clid, &cstate, nn, true);
- if (status)
+
+ status = nfserr_expired;
+ found = lookup_clientid(&cps->cp_p_clid, true, nn);
+ if (!found)
goto out;
- status = nfsd4_lookup_stateid(&cstate, &cps->cp_p_stateid,
- NFS4_DELEG_STID|NFS4_OPEN_STID|NFS4_LOCK_STID,
- stid, nn);
- put_client_renew(cstate.clp);
+
+ *stid = find_stateid_by_type(found, &cps->cp_p_stateid,
+ NFS4_DELEG_STID|NFS4_OPEN_STID|NFS4_LOCK_STID);
+ if (stid)
+ status = nfs_ok;
+ else
+ status = nfserr_bad_stateid;
+
+ put_client_renew(found);
out:
nfs4_put_cpntf_state(nn, cps);
return status;
--
2.29.2
From: "J. Bruce Fields" <[email protected]>
I'm not sure why we're writing this out the hard way in so many places.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4proc.c | 5 ++---
fs/nfsd/nfs4state.c | 16 ++++++++--------
2 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 567af1f10d2c..f63a12a5278a 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -373,8 +373,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
* Before RECLAIM_COMPLETE done, server should deny new lock
*/
if (nfsd4_has_session(cstate) &&
- !test_bit(NFSD4_CLIENT_RECLAIM_COMPLETE,
- &cstate->session->se_client->cl_flags) &&
+ !test_bit(NFSD4_CLIENT_RECLAIM_COMPLETE, &cstate->clp->cl_flags) &&
open->op_claim_type != NFS4_OPEN_CLAIM_PREVIOUS)
return nfserr_grace;
@@ -1882,7 +1881,7 @@ nfsd4_getdeviceinfo(struct svc_rqst *rqstp,
nfserr = nfs_ok;
if (gdp->gd_maxcount != 0) {
nfserr = ops->proc_getdeviceinfo(exp->ex_path.mnt->mnt_sb,
- rqstp, cstate->session->se_client, gdp);
+ rqstp, cstate->clp, gdp);
}
gdp->gd_notify_types &= ops->notify_types;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 860805ffde1a..7b865ed7c9d7 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3891,6 +3891,7 @@ nfsd4_reclaim_complete(struct svc_rqst *rqstp,
struct nfsd4_compound_state *cstate, union nfsd4_op_u *u)
{
struct nfsd4_reclaim_complete *rc = &u->reclaim_complete;
+ struct nfs4_client *clp = cstate->clp;
__be32 status = 0;
if (rc->rca_one_fs) {
@@ -3904,12 +3905,11 @@ nfsd4_reclaim_complete(struct svc_rqst *rqstp,
}
status = nfserr_complete_already;
- if (test_and_set_bit(NFSD4_CLIENT_RECLAIM_COMPLETE,
- &cstate->session->se_client->cl_flags))
+ if (test_and_set_bit(NFSD4_CLIENT_RECLAIM_COMPLETE, &clp->cl_flags))
goto out;
status = nfserr_stale_clientid;
- if (is_client_expired(cstate->session->se_client))
+ if (is_client_expired(clp))
/*
* The following error isn't really legal.
* But we only get here if the client just explicitly
@@ -3920,8 +3920,8 @@ nfsd4_reclaim_complete(struct svc_rqst *rqstp,
goto out;
status = nfs_ok;
- nfsd4_client_record_create(cstate->session->se_client);
- inc_reclaim_complete(cstate->session->se_client);
+ nfsd4_client_record_create(clp);
+ inc_reclaim_complete(clp);
out:
return status;
}
@@ -5917,7 +5917,7 @@ nfsd4_test_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
{
struct nfsd4_test_stateid *test_stateid = &u->test_stateid;
struct nfsd4_test_stateid_id *stateid;
- struct nfs4_client *cl = cstate->session->se_client;
+ struct nfs4_client *cl = cstate->clp;
list_for_each_entry(stateid, &test_stateid->ts_stateid_list, ts_id_list)
stateid->ts_id_status =
@@ -5963,7 +5963,7 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
stateid_t *stateid = &free_stateid->fr_stateid;
struct nfs4_stid *s;
struct nfs4_delegation *dp;
- struct nfs4_client *cl = cstate->session->se_client;
+ struct nfs4_client *cl = cstate->clp;
__be32 ret = nfserr_bad_stateid;
spin_lock(&cl->cl_lock);
@@ -6692,7 +6692,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
if (nfsd4_has_session(cstate))
/* See rfc 5661 18.10.3: given clientid is ignored: */
memcpy(&lock->lk_new_clientid,
- &cstate->session->se_client->cl_clientid,
+ &cstate->clp->cl_clientid,
sizeof(clientid_t));
/* validate and update open stateid and open seqid */
--
2.29.2
From: "J. Bruce Fields" <[email protected]>
You can take the single-exit thing too far, I think.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4state.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 7ea63d7cec4d..ba955bbf21df 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5300,15 +5300,12 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
trace_nfsd_clid_renew(clid);
status = lookup_clientid(clid, cstate, nn, false);
if (status)
- goto out;
+ return status;
clp = cstate->clp;
- status = nfserr_cb_path_down;
if (!list_empty(&clp->cl_delegations)
&& clp->cl_cb_state != NFSD4_CB_UP)
- goto out;
- status = nfs_ok;
-out:
- return status;
+ return nfserr_cb_path_down;
+ return nfs_ok;
}
void
--
2.29.2
From: "J. Bruce Fields" <[email protected]>
The set_client() was already taken care of by process_open1().
The comments here are mostly redundant with the code.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4proc.c | 3 +--
fs/nfsd/nfs4state.c | 18 +++---------------
fs/nfsd/state.h | 3 +--
3 files changed, 5 insertions(+), 19 deletions(-)
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 4727b7f03c5b..567af1f10d2c 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -423,8 +423,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
goto out;
break;
case NFS4_OPEN_CLAIM_PREVIOUS:
- status = nfs4_check_open_reclaim(&open->op_clientid,
- cstate, nn);
+ status = nfs4_check_open_reclaim(cstate->clp);
if (status)
goto out;
open->op_openowner->oo_flags |= NFS4_OO_CONFIRMED;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 7c95f8808324..860805ffde1a 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -7221,25 +7221,13 @@ nfsd4_find_reclaim_client(struct xdr_netobj name, struct nfsd_net *nn)
return NULL;
}
-/*
-* Called from OPEN. Look for clientid in reclaim list.
-*/
__be32
-nfs4_check_open_reclaim(clientid_t *clid,
- struct nfsd4_compound_state *cstate,
- struct nfsd_net *nn)
+nfs4_check_open_reclaim(struct nfs4_client *clp)
{
- __be32 status;
-
- /* find clientid in conf_id_hashtbl */
- status = set_client(clid, cstate, nn, false);
- if (status)
- return nfserr_reclaim_bad;
-
- if (test_bit(NFSD4_CLIENT_RECLAIM_COMPLETE, &cstate->clp->cl_flags))
+ if (test_bit(NFSD4_CLIENT_RECLAIM_COMPLETE, &clp->cl_flags))
return nfserr_no_grace;
- if (nfsd4_client_record_check(cstate->clp))
+ if (nfsd4_client_record_check(clp))
return nfserr_reclaim_bad;
return nfs_ok;
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 9eae11a9d21c..73deea353169 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -649,8 +649,7 @@ void nfs4_remove_reclaim_record(struct nfs4_client_reclaim *, struct nfsd_net *)
extern void nfs4_release_reclaim(struct nfsd_net *);
extern struct nfs4_client_reclaim *nfsd4_find_reclaim_client(struct xdr_netobj name,
struct nfsd_net *nn);
-extern __be32 nfs4_check_open_reclaim(clientid_t *clid,
- struct nfsd4_compound_state *cstate, struct nfsd_net *nn);
+extern __be32 nfs4_check_open_reclaim(struct nfs4_client *);
extern void nfsd4_probe_callback(struct nfs4_client *clp);
extern void nfsd4_probe_callback_sync(struct nfs4_client *clp);
extern void nfsd4_change_callback(struct nfs4_client *clp, struct nfs4_cb_conn *);
--
2.29.2
From: "J. Bruce Fields" <[email protected]>
I think this is a better name, and I'm going to reuse elsewhere the code
that does the lookup itself.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4state.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index ba955bbf21df..4bdd90074e24 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4633,7 +4633,7 @@ static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate, struct nfs4
return nfserr_bad_seqid;
}
-static __be32 lookup_clientid(clientid_t *clid,
+static __be32 set_client(clientid_t *clid,
struct nfsd4_compound_state *cstate,
struct nfsd_net *nn,
bool sessions)
@@ -4688,7 +4688,7 @@ nfsd4_process_open1(struct nfsd4_compound_state *cstate,
if (open->op_file == NULL)
return nfserr_jukebox;
- status = lookup_clientid(clientid, cstate, nn, false);
+ status = set_client(clientid, cstate, nn, false);
if (status)
return status;
clp = cstate->clp;
@@ -5298,7 +5298,7 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
trace_nfsd_clid_renew(clid);
- status = lookup_clientid(clid, cstate, nn, false);
+ status = set_client(clid, cstate, nn, false);
if (status)
return status;
clp = cstate->clp;
@@ -5681,8 +5681,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
CLOSE_STATEID(stateid))
return nfserr_bad_stateid;
- status = lookup_clientid(&stateid->si_opaque.so_clid, cstate, nn,
- false);
+ status = set_client(&stateid->si_opaque.so_clid, cstate, nn, false);
if (status == nfserr_stale_clientid) {
if (cstate->session)
return nfserr_bad_stateid;
@@ -5821,7 +5820,7 @@ static __be32 find_cpntf_state(struct nfsd_net *nn, stateid_t *st,
cps->cpntf_time = ktime_get_boottime_seconds();
memset(&cstate, 0, sizeof(cstate));
- status = lookup_clientid(&cps->cp_p_clid, &cstate, nn, true);
+ status = set_client(&cps->cp_p_clid, &cstate, nn, true);
if (status)
goto out;
status = nfsd4_lookup_stateid(&cstate, &cps->cp_p_stateid,
@@ -6900,8 +6899,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
return nfserr_inval;
if (!nfsd4_has_session(cstate)) {
- status = lookup_clientid(&lockt->lt_clientid, cstate, nn,
- false);
+ status = set_client(&lockt->lt_clientid, cstate, nn, false);
if (status)
goto out;
}
@@ -7085,7 +7083,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
dprintk("nfsd4_release_lockowner clientid: (%08x/%08x):\n",
clid->cl_boot, clid->cl_id);
- status = lookup_clientid(clid, cstate, nn, false);
+ status = set_client(clid, cstate, nn, false);
if (status)
return status;
@@ -7232,7 +7230,7 @@ nfs4_check_open_reclaim(clientid_t *clid,
__be32 status;
/* find clientid in conf_id_hashtbl */
- status = lookup_clientid(clid, cstate, nn, false);
+ status = set_client(clid, cstate, nn, false);
if (status)
return nfserr_reclaim_bad;
--
2.29.2
From: "J. Bruce Fields" <[email protected]>
Similarly, this STALE_CLIENTID check is already handled by:
nfs4_preprocess_confirmed_seqid_op()->
nfs4_preprocess_seqid_op()->
nfsd4_lookup_stateid()->
set_client()->
STALE_CLIENTID()
(This may cause it to return a different error in some cases where
there are multiple things wrong; pynfs test SEQ10 regressed on this
commit because of that, but I think that's the test's fault, and I've
fixed it separately.)
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4state.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index f9f89229dba6..7ea63d7cec4d 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -6697,10 +6697,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
&cstate->session->se_client->cl_clientid,
sizeof(clientid_t));
- status = nfserr_stale_clientid;
- if (STALE_CLIENTID(&lock->lk_new_clientid, nn))
- goto out;
-
/* validate and update open stateid and open seqid */
status = nfs4_preprocess_confirmed_seqid_op(cstate,
lock->lk_new_open_seqid,
--
2.29.2
From: "J. Bruce Fields" <[email protected]>
Every caller is setting this argument to false, so we don't need it.
Also clarify comments a little.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4state.c | 22 +++++++++-------------
1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index db10fef1c1d2..7c95f8808324 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4648,8 +4648,7 @@ static struct nfs4_client *lookup_clientid(clientid_t *clid, bool sessions,
static __be32 set_client(clientid_t *clid,
struct nfsd4_compound_state *cstate,
- struct nfsd_net *nn,
- bool sessions)
+ struct nfsd_net *nn)
{
if (cstate->clp) {
if (!same_clid(&cstate->clp->cl_clientid, clid))
@@ -4658,13 +4657,10 @@ static __be32 set_client(clientid_t *clid,
}
if (STALE_CLIENTID(clid, nn))
return nfserr_stale_clientid;
- /*
- * For v4.1+ we get the client in the SEQUENCE op. If we don't have one
- * cached already then we know this is for is for v4.0 and "sessions"
- * will be false.
- */
+ /* For v4.1+ we should have gotten the client in the SEQUENCE op: */
WARN_ON_ONCE(cstate->session);
- cstate->clp = lookup_clientid(clid, sessions, nn);
+ /* So we're looking for a 4.0 client (sessions = false): */
+ cstate->clp = lookup_clientid(clid, false, nn);
if (!cstate->clp)
return nfserr_expired;
return nfs_ok;
@@ -4688,7 +4684,7 @@ nfsd4_process_open1(struct nfsd4_compound_state *cstate,
if (open->op_file == NULL)
return nfserr_jukebox;
- status = set_client(clientid, cstate, nn, false);
+ status = set_client(clientid, cstate, nn);
if (status)
return status;
clp = cstate->clp;
@@ -5298,7 +5294,7 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
trace_nfsd_clid_renew(clid);
- status = set_client(clid, cstate, nn, false);
+ status = set_client(clid, cstate, nn);
if (status)
return status;
clp = cstate->clp;
@@ -5681,7 +5677,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
CLOSE_STATEID(stateid))
return nfserr_bad_stateid;
- status = set_client(&stateid->si_opaque.so_clid, cstate, nn, false);
+ status = set_client(&stateid->si_opaque.so_clid, cstate, nn);
if (status == nfserr_stale_clientid) {
if (cstate->session)
return nfserr_bad_stateid;
@@ -6905,7 +6901,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
return nfserr_inval;
if (!nfsd4_has_session(cstate)) {
- status = set_client(&lockt->lt_clientid, cstate, nn, false);
+ status = set_client(&lockt->lt_clientid, cstate, nn);
if (status)
goto out;
}
@@ -7089,7 +7085,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
dprintk("nfsd4_release_lockowner clientid: (%08x/%08x):\n",
clid->cl_boot, clid->cl_id);
- status = set_client(clid, cstate, nn, false);
+ status = set_client(clid, cstate, nn);
if (status)
return status;
--
2.29.2
> On Jan 21, 2021, at 1:49 PM, J. Bruce Fields <[email protected]> wrote:
>
> From: "J. Bruce Fields" <[email protected]>
>
> Every caller is setting this argument to false, so we don't need it.
>
> Also clarify comments a little.
Some nits:
- The subject says "set_clientid" but the function name is "set_client"
- Is the WARN_ON_ONCE() still needed? (noticed yesterday, but I forgot
to mention it)
- This one doesn't compile:
[cel@klimt linux]$ make C=1 W=1 fs/nfsd/nfs4state.o
make[1]: Entering directory '/home/cel/src/linux/obj/klimt.1015granger.net'
GEN Makefile
UPD include/config/kernel.release
UPD include/generated/utsrelease.h
CALL /home/cel/src/linux/linux/scripts/checksyscalls.sh
CALL /home/cel/src/linux/linux/scripts/atomic/check-atomics.sh
DESCEND objtool
DESCEND bpf/resolve_btfids
CC [M] fs/nfsd/nfs4state.o
/home/cel/src/linux/linux/fs/nfsd/nfs4state.c: In function ‘nfs4_check_open_reclaim’:
/home/cel/src/linux/linux/fs/nfsd/nfs4state.c:7235:11: error: too many arguments to function ‘set_client’
7235 | status = set_client(clid, cstate, nn, false);
| ^~~~~~~~~~
/home/cel/src/linux/linux/fs/nfsd/nfs4state.c:4649:15: note: declared here
4649 | static __be32 set_client(clientid_t *clid,
| ^~~~~~~~~~
make[3]: *** [/home/cel/src/linux/linux/scripts/Makefile.build:279: fs/nfsd/nfs4state.o] Error 1
make[2]: *** [/home/cel/src/linux/linux/scripts/Makefile.build:496: fs/nfsd] Error 2
make[1]: *** [/home/cel/src/linux/linux/Makefile:1805: fs] Error 2
make[1]: Leaving directory '/home/cel/src/linux/obj/klimt.1015granger.net'
make: *** [Makefile:185: __sub-make] Error 2
[cel@klimt linux]$
I think I will need a v3 of this series because 8/9 will have to
be fixed up too. Please be sure to add the series version number
when you repost.
Thanks!
> Signed-off-by: J. Bruce Fields <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 22 +++++++++-------------
> 1 file changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index db10fef1c1d2..7c95f8808324 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4648,8 +4648,7 @@ static struct nfs4_client *lookup_clientid(clientid_t *clid, bool sessions,
>
> static __be32 set_client(clientid_t *clid,
> struct nfsd4_compound_state *cstate,
> - struct nfsd_net *nn,
> - bool sessions)
> + struct nfsd_net *nn)
> {
> if (cstate->clp) {
> if (!same_clid(&cstate->clp->cl_clientid, clid))
> @@ -4658,13 +4657,10 @@ static __be32 set_client(clientid_t *clid,
> }
> if (STALE_CLIENTID(clid, nn))
> return nfserr_stale_clientid;
> - /*
> - * For v4.1+ we get the client in the SEQUENCE op. If we don't have one
> - * cached already then we know this is for is for v4.0 and "sessions"
> - * will be false.
> - */
> + /* For v4.1+ we should have gotten the client in the SEQUENCE op: */
> WARN_ON_ONCE(cstate->session);
> - cstate->clp = lookup_clientid(clid, sessions, nn);
> + /* So we're looking for a 4.0 client (sessions = false): */
> + cstate->clp = lookup_clientid(clid, false, nn);
> if (!cstate->clp)
> return nfserr_expired;
> return nfs_ok;
> @@ -4688,7 +4684,7 @@ nfsd4_process_open1(struct nfsd4_compound_state *cstate,
> if (open->op_file == NULL)
> return nfserr_jukebox;
>
> - status = set_client(clientid, cstate, nn, false);
> + status = set_client(clientid, cstate, nn);
> if (status)
> return status;
> clp = cstate->clp;
> @@ -5298,7 +5294,7 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
>
> trace_nfsd_clid_renew(clid);
> - status = set_client(clid, cstate, nn, false);
> + status = set_client(clid, cstate, nn);
> if (status)
> return status;
> clp = cstate->clp;
> @@ -5681,7 +5677,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
> if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
> CLOSE_STATEID(stateid))
> return nfserr_bad_stateid;
> - status = set_client(&stateid->si_opaque.so_clid, cstate, nn, false);
> + status = set_client(&stateid->si_opaque.so_clid, cstate, nn);
> if (status == nfserr_stale_clientid) {
> if (cstate->session)
> return nfserr_bad_stateid;
> @@ -6905,7 +6901,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> return nfserr_inval;
>
> if (!nfsd4_has_session(cstate)) {
> - status = set_client(&lockt->lt_clientid, cstate, nn, false);
> + status = set_client(&lockt->lt_clientid, cstate, nn);
> if (status)
> goto out;
> }
> @@ -7089,7 +7085,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
> dprintk("nfsd4_release_lockowner clientid: (%08x/%08x):\n",
> clid->cl_boot, clid->cl_id);
>
> - status = set_client(clid, cstate, nn, false);
> + status = set_client(clid, cstate, nn);
> if (status)
> return status;
>
> --
> 2.29.2
>
--
Chuck Lever
On Thu, Jan 21, 2021 at 08:20:53PM +0000, Chuck Lever wrote:
>
>
> > On Jan 21, 2021, at 1:49 PM, J. Bruce Fields <[email protected]> wrote:
> >
> > From: "J. Bruce Fields" <[email protected]>
> >
> > Every caller is setting this argument to false, so we don't need it.
> >
> > Also clarify comments a little.
>
> Some nits:
>
> - The subject says "set_clientid" but the function name is "set_client"
Thanks, fixed.
> - Is the WARN_ON_ONCE() still needed? (noticed yesterday, but I forgot
> to mention it)
No, I don't think that's an especially interesting case to check for.
I'll drop it.
>
> - This one doesn't compile:
Oops, thanks, yes, I missed a caller that gets deleted in the next
patch.
> I think I will need a v3 of this series because 8/9 will have to
> be fixed up too. Please be sure to add the series version number
> when you repost.
OK!--b.
>
> Thanks!
>
>
> > Signed-off-by: J. Bruce Fields <[email protected]>
> > ---
> > fs/nfsd/nfs4state.c | 22 +++++++++-------------
> > 1 file changed, 9 insertions(+), 13 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index db10fef1c1d2..7c95f8808324 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -4648,8 +4648,7 @@ static struct nfs4_client *lookup_clientid(clientid_t *clid, bool sessions,
> >
> > static __be32 set_client(clientid_t *clid,
> > struct nfsd4_compound_state *cstate,
> > - struct nfsd_net *nn,
> > - bool sessions)
> > + struct nfsd_net *nn)
> > {
> > if (cstate->clp) {
> > if (!same_clid(&cstate->clp->cl_clientid, clid))
> > @@ -4658,13 +4657,10 @@ static __be32 set_client(clientid_t *clid,
> > }
> > if (STALE_CLIENTID(clid, nn))
> > return nfserr_stale_clientid;
> > - /*
> > - * For v4.1+ we get the client in the SEQUENCE op. If we don't have one
> > - * cached already then we know this is for is for v4.0 and "sessions"
> > - * will be false.
> > - */
> > + /* For v4.1+ we should have gotten the client in the SEQUENCE op: */
> > WARN_ON_ONCE(cstate->session);
> > - cstate->clp = lookup_clientid(clid, sessions, nn);
> > + /* So we're looking for a 4.0 client (sessions = false): */
> > + cstate->clp = lookup_clientid(clid, false, nn);
> > if (!cstate->clp)
> > return nfserr_expired;
> > return nfs_ok;
> > @@ -4688,7 +4684,7 @@ nfsd4_process_open1(struct nfsd4_compound_state *cstate,
> > if (open->op_file == NULL)
> > return nfserr_jukebox;
> >
> > - status = set_client(clientid, cstate, nn, false);
> > + status = set_client(clientid, cstate, nn);
> > if (status)
> > return status;
> > clp = cstate->clp;
> > @@ -5298,7 +5294,7 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
> >
> > trace_nfsd_clid_renew(clid);
> > - status = set_client(clid, cstate, nn, false);
> > + status = set_client(clid, cstate, nn);
> > if (status)
> > return status;
> > clp = cstate->clp;
> > @@ -5681,7 +5677,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
> > if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
> > CLOSE_STATEID(stateid))
> > return nfserr_bad_stateid;
> > - status = set_client(&stateid->si_opaque.so_clid, cstate, nn, false);
> > + status = set_client(&stateid->si_opaque.so_clid, cstate, nn);
> > if (status == nfserr_stale_clientid) {
> > if (cstate->session)
> > return nfserr_bad_stateid;
> > @@ -6905,7 +6901,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > return nfserr_inval;
> >
> > if (!nfsd4_has_session(cstate)) {
> > - status = set_client(&lockt->lt_clientid, cstate, nn, false);
> > + status = set_client(&lockt->lt_clientid, cstate, nn);
> > if (status)
> > goto out;
> > }
> > @@ -7089,7 +7085,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
> > dprintk("nfsd4_release_lockowner clientid: (%08x/%08x):\n",
> > clid->cl_boot, clid->cl_id);
> >
> > - status = set_client(clid, cstate, nn, false);
> > + status = set_client(clid, cstate, nn);
> > if (status)
> > return status;
> >
> > --
> > 2.29.2
> >
>
> --
> Chuck Lever
>
>
>
From: "J. Bruce Fields" <[email protected]>
This STALE_CLIENTID check is redundant with the one in
lookup_clientid().
There's a difference in behavior is in case of memory allocation
failure, which I think isn't a big deal.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4state.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 1d2cd6a88f61..f9f89229dba6 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4680,8 +4680,6 @@ nfsd4_process_open1(struct nfsd4_compound_state *cstate,
struct nfs4_openowner *oo = NULL;
__be32 status;
- if (STALE_CLIENTID(&open->op_clientid, nn))
- return nfserr_stale_clientid;
/*
* In case we need it later, after we've already created the
* file and don't want to risk a further failure:
--
2.29.2
From: "J. Bruce Fields" <[email protected]>
I'm not sure why we're writing this out the hard way in so many places.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4proc.c | 5 ++---
fs/nfsd/nfs4state.c | 16 ++++++++--------
2 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 567af1f10d2c..f63a12a5278a 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -373,8 +373,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
* Before RECLAIM_COMPLETE done, server should deny new lock
*/
if (nfsd4_has_session(cstate) &&
- !test_bit(NFSD4_CLIENT_RECLAIM_COMPLETE,
- &cstate->session->se_client->cl_flags) &&
+ !test_bit(NFSD4_CLIENT_RECLAIM_COMPLETE, &cstate->clp->cl_flags) &&
open->op_claim_type != NFS4_OPEN_CLAIM_PREVIOUS)
return nfserr_grace;
@@ -1882,7 +1881,7 @@ nfsd4_getdeviceinfo(struct svc_rqst *rqstp,
nfserr = nfs_ok;
if (gdp->gd_maxcount != 0) {
nfserr = ops->proc_getdeviceinfo(exp->ex_path.mnt->mnt_sb,
- rqstp, cstate->session->se_client, gdp);
+ rqstp, cstate->clp, gdp);
}
gdp->gd_notify_types &= ops->notify_types;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 389456937bbe..f554e3480bb1 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3891,6 +3891,7 @@ nfsd4_reclaim_complete(struct svc_rqst *rqstp,
struct nfsd4_compound_state *cstate, union nfsd4_op_u *u)
{
struct nfsd4_reclaim_complete *rc = &u->reclaim_complete;
+ struct nfs4_client *clp = cstate->clp;
__be32 status = 0;
if (rc->rca_one_fs) {
@@ -3904,12 +3905,11 @@ nfsd4_reclaim_complete(struct svc_rqst *rqstp,
}
status = nfserr_complete_already;
- if (test_and_set_bit(NFSD4_CLIENT_RECLAIM_COMPLETE,
- &cstate->session->se_client->cl_flags))
+ if (test_and_set_bit(NFSD4_CLIENT_RECLAIM_COMPLETE, &clp->cl_flags))
goto out;
status = nfserr_stale_clientid;
- if (is_client_expired(cstate->session->se_client))
+ if (is_client_expired(clp))
/*
* The following error isn't really legal.
* But we only get here if the client just explicitly
@@ -3920,8 +3920,8 @@ nfsd4_reclaim_complete(struct svc_rqst *rqstp,
goto out;
status = nfs_ok;
- nfsd4_client_record_create(cstate->session->se_client);
- inc_reclaim_complete(cstate->session->se_client);
+ nfsd4_client_record_create(clp);
+ inc_reclaim_complete(clp);
out:
return status;
}
@@ -5918,7 +5918,7 @@ nfsd4_test_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
{
struct nfsd4_test_stateid *test_stateid = &u->test_stateid;
struct nfsd4_test_stateid_id *stateid;
- struct nfs4_client *cl = cstate->session->se_client;
+ struct nfs4_client *cl = cstate->clp;
list_for_each_entry(stateid, &test_stateid->ts_stateid_list, ts_id_list)
stateid->ts_id_status =
@@ -5964,7 +5964,7 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
stateid_t *stateid = &free_stateid->fr_stateid;
struct nfs4_stid *s;
struct nfs4_delegation *dp;
- struct nfs4_client *cl = cstate->session->se_client;
+ struct nfs4_client *cl = cstate->clp;
__be32 ret = nfserr_bad_stateid;
spin_lock(&cl->cl_lock);
@@ -6693,7 +6693,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
if (nfsd4_has_session(cstate))
/* See rfc 5661 18.10.3: given clientid is ignored: */
memcpy(&lock->lk_new_clientid,
- &cstate->session->se_client->cl_clientid,
+ &cstate->clp->cl_clientid,
sizeof(clientid_t));
/* validate and update open stateid and open seqid */
--
2.29.2
From: "J. Bruce Fields" <[email protected]>
I think this is a better name, and I'm going to reuse elsewhere the code
that does the lookup itself.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4state.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index ba955bbf21df..4bdd90074e24 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4633,7 +4633,7 @@ static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate, struct nfs4
return nfserr_bad_seqid;
}
-static __be32 lookup_clientid(clientid_t *clid,
+static __be32 set_client(clientid_t *clid,
struct nfsd4_compound_state *cstate,
struct nfsd_net *nn,
bool sessions)
@@ -4688,7 +4688,7 @@ nfsd4_process_open1(struct nfsd4_compound_state *cstate,
if (open->op_file == NULL)
return nfserr_jukebox;
- status = lookup_clientid(clientid, cstate, nn, false);
+ status = set_client(clientid, cstate, nn, false);
if (status)
return status;
clp = cstate->clp;
@@ -5298,7 +5298,7 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
trace_nfsd_clid_renew(clid);
- status = lookup_clientid(clid, cstate, nn, false);
+ status = set_client(clid, cstate, nn, false);
if (status)
return status;
clp = cstate->clp;
@@ -5681,8 +5681,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
CLOSE_STATEID(stateid))
return nfserr_bad_stateid;
- status = lookup_clientid(&stateid->si_opaque.so_clid, cstate, nn,
- false);
+ status = set_client(&stateid->si_opaque.so_clid, cstate, nn, false);
if (status == nfserr_stale_clientid) {
if (cstate->session)
return nfserr_bad_stateid;
@@ -5821,7 +5820,7 @@ static __be32 find_cpntf_state(struct nfsd_net *nn, stateid_t *st,
cps->cpntf_time = ktime_get_boottime_seconds();
memset(&cstate, 0, sizeof(cstate));
- status = lookup_clientid(&cps->cp_p_clid, &cstate, nn, true);
+ status = set_client(&cps->cp_p_clid, &cstate, nn, true);
if (status)
goto out;
status = nfsd4_lookup_stateid(&cstate, &cps->cp_p_stateid,
@@ -6900,8 +6899,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
return nfserr_inval;
if (!nfsd4_has_session(cstate)) {
- status = lookup_clientid(&lockt->lt_clientid, cstate, nn,
- false);
+ status = set_client(&lockt->lt_clientid, cstate, nn, false);
if (status)
goto out;
}
@@ -7085,7 +7083,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
dprintk("nfsd4_release_lockowner clientid: (%08x/%08x):\n",
clid->cl_boot, clid->cl_id);
- status = lookup_clientid(clid, cstate, nn, false);
+ status = set_client(clid, cstate, nn, false);
if (status)
return status;
@@ -7232,7 +7230,7 @@ nfs4_check_open_reclaim(clientid_t *clid,
__be32 status;
/* find clientid in conf_id_hashtbl */
- status = lookup_clientid(clid, cstate, nn, false);
+ status = set_client(clid, cstate, nn, false);
if (status)
return nfserr_reclaim_bad;
--
2.29.2
From: "J. Bruce Fields" <[email protected]>
This'll be useful elsewhere.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4state.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 4bdd90074e24..c74bf3b5b0de 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4633,40 +4633,40 @@ static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate, struct nfs4
return nfserr_bad_seqid;
}
+static struct nfs4_client *lookup_clientid(clientid_t *clid, bool sessions,
+ struct nfsd_net *nn)
+{
+ struct nfs4_client *found;
+
+ spin_lock(&nn->client_lock);
+ found = find_confirmed_client(clid, sessions, nn);
+ if (found)
+ atomic_inc(&found->cl_rpc_users);
+ spin_unlock(&nn->client_lock);
+ return found;
+}
+
static __be32 set_client(clientid_t *clid,
struct nfsd4_compound_state *cstate,
struct nfsd_net *nn,
bool sessions)
{
- struct nfs4_client *found;
-
if (cstate->clp) {
- found = cstate->clp;
- if (!same_clid(&found->cl_clientid, clid))
+ if (!same_clid(&cstate->clp->cl_clientid, clid))
return nfserr_stale_clientid;
return nfs_ok;
}
-
if (STALE_CLIENTID(clid, nn))
return nfserr_stale_clientid;
-
/*
* For v4.1+ we get the client in the SEQUENCE op. If we don't have one
* cached already then we know this is for is for v4.0 and "sessions"
* will be false.
*/
WARN_ON_ONCE(cstate->session);
- spin_lock(&nn->client_lock);
- found = find_confirmed_client(clid, sessions, nn);
- if (!found) {
- spin_unlock(&nn->client_lock);
+ cstate->clp = lookup_clientid(clid, sessions, nn);
+ if (!cstate->clp)
return nfserr_expired;
- }
- atomic_inc(&found->cl_rpc_users);
- spin_unlock(&nn->client_lock);
-
- /* Cache the nfs4_client in cstate! */
- cstate->clp = found;
return nfs_ok;
}
--
2.29.2
From: "J. Bruce Fields" <[email protected]>
Similarly, this STALE_CLIENTID check is already handled by:
nfs4_preprocess_confirmed_seqid_op()->
nfs4_preprocess_seqid_op()->
nfsd4_lookup_stateid()->
set_client()->
STALE_CLIENTID()
(This may cause it to return a different error in some cases where
there are multiple things wrong; pynfs test SEQ10 regressed on this
commit because of that, but I think that's the test's fault, and I've
fixed it separately.)
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4state.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index f9f89229dba6..7ea63d7cec4d 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -6697,10 +6697,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
&cstate->session->se_client->cl_clientid,
sizeof(clientid_t));
- status = nfserr_stale_clientid;
- if (STALE_CLIENTID(&lock->lk_new_clientid, nn))
- goto out;
-
/* validate and update open stateid and open seqid */
status = nfs4_preprocess_confirmed_seqid_op(cstate,
lock->lk_new_open_seqid,
--
2.29.2
From: "J. Bruce Fields" <[email protected]>
The set_client() was already taken care of by process_open1().
The comments here are mostly redundant with the code.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4proc.c | 3 +--
fs/nfsd/nfs4state.c | 18 +++---------------
fs/nfsd/state.h | 3 +--
3 files changed, 5 insertions(+), 19 deletions(-)
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 4727b7f03c5b..567af1f10d2c 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -423,8 +423,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
goto out;
break;
case NFS4_OPEN_CLAIM_PREVIOUS:
- status = nfs4_check_open_reclaim(&open->op_clientid,
- cstate, nn);
+ status = nfs4_check_open_reclaim(cstate->clp);
if (status)
goto out;
open->op_openowner->oo_flags |= NFS4_OO_CONFIRMED;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 6d4b77b536fb..389456937bbe 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -7222,25 +7222,13 @@ nfsd4_find_reclaim_client(struct xdr_netobj name, struct nfsd_net *nn)
return NULL;
}
-/*
-* Called from OPEN. Look for clientid in reclaim list.
-*/
__be32
-nfs4_check_open_reclaim(clientid_t *clid,
- struct nfsd4_compound_state *cstate,
- struct nfsd_net *nn)
+nfs4_check_open_reclaim(struct nfs4_client *clp)
{
- __be32 status;
-
- /* find clientid in conf_id_hashtbl */
- status = set_client(clid, cstate, nn);
- if (status)
- return nfserr_reclaim_bad;
-
- if (test_bit(NFSD4_CLIENT_RECLAIM_COMPLETE, &cstate->clp->cl_flags))
+ if (test_bit(NFSD4_CLIENT_RECLAIM_COMPLETE, &clp->cl_flags))
return nfserr_no_grace;
- if (nfsd4_client_record_check(cstate->clp))
+ if (nfsd4_client_record_check(clp))
return nfserr_reclaim_bad;
return nfs_ok;
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 9eae11a9d21c..73deea353169 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -649,8 +649,7 @@ void nfs4_remove_reclaim_record(struct nfs4_client_reclaim *, struct nfsd_net *)
extern void nfs4_release_reclaim(struct nfsd_net *);
extern struct nfs4_client_reclaim *nfsd4_find_reclaim_client(struct xdr_netobj name,
struct nfsd_net *nn);
-extern __be32 nfs4_check_open_reclaim(clientid_t *clid,
- struct nfsd4_compound_state *cstate, struct nfsd_net *nn);
+extern __be32 nfs4_check_open_reclaim(struct nfs4_client *);
extern void nfsd4_probe_callback(struct nfs4_client *clp);
extern void nfsd4_probe_callback_sync(struct nfs4_client *clp);
extern void nfsd4_change_callback(struct nfs4_client *clp, struct nfs4_cb_conn *);
--
2.29.2
From: "J. Bruce Fields" <[email protected]>
Every caller is setting this argument to false, so we don't need it.
Also cut this comment a bit and remove an unnecessary warning.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4state.c | 23 ++++++++++-------------
1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index db10fef1c1d2..6d4b77b536fb 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4648,8 +4648,7 @@ static struct nfs4_client *lookup_clientid(clientid_t *clid, bool sessions,
static __be32 set_client(clientid_t *clid,
struct nfsd4_compound_state *cstate,
- struct nfsd_net *nn,
- bool sessions)
+ struct nfsd_net *nn)
{
if (cstate->clp) {
if (!same_clid(&cstate->clp->cl_clientid, clid))
@@ -4659,12 +4658,10 @@ static __be32 set_client(clientid_t *clid,
if (STALE_CLIENTID(clid, nn))
return nfserr_stale_clientid;
/*
- * For v4.1+ we get the client in the SEQUENCE op. If we don't have one
- * cached already then we know this is for is for v4.0 and "sessions"
- * will be false.
+ * We're in the 4.0 case (otherwise the SEQUENCE op would have
+ * set cstate->clp), so session = false:
*/
- WARN_ON_ONCE(cstate->session);
- cstate->clp = lookup_clientid(clid, sessions, nn);
+ cstate->clp = lookup_clientid(clid, false, nn);
if (!cstate->clp)
return nfserr_expired;
return nfs_ok;
@@ -4688,7 +4685,7 @@ nfsd4_process_open1(struct nfsd4_compound_state *cstate,
if (open->op_file == NULL)
return nfserr_jukebox;
- status = set_client(clientid, cstate, nn, false);
+ status = set_client(clientid, cstate, nn);
if (status)
return status;
clp = cstate->clp;
@@ -5298,7 +5295,7 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
trace_nfsd_clid_renew(clid);
- status = set_client(clid, cstate, nn, false);
+ status = set_client(clid, cstate, nn);
if (status)
return status;
clp = cstate->clp;
@@ -5681,7 +5678,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
CLOSE_STATEID(stateid))
return nfserr_bad_stateid;
- status = set_client(&stateid->si_opaque.so_clid, cstate, nn, false);
+ status = set_client(&stateid->si_opaque.so_clid, cstate, nn);
if (status == nfserr_stale_clientid) {
if (cstate->session)
return nfserr_bad_stateid;
@@ -6905,7 +6902,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
return nfserr_inval;
if (!nfsd4_has_session(cstate)) {
- status = set_client(&lockt->lt_clientid, cstate, nn, false);
+ status = set_client(&lockt->lt_clientid, cstate, nn);
if (status)
goto out;
}
@@ -7089,7 +7086,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
dprintk("nfsd4_release_lockowner clientid: (%08x/%08x):\n",
clid->cl_boot, clid->cl_id);
- status = set_client(clid, cstate, nn, false);
+ status = set_client(clid, cstate, nn);
if (status)
return status;
@@ -7236,7 +7233,7 @@ nfs4_check_open_reclaim(clientid_t *clid,
__be32 status;
/* find clientid in conf_id_hashtbl */
- status = set_client(clid, cstate, nn, false);
+ status = set_client(clid, cstate, nn);
if (status)
return nfserr_reclaim_bad;
--
2.29.2
From: "J. Bruce Fields" <[email protected]>
I think this unusual use of struct compound_state could cause confusion.
It's not that much more complicated just to open-code this stateid
lookup.
The only change in behavior should be a different error return in the
case the copy is using a source stateid that is a revoked delegation,
but I doubt that matters.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs4state.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index c74bf3b5b0de..db10fef1c1d2 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5812,21 +5812,27 @@ static __be32 find_cpntf_state(struct nfsd_net *nn, stateid_t *st,
{
__be32 status;
struct nfs4_cpntf_state *cps = NULL;
- struct nfsd4_compound_state cstate;
+ struct nfs4_client *found;
status = manage_cpntf_state(nn, st, NULL, &cps);
if (status)
return status;
cps->cpntf_time = ktime_get_boottime_seconds();
- memset(&cstate, 0, sizeof(cstate));
- status = set_client(&cps->cp_p_clid, &cstate, nn, true);
- if (status)
+
+ status = nfserr_expired;
+ found = lookup_clientid(&cps->cp_p_clid, true, nn);
+ if (!found)
goto out;
- status = nfsd4_lookup_stateid(&cstate, &cps->cp_p_stateid,
- NFS4_DELEG_STID|NFS4_OPEN_STID|NFS4_LOCK_STID,
- stid, nn);
- put_client_renew(cstate.clp);
+
+ *stid = find_stateid_by_type(found, &cps->cp_p_stateid,
+ NFS4_DELEG_STID|NFS4_OPEN_STID|NFS4_LOCK_STID);
+ if (stid)
+ status = nfs_ok;
+ else
+ status = nfserr_bad_stateid;
+
+ put_client_renew(found);
out:
nfs4_put_cpntf_state(nn, cps);
return status;
--
2.29.2
Hi Bruce-
> On Jan 21, 2021, at 5:57 PM, J. Bruce Fields <[email protected]> wrote:
>
> From: "J. Bruce Fields" <[email protected]>
>
> I'm not sure why we're writing this out the hard way in so many places.
>
> Signed-off-by: J. Bruce Fields <[email protected]>
Thanks. v3 of this series has been committed to the for-next branch at
git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git
in preparation for the v5.12 merge window.
> ---
> fs/nfsd/nfs4proc.c | 5 ++---
> fs/nfsd/nfs4state.c | 16 ++++++++--------
> 2 files changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 567af1f10d2c..f63a12a5278a 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -373,8 +373,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> * Before RECLAIM_COMPLETE done, server should deny new lock
> */
> if (nfsd4_has_session(cstate) &&
> - !test_bit(NFSD4_CLIENT_RECLAIM_COMPLETE,
> - &cstate->session->se_client->cl_flags) &&
> + !test_bit(NFSD4_CLIENT_RECLAIM_COMPLETE, &cstate->clp->cl_flags) &&
> open->op_claim_type != NFS4_OPEN_CLAIM_PREVIOUS)
> return nfserr_grace;
>
> @@ -1882,7 +1881,7 @@ nfsd4_getdeviceinfo(struct svc_rqst *rqstp,
> nfserr = nfs_ok;
> if (gdp->gd_maxcount != 0) {
> nfserr = ops->proc_getdeviceinfo(exp->ex_path.mnt->mnt_sb,
> - rqstp, cstate->session->se_client, gdp);
> + rqstp, cstate->clp, gdp);
> }
>
> gdp->gd_notify_types &= ops->notify_types;
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 389456937bbe..f554e3480bb1 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3891,6 +3891,7 @@ nfsd4_reclaim_complete(struct svc_rqst *rqstp,
> struct nfsd4_compound_state *cstate, union nfsd4_op_u *u)
> {
> struct nfsd4_reclaim_complete *rc = &u->reclaim_complete;
> + struct nfs4_client *clp = cstate->clp;
> __be32 status = 0;
>
> if (rc->rca_one_fs) {
> @@ -3904,12 +3905,11 @@ nfsd4_reclaim_complete(struct svc_rqst *rqstp,
> }
>
> status = nfserr_complete_already;
> - if (test_and_set_bit(NFSD4_CLIENT_RECLAIM_COMPLETE,
> - &cstate->session->se_client->cl_flags))
> + if (test_and_set_bit(NFSD4_CLIENT_RECLAIM_COMPLETE, &clp->cl_flags))
> goto out;
>
> status = nfserr_stale_clientid;
> - if (is_client_expired(cstate->session->se_client))
> + if (is_client_expired(clp))
> /*
> * The following error isn't really legal.
> * But we only get here if the client just explicitly
> @@ -3920,8 +3920,8 @@ nfsd4_reclaim_complete(struct svc_rqst *rqstp,
> goto out;
>
> status = nfs_ok;
> - nfsd4_client_record_create(cstate->session->se_client);
> - inc_reclaim_complete(cstate->session->se_client);
> + nfsd4_client_record_create(clp);
> + inc_reclaim_complete(clp);
> out:
> return status;
> }
> @@ -5918,7 +5918,7 @@ nfsd4_test_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> {
> struct nfsd4_test_stateid *test_stateid = &u->test_stateid;
> struct nfsd4_test_stateid_id *stateid;
> - struct nfs4_client *cl = cstate->session->se_client;
> + struct nfs4_client *cl = cstate->clp;
>
> list_for_each_entry(stateid, &test_stateid->ts_stateid_list, ts_id_list)
> stateid->ts_id_status =
> @@ -5964,7 +5964,7 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> stateid_t *stateid = &free_stateid->fr_stateid;
> struct nfs4_stid *s;
> struct nfs4_delegation *dp;
> - struct nfs4_client *cl = cstate->session->se_client;
> + struct nfs4_client *cl = cstate->clp;
> __be32 ret = nfserr_bad_stateid;
>
> spin_lock(&cl->cl_lock);
> @@ -6693,7 +6693,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> if (nfsd4_has_session(cstate))
> /* See rfc 5661 18.10.3: given clientid is ignored: */
> memcpy(&lock->lk_new_clientid,
> - &cstate->session->se_client->cl_clientid,
> + &cstate->clp->cl_clientid,
> sizeof(clientid_t));
>
> /* validate and update open stateid and open seqid */
> --
> 2.29.2
>
--
Chuck Lever