Re: [PATCH 2/3] xfs: unwind xfs_extent_busy_clear

["Darrick J. Wong" <djwong@kernel.org>]

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
> 
>