On Mon, Sep 30, 2024 at 06:41:42PM +0200, Christoph Hellwig wrote: > Currently only the new agcount is passed to xfs_initialize_perag, which > requires lookups of existing AGs to skip them and complicates error > handling. Also pass the previous agcount so that the range that > xfs_initialize_perag operates on is exactly defined. That way the > extra lookups can be avoided, and error handling can clean up the > exact range from the old count to the last added perag structure. > > Signed-off-by: Christoph Hellwig <hch@xxxxxx> > Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx> > --- > fs/xfs/libxfs/xfs_ag.c | 29 ++++++++--------------------- > fs/xfs/libxfs/xfs_ag.h | 5 +++-- > fs/xfs/xfs_fsops.c | 18 ++++++++---------- > fs/xfs/xfs_log_recover.c | 5 +++-- > fs/xfs/xfs_mount.c | 4 ++-- > 5 files changed, 24 insertions(+), 37 deletions(-) > ... > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > index ec766b4bc8537b..6a165ca55da1a8 100644 > --- a/fs/xfs/xfs_log_recover.c > +++ b/fs/xfs/xfs_log_recover.c > @@ -3346,6 +3346,7 @@ xlog_do_recover( > struct xfs_mount *mp = log->l_mp; > struct xfs_buf *bp = mp->m_sb_bp; > struct xfs_sb *sbp = &mp->m_sb; > + xfs_agnumber_t old_agcount = sbp->sb_agcount; > int error; > > trace_xfs_log_recover(log, head_blk, tail_blk); > @@ -3393,8 +3394,8 @@ xlog_do_recover( > /* re-initialise in-core superblock and geometry structures */ > mp->m_features |= xfs_sb_version_to_features(sbp); > xfs_reinit_percpu_counters(mp); > - error = xfs_initialize_perag(mp, sbp->sb_agcount, sbp->sb_dblocks, > - &mp->m_maxagi); > + error = xfs_initialize_perag(mp, old_agcount, sbp->sb_agcount, > + sbp->sb_dblocks, &mp->m_maxagi); I assume this is because the superblock can change across recovery, but code wise this seems kind of easy to misread into thinking the variable is the same. I think the whole old/new terminology is kind of clunky for an interface that is not just for growfs. Maybe it would be more clear to use start/end terminology for xfs_initialize_perag(), then it's more straightforward that mount would init the full range whereas growfs inits a subrange. A oneliner comment or s/old_agcount/orig_agcount/ wouldn't hurt here either. Actually if that's the only purpose for this call and if you already have to sample sb_agcount, maybe just lifting/copying the if (old_agcount >= new_agcount) check into the caller would make the logic more self-explanatory. Hm? Otherwise the logic changes look Ok to me functionally. Brian > if (error) { > xfs_warn(mp, "Failed post-recovery per-ag init: %d", error); > return error; > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c > index 1fdd79c5bfa04e..6fa7239a4a01b6 100644 > --- a/fs/xfs/xfs_mount.c > +++ b/fs/xfs/xfs_mount.c > @@ -810,8 +810,8 @@ xfs_mountfs( > /* > * Allocate and initialize the per-ag data. > */ > - error = xfs_initialize_perag(mp, sbp->sb_agcount, mp->m_sb.sb_dblocks, > - &mp->m_maxagi); > + error = xfs_initialize_perag(mp, 0, sbp->sb_agcount, > + mp->m_sb.sb_dblocks, &mp->m_maxagi); > if (error) { > xfs_warn(mp, "Failed per-ag init: %d", error); > goto out_free_dir; > -- > 2.45.2 > >