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.
