Re: [PATCH] nfsd: don't take fi_lock in nfsd_break_deleg_cb()

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2024-02-05 at 13:22 +1100, NeilBrown wrote:
> A recent change to check_for_locks() changed it to take ->flc_lock while
> holding ->fi_lock.  This creates a lock inversion (reported by lockdep)
> because there is a case where ->fi_lock is taken while holding
> ->flc_lock.
> 
> ->flc_lock is held across ->fl_lmops callbacks, and
> nfsd_break_deleg_cb() is one of those and does take ->fi_lock.  However
> it doesn't need to.
> 
> Prior to v4.17-rc1~110^2~22 ("nfsd: create a separate lease for each
> delegation") nfsd_break_deleg_cb() would walk the ->fi_delegations list
> and so needed the lock.  Since then it doesn't walk the list and doesn't
> need the lock.
> 
> Two actions are performed under the lock.  One is to call
> nfsd_break_one_deleg which calls nfsd4_run_cb().  These doesn't act on
> the nfs4_file at all, so don't need the lock.
> 
> The other is to set ->fi_had_conflict which is in the nfs4_file.
> This field is only ever set here (except when initialised to false)
> so there is no possible problem will multiple threads racing when
> setting it.
> 
> The field is tested twice in nfs4_set_delegation().  The first test does
> not hold a lock and is documented as an opportunistic optimisation, so
> it doesn't impose any need to hold ->fi_lock while setting
> ->fi_had_conflict.
> 
> The second test in nfs4_set_delegation() *is* make under ->fi_lock, so
> removing the locking when ->fi_had_conflict is set could make a change.
> The change could only be interesting if ->fi_had_conflict tested as
> false even though nfsd_break_one_deleg() ran before ->fi_lock was
> unlocked.  i.e. while hash_delegation_locked() was running.
> As hash_delegation_lock() doesn't interact in any way with nfs4_run_cb()
> there can be no importance to this interaction.
> 
> So this patch removes the locking from nfsd_break_one_deleg() and moves
> the final test on ->fi_had_conflict out of the locked region to make it
> clear that locking isn't important to the test.  It is still tested
> *after* vfs_setlease() has succeeded.  This might be significant and as
> vfs_setlease() takes ->flc_lock, and nfsd_break_one_deleg() is called
> under ->flc_lock this "after" is a true ordering provided by a spinlock.
> 
> Fixes: edcf9725150e ("nfsd: fix RELEASE_LOCKOWNER")
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/nfs4state.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 12534e12dbb3..8b112673d389 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5154,10 +5154,8 @@ nfsd_break_deleg_cb(struct file_lock *fl)
>  	 */
>  	fl->fl_break_time = 0;
>  
> -	spin_lock(&fp->fi_lock);
>  	fp->fi_had_conflict = true;
>  	nfsd_break_one_deleg(dp);
> -	spin_unlock(&fp->fi_lock);
>  	return false;
>  }
>  
> @@ -5771,13 +5769,14 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>  	if (status)
>  		goto out_unlock;
>  
> +	status = -EAGAIN;
> +	if (fp->fi_had_conflict)
> +		goto out_unlock;
> +
>  	spin_lock(&state_lock);
>  	spin_lock(&clp->cl_lock);
>  	spin_lock(&fp->fi_lock);
> -	if (fp->fi_had_conflict)
> -		status = -EAGAIN;
> -	else
> -		status = hash_delegation_locked(dp, fp);
> +	status = hash_delegation_locked(dp, fp);
>  	spin_unlock(&fp->fi_lock);
>  	spin_unlock(&clp->cl_lock);
>  	spin_unlock(&state_lock);

Love the detailed explanation:

Reviewed-by: Jeff Layton <jlayton@kernel.org>