[PATCH Version 4 1/7] NFSv4.1: fix backchannel slotid off-by-one bug

[PATCH Version 4 1/7] NFSv4.1: fix backchannel slotid off-by-one bug

[andros]

From: Andy Adamson <andros@netapp.com>

Cc:stable@kernel.org
Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/callback_proc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
index 43926ad..54cea8a 100644
--- a/fs/nfs/callback_proc.c
+++ b/fs/nfs/callback_proc.c
@@ -339,7 +339,7 @@ validate_seqid(struct nfs4_slot_table *tbl, struct cb_sequenceargs * args)
 	dprintk("%s enter. slotid %d seqid %d\n",
 		__func__, args->csa_slotid, args->csa_sequenceid);
 
-	if (args->csa_slotid > NFS41_BC_MAX_CALLBACKS)
+	if (args->csa_slotid >= NFS41_BC_MAX_CALLBACKS)
 		return htonl(NFS4ERR_BADSLOT);
 
 	slot = tbl->slots + args->csa_slotid;
-- 
1.7.6.4

[PATCH Version 4 2/7] NFSv4.1: cleanup init and reset of session slot tables

[andros]

From: Andy Adamson <andros@netapp.com>

We are either initializing or resetting a session. Initialize or reset
the session slot tables accordingly.

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/nfs4proc.c |   59 +++++++++++++++++++---------------------------------
 1 files changed, 22 insertions(+), 37 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index b60fddf..ef098a8 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5014,23 +5014,6 @@ out:
 	return ret;
 }
 
-/*
- * Reset the forechannel and backchannel slot tables
- */
-static int nfs4_reset_slot_tables(struct nfs4_session *session)
-{
-	int status;
-
-	status = nfs4_reset_slot_table(&session->fc_slot_table,
-			session->fc_attrs.max_reqs, 1);
-	if (status)
-		return status;
-
-	status = nfs4_reset_slot_table(&session->bc_slot_table,
-			session->bc_attrs.max_reqs, 0);
-	return status;
-}
-
 /* Destroy the slot table */
 static void nfs4_destroy_slot_tables(struct nfs4_session *session)
 {
@@ -5076,29 +5059,35 @@ out:
 }
 
 /*
- * Initialize the forechannel and backchannel tables
+ * Initialize or reset the forechannel and backchannel tables
  */
-static int nfs4_init_slot_tables(struct nfs4_session *session)
+static int nfs4_setup_session_slot_tables(struct nfs4_session *ses)
 {
 	struct nfs4_slot_table *tbl;
-	int status = 0;
+	int status;
 
-	tbl = &session->fc_slot_table;
+	dprintk("--> %s\n", __func__);
+	/* Fore channel */
+	tbl = &ses->fc_slot_table;
 	if (tbl->slots == NULL) {
-		status = nfs4_init_slot_table(tbl,
-				session->fc_attrs.max_reqs, 1);
+		status = nfs4_init_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
+		if (status) /* -ENOMEM */
+			return status;
+	} else {
+		status = nfs4_reset_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
 		if (status)
 			return status;
 	}
-
-	tbl = &session->bc_slot_table;
+	/* Back channel */
+	tbl = &ses->bc_slot_table;
 	if (tbl->slots == NULL) {
-		status = nfs4_init_slot_table(tbl,
-				session->bc_attrs.max_reqs, 0);
+		status = nfs4_init_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
 		if (status)
-			nfs4_destroy_slot_tables(session);
-	}
-
+			/* Fore and back channel share a connection so get
+			 * both slot tables or neither */
+			nfs4_destroy_slot_tables(ses);
+	} else
+		status = nfs4_reset_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
 	return status;
 }
 
@@ -5286,13 +5275,9 @@ int nfs4_proc_create_session(struct nfs_client *clp)
 	if (status)
 		goto out;
 
-	/* Init and reset the fore channel */
-	status = nfs4_init_slot_tables(session);
-	dprintk("slot table initialization returned %d\n", status);
-	if (status)
-		goto out;
-	status = nfs4_reset_slot_tables(session);
-	dprintk("slot table reset returned %d\n", status);
+	/* Init or reset the session slot tables */
+	status = nfs4_setup_session_slot_tables(session);
+	dprintk("slot table setup returned %d\n", status);
 	if (status)
 		goto out;
 
-- 
1.7.6.4

[PATCH Version 4 3/7] NFSv4.1: change nfs4_free_slot parameters for dynamic slots

[andros]

From: Andy Adamson <andros@netapp.com>

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/nfs4proc.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index ef098a8..2638291 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -361,9 +361,8 @@ static void renew_lease(const struct nfs_server *server, unsigned long timestamp
  * Must be called while holding tbl->slot_tbl_lock
  */
 static void
-nfs4_free_slot(struct nfs4_slot_table *tbl, struct nfs4_slot *free_slot)
+nfs4_free_slot(struct nfs4_slot_table *tbl, u8 free_slotid)
 {
-	int free_slotid = free_slot - tbl->slots;
 	int slotid = free_slotid;
 
 	BUG_ON(slotid < 0 || slotid >= NFS4_MAX_SLOT_TABLE);
@@ -428,7 +427,7 @@ static void nfs41_sequence_free_slot(struct nfs4_sequence_res *res)
 	}
 
 	spin_lock(&tbl->slot_tbl_lock);
-	nfs4_free_slot(tbl, res->sr_slot);
+	nfs4_free_slot(tbl, res->sr_slot - tbl->slots);
 	nfs4_check_drain_fc_complete(res->sr_session);
 	spin_unlock(&tbl->slot_tbl_lock);
 	res->sr_slot = NULL;
-- 
1.7.6.4

[PATCH Version 4 4/7] NFSv4.1: dynamic session slots

[andros]

From: Andy Adamson <andros@netapp.com>

The session fore/backchannel slot tables implementation is changed from a
static array to an hlist hashed on slotid. Slotid's are values 0..N where N
is initially negotiated between the client and server via the CREATE_SESSION
call, and can be dynamically changed by the server during an active session.

When there are no allocated slots (mount), an initial number of session slots
is allocated equal to the inital number of dynamic rpc_slots
created upon transport creation (xprt->min_reqs).  Slots are then dynamically
allocated as needed upto the CREATE_SESSION negotiated max_reqs value.

For session reset the session is drained (inactive). The (old) session has
allocated slot tables which we will reuse. nfs4_reduce_slots_locked is called
and only reduces the allocated_slots if the new max_reqs negotiated by the
reset CREATE_SESSION is less than the number of allocated_slots. The resultant
slot table is then re-initialized.

CB_SLOT_RECALL is changed only in that it now shares code with session
reset - it also calls nfs4_reduce_slots_locked.

The backchannel uses the new dynamic slot allocator, even though we only
currently support a single back channel slot.

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/callback_proc.c    |   25 +++-
 fs/nfs/internal.h         |    2 +
 fs/nfs/nfs4_fs.h          |    1 +
 fs/nfs/nfs4proc.c         |  297 ++++++++++++++++++++++++++++++++++-----------
 fs/nfs/nfs4state.c        |   12 +--
 fs/nfs/nfs4xdr.c          |   20 ++--
 include/linux/nfs_fs_sb.h |   12 +-
 include/linux/nfs_xdr.h   |    4 +-
 8 files changed, 266 insertions(+), 107 deletions(-)

diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
index 54cea8a..77cc96c 100644
--- a/fs/nfs/callback_proc.c
+++ b/fs/nfs/callback_proc.c
@@ -342,7 +342,7 @@ validate_seqid(struct nfs4_slot_table *tbl, struct cb_sequenceargs * args)
 	if (args->csa_slotid >= NFS41_BC_MAX_CALLBACKS)
 		return htonl(NFS4ERR_BADSLOT);
 
-	slot = tbl->slots + args->csa_slotid;
+	slot = nfs4_lookup_slot_locked(tbl, args->csa_slotid);
 	dprintk("%s slot table seqid: %d\n", __func__, slot->seq_nr);
 
 	/* Normal */
@@ -377,6 +377,22 @@ out_ok:
 	return htonl(NFS4_OK);
 }
 
+static bool test_slot_referring(struct nfs4_slot_table *tbl,
+				struct referring_call *ref)
+{
+	struct nfs4_slot *slot;
+	bool status = false;
+
+	spin_lock(&tbl->slot_tbl_lock);
+	if (test_bit(ref->rc_slotid, tbl->used_slots)) {
+		slot = nfs4_lookup_slot_locked(tbl, ref->rc_slotid);
+		if (slot->seq_nr == ref->rc_sequenceid)
+			status = true;
+	}
+	spin_unlock(&tbl->slot_tbl_lock);
+	return status;
+}
+
 /*
  * For each referring call triple, check the session's slot table for
  * a match.  If the slot is in use and the sequence numbers match, the
@@ -417,12 +433,7 @@ static bool referring_call_exists(struct nfs_client *clp,
 				((u32 *)&rclist->rcl_sessionid.data)[2],
 				((u32 *)&rclist->rcl_sessionid.data)[3],
 				ref->rc_sequenceid, ref->rc_slotid);
-
-			spin_lock(&tbl->slot_tbl_lock);
-			status = (test_bit(ref->rc_slotid, tbl->used_slots) &&
-				  tbl->slots[ref->rc_slotid].seq_nr ==
-					ref->rc_sequenceid);
-			spin_unlock(&tbl->slot_tbl_lock);
+			status = test_slot_referring(tbl, ref);
 			if (status)
 				goto out;
 		}
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index c1a1bd8..4451abf 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -354,6 +354,8 @@ extern int _nfs4_call_sync_session(struct rpc_clnt *clnt,
 				   struct nfs4_sequence_args *args,
 				   struct nfs4_sequence_res *res,
 				   int cache_reply);
+struct nfs4_slot *nfs4_lookup_slot_locked(struct nfs4_slot_table *tbl,
+					  u8 slotid);
 
 /*
  * Determine the device name as a string
diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
index 693ae22..ff87169 100644
--- a/fs/nfs/nfs4_fs.h
+++ b/fs/nfs/nfs4_fs.h
@@ -242,6 +242,7 @@ extern struct nfs4_session *nfs4_alloc_session(struct nfs_client *clp);
 extern int nfs4_proc_create_session(struct nfs_client *);
 extern int nfs4_proc_destroy_session(struct nfs4_session *);
 extern int nfs4_init_session(struct nfs_server *server);
+extern void nfs4_reduce_slots_locked(struct nfs4_slot_table *tbl);
 extern int nfs4_proc_get_lease_time(struct nfs_client *clp,
 		struct nfs_fsinfo *fsinfo);
 extern int nfs4_proc_layoutcommit(struct nfs4_layoutcommit_data *data,
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 2638291..e3a7663 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -346,6 +346,162 @@ static void renew_lease(const struct nfs_server *server, unsigned long timestamp
 #if defined(CONFIG_NFS_V4_1)
 
 /*
+ * Session dynamic slot table helper functions
+ */
+
+static inline u32 slot_tbl_hash(const u8 slotid)
+{
+	return (u32)slotid % SLOT_TABLE_SIZE;
+}
+
+static void nfs4_init_slot(struct nfs4_slot *new, u8 slotid, int ivalue)
+{
+	INIT_HLIST_NODE(&new->slot_node);
+	new->slotid = slotid;
+	new->seq_nr = ivalue;
+}
+
+static void nfs4_insert_slot_locked(struct nfs4_slot_table *tbl,
+				    struct nfs4_slot *new)
+{
+	u32 hash = slot_tbl_hash(new->slotid);
+
+	hlist_add_head(&new->slot_node, &tbl->slots[hash]);
+	tbl->allocated_slots++;
+}
+
+/*
+ * Allocate a slot for the next slotid (tbl->allocated_slotid)
+ */
+static int nfs4_alloc_add_slot(struct nfs4_slot_table *tbl,
+			       int ivalue, int slotid,
+			       gfp_t gfp_flags)
+{
+	struct nfs4_slot *new;
+	struct rpc_task *task;
+
+	dprintk("--> %s slotid=%u tbl->allocated_slots=%d\n", __func__,
+		slotid, tbl->allocated_slots);
+	new = kzalloc(sizeof(struct nfs4_slot), gfp_flags);
+	if (!new)
+		return -ENOMEM;
+	/* tbl->allocated_slots holds the next unallocated slotid */
+	nfs4_init_slot(new, slotid, ivalue);
+	spin_lock(&tbl->slot_tbl_lock);
+	if (tbl->allocated_slots != slotid) {
+		spin_unlock(&tbl->slot_tbl_lock);
+		kfree(new);
+		dprintk("%s slotid %u already allocated\n", __func__,
+			slotid);
+		return 0;
+	}
+	nfs4_insert_slot_locked(tbl, new);
+
+	/* This only applies to dynamic allocation */
+	task = rpc_wake_up_next(&tbl->slot_tbl_waitq);
+	if (task)
+		dprintk("%s woke up task %5u\n", __func__, task->tk_pid);
+	spin_unlock(&tbl->slot_tbl_lock);
+	return 0;
+}
+
+/*
+ * Allocate the minimal number of slots required by the transport.
+ * Called at session creation.
+ */
+static int nfs4_prealloc_slots(struct nfs4_slot_table *tbl, int num, int ivalue)
+{
+	int i;
+
+	for (i = 0; i < num; i++) {
+		if (nfs4_alloc_add_slot(tbl, ivalue, tbl->allocated_slots,
+					GFP_NOFS) != 0)
+			return -ENOMEM;
+	}
+	return 0;
+}
+
+/*
+ * Allocate the next slotid. If allocation fails, next request will retry.
+ */
+static void nfs4_dynamic_alloc_slot(struct nfs4_slot_table *tbl, u8 slotid)
+{
+	if (slotid != tbl->allocated_slots) {
+		dprintk("%s slotid %u already allocated\n", __func__, slotid);
+		return;
+	}
+	nfs4_alloc_add_slot(tbl, 1, slotid, GFP_NOWAIT);
+}
+
+/*
+ * Return the slot associated with the slotid returned by nfs4_find_slot.
+ * If the slot is not found, it has not been allocated.
+ */
+struct nfs4_slot *
+nfs4_lookup_slot_locked(struct nfs4_slot_table *tbl, u8 slotid)
+{
+	struct nfs4_slot *sp;
+	struct hlist_node *n;
+	u32 hash = slot_tbl_hash(slotid);
+
+	hlist_for_each_entry(sp, n, &tbl->slots[hash], slot_node) {
+		if (sp->slotid == slotid)
+			return sp;
+	}
+	return NULL;
+}
+
+static void nfs4_remove_slot_locked(struct nfs4_slot_table *tbl,
+				    struct nfs4_slot *slot)
+{
+	dprintk("--> %s slotid %d\n", __func__, slot->slotid);
+	hlist_del_init(&slot->slot_node);
+	tbl->allocated_slots--;
+	kfree(slot);
+}
+
+static void nfs4_free_all_slots(struct nfs4_slot_table *tbl)
+{
+	struct nfs4_slot *slotp;
+	int i;
+
+	spin_lock(&tbl->slot_tbl_lock);
+	for (i = 0; i < SLOT_TABLE_SIZE; i++) {
+		while (!hlist_empty(&tbl->slots[i])) {
+			slotp = hlist_entry(tbl->slots[i].first,
+					    struct nfs4_slot, slot_node);
+			nfs4_remove_slot_locked(tbl, slotp);
+		}
+		/* re-initialize hlist_head */
+		tbl->slots[i].first = NULL;
+	}
+	spin_unlock(&tbl->slot_tbl_lock);
+}
+
+/*
+ * Remove num_dec number of contiguous slotids starting from highest
+ * allocated slotid
+ *
+ * Note: set the new tbl->max_slots prior to calling.
+ */
+void nfs4_reduce_slots_locked(struct nfs4_slot_table *tbl)
+{
+	struct nfs4_slot *removeme;
+	int i, rm_slotid = tbl->allocated_slots - 1;
+	int num_dec = tbl->allocated_slots - tbl->max_slots;
+
+	/* Reset of the slot table can make num_dec <= 0 */
+	if (num_dec <= 0)
+		return;
+	for (i = 0; i < num_dec; i++) {
+		removeme = nfs4_lookup_slot_locked(tbl, rm_slotid);
+		BUG_ON(removeme == NULL);
+		nfs4_remove_slot_locked(tbl, removeme);
+		rm_slotid--;
+	}
+}
+
+/*
  * nfs4_free_slot - free a slot and efficiently update slot table.
  *
  * freeing a slot is trivially done by clearing its respective bit
@@ -390,8 +546,11 @@ static void nfs4_check_drain_fc_complete(struct nfs4_session *ses)
 
 	if (!test_bit(NFS4_SESSION_DRAINING, &ses->session_state)) {
 		task = rpc_wake_up_next(&ses->fc_slot_table.slot_tbl_waitq);
-		if (task)
+		if (task) {
 			rpc_task_set_priority(task, RPC_PRIORITY_PRIVILEGED);
+			dprintk("%s woke up task pid %5u\n", __func__,
+				task->tk_pid);
+		}
 		return;
 	}
 
@@ -427,7 +586,7 @@ static void nfs41_sequence_free_slot(struct nfs4_sequence_res *res)
 	}
 
 	spin_lock(&tbl->slot_tbl_lock);
-	nfs4_free_slot(tbl, res->sr_slot - tbl->slots);
+	nfs4_free_slot(tbl, res->sr_slot->slotid);
 	nfs4_check_drain_fc_complete(res->sr_session);
 	spin_unlock(&tbl->slot_tbl_lock);
 	res->sr_slot = NULL;
@@ -468,10 +627,8 @@ static int nfs41_sequence_done(struct rpc_task *task, struct nfs4_sequence_res *
 		 * returned NFS4ERR_DELAY as per Section 2.10.6.2
 		 * of RFC5661.
 		 */
-		dprintk("%s: slot=%td seq=%d: Operation in progress\n",
-			__func__,
-			res->sr_slot - res->sr_session->fc_slot_table.slots,
-			res->sr_slot->seq_nr);
+		dprintk("%s: slotid=%u seq=%d: Operation in progress\n",
+			__func__, res->sr_slot->slotid, res->sr_slot->seq_nr);
 		goto out_retry;
 	default:
 		/* Just update the slot sequence no. */
@@ -479,7 +636,8 @@ static int nfs41_sequence_done(struct rpc_task *task, struct nfs4_sequence_res *
 	}
 out:
 	/* The session may be reset by one of the error handlers. */
-	dprintk("%s: Error %d free the slot \n", __func__, res->sr_status);
+	dprintk("%s: Error %d free the slot for task pid %5u\n", __func__,
+		res->sr_status, task->tk_pid);
 	nfs41_sequence_free_slot(res);
 	return 1;
 out_retry:
@@ -540,7 +698,7 @@ int nfs41_setup_sequence(struct nfs4_session *session,
 	struct nfs4_slot_table *tbl;
 	u8 slotid;
 
-	dprintk("--> %s\n", __func__);
+	dprintk("--> %s task pid %5u\n", __func__, task->tk_pid);
 	/* slot already allocated? */
 	if (res->sr_slot != NULL)
 		return 0;
@@ -575,12 +733,24 @@ int nfs41_setup_sequence(struct nfs4_session *session,
 		dprintk("<-- %s: no free slots\n", __func__);
 		return -EAGAIN;
 	}
+	slot = nfs4_lookup_slot_locked(tbl, slotid);
+	if (slot == NULL) {
+		/* keep FIFO order */
+		rpc_task_set_priority(task, RPC_PRIORITY_PRIVILEGED);
+		rpc_sleep_on(&tbl->slot_tbl_waitq, task, NULL);
+		nfs4_free_slot(tbl, slotid);
+		spin_unlock(&tbl->slot_tbl_lock);
+		dprintk("%s task pid %5u sleeping for dynamic slot %d\n",
+			__func__, task->tk_pid, slotid);
+		/* rpc_wakeup_next called upon success by slot allocator */
+		nfs4_dynamic_alloc_slot(tbl, slotid);
+		return -EAGAIN;
+	}
 	spin_unlock(&tbl->slot_tbl_lock);
 
 	rpc_task_set_priority(task, RPC_PRIORITY_NORMAL);
-	slot = tbl->slots + slotid;
 	args->sa_session = session;
-	args->sa_slotid = slotid;
+	args->sa_slot = slot;
 	args->sa_cache_this = cache_reply;
 
 	dprintk("<-- %s slotid=%d seqid=%d\n", __func__, slotid, slot->seq_nr);
@@ -613,9 +783,9 @@ int nfs4_setup_sequence(const struct nfs_server *server,
 		goto out;
 	}
 
-	dprintk("--> %s clp %p session %p sr_slot %td\n",
+	dprintk("--> %s clp %p session %p sr_slotid %d\n",
 		__func__, session->clp, session, res->sr_slot ?
-			res->sr_slot - session->fc_slot_table.slots : -1);
+			res->sr_slot->slotid : -1);
 
 	ret = nfs41_setup_sequence(session, args, res, cache_reply,
 				   task);
@@ -4976,84 +5146,64 @@ int nfs4_proc_get_lease_time(struct nfs_client *clp, struct nfs_fsinfo *fsinfo)
 }
 
 /*
- * Reset a slot table
+ * Reset a slot table. Session has been drained and reset with new slot table
+ * parameters from create_session.
+ * Keep all allocated slots that fit into new slot table max_reqs parameter.
  */
