Re: [PATCH 2/5] xfs: fix sparse warning in xfs_extent_busy_clear

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Apr 03, 2024 at 08:28:29AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Sparse reports:
> 
> fs/xfs/xfs_extent_busy.c:588:17: warning: context imbalance in 'xfs_extent_busy_clear' - unexpected unlock
> 
> But there is no locking bug here. Sparse simply doesn't understand
> the logic and locking in the busy extent processing loop.
> xfs_extent_busy_put_pag() has an annotation to suppresses an
> unexpected unlock warning, but that isn't sufficient.
> 
> If we move the pag existence check into xfs_extent_busy_put_pag() and
> annotate that with a __release() so that this function always
> appears to release the pag->pagb_lock, sparse now thinks the loop
> locking is balanced (one unlock, one lock per loop) but still throws
> an unexpected unlock warning after loop cleanup.
> 
> i.e. it does not understand that we enter the loop without any locks
> held and exit it with the last lock still held. Whilst the locking
> within the loop is inow balanced, we need to add an __acquire() to
> xfs_extent_busy_clear() to set the initial lock context needed to
> avoid false warnings.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_extent_busy.c | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_extent_busy.c b/fs/xfs/xfs_extent_busy.c
> index 56cfa1498571..686b67372030 100644
> --- a/fs/xfs/xfs_extent_busy.c
> +++ b/fs/xfs/xfs_extent_busy.c
> @@ -534,12 +534,24 @@ xfs_extent_busy_clear_one(
>  	kfree(busyp);
>  }
>  
> +/*
> + * Sparse has real trouble with the structure of xfs_extent_busy_clear() and it
> + * is impossible to annotate it correctly if we leave the 'if (pag)' conditional
> + * in xfs_extent_busy_clear(). Hence we always "release" the lock in
> + * xfs_extent_busy_put_pag() so sparse only ever sees one possible path to
> + * drop the lock.
> + */
>  static void
>  xfs_extent_busy_put_pag(
>  	struct xfs_perag	*pag,
>  	bool			wakeup)
>  		__releases(pag->pagb_lock)
>  {
> +	if (!pag) {
> +		__release(pag->pagb_lock);
> +		return;
> +	}

Passing in a null pointer so we can fake out a compliance tool with a
nonsense annotation really feels like the height of software bureaucracy
compliance culture now...

I don't want to RVB this but I'm so tired of fighting pointless battles
with people over their clearly inadequate tooling, so GIGO:

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> +
>  	if (wakeup) {
>  		pag->pagb_gen++;
>  		wake_up_all(&pag->pagb_wait);
> @@ -565,10 +577,18 @@ xfs_extent_busy_clear(
>  	xfs_agnumber_t		agno = NULLAGNUMBER;
>  	bool			wakeup = false;
>  
> +	/*
> +	 * Sparse thinks the locking in the loop below is balanced (one unlock,
> +	 * one lock per loop iteration) and doesn't understand that we enter
> +	 * with no lock held and exit with a lock held. Hence we need to
> +	 * "acquire" the lock to create the correct initial condition for the
> +	 * cleanup after loop termination to avoid an unexpected unlock warning.
> +	 */
> +	__acquire(pag->pagb_lock);
> +
>  	list_for_each_entry_safe(busyp, n, list, list) {
>  		if (busyp->agno != agno) {
> -			if (pag)
> -				xfs_extent_busy_put_pag(pag, wakeup);
> +			xfs_extent_busy_put_pag(pag, wakeup);
>  			agno = busyp->agno;
>  			pag = xfs_perag_get(mp, agno);
>  			spin_lock(&pag->pagb_lock);
> @@ -584,8 +604,7 @@ xfs_extent_busy_clear(
>  		}
>  	}
>  
> -	if (pag)
> -		xfs_extent_busy_put_pag(pag, wakeup);
> +	xfs_extent_busy_put_pag(pag, wakeup);
>  }
>  
>  /*
> -- 
> 2.43.0
> 
>