On Tue, May 11, 2021 at 05:35:19PM +1000, Dave Chinner wrote:

On Mon, May 10, 2021 at 08:53:56AM -0400, Brian Foster wrote:

quoted

On Thu, May 06, 2021 at 05:20:36PM +1000, Dave Chinner wrote:

quoted

From: Dave Chinner <redacted>

for_each_perag_tag() is defined in xfs_icache.c for local use.
Promote this to xfs_ag.h and define equivalent iteration functions
so that we can use them to iterate AGs instead to replace open coded
perag walks and perag lookups.

We also convert as many of the straight forward open coded AG walks
to use these iterators as possible. Anything that is not a direct
conversion to an iterator is ignored and will be updated in future
commits.

Signed-off-by: Dave Chinner <redacted>
---
 fs/xfs/libxfs/xfs_ag.h    | 17 +++++++++++++++++
 fs/xfs/scrub/fscounters.c | 36 ++++++++++++++----------------------
 fs/xfs/xfs_extent_busy.c  |  7 ++-----
 fs/xfs/xfs_fsops.c        |  8 ++------
 fs/xfs/xfs_health.c       |  4 +---
 fs/xfs/xfs_icache.c       | 15 ++-------------
 6 files changed, 38 insertions(+), 49 deletions(-)

...

quoted

diff --git a/fs/xfs/scrub/fscounters.c b/fs/xfs/scrub/fscounters.c
index 453ae9adf94c..2dfdac566399 100644
--- a/fs/xfs/scrub/fscounters.c
+++ b/fs/xfs/scrub/fscounters.c

...

quoted

@@ -229,12 +224,9 @@ xchk_fscount_aggregate_agcounts(
 		fsc->fdblocks -= pag->pag_meta_resv.ar_reserved;
 		fsc->fdblocks -= pag->pag_rmapbt_resv.ar_orig_reserved;
 
-		xfs_perag_put(pag);
-
-		if (xchk_should_terminate(sc, &error))
-			break;
 	}
-
+	if (pag)
+		xfs_perag_put(pag);

It's not shown in the diff, but there is still an exit path out of the
above loop that calls xfs_perag_put(). The rest of the patch LGTM.

Good spot. Fixed.

FWIW, I'm not entirely happy with the way the iterator can break and
require conditional cleanup. I'm thinking that I'll come back to
these and convert them to a iterator structure that will turn this
into the pattern:

	perag_iter_init(&iter, start_agno, end_agno);
	for_each_perag(pag, iter) {
		....
	}
	perag_iter_done(&iter);

and so the code doesn't need to care about whether it exits the loop
via a break or running out of perags to iterate. I haven't fully
thought this through, though, so I'm leaving it alone for now...

I think something like that would be an improvement. It's
straightforward enough to follow through these changes with the loop
break quirk in mind, but I suspect that somebody modifying (and/or
reviewing) related code farther in the future might very easily miss
something like an external put being required if a loop is modified to
break out early.

Brian

-Dave.

PS - ain't english great? thought, through, though: look the same,
sound completely different...
-- 
Dave Chinner
david@fromorbit.com