| Message ID | 20240402213541.1199959-3-david@fromorbit.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xfs: sparse warning fixes | expand |
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 > >
So while this works it looks pretty ugly. I'd rewrite the nested loop
as what it is: a nested loop. Something like the patch below. Only
compile tested so far, but I'll kick off a real test later.
diff --git a/fs/xfs/xfs_extent_busy.c b/fs/xfs/xfs_extent_busy.c
index 2ccde32c9a9e97..1334360128662a 100644
--- a/fs/xfs/xfs_extent_busy.c
+++ b/fs/xfs/xfs_extent_busy.c
@@ -533,21 +533,6 @@ xfs_extent_busy_clear_one(
kmem_free(busyp);
}
-static void
-xfs_extent_busy_put_pag(
- struct xfs_perag *pag,
- bool wakeup)
- __releases(pag->pagb_lock)
-{
- if (wakeup) {
- pag->pagb_gen++;
- wake_up_all(&pag->pagb_wait);
- }
-
- spin_unlock(&pag->pagb_lock);
- xfs_perag_put(pag);
-}
-
/*
* Remove all extents on the passed in list from the busy extents tree.
* If do_discard is set skip extents that need to be discarded, and mark
@@ -559,32 +544,43 @@ xfs_extent_busy_clear(
struct list_head *list,
bool do_discard)
{
- struct xfs_extent_busy *busyp, *n;
- struct xfs_perag *pag = NULL;
- xfs_agnumber_t agno = NULLAGNUMBER;
- bool wakeup = false;
-
- list_for_each_entry_safe(busyp, n, list, list) {
- if (busyp->agno != agno) {
- if (pag)
- xfs_extent_busy_put_pag(pag, wakeup);
- agno = busyp->agno;
- pag = xfs_perag_get(mp, agno);
- spin_lock(&pag->pagb_lock);
- wakeup = false;
- }
+ struct xfs_extent_busy *busyp;
- if (do_discard && busyp->length &&
- !(busyp->flags & XFS_EXTENT_BUSY_SKIP_DISCARD)) {
- busyp->flags = XFS_EXTENT_BUSY_DISCARDED;
- } else {
- xfs_extent_busy_clear_one(mp, pag, busyp);
- wakeup = true;
- }
- }
+ busyp = list_first_entry_or_null(list, typeof(*busyp), list);
+ if (!busyp)
+ return;
+
+ do {
+ struct xfs_perag *pag = xfs_perag_get(mp, busyp->agno);
+ bool wakeup = false;
+ struct xfs_extent_busy *next;
- if (pag)
- xfs_extent_busy_put_pag(pag, wakeup);
+ spin_lock(&pag->pagb_lock);
+ for (;;) {
+ next = list_next_entry(busyp, list);
+
+ if (do_discard && busyp->length &&
+ !(busyp->flags & XFS_EXTENT_BUSY_SKIP_DISCARD)) {
+ busyp->flags = XFS_EXTENT_BUSY_DISCARDED;
+ } else {
+ xfs_extent_busy_clear_one(mp, pag, busyp);
+ wakeup = true;
+ }
+
+ if (list_entry_is_head(next, list, list) ||
+ next->agno != pag->pag_agno)
+ break;
+ busyp = next;
+ }
+ if (wakeup) {
+ pag->pagb_gen++;
+ wake_up_all(&pag->pagb_wait);
+ }
+ spin_unlock(&pag->pagb_lock);
+ xfs_perag_put(pag);
+
+ busyp = next;
+ } while (!list_entry_is_head(busyp, list, list));
}
/*
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); } /*