On Fri, Oct 11, 2024 at 09:53:14AM +0200, Christoph Hellwig wrote: > On Thu, Oct 10, 2024 at 10:02:49AM -0400, Brian Foster wrote: > > > - 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. > > Which variable? > old_agcount and sb_agcount and the fact that the value of the latter might change down in the recovery code isn't immediately obvious. A oneliner and/or logic check suggested below would clear it up IMO, thanks. Brian > > 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. > > fine with me. > > > 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? > > Sure. >
