diff mbox series

[RFC,1/2] NFSD: nfsd4_release_lockowner() should drop clp->cl_lock sooner

Message ID 165230597191.5906.5961060184718742042.stgit@klimt.1015granger.net (mailing list archive)
State New, archived
Headers show
Series Fix "sleep while locked" in RELEASE_LOCKOWNER | expand

Commit Message

Chuck Lever May 11, 2022, 9:52 p.m. UTC 
nfsd4_release_lockowner() mustn't hold clp->cl_lock when
check_for_locks() invokes nfsd_file_put(), which can sleep.

Reported-by: Dai Ngo <dai.ngo@oracle.com>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Cc: <stable@vger.kernel.org>
---
 fs/nfsd/nfs4state.c |   56 +++++++++++++++++++++++----------------------------
 1 file changed, 25 insertions(+), 31 deletions(-)

Comments

Trond Myklebust May 11, 2022, 10:04 p.m. UTC | #1 
On Wed, 2022-05-11 at 17:52 -0400, Chuck Lever wrote:
> nfsd4_release_lockowner() mustn't hold clp->cl_lock when
> check_for_locks() invokes nfsd_file_put(), which can sleep.
> 
> Reported-by: Dai Ngo <dai.ngo@oracle.com>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> Cc: <stable@vger.kernel.org>
> ---
>  fs/nfsd/nfs4state.c |   56 +++++++++++++++++++++++------------------
> ----------
>  1 file changed, 25 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 234e852fcdfa..e2eb6d031643 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -6611,8 +6611,8 @@ nfs4_set_lock_denied(struct file_lock *fl,
> struct nfsd4_lock_denied *deny)
>                 deny->ld_type = NFS4_WRITE_LT;
>  }
>  
> -static struct nfs4_lockowner *
> -find_lockowner_str_locked(struct nfs4_client *clp, struct xdr_netobj
> *owner)
> +static struct nfs4_stateowner *
> +find_stateowner_str_locked(struct nfs4_client *clp, struct
> xdr_netobj *owner)
>  {
>         unsigned int strhashval = ownerstr_hashval(owner);
>         struct nfs4_stateowner *so;
> @@ -6624,11 +6624,22 @@ find_lockowner_str_locked(struct nfs4_client
> *clp, struct xdr_netobj *owner)
>                 if (so->so_is_open_owner)
>                         continue;
>                 if (same_owner_str(so, owner))
> -                       return lockowner(nfs4_get_stateowner(so));
> +                       return nfs4_get_stateowner(so);

Hmm... If nfs4_get_stateowner() can fail here, don't you want to
continue searching the list when it does?

>         }
>         return NULL;
>  }
>  
> +static struct nfs4_lockowner *
> +find_lockowner_str_locked(struct nfs4_client *clp, struct xdr_netobj
> *owner)
> +{
> +       struct nfs4_stateowner *so;
> +
> +       so = find_stateowner_str_locked(clp, owner);
> +       if (!so)
> +               return NULL;
> +       return lockowner(so);
> +}
> +
>  static struct nfs4_lockowner *
>  find_lockowner_str(struct nfs4_client *clp, struct xdr_netobj
> *owner)
>  {
> @@ -7305,10 +7316,8 @@ nfsd4_release_lockowner(struct svc_rqst
> *rqstp,
>         struct nfsd4_release_lockowner *rlockowner = &u-
> >release_lockowner;
>         clientid_t *clid = &rlockowner->rl_clientid;
>         struct nfs4_stateowner *sop;
> -       struct nfs4_lockowner *lo = NULL;
> +       struct nfs4_lockowner *lo;
>         struct nfs4_ol_stateid *stp;
> -       struct xdr_netobj *owner = &rlockowner->rl_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;
> @@ -7322,32 +7331,18 @@ nfsd4_release_lockowner(struct svc_rqst
> *rqstp,
>                 return status;
>  
>         clp = cstate->clp;
> -       /* Find the matching lock stateowner */
>         spin_lock(&clp->cl_lock);
> -       list_for_each_entry(sop, &clp->cl_ownerstr_hashtbl[hashval],
> -                           so_strhash) {
> -
> -               if (sop->so_is_open_owner || !same_owner_str(sop,
> owner))
> -                       continue;
> -
> -               /* see if there are still any locks associated with
> it */
> -               lo = lockowner(sop);
> -               list_for_each_entry(stp, &sop->so_stateids,
> st_perstateowner) {
> -                       if (check_for_locks(stp->st_stid.sc_file,
> lo)) {
> -                               status = nfserr_locks_held;
> -                               spin_unlock(&clp->cl_lock);
> -                               return status;
> -                       }
> -               }
> +       sop = find_stateowner_str_locked(clp, &rlockowner->rl_owner);
> +       spin_unlock(&clp->cl_lock);
> +       if (!sop)
> +               return nfs_ok;
>  
> -               nfs4_get_stateowner(sop);
> -               break;
> -       }
> -       if (!lo) {
> -               spin_unlock(&clp->cl_lock);
> -               return status;
> -       }
> +       lo = lockowner(sop);
> +       list_for_each_entry(stp, &sop->so_stateids, st_perstateowner)
> +               if (check_for_locks(stp->st_stid.sc_file, lo))
> +                       return nfserr_locks_held;

Isn't this line now leaking the reference to sop?

>  
> +       spin_lock(&clp->cl_lock);
>         unhash_lockowner_locked(lo);
>         while (!list_empty(&lo->lo_owner.so_stateids)) {
>                 stp = list_first_entry(&lo->lo_owner.so_stateids,
> @@ -7360,8 +7355,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
>         free_ol_stateid_reaplist(&reaplist);
>         remove_blocked_locks(lo);
>         nfs4_put_stateowner(&lo->lo_owner);
> -
> -       return status;
> +       return nfs_ok;
>  }
>  
>  static inline struct nfs4_client_reclaim *
> 
>
Jeff Layton May 12, 2022, 10:07 a.m. UTC | #2 
On Wed, 2022-05-11 at 17:52 -0400, Chuck Lever wrote:
> nfsd4_release_lockowner() mustn't hold clp->cl_lock when
> check_for_locks() invokes nfsd_file_put(), which can sleep.
> 
> Reported-by: Dai Ngo <dai.ngo@oracle.com>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> Cc: <stable@vger.kernel.org>
> ---
>  fs/nfsd/nfs4state.c |   56 +++++++++++++++++++++++----------------------------
>  1 file changed, 25 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 234e852fcdfa..e2eb6d031643 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -6611,8 +6611,8 @@ nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny)
>  		deny->ld_type = NFS4_WRITE_LT;
>  }
>  
> -static struct nfs4_lockowner *
> -find_lockowner_str_locked(struct nfs4_client *clp, struct xdr_netobj *owner)
> +static struct nfs4_stateowner *
> +find_stateowner_str_locked(struct nfs4_client *clp, struct xdr_netobj *owner)
>  {
>  	unsigned int strhashval = ownerstr_hashval(owner);
>  	struct nfs4_stateowner *so;
> @@ -6624,11 +6624,22 @@ find_lockowner_str_locked(struct nfs4_client *clp, struct xdr_netobj *owner)
>  		if (so->so_is_open_owner)
>  			continue;
>  		if (same_owner_str(so, owner))
> -			return lockowner(nfs4_get_stateowner(so));
> +			return nfs4_get_stateowner(so);
>  	}
>  	return NULL;
>  }
>  
> +static struct nfs4_lockowner *
> +find_lockowner_str_locked(struct nfs4_client *clp, struct xdr_netobj *owner)
> +{
> +	struct nfs4_stateowner *so;
> +
> +	so = find_stateowner_str_locked(clp, owner);
> +	if (!so)
> +		return NULL;
> +	return lockowner(so);
> +}
> +
>  static struct nfs4_lockowner *
>  find_lockowner_str(struct nfs4_client *clp, struct xdr_netobj *owner)
>  {
> @@ -7305,10 +7316,8 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
>  	struct nfsd4_release_lockowner *rlockowner = &u->release_lockowner;
>  	clientid_t *clid = &rlockowner->rl_clientid;
>  	struct nfs4_stateowner *sop;
> -	struct nfs4_lockowner *lo = NULL;
> +	struct nfs4_lockowner *lo;
>  	struct nfs4_ol_stateid *stp;
> -	struct xdr_netobj *owner = &rlockowner->rl_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;
> @@ -7322,32 +7331,18 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
>  		return status;
>  
>  	clp = cstate->clp;
> -	/* Find the matching lock stateowner */
>  	spin_lock(&clp->cl_lock);
> -	list_for_each_entry(sop, &clp->cl_ownerstr_hashtbl[hashval],
> -			    so_strhash) {
> -
> -		if (sop->so_is_open_owner || !same_owner_str(sop, owner))
> -			continue;
> -
> -		/* see if there are still any locks associated with it */
> -		lo = lockowner(sop);
> -		list_for_each_entry(stp, &sop->so_stateids, st_perstateowner) {
> -			if (check_for_locks(stp->st_stid.sc_file, lo)) {
> -				status = nfserr_locks_held;
> -				spin_unlock(&clp->cl_lock);
> -				return status;
> -			}
> -		}
> +	sop = find_stateowner_str_locked(clp, &rlockowner->rl_owner);
> +	spin_unlock(&clp->cl_lock);
> +	if (!sop)
> +		return nfs_ok;
>  
> -		nfs4_get_stateowner(sop);
> -		break;
> -	}
> -	if (!lo) {
> -		spin_unlock(&clp->cl_lock);
> -		return status;
> -	}
> +	lo = lockowner(sop);
> +	list_for_each_entry(stp, &sop->so_stateids, st_perstateowner)
> +		if (check_for_locks(stp->st_stid.sc_file, lo))
> +			return nfserr_locks_held;
>  

It has been a while since I was in this code, but is it safe to walk the
above list without holding the cl_lock?

> +	spin_lock(&clp->cl_lock);
>  	unhash_lockowner_locked(lo);
>  	while (!list_empty(&lo->lo_owner.so_stateids)) {
>  		stp = list_first_entry(&lo->lo_owner.so_stateids,
> @@ -7360,8 +7355,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
>  	free_ol_stateid_reaplist(&reaplist);
>  	remove_blocked_locks(lo);
>  	nfs4_put_stateowner(&lo->lo_owner);
> -
> -	return status;
> +	return nfs_ok;
>  }
>  
>  static inline struct nfs4_client_reclaim *
> 
>
Chuck Lever May 12, 2022, 2:03 p.m. UTC | #3 
> On May 11, 2022, at 6:04 PM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> 
> On Wed, 2022-05-11 at 17:52 -0400, Chuck Lever wrote:
>> nfsd4_release_lockowner() mustn't hold clp->cl_lock when
>> check_for_locks() invokes nfsd_file_put(), which can sleep.
>> 
>> Reported-by: Dai Ngo <dai.ngo@oracle.com>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> Cc: <stable@vger.kernel.org>
>> ---
>>  fs/nfsd/nfs4state.c |   56 +++++++++++++++++++++++------------------
>> ----------
>>  1 file changed, 25 insertions(+), 31 deletions(-)
>> 
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 234e852fcdfa..e2eb6d031643 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -6611,8 +6611,8 @@ nfs4_set_lock_denied(struct file_lock *fl,
>> struct nfsd4_lock_denied *deny)
>>                 deny->ld_type = NFS4_WRITE_LT;
>>  }
>>  
>> -static struct nfs4_lockowner *
>> -find_lockowner_str_locked(struct nfs4_client *clp, struct xdr_netobj
>> *owner)
>> +static struct nfs4_stateowner *
>> +find_stateowner_str_locked(struct nfs4_client *clp, struct
>> xdr_netobj *owner)
>>  {
>>         unsigned int strhashval = ownerstr_hashval(owner);
>>         struct nfs4_stateowner *so;
>> @@ -6624,11 +6624,22 @@ find_lockowner_str_locked(struct nfs4_client
>> *clp, struct xdr_netobj *owner)
>>                 if (so->so_is_open_owner)
>>                         continue;
>>                 if (same_owner_str(so, owner))
>> -                       return lockowner(nfs4_get_stateowner(so));
>> +                       return nfs4_get_stateowner(so);
> 
> Hmm... If nfs4_get_stateowner() can fail here, don't you want to
> continue searching the list when it does?

You've lost me on this.

 494 static inline struct nfs4_stateowner *
 495 nfs4_get_stateowner(struct nfs4_stateowner *sop)
 496 {
 497         atomic_inc(&sop->so_count);
 498         return sop;
 499 }
 500 

How would nfs4_get_stateowner() fail?


>>         }
>>         return NULL;
>>  }
>>  
>> +static struct nfs4_lockowner *
>> +find_lockowner_str_locked(struct nfs4_client *clp, struct xdr_netobj
>> *owner)
>> +{
>> +       struct nfs4_stateowner *so;
>> +
>> +       so = find_stateowner_str_locked(clp, owner);
>> +       if (!so)
>> +               return NULL;
>> +       return lockowner(so);
>> +}
>> +
>>  static struct nfs4_lockowner *
>>  find_lockowner_str(struct nfs4_client *clp, struct xdr_netobj
>> *owner)
>>  {
>> @@ -7305,10 +7316,8 @@ nfsd4_release_lockowner(struct svc_rqst
>> *rqstp,
>>         struct nfsd4_release_lockowner *rlockowner = &u-
>>> release_lockowner;
>>         clientid_t *clid = &rlockowner->rl_clientid;
>>         struct nfs4_stateowner *sop;
>> -       struct nfs4_lockowner *lo = NULL;
>> +       struct nfs4_lockowner *lo;
>>         struct nfs4_ol_stateid *stp;
>> -       struct xdr_netobj *owner = &rlockowner->rl_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;
>> @@ -7322,32 +7331,18 @@ nfsd4_release_lockowner(struct svc_rqst
>> *rqstp,
>>                 return status;
>>  
>>         clp = cstate->clp;
>> -       /* Find the matching lock stateowner */
>>         spin_lock(&clp->cl_lock);
>> -       list_for_each_entry(sop, &clp->cl_ownerstr_hashtbl[hashval],
>> -                           so_strhash) {
>> -
>> -               if (sop->so_is_open_owner || !same_owner_str(sop,
>> owner))
>> -                       continue;
>> -
>> -               /* see if there are still any locks associated with
>> it */
>> -               lo = lockowner(sop);
>> -               list_for_each_entry(stp, &sop->so_stateids,
>> st_perstateowner) {
>> -                       if (check_for_locks(stp->st_stid.sc_file,
>> lo)) {
>> -                               status = nfserr_locks_held;
>> -                               spin_unlock(&clp->cl_lock);
>> -                               return status;
>> -                       }
>> -               }
>> +       sop = find_stateowner_str_locked(clp, &rlockowner->rl_owner);
>> +       spin_unlock(&clp->cl_lock);
>> +       if (!sop)
>> +               return nfs_ok;
>>  
>> -               nfs4_get_stateowner(sop);
>> -               break;
>> -       }
>> -       if (!lo) {
>> -               spin_unlock(&clp->cl_lock);
>> -               return status;
>> -       }
>> +       lo = lockowner(sop);
>> +       list_for_each_entry(stp, &sop->so_stateids, st_perstateowner)
>> +               if (check_for_locks(stp->st_stid.sc_file, lo))
>> +                       return nfserr_locks_held;
> 
> Isn't this line now leaking the reference to sop?

Indeed. Fixed.


>>  
>> +       spin_lock(&clp->cl_lock);
>>         unhash_lockowner_locked(lo);
>>         while (!list_empty(&lo->lo_owner.so_stateids)) {
>>                 stp = list_first_entry(&lo->lo_owner.so_stateids,
>> @@ -7360,8 +7355,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
>>         free_ol_stateid_reaplist(&reaplist);
>>         remove_blocked_locks(lo);
>>         nfs4_put_stateowner(&lo->lo_owner);
>> -
>> -       return status;
>> +       return nfs_ok;
>>  }
>>  
>>  static inline struct nfs4_client_reclaim *
>> 
>> 
> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
> 
> 

--
Chuck Lever
Chuck Lever May 12, 2022, 2:10 p.m. UTC | #4 
> On May 12, 2022, at 6:07 AM, Jeff Layton <jlayton@redhat.com> wrote:
> 
> On Wed, 2022-05-11 at 17:52 -0400, Chuck Lever wrote:
>> nfsd4_release_lockowner() mustn't hold clp->cl_lock when
>> check_for_locks() invokes nfsd_file_put(), which can sleep.
>> 
>> Reported-by: Dai Ngo <dai.ngo@oracle.com>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> Cc: <stable@vger.kernel.org>
>> ---
>> fs/nfsd/nfs4state.c |   56 +++++++++++++++++++++++----------------------------
>> 1 file changed, 25 insertions(+), 31 deletions(-)
>> 
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 234e852fcdfa..e2eb6d031643 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -6611,8 +6611,8 @@ nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny)
>> 		deny->ld_type = NFS4_WRITE_LT;
>> }
>> 
>> -static struct nfs4_lockowner *
>> -find_lockowner_str_locked(struct nfs4_client *clp, struct xdr_netobj *owner)
>> +static struct nfs4_stateowner *
>> +find_stateowner_str_locked(struct nfs4_client *clp, struct xdr_netobj *owner)
>> {
>> 	unsigned int strhashval = ownerstr_hashval(owner);
>> 	struct nfs4_stateowner *so;
>> @@ -6624,11 +6624,22 @@ find_lockowner_str_locked(struct nfs4_client *clp, struct xdr_netobj *owner)
>> 		if (so->so_is_open_owner)
>> 			continue;
>> 		if (same_owner_str(so, owner))
>> -			return lockowner(nfs4_get_stateowner(so));
>> +			return nfs4_get_stateowner(so);
>> 	}
>> 	return NULL;
>> }
>> 
>> +static struct nfs4_lockowner *
>> +find_lockowner_str_locked(struct nfs4_client *clp, struct xdr_netobj *owner)
>> +{
>> +	struct nfs4_stateowner *so;
>> +
>> +	so = find_stateowner_str_locked(clp, owner);
>> +	if (!so)
>> +		return NULL;
>> +	return lockowner(so);
>> +}
>> +
>> static struct nfs4_lockowner *
>> find_lockowner_str(struct nfs4_client *clp, struct xdr_netobj *owner)
>> {
>> @@ -7305,10 +7316,8 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
>> 	struct nfsd4_release_lockowner *rlockowner = &u->release_lockowner;
>> 	clientid_t *clid = &rlockowner->rl_clientid;
>> 	struct nfs4_stateowner *sop;
>> -	struct nfs4_lockowner *lo = NULL;
>> +	struct nfs4_lockowner *lo;
>> 	struct nfs4_ol_stateid *stp;
>> -	struct xdr_netobj *owner = &rlockowner->rl_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;
>> @@ -7322,32 +7331,18 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
>> 		return status;
>> 
>> 	clp = cstate->clp;
>> -	/* Find the matching lock stateowner */
>> 	spin_lock(&clp->cl_lock);
>> -	list_for_each_entry(sop, &clp->cl_ownerstr_hashtbl[hashval],
>> -			    so_strhash) {
>> -
>> -		if (sop->so_is_open_owner || !same_owner_str(sop, owner))
>> -			continue;
>> -
>> -		/* see if there are still any locks associated with it */
>> -		lo = lockowner(sop);
>> -		list_for_each_entry(stp, &sop->so_stateids, st_perstateowner) {
>> -			if (check_for_locks(stp->st_stid.sc_file, lo)) {
>> -				status = nfserr_locks_held;
>> -				spin_unlock(&clp->cl_lock);
>> -				return status;
>> -			}
>> -		}
>> +	sop = find_stateowner_str_locked(clp, &rlockowner->rl_owner);
>> +	spin_unlock(&clp->cl_lock);
>> +	if (!sop)
>> +		return nfs_ok;
>> 
>> -		nfs4_get_stateowner(sop);
>> -		break;
>> -	}
>> -	if (!lo) {
>> -		spin_unlock(&clp->cl_lock);
>> -		return status;
>> -	}
>> +	lo = lockowner(sop);
>> +	list_for_each_entry(stp, &sop->so_stateids, st_perstateowner)
>> +		if (check_for_locks(stp->st_stid.sc_file, lo))
>> +			return nfserr_locks_held;
>> 
> 
> It has been a while since I was in this code, but is it safe to walk the
> above list without holding the cl_lock?

Shoot.

I thought holding a reference on the stateowner would be enough,
but looks like everyone else holds cl_lock. More chin scratching
and caffeine needed.


>> +	spin_lock(&clp->cl_lock);
>> 	unhash_lockowner_locked(lo);
>> 	while (!list_empty(&lo->lo_owner.so_stateids)) {
>> 		stp = list_first_entry(&lo->lo_owner.so_stateids,
>> @@ -7360,8 +7355,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
>> 	free_ol_stateid_reaplist(&reaplist);
>> 	remove_blocked_locks(lo);
>> 	nfs4_put_stateowner(&lo->lo_owner);
>> -
>> -	return status;
>> +	return nfs_ok;
>> }
>> 
>> static inline struct nfs4_client_reclaim *
>> 
>> 
> 
> -- 
> Jeff Layton <jlayton@redhat.com>
> 

--
Chuck Lever
diff mbox series

Patch

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 234e852fcdfa..e2eb6d031643 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -6611,8 +6611,8 @@  nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny)
 		deny->ld_type = NFS4_WRITE_LT;
 }
 
-static struct nfs4_lockowner *
-find_lockowner_str_locked(struct nfs4_client *clp, struct xdr_netobj *owner)
+static struct nfs4_stateowner *
+find_stateowner_str_locked(struct nfs4_client *clp, struct xdr_netobj *owner)
 {
 	unsigned int strhashval = ownerstr_hashval(owner);
 	struct nfs4_stateowner *so;
@@ -6624,11 +6624,22 @@  find_lockowner_str_locked(struct nfs4_client *clp, struct xdr_netobj *owner)
 		if (so->so_is_open_owner)
 			continue;
 		if (same_owner_str(so, owner))
-			return lockowner(nfs4_get_stateowner(so));
+			return nfs4_get_stateowner(so);
 	}
 	return NULL;
 }
 
+static struct nfs4_lockowner *
+find_lockowner_str_locked(struct nfs4_client *clp, struct xdr_netobj *owner)
+{
+	struct nfs4_stateowner *so;
+
+	so = find_stateowner_str_locked(clp, owner);
+	if (!so)
+		return NULL;
+	return lockowner(so);
+}
+
 static struct nfs4_lockowner *
 find_lockowner_str(struct nfs4_client *clp, struct xdr_netobj *owner)
 {
@@ -7305,10 +7316,8 @@  nfsd4_release_lockowner(struct svc_rqst *rqstp,
 	struct nfsd4_release_lockowner *rlockowner = &u->release_lockowner;
 	clientid_t *clid = &rlockowner->rl_clientid;
 	struct nfs4_stateowner *sop;
-	struct nfs4_lockowner *lo = NULL;
+	struct nfs4_lockowner *lo;
 	struct nfs4_ol_stateid *stp;
-	struct xdr_netobj *owner = &rlockowner->rl_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;
@@ -7322,32 +7331,18 @@  nfsd4_release_lockowner(struct svc_rqst *rqstp,
 		return status;
 
 	clp = cstate->clp;
-	/* Find the matching lock stateowner */
 	spin_lock(&clp->cl_lock);
-	list_for_each_entry(sop, &clp->cl_ownerstr_hashtbl[hashval],
-			    so_strhash) {
-
-		if (sop->so_is_open_owner || !same_owner_str(sop, owner))
-			continue;
-
-		/* see if there are still any locks associated with it */
-		lo = lockowner(sop);
-		list_for_each_entry(stp, &sop->so_stateids, st_perstateowner) {
-			if (check_for_locks(stp->st_stid.sc_file, lo)) {
-				status = nfserr_locks_held;
-				spin_unlock(&clp->cl_lock);
-				return status;
-			}
-		}
+	sop = find_stateowner_str_locked(clp, &rlockowner->rl_owner);
+	spin_unlock(&clp->cl_lock);
+	if (!sop)
+		return nfs_ok;
 
-		nfs4_get_stateowner(sop);
-		break;
-	}
-	if (!lo) {
-		spin_unlock(&clp->cl_lock);
-		return status;
-	}
+	lo = lockowner(sop);
+	list_for_each_entry(stp, &sop->so_stateids, st_perstateowner)
+		if (check_for_locks(stp->st_stid.sc_file, lo))
+			return nfserr_locks_held;
 
+	spin_lock(&clp->cl_lock);
 	unhash_lockowner_locked(lo);
 	while (!list_empty(&lo->lo_owner.so_stateids)) {
 		stp = list_first_entry(&lo->lo_owner.so_stateids,
@@ -7360,8 +7355,7 @@  nfsd4_release_lockowner(struct svc_rqst *rqstp,
 	free_ol_stateid_reaplist(&reaplist);
 	remove_blocked_locks(lo);
 	nfs4_put_stateowner(&lo->lo_owner);
-
-	return status;
+	return nfs_ok;
 }
 
 static inline struct nfs4_client_reclaim *