[v3,31/38] nfsd: Move the open owner hash table into struct nfs4_client

Commit Message

Jeff Layton July 30, 2014, 1:34 a.m. UTC

From: Trond Myklebust <[e-mail]>

Preparation for removing the client_mutex.

Convert the open owner hash table into a per-client table and protect it
using the nfs4_client->cl_lock spin lock.

Signed-off-by: Trond Myklebust <[e-mail]>
---
 fs/nfsd/netns.h     |   1 -
 fs/nfsd/nfs4state.c | 187 ++++++++++++++++++++++++----------------------------
 fs/nfsd/state.h     |   1 +
 3 files changed, 86 insertions(+), 103 deletions(-)

Comments

Kinglong Mee Aug. 2, 2014, 10:39 a.m. UTC | #1

On 7/30/2014 09:34, Jeff Layton wrote:
> From: Trond Myklebust <[e-mail]>
> 
> Preparation for removing the client_mutex.
> 
> Convert the open owner hash table into a per-client table and protect it
> using the nfs4_client->cl_lock spin lock.
> 
> Signed-off-by: Trond Myklebust <[e-mail]>
> ---
>  fs/nfsd/netns.h     |   1 -
>  fs/nfsd/nfs4state.c | 187 ++++++++++++++++++++++++----------------------------
>  fs/nfsd/state.h     |   1 +
>  3 files changed, 86 insertions(+), 103 deletions(-)
> 
> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> index a71d14413d39..e1f479c162b5 100644
> --- a/fs/nfsd/netns.h
> +++ b/fs/nfsd/netns.h
> @@ -63,7 +63,6 @@ struct nfsd_net {
>  	struct rb_root conf_name_tree;
>  	struct list_head *unconf_id_hashtbl;
>  	struct rb_root unconf_name_tree;
> -	struct list_head *ownerstr_hashtbl;

I send a patch "NFSD: Rervert "knfsd: locks: flag NFSv4-owned locks"" before,
http://comments.gmane.org/gmane.linux.nfs/64382

nfsd needs the hashtbl to find the lockowner for locking by owner from
fl->fl_owner stored in struct file_lock, but without nfs_client.

If moving the hashtbl to nfs_client, it's hard to finding the lockowner for locking.

thanks,
Kinglong Mee

>  	struct list_head *sessionid_hashtbl;
>  	/*
>  	 * client_lru holds client queue ordered by nfs4_client.cl_time
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 7c15918d20f0..4af4e5eff491 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -240,35 +240,27 @@ static void nfsd4_put_session(struct nfsd4_session *ses)
>  }
>  
>  static int
> -same_owner_str(struct nfs4_stateowner *sop, struct xdr_netobj *owner,
> -							clientid_t *clid)
> +same_owner_str(struct nfs4_stateowner *sop, struct xdr_netobj *owner)
>  {
>  	return (sop->so_owner.len == owner->len) &&
> -		0 == memcmp(sop->so_owner.data, owner->data, owner->len) &&
> -		(sop->so_client->cl_clientid.cl_id == clid->cl_id);
> +		0 == memcmp(sop->so_owner.data, owner->data, owner->len);
>  }
>  
>  static struct nfs4_openowner *
>  find_openstateowner_str_locked(unsigned int hashval, struct nfsd4_open *open,
> -			bool sessions, struct nfsd_net *nn)
> +			struct nfs4_client *clp)
>  {
>  	struct nfs4_stateowner *so;
> -	struct nfs4_openowner *oo;
> -	struct nfs4_client *clp;
>  
> -	lockdep_assert_held(&nn->client_lock);
> +	lockdep_assert_held(&clp->cl_lock);
>  
> -	list_for_each_entry(so, &nn->ownerstr_hashtbl[hashval], so_strhash) {
> +	list_for_each_entry(so, &clp->cl_ownerstr_hashtbl[hashval],
> +			    so_strhash) {
>  		if (!so->so_is_open_owner)
>  			continue;
> -		if (same_owner_str(so, &open->op_owner, &open->op_clientid)) {
> -			oo = openowner(so);
> -			clp = oo->oo_owner.so_client;
> -			if ((bool)clp->cl_minorversion != sessions)
> -				break;
> -			renew_client_locked(clp);
> +		if (same_owner_str(so, &open->op_owner)) {
>  			atomic_inc(&so->so_count);
> -			return oo;
> +			return openowner(so);
>  		}
>  	}
>  	return NULL;
> @@ -276,17 +268,16 @@ find_openstateowner_str_locked(unsigned int hashval, struct nfsd4_open *open,
>  
>  static struct nfs4_openowner *
>  find_openstateowner_str(unsigned int hashval, struct nfsd4_open *open,
> -			bool sessions, struct nfsd_net *nn)
> +			struct nfs4_client *clp)
>  {
>  	struct nfs4_openowner *oo;
>  
> -	spin_lock(&nn->client_lock);
> -	oo = find_openstateowner_str_locked(hashval, open, sessions, nn);
> -	spin_unlock(&nn->client_lock);
> +	spin_lock(&clp->cl_lock);
> +	oo = find_openstateowner_str_locked(hashval, open, clp);
> +	spin_unlock(&clp->cl_lock);
>  	return oo;
>  }
>  
> -
>  static inline u32
>  opaque_hashval(const void *ptr, int nbytes)
>  {
> @@ -408,12 +399,11 @@ unsigned long max_delegations;
>  #define OWNER_HASH_SIZE             (1 << OWNER_HASH_BITS)
>  #define OWNER_HASH_MASK             (OWNER_HASH_SIZE - 1)
>  
> -static unsigned int ownerstr_hashval(u32 clientid, struct xdr_netobj *ownername)
> +static unsigned int ownerstr_hashval(struct xdr_netobj *ownername)
>  {
>  	unsigned int ret;
>  
>  	ret = opaque_hashval(ownername->data, ownername->len);
> -	ret += clientid;
>  	return ret & OWNER_HASH_MASK;
>  }
>  
> @@ -1002,40 +992,37 @@ static void release_lock_stateid(struct nfs4_ol_stateid *stp)
>  
>  static void unhash_lockowner_locked(struct nfs4_lockowner *lo)
>  {
> -	struct nfsd_net *nn = net_generic(lo->lo_owner.so_client->net,
> -						nfsd_net_id);
> +	struct nfs4_client *clp = lo->lo_owner.so_client;
>  
> -	lockdep_assert_held(&nn->client_lock);
> +	lockdep_assert_held(&clp->cl_lock);
>  
>  	list_del_init(&lo->lo_owner.so_strhash);
>  }
>  
>  static void release_lockowner_stateids(struct nfs4_lockowner *lo)
>  {
> -	struct nfsd_net *nn = net_generic(lo->lo_owner.so_client->net,
> -						nfsd_net_id);
> +	struct nfs4_client *clp = lo->lo_owner.so_client;
>  	struct nfs4_ol_stateid *stp;
>  
> -	lockdep_assert_held(&nn->client_lock);
> +	lockdep_assert_held(&clp->cl_lock);
>  
>  	while (!list_empty(&lo->lo_owner.so_stateids)) {
>  		stp = list_first_entry(&lo->lo_owner.so_stateids,
>  				struct nfs4_ol_stateid, st_perstateowner);
> -		spin_unlock(&nn->client_lock);
> +		spin_unlock(&clp->cl_lock);
>  		release_lock_stateid(stp);
> -		spin_lock(&nn->client_lock);
> +		spin_lock(&clp->cl_lock);
>  	}
>  }
>  
>  static void release_lockowner(struct nfs4_lockowner *lo)
>  {
> -	struct nfsd_net *nn = net_generic(lo->lo_owner.so_client->net,
> -						nfsd_net_id);
> +	struct nfs4_client *clp = lo->lo_owner.so_client;
>  
> -	spin_lock(&nn->client_lock);
> +	spin_lock(&clp->cl_lock);
>  	unhash_lockowner_locked(lo);
>  	release_lockowner_stateids(lo);
> -	spin_unlock(&nn->client_lock);
> +	spin_unlock(&clp->cl_lock);
>  	nfs4_put_stateowner(&lo->lo_owner);
>  }
>  
> @@ -1070,10 +1057,9 @@ static void release_open_stateid(struct nfs4_ol_stateid *stp)
>  
>  static void unhash_openowner_locked(struct nfs4_openowner *oo)
>  {
> -	struct nfsd_net *nn = net_generic(oo->oo_owner.so_client->net,
> -						nfsd_net_id);
> +	struct nfs4_client *clp = oo->oo_owner.so_client;
>  
> -	lockdep_assert_held(&nn->client_lock);
> +	lockdep_assert_held(&clp->cl_lock);
>  
>  	list_del_init(&oo->oo_owner.so_strhash);
>  	list_del_init(&oo->oo_perclient);
> @@ -1093,29 +1079,27 @@ static void release_last_closed_stateid(struct nfs4_openowner *oo)
>  static void release_openowner_stateids(struct nfs4_openowner *oo)
>  {
>  	struct nfs4_ol_stateid *stp;
> -	struct nfsd_net *nn = net_generic(oo->oo_owner.so_client->net,
> -						nfsd_net_id);
> +	struct nfs4_client *clp = oo->oo_owner.so_client;
>  
> -	lockdep_assert_held(&nn->client_lock);
> +	lockdep_assert_held(&clp->cl_lock);
>  
>  	while (!list_empty(&oo->oo_owner.so_stateids)) {
>  		stp = list_first_entry(&oo->oo_owner.so_stateids,
>  				struct nfs4_ol_stateid, st_perstateowner);
> -		spin_unlock(&nn->client_lock);
> +		spin_unlock(&clp->cl_lock);
>  		release_open_stateid(stp);
> -		spin_lock(&nn->client_lock);
> +		spin_lock(&clp->cl_lock);
>  	}
>  }
>  
>  static void release_openowner(struct nfs4_openowner *oo)
>  {
> -	struct nfsd_net *nn = net_generic(oo->oo_owner.so_client->net,
> -						nfsd_net_id);
> +	struct nfs4_client *clp = oo->oo_owner.so_client;
>  
> -	spin_lock(&nn->client_lock);
> +	spin_lock(&clp->cl_lock);
>  	unhash_openowner_locked(oo);
>  	release_openowner_stateids(oo);
> -	spin_unlock(&nn->client_lock);
> +	spin_unlock(&clp->cl_lock);
>  	release_last_closed_stateid(oo);
>  	nfs4_put_stateowner(&oo->oo_owner);
>  }
> @@ -1497,15 +1481,20 @@ STALE_CLIENTID(clientid_t *clid, struct nfsd_net *nn)
>  static struct nfs4_client *alloc_client(struct xdr_netobj name)
>  {
>  	struct nfs4_client *clp;
> +	int i;
>  
>  	clp = kzalloc(sizeof(struct nfs4_client), GFP_KERNEL);
>  	if (clp == NULL)
>  		return NULL;
>  	clp->cl_name.data = kmemdup(name.data, name.len, GFP_KERNEL);
> -	if (clp->cl_name.data == NULL) {
> -		kfree(clp);
> -		return NULL;
> -	}
> +	if (clp->cl_name.data == NULL)
> +		goto err_no_name;
> +	clp->cl_ownerstr_hashtbl = kmalloc(sizeof(struct list_head) *
> +			OWNER_HASH_SIZE, GFP_KERNEL);
> +	if (!clp->cl_ownerstr_hashtbl)
> +		goto err_no_hashtbl;
> +	for (i = 0; i < OWNER_HASH_SIZE; i++)
> +		INIT_LIST_HEAD(&clp->cl_ownerstr_hashtbl[i]);
>  	clp->cl_name.len = name.len;
>  	INIT_LIST_HEAD(&clp->cl_sessions);
>  	idr_init(&clp->cl_stateids);
> @@ -1520,6 +1509,11 @@ static struct nfs4_client *alloc_client(struct xdr_netobj name)
>  	spin_lock_init(&clp->cl_lock);
>  	rpc_init_wait_queue(&clp->cl_cb_waitq, "Backchannel slot table");
>  	return clp;
> +err_no_hashtbl:
> +	kfree(clp->cl_name.data);
> +err_no_name:
> +	kfree(clp);
> +	return NULL;
>  }
>  
>  static void
> @@ -1538,6 +1532,7 @@ free_client(struct nfs4_client *clp)
>  	}
>  	rpc_destroy_wait_queue(&clp->cl_cb_waitq);
>  	free_svc_cred(&clp->cl_cred);
> +	kfree(clp->cl_ownerstr_hashtbl);
>  	kfree(clp->cl_name.data);
>  	idr_destroy(&clp->cl_stateids);
>  	kfree(clp);
> @@ -3074,20 +3069,20 @@ static inline void *alloc_stateowner(struct kmem_cache *slab, struct xdr_netobj
>  
>  static void hash_openowner(struct nfs4_openowner *oo, struct nfs4_client *clp, unsigned int strhashval)
>  {
> -	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
> +	lockdep_assert_held(&clp->cl_lock);
>  
> -	list_add(&oo->oo_owner.so_strhash, &nn->ownerstr_hashtbl[strhashval]);
> +	list_add(&oo->oo_owner.so_strhash,
> +		 &clp->cl_ownerstr_hashtbl[strhashval]);
>  	list_add(&oo->oo_perclient, &clp->cl_openowners);
>  }
>  
>  static void nfs4_unhash_openowner(struct nfs4_stateowner *so)
>  {
> -	struct nfs4_openowner *oo = openowner(so);
> -	struct nfsd_net *nn = net_generic(so->so_client->net, nfsd_net_id);
> +	struct nfs4_client *clp = so->so_client;
>  
> -	spin_lock(&nn->client_lock);
> -	unhash_openowner_locked(oo);
> -	spin_unlock(&nn->client_lock);
> +	spin_lock(&clp->cl_lock);
> +	unhash_openowner_locked(openowner(so));
> +	spin_unlock(&clp->cl_lock);
>  }
>  
>  static void nfs4_free_openowner(struct nfs4_stateowner *so)
> @@ -3107,7 +3102,6 @@ alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
>  			   struct nfsd4_compound_state *cstate)
>  {
>  	struct nfs4_client *clp = cstate->clp;
> -	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
>  	struct nfs4_openowner *oo, *ret;
>  
>  	oo = alloc_stateowner(openowner_slab, &open->op_owner, clp);
> @@ -3122,15 +3116,14 @@ alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
>  	oo->oo_time = 0;
>  	oo->oo_last_closed_stid = NULL;
>  	INIT_LIST_HEAD(&oo->oo_close_lru);
> -	spin_lock(&nn->client_lock);
> -	ret = find_openstateowner_str_locked(strhashval,
> -			open, clp->cl_minorversion, nn);
> +	spin_lock(&clp->cl_lock);
> +	ret = find_openstateowner_str_locked(strhashval, open, clp);
>  	if (ret == NULL) {
>  		hash_openowner(oo, clp, strhashval);
>  		ret = oo;
>  	} else
>  		nfs4_free_openowner(&oo->oo_owner);
> -	spin_unlock(&nn->client_lock);
> +	spin_unlock(&clp->cl_lock);
>  	return oo;
>  }
>  
> @@ -3412,8 +3405,8 @@ nfsd4_process_open1(struct nfsd4_compound_state *cstate,
>  		return status;
>  	clp = cstate->clp;
>  
> -	strhashval = ownerstr_hashval(clientid->cl_id, &open->op_owner);
> -	oo = find_openstateowner_str(strhashval, open, cstate->minorversion, nn);
> +	strhashval = ownerstr_hashval(&open->op_owner);
> +	oo = find_openstateowner_str(strhashval, open, clp);
>  	open->op_openowner = oo;
>  	if (!oo) {
>  		goto new_owner;
> @@ -4818,15 +4811,16 @@ nevermind:
>  
>  static struct nfs4_lockowner *
>  find_lockowner_str_locked(clientid_t *clid, struct xdr_netobj *owner,
> -		struct nfsd_net *nn)
> +		struct nfs4_client *clp)
>  {
> -	unsigned int strhashval = ownerstr_hashval(clid->cl_id, owner);
> +	unsigned int strhashval = ownerstr_hashval(owner);
>  	struct nfs4_stateowner *so;
>  
> -	list_for_each_entry(so, &nn->ownerstr_hashtbl[strhashval], so_strhash) {
> +	list_for_each_entry(so, &clp->cl_ownerstr_hashtbl[strhashval],
> +			    so_strhash) {
>  		if (so->so_is_open_owner)
>  			continue;
> -		if (!same_owner_str(so, owner, clid))
> +		if (!same_owner_str(so, owner))
>  			continue;
>  		atomic_inc(&so->so_count);
>  		return lockowner(so);
> @@ -4836,23 +4830,23 @@ find_lockowner_str_locked(clientid_t *clid, struct xdr_netobj *owner,
>  
>  static struct nfs4_lockowner *
>  find_lockowner_str(clientid_t *clid, struct xdr_netobj *owner,
> -		struct nfsd_net *nn)
> +		struct nfs4_client *clp)
>  {
>  	struct nfs4_lockowner *lo;
>  
> -	spin_lock(&nn->client_lock);
> -	lo = find_lockowner_str_locked(clid, owner, nn);
> -	spin_unlock(&nn->client_lock);
> +	spin_lock(&clp->cl_lock);
> +	lo = find_lockowner_str_locked(clid, owner, clp);
> +	spin_unlock(&clp->cl_lock);
>  	return lo;
>  }
>  
>  static void nfs4_unhash_lockowner(struct nfs4_stateowner *sop)
>  {
> -	struct nfsd_net *nn = net_generic(sop->so_client->net, nfsd_net_id);
> +	struct nfs4_client *clp = sop->so_client;
>  
> -	spin_lock(&nn->client_lock);
> +	spin_lock(&clp->cl_lock);
>  	unhash_lockowner_locked(lockowner(sop));
> -	spin_unlock(&nn->client_lock);
> +	spin_unlock(&clp->cl_lock);
>  }
>  
>  static void nfs4_free_lockowner(struct nfs4_stateowner *sop)
> @@ -4879,7 +4873,6 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp,
>  			   struct nfs4_ol_stateid *open_stp,
>  			   struct nfsd4_lock *lock)
>  {
> -	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
>  	struct nfs4_lockowner *lo, *ret;
>  
>  	lo = alloc_stateowner(lockowner_slab, &lock->lk_new_owner, clp);
> @@ -4889,16 +4882,16 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp,
>  	lo->lo_owner.so_is_open_owner = 0;
>  	lo->lo_owner.so_seqid = lock->lk_new_lock_seqid;
>  	lo->lo_owner.so_ops = &lockowner_ops;
> -	spin_lock(&nn->client_lock);
> +	spin_lock(&clp->cl_lock);
>  	ret = find_lockowner_str_locked(&clp->cl_clientid,
> -			&lock->lk_new_owner, nn);
> +			&lock->lk_new_owner, clp);
>  	if (ret == NULL) {
>  		list_add(&lo->lo_owner.so_strhash,
> -			 &nn->ownerstr_hashtbl[strhashval]);
> +			 &clp->cl_ownerstr_hashtbl[strhashval]);
>  		ret = lo;
>  	} else
>  		nfs4_free_lockowner(&lo->lo_owner);
> -	spin_unlock(&nn->client_lock);
> +	spin_unlock(&clp->cl_lock);
>  	return lo;
>  }
>  
> @@ -5010,12 +5003,10 @@ lookup_or_create_lock_state(struct nfsd4_compound_state *cstate,
>  	struct inode *inode = cstate->current_fh.fh_dentry->d_inode;
>  	struct nfs4_lockowner *lo;
>  	unsigned int strhashval;
> -	struct nfsd_net *nn = net_generic(cl->net, nfsd_net_id);
>  
> -	lo = find_lockowner_str(&cl->cl_clientid, &lock->v.new.owner, nn);
> +	lo = find_lockowner_str(&cl->cl_clientid, &lock->v.new.owner, cl);
>  	if (!lo) {
> -		strhashval = ownerstr_hashval(cl->cl_clientid.cl_id,
> -				&lock->v.new.owner);
> +		strhashval = ownerstr_hashval(&lock->v.new.owner);
>  		lo = alloc_init_lock_stateowner(strhashval, cl, ost, lock);
>  		if (lo == NULL)
>  			return nfserr_jukebox;
> @@ -5293,7 +5284,8 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		goto out;
>  	}
>  
> -	lo = find_lockowner_str(&lockt->lt_clientid, &lockt->lt_owner, nn);
> +	lo = find_lockowner_str(&lockt->lt_clientid, &lockt->lt_owner,
> +				cstate->clp);
>  	if (lo)
>  		file_lock->fl_owner = (fl_owner_t)lo;
>  	file_lock->fl_pid = current->tgid;
> @@ -5436,7 +5428,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
>  	struct nfs4_lockowner *lo;
>  	struct nfs4_ol_stateid *stp;
>  	struct xdr_netobj *owner = &rlockowner->rl_owner;
> -	unsigned int hashval = ownerstr_hashval(clid->cl_id, owner);
> +	unsigned int hashval = ownerstr_hashval(owner);
>  	__be32 status;
>  	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
>  	struct nfs4_client *clp;
> @@ -5452,29 +5444,29 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
>  
>  	status = nfserr_locks_held;
>  
> +	clp = cstate->clp;
>  	/* Find the matching lock stateowner */
> -	spin_lock(&nn->client_lock);
> -	list_for_each_entry(tmp, &nn->ownerstr_hashtbl[hashval], so_strhash) {
> +	spin_lock(&clp->cl_lock);
> +	list_for_each_entry(tmp, &clp->cl_ownerstr_hashtbl[hashval],
> +			    so_strhash) {
>  		if (tmp->so_is_open_owner)
>  			continue;
> -		if (same_owner_str(tmp, owner, clid)) {
> +		if (same_owner_str(tmp, owner)) {
>  			sop = tmp;
>  			atomic_inc(&sop->so_count);
>  			break;
>  		}
>  	}
> -	spin_unlock(&nn->client_lock);
>  
>  	/* No matching owner found, maybe a replay? Just declare victory... */
>  	if (!sop) {
> +		spin_unlock(&clp->cl_lock);
>  		status = nfs_ok;
>  		goto out;
>  	}
>  
>  	lo = lockowner(sop);
>  	/* see if there are still any locks associated with it */
> -	clp = cstate->clp;
> -	spin_lock(&clp->cl_lock);
>  	list_for_each_entry(stp, &sop->so_stateids, st_perstateowner) {
>  		if (check_for_locks(stp->st_stid.sc_file, lo)) {
>  			spin_unlock(&clp->cl_lock);
> @@ -5829,10 +5821,6 @@ static int nfs4_state_create_net(struct net *net)
>  			CLIENT_HASH_SIZE, GFP_KERNEL);
>  	if (!nn->unconf_id_hashtbl)
>  		goto err_unconf_id;
> -	nn->ownerstr_hashtbl = kmalloc(sizeof(struct list_head) *
> -			OWNER_HASH_SIZE, GFP_KERNEL);
> -	if (!nn->ownerstr_hashtbl)
> -		goto err_ownerstr;
>  	nn->sessionid_hashtbl = kmalloc(sizeof(struct list_head) *
>  			SESSION_HASH_SIZE, GFP_KERNEL);
>  	if (!nn->sessionid_hashtbl)
> @@ -5842,8 +5830,6 @@ static int nfs4_state_create_net(struct net *net)
>  		INIT_LIST_HEAD(&nn->conf_id_hashtbl[i]);
>  		INIT_LIST_HEAD(&nn->unconf_id_hashtbl[i]);
>  	}
> -	for (i = 0; i < OWNER_HASH_SIZE; i++)
> -		INIT_LIST_HEAD(&nn->ownerstr_hashtbl[i]);
>  	for (i = 0; i < SESSION_HASH_SIZE; i++)
>  		INIT_LIST_HEAD(&nn->sessionid_hashtbl[i]);
>  	nn->conf_name_tree = RB_ROOT;
> @@ -5859,8 +5845,6 @@ static int nfs4_state_create_net(struct net *net)
>  	return 0;
>  
>  err_sessionid:
> -	kfree(nn->ownerstr_hashtbl);
> -err_ownerstr:
>  	kfree(nn->unconf_id_hashtbl);
>  err_unconf_id:
>  	kfree(nn->conf_id_hashtbl);
> @@ -5890,7 +5874,6 @@ nfs4_state_destroy_net(struct net *net)
>  	}
>  
>  	kfree(nn->sessionid_hashtbl);
> -	kfree(nn->ownerstr_hashtbl);
>  	kfree(nn->unconf_id_hashtbl);
>  	kfree(nn->conf_id_hashtbl);
>  	put_net(net);
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index e073c86f389c..73a209dc352b 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -235,6 +235,7 @@ struct nfsd4_sessionid {
>  struct nfs4_client {
>  	struct list_head	cl_idhash; 	/* hash by cl_clientid.id */
>  	struct rb_node		cl_namenode;	/* link into by-name trees */
> +	struct list_head	*cl_ownerstr_hashtbl;
>  	struct list_head	cl_openowners;
>  	struct idr		cl_stateids;	/* stateid lookup */
>  	struct list_head	cl_delegations;
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Trond Myklebust Aug. 2, 2014, 1:11 p.m. UTC | #2

On Sat, Aug 2, 2014 at 6:39 AM, Kinglong Mee <[e-mail]> wrote:
> On 7/30/2014 09:34, Jeff Layton wrote:
>> From: Trond Myklebust <[e-mail]>
>>
>> Preparation for removing the client_mutex.
>>
>> Convert the open owner hash table into a per-client table and protect it
>> using the nfs4_client->cl_lock spin lock.
>>
>> Signed-off-by: Trond Myklebust <[e-mail]>
>> ---
>>  fs/nfsd/netns.h     |   1 -
>>  fs/nfsd/nfs4state.c | 187 ++++++++++++++++++++++++----------------------------
>>  fs/nfsd/state.h     |   1 +
>>  3 files changed, 86 insertions(+), 103 deletions(-)
>>
>> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
>> index a71d14413d39..e1f479c162b5 100644
>> --- a/fs/nfsd/netns.h
>> +++ b/fs/nfsd/netns.h
>> @@ -63,7 +63,6 @@ struct nfsd_net {
>>       struct rb_root conf_name_tree;
>>       struct list_head *unconf_id_hashtbl;
>>       struct rb_root unconf_name_tree;
>> -     struct list_head *ownerstr_hashtbl;
>
> I send a patch "NFSD: Rervert "knfsd: locks: flag NFSv4-owned locks"" before,
> http://comments.gmane.org/gmane.linux.nfs/64382
>
> nfsd needs the hashtbl to find the lockowner for locking by owner from
> fl->fl_owner stored in struct file_lock, but without nfs_client.

Why? We're not currently doing that.

> If moving the hashtbl to nfs_client, it's hard to finding the lockowner for locking.

Jeff Layton Aug. 2, 2014, 1:23 p.m. UTC | #3

On Sat, 2 Aug 2014 09:11:25 -0400
Trond Myklebust <[e-mail]> wrote:

> On Sat, Aug 2, 2014 at 6:39 AM, Kinglong Mee <[e-mail]> wrote:
> > On 7/30/2014 09:34, Jeff Layton wrote:
> >> From: Trond Myklebust <[e-mail]>
> >>
> >> Preparation for removing the client_mutex.
> >>
> >> Convert the open owner hash table into a per-client table and protect it
> >> using the nfs4_client->cl_lock spin lock.
> >>
> >> Signed-off-by: Trond Myklebust <[e-mail]>
> >> ---
> >>  fs/nfsd/netns.h     |   1 -
> >>  fs/nfsd/nfs4state.c | 187 ++++++++++++++++++++++++----------------------------
> >>  fs/nfsd/state.h     |   1 +
> >>  3 files changed, 86 insertions(+), 103 deletions(-)
> >>
> >> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> >> index a71d14413d39..e1f479c162b5 100644
> >> --- a/fs/nfsd/netns.h
> >> +++ b/fs/nfsd/netns.h
> >> @@ -63,7 +63,6 @@ struct nfsd_net {
> >>       struct rb_root conf_name_tree;
> >>       struct list_head *unconf_id_hashtbl;
> >>       struct rb_root unconf_name_tree;
> >> -     struct list_head *ownerstr_hashtbl;
> >
> > I send a patch "NFSD: Rervert "knfsd: locks: flag NFSv4-owned locks"" before,
> > http://comments.gmane.org/gmane.linux.nfs/64382
> >
> > nfsd needs the hashtbl to find the lockowner for locking by owner from
> > fl->fl_owner stored in struct file_lock, but without nfs_client.
> 
> Why? We're not currently doing that.
> 
> > If moving the hashtbl to nfs_client, it's hard to finding the lockowner for locking.
> 

I think there's a fundamental flaw in your original patch.

You're casting fl->fl_owner to a struct nfs4_lockowner pointer, and
then dereferencing that pointer to get to the lo->lo_hashval field. The
problem there is that conflock might refer to a lock that is not a
nfsv4 lock, and in that case there is zero guarantee that the
fl_owner_t is a pointer at all, so that method could end up causing
oopses.

The fl_owner is really intended to be an opaque token, and you can't
turn it back into a pointer without knowing for a fact what sort of
lock it is. In the v4 case, we use the fl_lmops field to try and
designate that.

Perhaps you need to change it so that the conflocks get fl_lmops set
properly instead of changing things like you are in that patch?

Kinglong Mee Aug. 2, 2014, 1:43 p.m. UTC | #4

On 8/2/2014 21:11, Trond Myklebust wrote:
> On Sat, Aug 2, 2014 at 6:39 AM, Kinglong Mee <[e-mail]> wrote:
>> On 7/30/2014 09:34, Jeff Layton wrote:
>>> From: Trond Myklebust <[e-mail]>
>>>
>>> Preparation for removing the client_mutex.
>>>
>>> Convert the open owner hash table into a per-client table and protect it
>>> using the nfs4_client->cl_lock spin lock.
>>>
>>> Signed-off-by: Trond Myklebust <[e-mail]>
>>> ---
>>>  fs/nfsd/netns.h     |   1 -
>>>  fs/nfsd/nfs4state.c | 187 ++++++++++++++++++++++++----------------------------
>>>  fs/nfsd/state.h     |   1 +
>>>  3 files changed, 86 insertions(+), 103 deletions(-)
>>>
>>> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
>>> index a71d14413d39..e1f479c162b5 100644
>>> --- a/fs/nfsd/netns.h
>>> +++ b/fs/nfsd/netns.h
>>> @@ -63,7 +63,6 @@ struct nfsd_net {
>>>       struct rb_root conf_name_tree;
>>>       struct list_head *unconf_id_hashtbl;
>>>       struct rb_root unconf_name_tree;
>>> -     struct list_head *ownerstr_hashtbl;
>>
>> I send a patch "NFSD: Rervert "knfsd: locks: flag NFSv4-owned locks"" before,
>> http://comments.gmane.org/gmane.linux.nfs/64382
>>
>> nfsd needs the hashtbl to find the lockowner for locking by owner from
>> fl->fl_owner stored in struct file_lock, but without nfs_client.
> 
> Why? We're not currently doing that.

Although not doing that right now, but there is a bug for getting the right ld_owner
in nfs4_set_lock_denied.

If denying locks, vfs don't copy fl->fl_lmops to the returned file_lock, so that,
fl->fl_lmops always be NULL, nfsd never return the owner who holds the conflock.

If we want fix this problem, needs finding the lockowner from struct file_lock.

thanks,
Kinglong Mee

> 
>> If moving the hashtbl to nfs_client, it's hard to finding the lockowner for locking.
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kinglong Mee Aug. 2, 2014, 1:51 p.m. UTC | #5

On 8/2/2014 21:23, Jeff Layton wrote:
> On Sat, 2 Aug 2014 09:11:25 -0400
> Trond Myklebust <[e-mail]> wrote:
> 
>> On Sat, Aug 2, 2014 at 6:39 AM, Kinglong Mee <[e-mail]> wrote:
>>> On 7/30/2014 09:34, Jeff Layton wrote:
>>>> From: Trond Myklebust <[e-mail]>
>>>>
>>>> Preparation for removing the client_mutex.
>>>>
>>>> Convert the open owner hash table into a per-client table and protect it
>>>> using the nfs4_client->cl_lock spin lock.
>>>>
>>>> Signed-off-by: Trond Myklebust <[e-mail]>
>>>> ---
>>>>  fs/nfsd/netns.h     |   1 -
>>>>  fs/nfsd/nfs4state.c | 187 ++++++++++++++++++++++++----------------------------
>>>>  fs/nfsd/state.h     |   1 +
>>>>  3 files changed, 86 insertions(+), 103 deletions(-)
>>>>
>>>> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
>>>> index a71d14413d39..e1f479c162b5 100644
>>>> --- a/fs/nfsd/netns.h
>>>> +++ b/fs/nfsd/netns.h
>>>> @@ -63,7 +63,6 @@ struct nfsd_net {
>>>>       struct rb_root conf_name_tree;
>>>>       struct list_head *unconf_id_hashtbl;
>>>>       struct rb_root unconf_name_tree;
>>>> -     struct list_head *ownerstr_hashtbl;
>>>
>>> I send a patch "NFSD: Rervert "knfsd: locks: flag NFSv4-owned locks"" before,
>>> http://comments.gmane.org/gmane.linux.nfs/64382
>>>
>>> nfsd needs the hashtbl to find the lockowner for locking by owner from
>>> fl->fl_owner stored in struct file_lock, but without nfs_client.
>>
>> Why? We're not currently doing that.
>>
>>> If moving the hashtbl to nfs_client, it's hard to finding the lockowner for locking.
>>
> 
> I think there's a fundamental flaw in your original patch.
> 
> You're casting fl->fl_owner to a struct nfs4_lockowner pointer, and
> then dereferencing that pointer to get to the lo->lo_hashval field. The
> problem there is that conflock might refer to a lock that is not a
> nfsv4 lock, and in that case there is zero guarantee that the
> fl_owner_t is a pointer at all, so that method could end up causing
> oopses.

Yes, you are right, the old patch has the problem.

> 
> The fl_owner is really intended to be an opaque token, and you can't
> turn it back into a pointer without knowing for a fact what sort of
> lock it is. In the v4 case, we use the fl_lmops field to try and
> designate that.

Got it.

> 
> Perhaps you need to change it so that the conflocks get fl_lmops set
> properly instead of changing things like you are in that patch?

OK, I will sends a new patch coping fl_lmops to conflocks for this problem.

thanks,
Kinglong Mee

> 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Trond Myklebust Aug. 2, 2014, 2:05 p.m. UTC | #6

On Sat, Aug 2, 2014 at 9:43 AM, Kinglong Mee <[e-mail]> wrote:
> On 8/2/2014 21:11, Trond Myklebust wrote:
>> On Sat, Aug 2, 2014 at 6:39 AM, Kinglong Mee <[e-mail]> wrote:
>>> On 7/30/2014 09:34, Jeff Layton wrote:
>>>> From: Trond Myklebust <[e-mail]>
>>>>
>>>> Preparation for removing the client_mutex.
>>>>
>>>> Convert the open owner hash table into a per-client table and protect it
>>>> using the nfs4_client->cl_lock spin lock.
>>>>
>>>> Signed-off-by: Trond Myklebust <[e-mail]>
>>>> ---
>>>>  fs/nfsd/netns.h     |   1 -
>>>>  fs/nfsd/nfs4state.c | 187 ++++++++++++++++++++++++----------------------------
>>>>  fs/nfsd/state.h     |   1 +
>>>>  3 files changed, 86 insertions(+), 103 deletions(-)
>>>>
>>>> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
>>>> index a71d14413d39..e1f479c162b5 100644
>>>> --- a/fs/nfsd/netns.h
>>>> +++ b/fs/nfsd/netns.h
>>>> @@ -63,7 +63,6 @@ struct nfsd_net {
>>>>       struct rb_root conf_name_tree;
>>>>       struct list_head *unconf_id_hashtbl;
>>>>       struct rb_root unconf_name_tree;
>>>> -     struct list_head *ownerstr_hashtbl;
>>>
>>> I send a patch "NFSD: Rervert "knfsd: locks: flag NFSv4-owned locks"" before,
>>> http://comments.gmane.org/gmane.linux.nfs/64382
>>>
>>> nfsd needs the hashtbl to find the lockowner for locking by owner from
>>> fl->fl_owner stored in struct file_lock, but without nfs_client.
>>
>> Why? We're not currently doing that.
>
> Although not doing that right now, but there is a bug for getting the right ld_owner
> in nfs4_set_lock_denied.
>
> If denying locks, vfs don't copy fl->fl_lmops to the returned file_lock, so that,
> fl->fl_lmops always be NULL, nfsd never return the owner who holds the conflock.
>
> If we want fix this problem, needs finding the lockowner from struct file_lock.

Do we really care enough about fixing nfs4_set_lock_denied enough to
do so at the cost of reducing overall scalability of locking state?
We will always be faking up the clientid etc for local locks. Are
there any clients out there that actually inspect the clientid on a
result of NFS4ERR_DENIED and that will break if we give them a fake
for non-local locks?

Kinglong Mee Aug. 2, 2014, 2:20 p.m. UTC | #7

On 8/2/2014 22:05, Trond Myklebust wrote:
> On Sat, Aug 2, 2014 at 9:43 AM, Kinglong Mee <[e-mail]> wrote:
>> On 8/2/2014 21:11, Trond Myklebust wrote:
>>> On Sat, Aug 2, 2014 at 6:39 AM, Kinglong Mee <[e-mail]> wrote:
>>>> On 7/30/2014 09:34, Jeff Layton wrote:
>>>>> From: Trond Myklebust <[e-mail]>
>>>>>
>>>>> Preparation for removing the client_mutex.
>>>>>
>>>>> Convert the open owner hash table into a per-client table and protect it
>>>>> using the nfs4_client->cl_lock spin lock.
>>>>>
>>>>> Signed-off-by: Trond Myklebust <[e-mail]>
>>>>> ---
>>>>>  fs/nfsd/netns.h     |   1 -
>>>>>  fs/nfsd/nfs4state.c | 187 ++++++++++++++++++++++++----------------------------
>>>>>  fs/nfsd/state.h     |   1 +
>>>>>  3 files changed, 86 insertions(+), 103 deletions(-)
>>>>>
>>>>> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
>>>>> index a71d14413d39..e1f479c162b5 100644
>>>>> --- a/fs/nfsd/netns.h
>>>>> +++ b/fs/nfsd/netns.h
>>>>> @@ -63,7 +63,6 @@ struct nfsd_net {
>>>>>       struct rb_root conf_name_tree;
>>>>>       struct list_head *unconf_id_hashtbl;
>>>>>       struct rb_root unconf_name_tree;
>>>>> -     struct list_head *ownerstr_hashtbl;
>>>>
>>>> I send a patch "NFSD: Rervert "knfsd: locks: flag NFSv4-owned locks"" before,
>>>> http://comments.gmane.org/gmane.linux.nfs/64382
>>>>
>>>> nfsd needs the hashtbl to find the lockowner for locking by owner from
>>>> fl->fl_owner stored in struct file_lock, but without nfs_client.
>>>
>>> Why? We're not currently doing that.
>>
>> Although not doing that right now, but there is a bug for getting the right ld_owner
>> in nfs4_set_lock_denied.
>>
>> If denying locks, vfs don't copy fl->fl_lmops to the returned file_lock, so that,
>> fl->fl_lmops always be NULL, nfsd never return the owner who holds the conflock.
>>
>> If we want fix this problem, needs finding the lockowner from struct file_lock.
> 
> Do we really care enough about fixing nfs4_set_lock_denied enough to
> do so at the cost of reducing overall scalability of locking state?

I just report this problem, don't think enough about the scalability.

> We will always be faking up the clientid etc for local locks. Are
> there any clients out there that actually inspect the clientid on a
> result of NFS4ERR_DENIED and that will break if we give them a fake
> for non-local locks?

Jeff has point the same problem of a non-nfs4_lockowner.
Maybe we should copy fl_lmops to conflock as before, nfsd can distinguish
the lockowner stored in struct file_lock by checking fl_lmops.

thanks,
Kinglong Mee

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Trond Myklebust Aug. 2, 2014, 2:47 p.m. UTC | #8

On Sat, Aug 2, 2014 at 10:20 AM, Kinglong Mee <[e-mail]> wrote:
> On 8/2/2014 22:05, Trond Myklebust wrote:
>> On Sat, Aug 2, 2014 at 9:43 AM, Kinglong Mee <[e-mail]> wrote:
>>> On 8/2/2014 21:11, Trond Myklebust wrote:
>>>> On Sat, Aug 2, 2014 at 6:39 AM, Kinglong Mee <[e-mail]> wrote:
>>>>> On 7/30/2014 09:34, Jeff Layton wrote:
>>>>>> From: Trond Myklebust <[e-mail]>
>>>>>>
>>>>>> Preparation for removing the client_mutex.
>>>>>>
>>>>>> Convert the open owner hash table into a per-client table and protect it
>>>>>> using the nfs4_client->cl_lock spin lock.
>>>>>>
>>>>>> Signed-off-by: Trond Myklebust <[e-mail]>
>>>>>> ---
>>>>>>  fs/nfsd/netns.h     |   1 -
>>>>>>  fs/nfsd/nfs4state.c | 187 ++++++++++++++++++++++++----------------------------
>>>>>>  fs/nfsd/state.h     |   1 +
>>>>>>  3 files changed, 86 insertions(+), 103 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
>>>>>> index a71d14413d39..e1f479c162b5 100644
>>>>>> --- a/fs/nfsd/netns.h
>>>>>> +++ b/fs/nfsd/netns.h
>>>>>> @@ -63,7 +63,6 @@ struct nfsd_net {
>>>>>>       struct rb_root conf_name_tree;
>>>>>>       struct list_head *unconf_id_hashtbl;
>>>>>>       struct rb_root unconf_name_tree;
>>>>>> -     struct list_head *ownerstr_hashtbl;
>>>>>
>>>>> I send a patch "NFSD: Rervert "knfsd: locks: flag NFSv4-owned locks"" before,
>>>>> http://comments.gmane.org/gmane.linux.nfs/64382
>>>>>
>>>>> nfsd needs the hashtbl to find the lockowner for locking by owner from
>>>>> fl->fl_owner stored in struct file_lock, but without nfs_client.
>>>>
>>>> Why? We're not currently doing that.
>>>
>>> Although not doing that right now, but there is a bug for getting the right ld_owner
>>> in nfs4_set_lock_denied.
>>>
>>> If denying locks, vfs don't copy fl->fl_lmops to the returned file_lock, so that,
>>> fl->fl_lmops always be NULL, nfsd never return the owner who holds the conflock.
>>>
>>> If we want fix this problem, needs finding the lockowner from struct file_lock.
>>
>> Do we really care enough about fixing nfs4_set_lock_denied enough to
>> do so at the cost of reducing overall scalability of locking state?
>
> I just report this problem, don't think enough about the scalability.
>
>> We will always be faking up the clientid etc for local locks. Are
>> there any clients out there that actually inspect the clientid on a
>> result of NFS4ERR_DENIED and that will break if we give them a fake
>> for non-local locks?
>
> Jeff has point the same problem of a non-nfs4_lockowner.
> Maybe we should copy fl_lmops to conflock as before, nfsd can distinguish
> the lockowner stored in struct file_lock by checking fl_lmops.
>

Alternatively, set a flag in fl_flags. Back in the days, we used to
have a FL_NFSD, perhaps it is time to resurrect that?

Cheers,
  Trond

Jeff Layton Aug. 2, 2014, 10:59 p.m. UTC | #9

On Sat, 2 Aug 2014 10:47:27 -0400
Trond Myklebust <[e-mail]> wrote:

> On Sat, Aug 2, 2014 at 10:20 AM, Kinglong Mee <[e-mail]> wrote:
> > On 8/2/2014 22:05, Trond Myklebust wrote:
> >> On Sat, Aug 2, 2014 at 9:43 AM, Kinglong Mee <[e-mail]> wrote:
> >>> On 8/2/2014 21:11, Trond Myklebust wrote:
> >>>> On Sat, Aug 2, 2014 at 6:39 AM, Kinglong Mee <[e-mail]> wrote:
> >>>>> On 7/30/2014 09:34, Jeff Layton wrote:
> >>>>>> From: Trond Myklebust <[e-mail]>
> >>>>>>
> >>>>>> Preparation for removing the client_mutex.
> >>>>>>
> >>>>>> Convert the open owner hash table into a per-client table and protect it
> >>>>>> using the nfs4_client->cl_lock spin lock.
> >>>>>>
> >>>>>> Signed-off-by: Trond Myklebust <[e-mail]>
> >>>>>> ---
> >>>>>>  fs/nfsd/netns.h     |   1 -
> >>>>>>  fs/nfsd/nfs4state.c | 187 ++++++++++++++++++++++++----------------------------
> >>>>>>  fs/nfsd/state.h     |   1 +
> >>>>>>  3 files changed, 86 insertions(+), 103 deletions(-)
> >>>>>>
> >>>>>> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> >>>>>> index a71d14413d39..e1f479c162b5 100644
> >>>>>> --- a/fs/nfsd/netns.h
> >>>>>> +++ b/fs/nfsd/netns.h
> >>>>>> @@ -63,7 +63,6 @@ struct nfsd_net {
> >>>>>>       struct rb_root conf_name_tree;
> >>>>>>       struct list_head *unconf_id_hashtbl;
> >>>>>>       struct rb_root unconf_name_tree;
> >>>>>> -     struct list_head *ownerstr_hashtbl;
> >>>>>
> >>>>> I send a patch "NFSD: Rervert "knfsd: locks: flag NFSv4-owned locks"" before,
> >>>>> http://comments.gmane.org/gmane.linux.nfs/64382
> >>>>>
> >>>>> nfsd needs the hashtbl to find the lockowner for locking by owner from
> >>>>> fl->fl_owner stored in struct file_lock, but without nfs_client.
> >>>>
> >>>> Why? We're not currently doing that.
> >>>
> >>> Although not doing that right now, but there is a bug for getting the right ld_owner
> >>> in nfs4_set_lock_denied.
> >>>
> >>> If denying locks, vfs don't copy fl->fl_lmops to the returned file_lock, so that,
> >>> fl->fl_lmops always be NULL, nfsd never return the owner who holds the conflock.
> >>>
> >>> If we want fix this problem, needs finding the lockowner from struct file_lock.
> >>
> >> Do we really care enough about fixing nfs4_set_lock_denied enough to
> >> do so at the cost of reducing overall scalability of locking state?
> >
> > I just report this problem, don't think enough about the scalability.
> >
> >> We will always be faking up the clientid etc for local locks. Are
> >> there any clients out there that actually inspect the clientid on a
> >> result of NFS4ERR_DENIED and that will break if we give them a fake
> >> for non-local locks?
> >
> > Jeff has point the same problem of a non-nfs4_lockowner.
> > Maybe we should copy fl_lmops to conflock as before, nfsd can distinguish
> > the lockowner stored in struct file_lock by checking fl_lmops.
> >
> 
> Alternatively, set a flag in fl_flags. Back in the days, we used to
> have a FL_NFSD, perhaps it is time to resurrect that?
> 

Would we need a similar flag for lockd too?

I'm not sure a flag is the correct approach for this. A "fl_lmtype" field
or something similar might make sense, but I'm not sure that really
adds much over just ensuring that fl_lmops is set properly for these locks.

That said, we definitely will need to ensure that there are no harmful
effects from setting fl_lmops on a conflock container. I don't see any
right offhand, but it's probably reasonable to put something like that
in -next for a bit of soak time...

Trond Myklebust Aug. 3, 2014, 1:59 a.m. UTC | #10

On Sat, Aug 2, 2014 at 6:59 PM, Jeff Layton <[e-mail]> wrote:
> On Sat, 2 Aug 2014 10:47:27 -0400
> Trond Myklebust <[e-mail]> wrote:
>
>> On Sat, Aug 2, 2014 at 10:20 AM, Kinglong Mee <[e-mail]> wrote:
>> > On 8/2/2014 22:05, Trond Myklebust wrote:
>> >> On Sat, Aug 2, 2014 at 9:43 AM, Kinglong Mee <[e-mail]> wrote:
>> >>> On 8/2/2014 21:11, Trond Myklebust wrote:
>> >>>> On Sat, Aug 2, 2014 at 6:39 AM, Kinglong Mee <[e-mail]> wrote:
>> >>>>> On 7/30/2014 09:34, Jeff Layton wrote:
>> >>>>>> From: Trond Myklebust <[e-mail]>
>> >>>>>>
>> >>>>>> Preparation for removing the client_mutex.
>> >>>>>>
>> >>>>>> Convert the open owner hash table into a per-client table and protect it
>> >>>>>> using the nfs4_client->cl_lock spin lock.
>> >>>>>>
>> >>>>>> Signed-off-by: Trond Myklebust <[e-mail]>
>> >>>>>> ---
>> >>>>>>  fs/nfsd/netns.h     |   1 -
>> >>>>>>  fs/nfsd/nfs4state.c | 187 ++++++++++++++++++++++++----------------------------
>> >>>>>>  fs/nfsd/state.h     |   1 +
>> >>>>>>  3 files changed, 86 insertions(+), 103 deletions(-)
>> >>>>>>
>> >>>>>> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
>> >>>>>> index a71d14413d39..e1f479c162b5 100644
>> >>>>>> --- a/fs/nfsd/netns.h
>> >>>>>> +++ b/fs/nfsd/netns.h
>> >>>>>> @@ -63,7 +63,6 @@ struct nfsd_net {
>> >>>>>>       struct rb_root conf_name_tree;
>> >>>>>>       struct list_head *unconf_id_hashtbl;
>> >>>>>>       struct rb_root unconf_name_tree;
>> >>>>>> -     struct list_head *ownerstr_hashtbl;
>> >>>>>
>> >>>>> I send a patch "NFSD: Rervert "knfsd: locks: flag NFSv4-owned locks"" before,
>> >>>>> http://comments.gmane.org/gmane.linux.nfs/64382
>> >>>>>
>> >>>>> nfsd needs the hashtbl to find the lockowner for locking by owner from
>> >>>>> fl->fl_owner stored in struct file_lock, but without nfs_client.
>> >>>>
>> >>>> Why? We're not currently doing that.
>> >>>
>> >>> Although not doing that right now, but there is a bug for getting the right ld_owner
>> >>> in nfs4_set_lock_denied.
>> >>>
>> >>> If denying locks, vfs don't copy fl->fl_lmops to the returned file_lock, so that,
>> >>> fl->fl_lmops always be NULL, nfsd never return the owner who holds the conflock.
>> >>>
>> >>> If we want fix this problem, needs finding the lockowner from struct file_lock.
>> >>
>> >> Do we really care enough about fixing nfs4_set_lock_denied enough to
>> >> do so at the cost of reducing overall scalability of locking state?
>> >
>> > I just report this problem, don't think enough about the scalability.
>> >
>> >> We will always be faking up the clientid etc for local locks. Are
>> >> there any clients out there that actually inspect the clientid on a
>> >> result of NFS4ERR_DENIED and that will break if we give them a fake
>> >> for non-local locks?
>> >
>> > Jeff has point the same problem of a non-nfs4_lockowner.
>> > Maybe we should copy fl_lmops to conflock as before, nfsd can distinguish
>> > the lockowner stored in struct file_lock by checking fl_lmops.
>> >
>>
>> Alternatively, set a flag in fl_flags. Back in the days, we used to
>> have a FL_NFSD, perhaps it is time to resurrect that?
>>
>
> Would we need a similar flag for lockd too?
>
> I'm not sure a flag is the correct approach for this. A "fl_lmtype" field
> or something similar might make sense, but I'm not sure that really
> adds much over just ensuring that fl_lmops is set properly for these locks.

It avoids adding an extra copy of fl_lmops that would only be useful
to knfsd as a flag.

> That said, we definitely will need to ensure that there are no harmful
> effects from setting fl_lmops on a conflock container. I don't see any
> right offhand, but it's probably reasonable to put something like that
> in -next for a bit of soak time...

The real problem here is that you are copying the lock, and then
trying to dereference the copied pointer without ensuring that the
thing the pointer is referencing still exists. Given that we're
removing the global state mutex, then it is now perfectly possible for
a competing knfsd thread to free the conflicting lock and the
lockowner pointed to by fl->fl_owner before your thread hits
nfs4_set_lock_denied.
The check for fl_lmtype does nothing to fix that race (nor does FL_NFSD).

Cheers
  Trond

Kinglong Mee Aug. 3, 2014, 2:15 p.m. UTC | #11

On 8/3/2014 19:35, Jeffrey Layton wrote:
> On Sat, Aug 2, 2014 at 9:59 PM, Trond Myklebust <[e-mail] <mailto:[e-mail]>> wrote:
> 
>     On Sat, Aug 2, 2014 at 6:59 PM, Jeff Layton <[e-mail] <mailto:[e-mail]>> wrote:
>     > On Sat, 2 Aug 2014 10:47:27 -0400
>     > Trond Myklebust <[e-mail] <mailto:[e-mail]>> wrote:
>     >
>     >> On Sat, Aug 2, 2014 at 10:20 AM, Kinglong Mee <[e-mail] <mailto:[e-mail]>> wrote:
>     >> > On 8/2/2014 22:05, Trond Myklebust wrote:
>     >> >> On Sat, Aug 2, 2014 at 9:43 AM, Kinglong Mee <[e-mail] <mailto:[e-mail]>> wrote:
>     >> >>> On 8/2/2014 21:11, Trond Myklebust wrote:
>     >> >>>> On Sat, Aug 2, 2014 at 6:39 AM, Kinglong Mee <[e-mail] <mailto:[e-mail]>> wrote:
>     >> >>>>> On 7/30/2014 09:34, Jeff Layton wrote:
>     >> >>>>>> From: Trond Myklebust <[e-mail] <mailto:[e-mail]>>
>     >> >>>>>>
>     >> >>>>>> Preparation for removing the client_mutex.
>     >> >>>>>>
>     >> >>>>>> Convert the open owner hash table into a per-client table and protect it
>     >> >>>>>> using the nfs4_client->cl_lock spin lock.
>     >> >>>>>>
>     >> >>>>>> Signed-off-by: Trond Myklebust <[e-mail] <mailto:[e-mail]>>
>     >> >>>>>> ---
>     >> >>>>>>  fs/nfsd/netns.h     |   1 -
>     >> >>>>>>  fs/nfsd/nfs4state.c | 187 ++++++++++++++++++++++++----------------------------
>     >> >>>>>>  fs/nfsd/state.h     |   1 +
>     >> >>>>>>  3 files changed, 86 insertions(+), 103 deletions(-)
>     >> >>>>>>
>     >> >>>>>> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
>     >> >>>>>> index a71d14413d39..e1f479c162b5 100644
>     >> >>>>>> --- a/fs/nfsd/netns.h
>     >> >>>>>> +++ b/fs/nfsd/netns.h
>     >> >>>>>> @@ -63,7 +63,6 @@ struct nfsd_net {
>     >> >>>>>>       struct rb_root conf_name_tree;
>     >> >>>>>>       struct list_head *unconf_id_hashtbl;
>     >> >>>>>>       struct rb_root unconf_name_tree;
>     >> >>>>>> -     struct list_head *ownerstr_hashtbl;
>     >> >>>>>
>     >> >>>>> I send a patch "NFSD: Rervert "knfsd: locks: flag NFSv4-owned locks"" before,
>     >> >>>>> http://comments.gmane.org/gmane.linux.nfs/64382
>     >> >>>>>
>     >> >>>>> nfsd needs the hashtbl to find the lockowner for locking by owner from
>     >> >>>>> fl->fl_owner stored in struct file_lock, but without nfs_client.
>     >> >>>>
>     >> >>>> Why? We're not currently doing that.
>     >> >>>
>     >> >>> Although not doing that right now, but there is a bug for getting the right ld_owner
>     >> >>> in nfs4_set_lock_denied.
>     >> >>>
>     >> >>> If denying locks, vfs don't copy fl->fl_lmops to the returned file_lock, so that,
>     >> >>> fl->fl_lmops always be NULL, nfsd never return the owner who holds the conflock.
>     >> >>>
>     >> >>> If we want fix this problem, needs finding the lockowner from struct file_lock.
>     >> >>
>     >> >> Do we really care enough about fixing nfs4_set_lock_denied enough to
>     >> >> do so at the cost of reducing overall scalability of locking state?
>     >> >
>     >> > I just report this problem, don't think enough about the scalability.
>     >> >
>     >> >> We will always be faking up the clientid etc for local locks. Are
>     >> >> there any clients out there that actually inspect the clientid on a
>     >> >> result of NFS4ERR_DENIED and that will break if we give them a fake
>     >> >> for non-local locks?
>     >> >
>     >> > Jeff has point the same problem of a non-nfs4_lockowner.
>     >> > Maybe we should copy fl_lmops to conflock as before, nfsd can distinguish
>     >> > the lockowner stored in struct file_lock by checking fl_lmops.
>     >> >
>     >>
>     >> Alternatively, set a flag in fl_flags. Back in the days, we used to
>     >> have a FL_NFSD, perhaps it is time to resurrect that?
>     >>
>     >
>     > Would we need a similar flag for lockd too?
>     >
>     > I'm not sure a flag is the correct approach for this. A "fl_lmtype" field
>     > or something similar might make sense, but I'm not sure that really
>     > adds much over just ensuring that fl_lmops is set properly for these locks.
> 
>     It avoids adding an extra copy of fl_lmops that would only be useful
>     to knfsd as a flag.
> 
>     > That said, we definitely will need to ensure that there are no harmful
>     > effects from setting fl_lmops on a conflock container. I don't see any
>     > right offhand, but it's probably reasonable to put something like that
>     > in -next for a bit of soak time...
> 
>     The real problem here is that you are copying the lock, and then
>     trying to dereference the copied pointer without ensuring that the
>     thing the pointer is referencing still exists. Given that we're
>     removing the global state mutex, then it is now perfectly possible for
>     a competing knfsd thread to free the conflicting lock and the
>     lockowner pointed to by fl->fl_owner before your thread hits
>     nfs4_set_lock_denied.
>     The check for fl_lmtype does nothing to fix that race (nor does FL_NFSD).
> 
>     Cheers
>       Trond
> 
> 
> Agreed. That's a significant bit of nastiness, and I think you're correct that the
> client_mutex removal will exacerbate the problem. The only way I can see to
> properly fix that would be to make it so that the conflock holds a reference to the
> lockowner, and then put it when the conflock is freed.
> 

I will check the reference to lockowner in nfsd, and try to simulate client-id expire.

> That will require some new fl_ops or fl_lmops. It would also be good to clean up
> conflock generation a bit, and turn it into a more explicit process.
> __locks_copy_lock doesn't really fully describe what's going on there, IMO...

I will have a try.

thanks,
Kinglong Mee
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jeff Layton Aug. 3, 2014, 2:49 p.m. UTC | #12

On Sun, 03 Aug 2014 22:15:05 +0800
Kinglong Mee <[e-mail]> wrote:

> On 8/3/2014 19:35, Jeffrey Layton wrote:
> > On Sat, Aug 2, 2014 at 9:59 PM, Trond Myklebust <[e-mail] <mailto:[e-mail]>> wrote:
> > 
> >     On Sat, Aug 2, 2014 at 6:59 PM, Jeff Layton <[e-mail] <mailto:[e-mail]>> wrote:
> >     > On Sat, 2 Aug 2014 10:47:27 -0400
> >     > Trond Myklebust <[e-mail] <mailto:[e-mail]>> wrote:
> >     >
> >     >> On Sat, Aug 2, 2014 at 10:20 AM, Kinglong Mee <[e-mail] <mailto:[e-mail]>> wrote:
> >     >> > On 8/2/2014 22:05, Trond Myklebust wrote:
> >     >> >> On Sat, Aug 2, 2014 at 9:43 AM, Kinglong Mee <[e-mail] <mailto:[e-mail]>> wrote:
> >     >> >>> On 8/2/2014 21:11, Trond Myklebust wrote:
> >     >> >>>> On Sat, Aug 2, 2014 at 6:39 AM, Kinglong Mee <[e-mail] <mailto:[e-mail]>> wrote:
> >     >> >>>>> On 7/30/2014 09:34, Jeff Layton wrote:
> >     >> >>>>>> From: Trond Myklebust <[e-mail] <mailto:[e-mail]>>
> >     >> >>>>>>
> >     >> >>>>>> Preparation for removing the client_mutex.
> >     >> >>>>>>
> >     >> >>>>>> Convert the open owner hash table into a per-client table and protect it
> >     >> >>>>>> using the nfs4_client->cl_lock spin lock.
> >     >> >>>>>>
> >     >> >>>>>> Signed-off-by: Trond Myklebust <[e-mail] <mailto:[e-mail]>>
> >     >> >>>>>> ---
> >     >> >>>>>>  fs/nfsd/netns.h     |   1 -
> >     >> >>>>>>  fs/nfsd/nfs4state.c | 187 ++++++++++++++++++++++++----------------------------
> >     >> >>>>>>  fs/nfsd/state.h     |   1 +
> >     >> >>>>>>  3 files changed, 86 insertions(+), 103 deletions(-)
> >     >> >>>>>>
> >     >> >>>>>> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> >     >> >>>>>> index a71d14413d39..e1f479c162b5 100644
> >     >> >>>>>> --- a/fs/nfsd/netns.h
> >     >> >>>>>> +++ b/fs/nfsd/netns.h
> >     >> >>>>>> @@ -63,7 +63,6 @@ struct nfsd_net {
> >     >> >>>>>>       struct rb_root conf_name_tree;
> >     >> >>>>>>       struct list_head *unconf_id_hashtbl;
> >     >> >>>>>>       struct rb_root unconf_name_tree;
> >     >> >>>>>> -     struct list_head *ownerstr_hashtbl;
> >     >> >>>>>
> >     >> >>>>> I send a patch "NFSD: Rervert "knfsd: locks: flag NFSv4-owned locks"" before,
> >     >> >>>>> http://comments.gmane.org/gmane.linux.nfs/64382
> >     >> >>>>>
> >     >> >>>>> nfsd needs the hashtbl to find the lockowner for locking by owner from
> >     >> >>>>> fl->fl_owner stored in struct file_lock, but without nfs_client.
> >     >> >>>>
> >     >> >>>> Why? We're not currently doing that.
> >     >> >>>
> >     >> >>> Although not doing that right now, but there is a bug for getting the right ld_owner
> >     >> >>> in nfs4_set_lock_denied.
> >     >> >>>
> >     >> >>> If denying locks, vfs don't copy fl->fl_lmops to the returned file_lock, so that,
> >     >> >>> fl->fl_lmops always be NULL, nfsd never return the owner who holds the conflock.
> >     >> >>>
> >     >> >>> If we want fix this problem, needs finding the lockowner from struct file_lock.
> >     >> >>
> >     >> >> Do we really care enough about fixing nfs4_set_lock_denied enough to
> >     >> >> do so at the cost of reducing overall scalability of locking state?
> >     >> >
> >     >> > I just report this problem, don't think enough about the scalability.
> >     >> >
> >     >> >> We will always be faking up the clientid etc for local locks. Are
> >     >> >> there any clients out there that actually inspect the clientid on a
> >     >> >> result of NFS4ERR_DENIED and that will break if we give them a fake
> >     >> >> for non-local locks?
> >     >> >
> >     >> > Jeff has point the same problem of a non-nfs4_lockowner.
> >     >> > Maybe we should copy fl_lmops to conflock as before, nfsd can distinguish
> >     >> > the lockowner stored in struct file_lock by checking fl_lmops.
> >     >> >
> >     >>
> >     >> Alternatively, set a flag in fl_flags. Back in the days, we used to
> >     >> have a FL_NFSD, perhaps it is time to resurrect that?
> >     >>
> >     >
> >     > Would we need a similar flag for lockd too?
> >     >
> >     > I'm not sure a flag is the correct approach for this. A "fl_lmtype" field
> >     > or something similar might make sense, but I'm not sure that really
> >     > adds much over just ensuring that fl_lmops is set properly for these locks.
> > 
> >     It avoids adding an extra copy of fl_lmops that would only be useful
> >     to knfsd as a flag.
> > 
> >     > That said, we definitely will need to ensure that there are no harmful
> >     > effects from setting fl_lmops on a conflock container. I don't see any
> >     > right offhand, but it's probably reasonable to put something like that
> >     > in -next for a bit of soak time...
> > 
> >     The real problem here is that you are copying the lock, and then
> >     trying to dereference the copied pointer without ensuring that the
> >     thing the pointer is referencing still exists. Given that we're
> >     removing the global state mutex, then it is now perfectly possible for
> >     a competing knfsd thread to free the conflicting lock and the
> >     lockowner pointed to by fl->fl_owner before your thread hits
> >     nfs4_set_lock_denied.
> >     The check for fl_lmtype does nothing to fix that race (nor does FL_NFSD).
> > 
> >     Cheers
> >       Trond
> > 
> > 
> > Agreed. That's a significant bit of nastiness, and I think you're correct that the
> > client_mutex removal will exacerbate the problem. The only way I can see to
> > properly fix that would be to make it so that the conflock holds a reference to the
> > lockowner, and then put it when the conflock is freed.
> > 
> 
> I will check the reference to lockowner in nfsd, and try to simulate client-id expire.
> 

You shouldn't need to do anything special there. It should be
sufficient to simply take a reference to the lockowner when you copy
the data to the conflock, and then put that reference when you go to
free it.

> > That will require some new fl_ops or fl_lmops. It would also be good to clean up
> > conflock generation a bit, and turn it into a more explicit process.
> > __locks_copy_lock doesn't really fully describe what's going on there, IMO...
> 
> I will have a try.
> 
> thanks,
> Kinglong Mee

Thanks. For now, I'm going to drop the patch that you proposed
yesterday since Trond's quite correct that there's more work needed in
this area before that's really safe.

J. Bruce Fields Aug. 5, 2014, 6:46 p.m. UTC | #13

On Sun, Aug 03, 2014 at 07:35:37AM -0400, Jeffrey Layton wrote:
> Agreed. That's a significant bit of nastiness, and I think you're
> correct that the client_mutex removal will exacerbate the problem. The
> only way I can see to properly fix that would be to make it so that
> the conflock holds a reference to the lockowner, and then put it when
> the conflock is freed.
> 
> That will require some new fl_ops or fl_lmops. It would also be good
> to clean up conflock generation a bit, and turn it into a more
> explicit process.  __locks_copy_lock doesn't really fully describe
> what's going on there, IMO...

Or do the work of nfs4_set_lock_denied under the i_lock in some sort of
callback from the locking code?

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
index a71d14413d39..e1f479c162b5 100644
--- a/fs/nfsd/netns.h
+++ b/fs/nfsd/netns.h
@@ -63,7 +63,6 @@  struct nfsd_net {
 	struct rb_root conf_name_tree;
 	struct list_head *unconf_id_hashtbl;
 	struct rb_root unconf_name_tree;
-	struct list_head *ownerstr_hashtbl;
 	struct list_head *sessionid_hashtbl;
 	/*
 	 * client_lru holds client queue ordered by nfs4_client.cl_time
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 7c15918d20f0..4af4e5eff491 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -240,35 +240,27 @@  static void nfsd4_put_session(struct nfsd4_session *ses)
 }
 
 static int
-same_owner_str(struct nfs4_stateowner *sop, struct xdr_netobj *owner,
-							clientid_t *clid)
+same_owner_str(struct nfs4_stateowner *sop, struct xdr_netobj *owner)
 {
 	return (sop->so_owner.len == owner->len) &&
-		0 == memcmp(sop->so_owner.data, owner->data, owner->len) &&
-		(sop->so_client->cl_clientid.cl_id == clid->cl_id);
+		0 == memcmp(sop->so_owner.data, owner->data, owner->len);
 }
 
 static struct nfs4_openowner *
 find_openstateowner_str_locked(unsigned int hashval, struct nfsd4_open *open,
-			bool sessions, struct nfsd_net *nn)
+			struct nfs4_client *clp)
 {
 	struct nfs4_stateowner *so;
-	struct nfs4_openowner *oo;
-	struct nfs4_client *clp;
 
-	lockdep_assert_held(&nn->client_lock);
+	lockdep_assert_held(&clp->cl_lock);
 
-	list_for_each_entry(so, &nn->ownerstr_hashtbl[hashval], so_strhash) {
+	list_for_each_entry(so, &clp->cl_ownerstr_hashtbl[hashval],
+			    so_strhash) {
 		if (!so->so_is_open_owner)
 			continue;
-		if (same_owner_str(so, &open->op_owner, &open->op_clientid)) {
-			oo = openowner(so);
-			clp = oo->oo_owner.so_client;
-			if ((bool)clp->cl_minorversion != sessions)
-				break;
-			renew_client_locked(clp);
+		if (same_owner_str(so, &open->op_owner)) {
 			atomic_inc(&so->so_count);
-			return oo;
+			return openowner(so);
 		}
 	}
 	return NULL;
@@ -276,17 +268,16 @@  find_openstateowner_str_locked(unsigned int hashval, struct nfsd4_open *open,
 
 static struct nfs4_openowner *
 find_openstateowner_str(unsigned int hashval, struct nfsd4_open *open,
-			bool sessions, struct nfsd_net *nn)
+			struct nfs4_client *clp)
 {
 	struct nfs4_openowner *oo;
 
-	spin_lock(&nn->client_lock);
-	oo = find_openstateowner_str_locked(hashval, open, sessions, nn);
-	spin_unlock(&nn->client_lock);
+	spin_lock(&clp->cl_lock);
+	oo = find_openstateowner_str_locked(hashval, open, clp);
+	spin_unlock(&clp->cl_lock);
 	return oo;
 }
 
-
 static inline u32
 opaque_hashval(const void *ptr, int nbytes)
 {
@@ -408,12 +399,11 @@  unsigned long max_delegations;
 #define OWNER_HASH_SIZE             (1 << OWNER_HASH_BITS)
 #define OWNER_HASH_MASK             (OWNER_HASH_SIZE - 1)
 
-static unsigned int ownerstr_hashval(u32 clientid, struct xdr_netobj *ownername)
+static unsigned int ownerstr_hashval(struct xdr_netobj *ownername)
 {
 	unsigned int ret;
 
 	ret = opaque_hashval(ownername->data, ownername->len);
-	ret += clientid;
 	return ret & OWNER_HASH_MASK;
 }
 
@@ -1002,40 +992,37 @@  static void release_lock_stateid(struct nfs4_ol_stateid *stp)
 
 static void unhash_lockowner_locked(struct nfs4_lockowner *lo)
 {
-	struct nfsd_net *nn = net_generic(lo->lo_owner.so_client->net,
-						nfsd_net_id);
+	struct nfs4_client *clp = lo->lo_owner.so_client;
 
-	lockdep_assert_held(&nn->client_lock);
+	lockdep_assert_held(&clp->cl_lock);
 
 	list_del_init(&lo->lo_owner.so_strhash);
 }
 
 static void release_lockowner_stateids(struct nfs4_lockowner *lo)
 {
-	struct nfsd_net *nn = net_generic(lo->lo_owner.so_client->net,
-						nfsd_net_id);
+	struct nfs4_client *clp = lo->lo_owner.so_client;
 	struct nfs4_ol_stateid *stp;
 
-	lockdep_assert_held(&nn->client_lock);
+	lockdep_assert_held(&clp->cl_lock);
 
 	while (!list_empty(&lo->lo_owner.so_stateids)) {
 		stp = list_first_entry(&lo->lo_owner.so_stateids,
 				struct nfs4_ol_stateid, st_perstateowner);
-		spin_unlock(&nn->client_lock);
+		spin_unlock(&clp->cl_lock);
 		release_lock_stateid(stp);
-		spin_lock(&nn->client_lock);
+		spin_lock(&clp->cl_lock);
 	}
 }
 
 static void release_lockowner(struct nfs4_lockowner *lo)
 {
-	struct nfsd_net *nn = net_generic(lo->lo_owner.so_client->net,
-						nfsd_net_id);
+	struct nfs4_client *clp = lo->lo_owner.so_client;
 
-	spin_lock(&nn->client_lock);
+	spin_lock(&clp->cl_lock);
 	unhash_lockowner_locked(lo);
 	release_lockowner_stateids(lo);
-	spin_unlock(&nn->client_lock);
+	spin_unlock(&clp->cl_lock);
 	nfs4_put_stateowner(&lo->lo_owner);
 }
 
@@ -1070,10 +1057,9 @@  static void release_open_stateid(struct nfs4_ol_stateid *stp)
 
 static void unhash_openowner_locked(struct nfs4_openowner *oo)
 {
-	struct nfsd_net *nn = net_generic(oo->oo_owner.so_client->net,
-						nfsd_net_id);
+	struct nfs4_client *clp = oo->oo_owner.so_client;
 
-	lockdep_assert_held(&nn->client_lock);
+	lockdep_assert_held(&clp->cl_lock);
 
 	list_del_init(&oo->oo_owner.so_strhash);
 	list_del_init(&oo->oo_perclient);
@@ -1093,29 +1079,27 @@  static void release_last_closed_stateid(struct nfs4_openowner *oo)
 static void release_openowner_stateids(struct nfs4_openowner *oo)
 {
 	struct nfs4_ol_stateid *stp;
-	struct nfsd_net *nn = net_generic(oo->oo_owner.so_client->net,
-						nfsd_net_id);
+	struct nfs4_client *clp = oo->oo_owner.so_client;
 
-	lockdep_assert_held(&nn->client_lock);
+	lockdep_assert_held(&clp->cl_lock);
 
 	while (!list_empty(&oo->oo_owner.so_stateids)) {
 		stp = list_first_entry(&oo->oo_owner.so_stateids,
 				struct nfs4_ol_stateid, st_perstateowner);
-		spin_unlock(&nn->client_lock);
+		spin_unlock(&clp->cl_lock);
 		release_open_stateid(stp);
-		spin_lock(&nn->client_lock);
+		spin_lock(&clp->cl_lock);
 	}
 }
 
 static void release_openowner(struct nfs4_openowner *oo)
 {
-	struct nfsd_net *nn = net_generic(oo->oo_owner.so_client->net,
-						nfsd_net_id);
+	struct nfs4_client *clp = oo->oo_owner.so_client;
 
-	spin_lock(&nn->client_lock);
+	spin_lock(&clp->cl_lock);
 	unhash_openowner_locked(oo);
 	release_openowner_stateids(oo);
-	spin_unlock(&nn->client_lock);
+	spin_unlock(&clp->cl_lock);
 	release_last_closed_stateid(oo);
 	nfs4_put_stateowner(&oo->oo_owner);
 }
@@ -1497,15 +1481,20 @@  STALE_CLIENTID(clientid_t *clid, struct nfsd_net *nn)
 static struct nfs4_client *alloc_client(struct xdr_netobj name)
 {
 	struct nfs4_client *clp;
+	int i;
 
 	clp = kzalloc(sizeof(struct nfs4_client), GFP_KERNEL);
 	if (clp == NULL)
 		return NULL;
 	clp->cl_name.data = kmemdup(name.data, name.len, GFP_KERNEL);
-	if (clp->cl_name.data == NULL) {
-		kfree(clp);
-		return NULL;
-	}
+	if (clp->cl_name.data == NULL)
+		goto err_no_name;
+	clp->cl_ownerstr_hashtbl = kmalloc(sizeof(struct list_head) *
+			OWNER_HASH_SIZE, GFP_KERNEL);
+	if (!clp->cl_ownerstr_hashtbl)
+		goto err_no_hashtbl;
+	for (i = 0; i < OWNER_HASH_SIZE; i++)
+		INIT_LIST_HEAD(&clp->cl_ownerstr_hashtbl[i]);
 	clp->cl_name.len = name.len;
 	INIT_LIST_HEAD(&clp->cl_sessions);
 	idr_init(&clp->cl_stateids);
@@ -1520,6 +1509,11 @@  static struct nfs4_client *alloc_client(struct xdr_netobj name)
 	spin_lock_init(&clp->cl_lock);
 	rpc_init_wait_queue(&clp->cl_cb_waitq, "Backchannel slot table");
 	return clp;
+err_no_hashtbl:
+	kfree(clp->cl_name.data);
+err_no_name:
+	kfree(clp);
+	return NULL;
 }
 
 static void
@@ -1538,6 +1532,7 @@  free_client(struct nfs4_client *clp)
 	}
 	rpc_destroy_wait_queue(&clp->cl_cb_waitq);
 	free_svc_cred(&clp->cl_cred);
+	kfree(clp->cl_ownerstr_hashtbl);
 	kfree(clp->cl_name.data);
 	idr_destroy(&clp->cl_stateids);
 	kfree(clp);
@@ -3074,20 +3069,20 @@  static inline void *alloc_stateowner(struct kmem_cache *slab, struct xdr_netobj
 
 static void hash_openowner(struct nfs4_openowner *oo, struct nfs4_client *clp, unsigned int strhashval)
 {
-	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
+	lockdep_assert_held(&clp->cl_lock);
 
-	list_add(&oo->oo_owner.so_strhash, &nn->ownerstr_hashtbl[strhashval]);
+	list_add(&oo->oo_owner.so_strhash,
+		 &clp->cl_ownerstr_hashtbl[strhashval]);
 	list_add(&oo->oo_perclient, &clp->cl_openowners);
 }
 
 static void nfs4_unhash_openowner(struct nfs4_stateowner *so)
 {
-	struct nfs4_openowner *oo = openowner(so);
-	struct nfsd_net *nn = net_generic(so->so_client->net, nfsd_net_id);
+	struct nfs4_client *clp = so->so_client;
 
-	spin_lock(&nn->client_lock);
-	unhash_openowner_locked(oo);
-	spin_unlock(&nn->client_lock);
+	spin_lock(&clp->cl_lock);
+	unhash_openowner_locked(openowner(so));
+	spin_unlock(&clp->cl_lock);
 }
 
 static void nfs4_free_openowner(struct nfs4_stateowner *so)
@@ -3107,7 +3102,6 @@  alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
 			   struct nfsd4_compound_state *cstate)
 {
 	struct nfs4_client *clp = cstate->clp;
-	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
 	struct nfs4_openowner *oo, *ret;
 
 	oo = alloc_stateowner(openowner_slab, &open->op_owner, clp);
@@ -3122,15 +3116,14 @@  alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
 	oo->oo_time = 0;
 	oo->oo_last_closed_stid = NULL;
 	INIT_LIST_HEAD(&oo->oo_close_lru);
-	spin_lock(&nn->client_lock);
-	ret = find_openstateowner_str_locked(strhashval,
-			open, clp->cl_minorversion, nn);
+	spin_lock(&clp->cl_lock);
+	ret = find_openstateowner_str_locked(strhashval, open, clp);
 	if (ret == NULL) {
 		hash_openowner(oo, clp, strhashval);
 		ret = oo;
 	} else
 		nfs4_free_openowner(&oo->oo_owner);
-	spin_unlock(&nn->client_lock);
+	spin_unlock(&clp->cl_lock);
 	return oo;
 }
 
@@ -3412,8 +3405,8 @@  nfsd4_process_open1(struct nfsd4_compound_state *cstate,
 		return status;
 	clp = cstate->clp;
 
-	strhashval = ownerstr_hashval(clientid->cl_id, &open->op_owner);
-	oo = find_openstateowner_str(strhashval, open, cstate->minorversion, nn);
+	strhashval = ownerstr_hashval(&open->op_owner);
+	oo = find_openstateowner_str(strhashval, open, clp);
 	open->op_openowner = oo;
 	if (!oo) {
 		goto new_owner;
@@ -4818,15 +4811,16 @@  nevermind:
 
 static struct nfs4_lockowner *
 find_lockowner_str_locked(clientid_t *clid, struct xdr_netobj *owner,
-		struct nfsd_net *nn)
+		struct nfs4_client *clp)
 {
-	unsigned int strhashval = ownerstr_hashval(clid->cl_id, owner);
+	unsigned int strhashval = ownerstr_hashval(owner);
 	struct nfs4_stateowner *so;
 
-	list_for_each_entry(so, &nn->ownerstr_hashtbl[strhashval], so_strhash) {
+	list_for_each_entry(so, &clp->cl_ownerstr_hashtbl[strhashval],
+			    so_strhash) {
 		if (so->so_is_open_owner)
 			continue;
-		if (!same_owner_str(so, owner, clid))
+		if (!same_owner_str(so, owner))
 			continue;
 		atomic_inc(&so->so_count);
 		return lockowner(so);
@@ -4836,23 +4830,23 @@  find_lockowner_str_locked(clientid_t *clid, struct xdr_netobj *owner,
 
 static struct nfs4_lockowner *
 find_lockowner_str(clientid_t *clid, struct xdr_netobj *owner,
-		struct nfsd_net *nn)
+		struct nfs4_client *clp)
 {
 	struct nfs4_lockowner *lo;
 
-	spin_lock(&nn->client_lock);
-	lo = find_lockowner_str_locked(clid, owner, nn);
-	spin_unlock(&nn->client_lock);
+	spin_lock(&clp->cl_lock);
+	lo = find_lockowner_str_locked(clid, owner, clp);
+	spin_unlock(&clp->cl_lock);
 	return lo;
 }
 
 static void nfs4_unhash_lockowner(struct nfs4_stateowner *sop)
 {
-	struct nfsd_net *nn = net_generic(sop->so_client->net, nfsd_net_id);
+	struct nfs4_client *clp = sop->so_client;
 
-	spin_lock(&nn->client_lock);
+	spin_lock(&clp->cl_lock);
 	unhash_lockowner_locked(lockowner(sop));
-	spin_unlock(&nn->client_lock);
+	spin_unlock(&clp->cl_lock);
 }
 
 static void nfs4_free_lockowner(struct nfs4_stateowner *sop)
@@ -4879,7 +4873,6 @@  alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp,
 			   struct nfs4_ol_stateid *open_stp,
 			   struct nfsd4_lock *lock)
 {
-	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
 	struct nfs4_lockowner *lo, *ret;
 
 	lo = alloc_stateowner(lockowner_slab, &lock->lk_new_owner, clp);
@@ -4889,16 +4882,16 @@  alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp,
 	lo->lo_owner.so_is_open_owner = 0;
 	lo->lo_owner.so_seqid = lock->lk_new_lock_seqid;
 	lo->lo_owner.so_ops = &lockowner_ops;
-	spin_lock(&nn->client_lock);
+	spin_lock(&clp->cl_lock);
 	ret = find_lockowner_str_locked(&clp->cl_clientid,
-			&lock->lk_new_owner, nn);
+			&lock->lk_new_owner, clp);
 	if (ret == NULL) {
 		list_add(&lo->lo_owner.so_strhash,
-			 &nn->ownerstr_hashtbl[strhashval]);
+			 &clp->cl_ownerstr_hashtbl[strhashval]);
 		ret = lo;
 	} else
 		nfs4_free_lockowner(&lo->lo_owner);
-	spin_unlock(&nn->client_lock);
+	spin_unlock(&clp->cl_lock);
 	return lo;
 }
 
@@ -5010,12 +5003,10 @@  lookup_or_create_lock_state(struct nfsd4_compound_state *cstate,
 	struct inode *inode = cstate->current_fh.fh_dentry->d_inode;
 	struct nfs4_lockowner *lo;
 	unsigned int strhashval;
-	struct nfsd_net *nn = net_generic(cl->net, nfsd_net_id);
 
-	lo = find_lockowner_str(&cl->cl_clientid, &lock->v.new.owner, nn);
+	lo = find_lockowner_str(&cl->cl_clientid, &lock->v.new.owner, cl);
 	if (!lo) {
-		strhashval = ownerstr_hashval(cl->cl_clientid.cl_id,
-				&lock->v.new.owner);
+		strhashval = ownerstr_hashval(&lock->v.new.owner);
 		lo = alloc_init_lock_stateowner(strhashval, cl, ost, lock);
 		if (lo == NULL)
 			return nfserr_jukebox;
@@ -5293,7 +5284,8 @@  nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		goto out;
 	}
 
-	lo = find_lockowner_str(&lockt->lt_clientid, &lockt->lt_owner, nn);
+	lo = find_lockowner_str(&lockt->lt_clientid, &lockt->lt_owner,
+				cstate->clp);
 	if (lo)
 		file_lock->fl_owner = (fl_owner_t)lo;
 	file_lock->fl_pid = current->tgid;
@@ -5436,7 +5428,7 @@  nfsd4_release_lockowner(struct svc_rqst *rqstp,
 	struct nfs4_lockowner *lo;
 	struct nfs4_ol_stateid *stp;
 	struct xdr_netobj *owner = &rlockowner->rl_owner;
-	unsigned int hashval = ownerstr_hashval(clid->cl_id, owner);
+	unsigned int hashval = ownerstr_hashval(owner);
 	__be32 status;
 	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
 	struct nfs4_client *clp;
@@ -5452,29 +5444,29 @@  nfsd4_release_lockowner(struct svc_rqst *rqstp,
 
 	status = nfserr_locks_held;
 
+	clp = cstate->clp;
 	/* Find the matching lock stateowner */
-	spin_lock(&nn->client_lock);
-	list_for_each_entry(tmp, &nn->ownerstr_hashtbl[hashval], so_strhash) {
+	spin_lock(&clp->cl_lock);
+	list_for_each_entry(tmp, &clp->cl_ownerstr_hashtbl[hashval],
+			    so_strhash) {
 		if (tmp->so_is_open_owner)
 			continue;
-		if (same_owner_str(tmp, owner, clid)) {
+		if (same_owner_str(tmp, owner)) {
 			sop = tmp;
 			atomic_inc(&sop->so_count);
 			break;
 		}
 	}
-	spin_unlock(&nn->client_lock);
 
 	/* No matching owner found, maybe a replay? Just declare victory... */
 	if (!sop) {
+		spin_unlock(&clp->cl_lock);
 		status = nfs_ok;
 		goto out;
 	}
 
 	lo = lockowner(sop);
 	/* see if there are still any locks associated with it */
-	clp = cstate->clp;
-	spin_lock(&clp->cl_lock);
 	list_for_each_entry(stp, &sop->so_stateids, st_perstateowner) {
 		if (check_for_locks(stp->st_stid.sc_file, lo)) {
 			spin_unlock(&clp->cl_lock);
@@ -5829,10 +5821,6 @@  static int nfs4_state_create_net(struct net *net)
 			CLIENT_HASH_SIZE, GFP_KERNEL);
 	if (!nn->unconf_id_hashtbl)
 		goto err_unconf_id;
-	nn->ownerstr_hashtbl = kmalloc(sizeof(struct list_head) *
-			OWNER_HASH_SIZE, GFP_KERNEL);
-	if (!nn->ownerstr_hashtbl)
-		goto err_ownerstr;
 	nn->sessionid_hashtbl = kmalloc(sizeof(struct list_head) *
 			SESSION_HASH_SIZE, GFP_KERNEL);
 	if (!nn->sessionid_hashtbl)
@@ -5842,8 +5830,6 @@  static int nfs4_state_create_net(struct net *net)
 		INIT_LIST_HEAD(&nn->conf_id_hashtbl[i]);
 		INIT_LIST_HEAD(&nn->unconf_id_hashtbl[i]);
 	}
-	for (i = 0; i < OWNER_HASH_SIZE; i++)
-		INIT_LIST_HEAD(&nn->ownerstr_hashtbl[i]);
 	for (i = 0; i < SESSION_HASH_SIZE; i++)
 		INIT_LIST_HEAD(&nn->sessionid_hashtbl[i]);
 	nn->conf_name_tree = RB_ROOT;
@@ -5859,8 +5845,6 @@  static int nfs4_state_create_net(struct net *net)
 	return 0;
 
 err_sessionid:
-	kfree(nn->ownerstr_hashtbl);
-err_ownerstr:
 	kfree(nn->unconf_id_hashtbl);
 err_unconf_id:
 	kfree(nn->conf_id_hashtbl);
@@ -5890,7 +5874,6 @@  nfs4_state_destroy_net(struct net *net)
 	}
 
 	kfree(nn->sessionid_hashtbl);
-	kfree(nn->ownerstr_hashtbl);
 	kfree(nn->unconf_id_hashtbl);
 	kfree(nn->conf_id_hashtbl);
 	put_net(net);
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index e073c86f389c..73a209dc352b 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -235,6 +235,7 @@  struct nfsd4_sessionid {
 struct nfs4_client {
 	struct list_head	cl_idhash; 	/* hash by cl_clientid.id */
 	struct rb_node		cl_namenode;	/* link into by-name trees */
+	struct list_head	*cl_ownerstr_hashtbl;
 	struct list_head	cl_openowners;
 	struct idr		cl_stateids;	/* stateid lookup */
 	struct list_head	cl_delegations;