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

[Benny Halevy <bhalevy@tonian.com>]

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