| Message ID | 20210330053059.1339949-3-david@fromorbit.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: fix eager attr fork init regressions | expand |
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. 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. > 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. > Hence we set the flag to mean both "in extent format and in memory". I don't agree. I think #1 tells us "in extent format" and #2 tells us "in memory"; and #3 and #4 are how we guarantee that. > That requires all new > fork allocations where the default state is "extent format with zero > extents" to set the XFS_IFEXTENT to indicate we've initialised the > in-memory state even though we've really done no such thing. <shrug> IMHO we initialized it trivially, but that's splitting hairs and doesn't warrant further argument, so I'll move on. > 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. --D > Fixes: e6a688c33238 ("xfs: initialise attr fork on inode create") > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/libxfs/xfs_bmap.c | 1 - > fs/xfs/libxfs/xfs_inode_fork.c | 9 +++++++++ > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > index 5574d345d066..2f72849c05f9 100644 > --- a/fs/xfs/libxfs/xfs_bmap.c > +++ b/fs/xfs/libxfs/xfs_bmap.c > @@ -1095,7 +1095,6 @@ xfs_bmap_add_attrfork( > ASSERT(ip->i_afp == NULL); > > ip->i_afp = xfs_ifork_alloc(XFS_DINODE_FMT_EXTENTS, 0); > - ip->i_afp->if_flags = XFS_IFEXTENTS; > logflags = 0; > switch (ip->i_df.if_format) { > case XFS_DINODE_FMT_LOCAL: > diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c > index 1851d6f266d0..03e1a21848eb 100644 > --- a/fs/xfs/libxfs/xfs_inode_fork.c > +++ b/fs/xfs/libxfs/xfs_inode_fork.c > @@ -292,6 +292,15 @@ xfs_ifork_alloc( > ifp = kmem_cache_zalloc(xfs_ifork_zone, GFP_NOFS | __GFP_NOFAIL); > ifp->if_format = format; > ifp->if_nextents = nextents; > + > + /* > + * If this is a caller initialising a newly created fork, we need to > + * set XFS_IFEXTENTS to indicate the fork state is completely up to > + * date. Otherwise it is up to the caller to initialise the in-memory > + * state of the inode fork from the on-disk state. > + */ > + if (format == XFS_DINODE_FMT_EXTENTS && nextents == 0) > + ifp->if_flags |= XFS_IFEXTENTS; > return ifp; > } > > -- > 2.31.0 >
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... 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? Cheers, Dave.
On Wed, Mar 31, 2021 at 08:40:07AM +1100, Dave Chinner wrote: > 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. No. All the above are flags, not alternatives. (and reading futher you actually properly document it below). > The problem is that we've overloaded XFS_IFEXTENTS to -also- mean > "extents loaded in memory". I would not call this an overload. It is a somewhat quirky encoding that actually works pretty well for the use case. > 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. No. The XFS_IFEXTENTS meaning did not change at all in that commit. Even before a lot of code looked at XFS_IFEXTENTS and if it wasn't set called xfs_iread_extents, XFS_IFEXTIREC was only used internally for the in-memory indirection array and was completely unrelated to the on-disk format. > - 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. No, that was not the case, even historically. btree based inodes already existed for more than 10 years when commit 0293ce3a9fd1 added XFS_IFEXTIREC to singinify the in-memory indirect extent array. > 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.... I strongly disagree. If we want to clean this up the right thing is to remove XFS_IFINLINE and XFS_IFBROOT entirely, and just look at the if_format field for the extent format.
On Tue, Mar 30, 2021 at 04:30:57PM +1100, Dave Chinner wrote: > +++ b/fs/xfs/libxfs/xfs_inode_fork.c > @@ -292,6 +292,15 @@ xfs_ifork_alloc( > ifp = kmem_cache_zalloc(xfs_ifork_zone, GFP_NOFS | __GFP_NOFAIL); > ifp->if_format = format; > ifp->if_nextents = nextents; > + > + /* > + * If this is a caller initialising a newly created fork, we need to > + * set XFS_IFEXTENTS to indicate the fork state is completely up to > + * date. Otherwise it is up to the caller to initialise the in-memory > + * state of the inode fork from the on-disk state. > + */ > + if (format == XFS_DINODE_FMT_EXTENTS && nextents == 0) > + ifp->if_flags |= XFS_IFEXTENTS; > return ifp; I'm not sure this is a good idea. I'd rather set XFS_IFEXTENTS manually in xfs_init_new_inode until we sort out the whole flags thing.
On Fri, Apr 02, 2021 at 08:02:37AM +0100, Christoph Hellwig wrote: > On Wed, Mar 31, 2021 at 08:40:07AM +1100, Dave Chinner wrote: > > 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.... > > I strongly disagree. If we want to clean this up the right thing is > to remove XFS_IFINLINE and XFS_IFBROOT entirely, and just look at the > if_format field for the extent format. Sounds great, but regardless of the historical argument, this bug still needs to be fix, and removing XFS_IFINLINE/XFS_IFBROOT is far to much for what is effectively a one liner... Cheers, Dave.
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
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c index 5574d345d066..2f72849c05f9 100644 --- a/fs/xfs/libxfs/xfs_bmap.c +++ b/fs/xfs/libxfs/xfs_bmap.c @@ -1095,7 +1095,6 @@ xfs_bmap_add_attrfork( ASSERT(ip->i_afp == NULL); ip->i_afp = xfs_ifork_alloc(XFS_DINODE_FMT_EXTENTS, 0); - ip->i_afp->if_flags = XFS_IFEXTENTS; logflags = 0; switch (ip->i_df.if_format) { case XFS_DINODE_FMT_LOCAL: diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c index 1851d6f266d0..03e1a21848eb 100644 --- a/fs/xfs/libxfs/xfs_inode_fork.c +++ b/fs/xfs/libxfs/xfs_inode_fork.c @@ -292,6 +292,15 @@ xfs_ifork_alloc( ifp = kmem_cache_zalloc(xfs_ifork_zone, GFP_NOFS | __GFP_NOFAIL); ifp->if_format = format; ifp->if_nextents = nextents; + + /* + * If this is a caller initialising a newly created fork, we need to + * set XFS_IFEXTENTS to indicate the fork state is completely up to + * date. Otherwise it is up to the caller to initialise the in-memory + * state of the inode fork from the on-disk state. + */ + if (format == XFS_DINODE_FMT_EXTENTS && nextents == 0) + ifp->if_flags |= XFS_IFEXTENTS; return ifp; }