-static int nfs4_reset_slot_table(struct nfs4_slot_table *tbl, u32 max_reqs,
+static void nfs4_reset_slot_table(struct nfs4_slot_table *tbl, u32 new_max_reqs,
 				 int ivalue)
 {
-	struct nfs4_slot *new = NULL;
+	struct nfs4_slot *sp;
+	struct hlist_node *n;
 	int i;
-	int ret = 0;
-
-	dprintk("--> %s: max_reqs=%u, tbl->max_slots %d\n", __func__,
-		max_reqs, tbl->max_slots);
 
-	/* Does the newly negotiated max_reqs match the existing slot table? */
-	if (max_reqs != tbl->max_slots) {
-		ret = -ENOMEM;
-		new = kmalloc(max_reqs * sizeof(struct nfs4_slot),
-			      GFP_NOFS);
-		if (!new)
-			goto out;
-		ret = 0;
-		kfree(tbl->slots);
-	}
 	spin_lock(&tbl->slot_tbl_lock);
-	if (new) {
-		tbl->slots = new;
-		tbl->max_slots = max_reqs;
+	tbl->max_slots = new_max_reqs;
+	nfs4_reduce_slots_locked(tbl);
+	tbl->highest_used_slotid = -1;
+
+	for (i = 0; i < SLOT_TABLE_SIZE; i++) {
+		hlist_for_each_entry(sp, n, &tbl->slots[i], slot_node)
+			sp->seq_nr = ivalue;
 	}
-	for (i = 0; i < tbl->max_slots; ++i)
-		tbl->slots[i].seq_nr = ivalue;
 	spin_unlock(&tbl->slot_tbl_lock);
-	dprintk("%s: tbl=%p slots=%p max_slots=%d\n", __func__,
-		tbl, tbl->slots, tbl->max_slots);
-out:
-	dprintk("<-- %s: return %d\n", __func__, ret);
-	return ret;
+	dprintk("%s: tbl->max_slots=%d tbl->allocated_slots=%d\n",
+		__func__, tbl->max_slots, tbl->allocated_slots);
+	return;
 }
 
 /* Destroy the slot table */
 static void nfs4_destroy_slot_tables(struct nfs4_session *session)
 {
-	if (session->fc_slot_table.slots != NULL) {
-		kfree(session->fc_slot_table.slots);
-		session->fc_slot_table.slots = NULL;
-	}
-	if (session->bc_slot_table.slots != NULL) {
-		kfree(session->bc_slot_table.slots);
-		session->bc_slot_table.slots = NULL;
-	}
-	return;
+	dprintk("--> %s\n", __func__);
+
+	nfs4_free_all_slots(&session->fc_slot_table);
+	nfs4_free_all_slots(&session->bc_slot_table);
 }
 
 /*
  * Initialize slot table
  */
 static int nfs4_init_slot_table(struct nfs4_slot_table *tbl,
-		int max_slots, int ivalue)
+		int max_slots, int num_prealloc, int ivalue)
 {
-	struct nfs4_slot *slot;
-	int ret = -ENOMEM;
+	int ret;
 
 	BUG_ON(max_slots > NFS4_MAX_SLOT_TABLE);
-
-	dprintk("--> %s: max_reqs=%u\n", __func__, max_slots);
-
-	slot = kcalloc(max_slots, sizeof(struct nfs4_slot), GFP_NOFS);
-	if (!slot)
+	ret = nfs4_prealloc_slots(tbl, num_prealloc, ivalue);
+	if (ret) {
+		nfs4_free_all_slots(tbl);
 		goto out;
+	}
 	ret = 0;
 
 	spin_lock(&tbl->slot_tbl_lock);
 	tbl->max_slots = max_slots;
-	tbl->slots = slot;
 	tbl->highest_used_slotid = -1;  /* no slot is currently used */
 	spin_unlock(&tbl->slot_tbl_lock);
-	dprintk("%s: tbl=%p slots=%p max_slots=%d\n", __func__,
-		tbl, tbl->slots, tbl->max_slots);
+	dprintk("%s: tbl=%p max_slots=%d, tbl->allocated_slots=%d\n", __func__,
+		tbl, tbl->max_slots, tbl->allocated_slots);
 out:
-	dprintk("<-- %s: return %d\n", __func__, ret);
 	return ret;
 }
 
@@ -5063,30 +5213,34 @@ out:
 static int nfs4_setup_session_slot_tables(struct nfs4_session *ses)
 {
 	struct nfs4_slot_table *tbl;
-	int status;
+	int status = 0;
 
 	dprintk("--> %s\n", __func__);
 	/* Fore channel */
 	tbl = &ses->fc_slot_table;
-	if (tbl->slots == NULL) {
-		status = nfs4_init_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
+	if (tbl->allocated_slots == 0) {
+		status = nfs4_init_slot_table(tbl, ses->fc_attrs.max_reqs,
+				ses->clp->cl_rpcclient->cl_xprt->min_reqs, 1);
 		if (status) /* -ENOMEM */
 			return status;
 	} else {
-		status = nfs4_reset_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
-		if (status)
-			return status;
+		nfs4_reset_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
 	}
+
 	/* Back channel */
 	tbl = &ses->bc_slot_table;
-	if (tbl->slots == NULL) {
-		status = nfs4_init_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
+	if (tbl->allocated_slots == 0) {
+		status = nfs4_init_slot_table(tbl, ses->bc_attrs.max_reqs,
+				/* Backchannel max_reqs == 1 */
+				ses->bc_attrs.max_reqs, 0);
 		if (status)
 			/* Fore and back channel share a connection so get
 			 * both slot tables or neither */
 			nfs4_destroy_slot_tables(ses);
-	} else
-		status = nfs4_reset_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
+	} else {
+		nfs4_reset_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
+	}
+
 	return status;
 }
 
@@ -5276,7 +5430,6 @@ int nfs4_proc_create_session(struct nfs_client *clp)
 
 	/* Init or reset the session slot tables */
 	status = nfs4_setup_session_slot_tables(session);
-	dprintk("slot table setup returned %d\n", status);
 	if (status)
 		goto out;
 
@@ -5284,7 +5437,7 @@ int nfs4_proc_create_session(struct nfs_client *clp)
 	dprintk("%s client>seqid %d sessionid %u:%u:%u:%u\n", __func__,
 		clp->cl_seqid, ptr[0], ptr[1], ptr[2], ptr[3]);
 out:
-	dprintk("<-- %s\n", __func__);
+	dprintk("<-- %s status %d\n", __func__, status);
 	return status;
 }
 
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 39914be..8260865 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -1573,26 +1573,16 @@ static int nfs4_recall_slot(struct nfs_client *clp)
 {
 	struct nfs4_slot_table *fc_tbl = &clp->cl_session->fc_slot_table;
 	struct nfs4_channel_attrs *fc_attrs = &clp->cl_session->fc_attrs;
-	struct nfs4_slot *new, *old;
-	int i;
 
 	nfs4_begin_drain_session(clp);
-	new = kmalloc(fc_tbl->target_max_slots * sizeof(struct nfs4_slot),
-		      GFP_NOFS);
-        if (!new)
-		return -ENOMEM;
 
 	spin_lock(&fc_tbl->slot_tbl_lock);
-	for (i = 0; i < fc_tbl->target_max_slots; i++)
-		new[i].seq_nr = fc_tbl->slots[i].seq_nr;
-	old = fc_tbl->slots;
-	fc_tbl->slots = new;
 	fc_tbl->max_slots = fc_tbl->target_max_slots;
+	nfs4_reduce_slots_locked(fc_tbl);
 	fc_tbl->target_max_slots = 0;
 	fc_attrs->max_reqs = fc_tbl->max_slots;
 	spin_unlock(&fc_tbl->slot_tbl_lock);
 
-	kfree(old);
 	nfs4_end_drain_session(clp);
 	return 0;
 }
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index e6161b2..c42ec29 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -1872,7 +1872,6 @@ static void encode_sequence(struct xdr_stream *xdr,
 #if defined(CONFIG_NFS_V4_1)
 	struct nfs4_session *session = args->sa_session;
 	struct nfs4_slot_table *tp;
-	struct nfs4_slot *slot;
 	__be32 *p;
 
 	if (!session)
@@ -1880,8 +1879,7 @@ static void encode_sequence(struct xdr_stream *xdr,
 
 	tp = &session->fc_slot_table;
 
-	WARN_ON(args->sa_slotid == NFS4_MAX_SLOT_TABLE);
-	slot = tp->slots + args->sa_slotid;
+	WARN_ON(args->sa_slot->slotid == NFS4_MAX_SLOT_TABLE);
 
 	p = reserve_space(xdr, 4 + NFS4_MAX_SESSIONID_LEN + 16);
 	*p++ = cpu_to_be32(OP_SEQUENCE);
@@ -1896,11 +1894,11 @@ static void encode_sequence(struct xdr_stream *xdr,
 		((u32 *)session->sess_id.data)[1],
 		((u32 *)session->sess_id.data)[2],
 		((u32 *)session->sess_id.data)[3],
-		slot->seq_nr, args->sa_slotid,
+		args->sa_slot->seq_nr, args->sa_slot->slotid,
 		tp->highest_used_slotid, args->sa_cache_this);
 	p = xdr_encode_opaque_fixed(p, session->sess_id.data, NFS4_MAX_SESSIONID_LEN);
-	*p++ = cpu_to_be32(slot->seq_nr);
-	*p++ = cpu_to_be32(args->sa_slotid);
+	*p++ = cpu_to_be32(args->sa_slot->seq_nr);
+	*p++ = cpu_to_be32(args->sa_slot->slotid);
 	*p++ = cpu_to_be32(tp->highest_used_slotid);
 	*p = cpu_to_be32(args->sa_cache_this);
 	hdr->nops++;
@@ -5361,13 +5359,17 @@ static int decode_sequence(struct xdr_stream *xdr,
 	/* seqid */
 	dummy = be32_to_cpup(p++);
 	if (dummy != res->sr_slot->seq_nr) {
-		dprintk("%s Invalid sequence number\n", __func__);
+		dprintk("%s Invalid seq num %d for [slotid:seq_nr] [%d:%d]\n",
+			__func__, dummy, res->sr_slot->slotid,
+			res->sr_slot->seq_nr);
 		goto out_err;
 	}
 	/* slot id */
 	dummy = be32_to_cpup(p++);
-	if (dummy != res->sr_slot - res->sr_session->fc_slot_table.slots) {
-		dprintk("%s Invalid slot id\n", __func__);
+	if (dummy != res->sr_slot->slotid) {
+		dprintk("%s Invalid slot id %d for [slotid:seq_nr] [%d:%d]\n",
+			__func__, dummy, res->sr_slot->slotid,
+			res->sr_slot->seq_nr);
 		goto out_err;
 	}
 	/* highest slot id - currently not processed */
diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
index b5479df..90c6e2b 100644
--- a/include/linux/nfs_fs_sb.h
+++ b/include/linux/nfs_fs_sb.h
@@ -193,12 +193,15 @@ struct nfs_server {
 
 /* Sessions */
 #define SLOT_TABLE_SZ (NFS4_MAX_SLOT_TABLE/(8*sizeof(long)))
+#define SLOT_TABLE_BITS                5
+#define SLOT_TABLE_SIZE                (1 << SLOT_TABLE_BITS)
 struct nfs4_slot_table {
-	struct nfs4_slot *slots;		/* seqid per slot */
+	struct hlist_head slots[SLOT_TABLE_SIZE];
 	unsigned long   used_slots[SLOT_TABLE_SZ]; /* used/unused bitmap */
 	spinlock_t	slot_tbl_lock;
 	struct rpc_wait_queue	slot_tbl_waitq;	/* allocators may wait here */
-	int		max_slots;		/* # slots in table */
+	int		allocated_slots;	/* # slots in table */
+	int		max_slots;		/* max # slots in table */
 	int		highest_used_slotid;	/* sent to server on each SEQ.
 						 * op for dynamic resizing */
 	int		target_max_slots;	/* Set by CB_RECALL_SLOT as
@@ -206,11 +209,6 @@ struct nfs4_slot_table {
 	struct completion complete;
 };
 
-static inline int slot_idx(struct nfs4_slot_table *tbl, struct nfs4_slot *sp)
-{
-	return sp - tbl->slots;
-}
-
 /*
  * Session related parameters
  */
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index c74595b..76b27a4 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -168,12 +168,14 @@ struct nfs4_channel_attrs {
 
 /* nfs41 sessions slot seqid */
 struct nfs4_slot {
+	struct hlist_node	slot_node;
 	u32		 	seq_nr;
+	u8			slotid;
 };
 
 struct nfs4_sequence_args {
 	struct nfs4_session	*sa_session;
-	u8			sa_slotid;
+	struct nfs4_slot        *sa_slot;
 	u8			sa_cache_this;
 };
 
-- 
1.7.6.4

Re: [PATCH Version 4 4/7] NFSv4.1: dynamic session slots

[Benny Halevy]

Hi Andy,

I have some minor-ish comments inline below, but the whole
hlist concept looks to me like it might incur pretty high overhead
in cpu and memory caching.

Have you considered simply reallocating the slot table on grow
and maybe on shrink below a certain threshold?

On 2011-11-09 20:58, andros@netapp.com wrote:
> From: Andy Adamson <andros@netapp.com>
> 
> The session fore/backchannel slot tables implementation is changed from a
> static array to an hlist hashed on slotid. Slotid's are values 0..N where N
> is initially negotiated between the client and server via the CREATE_SESSION
> call, and can be dynamically changed by the server during an active session.
> 
> When there are no allocated slots (mount), an initial number of session slots
> is allocated equal to the inital number of dynamic rpc_slots
> created upon transport creation (xprt->min_reqs).  Slots are then dynamically
> allocated as needed upto the CREATE_SESSION negotiated max_reqs value.
> 
> For session reset the session is drained (inactive). The (old) session has
> allocated slot tables which we will reuse. nfs4_reduce_slots_locked is called
> and only reduces the allocated_slots if the new max_reqs negotiated by the
> reset CREATE_SESSION is less than the number of allocated_slots. The resultant
> slot table is then re-initialized.
> 
> CB_SLOT_RECALL is changed only in that it now shares code with session
> reset - it also calls nfs4_reduce_slots_locked.
> 
> The backchannel uses the new dynamic slot allocator, even though we only
> currently support a single back channel slot.
> 
> Signed-off-by: Andy Adamson <andros@netapp.com>
> ---
>  fs/nfs/callback_proc.c    |   25 +++-
>  fs/nfs/internal.h         |    2 +
>  fs/nfs/nfs4_fs.h          |    1 +
>  fs/nfs/nfs4proc.c         |  297 ++++++++++++++++++++++++++++++++++-----------
>  fs/nfs/nfs4state.c        |   12 +--
>  fs/nfs/nfs4xdr.c          |   20 ++--
>  include/linux/nfs_fs_sb.h |   12 +-
>  include/linux/nfs_xdr.h   |    4 +-
>  8 files changed, 266 insertions(+), 107 deletions(-)
> 
> diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
> index 54cea8a..77cc96c 100644
> --- a/fs/nfs/callback_proc.c
> +++ b/fs/nfs/callback_proc.c
> @@ -342,7 +342,7 @@ validate_seqid(struct nfs4_slot_table *tbl, struct cb_sequenceargs * args)
>  	if (args->csa_slotid >= NFS41_BC_MAX_CALLBACKS)
>  		return htonl(NFS4ERR_BADSLOT);
>  
> -	slot = tbl->slots + args->csa_slotid;
> +	slot = nfs4_lookup_slot_locked(tbl, args->csa_slotid);
>  	dprintk("%s slot table seqid: %d\n", __func__, slot->seq_nr);
>  
>  	/* Normal */
> @@ -377,6 +377,22 @@ out_ok:
>  	return htonl(NFS4_OK);
>  }
>  
> +static bool test_slot_referring(struct nfs4_slot_table *tbl,
> +				struct referring_call *ref)
> +{
> +	struct nfs4_slot *slot;
> +	bool status = false;
> +
> +	spin_lock(&tbl->slot_tbl_lock);
> +	if (test_bit(ref->rc_slotid, tbl->used_slots)) {
> +		slot = nfs4_lookup_slot_locked(tbl, ref->rc_slotid);
> +		if (slot->seq_nr == ref->rc_sequenceid)
> +			status = true;
> +	}
> +	spin_unlock(&tbl->slot_tbl_lock);
> +	return status;
> +}
> +
>  /*
>   * For each referring call triple, check the session's slot table for
>   * a match.  If the slot is in use and the sequence numbers match, the
> @@ -417,12 +433,7 @@ static bool referring_call_exists(struct nfs_client *clp,
>  				((u32 *)&rclist->rcl_sessionid.data)[2],
>  				((u32 *)&rclist->rcl_sessionid.data)[3],
>  				ref->rc_sequenceid, ref->rc_slotid);
> -
> -			spin_lock(&tbl->slot_tbl_lock);
> -			status = (test_bit(ref->rc_slotid, tbl->used_slots) &&
> -				  tbl->slots[ref->rc_slotid].seq_nr ==
> -					ref->rc_sequenceid);
> -			spin_unlock(&tbl->slot_tbl_lock);
> +			status = test_slot_referring(tbl, ref);
>  			if (status)
>  				goto out;
>  		}
> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> index c1a1bd8..4451abf 100644
> --- a/fs/nfs/internal.h
> +++ b/fs/nfs/internal.h
> @@ -354,6 +354,8 @@ extern int _nfs4_call_sync_session(struct rpc_clnt *clnt,
>  				   struct nfs4_sequence_args *args,
>  				   struct nfs4_sequence_res *res,
>  				   int cache_reply);
> +struct nfs4_slot *nfs4_lookup_slot_locked(struct nfs4_slot_table *tbl,
> +					  u8 slotid);
>  
>  /*
>   * Determine the device name as a string
> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
> index 693ae22..ff87169 100644
> --- a/fs/nfs/nfs4_fs.h
> +++ b/fs/nfs/nfs4_fs.h
> @@ -242,6 +242,7 @@ extern struct nfs4_session *nfs4_alloc_session(struct nfs_client *clp);
>  extern int nfs4_proc_create_session(struct nfs_client *);
>  extern int nfs4_proc_destroy_session(struct nfs4_session *);
>  extern int nfs4_init_session(struct nfs_server *server);
> +extern void nfs4_reduce_slots_locked(struct nfs4_slot_table *tbl);
>  extern int nfs4_proc_get_lease_time(struct nfs_client *clp,
>  		struct nfs_fsinfo *fsinfo);
>  extern int nfs4_proc_layoutcommit(struct nfs4_layoutcommit_data *data,
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 2638291..e3a7663 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -346,6 +346,162 @@ static void renew_lease(const struct nfs_server *server, unsigned long timestamp
>  #if defined(CONFIG_NFS_V4_1)
>  
>  /*
> + * Session dynamic slot table helper functions
> + */
> +
> +static inline u32 slot_tbl_hash(const u8 slotid)

nit: "const" not really needed here...

> +{
> +	return (u32)slotid % SLOT_TABLE_SIZE;
> +}
> +
> +static void nfs4_init_slot(struct nfs4_slot *new, u8 slotid, int ivalue)
> +{
> +	INIT_HLIST_NODE(&new->slot_node);
> +	new->slotid = slotid;
> +	new->seq_nr = ivalue;
> +}
> +
> +static void nfs4_insert_slot_locked(struct nfs4_slot_table *tbl,
> +				    struct nfs4_slot *new)
> +{
> +	u32 hash = slot_tbl_hash(new->slotid);
> +
> +	hlist_add_head(&new->slot_node, &tbl->slots[hash]);
> +	tbl->allocated_slots++;
> +}
> +
> +/*
> + * Allocate a slot for the next slotid (tbl->allocated_slotid)
> + */
> +static int nfs4_alloc_add_slot(struct nfs4_slot_table *tbl,
> +			       int ivalue, int slotid,
> +			       gfp_t gfp_flags)
> +{
> +	struct nfs4_slot *new;
> +	struct rpc_task *task;
> +
> +	dprintk("--> %s slotid=%u tbl->allocated_slots=%d\n", __func__,
> +		slotid, tbl->allocated_slots);
> +	new = kzalloc(sizeof(struct nfs4_slot), gfp_flags);
> +	if (!new)
> +		return -ENOMEM;
> +	/* tbl->allocated_slots holds the next unallocated slotid */
> +	nfs4_init_slot(new, slotid, ivalue);
> +	spin_lock(&tbl->slot_tbl_lock);
> +	if (tbl->allocated_slots != slotid) {

This condition doesn't seem to be reliable.
What if the slot table shrunk?

> +		spin_unlock(&tbl->slot_tbl_lock);
> +		kfree(new);
> +		dprintk("%s slotid %u already allocated\n", __func__,
> +			slotid);
> +		return 0;
> +	}
> +	nfs4_insert_slot_locked(tbl, new);
> +
> +	/* This only applies to dynamic allocation */
> +	task = rpc_wake_up_next(&tbl->slot_tbl_waitq);
> +	if (task)
> +		dprintk("%s woke up task %5u\n", __func__, task->tk_pid);
> +	spin_unlock(&tbl->slot_tbl_lock);
> +	return 0;
> +}
> +
> +/*
> + * Allocate the minimal number of slots required by the transport.
> + * Called at session creation.
> + */
> +static int nfs4_prealloc_slots(struct nfs4_slot_table *tbl, int num, int ivalue)
> +{
> +	int i;
> +
> +	for (i = 0; i < num; i++) {
> +		if (nfs4_alloc_add_slot(tbl, ivalue, tbl->allocated_slots,
> +					GFP_NOFS) != 0)
> +			return -ENOMEM;

Returning nfs4_alloc_add_slot's status would be more appropriate.
How about also freeing 0 to i-1 in the error case?

> +	}
> +	return 0;
> +}
> +
> +/*
> + * Allocate the next slotid. If allocation fails, next request will retry.
> + */
> +static void nfs4_dynamic_alloc_slot(struct nfs4_slot_table *tbl, u8 slotid)
> +{
> +	if (slotid != tbl->allocated_slots) {
> +		dprintk("%s slotid %u already allocated\n", __func__, slotid);
> +		return;
> +	}

This is checked again under the lock in nfs4_alloc_add_slot, no?
so if it is not supposed to be a common case (which I don't believe
it is) I'd just forget about this optimization and call the latter
directly.

> +	nfs4_alloc_add_slot(tbl, 1, slotid, GFP_NOWAIT);
> +}
> +
> +/*
> + * Return the slot associated with the slotid returned by nfs4_find_slot.
> + * If the slot is not found, it has not been allocated.
> + */
> +struct nfs4_slot *
> +nfs4_lookup_slot_locked(struct nfs4_slot_table *tbl, u8 slotid)
> +{
> +	struct nfs4_slot *sp;
> +	struct hlist_node *n;
> +	u32 hash = slot_tbl_hash(slotid);
> +
> +	hlist_for_each_entry(sp, n, &tbl->slots[hash], slot_node) {
> +		if (sp->slotid == slotid)
> +			return sp;
> +	}
> +	return NULL;
> +}
> +
> +static void nfs4_remove_slot_locked(struct nfs4_slot_table *tbl,
> +				    struct nfs4_slot *slot)
> +{
> +	dprintk("--> %s slotid %d\n", __func__, slot->slotid);
> +	hlist_del_init(&slot->slot_node);
> +	tbl->allocated_slots--;
> +	kfree(slot);
> +}
> +
> +static void nfs4_free_all_slots(struct nfs4_slot_table *tbl)
> +{
> +	struct nfs4_slot *slotp;
> +	int i;
> +
> +	spin_lock(&tbl->slot_tbl_lock);
> +	for (i = 0; i < SLOT_TABLE_SIZE; i++) {
> +		while (!hlist_empty(&tbl->slots[i])) {
> +			slotp = hlist_entry(tbl->slots[i].first,
> +					    struct nfs4_slot, slot_node);
> +			nfs4_remove_slot_locked(tbl, slotp);
> +		}
> +		/* re-initialize hlist_head */
> +		tbl->slots[i].first = NULL;
> +	}
> +	spin_unlock(&tbl->slot_tbl_lock);
> +}
> +
> +/*
> + * Remove num_dec number of contiguous slotids starting from highest
> + * allocated slotid
> + *
> + * Note: set the new tbl->max_slots prior to calling.
> + */
> +void nfs4_reduce_slots_locked(struct nfs4_slot_table *tbl)
> +{
> +	struct nfs4_slot *removeme;
> +	int i, rm_slotid = tbl->allocated_slots - 1;
> +	int num_dec = tbl->allocated_slots - tbl->max_slots;
> +
> +	/* Reset of the slot table can make num_dec <= 0 */
> +	if (num_dec <= 0)
> +		return;
> +	for (i = 0; i < num_dec; i++) {
> +		removeme = nfs4_lookup_slot_locked(tbl, rm_slotid);
> +		BUG_ON(removeme == NULL);
> +		nfs4_remove_slot_locked(tbl, removeme);
> +		rm_slotid--;
> +	}
> +}
> +
> +/*
>   * nfs4_free_slot - free a slot and efficiently update slot table.
>   *
>   * freeing a slot is trivially done by clearing its respective bit
> @@ -390,8 +546,11 @@ static void nfs4_check_drain_fc_complete(struct nfs4_session *ses)
>  
>  	if (!test_bit(NFS4_SESSION_DRAINING, &ses->session_state)) {
>  		task = rpc_wake_up_next(&ses->fc_slot_table.slot_tbl_waitq);
> -		if (task)
> +		if (task) {
>  			rpc_task_set_priority(task, RPC_PRIORITY_PRIVILEGED);
> +			dprintk("%s woke up task pid %5u\n", __func__,
> +				task->tk_pid);
> +		}
>  		return;
>  	}
>  
> @@ -427,7 +586,7 @@ static void nfs41_sequence_free_slot(struct nfs4_sequence_res *res)
>  	}
>  
>  	spin_lock(&tbl->slot_tbl_lock);
> -	nfs4_free_slot(tbl, res->sr_slot - tbl->slots);
> +	nfs4_free_slot(tbl, res->sr_slot->slotid);
>  	nfs4_check_drain_fc_complete(res->sr_session);
>  	spin_unlock(&tbl->slot_tbl_lock);
>  	res->sr_slot = NULL;
> @@ -468,10 +627,8 @@ static int nfs41_sequence_done(struct rpc_task *task, struct nfs4_sequence_res *
>  		 * returned NFS4ERR_DELAY as per Section 2.10.6.2
>  		 * of RFC5661.
>  		 */
> -		dprintk("%s: slot=%td seq=%d: Operation in progress\n",
> -			__func__,
> -			res->sr_slot - res->sr_session->fc_slot_table.slots,
> -			res->sr_slot->seq_nr);
> +		dprintk("%s: slotid=%u seq=%d: Operation in progress\n",
> +			__func__, res->sr_slot->slotid, res->sr_slot->seq_nr);
>  		goto out_retry;
>  	default:
>  		/* Just update the slot sequence no. */
> @@ -479,7 +636,8 @@ static int nfs41_sequence_done(struct rpc_task *task, struct nfs4_sequence_res *
>  	}
>  out:
>  	/* The session may be reset by one of the error handlers. */
> -	dprintk("%s: Error %d free the slot \n", __func__, res->sr_status);
> +	dprintk("%s: Error %d free the slot for task pid %5u\n", __func__,
> +		res->sr_status, task->tk_pid);
>  	nfs41_sequence_free_slot(res);
>  	return 1;
>  out_retry:
> @@ -540,7 +698,7 @@ int nfs41_setup_sequence(struct nfs4_session *session,
>  	struct nfs4_slot_table *tbl;
>  	u8 slotid;
>  
> -	dprintk("--> %s\n", __func__);
> +	dprintk("--> %s task pid %5u\n", __func__, task->tk_pid);
>  	/* slot already allocated? */
>  	if (res->sr_slot != NULL)
>  		return 0;
> @@ -575,12 +733,24 @@ int nfs41_setup_sequence(struct nfs4_session *session,
>  		dprintk("<-- %s: no free slots\n", __func__);
>  		return -EAGAIN;
>  	}
> +	slot = nfs4_lookup_slot_locked(tbl, slotid);
> +	if (slot == NULL) {
> +		/* keep FIFO order */
> +		rpc_task_set_priority(task, RPC_PRIORITY_PRIVILEGED);
> +		rpc_sleep_on(&tbl->slot_tbl_waitq, task, NULL);
> +		nfs4_free_slot(tbl, slotid);

why free the slot here?

Benny

> +		spin_unlock(&tbl->slot_tbl_lock);
> +		dprintk("%s task pid %5u sleeping for dynamic slot %d\n",
> +			__func__, task->tk_pid, slotid);
> +		/* rpc_wakeup_next called upon success by slot allocator */
> +		nfs4_dynamic_alloc_slot(tbl, slotid);
> +		return -EAGAIN;
> +	}
>  	spin_unlock(&tbl->slot_tbl_lock);
>  
>  	rpc_task_set_priority(task, RPC_PRIORITY_NORMAL);
> -	slot = tbl->slots + slotid;
>  	args->sa_session = session;
> -	args->sa_slotid = slotid;
> +	args->sa_slot = slot;
>  	args->sa_cache_this = cache_reply;
>  
>  	dprintk("<-- %s slotid=%d seqid=%d\n", __func__, slotid, slot->seq_nr);
> @@ -613,9 +783,9 @@ int nfs4_setup_sequence(const struct nfs_server *server,
>  		goto out;
>  	}
>  
> -	dprintk("--> %s clp %p session %p sr_slot %td\n",
> +	dprintk("--> %s clp %p session %p sr_slotid %d\n",
>  		__func__, session->clp, session, res->sr_slot ?
> -			res->sr_slot - session->fc_slot_table.slots : -1);
> +			res->sr_slot->slotid : -1);
>  
>  	ret = nfs41_setup_sequence(session, args, res, cache_reply,
>  				   task);
> @@ -4976,84 +5146,64 @@ int nfs4_proc_get_lease_time(struct nfs_client *clp, struct nfs_fsinfo *fsinfo)
>  }
>  
>  /*
> - * Reset a slot table
> + * Reset a slot table. Session has been drained and reset with new slot table
> + * parameters from create_session.
> + * Keep all allocated slots that fit into new slot table max_reqs parameter.
>   */
> -static int nfs4_reset_slot_table(struct nfs4_slot_table *tbl, u32 max_reqs,
> +static void nfs4_reset_slot_table(struct nfs4_slot_table *tbl, u32 new_max_reqs,
>  				 int ivalue)
>  {
> -	struct nfs4_slot *new = NULL;
> +	struct nfs4_slot *sp;
> +	struct hlist_node *n;
>  	int i;
> -	int ret = 0;
> -
> -	dprintk("--> %s: max_reqs=%u, tbl->max_slots %d\n", __func__,
> -		max_reqs, tbl->max_slots);
>  
> -	/* Does the newly negotiated max_reqs match the existing slot table? */
> -	if (max_reqs != tbl->max_slots) {
> -		ret = -ENOMEM;
> -		new = kmalloc(max_reqs * sizeof(struct nfs4_slot),
> -			      GFP_NOFS);
> -		if (!new)
> -			goto out;
> -		ret = 0;
> -		kfree(tbl->slots);
> -	}
>  	spin_lock(&tbl->slot_tbl_lock);
> -	if (new) {
> -		tbl->slots = new;
> -		tbl->max_slots = max_reqs;
> +	tbl->max_slots = new_max_reqs;
> +	nfs4_reduce_slots_locked(tbl);
> +	tbl->highest_used_slotid = -1;
> +
> +	for (i = 0; i < SLOT_TABLE_SIZE; i++) {
> +		hlist_for_each_entry(sp, n, &tbl->slots[i], slot_node)
> +			sp->seq_nr = ivalue;
>  	}
> -	for (i = 0; i < tbl->max_slots; ++i)
> -		tbl->slots[i].seq_nr = ivalue;
>  	spin_unlock(&tbl->slot_tbl_lock);
> -	dprintk("%s: tbl=%p slots=%p max_slots=%d\n", __func__,
> -		tbl, tbl->slots, tbl->max_slots);
> -out:
> -	dprintk("<-- %s: return %d\n", __func__, ret);
> -	return ret;
> +	dprintk("%s: tbl->max_slots=%d tbl->allocated_slots=%d\n",
> +		__func__, tbl->max_slots, tbl->allocated_slots);
> +	return;
>  }
>  
>  /* Destroy the slot table */
>  static void nfs4_destroy_slot_tables(struct nfs4_session *session)
>  {
> -	if (session->fc_slot_table.slots != NULL) {
> -		kfree(session->fc_slot_table.slots);
> -		session->fc_slot_table.slots = NULL;
> -	}
> -	if (session->bc_slot_table.slots != NULL) {
> -		kfree(session->bc_slot_table.slots);
> -		session->bc_slot_table.slots = NULL;
> -	}
> -	return;
> +	dprintk("--> %s\n", __func__);
> +
> +	nfs4_free_all_slots(&session->fc_slot_table);
> +	nfs4_free_all_slots(&session->bc_slot_table);
>  }
>  
>  /*
>   * Initialize slot table
>   */
>  static int nfs4_init_slot_table(struct nfs4_slot_table *tbl,
> -		int max_slots, int ivalue)
> +		int max_slots, int num_prealloc, int ivalue)
>  {
> -	struct nfs4_slot *slot;
> -	int ret = -ENOMEM;
> +	int ret;
>  
>  	BUG_ON(max_slots > NFS4_MAX_SLOT_TABLE);
> -
> -	dprintk("--> %s: max_reqs=%u\n", __func__, max_slots);
> -
> -	slot = kcalloc(max_slots, sizeof(struct nfs4_slot), GFP_NOFS);
> -	if (!slot)
> +	ret = nfs4_prealloc_slots(tbl, num_prealloc, ivalue);
> +	if (ret) {
> +		nfs4_free_all_slots(tbl);
>  		goto out;
> +	}
>  	ret = 0;
>  
>  	spin_lock(&tbl->slot_tbl_lock);
>  	tbl->max_slots = max_slots;
> -	tbl->slots = slot;
>  	tbl->highest_used_slotid = -1;  /* no slot is currently used */
>  	spin_unlock(&tbl->slot_tbl_lock);
> -	dprintk("%s: tbl=%p slots=%p max_slots=%d\n", __func__,
> -		tbl, tbl->slots, tbl->max_slots);
> +	dprintk("%s: tbl=%p max_slots=%d, tbl->allocated_slots=%d\n", __func__,
> +		tbl, tbl->max_slots, tbl->allocated_slots);
>  out:
> -	dprintk("<-- %s: return %d\n", __func__, ret);
>  	return ret;
>  }
>  
> @@ -5063,30 +5213,34 @@ out:
>  static int nfs4_setup_session_slot_tables(struct nfs4_session *ses)
>  {
>  	struct nfs4_slot_table *tbl;
> -	int status;
> +	int status = 0;
>  
>  	dprintk("--> %s\n", __func__);
>  	/* Fore channel */
>  	tbl = &ses->fc_slot_table;
> -	if (tbl->slots == NULL) {
> -		status = nfs4_init_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
> +	if (tbl->allocated_slots == 0) {
> +		status = nfs4_init_slot_table(tbl, ses->fc_attrs.max_reqs,
> +				ses->clp->cl_rpcclient->cl_xprt->min_reqs, 1);
>  		if (status) /* -ENOMEM */
>  			return status;
>  	} else {
> -		status = nfs4_reset_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
> -		if (status)
> -			return status;
> +		nfs4_reset_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
>  	}
> +
>  	/* Back channel */
>  	tbl = &ses->bc_slot_table;
> -	if (tbl->slots == NULL) {
> -		status = nfs4_init_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
> +	if (tbl->allocated_slots == 0) {
> +		status = nfs4_init_slot_table(tbl, ses->bc_attrs.max_reqs,
> +				/* Backchannel max_reqs == 1 */
> +				ses->bc_attrs.max_reqs, 0);
>  		if (status)
>  			/* Fore and back channel share a connection so get
>  			 * both slot tables or neither */
>  			nfs4_destroy_slot_tables(ses);
> -	} else
> -		status = nfs4_reset_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
> +	} else {
> +		nfs4_reset_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
> +	}
> +
>  	return status;
>  }
>  
> @@ -5276,7 +5430,6 @@ int nfs4_proc_create_session(struct nfs_client *clp)
>  
>  	/* Init or reset the session slot tables */
>  	status = nfs4_setup_session_slot_tables(session);
> -	dprintk("slot table setup returned %d\n", status);
>  	if (status)
>  		goto out;
>  
> @@ -5284,7 +5437,7 @@ int nfs4_proc_create_session(struct nfs_client *clp)
>  	dprintk("%s client>seqid %d sessionid %u:%u:%u:%u\n", __func__,
>  		clp->cl_seqid, ptr[0], ptr[1], ptr[2], ptr[3]);
>  out:
> -	dprintk("<-- %s\n", __func__);
> +	dprintk("<-- %s status %d\n", __func__, status);
>  	return status;
>  }
>  
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index 39914be..8260865 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -1573,26 +1573,16 @@ static int nfs4_recall_slot(struct nfs_client *clp)
>  {
>  	struct nfs4_slot_table *fc_tbl = &clp->cl_session->fc_slot_table;
>  	struct nfs4_channel_attrs *fc_attrs = &clp->cl_session->fc_attrs;
> -	struct nfs4_slot *new, *old;
> -	int i;
>  
>  	nfs4_begin_drain_session(clp);
> -	new = kmalloc(fc_tbl->target_max_slots * sizeof(struct nfs4_slot),
> -		      GFP_NOFS);
> -        if (!new)
> -		return -ENOMEM;
>  
>  	spin_lock(&fc_tbl->slot_tbl_lock);
> -	for (i = 0; i < fc_tbl->target_max_slots; i++)
> -		new[i].seq_nr = fc_tbl->slots[i].seq_nr;
> -	old = fc_tbl->slots;
> -	fc_tbl->slots = new;
>  	fc_tbl->max_slots = fc_tbl->target_max_slots;
> +	nfs4_reduce_slots_locked(fc_tbl);
>  	fc_tbl->target_max_slots = 0;
>  	fc_attrs->max_reqs = fc_tbl->max_slots;
>  	spin_unlock(&fc_tbl->slot_tbl_lock);
>  
> -	kfree(old);
>  	nfs4_end_drain_session(clp);
>  	return 0;
>  }
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index e6161b2..c42ec29 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -1872,7 +1872,6 @@ static void encode_sequence(struct xdr_stream *xdr,
>  #if defined(CONFIG_NFS_V4_1)
>  	struct nfs4_session *session = args->sa_session;
>  	struct nfs4_slot_table *tp;
> -	struct nfs4_slot *slot;
>  	__be32 *p;
>  
>  	if (!session)
> @@ -1880,8 +1879,7 @@ static void encode_sequence(struct xdr_stream *xdr,
>  
>  	tp = &session->fc_slot_table;
>  
> -	WARN_ON(args->sa_slotid == NFS4_MAX_SLOT_TABLE);
> -	slot = tp->slots + args->sa_slotid;
> +	WARN_ON(args->sa_slot->slotid == NFS4_MAX_SLOT_TABLE);
>  
>  	p = reserve_space(xdr, 4 + NFS4_MAX_SESSIONID_LEN + 16);
>  	*p++ = cpu_to_be32(OP_SEQUENCE);
> @@ -1896,11 +1894,11 @@ static void encode_sequence(struct xdr_stream *xdr,
>  		((u32 *)session->sess_id.data)[1],
>  		((u32 *)session->sess_id.data)[2],
>  		((u32 *)session->sess_id.data)[3],
> -		slot->seq_nr, args->sa_slotid,
> +		args->sa_slot->seq_nr, args->sa_slot->slotid,
>  		tp->highest_used_slotid, args->sa_cache_this);
>  	p = xdr_encode_opaque_fixed(p, session->sess_id.data, NFS4_MAX_SESSIONID_LEN);
> -	*p++ = cpu_to_be32(slot->seq_nr);
> -	*p++ = cpu_to_be32(args->sa_slotid);
> +	*p++ = cpu_to_be32(args->sa_slot->seq_nr);
> +	*p++ = cpu_to_be32(args->sa_slot->slotid);
>  	*p++ = cpu_to_be32(tp->highest_used_slotid);
>  	*p = cpu_to_be32(args->sa_cache_this);
>  	hdr->nops++;
> @@ -5361,13 +5359,17 @@ static int decode_sequence(struct xdr_stream *xdr,
>  	/* seqid */
>  	dummy = be32_to_cpup(p++);
>  	if (dummy != res->sr_slot->seq_nr) {
> -		dprintk("%s Invalid sequence number\n", __func__);
> +		dprintk("%s Invalid seq num %d for [slotid:seq_nr] [%d:%d]\n",
> +			__func__, dummy, res->sr_slot->slotid,
> +			res->sr_slot->seq_nr);
>  		goto out_err;
>  	}
>  	/* slot id */
>  	dummy = be32_to_cpup(p++);
> -	if (dummy != res->sr_slot - res->sr_session->fc_slot_table.slots) {
> -		dprintk("%s Invalid slot id\n", __func__);
> +	if (dummy != res->sr_slot->slotid) {
> +		dprintk("%s Invalid slot id %d for [slotid:seq_nr] [%d:%d]\n",
> +			__func__, dummy, res->sr_slot->slotid,
> +			res->sr_slot->seq_nr);
>  		goto out_err;
>  	}
>  	/* highest slot id - currently not processed */
> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
> index b5479df..90c6e2b 100644
> --- a/include/linux/nfs_fs_sb.h
> +++ b/include/linux/nfs_fs_sb.h
> @@ -193,12 +193,15 @@ struct nfs_server {
>  
>  /* Sessions */
>  #define SLOT_TABLE_SZ (NFS4_MAX_SLOT_TABLE/(8*sizeof(long)))
> +#define SLOT_TABLE_BITS                5
> +#define SLOT_TABLE_SIZE                (1 << SLOT_TABLE_BITS)
>  struct nfs4_slot_table {
> -	struct nfs4_slot *slots;		/* seqid per slot */
> +	struct hlist_head slots[SLOT_TABLE_SIZE];
>  	unsigned long   used_slots[SLOT_TABLE_SZ]; /* used/unused bitmap */
>  	spinlock_t	slot_tbl_lock;
>  	struct rpc_wait_queue	slot_tbl_waitq;	/* allocators may wait here */
> -	int		max_slots;		/* # slots in table */
> +	int		allocated_slots;	/* # slots in table */
> +	int		max_slots;		/* max # slots in table */
>  	int		highest_used_slotid;	/* sent to server on each SEQ.
>  						 * op for dynamic resizing */
>  	int		target_max_slots;	/* Set by CB_RECALL_SLOT as
> @@ -206,11 +209,6 @@ struct nfs4_slot_table {
>  	struct completion complete;
>  };
>  
> -static inline int slot_idx(struct nfs4_slot_table *tbl, struct nfs4_slot *sp)
> -{
> -	return sp - tbl->slots;
> -}
> -
>  /*
>   * Session related parameters
>   */
> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> index c74595b..76b27a4 100644
> --- a/include/linux/nfs_xdr.h
> +++ b/include/linux/nfs_xdr.h
> @@ -168,12 +168,14 @@ struct nfs4_channel_attrs {
>  
>  /* nfs41 sessions slot seqid */
>  struct nfs4_slot {
> +	struct hlist_node	slot_node;
>  	u32		 	seq_nr;
> +	u8			slotid;
>  };
>  
>  struct nfs4_sequence_args {
>  	struct nfs4_session	*sa_session;
> -	u8			sa_slotid;
> +	struct nfs4_slot        *sa_slot;
>  	u8			sa_cache_this;
>  };
>  

Re: [PATCH Version 4 4/7] NFSv4.1: dynamic session slots

[Adamson, Andy]

On Dec 11, 2011, at 8:14 AM, Benny Halevy wrote:

> Hi Andy,
> 
> I have some minor-ish comments inline below, but the whole
> hlist concept looks to me like it might incur pretty high overhead
> in cpu and memory caching.

I'd like to measure the difference. What is the recommended way?

> 
> Have you considered simply reallocating the slot table on grow
> and maybe on shrink below a certain threshold?

Because the slot pointer is placed in the response, we would have to drain the slot table to grow/shrink in this manner.

-->Andy

> 
> On 2011-11-09 20:58, andros@netapp.com wrote:
>> From: Andy Adamson <andros@netapp.com>
>> 
>> The session fore/backchannel slot tables implementation is changed from a
>> static array to an hlist hashed on slotid. Slotid's are values 0..N where N
>> is initially negotiated between the client and server via the CREATE_SESSION
>> call, and can be dynamically changed by the server during an active session.
>> 
>> When there are no allocated slots (mount), an initial number of session slots
>> is allocated equal to the inital number of dynamic rpc_slots
>> created upon transport creation (xprt->min_reqs).  Slots are then dynamically
>> allocated as needed upto the CREATE_SESSION negotiated max_reqs value.
>> 
>> For session reset the session is drained (inactive). The (old) session has
>> allocated slot tables which we will reuse. nfs4_reduce_slots_locked is called
>> and only reduces the allocated_slots if the new max_reqs negotiated by the
>> reset CREATE_SESSION is less than the number of allocated_slots. The resultant
>> slot table is then re-initialized.
>> 
>> CB_SLOT_RECALL is changed only in that it now shares code with session
>> reset - it also calls nfs4_reduce_slots_locked.
>> 
>> The backchannel uses the new dynamic slot allocator, even though we only
>> currently support a single back channel slot.
>> 
>> Signed-off-by: Andy Adamson <andros@netapp.com>
>> ---
>> fs/nfs/callback_proc.c    |   25 +++-
>> fs/nfs/internal.h         |    2 +
>> fs/nfs/nfs4_fs.h          |    1 +
>> fs/nfs/nfs4proc.c         |  297 ++++++++++++++++++++++++++++++++++-----------
>> fs/nfs/nfs4state.c        |   12 +--
>> fs/nfs/nfs4xdr.c          |   20 ++--
>> include/linux/nfs_fs_sb.h |   12 +-
>> include/linux/nfs_xdr.h   |    4 +-
>> 8 files changed, 266 insertions(+), 107 deletions(-)
>> 
>> diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
>> index 54cea8a..77cc96c 100644
>> --- a/fs/nfs/callback_proc.c
>> +++ b/fs/nfs/callback_proc.c
>> @@ -342,7 +342,7 @@ validate_seqid(struct nfs4_slot_table *tbl, struct cb_sequenceargs * args)
>> 	if (args->csa_slotid >= NFS41_BC_MAX_CALLBACKS)
>> 		return htonl(NFS4ERR_BADSLOT);
>> 
>> -	slot = tbl->slots + args->csa_slotid;
>> +	slot = nfs4_lookup_slot_locked(tbl, args->csa_slotid);
>> 	dprintk("%s slot table seqid: %d\n", __func__, slot->seq_nr);
>> 
>> 	/* Normal */
>> @@ -377,6 +377,22 @@ out_ok:
>> 	return htonl(NFS4_OK);
>> }
>> 
>> +static bool test_slot_referring(struct nfs4_slot_table *tbl,
>> +				struct referring_call *ref)
>> +{
>> +	struct nfs4_slot *slot;
>> +	bool status = false;
>> +
>> +	spin_lock(&tbl->slot_tbl_lock);
>> +	if (test_bit(ref->rc_slotid, tbl->used_slots)) {
>> +		slot = nfs4_lookup_slot_locked(tbl, ref->rc_slotid);
>> +		if (slot->seq_nr == ref->rc_sequenceid)
>> +			status = true;
>> +	}
>> +	spin_unlock(&tbl->slot_tbl_lock);
>> +	return status;
>> +}
>> +
>> /*
>>  * For each referring call triple, check the session's slot table for
>>  * a match.  If the slot is in use and the sequence numbers match, the
>> @@ -417,12 +433,7 @@ static bool referring_call_exists(struct nfs_client *clp,
>> 				((u32 *)&rclist->rcl_sessionid.data)[2],
>> 				((u32 *)&rclist->rcl_sessionid.data)[3],
>> 				ref->rc_sequenceid, ref->rc_slotid);
>> -
>> -			spin_lock(&tbl->slot_tbl_lock);
>> -			status = (test_bit(ref->rc_slotid, tbl->used_slots) &&
>> -				  tbl->slots[ref->rc_slotid].seq_nr ==
>> -					ref->rc_sequenceid);
>> -			spin_unlock(&tbl->slot_tbl_lock);
>> +			status = test_slot_referring(tbl, ref);
>> 			if (status)
>> 				goto out;
>> 		}
>> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
>> index c1a1bd8..4451abf 100644
>> --- a/fs/nfs/internal.h
>> +++ b/fs/nfs/internal.h
>> @@ -354,6 +354,8 @@ extern int _nfs4_call_sync_session(struct rpc_clnt *clnt,
>> 				   struct nfs4_sequence_args *args,
>> 				   struct nfs4_sequence_res *res,
>> 				   int cache_reply);
>> +struct nfs4_slot *nfs4_lookup_slot_locked(struct nfs4_slot_table *tbl,
>> +					  u8 slotid);
>> 
>> /*
>>  * Determine the device name as a string
>> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
>> index 693ae22..ff87169 100644
>> --- a/fs/nfs/nfs4_fs.h
>> +++ b/fs/nfs/nfs4_fs.h
>> @@ -242,6 +242,7 @@ extern struct nfs4_session *nfs4_alloc_session(struct nfs_client *clp);
>> extern int nfs4_proc_create_session(struct nfs_client *);
>> extern int nfs4_proc_destroy_session(struct nfs4_session *);
>> extern int nfs4_init_session(struct nfs_server *server);
>> +extern void nfs4_reduce_slots_locked(struct nfs4_slot_table *tbl);
>> extern int nfs4_proc_get_lease_time(struct nfs_client *clp,
>> 		struct nfs_fsinfo *fsinfo);
>> extern int nfs4_proc_layoutcommit(struct nfs4_layoutcommit_data *data,
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index 2638291..e3a7663 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -346,6 +346,162 @@ static void renew_lease(const struct nfs_server *server, unsigned long timestamp
>> #if defined(CONFIG_NFS_V4_1)
>> 
>> /*
>> + * Session dynamic slot table helper functions
>> + */
>> +
>> +static inline u32 slot_tbl_hash(const u8 slotid)
> 
> nit: "const" not really needed here...
> 
>> +{
>> +	return (u32)slotid % SLOT_TABLE_SIZE;
>> +}
>> +
>> +static void nfs4_init_slot(struct nfs4_slot *new, u8 slotid, int ivalue)
>> +{
>> +	INIT_HLIST_NODE(&new->slot_node);
>> +	new->slotid = slotid;
>> +	new->seq_nr = ivalue;
>> +}
>> +
>> +static void nfs4_insert_slot_locked(struct nfs4_slot_table *tbl,
>> +				    struct nfs4_slot *new)
>> +{
>> +	u32 hash = slot_tbl_hash(new->slotid);
>> +
>> +	hlist_add_head(&new->slot_node, &tbl->slots[hash]);
>> +	tbl->allocated_slots++;
>> +}
>> +
>> +/*
>> + * Allocate a slot for the next slotid (tbl->allocated_slotid)
>> + */
>> +static int nfs4_alloc_add_slot(struct nfs4_slot_table *tbl,
>> +			       int ivalue, int slotid,
>> +			       gfp_t gfp_flags)
>> +{
>> +	struct nfs4_slot *new;
>> +	struct rpc_task *task;
>> +
>> +	dprintk("--> %s slotid=%u tbl->allocated_slots=%d\n", __func__,
>> +		slotid, tbl->allocated_slots);
>> +	new = kzalloc(sizeof(struct nfs4_slot), gfp_flags);
>> +	if (!new)
>> +		return -ENOMEM;
>> +	/* tbl->allocated_slots holds the next unallocated slotid */
>> +	nfs4_init_slot(new, slotid, ivalue);
>> +	spin_lock(&tbl->slot_tbl_lock);
>> +	if (tbl->allocated_slots != slotid) {
> 
> This condition doesn't seem to be reliable.
> What if the slot table shrunk?
> 
>> +		spin_unlock(&tbl->slot_tbl_lock);
>> +		kfree(new);
>> +		dprintk("%s slotid %u already allocated\n", __func__,
>> +			slotid);
>> +		return 0;
>> +	}
>> +	nfs4_insert_slot_locked(tbl, new);
>> +
>> +	/* This only applies to dynamic allocation */
>> +	task = rpc_wake_up_next(&tbl->slot_tbl_waitq);
>> +	if (task)
>> +		dprintk("%s woke up task %5u\n", __func__, task->tk_pid);
>> +	spin_unlock(&tbl->slot_tbl_lock);
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Allocate the minimal number of slots required by the transport.
>> + * Called at session creation.
>> + */
>> +static int nfs4_prealloc_slots(struct nfs4_slot_table *tbl, int num, int ivalue)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < num; i++) {
>> +		if (nfs4_alloc_add_slot(tbl, ivalue, tbl->allocated_slots,
>> +					GFP_NOFS) != 0)
>> +			return -ENOMEM;
> 
> Returning nfs4_alloc_add_slot's status would be more appropriate.
> How about also freeing 0 to i-1 in the error case?
> 
>> +	}
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Allocate the next slotid. If allocation fails, next request will retry.
>> + */
>> +static void nfs4_dynamic_alloc_slot(struct nfs4_slot_table *tbl, u8 slotid)
>> +{
>> +	if (slotid != tbl->allocated_slots) {
>> +		dprintk("%s slotid %u already allocated\n", __func__, slotid);
>> +		return;
>> +	}
> 
> This is checked again under the lock in nfs4_alloc_add_slot, no?
> so if it is not supposed to be a common case (which I don't believe
> it is) I'd just forget about this optimization and call the latter
> directly.
> 
>> +	nfs4_alloc_add_slot(tbl, 1, slotid, GFP_NOWAIT);
>> +}
>> +
>> +/*
>> + * Return the slot associated with the slotid returned by nfs4_find_slot.
>> + * If the slot is not found, it has not been allocated.
>> + */
>> +struct nfs4_slot *
>> +nfs4_lookup_slot_locked(struct nfs4_slot_table *tbl, u8 slotid)
>> +{
>> +	struct nfs4_slot *sp;
>> +	struct hlist_node *n;
>> +	u32 hash = slot_tbl_hash(slotid);
>> +
>> +	hlist_for_each_entry(sp, n, &tbl->slots[hash], slot_node) {
>> +		if (sp->slotid == slotid)
>> +			return sp;
>> +	}
>> +	return NULL;
>> +}
>> +
>> +static void nfs4_remove_slot_locked(struct nfs4_slot_table *tbl,
>> +				    struct nfs4_slot *slot)
>> +{
>> +	dprintk("--> %s slotid %d\n", __func__, slot->slotid);
>> +	hlist_del_init(&slot->slot_node);
>> +	tbl->allocated_slots--;
>> +	kfree(slot);
>> +}
>> +
>> +static void nfs4_free_all_slots(struct nfs4_slot_table *tbl)
>> +{
>> +	struct nfs4_slot *slotp;
>> +	int i;
>> +
>> +	spin_lock(&tbl->slot_tbl_lock);
>> +	for (i = 0; i < SLOT_TABLE_SIZE; i++) {
>> +		while (!hlist_empty(&tbl->slots[i])) {
>> +			slotp = hlist_entry(tbl->slots[i].first,
>> +					    struct nfs4_slot, slot_node);
>> +			nfs4_remove_slot_locked(tbl, slotp);
>> +		}
>> +		/* re-initialize hlist_head */
>> +		tbl->slots[i].first = NULL;
>> +	}
>> +	spin_unlock(&tbl->slot_tbl_lock);
>> +}
>> +
>> +/*
>> + * Remove num_dec number of contiguous slotids starting from highest
>> + * allocated slotid
>> + *
>> + * Note: set the new tbl->max_slots prior to calling.
>> + */
>> +void nfs4_reduce_slots_locked(struct nfs4_slot_table *tbl)
>> +{
>> +	struct nfs4_slot *removeme;
>> +	int i, rm_slotid = tbl->allocated_slots - 1;
>> +	int num_dec = tbl->allocated_slots - tbl->max_slots;
>> +
>> +	/* Reset of the slot table can make num_dec <= 0 */
>> +	if (num_dec <= 0)
>> +		return;
>> +	for (i = 0; i < num_dec; i++) {
>> +		removeme = nfs4_lookup_slot_locked(tbl, rm_slotid);
>> +		BUG_ON(removeme == NULL);
>> +		nfs4_remove_slot_locked(tbl, removeme);
>> +		rm_slotid--;
>> +	}
>> +}
>> +
>> +/*
>>  * nfs4_free_slot - free a slot and efficiently update slot table.
>>  *
>>  * freeing a slot is trivially done by clearing its respective bit
>> @@ -390,8 +546,11 @@ static void nfs4_check_drain_fc_complete(struct nfs4_session *ses)
>> 
>> 	if (!test_bit(NFS4_SESSION_DRAINING, &ses->session_state)) {
>> 		task = rpc_wake_up_next(&ses->fc_slot_table.slot_tbl_waitq);
>> -		if (task)
>> +		if (task) {
>> 			rpc_task_set_priority(task, RPC_PRIORITY_PRIVILEGED);
>> +			dprintk("%s woke up task pid %5u\n", __func__,
>> +				task->tk_pid);
>> +		}
>> 		return;
>> 	}
>> 
>> @@ -427,7 +586,7 @@ static void nfs41_sequence_free_slot(struct nfs4_sequence_res *res)
>> 	}
>> 
>> 	spin_lock(&tbl->slot_tbl_lock);
>> -	nfs4_free_slot(tbl, res->sr_slot - tbl->slots);
>> +	nfs4_free_slot(tbl, res->sr_slot->slotid);
>> 	nfs4_check_drain_fc_complete(res->sr_session);
>> 	spin_unlock(&tbl->slot_tbl_lock);
>> 	res->sr_slot = NULL;
>> @@ -468,10 +627,8 @@ static int nfs41_sequence_done(struct rpc_task *task, struct nfs4_sequence_res *
>> 		 * returned NFS4ERR_DELAY as per Section 2.10.6.2
>> 		 * of RFC5661.
>> 		 */
>> -		dprintk("%s: slot=%td seq=%d: Operation in progress\n",
>> -			__func__,
>> -			res->sr_slot - res->sr_session->fc_slot_table.slots,
>> -			res->sr_slot->seq_nr);
>> +		dprintk("%s: slotid=%u seq=%d: Operation in progress\n",
>> +			__func__, res->sr_slot->slotid, res->sr_slot->seq_nr);
>> 		goto out_retry;
>> 	default:
>> 		/* Just update the slot sequence no. */
>> @@ -479,7 +636,8 @@ static int nfs41_sequence_done(struct rpc_task *task, struct nfs4_sequence_res *
>> 	}
>> out:
>> 	/* The session may be reset by one of the error handlers. */
>> -	dprintk("%s: Error %d free the slot \n", __func__, res->sr_status);
>> +	dprintk("%s: Error %d free the slot for task pid %5u\n", __func__,
>> +		res->sr_status, task->tk_pid);
>> 	nfs41_sequence_free_slot(res);
>> 	return 1;
>> out_retry:
>> @@ -540,7 +698,7 @@ int nfs41_setup_sequence(struct nfs4_session *session,
>> 	struct nfs4_slot_table *tbl;
>> 	u8 slotid;
>> 
>> -	dprintk("--> %s\n", __func__);
>> +	dprintk("--> %s task pid %5u\n", __func__, task->tk_pid);
>> 	/* slot already allocated? */
>> 	if (res->sr_slot != NULL)
>> 		return 0;
>> @@ -575,12 +733,24 @@ int nfs41_setup_sequence(struct nfs4_session *session,
>> 		dprintk("<-- %s: no free slots\n", __func__);
>> 		return -EAGAIN;
>> 	}
>> +	slot = nfs4_lookup_slot_locked(tbl, slotid);
>> +	if (slot == NULL) {
>> +		/* keep FIFO order */
>> +		rpc_task_set_priority(task, RPC_PRIORITY_PRIVILEGED);
>> +		rpc_sleep_on(&tbl->slot_tbl_waitq, task, NULL);
>> +		nfs4_free_slot(tbl, slotid);
> 
> why free the slot here?
> 
> Benny
> 
>> +		spin_unlock(&tbl->slot_tbl_lock);
>> +		dprintk("%s task pid %5u sleeping for dynamic slot %d\n",
>> +			__func__, task->tk_pid, slotid);
>> +		/* rpc_wakeup_next called upon success by slot allocator */
>> +		nfs4_dynamic_alloc_slot(tbl, slotid);
>> +		return -EAGAIN;
>> +	}
>> 	spin_unlock(&tbl->slot_tbl_lock);
>> 
>> 	rpc_task_set_priority(task, RPC_PRIORITY_NORMAL);
>> -	slot = tbl->slots + slotid;
>> 	args->sa_session = session;
>> -	args->sa_slotid = slotid;
>> +	args->sa_slot = slot;
>> 	args->sa_cache_this = cache_reply;
>> 
>> 	dprintk("<-- %s slotid=%d seqid=%d\n", __func__, slotid, slot->seq_nr);
>> @@ -613,9 +783,9 @@ int nfs4_setup_sequence(const struct nfs_server *server,
>> 		goto out;
>> 	}
>> 
>> -	dprintk("--> %s clp %p session %p sr_slot %td\n",
>> +	dprintk("--> %s clp %p session %p sr_slotid %d\n",
>> 		__func__, session->clp, session, res->sr_slot ?
>> -			res->sr_slot - session->fc_slot_table.slots : -1);
>> +			res->sr_slot->slotid : -1);
>> 
>> 	ret = nfs41_setup_sequence(session, args, res, cache_reply,
>> 				   task);
>> @@ -4976,84 +5146,64 @@ int nfs4_proc_get_lease_time(struct nfs_client *clp, struct nfs_fsinfo *fsinfo)
>> }
>> 
>> /*
>> - * Reset a slot table
>> + * Reset a slot table. Session has been drained and reset with new slot table
>> + * parameters from create_session.
>> + * Keep all allocated slots that fit into new slot table max_reqs parameter.
>>  */
>> -static int nfs4_reset_slot_table(struct nfs4_slot_table *tbl, u32 max_reqs,
>> +static void nfs4_reset_slot_table(struct nfs4_slot_table *tbl, u32 new_max_reqs,
>> 				 int ivalue)
>> {
>> -	struct nfs4_slot *new = NULL;
>> +	struct nfs4_slot *sp;
>> +	struct hlist_node *n;
>> 	int i;
>> -	int ret = 0;
>> -
>> -	dprintk("--> %s: max_reqs=%u, tbl->max_slots %d\n", __func__,
>> -		max_reqs, tbl->max_slots);
>> 
>> -	/* Does the newly negotiated max_reqs match the existing slot table? */
>> -	if (max_reqs != tbl->max_slots) {
>> -		ret = -ENOMEM;
>> -		new = kmalloc(max_reqs * sizeof(struct nfs4_slot),
>> -			      GFP_NOFS);
>> -		if (!new)
>> -			goto out;
>> -		ret = 0;
>> -		kfree(tbl->slots);
>> -	}
>> 	spin_lock(&tbl->slot_tbl_lock);
>> -	if (new) {
>> -		tbl->slots = new;
>> -		tbl->max_slots = max_reqs;
>> +	tbl->max_slots = new_max_reqs;
>> +	nfs4_reduce_slots_locked(tbl);
>> +	tbl->highest_used_slotid = -1;
>> +
>> +	for (i = 0; i < SLOT_TABLE_SIZE; i++) {
>> +		hlist_for_each_entry(sp, n, &tbl->slots[i], slot_node)
>> +			sp->seq_nr = ivalue;
>> 	}
>> -	for (i = 0; i < tbl->max_slots; ++i)
>> -		tbl->slots[i].seq_nr = ivalue;
>> 	spin_unlock(&tbl->slot_tbl_lock);
>> -	dprintk("%s: tbl=%p slots=%p max_slots=%d\n", __func__,
>> -		tbl, tbl->slots, tbl->max_slots);
>> -out:
>> -	dprintk("<-- %s: return %d\n", __func__, ret);
>> -	return ret;
>> +	dprintk("%s: tbl->max_slots=%d tbl->allocated_slots=%d\n",
>> +		__func__, tbl->max_slots, tbl->allocated_slots);
>> +	return;
>> }
>> 
>> /* Destroy the slot table */
>> static void nfs4_destroy_slot_tables(struct nfs4_session *session)
>> {
>> -	if (session->fc_slot_table.slots != NULL) {
>> -		kfree(session->fc_slot_table.slots);
>> -		session->fc_slot_table.slots = NULL;
>> -	}
>> -	if (session->bc_slot_table.slots != NULL) {
>> -		kfree(session->bc_slot_table.slots);
>> -		session->bc_slot_table.slots = NULL;
>> -	}
>> -	return;
>> +	dprintk("--> %s\n", __func__);
>> +
>> +	nfs4_free_all_slots(&session->fc_slot_table);
>> +	nfs4_free_all_slots(&session->bc_slot_table);
>> }
>> 
>> /*
>>  * Initialize slot table
>>  */
>> static int nfs4_init_slot_table(struct nfs4_slot_table *tbl,
>> -		int max_slots, int ivalue)
>> +		int max_slots, int num_prealloc, int ivalue)
>> {
>> -	struct nfs4_slot *slot;
>> -	int ret = -ENOMEM;
>> +	int ret;
>> 
>> 	BUG_ON(max_slots > NFS4_MAX_SLOT_TABLE);
>> -
>> -	dprintk("--> %s: max_reqs=%u\n", __func__, max_slots);
>> -
>> -	slot = kcalloc(max_slots, sizeof(struct nfs4_slot), GFP_NOFS);
>> -	if (!slot)
>> +	ret = nfs4_prealloc_slots(tbl, num_prealloc, ivalue);
>> +	if (ret) {
>> +		nfs4_free_all_slots(tbl);
>> 		goto out;
>> +	}
>> 	ret = 0;
>> 
>> 	spin_lock(&tbl->slot_tbl_lock);
>> 	tbl->max_slots = max_slots;
>> -	tbl->slots = slot;
>> 	tbl->highest_used_slotid = -1;  /* no slot is currently used */
>> 	spin_unlock(&tbl->slot_tbl_lock);
>> -	dprintk("%s: tbl=%p slots=%p max_slots=%d\n", __func__,
>> -		tbl, tbl->slots, tbl->max_slots);
>> +	dprintk("%s: tbl=%p max_slots=%d, tbl->allocated_slots=%d\n", __func__,
>> +		tbl, tbl->max_slots, tbl->allocated_slots);
>> out:
>> -	dprintk("<-- %s: return %d\n", __func__, ret);
>> 	return ret;
>> }
>> 
>> @@ -5063,30 +5213,34 @@ out:
>> static int nfs4_setup_session_slot_tables(struct nfs4_session *ses)
>> {
>> 	struct nfs4_slot_table *tbl;
>> -	int status;
>> +	int status = 0;
>> 
>> 	dprintk("--> %s\n", __func__);
>> 	/* Fore channel */
>> 	tbl = &ses->fc_slot_table;
>> -	if (tbl->slots == NULL) {
>> -		status = nfs4_init_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
>> +	if (tbl->allocated_slots == 0) {
>> +		status = nfs4_init_slot_table(tbl, ses->fc_attrs.max_reqs,
>> +				ses->clp->cl_rpcclient->cl_xprt->min_reqs, 1);
>> 		if (status) /* -ENOMEM */
>> 			return status;
>> 	} else {
>> -		status = nfs4_reset_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
>> -		if (status)
>> -			return status;
>> +		nfs4_reset_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
>> 	}
>> +
>> 	/* Back channel */
>> 	tbl = &ses->bc_slot_table;
>> -	if (tbl->slots == NULL) {
>> -		status = nfs4_init_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
>> +	if (tbl->allocated_slots == 0) {
>> +		status = nfs4_init_slot_table(tbl, ses->bc_attrs.max_reqs,
>> +				/* Backchannel max_reqs == 1 */
>> +				ses->bc_attrs.max_reqs, 0);
>> 		if (status)
>> 			/* Fore and back channel share a connection so get
>> 			 * both slot tables or neither */
>> 			nfs4_destroy_slot_tables(ses);
>> -	} else
>> -		status = nfs4_reset_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
>> +	} else {
>> +		nfs4_reset_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
>> +	}
>> +
>> 	return status;
>> }
>> 
>> @@ -5276,7 +5430,6 @@ int nfs4_proc_create_session(struct nfs_client *clp)
>> 
>> 	/* Init or reset the session slot tables */
>> 	status = nfs4_setup_session_slot_tables(session);
>> -	dprintk("slot table setup returned %d\n", status);
>> 	if (status)
>> 		goto out;
>> 
>> @@ -5284,7 +5437,7 @@ int nfs4_proc_create_session(struct nfs_client *clp)
>> 	dprintk("%s client>seqid %d sessionid %u:%u:%u:%u\n", __func__,
>> 		clp->cl_seqid, ptr[0], ptr[1], ptr[2], ptr[3]);
>> out:
>> -	dprintk("<-- %s\n", __func__);
>> +	dprintk("<-- %s status %d\n", __func__, status);
>> 	return status;
>> }
>> 
>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>> index 39914be..8260865 100644
>> --- a/fs/nfs/nfs4state.c
>> +++ b/fs/nfs/nfs4state.c
>> @@ -1573,26 +1573,16 @@ static int nfs4_recall_slot(struct nfs_client *clp)
>> {
>> 	struct nfs4_slot_table *fc_tbl = &clp->cl_session->fc_slot_table;
>> 	struct nfs4_channel_attrs *fc_attrs = &clp->cl_session->fc_attrs;
>> -	struct nfs4_slot *new, *old;
>> -	int i;
>> 
>> 	nfs4_begin_drain_session(clp);
>> -	new = kmalloc(fc_tbl->target_max_slots * sizeof(struct nfs4_slot),
>> -		      GFP_NOFS);
>> -        if (!new)
>> -		return -ENOMEM;
>> 
>> 	spin_lock(&fc_tbl->slot_tbl_lock);
>> -	for (i = 0; i < fc_tbl->target_max_slots; i++)
>> -		new[i].seq_nr = fc_tbl->slots[i].seq_nr;
>> -	old = fc_tbl->slots;
>> -	fc_tbl->slots = new;
>> 	fc_tbl->max_slots = fc_tbl->target_max_slots;
>> +	nfs4_reduce_slots_locked(fc_tbl);
>> 	fc_tbl->target_max_slots = 0;
>> 	fc_attrs->max_reqs = fc_tbl->max_slots;
>> 	spin_unlock(&fc_tbl->slot_tbl_lock);
>> 
>> -	kfree(old);
>> 	nfs4_end_drain_session(clp);
>> 	return 0;
>> }
>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
>> index e6161b2..c42ec29 100644
>> --- a/fs/nfs/nfs4xdr.c
>> +++ b/fs/nfs/nfs4xdr.c
>> @@ -1872,7 +1872,6 @@ static void encode_sequence(struct xdr_stream *xdr,
>> #if defined(CONFIG_NFS_V4_1)
>> 	struct nfs4_session *session = args->sa_session;
>> 	struct nfs4_slot_table *tp;
>> -	struct nfs4_slot *slot;
>> 	__be32 *p;
>> 
>> 	if (!session)
>> @@ -1880,8 +1879,7 @@ static void encode_sequence(struct xdr_stream *xdr,
>> 
>> 	tp = &session->fc_slot_table;
>> 
>> -	WARN_ON(args->sa_slotid == NFS4_MAX_SLOT_TABLE);
>> -	slot = tp->slots + args->sa_slotid;
>> +	WARN_ON(args->sa_slot->slotid == NFS4_MAX_SLOT_TABLE);
>> 
>> 	p = reserve_space(xdr, 4 + NFS4_MAX_SESSIONID_LEN + 16);
>> 	*p++ = cpu_to_be32(OP_SEQUENCE);
>> @@ -1896,11 +1894,11 @@ static void encode_sequence(struct xdr_stream *xdr,
>> 		((u32 *)session->sess_id.data)[1],
>> 		((u32 *)session->sess_id.data)[2],
>> 		((u32 *)session->sess_id.data)[3],
>> -		slot->seq_nr, args->sa_slotid,
>> +		args->sa_slot->seq_nr, args->sa_slot->slotid,
>> 		tp->highest_used_slotid, args->sa_cache_this);
>> 	p = xdr_encode_opaque_fixed(p, session->sess_id.data, NFS4_MAX_SESSIONID_LEN);
>> -	*p++ = cpu_to_be32(slot->seq_nr);
>> -	*p++ = cpu_to_be32(args->sa_slotid);
>> +	*p++ = cpu_to_be32(args->sa_slot->seq_nr);
>> +	*p++ = cpu_to_be32(args->sa_slot->slotid);
>> 	*p++ = cpu_to_be32(tp->highest_used_slotid);
>> 	*p = cpu_to_be32(args->sa_cache_this);
>> 	hdr->nops++;
>> @@ -5361,13 +5359,17 @@ static int decode_sequence(struct xdr_stream *xdr,
>> 	/* seqid */
>> 	dummy = be32_to_cpup(p++);
>> 	if (dummy != res->sr_slot->seq_nr) {
>> -		dprintk("%s Invalid sequence number\n", __func__);
>> +		dprintk("%s Invalid seq num %d for [slotid:seq_nr] [%d:%d]\n",
>> +			__func__, dummy, res->sr_slot->slotid,
>> +			res->sr_slot->seq_nr);
>> 		goto out_err;
>> 	}
>> 	/* slot id */
>> 	dummy = be32_to_cpup(p++);
>> -	if (dummy != res->sr_slot - res->sr_session->fc_slot_table.slots) {
>> -		dprintk("%s Invalid slot id\n", __func__);
>> +	if (dummy != res->sr_slot->slotid) {
>> +		dprintk("%s Invalid slot id %d for [slotid:seq_nr] [%d:%d]\n",
>> +			__func__, dummy, res->sr_slot->slotid,
>> +			res->sr_slot->seq_nr);
>> 		goto out_err;
>> 	}
>> 	/* highest slot id - currently not processed */
>> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
>> index b5479df..90c6e2b 100644
>> --- a/include/linux/nfs_fs_sb.h
>> +++ b/include/linux/nfs_fs_sb.h
>> @@ -193,12 +193,15 @@ struct nfs_server {
>> 
>> /* Sessions */
>> #define SLOT_TABLE_SZ (NFS4_MAX_SLOT_TABLE/(8*sizeof(long)))
>> +#define SLOT_TABLE_BITS                5
>> +#define SLOT_TABLE_SIZE                (1 << SLOT_TABLE_BITS)
>> struct nfs4_slot_table {
>> -	struct nfs4_slot *slots;		/* seqid per slot */
>> +	struct hlist_head slots[SLOT_TABLE_SIZE];
>> 	unsigned long   used_slots[SLOT_TABLE_SZ]; /* used/unused bitmap */
>> 	spinlock_t	slot_tbl_lock;
>> 	struct rpc_wait_queue	slot_tbl_waitq;	/* allocators may wait here */
>> -	int		max_slots;		/* # slots in table */
>> +	int		allocated_slots;	/* # slots in table */
>> +	int		max_slots;		/* max # slots in table */
>> 	int		highest_used_slotid;	/* sent to server on each SEQ.
>> 						 * op for dynamic resizing */
>> 	int		target_max_slots;	/* Set by CB_RECALL_SLOT as
>> @@ -206,11 +209,6 @@ struct nfs4_slot_table {
>> 	struct completion complete;
>> };
>> 
>> -static inline int slot_idx(struct nfs4_slot_table *tbl, struct nfs4_slot *sp)
>> -{
>> -	return sp - tbl->slots;
>> -}
>> -
>> /*
>>  * Session related parameters
>>  */
>> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
>> index c74595b..76b27a4 100644
>> --- a/include/linux/nfs_xdr.h
>> +++ b/include/linux/nfs_xdr.h
>> @@ -168,12 +168,14 @@ struct nfs4_channel_attrs {
>> 
>> /* nfs41 sessions slot seqid */
>> struct nfs4_slot {
>> +	struct hlist_node	slot_node;
>> 	u32		 	seq_nr;
>> +	u8			slotid;
>> };
>> 
>> struct nfs4_sequence_args {
>> 	struct nfs4_session	*sa_session;
>> -	u8			sa_slotid;
>> +	struct nfs4_slot        *sa_slot;
>> 	u8			sa_cache_this;
>> };
>> 

Re: [PATCH Version 4 4/7] NFSv4.1: dynamic session slots

[Benny Halevy]

On 2011-12-12 18:11, Adamson, Andy wrote:
> 
> On Dec 11, 2011, at 8:14 AM, Benny Halevy wrote:
> 
>> Hi Andy,
>>
>> I have some minor-ish comments inline below, but the whole
>> hlist concept looks to me like it might incur pretty high overhead
>> in cpu and memory caching.
> 
> I'd like to measure the difference. What is the recommended way?
> 

I think that the best way is to create a synthetic storm of SEQUENCE
COMPOUNDs on random slots that also expand or shrink the slot table
in a pre-defined probability.  The metric would be ops / sec
at saturation.

>>
>> Have you considered simply reallocating the slot table on grow
>> and maybe on shrink below a certain threshold?
> 
> Because the slot pointer is placed in the response, we would have to drain the slot table to grow/shrink in this manner.

Maybe you are optimizing for the rare case rather than for the common case.
It might not be straight forward but I think we can leverage the exactly
once semantics and since slots cannot be reused while their respective
RPC is in progress, we keep the old table around after cloning it to a newly
allocated array and release the old one only after it is drained.
the trickier parts would be to keep a correct reference count
on the table (doesn't seem to be too hard to do under the lock)
and update the new table on completion rather than the old one
when it's marked as being resized.

Benny

> 
> -->Andy
> 
>>
>> On 2011-11-09 20:58, andros@netapp.com wrote:
>>> From: Andy Adamson <andros@netapp.com>
>>>
>>> The session fore/backchannel slot tables implementation is changed from a
>>> static array to an hlist hashed on slotid. Slotid's are values 0..N where N
>>> is initially negotiated between the client and server via the CREATE_SESSION
>>> call, and can be dynamically changed by the server during an active session.
>>>
>>> When there are no allocated slots (mount), an initial number of session slots
>>> is allocated equal to the inital number of dynamic rpc_slots
>>> created upon transport creation (xprt->min_reqs).  Slots are then dynamically
>>> allocated as needed upto the CREATE_SESSION negotiated max_reqs value.
>>>
>>> For session reset the session is drained (inactive). The (old) session has
>>> allocated slot tables which we will reuse. nfs4_reduce_slots_locked is called
>>> and only reduces the allocated_slots if the new max_reqs negotiated by the
>>> reset CREATE_SESSION is less than the number of allocated_slots. The resultant
>>> slot table is then re-initialized.
>>>
>>> CB_SLOT_RECALL is changed only in that it now shares code with session
>>> reset - it also calls nfs4_reduce_slots_locked.
>>>
>>> The backchannel uses the new dynamic slot allocator, even though we only
>>> currently support a single back channel slot.
>>>
>>> Signed-off-by: Andy Adamson <andros@netapp.com>
>>> ---
>>> fs/nfs/callback_proc.c    |   25 +++-
>>> fs/nfs/internal.h         |    2 +
>>> fs/nfs/nfs4_fs.h          |    1 +
>>> fs/nfs/nfs4proc.c         |  297 ++++++++++++++++++++++++++++++++++-----------
>>> fs/nfs/nfs4state.c        |   12 +--
>>> fs/nfs/nfs4xdr.c          |   20 ++--
>>> include/linux/nfs_fs_sb.h |   12 +-
>>> include/linux/nfs_xdr.h   |    4 +-
>>> 8 files changed, 266 insertions(+), 107 deletions(-)
>>>
>>> diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
>>> index 54cea8a..77cc96c 100644
>>> --- a/fs/nfs/callback_proc.c
>>> +++ b/fs/nfs/callback_proc.c
>>> @@ -342,7 +342,7 @@ validate_seqid(struct nfs4_slot_table *tbl, struct cb_sequenceargs * args)
>>> 	if (args->csa_slotid >= NFS41_BC_MAX_CALLBACKS)
>>> 		return htonl(NFS4ERR_BADSLOT);
>>>
>>> -	slot = tbl->slots + args->csa_slotid;
>>> +	slot = nfs4_lookup_slot_locked(tbl, args->csa_slotid);
>>> 	dprintk("%s slot table seqid: %d\n", __func__, slot->seq_nr);
>>>
>>> 	/* Normal */
>>> @@ -377,6 +377,22 @@ out_ok:
>>> 	return htonl(NFS4_OK);
>>> }
>>>
>>> +static bool test_slot_referring(struct nfs4_slot_table *tbl,
>>> +				struct referring_call *ref)
>>> +{
>>> +	struct nfs4_slot *slot;
>>> +	bool status = false;
>>> +
>>> +	spin_lock(&tbl->slot_tbl_lock);
>>> +	if (test_bit(ref->rc_slotid, tbl->used_slots)) {
>>> +		slot = nfs4_lookup_slot_locked(tbl, ref->rc_slotid);
>>> +		if (slot->seq_nr == ref->rc_sequenceid)
>>> +			status = true;
>>> +	}
>>> +	spin_unlock(&tbl->slot_tbl_lock);
>>> +	return status;
>>> +}
>>> +
>>> /*
>>>  * For each referring call triple, check the session's slot table for
>>>  * a match.  If the slot is in use and the sequence numbers match, the
>>> @@ -417,12 +433,7 @@ static bool referring_call_exists(struct nfs_client *clp,
>>> 				((u32 *)&rclist->rcl_sessionid.data)[2],
>>> 				((u32 *)&rclist->rcl_sessionid.data)[3],
>>> 				ref->rc_sequenceid, ref->rc_slotid);
>>> -
>>> -			spin_lock(&tbl->slot_tbl_lock);
>>> -			status = (test_bit(ref->rc_slotid, tbl->used_slots) &&
>>> -				  tbl->slots[ref->rc_slotid].seq_nr ==
>>> -					ref->rc_sequenceid);
>>> -			spin_unlock(&tbl->slot_tbl_lock);
>>> +			status = test_slot_referring(tbl, ref);
>>> 			if (status)
>>> 				goto out;
>>> 		}
>>> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
>>> index c1a1bd8..4451abf 100644
>>> --- a/fs/nfs/internal.h
>>> +++ b/fs/nfs/internal.h
>>> @@ -354,6 +354,8 @@ extern int _nfs4_call_sync_session(struct rpc_clnt *clnt,
>>> 				   struct nfs4_sequence_args *args,
>>> 				   struct nfs4_sequence_res *res,
>>> 				   int cache_reply);
>>> +struct nfs4_slot *nfs4_lookup_slot_locked(struct nfs4_slot_table *tbl,
>>> +					  u8 slotid);
>>>
>>> /*
>>>  * Determine the device name as a string
>>> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
>>> index 693ae22..ff87169 100644
>>> --- a/fs/nfs/nfs4_fs.h
>>> +++ b/fs/nfs/nfs4_fs.h
>>> @@ -242,6 +242,7 @@ extern struct nfs4_session *nfs4_alloc_session(struct nfs_client *clp);
>>> extern int nfs4_proc_create_session(struct nfs_client *);
>>> extern int nfs4_proc_destroy_session(struct nfs4_session *);
>>> extern int nfs4_init_session(struct nfs_server *server);
>>> +extern void nfs4_reduce_slots_locked(struct nfs4_slot_table *tbl);
>>> extern int nfs4_proc_get_lease_time(struct nfs_client *clp,
>>> 		struct nfs_fsinfo *fsinfo);
>>> extern int nfs4_proc_layoutcommit(struct nfs4_layoutcommit_data *data,
>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>> index 2638291..e3a7663 100644
>>> --- a/fs/nfs/nfs4proc.c
>>> +++ b/fs/nfs/nfs4proc.c
>>> @@ -346,6 +346,162 @@ static void renew_lease(const struct nfs_server *server, unsigned long timestamp
>>> #if defined(CONFIG_NFS_V4_1)
>>>
>>> /*
>>> + * Session dynamic slot table helper functions
>>> + */
>>> +
>>> +static inline u32 slot_tbl_hash(const u8 slotid)
>>
>> nit: "const" not really needed here...
>>
>>> +{
>>> +	return (u32)slotid % SLOT_TABLE_SIZE;
>>> +}
>>> +
>>> +static void nfs4_init_slot(struct nfs4_slot *new, u8 slotid, int ivalue)
>>> +{
>>> +	INIT_HLIST_NODE(&new->slot_node);
>>> +	new->slotid = slotid;
>>> +	new->seq_nr = ivalue;
>>> +}
>>> +
>>> +static void nfs4_insert_slot_locked(struct nfs4_slot_table *tbl,
>>> +				    struct nfs4_slot *new)
>>> +{
>>> +	u32 hash = slot_tbl_hash(new->slotid);
>>> +
>>> +	hlist_add_head(&new->slot_node, &tbl->slots[hash]);
>>> +	tbl->allocated_slots++;
>>> +}
>>> +
>>> +/*
>>> + * Allocate a slot for the next slotid (tbl->allocated_slotid)
>>> + */
>>> +static int nfs4_alloc_add_slot(struct nfs4_slot_table *tbl,
>>> +			       int ivalue, int slotid,
>>> +			       gfp_t gfp_flags)
>>> +{
>>> +	struct nfs4_slot *new;
>>> +	struct rpc_task *task;
>>> +
>>> +	dprintk("--> %s slotid=%u tbl->allocated_slots=%d\n", __func__,
>>> +		slotid, tbl->allocated_slots);
>>> +	new = kzalloc(sizeof(struct nfs4_slot), gfp_flags);
>>> +	if (!new)
>>> +		return -ENOMEM;
>>> +	/* tbl->allocated_slots holds the next unallocated slotid */
>>> +	nfs4_init_slot(new, slotid, ivalue);
>>> +	spin_lock(&tbl->slot_tbl_lock);
>>> +	if (tbl->allocated_slots != slotid) {
>>
>> This condition doesn't seem to be reliable.
>> What if the slot table shrunk?
>>
>>> +		spin_unlock(&tbl->slot_tbl_lock);
>>> +		kfree(new);
>>> +		dprintk("%s slotid %u already allocated\n", __func__,
>>> +			slotid);
>>> +		return 0;
>>> +	}
>>> +	nfs4_insert_slot_locked(tbl, new);
>>> +
>>> +	/* This only applies to dynamic allocation */
>>> +	task = rpc_wake_up_next(&tbl->slot_tbl_waitq);
>>> +	if (task)
>>> +		dprintk("%s woke up task %5u\n", __func__, task->tk_pid);
>>> +	spin_unlock(&tbl->slot_tbl_lock);
>>> +	return 0;
>>> +}
>>> +
>>> +/*
>>> + * Allocate the minimal number of slots required by the transport.
>>> + * Called at session creation.
>>> + */
>>> +static int nfs4_prealloc_slots(struct nfs4_slot_table *tbl, int num, int ivalue)
>>> +{
>>> +	int i;
>>> +
>>> +	for (i = 0; i < num; i++) {
>>> +		if (nfs4_alloc_add_slot(tbl, ivalue, tbl->allocated_slots,
>>> +					GFP_NOFS) != 0)
>>> +			return -ENOMEM;
>>
>> Returning nfs4_alloc_add_slot's status would be more appropriate.
>> How about also freeing 0 to i-1 in the error case?
>>
>>> +	}
>>> +	return 0;
>>> +}
>>> +
>>> +/*
>>> + * Allocate the next slotid. If allocation fails, next request will retry.
>>> + */
>>> +static void nfs4_dynamic_alloc_slot(struct nfs4_slot_table *tbl, u8 slotid)
>>> +{
>>> +	if (slotid != tbl->allocated_slots) {
>>> +		dprintk("%s slotid %u already allocated\n", __func__, slotid);
>>> +		return;
>>> +	}
>>
>> This is checked again under the lock in nfs4_alloc_add_slot, no?
>> so if it is not supposed to be a common case (which I don't believe
>> it is) I'd just forget about this optimization and call the latter
>> directly.
>>
>>> +	nfs4_alloc_add_slot(tbl, 1, slotid, GFP_NOWAIT);
>>> +}
>>> +
>>> +/*
>>> + * Return the slot associated with the slotid returned by nfs4_find_slot.
>>> + * If the slot is not found, it has not been allocated.
>>> + */
>>> +struct nfs4_slot *
>>> +nfs4_lookup_slot_locked(struct nfs4_slot_table *tbl, u8 slotid)
>>> +{
>>> +	struct nfs4_slot *sp;
>>> +	struct hlist_node *n;
>>> +	u32 hash = slot_tbl_hash(slotid);
>>> +
>>> +	hlist_for_each_entry(sp, n, &tbl->slots[hash], slot_node) {
>>> +		if (sp->slotid == slotid)
>>> +			return sp;
>>> +	}
>>> +	return NULL;
>>> +}
>>> +
>>> +static void nfs4_remove_slot_locked(struct nfs4_slot_table *tbl,
>>> +				    struct nfs4_slot *slot)
>>> +{
>>> +	dprintk("--> %s slotid %d\n", __func__, slot->slotid);
>>> +	hlist_del_init(&slot->slot_node);
>>> +	tbl->allocated_slots--;
>>> +	kfree(slot);
>>> +}
>>> +
>>> +static void nfs4_free_all_slots(struct nfs4_slot_table *tbl)
>>> +{
>>> +	struct nfs4_slot *slotp;
>>> +	int i;
>>> +
>>> +	spin_lock(&tbl->slot_tbl_lock);
>>> +	for (i = 0; i < SLOT_TABLE_SIZE; i++) {
>>> +		while (!hlist_empty(&tbl->slots[i])) {
>>> +			slotp = hlist_entry(tbl->slots[i].first,
>>> +					    struct nfs4_slot, slot_node);
>>> +			nfs4_remove_slot_locked(tbl, slotp);
>>> +		}
>>> +		/* re-initialize hlist_head */
>>> +		tbl->slots[i].first = NULL;
>>> +	}
>>> +	spin_unlock(&tbl->slot_tbl_lock);
>>> +}
>>> +
>>> +/*
>>> + * Remove num_dec number of contiguous slotids starting from highest
>>> + * allocated slotid
>>> + *
>>> + * Note: set the new tbl->max_slots prior to calling.
>>> + */
>>> +void nfs4_reduce_slots_locked(struct nfs4_slot_table *tbl)
>>> +{
>>> +	struct nfs4_slot *removeme;
>>> +	int i, rm_slotid = tbl->allocated_slots - 1;
>>> +	int num_dec = tbl->allocated_slots - tbl->max_slots;
>>> +
>>> +	/* Reset of the slot table can make num_dec <= 0 */
>>> +	if (num_dec <= 0)
>>> +		return;
>>> +	for (i = 0; i < num_dec; i++) {
>>> +		removeme = nfs4_lookup_slot_locked(tbl, rm_slotid);
>>> +		BUG_ON(removeme == NULL);
>>> +		nfs4_remove_slot_locked(tbl, removeme);
>>> +		rm_slotid--;
>>> +	}
>>> +}
>>> +
>>> +/*
>>>  * nfs4_free_slot - free a slot and efficiently update slot table.
>>>  *
>>>  * freeing a slot is trivially done by clearing its respective bit
>>> @@ -390,8 +546,11 @@ static void nfs4_check_drain_fc_complete(struct nfs4_session *ses)
>>>
>>> 	if (!test_bit(NFS4_SESSION_DRAINING, &ses->session_state)) {
>>> 		task = rpc_wake_up_next(&ses->fc_slot_table.slot_tbl_waitq);
>>> -		if (task)
>>> +		if (task) {
>>> 			rpc_task_set_priority(task, RPC_PRIORITY_PRIVILEGED);
>>> +			dprintk("%s woke up task pid %5u\n", __func__,
>>> +				task->tk_pid);
>>> +		}
>>> 		return;
>>> 	}
>>>
>>> @@ -427,7 +586,7 @@ static void nfs41_sequence_free_slot(struct nfs4_sequence_res *res)
>>> 	}
>>>
>>> 	spin_lock(&tbl->slot_tbl_lock);
>>> -	nfs4_free_slot(tbl, res->sr_slot - tbl->slots);
>>> +	nfs4_free_slot(tbl, res->sr_slot->slotid);
>>> 	nfs4_check_drain_fc_complete(res->sr_session);
>>> 	spin_unlock(&tbl->slot_tbl_lock);
>>> 	res->sr_slot = NULL;
>>> @@ -468,10 +627,8 @@ static int nfs41_sequence_done(struct rpc_task *task, struct nfs4_sequence_res *
>>> 		 * returned NFS4ERR_DELAY as per Section 2.10.6.2
>>> 		 * of RFC5661.
>>> 		 */
>>> -		dprintk("%s: slot=%td seq=%d: Operation in progress\n",
>>> -			__func__,
>>> -			res->sr_slot - res->sr_session->fc_slot_table.slots,
>>> -			res->sr_slot->seq_nr);
>>> +		dprintk("%s: slotid=%u seq=%d: Operation in progress\n",
>>> +			__func__, res->sr_slot->slotid, res->sr_slot->seq_nr);
>>> 		goto out_retry;
>>> 	default:
>>> 		/* Just update the slot sequence no. */
>>> @@ -479,7 +636,8 @@ static int nfs41_sequence_done(struct rpc_task *task, struct nfs4_sequence_res *
>>> 	}
>>> out:
>>> 	/* The session may be reset by one of the error handlers. */
>>> -	dprintk("%s: Error %d free the slot \n", __func__, res->sr_status);
>>> +	dprintk("%s: Error %d free the slot for task pid %5u\n", __func__,
>>> +		res->sr_status, task->tk_pid);
>>> 	nfs41_sequence_free_slot(res);
>>> 	return 1;
>>> out_retry:
>>> @@ -540,7 +698,7 @@ int nfs41_setup_sequence(struct nfs4_session *session,
>>> 	struct nfs4_slot_table *tbl;
>>> 	u8 slotid;
>>>
>>> -	dprintk("--> %s\n", __func__);
>>> +	dprintk("--> %s task pid %5u\n", __func__, task->tk_pid);
>>> 	/* slot already allocated? */
>>> 	if (res->sr_slot != NULL)
>>> 		return 0;
>>> @@ -575,12 +733,24 @@ int nfs41_setup_sequence(struct nfs4_session *session,
>>> 		dprintk("<-- %s: no free slots\n", __func__);
>>> 		return -EAGAIN;
>>> 	}
>>> +	slot = nfs4_lookup_slot_locked(tbl, slotid);
>>> +	if (slot == NULL) {
>>> +		/* keep FIFO order */
>>> +		rpc_task_set_priority(task, RPC_PRIORITY_PRIVILEGED);
>>> +		rpc_sleep_on(&tbl->slot_tbl_waitq, task, NULL);
>>> +		nfs4_free_slot(tbl, slotid);
>>
>> why free the slot here?
>>
>> Benny
>>
>>> +		spin_unlock(&tbl->slot_tbl_lock);
>>> +		dprintk("%s task pid %5u sleeping for dynamic slot %d\n",
>>> +			__func__, task->tk_pid, slotid);
>>> +		/* rpc_wakeup_next called upon success by slot allocator */
>>> +		nfs4_dynamic_alloc_slot(tbl, slotid);
>>> +		return -EAGAIN;
>>> +	}
>>> 	spin_unlock(&tbl->slot_tbl_lock);
>>>
>>> 	rpc_task_set_priority(task, RPC_PRIORITY_NORMAL);
>>> -	slot = tbl->slots + slotid;
>>> 	args->sa_session = session;
>>> -	args->sa_slotid = slotid;
>>> +	args->sa_slot = slot;
>>> 	args->sa_cache_this = cache_reply;
>>>
>>> 	dprintk("<-- %s slotid=%d seqid=%d\n", __func__, slotid, slot->seq_nr);
>>> @@ -613,9 +783,9 @@ int nfs4_setup_sequence(const struct nfs_server *server,
>>> 		goto out;
>>> 	}
>>>
>>> -	dprintk("--> %s clp %p session %p sr_slot %td\n",
>>> +	dprintk("--> %s clp %p session %p sr_slotid %d\n",
>>> 		__func__, session->clp, session, res->sr_slot ?
>>> -			res->sr_slot - session->fc_slot_table.slots : -1);
>>> +			res->sr_slot->slotid : -1);
>>>
>>> 	ret = nfs41_setup_sequence(session, args, res, cache_reply,
>>> 				   task);
>>> @@ -4976,84 +5146,64 @@ int nfs4_proc_get_lease_time(struct nfs_client *clp, struct nfs_fsinfo *fsinfo)
>>> }
>>>
>>> /*
>>> - * Reset a slot table
>>> + * Reset a slot table. Session has been drained and reset with new slot table
>>> + * parameters from create_session.
>>> + * Keep all allocated slots that fit into new slot table max_reqs parameter.
>>>  */
>>> -static int nfs4_reset_slot_table(struct nfs4_slot_table *tbl, u32 max_reqs,
>>> +static void nfs4_reset_slot_table(struct nfs4_slot_table *tbl, u32 new_max_reqs,
>>> 				 int ivalue)
>>> {
>>> -	struct nfs4_slot *new = NULL;
>>> +	struct nfs4_slot *sp;
>>> +	struct hlist_node *n;
>>> 	int i;
>>> -	int ret = 0;
>>> -
>>> -	dprintk("--> %s: max_reqs=%u, tbl->max_slots %d\n", __func__,
>>> -		max_reqs, tbl->max_slots);
>>>
>>> -	/* Does the newly negotiated max_reqs match the existing slot table? */
>>> -	if (max_reqs != tbl->max_slots) {
>>> -		ret = -ENOMEM;
>>> -		new = kmalloc(max_reqs * sizeof(struct nfs4_slot),
>>> -			      GFP_NOFS);
>>> -		if (!new)
>>> -			goto out;
>>> -		ret = 0;
>>> -		kfree(tbl->slots);
>>> -	}
>>> 	spin_lock(&tbl->slot_tbl_lock);
>>> -	if (new) {
>>> -		tbl->slots = new;
>>> -		tbl->max_slots = max_reqs;
>>> +	tbl->max_slots = new_max_reqs;
>>> +	nfs4_reduce_slots_locked(tbl);
>>> +	tbl->highest_used_slotid = -1;
>>> +
>>> +	for (i = 0; i < SLOT_TABLE_SIZE; i++) {
>>> +		hlist_for_each_entry(sp, n, &tbl->slots[i], slot_node)
>>> +			sp->seq_nr = ivalue;
>>> 	}
>>> -	for (i = 0; i < tbl->max_slots; ++i)
>>> -		tbl->slots[i].seq_nr = ivalue;
>>> 	spin_unlock(&tbl->slot_tbl_lock);
>>> -	dprintk("%s: tbl=%p slots=%p max_slots=%d\n", __func__,
>>> -		tbl, tbl->slots, tbl->max_slots);
>>> -out:
>>> -	dprintk("<-- %s: return %d\n", __func__, ret);
>>> -	return ret;
>>> +	dprintk("%s: tbl->max_slots=%d tbl->allocated_slots=%d\n",
>>> +		__func__, tbl->max_slots, tbl->allocated_slots);
>>> +	return;
>>> }
>>>
>>> /* Destroy the slot table */
>>> static void nfs4_destroy_slot_tables(struct nfs4_session *session)
>>> {
>>> -	if (session->fc_slot_table.slots != NULL) {
>>> -		kfree(session->fc_slot_table.slots);
>>> -		session->fc_slot_table.slots = NULL;
>>> -	}
>>> -	if (session->bc_slot_table.slots != NULL) {
>>> -		kfree(session->bc_slot_table.slots);
>>> -		session->bc_slot_table.slots = NULL;
>>> -	}
>>> -	return;
>>> +	dprintk("--> %s\n", __func__);
>>> +
>>> +	nfs4_free_all_slots(&session->fc_slot_table);
>>> +	nfs4_free_all_slots(&session->bc_slot_table);
>>> }
>>>
>>> /*
>>>  * Initialize slot table
>>>  */
>>> static int nfs4_init_slot_table(struct nfs4_slot_table *tbl,
>>> -		int max_slots, int ivalue)
>>> +		int max_slots, int num_prealloc, int ivalue)
>>> {
>>> -	struct nfs4_slot *slot;
>>> -	int ret = -ENOMEM;
>>> +	int ret;
>>>
>>> 	BUG_ON(max_slots > NFS4_MAX_SLOT_TABLE);
>>> -
>>> -	dprintk("--> %s: max_reqs=%u\n", __func__, max_slots);
>>> -
>>> -	slot = kcalloc(max_slots, sizeof(struct nfs4_slot), GFP_NOFS);
>>> -	if (!slot)
>>> +	ret = nfs4_prealloc_slots(tbl, num_prealloc, ivalue);
>>> +	if (ret) {
>>> +		nfs4_free_all_slots(tbl);
>>> 		goto out;
>>> +	}
>>> 	ret = 0;
>>>
>>> 	spin_lock(&tbl->slot_tbl_lock);
>>> 	tbl->max_slots = max_slots;
>>> -	tbl->slots = slot;
>>> 	tbl->highest_used_slotid = -1;  /* no slot is currently used */
>>> 	spin_unlock(&tbl->slot_tbl_lock);
>>> -	dprintk("%s: tbl=%p slots=%p max_slots=%d\n", __func__,
>>> -		tbl, tbl->slots, tbl->max_slots);
>>> +	dprintk("%s: tbl=%p max_slots=%d, tbl->allocated_slots=%d\n", __func__,
>>> +		tbl, tbl->max_slots, tbl->allocated_slots);
>>> out:
>>> -	dprintk("<-- %s: return %d\n", __func__, ret);
>>> 	return ret;
>>> }
>>>
>>> @@ -5063,30 +5213,34 @@ out:
>>> static int nfs4_setup_session_slot_tables(struct nfs4_session *ses)
>>> {
>>> 	struct nfs4_slot_table *tbl;
>>> -	int status;
>>> +	int status = 0;
>>>
>>> 	dprintk("--> %s\n", __func__);
>>> 	/* Fore channel */
>>> 	tbl = &ses->fc_slot_table;
>>> -	if (tbl->slots == NULL) {
>>> -		status = nfs4_init_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
>>> +	if (tbl->allocated_slots == 0) {
>>> +		status = nfs4_init_slot_table(tbl, ses->fc_attrs.max_reqs,
>>> +				ses->clp->cl_rpcclient->cl_xprt->min_reqs, 1);
>>> 		if (status) /* -ENOMEM */
>>> 			return status;
>>> 	} else {
>>> -		status = nfs4_reset_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
>>> -		if (status)
>>> -			return status;
>>> +		nfs4_reset_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
>>> 	}
>>> +
>>> 	/* Back channel */
>>> 	tbl = &ses->bc_slot_table;
>>> -	if (tbl->slots == NULL) {
>>> -		status = nfs4_init_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
>>> +	if (tbl->allocated_slots == 0) {
>>> +		status = nfs4_init_slot_table(tbl, ses->bc_attrs.max_reqs,
>>> +				/* Backchannel max_reqs == 1 */
>>> +				ses->bc_attrs.max_reqs, 0);
>>> 		if (status)
>>> 			/* Fore and back channel share a connection so get
>>> 			 * both slot tables or neither */
>>> 			nfs4_destroy_slot_tables(ses);
>>> -	} else
>>> -		status = nfs4_reset_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
>>> +	} else {
>>> +		nfs4_reset_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
>>> +	}
>>> +
>>> 	return status;
>>> }
>>>
>>> @@ -5276,7 +5430,6 @@ int nfs4_proc_create_session(struct nfs_client *clp)
>>>
>>> 	/* Init or reset the session slot tables */
>>> 	status = nfs4_setup_session_slot_tables(session);
>>> -	dprintk("slot table setup returned %d\n", status);
>>> 	if (status)
>>> 		goto out;
>>>
>>> @@ -5284,7 +5437,7 @@ int nfs4_proc_create_session(struct nfs_client *clp)
>>> 	dprintk("%s client>seqid %d sessionid %u:%u:%u:%u\n", __func__,
>>> 		clp->cl_seqid, ptr[0], ptr[1], ptr[2], ptr[3]);
>>> out:
>>> -	dprintk("<-- %s\n", __func__);
>>> +	dprintk("<-- %s status %d\n", __func__, status);
>>> 	return status;
>>> }
>>>
>>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>>> index 39914be..8260865 100644
>>> --- a/fs/nfs/nfs4state.c
>>> +++ b/fs/nfs/nfs4state.c
>>> @@ -1573,26 +1573,16 @@ static int nfs4_recall_slot(struct nfs_client *clp)
>>> {
>>> 	struct nfs4_slot_table *fc_tbl = &clp->cl_session->fc_slot_table;
>>> 	struct nfs4_channel_attrs *fc_attrs = &clp->cl_session->fc_attrs;
>>> -	struct nfs4_slot *new, *old;
>>> -	int i;
>>>
>>> 	nfs4_begin_drain_session(clp);
>>> -	new = kmalloc(fc_tbl->target_max_slots * sizeof(struct nfs4_slot),
>>> -		      GFP_NOFS);
>>> -        if (!new)
>>> -		return -ENOMEM;
>>>
>>> 	spin_lock(&fc_tbl->slot_tbl_lock);
>>> -	for (i = 0; i < fc_tbl->target_max_slots; i++)
>>> -		new[i].seq_nr = fc_tbl->slots[i].seq_nr;
>>> -	old = fc_tbl->slots;
>>> -	fc_tbl->slots = new;
>>> 	fc_tbl->max_slots = fc_tbl->target_max_slots;
>>> +	nfs4_reduce_slots_locked(fc_tbl);
>>> 	fc_tbl->target_max_slots = 0;
>>> 	fc_attrs->max_reqs = fc_tbl->max_slots;
>>> 	spin_unlock(&fc_tbl->slot_tbl_lock);
>>>
>>> -	kfree(old);
>>> 	nfs4_end_drain_session(clp);
>>> 	return 0;
>>> }
>>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
>>> index e6161b2..c42ec29 100644
>>> --- a/fs/nfs/nfs4xdr.c
>>> +++ b/fs/nfs/nfs4xdr.c
>>> @@ -1872,7 +1872,6 @@ static void encode_sequence(struct xdr_stream *xdr,
>>> #if defined(CONFIG_NFS_V4_1)
>>> 	struct nfs4_session *session = args->sa_session;
>>> 	struct nfs4_slot_table *tp;
>>> -	struct nfs4_slot *slot;
>>> 	__be32 *p;
>>>
>>> 	if (!session)
>>> @@ -1880,8 +1879,7 @@ static void encode_sequence(struct xdr_stream *xdr,
>>>
>>> 	tp = &session->fc_slot_table;
>>>
>>> -	WARN_ON(args->sa_slotid == NFS4_MAX_SLOT_TABLE);
>>> -	slot = tp->slots + args->sa_slotid;
>>> +	WARN_ON(args->sa_slot->slotid == NFS4_MAX_SLOT_TABLE);
>>>
>>> 	p = reserve_space(xdr, 4 + NFS4_MAX_SESSIONID_LEN + 16);
>>> 	*p++ = cpu_to_be32(OP_SEQUENCE);
>>> @@ -1896,11 +1894,11 @@ static void encode_sequence(struct xdr_stream *xdr,
>>> 		((u32 *)session->sess_id.data)[1],
>>> 		((u32 *)session->sess_id.data)[2],
>>> 		((u32 *)session->sess_id.data)[3],
>>> -		slot->seq_nr, args->sa_slotid,
>>> +		args->sa_slot->seq_nr, args->sa_slot->slotid,
>>> 		tp->highest_used_slotid, args->sa_cache_this);
>>> 	p = xdr_encode_opaque_fixed(p, session->sess_id.data, NFS4_MAX_SESSIONID_LEN);
>>> -	*p++ = cpu_to_be32(slot->seq_nr);
>>> -	*p++ = cpu_to_be32(args->sa_slotid);
>>> +	*p++ = cpu_to_be32(args->sa_slot->seq_nr);
>>> +	*p++ = cpu_to_be32(args->sa_slot->slotid);
>>> 	*p++ = cpu_to_be32(tp->highest_used_slotid);
>>> 	*p = cpu_to_be32(args->sa_cache_this);
>>> 	hdr->nops++;
>>> @@ -5361,13 +5359,17 @@ static int decode_sequence(struct xdr_stream *xdr,
>>> 	/* seqid */
>>> 	dummy = be32_to_cpup(p++);
>>> 	if (dummy != res->sr_slot->seq_nr) {
>>> -		dprintk("%s Invalid sequence number\n", __func__);
>>> +		dprintk("%s Invalid seq num %d for [slotid:seq_nr] [%d:%d]\n",
>>> +			__func__, dummy, res->sr_slot->slotid,
>>> +			res->sr_slot->seq_nr);
>>> 		goto out_err;
>>> 	}
>>> 	/* slot id */
>>> 	dummy = be32_to_cpup(p++);
>>> -	if (dummy != res->sr_slot - res->sr_session->fc_slot_table.slots) {
>>> -		dprintk("%s Invalid slot id\n", __func__);
>>> +	if (dummy != res->sr_slot->slotid) {
>>> +		dprintk("%s Invalid slot id %d for [slotid:seq_nr] [%d:%d]\n",
>>> +			__func__, dummy, res->sr_slot->slotid,
>>> +			res->sr_slot->seq_nr);
>>> 		goto out_err;
>>> 	}
>>> 	/* highest slot id - currently not processed */
>>> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
>>> index b5479df..90c6e2b 100644
>>> --- a/include/linux/nfs_fs_sb.h
>>> +++ b/include/linux/nfs_fs_sb.h
>>> @@ -193,12 +193,15 @@ struct nfs_server {
>>>
>>> /* Sessions */
>>> #define SLOT_TABLE_SZ (NFS4_MAX_SLOT_TABLE/(8*sizeof(long)))
>>> +#define SLOT_TABLE_BITS                5
>>> +#define SLOT_TABLE_SIZE                (1 << SLOT_TABLE_BITS)
>>> struct nfs4_slot_table {
>>> -	struct nfs4_slot *slots;		/* seqid per slot */
>>> +	struct hlist_head slots[SLOT_TABLE_SIZE];
>>> 	unsigned long   used_slots[SLOT_TABLE_SZ]; /* used/unused bitmap */
>>> 	spinlock_t	slot_tbl_lock;
>>> 	struct rpc_wait_queue	slot_tbl_waitq;	/* allocators may wait here */
>>> -	int		max_slots;		/* # slots in table */
>>> +	int		allocated_slots;	/* # slots in table */
>>> +	int		max_slots;		/* max # slots in table */
>>> 	int		highest_used_slotid;	/* sent to server on each SEQ.
>>> 						 * op for dynamic resizing */
>>> 	int		target_max_slots;	/* Set by CB_RECALL_SLOT as
>>> @@ -206,11 +209,6 @@ struct nfs4_slot_table {
>>> 	struct completion complete;
>>> };
>>>
>>> -static inline int slot_idx(struct nfs4_slot_table *tbl, struct nfs4_slot *sp)
>>> -{
>>> -	return sp - tbl->slots;
>>> -}
>>> -
>>> /*
>>>  * Session related parameters
>>>  */
>>> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
>>> index c74595b..76b27a4 100644
>>> --- a/include/linux/nfs_xdr.h
>>> +++ b/include/linux/nfs_xdr.h
>>> @@ -168,12 +168,14 @@ struct nfs4_channel_attrs {
>>>
>>> /* nfs41 sessions slot seqid */
>>> struct nfs4_slot {
>>> +	struct hlist_node	slot_node;
>>> 	u32		 	seq_nr;
>>> +	u8			slotid;
>>> };
>>>
>>> struct nfs4_sequence_args {
>>> 	struct nfs4_session	*sa_session;
>>> -	u8			sa_slotid;
>>> +	struct nfs4_slot        *sa_slot;
>>> 	u8			sa_cache_this;
>>> };
>>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH Version 4 4/7] NFSv4.1: dynamic session slots

[Adamson, Andy]

On Dec 11, 2011, at 8:14 AM, Benny Halevy wrote:

> Hi Andy,
> 
> I have some minor-ish comments inline below, but the whole
> hlist concept looks to me like it might incur pretty high overhead
> in cpu and memory caching.

You mean the hash lookup?

> 
> Have you considered simply reallocating the slot table on grow
> and maybe on shrink below a certain threshold?
> 
> On 2011-11-09 20:58, andros@netapp.com wrote:
>> From: Andy Adamson <andros@netapp.com>
>> 
>> The session fore/backchannel slot tables implementation is changed from a
>> static array to an hlist hashed on slotid. Slotid's are values 0..N where N
>> is initially negotiated between the client and server via the CREATE_SESSION
>> call, and can be dynamically changed by the server during an active session.
>> 
>> When there are no allocated slots (mount), an initial number of session slots
>> is allocated equal to the inital number of dynamic rpc_slots
>> created upon transport creation (xprt->min_reqs).  Slots are then dynamically
>> allocated as needed upto the CREATE_SESSION negotiated max_reqs value.
>> 
>> For session reset the session is drained (inactive). The (old) session has
>> allocated slot tables which we will reuse. nfs4_reduce_slots_locked is called
>> and only reduces the allocated_slots if the new max_reqs negotiated by the
>> reset CREATE_SESSION is less than the number of allocated_slots. The resultant
>> slot table is then re-initialized.
>> 
>> CB_SLOT_RECALL is changed only in that it now shares code with session
>> reset - it also calls nfs4_reduce_slots_locked.
>> 
>> The backchannel uses the new dynamic slot allocator, even though we only
>> currently support a single back channel slot.
>> 
>> Signed-off-by: Andy Adamson <andros@netapp.com>
>> ---
>> fs/nfs/callback_proc.c    |   25 +++-
>> fs/nfs/internal.h         |    2 +
>> fs/nfs/nfs4_fs.h          |    1 +
>> fs/nfs/nfs4proc.c         |  297 ++++++++++++++++++++++++++++++++++-----------
>> fs/nfs/nfs4state.c        |   12 +--
>> fs/nfs/nfs4xdr.c          |   20 ++--
>> include/linux/nfs_fs_sb.h |   12 +-
>> include/linux/nfs_xdr.h   |    4 +-
>> 8 files changed, 266 insertions(+), 107 deletions(-)
>> 
>> diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
>> index 54cea8a..77cc96c 100644
>> --- a/fs/nfs/callback_proc.c
>> +++ b/fs/nfs/callback_proc.c
>> @@ -342,7 +342,7 @@ validate_seqid(struct nfs4_slot_table *tbl, struct cb_sequenceargs * args)
>> 	if (args->csa_slotid >= NFS41_BC_MAX_CALLBACKS)
>> 		return htonl(NFS4ERR_BADSLOT);
>> 
>> -	slot = tbl->slots + args->csa_slotid;
>> +	slot = nfs4_lookup_slot_locked(tbl, args->csa_slotid);
>> 	dprintk("%s slot table seqid: %d\n", __func__, slot->seq_nr);
>> 
>> 	/* Normal */
>> @@ -377,6 +377,22 @@ out_ok:
>> 	return htonl(NFS4_OK);
>> }
>> 
>> +static bool test_slot_referring(struct nfs4_slot_table *tbl,
>> +				struct referring_call *ref)
>> +{
>> +	struct nfs4_slot *slot;
>> +	bool status = false;
>> +
>> +	spin_lock(&tbl->slot_tbl_lock);
>> +	if (test_bit(ref->rc_slotid, tbl->used_slots)) {
>> +		slot = nfs4_lookup_slot_locked(tbl, ref->rc_slotid);
>> +		if (slot->seq_nr == ref->rc_sequenceid)
>> +			status = true;
>> +	}
>> +	spin_unlock(&tbl->slot_tbl_lock);
>> +	return status;
>> +}
>> +
>> /*
>>  * For each referring call triple, check the session's slot table for
>>  * a match.  If the slot is in use and the sequence numbers match, the
>> @@ -417,12 +433,7 @@ static bool referring_call_exists(struct nfs_client *clp,
>> 				((u32 *)&rclist->rcl_sessionid.data)[2],
>> 				((u32 *)&rclist->rcl_sessionid.data)[3],
>> 				ref->rc_sequenceid, ref->rc_slotid);
>> -
>> -			spin_lock(&tbl->slot_tbl_lock);
>> -			status = (test_bit(ref->rc_slotid, tbl->used_slots) &&
>> -				  tbl->slots[ref->rc_slotid].seq_nr ==
>> -					ref->rc_sequenceid);
>> -			spin_unlock(&tbl->slot_tbl_lock);
>> +			status = test_slot_referring(tbl, ref);
>> 			if (status)
>> 				goto out;
>> 		}
>> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
>> index c1a1bd8..4451abf 100644
>> --- a/fs/nfs/internal.h
>> +++ b/fs/nfs/internal.h
>> @@ -354,6 +354,8 @@ extern int _nfs4_call_sync_session(struct rpc_clnt *clnt,
>> 				   struct nfs4_sequence_args *args,
>> 				   struct nfs4_sequence_res *res,
>> 				   int cache_reply);
>> +struct nfs4_slot *nfs4_lookup_slot_locked(struct nfs4_slot_table *tbl,
>> +					  u8 slotid);
>> 
>> /*
>>  * Determine the device name as a string
>> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
>> index 693ae22..ff87169 100644
>> --- a/fs/nfs/nfs4_fs.h
>> +++ b/fs/nfs/nfs4_fs.h
>> @@ -242,6 +242,7 @@ extern struct nfs4_session *nfs4_alloc_session(struct nfs_client *clp);
>> extern int nfs4_proc_create_session(struct nfs_client *);
>> extern int nfs4_proc_destroy_session(struct nfs4_session *);
>> extern int nfs4_init_session(struct nfs_server *server);
>> +extern void nfs4_reduce_slots_locked(struct nfs4_slot_table *tbl);
>> extern int nfs4_proc_get_lease_time(struct nfs_client *clp,
>> 		struct nfs_fsinfo *fsinfo);
>> extern int nfs4_proc_layoutcommit(struct nfs4_layoutcommit_data *data,
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index 2638291..e3a7663 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -346,6 +346,162 @@ static void renew_lease(const struct nfs_server *server, unsigned long timestamp
>> #if defined(CONFIG_NFS_V4_1)
>> 
>> /*
>> + * Session dynamic slot table helper functions
>> + */
>> +
>> +static inline u32 slot_tbl_hash(const u8 slotid)
> 
> nit: "const" not really needed here…

OK

> 
>> +{
>> +	return (u32)slotid % SLOT_TABLE_SIZE;
>> +}
>> +
>> +static void nfs4_init_slot(struct nfs4_slot *new, u8 slotid, int ivalue)
>> +{
>> +	INIT_HLIST_NODE(&new->slot_node);
>> +	new->slotid = slotid;
>> +	new->seq_nr = ivalue;
>> +}
>> +
>> +static void nfs4_insert_slot_locked(struct nfs4_slot_table *tbl,
>> +				    struct nfs4_slot *new)
>> +{
>> +	u32 hash = slot_tbl_hash(new->slotid);
>> +
>> +	hlist_add_head(&new->slot_node, &tbl->slots[hash]);
>> +	tbl->allocated_slots++;
>> +}
>> +
>> +/*
>> + * Allocate a slot for the next slotid (tbl->allocated_slotid)
>> + */
>> +static int nfs4_alloc_add_slot(struct nfs4_slot_table *tbl,
>> +			       int ivalue, int slotid,
>> +			       gfp_t gfp_flags)
>> +{
>> +	struct nfs4_slot *new;
>> +	struct rpc_task *task;
>> +
>> +	dprintk("--> %s slotid=%u tbl->allocated_slots=%d\n", __func__,
>> +		slotid, tbl->allocated_slots);
>> +	new = kzalloc(sizeof(struct nfs4_slot), gfp_flags);
>> +	if (!new)
>> +		return -ENOMEM;
>> +	/* tbl->allocated_slots holds the next unallocated slotid */
>> +	nfs4_init_slot(new, slotid, ivalue);
>> +	spin_lock(&tbl->slot_tbl_lock);
>> +	if (tbl->allocated_slots != slotid) {
> 
> This condition doesn't seem to be reliable.
> What if the slot table shrunk?

tbl->allocated_slots is inc'ed/dec'ed under the slot_tbl_lock. If the slot table shrunk it's because nfs4_remove_slots_locked() was called which adjusts the allocated_slots accordingly

So I think it is absolutely reliable.

> 
>> +		spin_unlock(&tbl->slot_tbl_lock);
>> +		kfree(new);
>> +		dprintk("%s slotid %u already allocated\n", __func__,
>> +			slotid);
>> +		return 0;
>> +	}
>> +	nfs4_insert_slot_locked(tbl, new);
>> +
>> +	/* This only applies to dynamic allocation */
>> +	task = rpc_wake_up_next(&tbl->slot_tbl_waitq);
>> +	if (task)
>> +		dprintk("%s woke up task %5u\n", __func__, task->tk_pid);
>> +	spin_unlock(&tbl->slot_tbl_lock);
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Allocate the minimal number of slots required by the transport.
>> + * Called at session creation.
>> + */
>> +static int nfs4_prealloc_slots(struct nfs4_slot_table *tbl, int num, int ivalue)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < num; i++) {
>> +		if (nfs4_alloc_add_slot(tbl, ivalue, tbl->allocated_slots,
>> +					GFP_NOFS) != 0)
>> +			return -ENOMEM;
> 
> Returning nfs4_alloc_add_slot's status would be more appropriate.

OK

> How about also freeing 0 to i-1 in the error case?

The transport minimum rpc_slots is the minimum number of (dynamic) rpc_slots needed to make progress. It's set to 2 for TCP.  We don't want to try and operate when we don't have the minimum number slots.
> 
>> +	}
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Allocate the next slotid. If allocation fails, next request will retry.
>> + */
>> +static void nfs4_dynamic_alloc_slot(struct nfs4_slot_table *tbl, u8 slotid)
>> +{
>> +	if (slotid != tbl->allocated_slots) {
>> +		dprintk("%s slotid %u already allocated\n", __func__, slotid);
>> +		return;
>> +	}
> 
> This is checked again under the lock in nfs4_alloc_add_slot, no?
> so if it is not supposed to be a common case (which I don't believe
> it is) I'd just forget about this optimization and call the latter
> directly.

Yeah, I agree.
> 
>> +	nfs4_alloc_add_slot(tbl, 1, slotid, GFP_NOWAIT);
>> +}
>> +
>> +/*
>> + * Return the slot associated with the slotid returned by nfs4_find_slot.
>> + * If the slot is not found, it has not been allocated.
>> + */
>> +struct nfs4_slot *
>> +nfs4_lookup_slot_locked(struct nfs4_slot_table *tbl, u8 slotid)
>> +{
>> +	struct nfs4_slot *sp;
>> +	struct hlist_node *n;
>> +	u32 hash = slot_tbl_hash(slotid);
>> +
>> +	hlist_for_each_entry(sp, n, &tbl->slots[hash], slot_node) {
>> +		if (sp->slotid == slotid)
>> +			return sp;
>> +	}
>> +	return NULL;
>> +}
>> +
>> +static void nfs4_remove_slot_locked(struct nfs4_slot_table *tbl,
>> +				    struct nfs4_slot *slot)
>> +{
>> +	dprintk("--> %s slotid %d\n", __func__, slot->slotid);
>> +	hlist_del_init(&slot->slot_node);
>> +	tbl->allocated_slots--;
>> +	kfree(slot);
>> +}
>> +
>> +static void nfs4_free_all_slots(struct nfs4_slot_table *tbl)
>> +{
>> +	struct nfs4_slot *slotp;
>> +	int i;
>> +
>> +	spin_lock(&tbl->slot_tbl_lock);
>> +	for (i = 0; i < SLOT_TABLE_SIZE; i++) {
>> +		while (!hlist_empty(&tbl->slots[i])) {
>> +			slotp = hlist_entry(tbl->slots[i].first,
>> +					    struct nfs4_slot, slot_node);
>> +			nfs4_remove_slot_locked(tbl, slotp);
>> +		}
>> +		/* re-initialize hlist_head */
>> +		tbl->slots[i].first = NULL;
>> +	}
>> +	spin_unlock(&tbl->slot_tbl_lock);
>> +}
>> +
>> +/*
>> + * Remove num_dec number of contiguous slotids starting from highest
>> + * allocated slotid
>> + *
>> + * Note: set the new tbl->max_slots prior to calling.
>> + */
>> +void nfs4_reduce_slots_locked(struct nfs4_slot_table *tbl)
>> +{
>> +	struct nfs4_slot *removeme;
>> +	int i, rm_slotid = tbl->allocated_slots - 1;
>> +	int num_dec = tbl->allocated_slots - tbl->max_slots;
>> +
>> +	/* Reset of the slot table can make num_dec <= 0 */
>> +	if (num_dec <= 0)
>> +		return;
>> +	for (i = 0; i < num_dec; i++) {
>> +		removeme = nfs4_lookup_slot_locked(tbl, rm_slotid);
>> +		BUG_ON(removeme == NULL);
>> +		nfs4_remove_slot_locked(tbl, removeme);
>> +		rm_slotid--;
>> +	}
>> +}
>> +
>> +/*
>>  * nfs4_free_slot - free a slot and efficiently update slot table.
>>  *
>>  * freeing a slot is trivially done by clearing its respective bit
>> @@ -390,8 +546,11 @@ static void nfs4_check_drain_fc_complete(struct nfs4_session *ses)
>> 
>> 	if (!test_bit(NFS4_SESSION_DRAINING, &ses->session_state)) {
>> 		task = rpc_wake_up_next(&ses->fc_slot_table.slot_tbl_waitq);
>> -		if (task)
>> +		if (task) {
>> 			rpc_task_set_priority(task, RPC_PRIORITY_PRIVILEGED);
>> +			dprintk("%s woke up task pid %5u\n", __func__,
>> +				task->tk_pid);
>> +		}
>> 		return;
>> 	}
>> 
>> @@ -427,7 +586,7 @@ static void nfs41_sequence_free_slot(struct nfs4_sequence_res *res)
>> 	}
>> 
>> 	spin_lock(&tbl->slot_tbl_lock);
>> -	nfs4_free_slot(tbl, res->sr_slot - tbl->slots);
>> +	nfs4_free_slot(tbl, res->sr_slot->slotid);
>> 	nfs4_check_drain_fc_complete(res->sr_session);
>> 	spin_unlock(&tbl->slot_tbl_lock);
>> 	res->sr_slot = NULL;
>> @@ -468,10 +627,8 @@ static int nfs41_sequence_done(struct rpc_task *task, struct nfs4_sequence_res *
>> 		 * returned NFS4ERR_DELAY as per Section 2.10.6.2
>> 		 * of RFC5661.
>> 		 */
>> -		dprintk("%s: slot=%td seq=%d: Operation in progress\n",
>> -			__func__,
>> -			res->sr_slot - res->sr_session->fc_slot_table.slots,
>> -			res->sr_slot->seq_nr);
>> +		dprintk("%s: slotid=%u seq=%d: Operation in progress\n",
>> +			__func__, res->sr_slot->slotid, res->sr_slot->seq_nr);
>> 		goto out_retry;
>> 	default:
>> 		/* Just update the slot sequence no. */
>> @@ -479,7 +636,8 @@ static int nfs41_sequence_done(struct rpc_task *task, struct nfs4_sequence_res *
>> 	}
>> out:
>> 	/* The session may be reset by one of the error handlers. */
>> -	dprintk("%s: Error %d free the slot \n", __func__, res->sr_status);
>> +	dprintk("%s: Error %d free the slot for task pid %5u\n", __func__,
>> +		res->sr_status, task->tk_pid);
>> 	nfs41_sequence_free_slot(res);
>> 	return 1;
>> out_retry:
>> @@ -540,7 +698,7 @@ int nfs41_setup_sequence(struct nfs4_session *session,
>> 	struct nfs4_slot_table *tbl;
>> 	u8 slotid;
>> 
>> -	dprintk("--> %s\n", __func__);
>> +	dprintk("--> %s task pid %5u\n", __func__, task->tk_pid);
>> 	/* slot already allocated? */
>> 	if (res->sr_slot != NULL)
>> 		return 0;
>> @@ -575,12 +733,24 @@ int nfs41_setup_sequence(struct nfs4_session *session,
>> 		dprintk("<-- %s: no free slots\n", __func__);
>> 		return -EAGAIN;
>> 	}
>> +	slot = nfs4_lookup_slot_locked(tbl, slotid);
>> +	if (slot == NULL) {
>> +		/* keep FIFO order */
>> +		rpc_task_set_priority(task, RPC_PRIORITY_PRIVILEGED);
>> +		rpc_sleep_on(&tbl->slot_tbl_waitq, task, NULL);
>> +		nfs4_free_slot(tbl, slotid);
> 
> why free the slot here?

We just grabbed the bit in nfs4_find_slot  - which is the "next available lowest slotid".  Since we don't actually have a slot because we need to allocate one, we free it here which allows for another request to grab the slot in between the time we give up the lock, allocate a slot, and call wake up to proceed.

-->Andy

> 
> Benny
> 
>> +		spin_unlock(&tbl->slot_tbl_lock);
>> +		dprintk("%s task pid %5u sleeping for dynamic slot %d\n",
>> +			__func__, task->tk_pid, slotid);
>> +		/* rpc_wakeup_next called upon success by slot allocator */
>> +		nfs4_dynamic_alloc_slot(tbl, slotid);
>> +		return -EAGAIN;
>> +	}
>> 	spin_unlock(&tbl->slot_tbl_lock);
>> 
>> 	rpc_task_set_priority(task, RPC_PRIORITY_NORMAL);
>> -	slot = tbl->slots + slotid;
>> 	args->sa_session = session;
>> -	args->sa_slotid = slotid;
>> +	args->sa_slot = slot;
>> 	args->sa_cache_this = cache_reply;
>> 
>> 	dprintk("<-- %s slotid=%d seqid=%d\n", __func__, slotid, slot->seq_nr);
>> @@ -613,9 +783,9 @@ int nfs4_setup_sequence(const struct nfs_server *server,
>> 		goto out;
>> 	}
>> 
>> -	dprintk("--> %s clp %p session %p sr_slot %td\n",
>> +	dprintk("--> %s clp %p session %p sr_slotid %d\n",
>> 		__func__, session->clp, session, res->sr_slot ?
>> -			res->sr_slot - session->fc_slot_table.slots : -1);
>> +			res->sr_slot->slotid : -1);
>> 
>> 	ret = nfs41_setup_sequence(session, args, res, cache_reply,
>> 				   task);
>> @@ -4976,84 +5146,64 @@ int nfs4_proc_get_lease_time(struct nfs_client *clp, struct nfs_fsinfo *fsinfo)
>> }
>> 
>> /*
>> - * Reset a slot table
>> + * Reset a slot table. Session has been drained and reset with new slot table
>> + * parameters from create_session.
>> + * Keep all allocated slots that fit into new slot table max_reqs parameter.
>>  */
>> -static int nfs4_reset_slot_table(struct nfs4_slot_table *tbl, u32 max_reqs,
>> +static void nfs4_reset_slot_table(struct nfs4_slot_table *tbl, u32 new_max_reqs,
>> 				 int ivalue)
>> {
>> -	struct nfs4_slot *new = NULL;
>> +	struct nfs4_slot *sp;
>> +	struct hlist_node *n;
>> 	int i;
>> -	int ret = 0;
>> -
>> -	dprintk("--> %s: max_reqs=%u, tbl->max_slots %d\n", __func__,
>> -		max_reqs, tbl->max_slots);
>> 
>> -	/* Does the newly negotiated max_reqs match the existing slot table? */
>> -	if (max_reqs != tbl->max_slots) {
>> -		ret = -ENOMEM;
>> -		new = kmalloc(max_reqs * sizeof(struct nfs4_slot),
>> -			      GFP_NOFS);
>> -		if (!new)
>> -			goto out;
>> -		ret = 0;
>> -		kfree(tbl->slots);
>> -	}
>> 	spin_lock(&tbl->slot_tbl_lock);
>> -	if (new) {
>> -		tbl->slots = new;
>> -		tbl->max_slots = max_reqs;
>> +	tbl->max_slots = new_max_reqs;
>> +	nfs4_reduce_slots_locked(tbl);
>> +	tbl->highest_used_slotid = -1;
>> +
>> +	for (i = 0; i < SLOT_TABLE_SIZE; i++) {
>> +		hlist_for_each_entry(sp, n, &tbl->slots[i], slot_node)
>> +			sp->seq_nr = ivalue;
>> 	}
>> -	for (i = 0; i < tbl->max_slots; ++i)
>> -		tbl->slots[i].seq_nr = ivalue;
>> 	spin_unlock(&tbl->slot_tbl_lock);
>> -	dprintk("%s: tbl=%p slots=%p max_slots=%d\n", __func__,
>> -		tbl, tbl->slots, tbl->max_slots);
>> -out:
>> -	dprintk("<-- %s: return %d\n", __func__, ret);
>> -	return ret;
>> +	dprintk("%s: tbl->max_slots=%d tbl->allocated_slots=%d\n",
>> +		__func__, tbl->max_slots, tbl->allocated_slots);
>> +	return;
>> }
>> 
>> /* Destroy the slot table */
>> static void nfs4_destroy_slot_tables(struct nfs4_session *session)
>> {
>> -	if (session->fc_slot_table.slots != NULL) {
>> -		kfree(session->fc_slot_table.slots);
>> -		session->fc_slot_table.slots = NULL;
>> -	}
>> -	if (session->bc_slot_table.slots != NULL) {
>> -		kfree(session->bc_slot_table.slots);
>> -		session->bc_slot_table.slots = NULL;
>> -	}
>> -	return;
>> +	dprintk("--> %s\n", __func__);
>> +
>> +	nfs4_free_all_slots(&session->fc_slot_table);
>> +	nfs4_free_all_slots(&session->bc_slot_table);
>> }
>> 
>> /*
>>  * Initialize slot table
>>  */
>> static int nfs4_init_slot_table(struct nfs4_slot_table *tbl,
>> -		int max_slots, int ivalue)
>> +		int max_slots, int num_prealloc, int ivalue)
>> {
>> -	struct nfs4_slot *slot;
>> -	int ret = -ENOMEM;
>> +	int ret;
>> 
>> 	BUG_ON(max_slots > NFS4_MAX_SLOT_TABLE);
>> -
>> -	dprintk("--> %s: max_reqs=%u\n", __func__, max_slots);
>> -
>> -	slot = kcalloc(max_slots, sizeof(struct nfs4_slot), GFP_NOFS);
>> -	if (!slot)
>> +	ret = nfs4_prealloc_slots(tbl, num_prealloc, ivalue);
>> +	if (ret) {
>> +		nfs4_free_all_slots(tbl);
>> 		goto out;
>> +	}
>> 	ret = 0;
>> 
>> 	spin_lock(&tbl->slot_tbl_lock);
>> 	tbl->max_slots = max_slots;
>> -	tbl->slots = slot;
>> 	tbl->highest_used_slotid = -1;  /* no slot is currently used */
>> 	spin_unlock(&tbl->slot_tbl_lock);
>> -	dprintk("%s: tbl=%p slots=%p max_slots=%d\n", __func__,
>> -		tbl, tbl->slots, tbl->max_slots);
>> +	dprintk("%s: tbl=%p max_slots=%d, tbl->allocated_slots=%d\n", __func__,
>> +		tbl, tbl->max_slots, tbl->allocated_slots);
>> out:
>> -	dprintk("<-- %s: return %d\n", __func__, ret);
>> 	return ret;
>> }
>> 
>> @@ -5063,30 +5213,34 @@ out:
>> static int nfs4_setup_session_slot_tables(struct nfs4_session *ses)
>> {
>> 	struct nfs4_slot_table *tbl;
>> -	int status;
>> +	int status = 0;
>> 
>> 	dprintk("--> %s\n", __func__);
>> 	/* Fore channel */
>> 	tbl = &ses->fc_slot_table;
>> -	if (tbl->slots == NULL) {
>> -		status = nfs4_init_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
>> +	if (tbl->allocated_slots == 0) {
>> +		status = nfs4_init_slot_table(tbl, ses->fc_attrs.max_reqs,
>> +				ses->clp->cl_rpcclient->cl_xprt->min_reqs, 1);
>> 		if (status) /* -ENOMEM */
>> 			return status;
>> 	} else {
>> -		status = nfs4_reset_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
>> -		if (status)
>> -			return status;
>> +		nfs4_reset_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
>> 	}
>> +
>> 	/* Back channel */
>> 	tbl = &ses->bc_slot_table;
>> -	if (tbl->slots == NULL) {
>> -		status = nfs4_init_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
>> +	if (tbl->allocated_slots == 0) {
>> +		status = nfs4_init_slot_table(tbl, ses->bc_attrs.max_reqs,
>> +				/* Backchannel max_reqs == 1 */
>> +				ses->bc_attrs.max_reqs, 0);
>> 		if (status)
>> 			/* Fore and back channel share a connection so get
>> 			 * both slot tables or neither */
>> 			nfs4_destroy_slot_tables(ses);
>> -	} else
>> -		status = nfs4_reset_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
>> +	} else {
>> +		nfs4_reset_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
>> +	}
>> +
>> 	return status;
>> }
>> 
>> @@ -5276,7 +5430,6 @@ int nfs4_proc_create_session(struct nfs_client *clp)
>> 
>> 	/* Init or reset the session slot tables */
>> 	status = nfs4_setup_session_slot_tables(session);
>> -	dprintk("slot table setup returned %d\n", status);
>> 	if (status)
>> 		goto out;
>> 
>> @@ -5284,7 +5437,7 @@ int nfs4_proc_create_session(struct nfs_client *clp)
>> 	dprintk("%s client>seqid %d sessionid %u:%u:%u:%u\n", __func__,
>> 		clp->cl_seqid, ptr[0], ptr[1], ptr[2], ptr[3]);
>> out:
>> -	dprintk("<-- %s\n", __func__);
>> +	dprintk("<-- %s status %d\n", __func__, status);
>> 	return status;
>> }
>> 
>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>> index 39914be..8260865 100644
>> --- a/fs/nfs/nfs4state.c
>> +++ b/fs/nfs/nfs4state.c
>> @@ -1573,26 +1573,16 @@ static int nfs4_recall_slot(struct nfs_client *clp)
>> {
>> 	struct nfs4_slot_table *fc_tbl = &clp->cl_session->fc_slot_table;
>> 	struct nfs4_channel_attrs *fc_attrs = &clp->cl_session->fc_attrs;
>> -	struct nfs4_slot *new, *old;
>> -	int i;
>> 
>> 	nfs4_begin_drain_session(clp);
>> -	new = kmalloc(fc_tbl->target_max_slots * sizeof(struct nfs4_slot),
>> -		      GFP_NOFS);
>> -        if (!new)
>> -		return -ENOMEM;
>> 
>> 	spin_lock(&fc_tbl->slot_tbl_lock);
>> -	for (i = 0; i < fc_tbl->target_max_slots; i++)
>> -		new[i].seq_nr = fc_tbl->slots[i].seq_nr;
>> -	old = fc_tbl->slots;
>> -	fc_tbl->slots = new;
>> 	fc_tbl->max_slots = fc_tbl->target_max_slots;
>> +	nfs4_reduce_slots_locked(fc_tbl);
>> 	fc_tbl->target_max_slots = 0;
>> 	fc_attrs->max_reqs = fc_tbl->max_slots;
>> 	spin_unlock(&fc_tbl->slot_tbl_lock);
>> 
>> -	kfree(old);
>> 	nfs4_end_drain_session(clp);
>> 	return 0;
>> }
>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
>> index e6161b2..c42ec29 100644
>> --- a/fs/nfs/nfs4xdr.c
>> +++ b/fs/nfs/nfs4xdr.c
>> @@ -1872,7 +1872,6 @@ static void encode_sequence(struct xdr_stream *xdr,
>> #if defined(CONFIG_NFS_V4_1)
>> 	struct nfs4_session *session = args->sa_session;
>> 	struct nfs4_slot_table *tp;
>> -	struct nfs4_slot *slot;
>> 	__be32 *p;
>> 
>> 	if (!session)
>> @@ -1880,8 +1879,7 @@ static void encode_sequence(struct xdr_stream *xdr,
>> 
>> 	tp = &session->fc_slot_table;
>> 
>> -	WARN_ON(args->sa_slotid == NFS4_MAX_SLOT_TABLE);
>> -	slot = tp->slots + args->sa_slotid;
>> +	WARN_ON(args->sa_slot->slotid == NFS4_MAX_SLOT_TABLE);
>> 
>> 	p = reserve_space(xdr, 4 + NFS4_MAX_SESSIONID_LEN + 16);
>> 	*p++ = cpu_to_be32(OP_SEQUENCE);
>> @@ -1896,11 +1894,11 @@ static void encode_sequence(struct xdr_stream *xdr,
>> 		((u32 *)session->sess_id.data)[1],
>> 		((u32 *)session->sess_id.data)[2],
>> 		((u32 *)session->sess_id.data)[3],
>> -		slot->seq_nr, args->sa_slotid,
>> +		args->sa_slot->seq_nr, args->sa_slot->slotid,
>> 		tp->highest_used_slotid, args->sa_cache_this);
>> 	p = xdr_encode_opaque_fixed(p, session->sess_id.data, NFS4_MAX_SESSIONID_LEN);
>> -	*p++ = cpu_to_be32(slot->seq_nr);
>> -	*p++ = cpu_to_be32(args->sa_slotid);
>> +	*p++ = cpu_to_be32(args->sa_slot->seq_nr);
>> +	*p++ = cpu_to_be32(args->sa_slot->slotid);
>> 	*p++ = cpu_to_be32(tp->highest_used_slotid);
>> 	*p = cpu_to_be32(args->sa_cache_this);
>> 	hdr->nops++;
>> @@ -5361,13 +5359,17 @@ static int decode_sequence(struct xdr_stream *xdr,
>> 	/* seqid */
>> 	dummy = be32_to_cpup(p++);
>> 	if (dummy != res->sr_slot->seq_nr) {
>> -		dprintk("%s Invalid sequence number\n", __func__);
>> +		dprintk("%s Invalid seq num %d for [slotid:seq_nr] [%d:%d]\n",
>> +			__func__, dummy, res->sr_slot->slotid,
>> +			res->sr_slot->seq_nr);
>> 		goto out_err;
>> 	}
>> 	/* slot id */
>> 	dummy = be32_to_cpup(p++);
>> -	if (dummy != res->sr_slot - res->sr_session->fc_slot_table.slots) {
>> -		dprintk("%s Invalid slot id\n", __func__);
>> +	if (dummy != res->sr_slot->slotid) {
>> +		dprintk("%s Invalid slot id %d for [slotid:seq_nr] [%d:%d]\n",
>> +			__func__, dummy, res->sr_slot->slotid,
>> +			res->sr_slot->seq_nr);
>> 		goto out_err;
>> 	}
>> 	/* highest slot id - currently not processed */
>> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
>> index b5479df..90c6e2b 100644
>> --- a/include/linux/nfs_fs_sb.h
>> +++ b/include/linux/nfs_fs_sb.h
>> @@ -193,12 +193,15 @@ struct nfs_server {
>> 
>> /* Sessions */
>> #define SLOT_TABLE_SZ (NFS4_MAX_SLOT_TABLE/(8*sizeof(long)))
>> +#define SLOT_TABLE_BITS                5
>> +#define SLOT_TABLE_SIZE                (1 << SLOT_TABLE_BITS)
>> struct nfs4_slot_table {
>> -	struct nfs4_slot *slots;		/* seqid per slot */
>> +	struct hlist_head slots[SLOT_TABLE_SIZE];
>> 	unsigned long   used_slots[SLOT_TABLE_SZ]; /* used/unused bitmap */
>> 	spinlock_t	slot_tbl_lock;
>> 	struct rpc_wait_queue	slot_tbl_waitq;	/* allocators may wait here */
>> -	int		max_slots;		/* # slots in table */
>> +	int		allocated_slots;	/* # slots in table */
>> +	int		max_slots;		/* max # slots in table */
>> 	int		highest_used_slotid;	/* sent to server on each SEQ.
>> 						 * op for dynamic resizing */
>> 	int		target_max_slots;	/* Set by CB_RECALL_SLOT as
>> @@ -206,11 +209,6 @@ struct nfs4_slot_table {
>> 	struct completion complete;
>> };
>> 
>> -static inline int slot_idx(struct nfs4_slot_table *tbl, struct nfs4_slot *sp)
>> -{
>> -	return sp - tbl->slots;
>> -}
>> -
>> /*
>>  * Session related parameters
>>  */
>> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
>> index c74595b..76b27a4 100644
>> --- a/include/linux/nfs_xdr.h
>> +++ b/include/linux/nfs_xdr.h
>> @@ -168,12 +168,14 @@ struct nfs4_channel_attrs {
>> 
>> /* nfs41 sessions slot seqid */
>> struct nfs4_slot {
>> +	struct hlist_node	slot_node;
>> 	u32		 	seq_nr;
>> +	u8			slotid;
>> };
>> 
>> struct nfs4_sequence_args {
>> 	struct nfs4_session	*sa_session;
>> -	u8			sa_slotid;
>> +	struct nfs4_slot        *sa_slot;
>> 	u8			sa_cache_this;
>> };
>> 

Re: [PATCH Version 4 4/7] NFSv4.1: dynamic session slots

[Benny Halevy]

On 2011-12-12 21:21, Adamson, Andy wrote:
> 
> On Dec 11, 2011, at 8:14 AM, Benny Halevy wrote:
> 
>> Hi Andy,
>>
>> I have some minor-ish comments inline below, but the whole
>> hlist concept looks to me like it might incur pretty high overhead
>> in cpu and memory caching.
> 
> You mean the hash lookup?
> 

hash lookup, list traversal, memory fragmentation...

>>
>> Have you considered simply reallocating the slot table on grow
>> and maybe on shrink below a certain threshold?
>>
>> On 2011-11-09 20:58, andros@netapp.com wrote:
>>> From: Andy Adamson <andros@netapp.com>
>>>
>>> The session fore/backchannel slot tables implementation is changed from a
>>> static array to an hlist hashed on slotid. Slotid's are values 0..N where N
>>> is initially negotiated between the client and server via the CREATE_SESSION
>>> call, and can be dynamically changed by the server during an active session.
>>>
>>> When there are no allocated slots (mount), an initial number of session slots
>>> is allocated equal to the inital number of dynamic rpc_slots
>>> created upon transport creation (xprt->min_reqs).  Slots are then dynamically
>>> allocated as needed upto the CREATE_SESSION negotiated max_reqs value.
>>>
>>> For session reset the session is drained (inactive). The (old) session has
>>> allocated slot tables which we will reuse. nfs4_reduce_slots_locked is called
>>> and only reduces the allocated_slots if the new max_reqs negotiated by the
>>> reset CREATE_SESSION is less than the number of allocated_slots. The resultant
>>> slot table is then re-initialized.
>>>
>>> CB_SLOT_RECALL is changed only in that it now shares code with session
>>> reset - it also calls nfs4_reduce_slots_locked.
>>>
>>> The backchannel uses the new dynamic slot allocator, even though we only
>>> currently support a single back channel slot.
>>>
>>> Signed-off-by: Andy Adamson <andros@netapp.com>
>>> ---
>>> fs/nfs/callback_proc.c    |   25 +++-
>>> fs/nfs/internal.h         |    2 +
>>> fs/nfs/nfs4_fs.h          |    1 +
>>> fs/nfs/nfs4proc.c         |  297 ++++++++++++++++++++++++++++++++++-----------
>>> fs/nfs/nfs4state.c        |   12 +--
>>> fs/nfs/nfs4xdr.c          |   20 ++--
>>> include/linux/nfs_fs_sb.h |   12 +-
>>> include/linux/nfs_xdr.h   |    4 +-
>>> 8 files changed, 266 insertions(+), 107 deletions(-)
>>>
>>> diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
>>> index 54cea8a..77cc96c 100644
>>> --- a/fs/nfs/callback_proc.c
>>> +++ b/fs/nfs/callback_proc.c
>>> @@ -342,7 +342,7 @@ validate_seqid(struct nfs4_slot_table *tbl, struct cb_sequenceargs * args)
>>> 	if (args->csa_slotid >= NFS41_BC_MAX_CALLBACKS)
>>> 		return htonl(NFS4ERR_BADSLOT);
>>>
>>> -	slot = tbl->slots + args->csa_slotid;
>>> +	slot = nfs4_lookup_slot_locked(tbl, args->csa_slotid);
>>> 	dprintk("%s slot table seqid: %d\n", __func__, slot->seq_nr);
>>>
>>> 	/* Normal */
>>> @@ -377,6 +377,22 @@ out_ok:
>>> 	return htonl(NFS4_OK);
>>> }
>>>
>>> +static bool test_slot_referring(struct nfs4_slot_table *tbl,
>>> +				struct referring_call *ref)
>>> +{
>>> +	struct nfs4_slot *slot;
>>> +	bool status = false;
>>> +
>>> +	spin_lock(&tbl->slot_tbl_lock);
>>> +	if (test_bit(ref->rc_slotid, tbl->used_slots)) {
>>> +		slot = nfs4_lookup_slot_locked(tbl, ref->rc_slotid);
>>> +		if (slot->seq_nr == ref->rc_sequenceid)
>>> +			status = true;
>>> +	}
>>> +	spin_unlock(&tbl->slot_tbl_lock);
>>> +	return status;
>>> +}
>>> +
>>> /*
>>>  * For each referring call triple, check the session's slot table for
>>>  * a match.  If the slot is in use and the sequence numbers match, the
>>> @@ -417,12 +433,7 @@ static bool referring_call_exists(struct nfs_client *clp,
>>> 				((u32 *)&rclist->rcl_sessionid.data)[2],
>>> 				((u32 *)&rclist->rcl_sessionid.data)[3],
>>> 				ref->rc_sequenceid, ref->rc_slotid);
>>> -
>>> -			spin_lock(&tbl->slot_tbl_lock);
>>> -			status = (test_bit(ref->rc_slotid, tbl->used_slots) &&
>>> -				  tbl->slots[ref->rc_slotid].seq_nr ==
>>> -					ref->rc_sequenceid);
>>> -			spin_unlock(&tbl->slot_tbl_lock);
>>> +			status = test_slot_referring(tbl, ref);
>>> 			if (status)
>>> 				goto out;
>>> 		}
>>> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
>>> index c1a1bd8..4451abf 100644
>>> --- a/fs/nfs/internal.h
>>> +++ b/fs/nfs/internal.h
>>> @@ -354,6 +354,8 @@ extern int _nfs4_call_sync_session(struct rpc_clnt *clnt,
>>> 				   struct nfs4_sequence_args *args,
>>> 				   struct nfs4_sequence_res *res,
>>> 				   int cache_reply);
>>> +struct nfs4_slot *nfs4_lookup_slot_locked(struct nfs4_slot_table *tbl,
>>> +					  u8 slotid);
>>>
>>> /*
>>>  * Determine the device name as a string
>>> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
>>> index 693ae22..ff87169 100644
>>> --- a/fs/nfs/nfs4_fs.h
>>> +++ b/fs/nfs/nfs4_fs.h
>>> @@ -242,6 +242,7 @@ extern struct nfs4_session *nfs4_alloc_session(struct nfs_client *clp);
>>> extern int nfs4_proc_create_session(struct nfs_client *);
>>> extern int nfs4_proc_destroy_session(struct nfs4_session *);
>>> extern int nfs4_init_session(struct nfs_server *server);
>>> +extern void nfs4_reduce_slots_locked(struct nfs4_slot_table *tbl);
>>> extern int nfs4_proc_get_lease_time(struct nfs_client *clp,
>>> 		struct nfs_fsinfo *fsinfo);
>>> extern int nfs4_proc_layoutcommit(struct nfs4_layoutcommit_data *data,
>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>> index 2638291..e3a7663 100644
>>> --- a/fs/nfs/nfs4proc.c
>>> +++ b/fs/nfs/nfs4proc.c
>>> @@ -346,6 +346,162 @@ static void renew_lease(const struct nfs_server *server, unsigned long timestamp
>>> #if defined(CONFIG_NFS_V4_1)
>>>
>>> /*
>>> + * Session dynamic slot table helper functions
>>> + */
>>> +
>>> +static inline u32 slot_tbl_hash(const u8 slotid)
>>
>> nit: "const" not really needed here…
> 
> OK
> 
>>
>>> +{
>>> +	return (u32)slotid % SLOT_TABLE_SIZE;
>>> +}
>>> +
>>> +static void nfs4_init_slot(struct nfs4_slot *new, u8 slotid, int ivalue)
>>> +{
>>> +	INIT_HLIST_NODE(&new->slot_node);
>>> +	new->slotid = slotid;
>>> +	new->seq_nr = ivalue;
>>> +}
>>> +
>>> +static void nfs4_insert_slot_locked(struct nfs4_slot_table *tbl,
>>> +				    struct nfs4_slot *new)
>>> +{
>>> +	u32 hash = slot_tbl_hash(new->slotid);
>>> +
>>> +	hlist_add_head(&new->slot_node, &tbl->slots[hash]);
>>> +	tbl->allocated_slots++;
>>> +}
>>> +
>>> +/*
>>> + * Allocate a slot for the next slotid (tbl->allocated_slotid)
>>> + */
>>> +static int nfs4_alloc_add_slot(struct nfs4_slot_table *tbl,
>>> +			       int ivalue, int slotid,
>>> +			       gfp_t gfp_flags)
>>> +{
>>> +	struct nfs4_slot *new;
>>> +	struct rpc_task *task;
>>> +
>>> +	dprintk("--> %s slotid=%u tbl->allocated_slots=%d\n", __func__,
>>> +		slotid, tbl->allocated_slots);
>>> +	new = kzalloc(sizeof(struct nfs4_slot), gfp_flags);
>>> +	if (!new)
>>> +		return -ENOMEM;
>>> +	/* tbl->allocated_slots holds the next unallocated slotid */
>>> +	nfs4_init_slot(new, slotid, ivalue);
>>> +	spin_lock(&tbl->slot_tbl_lock);
>>> +	if (tbl->allocated_slots != slotid) {
>>
>> This condition doesn't seem to be reliable.
>> What if the slot table shrunk?
> 
> tbl->allocated_slots is inc'ed/dec'ed under the slot_tbl_lock. If the slot table shrunk it's because nfs4_remove_slots_locked() was called which adjusts the allocated_slots accordingly
> 
> So I think it is absolutely reliable.
> 

It will detect that for sure.
I was concerned about the case where the caller tried to allocate
a new slot, we fail this test but return success (0)
and the table shrunk.  The caller won't find the slot in this case.
Therefore it seems unreliable to me - maybe better refer to
the return value as unreliable rather than the test itself.
I.e. it doesn't really indicate a success for alloc_add'ing the slot.

I see that in the dynamic case, you don't check the return value and return
-EAGAIN unconditionally and in the static case you just assume any failure
is due to -ENOMEM (again ignoring the returned status :).  It just goes
to tell you something's fishy about this function's interface to
the world ;-)

Benny

>>
>>> +		spin_unlock(&tbl->slot_tbl_lock);
>>> +		kfree(new);
>>> +		dprintk("%s slotid %u already allocated\n", __func__,
>>> +			slotid);
>>> +		return 0;
>>> +	}
>>> +	nfs4_insert_slot_locked(tbl, new);
>>> +
>>> +	/* This only applies to dynamic allocation */
>>> +	task = rpc_wake_up_next(&tbl->slot_tbl_waitq);
>>> +	if (task)
>>> +		dprintk("%s woke up task %5u\n", __func__, task->tk_pid);
>>> +	spin_unlock(&tbl->slot_tbl_lock);
>>> +	return 0;
>>> +}
>>> +
>>> +/*
>>> + * Allocate the minimal number of slots required by the transport.
>>> + * Called at session creation.
>>> + */
>>> +static int nfs4_prealloc_slots(struct nfs4_slot_table *tbl, int num, int ivalue)
>>> +{
>>> +	int i;
>>> +
>>> +	for (i = 0; i < num; i++) {
>>> +		if (nfs4_alloc_add_slot(tbl, ivalue, tbl->allocated_slots,
>>> +					GFP_NOFS) != 0)
>>> +			return -ENOMEM;
>>
>> Returning nfs4_alloc_add_slot's status would be more appropriate.
> 
> OK
> 
>> How about also freeing 0 to i-1 in the error case?
> 
> The transport minimum rpc_slots is the minimum number of (dynamic) rpc_slots needed to make progress. It's set to 2 for TCP.  We don't want to try and operate when we don't have the minimum number slots.
>>
>>> +	}
>>> +	return 0;
>>> +}
>>> +
>>> +/*
>>> + * Allocate the next slotid. If allocation fails, next request will retry.
>>> + */
>>> +static void nfs4_dynamic_alloc_slot(struct nfs4_slot_table *tbl, u8 slotid)
>>> +{
>>> +	if (slotid != tbl->allocated_slots) {
>>> +		dprintk("%s slotid %u already allocated\n", __func__, slotid);
>>> +		return;
>>> +	}
>>
>> This is checked again under the lock in nfs4_alloc_add_slot, no?
>> so if it is not supposed to be a common case (which I don't believe
>> it is) I'd just forget about this optimization and call the latter
>> directly.
> 
> Yeah, I agree.
>>
>>> +	nfs4_alloc_add_slot(tbl, 1, slotid, GFP_NOWAIT);
>>> +}
>>> +
>>> +/*
>>> + * Return the slot associated with the slotid returned by nfs4_find_slot.
>>> + * If the slot is not found, it has not been allocated.
>>> + */
>>> +struct nfs4_slot *
>>> +nfs4_lookup_slot_locked(struct nfs4_slot_table *tbl, u8 slotid)
>>> +{
>>> +	struct nfs4_slot *sp;
>>> +	struct hlist_node *n;
>>> +	u32 hash = slot_tbl_hash(slotid);
>>> +
>>> +	hlist_for_each_entry(sp, n, &tbl->slots[hash], slot_node) {
>>> +		if (sp->slotid == slotid)
>>> +			return sp;
>>> +	}
>>> +	return NULL;
>>> +}
>>> +
>>> +static void nfs4_remove_slot_locked(struct nfs4_slot_table *tbl,
>>> +				    struct nfs4_slot *slot)
>>> +{
>>> +	dprintk("--> %s slotid %d\n", __func__, slot->slotid);
>>> +	hlist_del_init(&slot->slot_node);
>>> +	tbl->allocated_slots--;
>>> +	kfree(slot);
>>> +}
>>> +
>>> +static void nfs4_free_all_slots(struct nfs4_slot_table *tbl)
>>> +{
>>> +	struct nfs4_slot *slotp;
>>> +	int i;
>>> +
>>> +	spin_lock(&tbl->slot_tbl_lock);
>>> +	for (i = 0; i < SLOT_TABLE_SIZE; i++) {
>>> +		while (!hlist_empty(&tbl->slots[i])) {
>>> +			slotp = hlist_entry(tbl->slots[i].first,
>>> +					    struct nfs4_slot, slot_node);
>>> +			nfs4_remove_slot_locked(tbl, slotp);
>>> +		}
>>> +		/* re-initialize hlist_head */
>>> +		tbl->slots[i].first = NULL;
>>> +	}
>>> +	spin_unlock(&tbl->slot_tbl_lock);
>>> +}
>>> +
>>> +/*
>>> + * Remove num_dec number of contiguous slotids starting from highest
>>> + * allocated slotid
>>> + *
>>> + * Note: set the new tbl->max_slots prior to calling.
>>> + */
>>> +void nfs4_reduce_slots_locked(struct nfs4_slot_table *tbl)
>>> +{
>>> +	struct nfs4_slot *removeme;
>>> +	int i, rm_slotid = tbl->allocated_slots - 1;
>>> +	int num_dec = tbl->allocated_slots - tbl->max_slots;
>>> +
>>> +	/* Reset of the slot table can make num_dec <= 0 */
>>> +	if (num_dec <= 0)
>>> +		return;
>>> +	for (i = 0; i < num_dec; i++) {
>>> +		removeme = nfs4_lookup_slot_locked(tbl, rm_slotid);
>>> +		BUG_ON(removeme == NULL);
>>> +		nfs4_remove_slot_locked(tbl, removeme);
>>> +		rm_slotid--;
>>> +	}
>>> +}
>>> +
>>> +/*
>>>  * nfs4_free_slot - free a slot and efficiently update slot table.
>>>  *
>>>  * freeing a slot is trivially done by clearing its respective bit
>>> @@ -390,8 +546,11 @@ static void nfs4_check_drain_fc_complete(struct nfs4_session *ses)
>>>
>>> 	if (!test_bit(NFS4_SESSION_DRAINING, &ses->session_state)) {
>>> 		task = rpc_wake_up_next(&ses->fc_slot_table.slot_tbl_waitq);
>>> -		if (task)
>>> +		if (task) {
>>> 			rpc_task_set_priority(task, RPC_PRIORITY_PRIVILEGED);
>>> +			dprintk("%s woke up task pid %5u\n", __func__,
>>> +				task->tk_pid);
>>> +		}
>>> 		return;
>>> 	}
>>>
>>> @@ -427,7 +586,7 @@ static void nfs41_sequence_free_slot(struct nfs4_sequence_res *res)
>>> 	}
>>>
>>> 	spin_lock(&tbl->slot_tbl_lock);
>>> -	nfs4_free_slot(tbl, res->sr_slot - tbl->slots);
>>> +	nfs4_free_slot(tbl, res->sr_slot->slotid);
>>> 	nfs4_check_drain_fc_complete(res->sr_session);
>>> 	spin_unlock(&tbl->slot_tbl_lock);
>>> 	res->sr_slot = NULL;
>>> @@ -468,10 +627,8 @@ static int nfs41_sequence_done(struct rpc_task *task, struct nfs4_sequence_res *
>>> 		 * returned NFS4ERR_DELAY as per Section 2.10.6.2
>>> 		 * of RFC5661.
>>> 		 */
>>> -		dprintk("%s: slot=%td seq=%d: Operation in progress\n",
>>> -			__func__,
>>> -			res->sr_slot - res->sr_session->fc_slot_table.slots,
>>> -			res->sr_slot->seq_nr);
>>> +		dprintk("%s: slotid=%u seq=%d: Operation in progress\n",
>>> +			__func__, res->sr_slot->slotid, res->sr_slot->seq_nr);
>>> 		goto out_retry;
>>> 	default:
>>> 		/* Just update the slot sequence no. */
>>> @@ -479,7 +636,8 @@ static int nfs41_sequence_done(struct rpc_task *task, struct nfs4_sequence_res *
>>> 	}
>>> out:
>>> 	/* The session may be reset by one of the error handlers. */
>>> -	dprintk("%s: Error %d free the slot \n", __func__, res->sr_status);
>>> +	dprintk("%s: Error %d free the slot for task pid %5u\n", __func__,
>>> +		res->sr_status, task->tk_pid);
>>> 	nfs41_sequence_free_slot(res);
>>> 	return 1;
>>> out_retry:
>>> @@ -540,7 +698,7 @@ int nfs41_setup_sequence(struct nfs4_session *session,
>>> 	struct nfs4_slot_table *tbl;
>>> 	u8 slotid;
>>>
>>> -	dprintk("--> %s\n", __func__);
>>> +	dprintk("--> %s task pid %5u\n", __func__, task->tk_pid);
>>> 	/* slot already allocated? */
>>> 	if (res->sr_slot != NULL)
>>> 		return 0;
>>> @@ -575,12 +733,24 @@ int nfs41_setup_sequence(struct nfs4_session *session,
>>> 		dprintk("<-- %s: no free slots\n", __func__);
>>> 		return -EAGAIN;
>>> 	}
>>> +	slot = nfs4_lookup_slot_locked(tbl, slotid);
>>> +	if (slot == NULL) {
>>> +		/* keep FIFO order */
>>> +		rpc_task_set_priority(task, RPC_PRIORITY_PRIVILEGED);
>>> +		rpc_sleep_on(&tbl->slot_tbl_waitq, task, NULL);
>>> +		nfs4_free_slot(tbl, slotid);
>>
>> why free the slot here?
> 
> We just grabbed the bit in nfs4_find_slot  - which is the "next available lowest slotid".  Since we don't actually have a slot because we need to allocate one, we free it here which allows for another request to grab the slot in between the time we give up the lock, allocate a slot, and call wake up to proceed.
> 
> -->Andy
> 
>>
>> Benny
>>
>>> +		spin_unlock(&tbl->slot_tbl_lock);
>>> +		dprintk("%s task pid %5u sleeping for dynamic slot %d\n",
>>> +			__func__, task->tk_pid, slotid);
>>> +		/* rpc_wakeup_next called upon success by slot allocator */
>>> +		nfs4_dynamic_alloc_slot(tbl, slotid);
>>> +		return -EAGAIN;
>>> +	}
>>> 	spin_unlock(&tbl->slot_tbl_lock);
>>>
>>> 	rpc_task_set_priority(task, RPC_PRIORITY_NORMAL);
>>> -	slot = tbl->slots + slotid;
>>> 	args->sa_session = session;
>>> -	args->sa_slotid = slotid;
>>> +	args->sa_slot = slot;
>>> 	args->sa_cache_this = cache_reply;
>>>
>>> 	dprintk("<-- %s slotid=%d seqid=%d\n", __func__, slotid, slot->seq_nr);
>>> @@ -613,9 +783,9 @@ int nfs4_setup_sequence(const struct nfs_server *server,
>>> 		goto out;
>>> 	}
>>>
>>> -	dprintk("--> %s clp %p session %p sr_slot %td\n",
>>> +	dprintk("--> %s clp %p session %p sr_slotid %d\n",
>>> 		__func__, session->clp, session, res->sr_slot ?
>>> -			res->sr_slot - session->fc_slot_table.slots : -1);
>>> +			res->sr_slot->slotid : -1);
>>>
>>> 	ret = nfs41_setup_sequence(session, args, res, cache_reply,
>>> 				   task);
>>> @@ -4976,84 +5146,64 @@ int nfs4_proc_get_lease_time(struct nfs_client *clp, struct nfs_fsinfo *fsinfo)
>>> }
>>>
>>> /*
>>> - * Reset a slot table
>>> + * Reset a slot table. Session has been drained and reset with new slot table
>>> + * parameters from create_session.
>>> + * Keep all allocated slots that fit into new slot table max_reqs parameter.
>>>  */
>>> -static int nfs4_reset_slot_table(struct nfs4_slot_table *tbl, u32 max_reqs,
>>> +static void nfs4_reset_slot_table(struct nfs4_slot_table *tbl, u32 new_max_reqs,
>>> 				 int ivalue)
>>> {
>>> -	struct nfs4_slot *new = NULL;
>>> +	struct nfs4_slot *sp;
>>> +	struct hlist_node *n;
>>> 	int i;
>>> -	int ret = 0;
>>> -
>>> -	dprintk("--> %s: max_reqs=%u, tbl->max_slots %d\n", __func__,
>>> -		max_reqs, tbl->max_slots);
>>>
>>> -	/* Does the newly negotiated max_reqs match the existing slot table? */
>>> -	if (max_reqs != tbl->max_slots) {
>>> -		ret = -ENOMEM;
>>> -		new = kmalloc(max_reqs * sizeof(struct nfs4_slot),
>>> -			      GFP_NOFS);
>>> -		if (!new)
>>> -			goto out;
>>> -		ret = 0;
>>> -		kfree(tbl->slots);
>>> -	}
>>> 	spin_lock(&tbl->slot_tbl_lock);
>>> -	if (new) {
>>> -		tbl->slots = new;
>>> -		tbl->max_slots = max_reqs;
>>> +	tbl->max_slots = new_max_reqs;
>>> +	nfs4_reduce_slots_locked(tbl);
>>> +	tbl->highest_used_slotid = -1;
>>> +
>>> +	for (i = 0; i < SLOT_TABLE_SIZE; i++) {
>>> +		hlist_for_each_entry(sp, n, &tbl->slots[i], slot_node)
>>> +			sp->seq_nr = ivalue;
>>> 	}
>>> -	for (i = 0; i < tbl->max_slots; ++i)
>>> -		tbl->slots[i].seq_nr = ivalue;
>>> 	spin_unlock(&tbl->slot_tbl_lock);
>>> -	dprintk("%s: tbl=%p slots=%p max_slots=%d\n", __func__,
>>> -		tbl, tbl->slots, tbl->max_slots);
>>> -out:
>>> -	dprintk("<-- %s: return %d\n", __func__, ret);
>>> -	return ret;
>>> +	dprintk("%s: tbl->max_slots=%d tbl->allocated_slots=%d\n",
>>> +		__func__, tbl->max_slots, tbl->allocated_slots);
>>> +	return;
>>> }
>>>
>>> /* Destroy the slot table */
>>> static void nfs4_destroy_slot_tables(struct nfs4_session *session)
>>> {
>>> -	if (session->fc_slot_table.slots != NULL) {
>>> -		kfree(session->fc_slot_table.slots);
>>> -		session->fc_slot_table.slots = NULL;
>>> -	}
>>> -	if (session->bc_slot_table.slots != NULL) {
>>> -		kfree(session->bc_slot_table.slots);
>>> -		session->bc_slot_table.slots = NULL;
>>> -	}
>>> -	return;
>>> +	dprintk("--> %s\n", __func__);
>>> +
>>> +	nfs4_free_all_slots(&session->fc_slot_table);
>>> +	nfs4_free_all_slots(&session->bc_slot_table);
>>> }
>>>
>>> /*
>>>  * Initialize slot table
>>>  */
>>> static int nfs4_init_slot_table(struct nfs4_slot_table *tbl,
>>> -		int max_slots, int ivalue)
>>> +		int max_slots, int num_prealloc, int ivalue)
>>> {
>>> -	struct nfs4_slot *slot;
>>> -	int ret = -ENOMEM;
>>> +	int ret;
>>>
>>> 	BUG_ON(max_slots > NFS4_MAX_SLOT_TABLE);
>>> -
>>> -	dprintk("--> %s: max_reqs=%u\n", __func__, max_slots);
>>> -
>>> -	slot = kcalloc(max_slots, sizeof(struct nfs4_slot), GFP_NOFS);
>>> -	if (!slot)
>>> +	ret = nfs4_prealloc_slots(tbl, num_prealloc, ivalue);
>>> +	if (ret) {
>>> +		nfs4_free_all_slots(tbl);
>>> 		goto out;
>>> +	}
>>> 	ret = 0;
>>>
>>> 	spin_lock(&tbl->slot_tbl_lock);
>>> 	tbl->max_slots = max_slots;
>>> -	tbl->slots = slot;
>>> 	tbl->highest_used_slotid = -1;  /* no slot is currently used */
>>> 	spin_unlock(&tbl->slot_tbl_lock);
>>> -	dprintk("%s: tbl=%p slots=%p max_slots=%d\n", __func__,
>>> -		tbl, tbl->slots, tbl->max_slots);
>>> +	dprintk("%s: tbl=%p max_slots=%d, tbl->allocated_slots=%d\n", __func__,
>>> +		tbl, tbl->max_slots, tbl->allocated_slots);
>>> out:
>>> -	dprintk("<-- %s: return %d\n", __func__, ret);
>>> 	return ret;
>>> }
>>>
>>> @@ -5063,30 +5213,34 @@ out:
>>> static int nfs4_setup_session_slot_tables(struct nfs4_session *ses)
>>> {
>>> 	struct nfs4_slot_table *tbl;
>>> -	int status;
>>> +	int status = 0;
>>>
>>> 	dprintk("--> %s\n", __func__);
>>> 	/* Fore channel */
>>> 	tbl = &ses->fc_slot_table;
>>> -	if (tbl->slots == NULL) {
>>> -		status = nfs4_init_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
>>> +	if (tbl->allocated_slots == 0) {
>>> +		status = nfs4_init_slot_table(tbl, ses->fc_attrs.max_reqs,
>>> +				ses->clp->cl_rpcclient->cl_xprt->min_reqs, 1);
>>> 		if (status) /* -ENOMEM */
>>> 			return status;
>>> 	} else {
>>> -		status = nfs4_reset_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
>>> -		if (status)
>>> -			return status;
>>> +		nfs4_reset_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
>>> 	}
>>> +
>>> 	/* Back channel */
>>> 	tbl = &ses->bc_slot_table;
>>> -	if (tbl->slots == NULL) {
>>> -		status = nfs4_init_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
>>> +	if (tbl->allocated_slots == 0) {
>>> +		status = nfs4_init_slot_table(tbl, ses->bc_attrs.max_reqs,
>>> +				/* Backchannel max_reqs == 1 */
>>> +				ses->bc_attrs.max_reqs, 0);
>>> 		if (status)
>>> 			/* Fore and back channel share a connection so get
>>> 			 * both slot tables or neither */
>>> 			nfs4_destroy_slot_tables(ses);
>>> -	} else
>>> -		status = nfs4_reset_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
>>> +	} else {
>>> +		nfs4_reset_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
>>> +	}
>>> +
>>> 	return status;
>>> }
>>>
>>> @@ -5276,7 +5430,6 @@ int nfs4_proc_create_session(struct nfs_client *clp)
>>>
>>> 	/* Init or reset the session slot tables */
>>> 	status = nfs4_setup_session_slot_tables(session);
>>> -	dprintk("slot table setup returned %d\n", status);
>>> 	if (status)
>>> 		goto out;
>>>
>>> @@ -5284,7 +5437,7 @@ int nfs4_proc_create_session(struct nfs_client *clp)
>>> 	dprintk("%s client>seqid %d sessionid %u:%u:%u:%u\n", __func__,
>>> 		clp->cl_seqid, ptr[0], ptr[1], ptr[2], ptr[3]);
>>> out:
>>> -	dprintk("<-- %s\n", __func__);
>>> +	dprintk("<-- %s status %d\n", __func__, status);
>>> 	return status;
>>> }
>>>
>>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>>> index 39914be..8260865 100644
>>> --- a/fs/nfs/nfs4state.c
>>> +++ b/fs/nfs/nfs4state.c
>>> @@ -1573,26 +1573,16 @@ static int nfs4_recall_slot(struct nfs_client *clp)
>>> {
>>> 	struct nfs4_slot_table *fc_tbl = &clp->cl_session->fc_slot_table;
>>> 	struct nfs4_channel_attrs *fc_attrs = &clp->cl_session->fc_attrs;
>>> -	struct nfs4_slot *new, *old;
>>> -	int i;
>>>
>>> 	nfs4_begin_drain_session(clp);
>>> -	new = kmalloc(fc_tbl->target_max_slots * sizeof(struct nfs4_slot),
>>> -		      GFP_NOFS);
>>> -        if (!new)
>>> -		return -ENOMEM;
>>>
>>> 	spin_lock(&fc_tbl->slot_tbl_lock);
>>> -	for (i = 0; i < fc_tbl->target_max_slots; i++)
>>> -		new[i].seq_nr = fc_tbl->slots[i].seq_nr;
>>> -	old = fc_tbl->slots;
>>> -	fc_tbl->slots = new;
>>> 	fc_tbl->max_slots = fc_tbl->target_max_slots;
>>> +	nfs4_reduce_slots_locked(fc_tbl);
>>> 	fc_tbl->target_max_slots = 0;
>>> 	fc_attrs->max_reqs = fc_tbl->max_slots;
>>> 	spin_unlock(&fc_tbl->slot_tbl_lock);
>>>
>>> -	kfree(old);
>>> 	nfs4_end_drain_session(clp);
>>> 	return 0;
>>> }
>>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
>>> index e6161b2..c42ec29 100644
>>> --- a/fs/nfs/nfs4xdr.c
>>> +++ b/fs/nfs/nfs4xdr.c
>>> @@ -1872,7 +1872,6 @@ static void encode_sequence(struct xdr_stream *xdr,
>>> #if defined(CONFIG_NFS_V4_1)
>>> 	struct nfs4_session *session = args->sa_session;
>>> 	struct nfs4_slot_table *tp;
>>> -	struct nfs4_slot *slot;
>>> 	__be32 *p;
>>>
>>> 	if (!session)
>>> @@ -1880,8 +1879,7 @@ static void encode_sequence(struct xdr_stream *xdr,
>>>
>>> 	tp = &session->fc_slot_table;
>>>
>>> -	WARN_ON(args->sa_slotid == NFS4_MAX_SLOT_TABLE);
>>> -	slot = tp->slots + args->sa_slotid;
>>> +	WARN_ON(args->sa_slot->slotid == NFS4_MAX_SLOT_TABLE);
>>>
>>> 	p = reserve_space(xdr, 4 + NFS4_MAX_SESSIONID_LEN + 16);
>>> 	*p++ = cpu_to_be32(OP_SEQUENCE);
>>> @@ -1896,11 +1894,11 @@ static void encode_sequence(struct xdr_stream *xdr,
>>> 		((u32 *)session->sess_id.data)[1],
>>> 		((u32 *)session->sess_id.data)[2],
>>> 		((u32 *)session->sess_id.data)[3],
>>> -		slot->seq_nr, args->sa_slotid,
>>> +		args->sa_slot->seq_nr, args->sa_slot->slotid,
>>> 		tp->highest_used_slotid, args->sa_cache_this);
>>> 	p = xdr_encode_opaque_fixed(p, session->sess_id.data, NFS4_MAX_SESSIONID_LEN);
>>> -	*p++ = cpu_to_be32(slot->seq_nr);
>>> -	*p++ = cpu_to_be32(args->sa_slotid);
>>> +	*p++ = cpu_to_be32(args->sa_slot->seq_nr);
>>> +	*p++ = cpu_to_be32(args->sa_slot->slotid);
>>> 	*p++ = cpu_to_be32(tp->highest_used_slotid);
>>> 	*p = cpu_to_be32(args->sa_cache_this);
>>> 	hdr->nops++;
>>> @@ -5361,13 +5359,17 @@ static int decode_sequence(struct xdr_stream *xdr,
>>> 	/* seqid */
>>> 	dummy = be32_to_cpup(p++);
>>> 	if (dummy != res->sr_slot->seq_nr) {
>>> -		dprintk("%s Invalid sequence number\n", __func__);
>>> +		dprintk("%s Invalid seq num %d for [slotid:seq_nr] [%d:%d]\n",
>>> +			__func__, dummy, res->sr_slot->slotid,
>>> +			res->sr_slot->seq_nr);
>>> 		goto out_err;
>>> 	}
>>> 	/* slot id */
>>> 	dummy = be32_to_cpup(p++);
>>> -	if (dummy != res->sr_slot - res->sr_session->fc_slot_table.slots) {
>>> -		dprintk("%s Invalid slot id\n", __func__);
>>> +	if (dummy != res->sr_slot->slotid) {
>>> +		dprintk("%s Invalid slot id %d for [slotid:seq_nr] [%d:%d]\n",
>>> +			__func__, dummy, res->sr_slot->slotid,
>>> +			res->sr_slot->seq_nr);
>>> 		goto out_err;
>>> 	}
>>> 	/* highest slot id - currently not processed */
>>> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
>>> index b5479df..90c6e2b 100644
>>> --- a/include/linux/nfs_fs_sb.h
>>> +++ b/include/linux/nfs_fs_sb.h
>>> @@ -193,12 +193,15 @@ struct nfs_server {
>>>
>>> /* Sessions */
>>> #define SLOT_TABLE_SZ (NFS4_MAX_SLOT_TABLE/(8*sizeof(long)))
>>> +#define SLOT_TABLE_BITS                5
>>> +#define SLOT_TABLE_SIZE                (1 << SLOT_TABLE_BITS)
>>> struct nfs4_slot_table {
>>> -	struct nfs4_slot *slots;		/* seqid per slot */
>>> +	struct hlist_head slots[SLOT_TABLE_SIZE];
>>> 	unsigned long   used_slots[SLOT_TABLE_SZ]; /* used/unused bitmap */
>>> 	spinlock_t	slot_tbl_lock;
>>> 	struct rpc_wait_queue	slot_tbl_waitq;	/* allocators may wait here */
>>> -	int		max_slots;		/* # slots in table */
>>> +	int		allocated_slots;	/* # slots in table */
>>> +	int		max_slots;		/* max # slots in table */
>>> 	int		highest_used_slotid;	/* sent to server on each SEQ.
>>> 						 * op for dynamic resizing */
>>> 	int		target_max_slots;	/* Set by CB_RECALL_SLOT as
>>> @@ -206,11 +209,6 @@ struct nfs4_slot_table {
>>> 	struct completion complete;
>>> };
>>>
>>> -static inline int slot_idx(struct nfs4_slot_table *tbl, struct nfs4_slot *sp)
>>> -{
>>> -	return sp - tbl->slots;
>>> -}
>>> -
>>> /*
>>>  * Session related parameters
>>>  */
>>> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
>>> index c74595b..76b27a4 100644
>>> --- a/include/linux/nfs_xdr.h
>>> +++ b/include/linux/nfs_xdr.h
>>> @@ -168,12 +168,14 @@ struct nfs4_channel_attrs {
>>>
>>> /* nfs41 sessions slot seqid */
>>> struct nfs4_slot {
>>> +	struct hlist_node	slot_node;
>>> 	u32		 	seq_nr;
>>> +	u8			slotid;
>>> };
>>>
>>> struct nfs4_sequence_args {
>>> 	struct nfs4_session	*sa_session;
>>> -	u8			sa_slotid;
>>> +	struct nfs4_slot        *sa_slot;
>>> 	u8			sa_cache_this;
>>> };
>>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH Version 4 4/7] NFSv4.1: dynamic session slots

[Adamson, Andy]

On Dec 12, 2011, at 3:13 PM, Benny Halevy wrote:

> On 2011-12-12 21:21, Adamson, Andy wrote:
>> 
>> On Dec 11, 2011, at 8:14 AM, Benny Halevy wrote:
>> 
>>> Hi Andy,
>>> 
>>> I have some minor-ish comments inline below, but the whole
>>> hlist concept looks to me like it might incur pretty high overhead
>>> in cpu and memory caching.
>> 
>> You mean the hash lookup?
>> 
> 
> hash lookup, list traversal, memory fragmentation…

I'll setup some tests. Note that the rpc_rqst, the rpc_slot need to be allocated as well, and that the tcp layer has a fair number of hlist's….

> 
>>> 
>>> Have you considered simply reallocating the slot table on grow
>>> and maybe on shrink below a certain threshold?
>>> 
>>> On 2011-11-09 20:58, andros@netapp.com wrote:
>>>> From: Andy Adamson <andros@netapp.com>
>>>> 
>>>> The session fore/backchannel slot tables implementation is changed from a
>>>> static array to an hlist hashed on slotid. Slotid's are values 0..N where N
>>>> is initially negotiated between the client and server via the CREATE_SESSION
>>>> call, and can be dynamically changed by the server during an active session.
>>>> 
>>>> When there are no allocated slots (mount), an initial number of session slots
>>>> is allocated equal to the inital number of dynamic rpc_slots
>>>> created upon transport creation (xprt->min_reqs).  Slots are then dynamically
>>>> allocated as needed upto the CREATE_SESSION negotiated max_reqs value.
>>>> 
>>>> For session reset the session is drained (inactive). The (old) session has
>>>> allocated slot tables which we will reuse. nfs4_reduce_slots_locked is called
>>>> and only reduces the allocated_slots if the new max_reqs negotiated by the
>>>> reset CREATE_SESSION is less than the number of allocated_slots. The resultant
>>>> slot table is then re-initialized.
>>>> 
>>>> CB_SLOT_RECALL is changed only in that it now shares code with session
>>>> reset - it also calls nfs4_reduce_slots_locked.
>>>> 
>>>> The backchannel uses the new dynamic slot allocator, even though we only
>>>> currently support a single back channel slot.
>>>> 
>>>> Signed-off-by: Andy Adamson <andros@netapp.com>
>>>> ---
>>>> fs/nfs/callback_proc.c    |   25 +++-
>>>> fs/nfs/internal.h         |    2 +
>>>> fs/nfs/nfs4_fs.h          |    1 +
>>>> fs/nfs/nfs4proc.c         |  297 ++++++++++++++++++++++++++++++++++-----------
>>>> fs/nfs/nfs4state.c        |   12 +--
>>>> fs/nfs/nfs4xdr.c          |   20 ++--
>>>> include/linux/nfs_fs_sb.h |   12 +-
>>>> include/linux/nfs_xdr.h   |    4 +-
>>>> 8 files changed, 266 insertions(+), 107 deletions(-)
>>>> 
>>>> diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
>>>> index 54cea8a..77cc96c 100644
>>>> --- a/fs/nfs/callback_proc.c
>>>> +++ b/fs/nfs/callback_proc.c
>>>> @@ -342,7 +342,7 @@ validate_seqid(struct nfs4_slot_table *tbl, struct cb_sequenceargs * args)
>>>> 	if (args->csa_slotid >= NFS41_BC_MAX_CALLBACKS)
>>>> 		return htonl(NFS4ERR_BADSLOT);
>>>> 
>>>> -	slot = tbl->slots + args->csa_slotid;
>>>> +	slot = nfs4_lookup_slot_locked(tbl, args->csa_slotid);
>>>> 	dprintk("%s slot table seqid: %d\n", __func__, slot->seq_nr);
>>>> 
>>>> 	/* Normal */
>>>> @@ -377,6 +377,22 @@ out_ok:
>>>> 	return htonl(NFS4_OK);
>>>> }
>>>> 
>>>> +static bool test_slot_referring(struct nfs4_slot_table *tbl,
>>>> +				struct referring_call *ref)
>>>> +{
>>>> +	struct nfs4_slot *slot;
>>>> +	bool status = false;
>>>> +
>>>> +	spin_lock(&tbl->slot_tbl_lock);
>>>> +	if (test_bit(ref->rc_slotid, tbl->used_slots)) {
>>>> +		slot = nfs4_lookup_slot_locked(tbl, ref->rc_slotid);
>>>> +		if (slot->seq_nr == ref->rc_sequenceid)
>>>> +			status = true;
>>>> +	}
>>>> +	spin_unlock(&tbl->slot_tbl_lock);
>>>> +	return status;
>>>> +}
>>>> +
>>>> /*
>>>> * For each referring call triple, check the session's slot table for
>>>> * a match.  If the slot is in use and the sequence numbers match, the
>>>> @@ -417,12 +433,7 @@ static bool referring_call_exists(struct nfs_client *clp,
>>>> 				((u32 *)&rclist->rcl_sessionid.data)[2],
>>>> 				((u32 *)&rclist->rcl_sessionid.data)[3],
>>>> 				ref->rc_sequenceid, ref->rc_slotid);
>>>> -
>>>> -			spin_lock(&tbl->slot_tbl_lock);
>>>> -			status = (test_bit(ref->rc_slotid, tbl->used_slots) &&
>>>> -				  tbl->slots[ref->rc_slotid].seq_nr ==
>>>> -					ref->rc_sequenceid);
>>>> -			spin_unlock(&tbl->slot_tbl_lock);
>>>> +			status = test_slot_referring(tbl, ref);
>>>> 			if (status)
>>>> 				goto out;
>>>> 		}
>>>> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
>>>> index c1a1bd8..4451abf 100644
>>>> --- a/fs/nfs/internal.h
>>>> +++ b/fs/nfs/internal.h
>>>> @@ -354,6 +354,8 @@ extern int _nfs4_call_sync_session(struct rpc_clnt *clnt,
>>>> 				   struct nfs4_sequence_args *args,
>>>> 				   struct nfs4_sequence_res *res,
>>>> 				   int cache_reply);
>>>> +struct nfs4_slot *nfs4_lookup_slot_locked(struct nfs4_slot_table *tbl,
>>>> +					  u8 slotid);
>>>> 
>>>> /*
>>>> * Determine the device name as a string
>>>> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
>>>> index 693ae22..ff87169 100644
>>>> --- a/fs/nfs/nfs4_fs.h
>>>> +++ b/fs/nfs/nfs4_fs.h
>>>> @@ -242,6 +242,7 @@ extern struct nfs4_session *nfs4_alloc_session(struct nfs_client *clp);
>>>> extern int nfs4_proc_create_session(struct nfs_client *);
>>>> extern int nfs4_proc_destroy_session(struct nfs4_session *);
>>>> extern int nfs4_init_session(struct nfs_server *server);
>>>> +extern void nfs4_reduce_slots_locked(struct nfs4_slot_table *tbl);
>>>> extern int nfs4_proc_get_lease_time(struct nfs_client *clp,
>>>> 		struct nfs_fsinfo *fsinfo);
>>>> extern int nfs4_proc_layoutcommit(struct nfs4_layoutcommit_data *data,
>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>>> index 2638291..e3a7663 100644
>>>> --- a/fs/nfs/nfs4proc.c
>>>> +++ b/fs/nfs/nfs4proc.c
>>>> @@ -346,6 +346,162 @@ static void renew_lease(const struct nfs_server *server, unsigned long timestamp
>>>> #if defined(CONFIG_NFS_V4_1)
>>>> 
>>>> /*
>>>> + * Session dynamic slot table helper functions
>>>> + */
>>>> +
>>>> +static inline u32 slot_tbl_hash(const u8 slotid)
>>> 
>>> nit: "const" not really needed here…
>> 
>> OK
>> 
>>> 
>>>> +{
>>>> +	return (u32)slotid % SLOT_TABLE_SIZE;
>>>> +}
>>>> +
>>>> +static void nfs4_init_slot(struct nfs4_slot *new, u8 slotid, int ivalue)
>>>> +{
>>>> +	INIT_HLIST_NODE(&new->slot_node);
>>>> +	new->slotid = slotid;
>>>> +	new->seq_nr = ivalue;
>>>> +}
>>>> +
>>>> +static void nfs4_insert_slot_locked(struct nfs4_slot_table *tbl,
>>>> +				    struct nfs4_slot *new)
>>>> +{
>>>> +	u32 hash = slot_tbl_hash(new->slotid);
>>>> +
>>>> +	hlist_add_head(&new->slot_node, &tbl->slots[hash]);
>>>> +	tbl->allocated_slots++;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Allocate a slot for the next slotid (tbl->allocated_slotid)
>>>> + */
>>>> +static int nfs4_alloc_add_slot(struct nfs4_slot_table *tbl,
>>>> +			       int ivalue, int slotid,
>>>> +			       gfp_t gfp_flags)
>>>> +{
>>>> +	struct nfs4_slot *new;
>>>> +	struct rpc_task *task;
>>>> +
>>>> +	dprintk("--> %s slotid=%u tbl->allocated_slots=%d\n", __func__,
>>>> +		slotid, tbl->allocated_slots);
>>>> +	new = kzalloc(sizeof(struct nfs4_slot), gfp_flags);
>>>> +	if (!new)
>>>> +		return -ENOMEM;
>>>> +	/* tbl->allocated_slots holds the next unallocated slotid */
>>>> +	nfs4_init_slot(new, slotid, ivalue);
>>>> +	spin_lock(&tbl->slot_tbl_lock);
>>>> +	if (tbl->allocated_slots != slotid) {
>>> 
>>> This condition doesn't seem to be reliable.
>>> What if the slot table shrunk?
>> 
>> tbl->allocated_slots is inc'ed/dec'ed under the slot_tbl_lock. If the slot table shrunk it's because nfs4_remove_slots_locked() was called which adjusts the allocated_slots accordingly
>> 
>> So I think it is absolutely reliable.
>> 
> 
> It will detect that for sure.
> I was concerned about the case where the caller tried to allocate
> a new slot, we fail this test but return success (0)
> and the table shrunk.  The caller won't find the slot in this case.
> Therefore it seems unreliable to me - maybe better refer to
> the return value as unreliable rather than the test itself.
> I.e. it doesn't really indicate a success for alloc_add'ing the slot.
> 
> I see that in the dynamic case, you don't check the return value and return
> -EAGAIN unconditionally and in the static case you just assume any failure
> is due to -ENOMEM (again ignoring the returned status :).  It just goes
> to tell you something's fishy about this function's interface to
> the world ;-)

I see your point. Could be a lot cleaner. I'll refactor

Thanks for the review :)

-->Andy
> 
> Benny
> 
>>> 
>>>> +		spin_unlock(&tbl->slot_tbl_lock);
>>>> +		kfree(new);
>>>> +		dprintk("%s slotid %u already allocated\n", __func__,
>>>> +			slotid);
>>>> +		return 0;
>>>> +	}
>>>> +	nfs4_insert_slot_locked(tbl, new);
>>>> +
>>>> +	/* This only applies to dynamic allocation */
>>>> +	task = rpc_wake_up_next(&tbl->slot_tbl_waitq);
>>>> +	if (task)
>>>> +		dprintk("%s woke up task %5u\n", __func__, task->tk_pid);
>>>> +	spin_unlock(&tbl->slot_tbl_lock);
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Allocate the minimal number of slots required by the transport.
>>>> + * Called at session creation.
>>>> + */
>>>> +static int nfs4_prealloc_slots(struct nfs4_slot_table *tbl, int num, int ivalue)
>>>> +{
>>>> +	int i;
>>>> +
>>>> +	for (i = 0; i < num; i++) {
>>>> +		if (nfs4_alloc_add_slot(tbl, ivalue, tbl->allocated_slots,
>>>> +					GFP_NOFS) != 0)
>>>> +			return -ENOMEM;
>>> 
>>> Returning nfs4_alloc_add_slot's status would be more appropriate.
>> 
>> OK
>> 
>>> How about also freeing 0 to i-1 in the error case?
>> 
>> The transport minimum rpc_slots is the minimum number of (dynamic) rpc_slots needed to make progress. It's set to 2 for TCP.  We don't want to try and operate when we don't have the minimum number slots.
>>> 
>>>> +	}
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Allocate the next slotid. If allocation fails, next request will retry.
>>>> + */
>>>> +static void nfs4_dynamic_alloc_slot(struct nfs4_slot_table *tbl, u8 slotid)
>>>> +{
>>>> +	if (slotid != tbl->allocated_slots) {
>>>> +		dprintk("%s slotid %u already allocated\n", __func__, slotid);
>>>> +		return;
>>>> +	}
>>> 
>>> This is checked again under the lock in nfs4_alloc_add_slot, no?
>>> so if it is not supposed to be a common case (which I don't believe
>>> it is) I'd just forget about this optimization and call the latter
>>> directly.
>> 
>> Yeah, I agree.
>>> 
>>>> +	nfs4_alloc_add_slot(tbl, 1, slotid, GFP_NOWAIT);
>>>> +}
>>>> +
>>>> +/*
>>>> + * Return the slot associated with the slotid returned by nfs4_find_slot.
>>>> + * If the slot is not found, it has not been allocated.
>>>> + */
>>>> +struct nfs4_slot *
>>>> +nfs4_lookup_slot_locked(struct nfs4_slot_table *tbl, u8 slotid)
>>>> +{
>>>> +	struct nfs4_slot *sp;
>>>> +	struct hlist_node *n;
>>>> +	u32 hash = slot_tbl_hash(slotid);
>>>> +
>>>> +	hlist_for_each_entry(sp, n, &tbl->slots[hash], slot_node) {
>>>> +		if (sp->slotid == slotid)
>>>> +			return sp;
>>>> +	}
>>>> +	return NULL;
>>>> +}
>>>> +
>>>> +static void nfs4_remove_slot_locked(struct nfs4_slot_table *tbl,
>>>> +				    struct nfs4_slot *slot)
>>>> +{
>>>> +	dprintk("--> %s slotid %d\n", __func__, slot->slotid);
>>>> +	hlist_del_init(&slot->slot_node);
>>>> +	tbl->allocated_slots--;
>>>> +	kfree(slot);
>>>> +}
>>>> +
>>>> +static void nfs4_free_all_slots(struct nfs4_slot_table *tbl)
>>>> +{
>>>> +	struct nfs4_slot *slotp;
>>>> +	int i;
>>>> +
>>>> +	spin_lock(&tbl->slot_tbl_lock);
>>>> +	for (i = 0; i < SLOT_TABLE_SIZE; i++) {
>>>> +		while (!hlist_empty(&tbl->slots[i])) {
>>>> +			slotp = hlist_entry(tbl->slots[i].first,
>>>> +					    struct nfs4_slot, slot_node);
>>>> +			nfs4_remove_slot_locked(tbl, slotp);
>>>> +		}
>>>> +		/* re-initialize hlist_head */
>>>> +		tbl->slots[i].first = NULL;
>>>> +	}
>>>> +	spin_unlock(&tbl->slot_tbl_lock);
>>>> +}
>>>> +
>>>> +/*
>>>> + * Remove num_dec number of contiguous slotids starting from highest
>>>> + * allocated slotid
>>>> + *
>>>> + * Note: set the new tbl->max_slots prior to calling.
>>>> + */
>>>> +void nfs4_reduce_slots_locked(struct nfs4_slot_table *tbl)
>>>> +{
>>>> +	struct nfs4_slot *removeme;
>>>> +	int i, rm_slotid = tbl->allocated_slots - 1;
>>>> +	int num_dec = tbl->allocated_slots - tbl->max_slots;
>>>> +
>>>> +	/* Reset of the slot table can make num_dec <= 0 */
>>>> +	if (num_dec <= 0)
>>>> +		return;
>>>> +	for (i = 0; i < num_dec; i++) {
>>>> +		removeme = nfs4_lookup_slot_locked(tbl, rm_slotid);
>>>> +		BUG_ON(removeme == NULL);
>>>> +		nfs4_remove_slot_locked(tbl, removeme);
>>>> +		rm_slotid--;
>>>> +	}
>>>> +}
>>>> +
>>>> +/*
>>>> * nfs4_free_slot - free a slot and efficiently update slot table.
>>>> *
>>>> * freeing a slot is trivially done by clearing its respective bit
>>>> @@ -390,8 +546,11 @@ static void nfs4_check_drain_fc_complete(struct nfs4_session *ses)
>>>> 
>>>> 	if (!test_bit(NFS4_SESSION_DRAINING, &ses->session_state)) {
>>>> 		task = rpc_wake_up_next(&ses->fc_slot_table.slot_tbl_waitq);
>>>> -		if (task)
>>>> +		if (task) {
>>>> 			rpc_task_set_priority(task, RPC_PRIORITY_PRIVILEGED);
>>>> +			dprintk("%s woke up task pid %5u\n", __func__,
>>>> +				task->tk_pid);
>>>> +		}
>>>> 		return;
>>>> 	}
>>>> 
>>>> @@ -427,7 +586,7 @@ static void nfs41_sequence_free_slot(struct nfs4_sequence_res *res)
>>>> 	}
>>>> 
>>>> 	spin_lock(&tbl->slot_tbl_lock);
>>>> -	nfs4_free_slot(tbl, res->sr_slot - tbl->slots);
>>>> +	nfs4_free_slot(tbl, res->sr_slot->slotid);
>>>> 	nfs4_check_drain_fc_complete(res->sr_session);
>>>> 	spin_unlock(&tbl->slot_tbl_lock);
>>>> 	res->sr_slot = NULL;
>>>> @@ -468,10 +627,8 @@ static int nfs41_sequence_done(struct rpc_task *task, struct nfs4_sequence_res *
>>>> 		 * returned NFS4ERR_DELAY as per Section 2.10.6.2
>>>> 		 * of RFC5661.
>>>> 		 */
>>>> -		dprintk("%s: slot=%td seq=%d: Operation in progress\n",
>>>> -			__func__,
>>>> -			res->sr_slot - res->sr_session->fc_slot_table.slots,
>>>> -			res->sr_slot->seq_nr);
>>>> +		dprintk("%s: slotid=%u seq=%d: Operation in progress\n",
>>>> +			__func__, res->sr_slot->slotid, res->sr_slot->seq_nr);
>>>> 		goto out_retry;
>>>> 	default:
>>>> 		/* Just update the slot sequence no. */
>>>> @@ -479,7 +636,8 @@ static int nfs41_sequence_done(struct rpc_task *task, struct nfs4_sequence_res *
>>>> 	}
>>>> out:
>>>> 	/* The session may be reset by one of the error handlers. */
>>>> -	dprintk("%s: Error %d free the slot \n", __func__, res->sr_status);
>>>> +	dprintk("%s: Error %d free the slot for task pid %5u\n", __func__,
>>>> +		res->sr_status, task->tk_pid);
>>>> 	nfs41_sequence_free_slot(res);
>>>> 	return 1;
>>>> out_retry:
>>>> @@ -540,7 +698,7 @@ int nfs41_setup_sequence(struct nfs4_session *session,
>>>> 	struct nfs4_slot_table *tbl;
>>>> 	u8 slotid;
>>>> 
>>>> -	dprintk("--> %s\n", __func__);
>>>> +	dprintk("--> %s task pid %5u\n", __func__, task->tk_pid);
>>>> 	/* slot already allocated? */
>>>> 	if (res->sr_slot != NULL)
>>>> 		return 0;
>>>> @@ -575,12 +733,24 @@ int nfs41_setup_sequence(struct nfs4_session *session,
>>>> 		dprintk("<-- %s: no free slots\n", __func__);
>>>> 		return -EAGAIN;
>>>> 	}
>>>> +	slot = nfs4_lookup_slot_locked(tbl, slotid);
>>>> +	if (slot == NULL) {
>>>> +		/* keep FIFO order */
>>>> +		rpc_task_set_priority(task, RPC_PRIORITY_PRIVILEGED);
>>>> +		rpc_sleep_on(&tbl->slot_tbl_waitq, task, NULL);
>>>> +		nfs4_free_slot(tbl, slotid);
>>> 
>>> why free the slot here?
>> 
>> We just grabbed the bit in nfs4_find_slot  - which is the "next available lowest slotid".  Since we don't actually have a slot because we need to allocate one, we free it here which allows for another request to grab the slot in between the time we give up the lock, allocate a slot, and call wake up to proceed.
>> 
>> -->Andy
>> 
>>> 
>>> Benny
>>> 
>>>> +		spin_unlock(&tbl->slot_tbl_lock);
>>>> +		dprintk("%s task pid %5u sleeping for dynamic slot %d\n",
>>>> +			__func__, task->tk_pid, slotid);
>>>> +		/* rpc_wakeup_next called upon success by slot allocator */
>>>> +		nfs4_dynamic_alloc_slot(tbl, slotid);
>>>> +		return -EAGAIN;
>>>> +	}
>>>> 	spin_unlock(&tbl->slot_tbl_lock);
>>>> 
>>>> 	rpc_task_set_priority(task, RPC_PRIORITY_NORMAL);
>>>> -	slot = tbl->slots + slotid;
>>>> 	args->sa_session = session;
>>>> -	args->sa_slotid = slotid;
>>>> +	args->sa_slot = slot;
>>>> 	args->sa_cache_this = cache_reply;
>>>> 
>>>> 	dprintk("<-- %s slotid=%d seqid=%d\n", __func__, slotid, slot->seq_nr);
>>>> @@ -613,9 +783,9 @@ int nfs4_setup_sequence(const struct nfs_server *server,
>>>> 		goto out;
>>>> 	}
>>>> 
>>>> -	dprintk("--> %s clp %p session %p sr_slot %td\n",
>>>> +	dprintk("--> %s clp %p session %p sr_slotid %d\n",
>>>> 		__func__, session->clp, session, res->sr_slot ?
>>>> -			res->sr_slot - session->fc_slot_table.slots : -1);
>>>> +			res->sr_slot->slotid : -1);
>>>> 
>>>> 	ret = nfs41_setup_sequence(session, args, res, cache_reply,
>>>> 				   task);
>>>> @@ -4976,84 +5146,64 @@ int nfs4_proc_get_lease_time(struct nfs_client *clp, struct nfs_fsinfo *fsinfo)
>>>> }
>>>> 
>>>> /*
>>>> - * Reset a slot table
>>>> + * Reset a slot table. Session has been drained and reset with new slot table
>>>> + * parameters from create_session.
>>>> + * Keep all allocated slots that fit into new slot table max_reqs parameter.
>>>> */
>>>> -static int nfs4_reset_slot_table(struct nfs4_slot_table *tbl, u32 max_reqs,
>>>> +static void nfs4_reset_slot_table(struct nfs4_slot_table *tbl, u32 new_max_reqs,
>>>> 				 int ivalue)
>>>> {
>>>> -	struct nfs4_slot *new = NULL;
>>>> +	struct nfs4_slot *sp;
>>>> +	struct hlist_node *n;
>>>> 	int i;
>>>> -	int ret = 0;
>>>> -
>>>> -	dprintk("--> %s: max_reqs=%u, tbl->max_slots %d\n", __func__,
>>>> -		max_reqs, tbl->max_slots);
>>>> 
>>>> -	/* Does the newly negotiated max_reqs match the existing slot table? */
>>>> -	if (max_reqs != tbl->max_slots) {
>>>> -		ret = -ENOMEM;
>>>> -		new = kmalloc(max_reqs * sizeof(struct nfs4_slot),
>>>> -			      GFP_NOFS);
>>>> -		if (!new)
>>>> -			goto out;
>>>> -		ret = 0;
>>>> -		kfree(tbl->slots);
>>>> -	}
>>>> 	spin_lock(&tbl->slot_tbl_lock);
>>>> -	if (new) {
>>>> -		tbl->slots = new;
>>>> -		tbl->max_slots = max_reqs;
>>>> +	tbl->max_slots = new_max_reqs;
>>>> +	nfs4_reduce_slots_locked(tbl);
>>>> +	tbl->highest_used_slotid = -1;
>>>> +
>>>> +	for (i = 0; i < SLOT_TABLE_SIZE; i++) {
>>>> +		hlist_for_each_entry(sp, n, &tbl->slots[i], slot_node)
>>>> +			sp->seq_nr = ivalue;
>>>> 	}
>>>> -	for (i = 0; i < tbl->max_slots; ++i)
>>>> -		tbl->slots[i].seq_nr = ivalue;
>>>> 	spin_unlock(&tbl->slot_tbl_lock);
>>>> -	dprintk("%s: tbl=%p slots=%p max_slots=%d\n", __func__,
>>>> -		tbl, tbl->slots, tbl->max_slots);
>>>> -out:
>>>> -	dprintk("<-- %s: return %d\n", __func__, ret);
>>>> -	return ret;
>>>> +	dprintk("%s: tbl->max_slots=%d tbl->allocated_slots=%d\n",
>>>> +		__func__, tbl->max_slots, tbl->allocated_slots);
>>>> +	return;
>>>> }
>>>> 
>>>> /* Destroy the slot table */
>>>> static void nfs4_destroy_slot_tables(struct nfs4_session *session)
>>>> {
>>>> -	if (session->fc_slot_table.slots != NULL) {
>>>> -		kfree(session->fc_slot_table.slots);
>>>> -		session->fc_slot_table.slots = NULL;
>>>> -	}
>>>> -	if (session->bc_slot_table.slots != NULL) {
>>>> -		kfree(session->bc_slot_table.slots);
>>>> -		session->bc_slot_table.slots = NULL;
>>>> -	}
>>>> -	return;
>>>> +	dprintk("--> %s\n", __func__);
>>>> +
>>>> +	nfs4_free_all_slots(&session->fc_slot_table);
>>>> +	nfs4_free_all_slots(&session->bc_slot_table);
>>>> }
>>>> 
>>>> /*
>>>> * Initialize slot table
>>>> */
>>>> static int nfs4_init_slot_table(struct nfs4_slot_table *tbl,
>>>> -		int max_slots, int ivalue)
>>>> +		int max_slots, int num_prealloc, int ivalue)
>>>> {
>>>> -	struct nfs4_slot *slot;
>>>> -	int ret = -ENOMEM;
>>>> +	int ret;
>>>> 
>>>> 	BUG_ON(max_slots > NFS4_MAX_SLOT_TABLE);
>>>> -
>>>> -	dprintk("--> %s: max_reqs=%u\n", __func__, max_slots);
>>>> -
>>>> -	slot = kcalloc(max_slots, sizeof(struct nfs4_slot), GFP_NOFS);
>>>> -	if (!slot)
>>>> +	ret = nfs4_prealloc_slots(tbl, num_prealloc, ivalue);
>>>> +	if (ret) {
>>>> +		nfs4_free_all_slots(tbl);
>>>> 		goto out;
>>>> +	}
>>>> 	ret = 0;
>>>> 
>>>> 	spin_lock(&tbl->slot_tbl_lock);
>>>> 	tbl->max_slots = max_slots;
>>>> -	tbl->slots = slot;
>>>> 	tbl->highest_used_slotid = -1;  /* no slot is currently used */
>>>> 	spin_unlock(&tbl->slot_tbl_lock);
>>>> -	dprintk("%s: tbl=%p slots=%p max_slots=%d\n", __func__,
>>>> -		tbl, tbl->slots, tbl->max_slots);
>>>> +	dprintk("%s: tbl=%p max_slots=%d, tbl->allocated_slots=%d\n", __func__,
>>>> +		tbl, tbl->max_slots, tbl->allocated_slots);
>>>> out:
>>>> -	dprintk("<-- %s: return %d\n", __func__, ret);
>>>> 	return ret;
>>>> }
>>>> 
>>>> @@ -5063,30 +5213,34 @@ out:
>>>> static int nfs4_setup_session_slot_tables(struct nfs4_session *ses)
>>>> {
>>>> 	struct nfs4_slot_table *tbl;
>>>> -	int status;
>>>> +	int status = 0;
>>>> 
>>>> 	dprintk("--> %s\n", __func__);
>>>> 	/* Fore channel */
>>>> 	tbl = &ses->fc_slot_table;
>>>> -	if (tbl->slots == NULL) {
>>>> -		status = nfs4_init_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
>>>> +	if (tbl->allocated_slots == 0) {
>>>> +		status = nfs4_init_slot_table(tbl, ses->fc_attrs.max_reqs,
>>>> +				ses->clp->cl_rpcclient->cl_xprt->min_reqs, 1);
>>>> 		if (status) /* -ENOMEM */
>>>> 			return status;
>>>> 	} else {
>>>> -		status = nfs4_reset_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
>>>> -		if (status)
>>>> -			return status;
>>>> +		nfs4_reset_slot_table(tbl, ses->fc_attrs.max_reqs, 1);
>>>> 	}
>>>> +
>>>> 	/* Back channel */
>>>> 	tbl = &ses->bc_slot_table;
>>>> -	if (tbl->slots == NULL) {
>>>> -		status = nfs4_init_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
>>>> +	if (tbl->allocated_slots == 0) {
>>>> +		status = nfs4_init_slot_table(tbl, ses->bc_attrs.max_reqs,
>>>> +				/* Backchannel max_reqs == 1 */
>>>> +				ses->bc_attrs.max_reqs, 0);
>>>> 		if (status)
>>>> 			/* Fore and back channel share a connection so get
>>>> 			 * both slot tables or neither */
>>>> 			nfs4_destroy_slot_tables(ses);
>>>> -	} else
>>>> -		status = nfs4_reset_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
>>>> +	} else {
>>>> +		nfs4_reset_slot_table(tbl, ses->bc_attrs.max_reqs, 0);
>>>> +	}
>>>> +
>>>> 	return status;
>>>> }
>>>> 
>>>> @@ -5276,7 +5430,6 @@ int nfs4_proc_create_session(struct nfs_client *clp)
>>>> 
>>>> 	/* Init or reset the session slot tables */
>>>> 	status = nfs4_setup_session_slot_tables(session);
>>>> -	dprintk("slot table setup returned %d\n", status);
>>>> 	if (status)
>>>> 		goto out;
>>>> 
>>>> @@ -5284,7 +5437,7 @@ int nfs4_proc_create_session(struct nfs_client *clp)
>>>> 	dprintk("%s client>seqid %d sessionid %u:%u:%u:%u\n", __func__,
>>>> 		clp->cl_seqid, ptr[0], ptr[1], ptr[2], ptr[3]);
>>>> out:
>>>> -	dprintk("<-- %s\n", __func__);
>>>> +	dprintk("<-- %s status %d\n", __func__, status);
>>>> 	return status;
>>>> }
>>>> 
>>>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>>>> index 39914be..8260865 100644
>>>> --- a/fs/nfs/nfs4state.c
>>>> +++ b/fs/nfs/nfs4state.c
>>>> @@ -1573,26 +1573,16 @@ static int nfs4_recall_slot(struct nfs_client *clp)
>>>> {
>>>> 	struct nfs4_slot_table *fc_tbl = &clp->cl_session->fc_slot_table;
>>>> 	struct nfs4_channel_attrs *fc_attrs = &clp->cl_session->fc_attrs;
>>>> -	struct nfs4_slot *new, *old;
>>>> -	int i;
>>>> 
>>>> 	nfs4_begin_drain_session(clp);
>>>> -	new = kmalloc(fc_tbl->target_max_slots * sizeof(struct nfs4_slot),
>>>> -		      GFP_NOFS);
>>>> -        if (!new)
>>>> -		return -ENOMEM;
>>>> 
>>>> 	spin_lock(&fc_tbl->slot_tbl_lock);
>>>> -	for (i = 0; i < fc_tbl->target_max_slots; i++)
>>>> -		new[i].seq_nr = fc_tbl->slots[i].seq_nr;
>>>> -	old = fc_tbl->slots;
>>>> -	fc_tbl->slots = new;
>>>> 	fc_tbl->max_slots = fc_tbl->target_max_slots;
>>>> +	nfs4_reduce_slots_locked(fc_tbl);
>>>> 	fc_tbl->target_max_slots = 0;
>>>> 	fc_attrs->max_reqs = fc_tbl->max_slots;
>>>> 	spin_unlock(&fc_tbl->slot_tbl_lock);
>>>> 
>>>> -	kfree(old);
>>>> 	nfs4_end_drain_session(clp);
>>>> 	return 0;
>>>> }
>>>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
>>>> index e6161b2..c42ec29 100644
>>>> --- a/fs/nfs/nfs4xdr.c
>>>> +++ b/fs/nfs/nfs4xdr.c
>>>> @@ -1872,7 +1872,6 @@ static void encode_sequence(struct xdr_stream *xdr,
>>>> #if defined(CONFIG_NFS_V4_1)
>>>> 	struct nfs4_session *session = args->sa_session;
>>>> 	struct nfs4_slot_table *tp;
>>>> -	struct nfs4_slot *slot;
>>>> 	__be32 *p;
>>>> 
>>>> 	if (!session)
>>>> @@ -1880,8 +1879,7 @@ static void encode_sequence(struct xdr_stream *xdr,
>>>> 
>>>> 	tp = &session->fc_slot_table;
>>>> 
>>>> -	WARN_ON(args->sa_slotid == NFS4_MAX_SLOT_TABLE);
>>>> -	slot = tp->slots + args->sa_slotid;
>>>> +	WARN_ON(args->sa_slot->slotid == NFS4_MAX_SLOT_TABLE);
>>>> 
>>>> 	p = reserve_space(xdr, 4 + NFS4_MAX_SESSIONID_LEN + 16);
>>>> 	*p++ = cpu_to_be32(OP_SEQUENCE);
>>>> @@ -1896,11 +1894,11 @@ static void encode_sequence(struct xdr_stream *xdr,
>>>> 		((u32 *)session->sess_id.data)[1],
>>>> 		((u32 *)session->sess_id.data)[2],
>>>> 		((u32 *)session->sess_id.data)[3],
>>>> -		slot->seq_nr, args->sa_slotid,
>>>> +		args->sa_slot->seq_nr, args->sa_slot->slotid,
>>>> 		tp->highest_used_slotid, args->sa_cache_this);
>>>> 	p = xdr_encode_opaque_fixed(p, session->sess_id.data, NFS4_MAX_SESSIONID_LEN);
>>>> -	*p++ = cpu_to_be32(slot->seq_nr);
>>>> -	*p++ = cpu_to_be32(args->sa_slotid);
>>>> +	*p++ = cpu_to_be32(args->sa_slot->seq_nr);
>>>> +	*p++ = cpu_to_be32(args->sa_slot->slotid);
>>>> 	*p++ = cpu_to_be32(tp->highest_used_slotid);
>>>> 	*p = cpu_to_be32(args->sa_cache_this);
>>>> 	hdr->nops++;
>>>> @@ -5361,13 +5359,17 @@ static int decode_sequence(struct xdr_stream *xdr,
>>>> 	/* seqid */
>>>> 	dummy = be32_to_cpup(p++);
>>>> 	if (dummy != res->sr_slot->seq_nr) {
>>>> -		dprintk("%s Invalid sequence number\n", __func__);
>>>> +		dprintk("%s Invalid seq num %d for [slotid:seq_nr] [%d:%d]\n",
>>>> +			__func__, dummy, res->sr_slot->slotid,
>>>> +			res->sr_slot->seq_nr);
>>>> 		goto out_err;
>>>> 	}
>>>> 	/* slot id */
>>>> 	dummy = be32_to_cpup(p++);
>>>> -	if (dummy != res->sr_slot - res->sr_session->fc_slot_table.slots) {
>>>> -		dprintk("%s Invalid slot id\n", __func__);
>>>> +	if (dummy != res->sr_slot->slotid) {
>>>> +		dprintk("%s Invalid slot id %d for [slotid:seq_nr] [%d:%d]\n",
>>>> +			__func__, dummy, res->sr_slot->slotid,
>>>> +			res->sr_slot->seq_nr);
>>>> 		goto out_err;
>>>> 	}
>>>> 	/* highest slot id - currently not processed */
>>>> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
>>>> index b5479df..90c6e2b 100644
>>>> --- a/include/linux/nfs_fs_sb.h
>>>> +++ b/include/linux/nfs_fs_sb.h
>>>> @@ -193,12 +193,15 @@ struct nfs_server {
>>>> 
>>>> /* Sessions */
>>>> #define SLOT_TABLE_SZ (NFS4_MAX_SLOT_TABLE/(8*sizeof(long)))
>>>> +#define SLOT_TABLE_BITS                5
>>>> +#define SLOT_TABLE_SIZE                (1 << SLOT_TABLE_BITS)
>>>> struct nfs4_slot_table {
>>>> -	struct nfs4_slot *slots;		/* seqid per slot */
>>>> +	struct hlist_head slots[SLOT_TABLE_SIZE];
>>>> 	unsigned long   used_slots[SLOT_TABLE_SZ]; /* used/unused bitmap */
>>>> 	spinlock_t	slot_tbl_lock;
>>>> 	struct rpc_wait_queue	slot_tbl_waitq;	/* allocators may wait here */
>>>> -	int		max_slots;		/* # slots in table */
>>>> +	int		allocated_slots;	/* # slots in table */
>>>> +	int		max_slots;		/* max # slots in table */
>>>> 	int		highest_used_slotid;	/* sent to server on each SEQ.
>>>> 						 * op for dynamic resizing */
>>>> 	int		target_max_slots;	/* Set by CB_RECALL_SLOT as
>>>> @@ -206,11 +209,6 @@ struct nfs4_slot_table {
>>>> 	struct completion complete;
>>>> };
>>>> 
>>>> -static inline int slot_idx(struct nfs4_slot_table *tbl, struct nfs4_slot *sp)
>>>> -{
>>>> -	return sp - tbl->slots;
>>>> -}
>>>> -
>>>> /*
>>>> * Session related parameters
>>>> */
>>>> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
>>>> index c74595b..76b27a4 100644
>>>> --- a/include/linux/nfs_xdr.h
>>>> +++ b/include/linux/nfs_xdr.h
>>>> @@ -168,12 +168,14 @@ struct nfs4_channel_attrs {
>>>> 
>>>> /* nfs41 sessions slot seqid */
>>>> struct nfs4_slot {
>>>> +	struct hlist_node	slot_node;
>>>> 	u32		 	seq_nr;
>>>> +	u8			slotid;
>>>> };
>>>> 
>>>> struct nfs4_sequence_args {
>>>> 	struct nfs4_session	*sa_session;
>>>> -	u8			sa_slotid;
>>>> +	struct nfs4_slot        *sa_slot;
>>>> 	u8			sa_cache_this;
>>>> };
>>>> 
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH Version 4 5/7] NFSv4.1: do not drain session on CB_RECALL_SLOT

[andros]

From: Andy Adamson <andros@netapp.com>

Add a reference to the dynamic session slots while they are in use
to allow for reducing the number of active slots without
the need to drain the session.

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/callback_proc.c  |    6 +++++-
 fs/nfs/nfs4_fs.h        |    1 -
 fs/nfs/nfs4proc.c       |   23 +++++++++++++++++++++--
 fs/nfs/nfs4state.c      |   36 ------------------------------------
 include/linux/nfs_xdr.h |    1 +
 5 files changed, 27 insertions(+), 40 deletions(-)

diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
index 77cc96c..baf0847 100644
--- a/fs/nfs/callback_proc.c
+++ b/fs/nfs/callback_proc.c
@@ -578,8 +578,12 @@ __be32 nfs4_callback_recallslot(struct cb_recallslotargs *args, void *dummy,
 	if (args->crsa_target_max_slots == fc_tbl->max_slots)
 		goto out;
 
+        spin_lock(&fc_tbl->slot_tbl_lock);
 	fc_tbl->target_max_slots = args->crsa_target_max_slots;
-	nfs41_handle_recall_slot(cps->clp);
+        fc_tbl->max_slots = fc_tbl->target_max_slots;
+        nfs4_reduce_slots_locked(fc_tbl);
+        cps->clp->cl_session->fc_attrs.max_reqs = fc_tbl->max_slots;
+        spin_unlock(&fc_tbl->slot_tbl_lock);
 out:
 	dprintk("%s: exit with status = %d\n", __func__, ntohl(status));
 	return status;
diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
index ff87169..a1f889a 100644
--- a/fs/nfs/nfs4_fs.h
+++ b/fs/nfs/nfs4_fs.h
@@ -22,7 +22,6 @@ enum nfs4_client_state {
 	NFS4CLNT_DELEGRETURN,
 	NFS4CLNT_LAYOUTRECALL,
 	NFS4CLNT_SESSION_RESET,
-	NFS4CLNT_RECALL_SLOT,
 	NFS4CLNT_LEASE_CONFIRM,
 	NFS4CLNT_SERVER_SCOPE_MISMATCH,
 };
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index e3a7663..525635f 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -359,6 +359,7 @@ static void nfs4_init_slot(struct nfs4_slot *new, u8 slotid, int ivalue)
 	INIT_HLIST_NODE(&new->slot_node);
 	new->slotid = slotid;
 	new->seq_nr = ivalue;
+	atomic_set(&new->ref, 1);
 }
 
 static void nfs4_insert_slot_locked(struct nfs4_slot_table *tbl,
@@ -451,13 +452,29 @@ nfs4_lookup_slot_locked(struct nfs4_slot_table *tbl, u8 slotid)
 	return NULL;
 }
 
+static inline void nfs4_put_slot_locked(struct nfs4_slot *slot)
+{
+	dprintk("--> %s slotid %d ref %d\n", __func__, slot->slotid,
+		atomic_read(&slot->ref));
+	if (atomic_dec_and_test(&slot->ref)) {
+		BUG_ON(!hlist_unhashed(&slot->slot_node));
+		kfree(slot);
+	}
+}
+
+static inline void nfs4_get_slot(struct nfs4_slot *slot)
+{
+	dprintk("--> %s slotid %d ref %d\n", __func__, slot->slotid,
+		atomic_read(&slot->ref));
+	atomic_inc(&slot->ref);
+}
+
 static void nfs4_remove_slot_locked(struct nfs4_slot_table *tbl,
 				    struct nfs4_slot *slot)
 {
-	dprintk("--> %s slotid %d\n", __func__, slot->slotid);
 	hlist_del_init(&slot->slot_node);
 	tbl->allocated_slots--;
-	kfree(slot);
+	nfs4_put_slot_locked(slot);
 }
 
 static void nfs4_free_all_slots(struct nfs4_slot_table *tbl)
@@ -587,6 +604,7 @@ static void nfs41_sequence_free_slot(struct nfs4_sequence_res *res)
 
 	spin_lock(&tbl->slot_tbl_lock);
 	nfs4_free_slot(tbl, res->sr_slot->slotid);
+	nfs4_put_slot_locked(res->sr_slot);
 	nfs4_check_drain_fc_complete(res->sr_session);
 	spin_unlock(&tbl->slot_tbl_lock);
 	res->sr_slot = NULL;
@@ -746,6 +764,7 @@ int nfs41_setup_sequence(struct nfs4_session *session,
 		nfs4_dynamic_alloc_slot(tbl, slotid);
 		return -EAGAIN;
 	}
+	nfs4_get_slot(slot);
 	spin_unlock(&tbl->slot_tbl_lock);
 
 	rpc_task_set_priority(task, RPC_PRIORITY_NORMAL);
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 8260865..c08fb3b 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -1479,12 +1479,6 @@ void nfs4_schedule_session_recovery(struct nfs4_session *session)
 }
 EXPORT_SYMBOL_GPL(nfs4_schedule_session_recovery);
 
-void nfs41_handle_recall_slot(struct nfs_client *clp)
-{
-	set_bit(NFS4CLNT_RECALL_SLOT, &clp->cl_state);
-	nfs4_schedule_state_manager(clp);
-}
-
 static void nfs4_reset_all_state(struct nfs_client *clp)
 {
 	if (test_and_set_bit(NFS4CLNT_LEASE_EXPIRED, &clp->cl_state) == 0) {
@@ -1559,8 +1553,6 @@ static int nfs4_reset_session(struct nfs_client *clp)
 		goto out;
 	}
 	clear_bit(NFS4CLNT_SESSION_RESET, &clp->cl_state);
-	/* create_session negotiated new slot table */
-	clear_bit(NFS4CLNT_RECALL_SLOT, &clp->cl_state);
 
 	 /* Let the state manager reestablish state */
 	if (!test_bit(NFS4CLNT_LEASE_EXPIRED, &clp->cl_state))
@@ -1569,28 +1561,9 @@ out:
 	return status;
 }
 
-static int nfs4_recall_slot(struct nfs_client *clp)
-{
-	struct nfs4_slot_table *fc_tbl = &clp->cl_session->fc_slot_table;
-	struct nfs4_channel_attrs *fc_attrs = &clp->cl_session->fc_attrs;
-
-	nfs4_begin_drain_session(clp);
-
-	spin_lock(&fc_tbl->slot_tbl_lock);
-	fc_tbl->max_slots = fc_tbl->target_max_slots;
-	nfs4_reduce_slots_locked(fc_tbl);
-	fc_tbl->target_max_slots = 0;
-	fc_attrs->max_reqs = fc_tbl->max_slots;
-	spin_unlock(&fc_tbl->slot_tbl_lock);
-
-	nfs4_end_drain_session(clp);
-	return 0;
-}
-
 #else /* CONFIG_NFS_V4_1 */
 static int nfs4_reset_session(struct nfs_client *clp) { return 0; }
 static int nfs4_end_drain_session(struct nfs_client *clp) { return 0; }
-static int nfs4_recall_slot(struct nfs_client *clp) { return 0; }
 #endif /* CONFIG_NFS_V4_1 */
 
 /* Set NFS4CLNT_LEASE_EXPIRED for all v4.0 errors and for recoverable errors
@@ -1699,15 +1672,6 @@ static void nfs4_state_manager(struct nfs_client *clp)
 			nfs_client_return_marked_delegations(clp);
 			continue;
 		}
-		/* Recall session slots */
-		if (test_and_clear_bit(NFS4CLNT_RECALL_SLOT, &clp->cl_state)
-		   && nfs4_has_session(clp)) {
-			status = nfs4_recall_slot(clp);
-			if (status < 0)
-				goto out_error;
-			continue;
-		}
-
 
 		nfs4_clear_state_manager_bit(clp);
 		/* Did we race with an attempt to give us more work? */
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index 76b27a4..3bcfbaf 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -169,6 +169,7 @@ struct nfs4_channel_attrs {
 /* nfs41 sessions slot seqid */
 struct nfs4_slot {
 	struct hlist_node	slot_node;
+	atomic_t		ref;
 	u32		 	seq_nr;
 	u8			slotid;
 };
-- 
1.7.6.4

[PATCH Version 4 6/7] NFSv4.1: rename nfs4_free_slot and nfs4_find_slot

[andros]

From: Andy Adamson <andros@netapp.com>

These functions deal in slotid's, not slots.

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/nfs4proc.c |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 525635f..9ec3a13 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -519,9 +519,9 @@ void nfs4_reduce_slots_locked(struct nfs4_slot_table *tbl)
 }
 
 /*
- * nfs4_free_slot - free a slot and efficiently update slot table.
+ * nfs4_free_slotid - free a slotid and efficiently update slot table.
  *
- * freeing a slot is trivially done by clearing its respective bit
+ * freeing a slotid is trivially done by clearing its respective bit
  * in the bitmap.
  * If the freed slotid equals highest_used_slotid we want to update it
  * so that the server would be able to size down the slot table if needed,
@@ -534,7 +534,7 @@ void nfs4_reduce_slots_locked(struct nfs4_slot_table *tbl)
  * Must be called while holding tbl->slot_tbl_lock
  */
 static void
-nfs4_free_slot(struct nfs4_slot_table *tbl, u8 free_slotid)
+nfs4_free_slotid(struct nfs4_slot_table *tbl, u8 free_slotid)
 {
 	int slotid = free_slotid;
 
@@ -603,7 +603,7 @@ static void nfs41_sequence_free_slot(struct nfs4_sequence_res *res)
 	}
 
 	spin_lock(&tbl->slot_tbl_lock);
-	nfs4_free_slot(tbl, res->sr_slot->slotid);
+	nfs4_free_slotid(tbl, res->sr_slot->slotid);
 	nfs4_put_slot_locked(res->sr_slot);
 	nfs4_check_drain_fc_complete(res->sr_session);
 	spin_unlock(&tbl->slot_tbl_lock);
@@ -674,17 +674,17 @@ static int nfs4_sequence_done(struct rpc_task *task,
 }
 
 /*
- * nfs4_find_slot - efficiently look for a free slot
+ * nfs4_find_slotid - efficiently look for a free slotid
  *
- * nfs4_find_slot looks for an unset bit in the used_slots bitmap.
- * If found, we mark the slot as used, update the highest_used_slotid,
+ * nfs4_find_slotid looks for an unset bit in the used_slots bitmap.
+ * If found, we mark the slotid as used, update the highest_used_slotid,
  * and respectively set up the sequence operation args.
- * The slot number is returned if found, or NFS4_MAX_SLOT_TABLE otherwise.
+ * The slotid is returned if found, or NFS4_MAX_SLOT_TABLE otherwise.
  *
  * Note: must be called with under the slot_tbl_lock.
  */
 static u8
-nfs4_find_slot(struct nfs4_slot_table *tbl)
+nfs4_find_slotid(struct nfs4_slot_table *tbl)
 {
 	int slotid;
 	u8 ret_id = NFS4_MAX_SLOT_TABLE;
@@ -744,7 +744,7 @@ int nfs41_setup_sequence(struct nfs4_session *session,
 		return -EAGAIN;
 	}
 
-	slotid = nfs4_find_slot(tbl);
+	slotid = nfs4_find_slotid(tbl);
 	if (slotid == NFS4_MAX_SLOT_TABLE) {
 		rpc_sleep_on(&tbl->slot_tbl_waitq, task, NULL);
 		spin_unlock(&tbl->slot_tbl_lock);
@@ -756,7 +756,7 @@ int nfs41_setup_sequence(struct nfs4_session *session,
 		/* keep FIFO order */
 		rpc_task_set_priority(task, RPC_PRIORITY_PRIVILEGED);
 		rpc_sleep_on(&tbl->slot_tbl_waitq, task, NULL);
-		nfs4_free_slot(tbl, slotid);
+		nfs4_free_slotid(tbl, slotid);
 		spin_unlock(&tbl->slot_tbl_lock);
 		dprintk("%s task pid %5u sleeping for dynamic slot %d\n",
 			__func__, task->tk_pid, slotid);
-- 
1.7.6.4

[PATCH Version 4 7/7] NFSv4.1: cleanup comment and debug printk

[andros]

From: Andy Adamson <andros@netapp.com>

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/nfs4proc.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 9ec3a13..fe9ad43 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -726,13 +726,10 @@ int nfs41_setup_sequence(struct nfs4_session *session,
 	spin_lock(&tbl->slot_tbl_lock);
 	if (test_bit(NFS4_SESSION_DRAINING, &session->session_state) &&
 	    !rpc_task_has_priority(task, RPC_PRIORITY_PRIVILEGED)) {
-		/*
-		 * The state manager will wait until the slot table is empty.
-		 * Schedule the reset thread
-		 */
+		/* The state manager will wait until the slot table is empty */
 		rpc_sleep_on(&tbl->slot_tbl_waitq, task, NULL);
 		spin_unlock(&tbl->slot_tbl_lock);
-		dprintk("%s Schedule Session Reset\n", __func__);
+		dprintk("%s session is draining\n", __func__);
 		return -EAGAIN;
 	}
 
-- 
1.7.6.4