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

[Chuck Lever III <chuck.lever@oracle.com>]

> 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