spring cleaning for xfs_extent_busy_clear

spring cleaning for xfs_extent_busy_clear

[Christoph Hellwig]

Hi all,

this is a little cleanup for xfs_extent_busy_clear, primarily motivated
by getting rid of the sparse log annoation warning. 

Diffstat:
 xfs_extent_busy.c |   80 ++++++++++++++++++++++++------------------------------
 xfs_trace.h       |    1 
 2 files changed, 36 insertions(+), 45 deletions(-)

[PATCH 1/3] xfs: move more logic into xfs_extent_busy_clear_one

[Christoph Hellwig]

Move the handling of discarded entries into xfs_extent_busy_clear_one
to reuse the length check and tidy up the logic in the caller.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_extent_busy.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/xfs_extent_busy.c b/fs/xfs/xfs_extent_busy.c
index 56cfa1498571e3..6fbffa46e5e94b 100644
--- a/fs/xfs/xfs_extent_busy.c
+++ b/fs/xfs/xfs_extent_busy.c
@@ -518,20 +518,26 @@ xfs_extent_busy_trim(
 	goto out;
 }
 
-STATIC void
+static bool
 xfs_extent_busy_clear_one(
-	struct xfs_mount	*mp,
 	struct xfs_perag	*pag,
-	struct xfs_extent_busy	*busyp)
+	struct xfs_extent_busy	*busyp,
+	bool			do_discard)
 {
 	if (busyp->length) {
-		trace_xfs_extent_busy_clear(mp, busyp->agno, busyp->bno,
-						busyp->length);
+		if (do_discard &&
+		    !(busyp->flags & XFS_EXTENT_BUSY_SKIP_DISCARD)) {
+			busyp->flags = XFS_EXTENT_BUSY_DISCARDED;
+			return false;
+		}
+		trace_xfs_extent_busy_clear(pag->pag_mount, busyp->agno,
+				busyp->bno, busyp->length);
 		rb_erase(&busyp->rb_node, &pag->pagb_tree);
 	}
 
 	list_del_init(&busyp->list);
 	kfree(busyp);
+	return true;
 }
 
 static void
@@ -575,13 +581,8 @@ xfs_extent_busy_clear(
 			wakeup = false;
 		}
 
-		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);
+		if (xfs_extent_busy_clear_one(pag, busyp, do_discard))
 			wakeup = true;
-		}
 	}
 
 	if (pag)
-- 
2.39.2

[PATCH 2/3] xfs: unwind xfs_extent_busy_clear

[Christoph Hellwig]

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

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

[PATCH 3/3] xfs: remove the unused xfs_extent_busy_enomem trace event

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_trace.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index aea97fc074f8de..62ef0888398b09 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -1654,7 +1654,6 @@ DEFINE_EVENT(xfs_extent_busy_class, name, \
 		 xfs_agblock_t agbno, xfs_extlen_t len), \
 	TP_ARGS(mp, agno, agbno, len))
 DEFINE_BUSY_EVENT(xfs_extent_busy);
-DEFINE_BUSY_EVENT(xfs_extent_busy_enomem);
 DEFINE_BUSY_EVENT(xfs_extent_busy_force);
 DEFINE_BUSY_EVENT(xfs_extent_busy_reuse);
 DEFINE_BUSY_EVENT(xfs_extent_busy_clear);
-- 
2.39.2

Re: [PATCH 3/3] xfs: remove the unused xfs_extent_busy_enomem trace event

[Darrick J. Wong]

On Fri, Apr 05, 2024 at 08:07:10AM +0200, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>

If only we got compiler warnings about unused tracepoints -- I've been
wondering how many more of these are lurking.

