On Wed, Apr 03, 2024 at 08:28:29AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
--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
>
>
