Skip to contentSkip to conversation

4.1 session allocation cleanup

PATCH New J. Bruce Fields 1 participant 9 messages Oct 1, 2012

Subsystems (2)
NFS client NFS server
lore
Share & tools
lore

J. Bruce Fields

author
#0 lore raw

The following cleanup is preparation for some locking changes that aren't actually ready to merge yet--but I don't see any reason not to go ahead and merge the cleanup for 3.7.

--b.

author
#1 lore raw

PATCH 1/8 nfsd4: reject bad forechannel attrs earlier

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

This could simplify the logic a little later.

Signed-off-by J. Bruce Fields <[email protected]>
---
 fs/nfsd/nfs4state.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
fs/nfsd/nfs4state.c +2 -4
index 5507842..64938ca 100644
@@ -1771,6 +1771,8 @@ nfsd4_create_session(struct svc_rqst *rqstp,
 
 	if (cr_ses->flags & ~SESSION4_FLAG_MASK_A)
 		return nfserr_inval;
+	if (check_forechannel_attrs(cr_ses->fore_channel))
+		return nfserr_toosmall;
 
 	nfs4_lock_state();
 	unconf = find_unconfirmed_client(&cr_ses->clientid, true);

@@ -1811,10 +1813,6 @@ nfsd4_create_session(struct svc_rqst *rqstp,
 	cr_ses->flags &= ~SESSION4_PERSIST;
 	cr_ses->flags &= ~SESSION4_RDMA;
 
-	status = nfserr_toosmall;
-	if (check_forechannel_attrs(cr_ses->fore_channel))
-		goto out;
-
 	status = nfserr_jukebox;
 	new = alloc_init_session(rqstp, conf, cr_ses);
 	if (!new)
Signature · 1 line
1.7.9.5
author
#2 lore raw

PATCH 2/8 nfsd4: separate connection allocation and initialization

In reply to J. Bruce Fields

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

It'll be useful to have connection allocation and initialization as separate functions.

Also, note we'd been ignoring the alloc_conn error return in bind_conn_to_session.

Signed-off-by J. Bruce Fields <[email protected]>
---
 fs/nfsd/nfs4state.c |   25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)
fs/nfsd/nfs4state.c +15 -10
index 64938ca..101ccd8 100644
@@ -852,14 +852,10 @@ static int nfsd4_register_conn(struct nfsd4_conn *conn)
 	return register_xpt_user(conn->cn_xprt, &conn->cn_xpt_user);
 }
 
-static __be32 nfsd4_new_conn(struct svc_rqst *rqstp, struct nfsd4_session *ses, u32 dir)
+static void nfsd4_init_conn(struct svc_rqst *rqstp, struct nfsd4_conn *conn, struct nfsd4_session *ses, u32 dir)
 {
-	struct nfsd4_conn *conn;
 	int ret;
 
-	conn = alloc_conn(rqstp, dir);
-	if (!conn)
-		return nfserr_jukebox;
 	nfsd4_hash_conn(conn, ses);
 	ret = nfsd4_register_conn(conn);
 	if (ret)

@@ -870,17 +866,21 @@ static __be32 nfsd4_new_conn(struct svc_rqst *rqstp, struct nfsd4_session *ses,
 		/* callback channel may be back up */
 		nfsd4_probe_callback(ses->se_client);
 	}
-	return nfs_ok;
 }
 
 static __be32 nfsd4_new_conn_from_crses(struct svc_rqst *rqstp, struct nfsd4_session *ses)
 {
+	struct nfsd4_conn *conn;
 	u32 dir = NFS4_CDFC4_FORE;
 
 	if (ses->se_flags & SESSION4_BACK_CHAN)
 		dir |= NFS4_CDFC4_BACK;
 
-	return nfsd4_new_conn(rqstp, ses, dir);
+	conn = alloc_conn(rqstp, dir);
+	if (!conn)
+		return nfserr_jukebox;
+	nfsd4_init_conn(rqstp, conn, ses, dir);
+	return nfs_ok;
 }
 
 /* must be called under client_lock */

