On Wed, May 12, 2021 at 07:33:12AM +1000, Dave Chinner wrote:
> On Tue, May 11, 2021 at 08:29:38AM -0400, Brian Foster wrote:
> > 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.
>
> *nod*
>
> I think I'll need to address this when I do the conversion of these
> iterators to use active references to pin or skip perags that are
> being torn down. The iterators becomes slightly more complex at this
> point, so that's probably the best point to address this.
This sounds like a good change to me. :)
--D
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
