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

[Brian Foster <bfoster@redhat.com>]

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

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
>