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;
+ }
+
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
