Re: [PATCH 2/4] xfs: inode fork allocation depends on XFS_IFEXTENT flag

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Mar 31, 2021 at 08:40:07AM +1100, Dave Chinner wrote:
> On Tue, Mar 30, 2021 at 11:06:17AM -0700, Darrick J. Wong wrote:
> > On Tue, Mar 30, 2021 at 04:30:57PM +1100, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > XFS_IFEXTENT has two incompatible meanings to the code. The first
> > > meaning is that the fork is in extent format, the second meaning is
> > > that the extent list has been read into memory.
> > 
> > I don't agree that IFEXTENTS has two meanings.  This is what I
> > understand of how xfs_ifork fields and surrounding code are supposed to
> > work; can you point out what's wrong?
> > 
> >  1. xfs_ifork.if_format == XFS_DINODE_FMT_EXTENTS tells us if the fork
> >     is in extent format.
> > 
> >  2. (xfs_ifork.if_flags & XFS_IFEXTENTS) tells us if the incore extent
> >     map is initialized.
> > 
> >  3. If we are creating a fork with if_format == EXTENTS, the incore map
> >     is trivially initialized, and therefore IFEXTENTS should be set
> >     because no further work is required.
> > 
> >  4. If we are reading an if_format == EXTENTS fork in from disk (during
> >     xfs_iread), we always populate the incore map and set IFEXTENTS.
> > 
> >  5. If if_format == BTREE and IFEXTENTS is not set, the incore map is
> >     *not* initialized, and we must call xfs_iread_extents to walk the
> >     ondisk btree to initialize the incore btree, and to set IFEXTENTS.
> > 
> >  6. xfs_iread_extents requires that if_format == BTREE and will return
> >     an error and log a corruption report if it sees another fork format.
> > 
> > From points 3 and 4, I conclude that (prior to xfs-5.13-merge) IFEXTENTS
> > is always set if if_format is FMT_EXTENTS.
> 
> ifp->if_flags is set to XFS_IFINLINE for local format forks,
> XFS_IFEXTENTS for extent format forks, and XFS_IFBROOT for btree
> roots in the inode fork.
> 
> THe contents of the fork are mode definitely defined by the flags
> in the fork structure.
> 
> The problem is that we've overloaded XFS_IFEXTENTS to -also- mean
> "extents loaded in memory". The in-core extent tree used to be a
> IFBROOT only feature - XFS_IFEXTENTS format forks
> held the extent data in the inode fork itself, not in the incore
> extent tree, and so always had direct access to the extent records.
> It never needed another flag to mean "extents have been read into
> memory", because they always were present in the inode fork when
> XFS_IFEXTENTS was set. 
> 
> What we used to have is another flag - XFS_IFEXTIREC - to indicate
> that the XFS_IFBROOT format root was read into the incore memory
> tree. This was removed in commit 6bdcf26ade88 ("xfs: use a b+tree
> for the in-core extent list") when the btree was added for both
> extent format and btree format forks, and it's use to indicate that
> the btree had been read was replaced with the XFS_IFEXTENTS flag.
> 
> That's when XFS_IFEXTENTS gained it's dual meaning.
> 
> IOWS:
> 
> - XFS_IFINLINE means inode fork data is inode type specific data
> - XFS_IFEXTENTS means the inode fork data is in extent format and
>   that the in-core extent btree has been populated
> - XFS_IFBROOT means the inode fork data is a btree root
> - XFS_IFBROOT|XFS_IFEXTENTS mean the inode data fork is a btree root
>   and that the in-core extent btree has been populated
> 
> Historically, that last case was XFS_IFBROOT|XFS_IFEXTIREC. What
> should have been done in 6bdcf26ade88 is the XFS_IFEXTENTS format
> fork should have become XFS_IFEXTENTS|XFS_IFEXTIREC to indicate
> "extent format, extent tree populated", rather than eliding
> XFS_IFEXTIREC and redefining XFS_IFEXTENTS to mean "extent tree
> populated".  i.e. the separate flag to indicate the difference
> between fork format and in-memory state should have been
> retained....
> 
> > From point 6, I conclude that it's not possible for IFEXTENTS not to be
> > set if if_format is FMT_EXTENTS, because if an inode fork ever ended up
> > in that state, there would not be any way to escape.
> 
> That's an implementation detail arising from always reading the
> extent list from the on-disk inode fork into in-memory extent list.
> 
> > > When the inode fork is in extent format, we automatically read the
> > > extent list into memory and indexed by the inode extent btree when
> > > the inode is brought into memory off disk.
> > 
> > Agreed, that's #4 above.
> 
> Yes, that's an implementation detail - we currently do not allow an
> inode in extent form to be read in without populating the in-core
> extent btree, whether we need to read extents or not. Hence the
> confusion over "I know btree format uses this to indicate the extent
> tree has been read" vs "this always needs to be set when in extent
> format". That's the logic landmine I tripped over here.
> 
> Realistically, we should be separating the in-memory extent tree
> initialisation from inode fork initialisation because directory traversal
> workloads that just to look at inode state does not need to populate
> the extent btree. Doing so for every inode is wasted memory and
> CPU. We should init the extent btree on the first operation that
> needs the extent list, like we do for btrees, and for that we need
> XFS_IFEXTIREC to be re-introduced to clearly separate the in-memory
> fork format from the extent tree state.
> 
> > > This fixes a scrub regression because it assumes XFS_IFEXTENT means
> > > "on disk format" and not "read into memory" and e6a688c33238 assumed
> > > it mean "read into memory". In reality, the XFS_IFEXTENT flag needs
> > > to be split up into two flags - one for the on disk fork format and
> > > one for the in-memory "extent btree has been populated" state.
> > 
> > Let's look at the relevant code in xchk_bmap(), since I wrote that
> > function:
> > 
> > 	/* Check the fork values */
> > 	switch (ifp->if_format) {
> > 	...
> > 	case XFS_DINODE_FMT_EXTENTS:
> > 		if (!(ifp->if_flags & XFS_IFEXTENTS)) {
> > 			xchk_fblock_set_corrupt(sc, whichfork, 0);
> > 			goto out;
> > 		}
> > 		break;
> > 
> > The switch statement checks the format (#1), and the flag test checks
> > that the incore state (#3 and #4) hold true.  Perhaps it was unwise of
> > scrub to check *incore* state flags here, but as of the time the code
> > was written, it was always the case that FMT_EXTENTS and IFEXTENTS went
> > together.  Setting SCRUB_OFLAG_CORRUPT is how scrub signals that
> > something is wrong and administrator intervention is needed.
> > 
> > I agree with the code fix, but not with the justification.
> 
> If you take into account the history of this code, you can see that
> XFS_IFEXTIREC -> XFS_IFEXTENTS did, indeed, give XFS_IFEXTENTS two
> meanings...

Aha, so we ended up more or less agreeing on the code fix but via two
verrrry different paths (organizational knowledge vs. interpreting the
code).

> What I've written is mostly correct, yet what you've written is
> also mostly correct. So what do you want me to put in the commit
> message?

I'd be fine with pasting in both, along with a note that while we agree
on the code fix we arrived at it for different reasons ... if you think
that Christoph's RFC cleanup is a reasonable thing to bundle in right
after it.

It looks ok to me (once I cleaned up the build robot complaints) coming
from the narrower perspective of looking at iforks as they are now and
as they would become, FWIW, but since you know more about the historical
design points I'm curious what you think. :)

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com