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

[Christoph Hellwig <hch@lst.de>]

On Mon, Sep 16, 2024 at 11:28:26AM +1000, Dave Chinner wrote:
> I'm missing something - the intents aren't processed until the log
> has been recovered - queuing an intent to be processed does
> not require the per-ag to be present. We don't take per-ag
> references until we are recovering the intent. i.e. we've completed
> journal recovery and haven't found the corresponding EFD.
> 
> That leaves the EFI in the log->r_dfops, and we then run
> ->recover_work in the second phase of recovery. It is
> xfs_extent_free_recover_work() that creates the
> new transaction and runs the EFI processing that requires the
> perag references, isn't it?
> 
> IOWs, I don't see where the initial EFI/EFD recovery during the
> checkpoint processing requires the newly created perags to be
> present in memory for processing incomplete EFIs before the journal
> recovery phase has completed.

So my new test actually blows up before creating intents:

[   81.695529] XFS (nvme1n1): Mounting V5 Filesystem 07057234-4bec-4f17-97c5-420c71c83292
[   81.704541] XFS (nvme1n1): Starting recovery (logdev: internal)
[   81.707260] XFS (nvme1n1): xfs_buf_map_verify: daddr 0x40003 out of range, EOFS 0x40000
[   81.707974] ------------[ cut here ]------------
[   81.708376] WARNING: CPU: 1 PID: 5004 at fs/xfs/xfs_buf.c:553 xfs_buf_get_map+0x8b4/0xb70

Because sb_dblocks hasn't been updated yet.  I'd kinda assume we run
into the intents next, but maybe we don't.  I can try how far just
fixing the sb would get us, but that will potentially gets us into
more problems late the more we actually use the pag structure.

> If we are going to keep this logic, can you do this as a separate
> helper function? i.e.:

I actually did that earlier, and it turned out to create a bit more
boilerplate than I liked, but I can revert to it if there is a strong
preference.

> > +	xfs_sb_from_disk(&mp->m_sb, dsb);
> > +	if (mp->m_sb.sb_agcount < old_agcount) {
> > +		xfs_alert(mp, "Shrinking AG count in log recovery");
> > +		return -EFSCORRUPTED;
> > +	}
> > +	mp->m_features |= xfs_sb_version_to_features(&mp->m_sb);
> 
> I'm not sure this is safe. The item order in the checkpoint recovery
> isn't guaranteed to be exactly the same as when feature bits are
> modified at runtime. Hence there could be items in the checkpoint
> that haven't yet been recovered that are dependent on the original
> sb feature mask being present.  It may be OK to do this at the end
> of the checkpoint being recovered.
> 
> I'm also not sure why this feature update code is being changed
> because it's not mentioned at all in the commit message.

Mostly to keep the features in sync with the in-memory sb fields
updated above.  I'll switch to keep this as-is, but I fail to see how
updating features only after the entire reocvery is done will be safe
for all cases either.

Where would we depend on the old feature setting?

> 
> > +	error = xfs_initialize_perag(mp, old_agcount, mp->m_sb.sb_agcount,
> > +			mp->m_sb.sb_dblocks, &mp->m_maxagi);
> 
> Why do this if sb_agcount has not changed?  AFAICT it only iterates
> the AGs already initialised and so skips them, then recalculates
> inode32 and prealloc block parameters, which won't change. Hence
> it's a total no-op for anything other than an actual ag count change
> and should be skipped, right?

Yes, and the way how xfs_initialize_perag it is an entire no-op.
But I can add an extra explicit check to make that more clear.