Re: [PATCH 1/7] xfs: pass the exact range to initialize to xfs_initialize_perag

[Brian Foster <bfoster@redhat.com>]

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.
>