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@xxxxxx>
That is a lot easier to understand! Thank you for the cleanup.
Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
--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
>
>
