[PATCH 0/8] miscellaneous nfsd4 state cleanup

[PATCH 0/8] miscellaneous nfsd4 state cleanup

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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

[PATCH 1/8] nfsd4: simplify process_lookup1

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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 <bfields@redhat.com>
---
 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

[PATCH 2/8] nfsd: simplify process_lock

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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 <bfields@redhat.com>
---
 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

[PATCH 3/8] nfsd: simplify nfsd_renew

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

You can take the single-exit thing too far, I think.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 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

[PATCH 4/8] nfsd: refactor lookup_clientid

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

I think set_client is a better name, and the lookup itself I want to
use somewhere else.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 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

[PATCH 5/8] nfsd: find_cpntf_state cleanup

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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 <bfields@redhat.com>
---
 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

[PATCH 6/8] nfsd: remove unused set_clientid argument

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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 <bfields@redhat.com>
---
 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

[PATCH 7/8] nfsd: simplify nfsd4_check_open_reclaim

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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 <bfields@redhat.com>
---
 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

[PATCH 8/8] nfsd: cstate->session->se_client -> cstate->clp

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

I'm not sure why we're writing this out the hard way in so many places.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 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

Re: [PATCH 2/8] nfsd: simplify process_lock

[Chuck Lever]

> On Jan 19, 2021, at 5:35 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> 
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> 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 <bfields@redhat.com>
> ---
> 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

Re: [PATCH 4/8] nfsd: refactor lookup_clientid

[Chuck Lever]

> On Jan 19, 2021, at 5:35 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> 
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> 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 <bfields@redhat.com>
> ---
> 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

Re: [PATCH 2/8] nfsd: simplify process_lock

[J. Bruce Fields]

On Wed, Jan 20, 2021 at 09:01:53PM +0000, Chuck Lever wrote:
> 
> 
> > On Jan 19, 2021, at 5:35 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> > 
> > From: "J. Bruce Fields" <bfields@redhat.com>
> > 
> > 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 <bfields@redhat.com>
> > ---
> > 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
> 
> 
> 

Re: [PATCH 4/8] nfsd: refactor lookup_clientid

[J. Bruce Fields]

On Wed, Jan 20, 2021 at 09:02:25PM +0000, Chuck Lever wrote:
> 
> 
> > On Jan 19, 2021, at 5:35 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> > 
> > From: "J. Bruce Fields" <bfields@redhat.com>
> > 
> > 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 <bfields@redhat.com>
> > ---
> > 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
> 
> 
> 

Re: [PATCH 4/8] nfsd: refactor lookup_clientid

[Chuck Lever]

> On Jan 20, 2021, at 6:01 PM, J. Bruce Fields <bfields@redhat.com> 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 <bfields@redhat.com> wrote:
>>> 
>>> From: "J. Bruce Fields" <bfields@redhat.com>
>>> 
>>> 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 <bfields@redhat.com>
>>> ---
>>> 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
>> 
>> 
>> 
> 

[PATCH 1/9] nfsd4: simplify process_lookup1

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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 <bfields@redhat.com>
---
 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

[PATCH 2/9] nfsd: simplify process_lock

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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 <bfields@redhat.com>
---
 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

[PATCH 3/9] nfsd: simplify nfsd_renew

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

You can take the single-exit thing too far, I think.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 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

[PATCH 4/9] nfsd: rename lookup_clientid->set_client

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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 <bfields@redhat.com>
---
 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

[PATCH 5/9] nfsd: refactor lookup_clientid

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

This'll be useful elsewhere.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 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

[PATCH 6/9] nfsd: find_cpntf_state cleanup

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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 <bfields@redhat.com>
---
 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

[PATCH 7/9] nfsd: remove unused set_clientid argument

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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 <bfields@redhat.com>
---
 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

[PATCH 8/9] nfsd: simplify nfsd4_check_open_reclaim

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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 <bfields@redhat.com>
---
 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

[PATCH 9/9] nfsd: cstate->session->se_client -> cstate->clp

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

I'm not sure why we're writing this out the hard way in so many places.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 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

Re: [PATCH 7/9] nfsd: remove unused set_clientid argument

[Chuck Lever]

> On Jan 21, 2021, at 1:49 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> 
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> 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 <bfields@redhat.com>
> ---
> 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

Re: [PATCH 7/9] nfsd: remove unused set_clientid argument

[J. Bruce Fields]

On Thu, Jan 21, 2021 at 08:20:53PM +0000, Chuck Lever wrote:
> 
> 
> > On Jan 21, 2021, at 1:49 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> > 
> > From: "J. Bruce Fields" <bfields@redhat.com>
> > 
> > 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 <bfields@redhat.com>
> > ---
> > 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
> 
> 
> 

[PATCH v3 1/9] nfsd4: simplify process_lookup1

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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 <bfields@redhat.com>
---
 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

[PATCH v3 2/9] nfsd: simplify process_lock

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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 <bfields@redhat.com>
---
 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

[PATCH v3 3/9] nfsd: simplify nfsd_renew

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

You can take the single-exit thing too far, I think.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 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

[PATCH v3 4/9] nfsd: rename lookup_clientid->set_client

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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 <bfields@redhat.com>
---
 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

[PATCH v3 5/9] nfsd: refactor set_client

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

This'll be useful elsewhere.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 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

[PATCH v3 6/9] nfsd: find_cpntf_state cleanup

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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 <bfields@redhat.com>
---
 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

[PATCH v3 7/9] nfsd: remove unused set_client argument

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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 <bfields@redhat.com>
---
 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

[PATCH v3 8/9] nfsd: simplify nfsd4_check_open_reclaim

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

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 <bfields@redhat.com>
---
 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

[PATCH v3 9/9] nfsd: cstate->session->se_client -> cstate->clp

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

I'm not sure why we're writing this out the hard way in so many places.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 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

Re: [PATCH v3 9/9] nfsd: cstate->session->se_client -> cstate->clp

[Chuck Lever]

Hi Bruce-

> On Jan 21, 2021, at 5:57 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> 
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> I'm not sure why we're writing this out the hard way in so many places.
> 
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>

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