Nonetheless, this looks good so
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_trace.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index aea97fc074f8de..62ef0888398b09 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -1654,7 +1654,6 @@ DEFINE_EVENT(xfs_extent_busy_class, name, \
>  		 xfs_agblock_t agbno, xfs_extlen_t len), \
>  	TP_ARGS(mp, agno, agbno, len))
>  DEFINE_BUSY_EVENT(xfs_extent_busy);
> -DEFINE_BUSY_EVENT(xfs_extent_busy_enomem);
>  DEFINE_BUSY_EVENT(xfs_extent_busy_force);
>  DEFINE_BUSY_EVENT(xfs_extent_busy_reuse);
>  DEFINE_BUSY_EVENT(xfs_extent_busy_clear);
> -- 
> 2.39.2
> 
> 

Re: [PATCH 1/3] xfs: move more logic into xfs_extent_busy_clear_one

[Darrick J. Wong]

On Fri, Apr 05, 2024 at 08:07:08AM +0200, Christoph Hellwig wrote:
> Move the handling of discarded entries into xfs_extent_busy_clear_one
> to reuse the length check and tidy up the logic in the caller.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

AFAICT, the return value of xfs_extent_busy_clear_one is whether
or not it actually changed the pagb_tree, right?  And if that return
value is true, then we want to wake up anyone who might be waiting on
busy extents to clear the pagb_tree, which is what the @wakeup logic
does, right?

If the answers are yes and yes, then:
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D


> ---
>  fs/xfs/xfs_extent_busy.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/xfs/xfs_extent_busy.c b/fs/xfs/xfs_extent_busy.c
> index 56cfa1498571e3..6fbffa46e5e94b 100644
> --- a/fs/xfs/xfs_extent_busy.c
> +++ b/fs/xfs/xfs_extent_busy.c
> @@ -518,20 +518,26 @@ xfs_extent_busy_trim(
>  	goto out;
>  }
>  
> -STATIC void
> +static bool
>  xfs_extent_busy_clear_one(
> -	struct xfs_mount	*mp,
>  	struct xfs_perag	*pag,
> -	struct xfs_extent_busy	*busyp)
> +	struct xfs_extent_busy	*busyp,
> +	bool			do_discard)
>  {
>  	if (busyp->length) {
> -		trace_xfs_extent_busy_clear(mp, busyp->agno, busyp->bno,
> -						busyp->length);
> +		if (do_discard &&
> +		    !(busyp->flags & XFS_EXTENT_BUSY_SKIP_DISCARD)) {
> +			busyp->flags = XFS_EXTENT_BUSY_DISCARDED;
> +			return false;
> +		}
> +		trace_xfs_extent_busy_clear(pag->pag_mount, busyp->agno,
> +				busyp->bno, busyp->length);
>  		rb_erase(&busyp->rb_node, &pag->pagb_tree);
>  	}
>  
>  	list_del_init(&busyp->list);
>  	kfree(busyp);
> +	return true;
>  }
>  
>  static void
> @@ -575,13 +581,8 @@ xfs_extent_busy_clear(
>  			wakeup = false;
>  		}
>  
> -		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);
> +		if (xfs_extent_busy_clear_one(pag, busyp, do_discard))
>  			wakeup = true;
> -		}
>  	}
>  
>  	if (pag)
> -- 
> 2.39.2
> 
> 

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

[Darrick J. Wong]

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

Re: [PATCH 1/3] xfs: move more logic into xfs_extent_busy_clear_one

[Christoph Hellwig]

On Fri, Apr 05, 2024 at 08:44:29AM -0700, Darrick J. Wong wrote:
> On Fri, Apr 05, 2024 at 08:07:08AM +0200, Christoph Hellwig wrote:
> > Move the handling of discarded entries into xfs_extent_busy_clear_one
> > to reuse the length check and tidy up the logic in the caller.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> AFAICT, the return value of xfs_extent_busy_clear_one is whether
> or not it actually changed the pagb_tree, right?  And if that return
> value is true, then we want to wake up anyone who might be waiting on
> busy extents to clear the pagb_tree, which is what the @wakeup logic
> does, right?

Yes and yes.