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@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxx
>
