Re: [PATCH 04/22] xfs: make for_each_perag... a first class citizen

[Dave Chinner <david@fromorbit.com>]

On Mon, May 10, 2021 at 08:53:56AM -0400, Brian Foster wrote:
> On Thu, May 06, 2021 at 05:20:36PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > 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 <dchinner@redhat.com>
> > ---
> >  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(-)
> > 
> ...
> > 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
> ...
> > @@ -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...

-Dave.

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