@@ -1868,6 +1868,7 @@ __be32 nfsd4_bind_conn_to_session(struct svc_rqst *rqstp,
 		     struct nfsd4_bind_conn_to_session *bcts)
 {
 	__be32 status;
+	struct nfsd4_conn *conn;
 
 	if (!nfsd4_last_compound_op(rqstp))
 		return nfserr_not_only_op;

@@ -1884,9 +1885,13 @@ __be32 nfsd4_bind_conn_to_session(struct svc_rqst *rqstp,
 		return nfserr_badsession;
 
 	status = nfsd4_map_bcts_dir(&bcts->dir);
-	if (!status)
-		nfsd4_new_conn(rqstp, cstate->session, bcts->dir);
-	return status;
+	if (status)
+		return status;
+	conn = alloc_conn(rqstp, bcts->dir);
+	if (!conn)
+		return nfserr_jukebox;
+	nfsd4_init_conn(rqstp, conn, cstate->session, bcts->dir);
+	return nfs_ok;
 }
 
 static bool nfsd4_compound_in_session(struct nfsd4_session *session, struct nfs4_sessionid *sid)
Signature · 1 line
1.7.9.5
author
#3 lore raw

PATCH 3/8 nfsd4: new_conn_from_crses should only allocate

In reply to J. Bruce Fields

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

Do the initialization in the caller, and clarify that the only failure ever possible here was due to allocation.

Signed-off-by J. Bruce Fields <[email protected]>
---
 fs/nfsd/nfs4state.c |   26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)
fs/nfsd/nfs4state.c +10 -16
index 101ccd8..8b7f8f8 100644
@@ -852,7 +852,7 @@ static int nfsd4_register_conn(struct nfsd4_conn *conn)
 	return register_xpt_user(conn->cn_xprt, &conn->cn_xpt_user);
 }
 
