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.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
