knfsd miscellaneous cleanup

PATCH In review J. Bruce Fields 2 participants 7 messages Jun 14, 2018 – Jun 15, 2018

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

J. Bruce Fields

author

2018-06-14 19:32 UTC #0 lore raw

cover knfsd miscellaneous cleanup

From: "J. Bruce Fields" <[email protected]>

Some cleanup and one minor bugfix.

--b.

J. Bruce Fields (5):
nfsd4: less confusing nfsd4_compound_in_session
nfsd4: cleanup sessionid in nfsd4_destroy_session
nfsd: update obselete comment referencing the BKL
nfsd: clarify check_op_ordering
nfsd: fix corrupted reply to badly ordered compound

 fs/nfsd/nfs4proc.c  | 14 ++++++++++----
 fs/nfsd/nfs4state.c | 20 ++++++++++----------
 2 files changed, 20 insertions(+), 14 deletions(-)

Signature · 1 line

2.17.1

J. Bruce Fields

author

2018-06-14 19:32 UTC #1 lore raw

PATCH 1/5 nfsd4: less confusing nfsd4_compound_in_session

From: "J. Bruce Fields" <[email protected]>

Make the function prototype match the name a little better.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfs4state.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

fs/nfsd/nfs4state.c +4 -4

index 683a5f705d81..ac891b5b1d6a 100644
@@ -2955,11 +2955,11 @@ __be32 nfsd4_bind_conn_to_session(struct svc_rqst *rqstp,
 	return status;
 }
 
-static bool nfsd4_compound_in_session(struct nfsd4_session *session, struct nfs4_sessionid *sid)
+static bool nfsd4_compound_in_session(struct nfsd4_compound_state *cstate, struct nfs4_sessionid *sid)
 {
-	if (!session)
+	if (!cstate->session)
 		return false;
-	return !memcmp(sid, &session->se_sessionid, sizeof(*sid));
+	return !memcmp(sid, &cstate->session->se_sessionid, sizeof(*sid));
 }
 
 __be32