-static void nfsd4_init_conn(struct svc_rqst *rqstp, struct nfsd4_conn *conn, struct nfsd4_session *ses, u32 dir)
+static void nfsd4_init_conn(struct svc_rqst *rqstp, struct nfsd4_conn *conn, struct nfsd4_session *ses)
 {
 	int ret;
 

@@ -862,25 +862,19 @@ static void nfsd4_init_conn(struct svc_rqst *rqstp, struct nfsd4_conn *conn, str
 		/* oops; xprt is already down: */
 		nfsd4_conn_lost(&conn->cn_xpt_user);
 	if (ses->se_client->cl_cb_state == NFSD4_CB_DOWN &&
-		dir & NFS4_CDFC4_BACK) {
+			conn->cn_flags & NFS4_CDFC4_BACK) {
 		/* callback channel may be back up */
 		nfsd4_probe_callback(ses->se_client);
 	}
 }
 
-static __be32 nfsd4_new_conn_from_crses(struct svc_rqst *rqstp, struct nfsd4_session *ses)
+static struct nfsd4_conn *alloc_conn_from_crses(struct svc_rqst *rqstp, struct nfsd4_create_session *cses)
 {
-	struct nfsd4_conn *conn;
 	u32 dir = NFS4_CDFC4_FORE;
 
-	if (ses->se_flags & SESSION4_BACK_CHAN)
+	if (cses->flags & SESSION4_BACK_CHAN)
 		dir |= NFS4_CDFC4_BACK;
-
-	conn = alloc_conn(rqstp, dir);
-	if (!conn)
-		return nfserr_jukebox;
-	nfsd4_init_conn(rqstp, conn, ses, dir);
-	return nfs_ok;
+	return alloc_conn(rqstp, dir);
 }
 
 /* must be called under client_lock */

@@ -929,9 +923,9 @@ void nfsd4_put_session(struct nfsd4_session *ses)
 static struct nfsd4_session *alloc_init_session(struct svc_rqst *rqstp, struct nfs4_client *clp, struct nfsd4_create_session *cses)
 {
 	struct nfsd4_session *new;
+	struct nfsd4_conn *conn;
 	struct nfsd4_channel_attrs *fchan = &cses->fore_channel;
 	int numslots, slotsize;
-	__be32 status;
 	int idx;
 
 	/*

@@ -970,14 +964,14 @@ static struct nfsd4_session *alloc_init_session(struct svc_rqst *rqstp, struct n
 	spin_unlock(&clp->cl_lock);
 	spin_unlock(&client_lock);
 
-	status = nfsd4_new_conn_from_crses(rqstp, new);
-	/* whoops: benny points out, status is ignored! (err, or bogus) */
-	if (status) {
+	conn = alloc_conn_from_crses(rqstp, cses);
+	if (!conn) {
 		spin_lock(&client_lock);
 		free_session(&new->se_ref);
 		spin_unlock(&client_lock);
 		return NULL;
 	}
+	nfsd4_init_conn(rqstp, conn, new);
 	if (cses->flags & SESSION4_BACK_CHAN) {
 		struct sockaddr *sa = svc_addr(rqstp);
 		/*

@@ -1890,7 +1884,7 @@ __be32 nfsd4_bind_conn_to_session(struct svc_rqst *rqstp,
 	conn = alloc_conn(rqstp, bcts->dir);
 	if (!conn)
 		return nfserr_jukebox;
-	nfsd4_init_conn(rqstp, conn, cstate->session, bcts->dir);
+	nfsd4_init_conn(rqstp, conn, cstate->session);
 	return nfs_ok;
 }
Signature · 1 line
1.7.9.5
author
#4 lore raw

PATCH 4/8 nfsd4: minor free_session cleanup

In reply to J. Bruce Fields

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

Signed-off-by J. Bruce Fields <[email protected]>
---
 fs/nfsd/nfs4state.c |   19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)
fs/nfsd/nfs4state.c +9 -10
index 8b7f8f8..7536a88 100644
@@ -897,20 +897,21 @@ static void nfsd4_del_conns(struct nfsd4_session *s)
 	spin_unlock(&clp->cl_lock);
 }
 
+static void __free_session(struct nfsd4_session *ses)
+{
+	nfsd4_put_drc_mem(slot_bytes(&ses->se_fchannel), ses->se_fchannel.maxreqs);
+	free_session_slots(ses);
+	kfree(ses);
+}
+
 static void free_session(struct kref *kref)
 {
 	struct nfsd4_session *ses;
-	int mem;
 
 	lockdep_assert_held(&client_lock);
 	ses = container_of(kref, struct nfsd4_session, se_ref);
 	nfsd4_del_conns(ses);
-	spin_lock(&nfsd_drc_lock);
-	mem = ses->se_fchannel.maxreqs * slot_bytes(&ses->se_fchannel);
-	nfsd_drc_mem_used -= mem;
-	spin_unlock(&nfsd_drc_lock);
-	free_session_slots(ses);
-	kfree(ses);
+	__free_session(ses);
 }
 
 void nfsd4_put_session(struct nfsd4_session *ses)

@@ -966,9 +967,7 @@ static struct nfsd4_session *alloc_init_session(struct svc_rqst *rqstp, struct n
 
 	conn = alloc_conn_from_crses(rqstp, cses);
 	if (!conn) {
-		spin_lock(&client_lock);
-		free_session(&new->se_ref);
-		spin_unlock(&client_lock);
+		__free_session(new);
 		return NULL;
 	}
 	nfsd4_init_conn(rqstp, conn, new);
Signature · 1 line
1.7.9.5
author
#5 lore raw

PATCH 5/8 nfsd4: clean up session allocation

In reply to J. Bruce Fields

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

Signed-off-by J. Bruce Fields <[email protected]>
---
 fs/nfsd/nfs4state.c |   32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)
fs/nfsd/nfs4state.c +20 -12
index 7536a88..79f1fdb 100644
@@ -758,7 +758,7 @@ static void nfsd4_put_drc_mem(int slotsize, int num)
 	spin_unlock(&nfsd_drc_lock);
 }
 
-static struct nfsd4_session *alloc_session(int slotsize, int numslots)
+static struct nfsd4_session *__alloc_session(int slotsize, int numslots)
 {
 	struct nfsd4_session *new;
 	int mem, i;

@@ -921,14 +921,10 @@ void nfsd4_put_session(struct nfsd4_session *ses)
 	spin_unlock(&client_lock);
 }
 
-static struct nfsd4_session *alloc_init_session(struct svc_rqst *rqstp, struct nfs4_client *clp, struct nfsd4_create_session *cses)
+static struct nfsd4_session *alloc_session(struct nfsd4_channel_attrs *fchan)
 {
 	struct nfsd4_session *new;
-	struct nfsd4_conn *conn;
-	struct nfsd4_channel_attrs *fchan = &cses->fore_channel;
 	int numslots, slotsize;
-	int idx;
-
 	/*
 	 * Note decreasing slot size below client's request may
 	 * make it difficult for client to function correctly, whereas

@@ -941,13 +937,30 @@ static struct nfsd4_session *alloc_init_session(struct svc_rqst *rqstp, struct n
 	if (numslots < 1)
 		return NULL;
 
-	new = alloc_session(slotsize, numslots);
+	new = __alloc_session(slotsize, numslots);
 	if (!new) {
 		nfsd4_put_drc_mem(slotsize, fchan->maxreqs);
 		return NULL;
 	}
 	init_forechannel_attrs(&new->se_fchannel, fchan, numslots, slotsize);
+	return new;
+}
 
+static struct nfsd4_session *alloc_init_session(struct svc_rqst *rqstp, struct nfs4_client *clp, struct nfsd4_create_session *cses)
+{
+	struct nfsd4_session *new;
+	struct nfsd4_conn *conn;
+	int idx;
+
+	new = alloc_session(&cses->fore_channel);
+	if (!new)
+		return NULL;
+
+	conn = alloc_conn_from_crses(rqstp, cses);
+	if (!conn) {
+		__free_session(new);
+		return NULL;
+	}
 	new->se_client = clp;
 	gen_sessionid(new);
 

@@ -965,11 +978,6 @@ static struct nfsd4_session *alloc_init_session(struct svc_rqst *rqstp, struct n
 	spin_unlock(&clp->cl_lock);
 	spin_unlock(&client_lock);
 
-	conn = alloc_conn_from_crses(rqstp, cses);
-	if (!conn) {
-		__free_session(new);
-		return NULL;
-	}
 	nfsd4_init_conn(rqstp, conn, new);
 	if (cses->flags & SESSION4_BACK_CHAN) {
 		struct sockaddr *sa = svc_addr(rqstp);
Signature · 1 line
1.7.9.5
author
#6 lore raw

PATCH 6/8 nfsd4: separate session allocation and initialization

In reply to J. Bruce Fields

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

This will allow some further simplification.

Signed-off-by J. Bruce Fields <[email protected]>
---
 fs/nfsd/nfs4state.c |   46 +++++++++++++++++++++++-----------------------
 1 file changed, 23 insertions(+), 23 deletions(-)
fs/nfsd/nfs4state.c +23 -23
index 79f1fdb..1cd80d5 100644
@@ -946,21 +946,10 @@ static struct nfsd4_session *alloc_session(struct nfsd4_channel_attrs *fchan)
 	return new;
 }
 
-static struct nfsd4_session *alloc_init_session(struct svc_rqst *rqstp, struct nfs4_client *clp, struct nfsd4_create_session *cses)
+static struct nfsd4_session *init_session(struct svc_rqst *rqstp, struct nfsd4_session *new, struct nfs4_client *clp, struct nfsd4_create_session *cses)
 {
-	struct nfsd4_session *new;
-	struct nfsd4_conn *conn;
 	int idx;
 
-	new = alloc_session(&cses->fore_channel);
-	if (!new)
-		return NULL;
-
-	conn = alloc_conn_from_crses(rqstp, cses);
-	if (!conn) {
-		__free_session(new);
-		return NULL;
-	}
 	new->se_client = clp;
 	gen_sessionid(new);
 

@@ -978,7 +967,6 @@ static struct nfsd4_session *alloc_init_session(struct svc_rqst *rqstp, struct n
 	spin_unlock(&clp->cl_lock);
 	spin_unlock(&client_lock);
 
-	nfsd4_init_conn(rqstp, conn, new);
 	if (cses->flags & SESSION4_BACK_CHAN) {
 		struct sockaddr *sa = svc_addr(rqstp);
 		/*

@@ -1766,6 +1754,7 @@ nfsd4_create_session(struct svc_rqst *rqstp,
 	struct sockaddr *sa = svc_addr(rqstp);
 	struct nfs4_client *conf, *unconf;
 	struct nfsd4_session *new;
+	struct nfsd4_conn *conn;
 	struct nfsd4_clid_slot *cs_slot = NULL;
 	bool confirm_me = false;
 	__be32 status = 0;

@@ -1774,6 +1763,13 @@ nfsd4_create_session(struct svc_rqst *rqstp,
 		return nfserr_inval;
 	if (check_forechannel_attrs(cr_ses->fore_channel))
 		return nfserr_toosmall;
+	new = alloc_session(&cr_ses->fore_channel);
+	if (!new)
+		return nfserr_jukebox;
+	status = nfserr_jukebox;
+	conn = alloc_conn_from_crses(rqstp, cr_ses);
+	if (!conn)
+		goto out_free_session;
 
 	nfs4_lock_state();
 	unconf = find_unconfirmed_client(&cr_ses->clientid, true);

@@ -1784,41 +1780,40 @@ nfsd4_create_session(struct svc_rqst *rqstp,
 		status = check_slot_seqid(cr_ses->seqid, cs_slot->sl_seqid, 0);
 		if (status == nfserr_replay_cache) {
 			status = nfsd4_replay_create_session(cr_ses, cs_slot);
-			goto out;
+			goto out_free_conn;
 		} else if (cr_ses->seqid != cs_slot->sl_seqid + 1) {
 			status = nfserr_seq_misordered;
-			goto out;
+			goto out_free_conn;
 		}
 	} else if (unconf) {
 		if (!same_creds(&unconf->cl_cred, &rqstp->rq_cred) ||
 		    !rpc_cmp_addr(sa, (struct sockaddr *) &unconf->cl_addr)) {
 			status = nfserr_clid_inuse;
-			goto out;
+			goto out_free_conn;
 		}
 		cs_slot = &unconf->cl_cs_slot;
 		status = check_slot_seqid(cr_ses->seqid, cs_slot->sl_seqid, 0);
 		if (status) {
 			/* an unconfirmed replay returns misordered */
 			status = nfserr_seq_misordered;
-			goto out;
+			goto out_free_conn;
 		}
 		confirm_me = true;
 		conf = unconf;
 	} else {
 		status = nfserr_stale_clientid;
-		goto out;
+		goto out_free_conn;
 	}
+	status = nfs_ok;
 	/*
 	 * We do not support RDMA or persistent sessions
 	 */
 	cr_ses->flags &= ~SESSION4_PERSIST;
 	cr_ses->flags &= ~SESSION4_RDMA;
 
-	status = nfserr_jukebox;
-	new = alloc_init_session(rqstp, conf, cr_ses);
-	if (!new)
-		goto out;
-	status = nfs_ok;
+	init_session(rqstp, new, conf, cr_ses);
+	nfsd4_init_conn(rqstp, conn, new);
+
 	memcpy(cr_ses->sessionid.data, new->se_sessionid.data,
 	       NFS4_MAX_SESSIONID_LEN);
 	memcpy(&cr_ses->fore_channel, &new->se_fchannel,

@@ -1840,6 +1835,11 @@ out:
 	nfs4_unlock_state();
 	dprintk("%s returns %d\n", __func__, ntohl(status));
 	return status;
+out_free_conn:
+	free_conn(conn);
+out_free_session:
+	__free_session(new);
+	goto out;
 }
 
 static bool nfsd4_last_compound_op(struct svc_rqst *rqstp)
Signature · 1 line
1.7.9.5
author
#7 lore raw

PATCH 7/8 nfsd4: expire old client earlier

In reply to J. Bruce Fields

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

Before we had to delay expiring a client till we'd found out whether the session and connection allocations would succeed. That's no longer necessary.

Signed-off-by J. Bruce Fields <[email protected]>
---
 fs/nfsd/nfs4state.c |   17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)
fs/nfsd/nfs4state.c +7 -10
index 1cd80d5..592cd7f 100644
@@ -1756,7 +1756,6 @@ nfsd4_create_session(struct svc_rqst *rqstp,
 	struct nfsd4_session *new;
 	struct nfsd4_conn *conn;
 	struct nfsd4_clid_slot *cs_slot = NULL;
-	bool confirm_me = false;
 	__be32 status = 0;
 
 	if (cr_ses->flags & ~SESSION4_FLAG_MASK_A)

@@ -1786,6 +1785,8 @@ nfsd4_create_session(struct svc_rqst *rqstp,
 			goto out_free_conn;
 		}
 	} else if (unconf) {
+		unsigned int hash;
+		struct nfs4_client *old;
 		if (!same_creds(&unconf->cl_cred, &rqstp->rq_cred) ||
 		    !rpc_cmp_addr(sa, (struct sockaddr *) &unconf->cl_addr)) {
 			status = nfserr_clid_inuse;

@@ -1798,7 +1799,11 @@ nfsd4_create_session(struct svc_rqst *rqstp,
 			status = nfserr_seq_misordered;
 			goto out_free_conn;
 		}
-		confirm_me = true;
+		hash = clientstr_hashval(unconf->cl_recdir);
+		old = find_confirmed_client_by_str(unconf->cl_recdir, hash);
+		if (old)
+			expire_client(old);
+		move_to_confirmed(unconf);
 		conf = unconf;
 	} else {
 		status = nfserr_stale_clientid;

@@ -1823,14 +1828,6 @@ nfsd4_create_session(struct svc_rqst *rqstp,
 
 	/* cache solo and embedded create sessions under the state lock */
 	nfsd4_cache_create_session(cr_ses, cs_slot, status);
-	if (confirm_me) {
-		unsigned int hash = clientstr_hashval(unconf->cl_recdir);
-		struct nfs4_client *old =
-			find_confirmed_client_by_str(conf->cl_recdir, hash);
-		if (old)
-			expire_client(old);
-		move_to_confirmed(conf);
-	}
 out:
 	nfs4_unlock_state();
 	dprintk("%s returns %d\n", __func__, ntohl(status));
Signature · 1 line
1.7.9.5
author
#8 lore raw

PATCH 8/8 nfsd4: remove redundant callback probe

In reply to J. Bruce Fields

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

Both nfsd4_init_conn and alloc_init_session are probing the callback channel, harmless but pointless.

Also, nfsd4_init_conn should probably be probing in the "unknown" case as well. In fact I don't see any harm to just doing it unconditionally when we get a new backchannel connection.

Signed-off-by J. Bruce Fields <[email protected]>
---
 fs/nfsd/nfs4state.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
fs/nfsd/nfs4state.c +1 -3
index 592cd7f..773b903 100644
@@ -861,8 +861,7 @@ static void nfsd4_init_conn(struct svc_rqst *rqstp, struct nfsd4_conn *conn, str
 	if (ret)
 		/* oops; xprt is already down: */
 		nfsd4_conn_lost(&conn->cn_xpt_user);
-	if (ses->se_client->cl_cb_state == NFSD4_CB_DOWN &&
-			conn->cn_flags & NFS4_CDFC4_BACK) {
+	if (conn->cn_flags & NFS4_CDFC4_BACK) {
 		/* callback channel may be back up */
 		nfsd4_probe_callback(ses->se_client);
 	}

@@ -979,7 +978,6 @@ static struct nfsd4_session *init_session(struct svc_rqst *rqstp, struct nfsd4_s
 		rpc_copy_addr((struct sockaddr *)&clp->cl_cb_conn.cb_addr, sa);
 		clp->cl_cb_conn.cb_addrlen = svc_addr_len(sa);
 	}
-	nfsd4_probe_callback(clp);
 	return new;
 }
Signature · 1 line
1.7.9.5