Re: [PATCH 3/4] xfs: create perag structures as soon as possible during log recovery

[Dave Chinner <david@fromorbit.com>]

On Thu, Sep 19, 2024 at 09:46:31AM +0200, Christoph Hellwig wrote:
> On Thu, Sep 19, 2024 at 11:09:40AM +1000, Dave Chinner wrote:
> > Ideally, we should not be using the new AGs until *after* the growfs
> > transaction has hit stable storage (i.e. the journal has fully
> > commmitted the growfs transaction), not just committed to the CIL.
> 
> Yes.  A crude version of that - freeze/unfreeze before setting the
> AG live was my other initial idea, but Darrick wasn't exactly
> excited about that..

I'm not exactly excited by that idea, either...

> > The second step is preventing allocations that are running from
> > seeing the mp->m_sb.sb_agcount update until after the transaction is
> > stable.
> 
> Or just not seeing the pag as active by not setting the initial
> active reference until after the transaction is stable.  Given
> all the issues outlined by you about sb locking that might be the
> easier approach.

Yeah, that's a good idea for avoiding perag references from
iterations before the growfs is stable.

However, my concern is whether that is sufficient.  Whilst I didn't
mention it, changing sb_agcount and sb_dblocks before the grwofs is
stable affects things like size calculations for the old end runt
AG(*) because it is no longer considered a runt the moment we change
the in-memory size fields in the superblock. That will, at least,
affect ino/fsbno/agbno verification, as well as corruption checks
through the code.

An alternative is to delay the perag initialisation until after the
growfs is stable, because we don't want the old runt AG size to
visibly change until after the growfs is stable.

There may be more potential issues, but I haven't done a careful
code audit and that's kinda why I suggested simply delaying the
in-memory state update. Delaying the update removes the whole
in-memory transient state situation altogether...

-Dave.

(*) The precalculated AG length and inode min/max values we've added
to the perag (calculated at perag init time) should be used for
these calculations. That gets around this 'growfs changes sb values
dynamically' issue, but not all the places where we do these
verifications have a valid perag reference to pass these
functions. 

-- 
Dave Chinner
david@fromorbit.com