@@ -2974,7 +2974,7 @@ nfsd4_destroy_session(struct svc_rqst *r, struct nfsd4_compound_state *cstate,
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
 
 	status = nfserr_not_only_op;
-	if (nfsd4_compound_in_session(cstate->session, &sessionid->sessionid)) {
+	if (nfsd4_compound_in_session(cstate, &sessionid->sessionid)) {
 		if (!nfsd4_last_compound_op(r))
 			goto out;
 		ref_held_by_me++;

Signature · 1 line

2.17.1

J. Bruce Fields

author

2018-06-14 19:32 UTC #2 lore raw

PATCH 2/5 nfsd4: cleanup sessionid in nfsd4_destroy_session

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

The name of this variable doesn't fit the type. And we only ever use one field of it.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfs4state.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

fs/nfsd/nfs4state.c +4 -4

index ac891b5b1d6a..b762054377e0 100644
@@ -2966,7 +2966,7 @@ __be32
 nfsd4_destroy_session(struct svc_rqst *r, struct nfsd4_compound_state *cstate,
 		union nfsd4_op_u *u)
 {
-	struct nfsd4_destroy_session *sessionid = &u->destroy_session;
+	struct nfs4_sessionid *sessionid = &u->destroy_session.sessionid;
 	struct nfsd4_session *ses;
 	__be32 status;
 	int ref_held_by_me = 0;

@@ -2974,14 +2974,14 @@ nfsd4_destroy_session(struct svc_rqst *r, struct nfsd4_compound_state *cstate,
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
 
 	status = nfserr_not_only_op;
-	if (nfsd4_compound_in_session(cstate, &sessionid->sessionid)) {
+	if (nfsd4_compound_in_session(cstate, sessionid)) {
 		if (!nfsd4_last_compound_op(r))
 			goto out;
 		ref_held_by_me++;
 	}
-	dump_sessionid(__func__, &sessionid->sessionid);
+	dump_sessionid(__func__, sessionid);
 	spin_lock(&nn->client_lock);
-	ses = find_in_sessionid_hashtbl(&sessionid->sessionid, net, &status);
+	ses = find_in_sessionid_hashtbl(sessionid, net, &status);
 	if (!ses)
 		goto out_client_lock;
 	status = nfserr_wrong_cred;

Signature · 1 line

2.17.1

J. Bruce Fields

author

2018-06-14 19:32 UTC #3 lore raw

PATCH 3/5 nfsd: update obselete comment referencing the BKL

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

It's inode->i_lock that's now taken in setlease and break_lease, instead of the big kernel lock.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfs4state.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

fs/nfsd/nfs4state.c +3 -3

index b762054377e0..294b9a433594 100644
@@ -3944,9 +3944,9 @@ static void nfsd_break_one_deleg(struct nfs4_delegation *dp)
 	/*
 	 * We're assuming the state code never drops its reference
 	 * without first removing the lease.  Since we're in this lease
-	 * callback (and since the lease code is serialized by the kernel
-	 * lock) we know the server hasn't removed the lease yet, we know
-	 * it's safe to take a reference.
+	 * callback (and since the lease code is serialized by the
+	 * i_lock) we know the server hasn't removed the lease yet, and
+	 * we know it's safe to take a reference.
 	 */
 	refcount_inc(&dp->dl_stid.sc_count);
 	nfsd4_run_cb(&dp->dl_recall);

Signature · 1 line

2.17.1

J. Bruce Fields

author

2018-06-14 19:32 UTC #4 lore raw

PATCH 4/5 nfsd: clarify check_op_ordering

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

Document a couple things that confused me on a recent reading.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfs4proc.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

fs/nfsd/nfs4proc.c +9 -4

index 1929f85b8269..140b05c8a4be 100644
@@ -1603,7 +1603,7 @@ static const char *nfsd4_op_name(unsigned opnum);
  */
 static __be32 nfs41_check_op_ordering(struct nfsd4_compoundargs *args)
 {
-	struct nfsd4_op *op = &args->ops[0];
+	struct nfsd4_op *first_op = &args->ops[0];
 
 	/* These ordering requirements don't apply to NFSv4.0: */
 	if (args->minorversion == 0)

@@ -1611,12 +1611,17 @@ static __be32 nfs41_check_op_ordering(struct nfsd4_compoundargs *args)
 	/* This is weird, but OK, not our problem: */
 	if (args->opcnt == 0)
 		return nfs_ok;
-	if (op->status == nfserr_op_illegal)
+	if (first_op->status == nfserr_op_illegal)
 		return nfs_ok;
-	if (!(nfsd4_ops[op->opnum].op_flags & ALLOWED_AS_FIRST_OP))
+	if (!(nfsd4_ops[first_op->opnum].op_flags & ALLOWED_AS_FIRST_OP))
 		return nfserr_op_not_in_session;
-	if (op->opnum == OP_SEQUENCE)
+	if (first_op->opnum == OP_SEQUENCE)
 		return nfs_ok;
+	/*
+	 * So first_op is something allowed outside a session, like
+	 * EXCHANGE_ID; but then it has to be the only op in the
+	 * compound:
+	 */
 	if (args->opcnt != 1)
 		return nfserr_not_only_op;
 	return nfs_ok;

Signature · 1 line

2.17.1

J. Bruce Fields

author

2018-06-14 19:32 UTC #5 lore raw

PATCH 5/5 nfsd: fix corrupted reply to badly ordered compound

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

We're encoding a single op in the reply but leaving the number of ops zero, so the reply makes no sense.

Somewhat academic as this isn't a case any real client will hit, though in theory perhaps that could change in a future protocol extension.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfs4proc.c | 1 +
 1 file changed, 1 insertion(+)

fs/nfsd/nfs4proc.c +1 -0

index 140b05c8a4be..3652f9b1fb68 100644
@@ -1735,6 +1735,7 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
 	if (status) {
 		op = &args->ops[0];
 		op->status = status;
+		resp->opcnt = 1;
 		goto encode_op;
 	}

Signature · 1 line

2.17.1

Jeff Layton

maintainer Reviewed-by on the v1 series

2018-06-15 17:00 UTC #6 lore raw

In reply to J. Bruce Fields

Quoted 13 lines · J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>
Some cleanup and one minor bugfix.
--b.
J. Bruce Fields (5):
nfsd4: less confusing nfsd4_compound_in_session
nfsd4: cleanup sessionid in nfsd4_destroy_session
nfsd: update obselete comment referencing the BKL
nfsd: clarify check_op_ordering
nfsd: fix corrupted reply to badly ordered compound

Quoted 4 lines

 fs/nfsd/nfs4proc.c  | 14 ++++++++++----
 fs/nfsd/nfs4state.c | 20 ++++++++++----------
 2 files changed, 20 insertions(+), 14 deletions(-)

These also seem reasonable.

Reviewed-by Jeff Layton <[email protected]>