Re: [PATCH v2 11/47] nfsd41: sessionid hashing

["J. Bruce Fields" <bfields@fieldses.org>]

On Sat, Mar 28, 2009 at 11:31:50AM +0300, Benny Halevy wrote:
> From: Marc Eshel <eshel@almaden.ibm.com>
> 
> Simple sessionid hashing using its monotonically increasing sequence number.
> 
> Locking considerations:
> sessionid_hashtbl access is controlled by the sessionid_lock spin lock.
> It must be taken for insert, delete, and lookup.
> nfsd4_sequence looks up the session id and if the session is found,
> it calls nfsd4_get_session (still under the sessionid_lock).
> nfsd4_destroy_session calls nfsd4_put_session after unhashing
> it, so when the session's kref reaches zero it's going to get freed.
> 
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> [we don't use a prime for sessionid hash table size]
> [use sessionid_lock spin lock]
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> ---
>  fs/nfsd/nfs4state.c        |   57 +++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/nfsd/state.h |    7 +++++
>  2 files changed, 63 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index df9d42e..ac4e8f2 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -383,11 +383,62 @@ static void release_openowner(struct nfs4_stateowner *sop)
>  }
>  
>  #if defined(CONFIG_NFSD_V4_1)
> +static DEFINE_SPINLOCK(sessionid_lock);
> +#define SESSION_HASH_SIZE	512
> +static struct list_head sessionid_hashtbl[SESSION_HASH_SIZE];
> +
> +static inline int
> +hash_sessionid(struct nfs4_sessionid *sessionid)
> +{
> +	struct nfsd4_sessionid *sid = (struct nfsd4_sessionid *)sessionid;
> +
> +	return sid->sequence % SESSION_HASH_SIZE;
> +}
> +
> +static inline void
> +dump_sessionid(const char *fn, struct nfs4_sessionid *sessionid)
> +{
> +	u32 *ptr = (u32 *)(&sessionid->data[0]);
> +	dprintk("%s: %u:%u:%u:%u\n", fn, ptr[0], ptr[1], ptr[2], ptr[3]);
> +}
> +
> +/* caller must hold sessionid_lock */
> +static struct nfsd4_session *
> +find_in_sessionid_hashtbl(struct nfs4_sessionid *sessionid)
> +{
> +	struct nfsd4_session *elem;
> +	int idx;
> +
> +	dump_sessionid(__func__, sessionid);
> +	idx = hash_sessionid(sessionid);
> +	dprintk("%s: idx is %d\n", __func__, idx);
> +	/* Search in the appropriate list */
> +	list_for_each_entry(elem, &sessionid_hashtbl[idx], se_hash) {
> +		dump_sessionid("list traversal", &elem->se_sessionid);
> +		if (!memcmp(elem->se_sessionid.data, sessionid->data,
> +			    NFS4_MAX_SESSIONID_LEN)) {
> +			return elem;
> +		}
> +	}
> +
> +	dprintk("%s: session not found\n", __func__);

Massive dprintk overkill in this function.

> +	return NULL;
> +}
> +
> +/* caller must hold sessionid_lock */
>  static void
> -release_session(struct nfsd4_session *ses)
> +unhash_session(struct nfsd4_session *ses)
>  {
>  	list_del(&ses->se_hash);
>  	list_del(&ses->se_perclnt);
> +}
> +
> +static void
> +release_session(struct nfsd4_session *ses)
> +{
> +	spin_lock(&sessionid_lock);
> +	unhash_session(ses);
> +	spin_unlock(&sessionid_lock);
>  	nfsd4_put_session(ses);
>  }

It's not obvious from the names what the difference between
release_session() and nfsd4_put_session() is.

How about just renaming release_session() to unhash_session(), and
dumping hash_session?  The two list_del()'s don't need their own
function.

--b.

>  
> @@ -3213,6 +3264,10 @@ nfs4_state_init(void)
>  		INIT_LIST_HEAD(&unconf_str_hashtbl[i]);
>  		INIT_LIST_HEAD(&unconf_id_hashtbl[i]);
>  	}
> +#if defined(CONFIG_NFSD_V4_1)
> +	for (i = 0; i < SESSION_HASH_SIZE; i++)
> +		INIT_LIST_HEAD(&sessionid_hashtbl[i]);
> +#endif /* CONFIG_NFSD_V4_1 */
>  	for (i = 0; i < FILE_HASH_SIZE; i++) {
>  		INIT_LIST_HEAD(&file_hashtbl[i]);
>  	}
> diff --git a/include/linux/nfsd/state.h b/include/linux/nfsd/state.h
> index 29624b4..7592d7b 100644
> --- a/include/linux/nfsd/state.h
> +++ b/include/linux/nfsd/state.h
> @@ -133,6 +133,13 @@ nfsd4_get_session(struct nfsd4_session *ses)
>  	kref_get(&ses->se_ref);
>  }
>  
> +/* formatted contents of nfs4_sessionid */
> +struct nfsd4_sessionid {
> +	clientid_t	clientid;
> +	u32		sequence;
> +	u32		reserved;
> +};
> +
>  #define HEXDIR_LEN     33 /* hex version of 16 byte md5 of cl_name plus '\0' */
>  
>  /*
> -- 
> 1.6.2.1
>