| Message ID | 20240405060710.227096-3-hch@lst.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] xfs: move more logic into xfs_extent_busy_clear_one | expand |
On Fri, Apr 05, 2024 at 08:07:09AM +0200, Christoph Hellwig wrote: > The current structure of xfs_extent_busy_clear that locks the first busy > extent in each AG and unlocks when switching to a new AG makes sparse > unhappy as the lock critical section tracking can't cope with taking the > lock conditionally and inside a loop. > > Rewrite xfs_extent_busy_clear so that it has an outer loop only advancing > when moving to a new AG, and an inner loop that consumes busy extents for > the given AG to make life easier for sparse and to also make this logic > more obvious to humans. > > Signed-off-by: Christoph Hellwig <hch@lst.de> That is a lot easier to understand! Thank you for the cleanup. Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > fs/xfs/xfs_extent_busy.c | 59 +++++++++++++++++----------------------- > 1 file changed, 25 insertions(+), 34 deletions(-) > > diff --git a/fs/xfs/xfs_extent_busy.c b/fs/xfs/xfs_extent_busy.c > index 6fbffa46e5e94b..a73e7c73b664c6 100644 > --- a/fs/xfs/xfs_extent_busy.c > +++ b/fs/xfs/xfs_extent_busy.c > @@ -540,21 +540,6 @@ xfs_extent_busy_clear_one( > return true; > } > > -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 > @@ -566,27 +551,33 @@ 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, *next; > > - if (xfs_extent_busy_clear_one(pag, busyp, do_discard)) > - wakeup = true; > - } > + busyp = list_first_entry_or_null(list, typeof(*busyp), list); > + if (!busyp) > + return; > + > + do { > + bool wakeup = false; > + struct xfs_perag *pag; > > - if (pag) > - xfs_extent_busy_put_pag(pag, wakeup); > + pag = xfs_perag_get(mp, busyp->agno); > + spin_lock(&pag->pagb_lock); > + do { > + next = list_next_entry(busyp, list); > + if (xfs_extent_busy_clear_one(pag, busyp, do_discard)) > + wakeup = true; > + busyp = next; > + } while (!list_entry_is_head(busyp, list, list) && > + busyp->agno == pag->pag_agno); > + > + if (wakeup) { > + pag->pagb_gen++; > + wake_up_all(&pag->pagb_wait); > + } > + spin_unlock(&pag->pagb_lock); > + xfs_perag_put(pag); > + } while (!list_entry_is_head(busyp, list, list)); > } > > /* > -- > 2.39.2 > >
diff --git a/fs/xfs/xfs_extent_busy.c b/fs/xfs/xfs_extent_busy.c index 6fbffa46e5e94b..a73e7c73b664c6 100644 --- a/fs/xfs/xfs_extent_busy.c +++ b/fs/xfs/xfs_extent_busy.c @@ -540,21 +540,6 @@ xfs_extent_busy_clear_one( return true; } -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 @@ -566,27 +551,33 @@ 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, *next; - if (xfs_extent_busy_clear_one(pag, busyp, do_discard)) - wakeup = true; - } + busyp = list_first_entry_or_null(list, typeof(*busyp), list); + if (!busyp) + return; + + do { + bool wakeup = false; + struct xfs_perag *pag; - if (pag) - xfs_extent_busy_put_pag(pag, wakeup); + pag = xfs_perag_get(mp, busyp->agno); + spin_lock(&pag->pagb_lock); + do { + next = list_next_entry(busyp, list); + if (xfs_extent_busy_clear_one(pag, busyp, do_discard)) + wakeup = true; + busyp = next; + } while (!list_entry_is_head(busyp, list, list) && + busyp->agno == pag->pag_agno); + + if (wakeup) { + pag->pagb_gen++; + wake_up_all(&pag->pagb_wait); + } + spin_unlock(&pag->pagb_lock); + xfs_perag_put(pag); + } while (!list_entry_is_head(busyp, list, list)); } /*
The current structure of xfs_extent_busy_clear that locks the first busy extent in each AG and unlocks when switching to a new AG makes sparse unhappy as the lock critical section tracking can't cope with taking the lock conditionally and inside a loop. Rewrite xfs_extent_busy_clear so that it has an outer loop only advancing when moving to a new AG, and an inner loop that consumes busy extents for the given AG to make life easier for sparse and to also make this logic more obvious to humans. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_extent_busy.c | 59 +++++++++++++++++----------------------- 1 file changed, 25 insertions(+), 34 deletions(-)