* [PATCH 0/4] xfs: fix eager attr fork init regressions @ 2021-03-30 5:30 Dave Chinner 2021-03-30 5:30 ` [PATCH 1/4] xfs: eager inode attr fork init needs attr feature awareness Dave Chinner ` (3 more replies) 0 siblings, 4 replies; 20+ messages in thread From: Dave Chinner @ 2021-03-30 5:30 UTC (permalink / raw) To: linux-xfs Hi folks, These are the fixes for the attr fork regressions in the current for-next tree. The got through testing because none of my test regression VMs had selinux enabled on them, while my perf test VMs did. Hence it was never exercised by the fstests runs I did. I've re-enabled selinux on some of my test VMs, and run it through fstests ofor v4 and v5 filesystems with selinux enabled. The first 3 patches are the fixes that address the regressions. The last patch is an optimisation I noticed that avoids recalculating a static, fixed value on every call to xfs_default_attroffset(). It is not needed for the bugs to be fixed, but is definitely a for-next candidate while I'm touching that code again. Cheers, Dave. ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/4] xfs: eager inode attr fork init needs attr feature awareness 2021-03-30 5:30 [PATCH 0/4] xfs: fix eager attr fork init regressions Dave Chinner @ 2021-03-30 5:30 ` Dave Chinner 2021-03-30 18:10 ` Darrick J. Wong 2021-04-02 6:48 ` Christoph Hellwig 2021-03-30 5:30 ` [PATCH 2/4] xfs: inode fork allocation depends on XFS_IFEXTENT flag Dave Chinner ` (2 subsequent siblings) 3 siblings, 2 replies; 20+ messages in thread From: Dave Chinner @ 2021-03-30 5:30 UTC (permalink / raw) To: linux-xfs From: Dave Chinner <dchinner@redhat.com> The pitfalls of regression testing on a machine without realising that selinux was disabled. Only set the attr fork during inode allocation if the attr feature bits are already set on the superblock. Fixes: e6a688c33238 ("xfs: initialise attr fork on inode create") Signed-off-by: Dave Chinner <dchinner@redhat.com> --- fs/xfs/xfs_inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index c09bb39baeea..3b516ab7f22b 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -887,7 +887,7 @@ xfs_init_new_inode( * this saves us from needing to run a separate transaction to set the * fork offset in the immediate future. */ - if (init_xattrs) { + if (init_xattrs && xfs_sb_version_hasattr(&mp->m_sb)) { ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3; ip->i_afp = xfs_ifork_alloc(XFS_DINODE_FMT_EXTENTS, 0); } -- 2.31.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/4] xfs: eager inode attr fork init needs attr feature awareness 2021-03-30 5:30 ` [PATCH 1/4] xfs: eager inode attr fork init needs attr feature awareness Dave Chinner @ 2021-03-30 18:10 ` Darrick J. Wong 2021-04-02 6:48 ` Christoph Hellwig 1 sibling, 0 replies; 20+ messages in thread From: Darrick J. Wong @ 2021-03-30 18:10 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-xfs On Tue, Mar 30, 2021 at 04:30:56PM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > The pitfalls of regression testing on a machine without realising > that selinux was disabled. Only set the attr fork during inode > allocation if the attr feature bits are already set on the > superblock. > > Fixes: e6a688c33238 ("xfs: initialise attr fork on inode create") > Signed-off-by: Dave Chinner <dchinner@redhat.com> Looks reasonable to me, Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > fs/xfs/xfs_inode.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index c09bb39baeea..3b516ab7f22b 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -887,7 +887,7 @@ xfs_init_new_inode( > * this saves us from needing to run a separate transaction to set the > * fork offset in the immediate future. > */ > - if (init_xattrs) { > + if (init_xattrs && xfs_sb_version_hasattr(&mp->m_sb)) { > ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3; > ip->i_afp = xfs_ifork_alloc(XFS_DINODE_FMT_EXTENTS, 0); > } > -- > 2.31.0 > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/4] xfs: eager inode attr fork init needs attr feature awareness 2021-03-30 5:30 ` [PATCH 1/4] xfs: eager inode attr fork init needs attr feature awareness Dave Chinner 2021-03-30 18:10 ` Darrick J. Wong @ 2021-04-02 6:48 ` Christoph Hellwig 1 sibling, 0 replies; 20+ messages in thread From: Christoph Hellwig @ 2021-04-02 6:48 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-xfs On Tue, Mar 30, 2021 at 04:30:56PM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > The pitfalls of regression testing on a machine without realising > that selinux was disabled. Only set the attr fork during inode > allocation if the attr feature bits are already set on the > superblock. > > Fixes: e6a688c33238 ("xfs: initialise attr fork on inode create") > Signed-off-by: Dave Chinner <dchinner@redhat.com> Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/4] xfs: inode fork allocation depends on XFS_IFEXTENT flag 2021-03-30 5:30 [PATCH 0/4] xfs: fix eager attr fork init regressions Dave Chinner 2021-03-30 5:30 ` [PATCH 1/4] xfs: eager inode attr fork init needs attr feature awareness Dave Chinner @ 2021-03-30 5:30 ` Dave Chinner 2021-03-30 18:06 ` Darrick J. Wong 2021-04-02 7:06 ` Christoph Hellwig 2021-03-30 5:30 ` [PATCH 3/4] xfs: default attr fork size does not handle device inodes Dave Chinner 2021-03-30 5:30 ` [PATCH 4/4] xfs: precalculate default inode attribute offset Dave Chinner 3 siblings, 2 replies; 20+ messages in thread From: Dave Chinner @ 2021-03-30 5:30 UTC (permalink / raw) To: linux-xfs 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. 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. Hence we set the flag to mean both "in extent format and in memory". 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. 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. 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] xfs: inode fork allocation depends on XFS_IFEXTENT flag 2021-03-30 5:30 ` [PATCH 2/4] xfs: inode fork allocation depends on XFS_IFEXTENT flag Dave Chinner @ 2021-03-30 18:06 ` Darrick J. Wong 2021-03-30 21:40 ` Dave Chinner 2021-04-02 7:06 ` Christoph Hellwig 1 sibling, 1 reply; 20+ messages in thread From: Darrick J. Wong @ 2021-03-30 18:06 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-xfs 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 > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] xfs: inode fork allocation depends on XFS_IFEXTENT flag 2021-03-30 18:06 ` Darrick J. Wong @ 2021-03-30 21:40 ` Dave Chinner 2021-04-02 7:02 ` Christoph Hellwig 2021-04-04 3:25 ` Darrick J. Wong 0 siblings, 2 replies; 20+ messages in thread From: Dave Chinner @ 2021-03-30 21:40 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs 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. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] xfs: inode fork allocation depends on XFS_IFEXTENT flag 2021-03-30 21:40 ` Dave Chinner @ 2021-04-02 7:02 ` Christoph Hellwig 2021-04-03 22:21 ` Dave Chinner 2021-04-04 3:25 ` Darrick J. Wong 1 sibling, 1 reply; 20+ messages in thread From: Christoph Hellwig @ 2021-04-02 7:02 UTC (permalink / raw) To: Dave Chinner; +Cc: Darrick J. Wong, linux-xfs 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] xfs: inode fork allocation depends on XFS_IFEXTENT flag 2021-04-02 7:02 ` Christoph Hellwig @ 2021-04-03 22:21 ` Dave Chinner 0 siblings, 0 replies; 20+ messages in thread From: Dave Chinner @ 2021-04-03 22:21 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Darrick J. Wong, linux-xfs 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. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] xfs: inode fork allocation depends on XFS_IFEXTENT flag 2021-03-30 21:40 ` Dave Chinner 2021-04-02 7:02 ` Christoph Hellwig @ 2021-04-04 3:25 ` Darrick J. Wong 1 sibling, 0 replies; 20+ messages in thread From: Darrick J. Wong @ 2021-04-04 3:25 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-xfs 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] xfs: inode fork allocation depends on XFS_IFEXTENT flag 2021-03-30 5:30 ` [PATCH 2/4] xfs: inode fork allocation depends on XFS_IFEXTENT flag Dave Chinner 2021-03-30 18:06 ` Darrick J. Wong @ 2021-04-02 7:06 ` Christoph Hellwig 1 sibling, 0 replies; 20+ messages in thread From: Christoph Hellwig @ 2021-04-02 7:06 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-xfs 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/4] xfs: default attr fork size does not handle device inodes 2021-03-30 5:30 [PATCH 0/4] xfs: fix eager attr fork init regressions Dave Chinner 2021-03-30 5:30 ` [PATCH 1/4] xfs: eager inode attr fork init needs attr feature awareness Dave Chinner 2021-03-30 5:30 ` [PATCH 2/4] xfs: inode fork allocation depends on XFS_IFEXTENT flag Dave Chinner @ 2021-03-30 5:30 ` Dave Chinner 2021-03-30 18:10 ` Darrick J. Wong 2021-04-02 7:08 ` Christoph Hellwig 2021-03-30 5:30 ` [PATCH 4/4] xfs: precalculate default inode attribute offset Dave Chinner 3 siblings, 2 replies; 20+ messages in thread From: Dave Chinner @ 2021-03-30 5:30 UTC (permalink / raw) To: linux-xfs From: Dave Chinner <dchinner@redhat.com> Device inodes have a non-default data fork size of 8 bytes as checked/enforced by xfs_repair. xfs_default_attroffset() doesn't handle this, so lets do a minor refactor so it does. Fixes: e6a688c33238 ("xfs: initialise attr fork on inode create") Signed-off-by: Dave Chinner <dchinner@redhat.com> --- fs/xfs/libxfs/xfs_bmap.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c index 2f72849c05f9..16c8730c140f 100644 --- a/fs/xfs/libxfs/xfs_bmap.c +++ b/fs/xfs/libxfs/xfs_bmap.c @@ -195,6 +195,9 @@ xfs_default_attroffset( struct xfs_mount *mp = ip->i_mount; uint offset; + if (ip->i_df.if_format == XFS_DINODE_FMT_DEV) + return roundup(sizeof(xfs_dev_t), 8); + if (mp->m_sb.sb_inodesize == 256) offset = XFS_LITINO(mp) - XFS_BMDR_SPACE_CALC(MINABTPTRS); else @@ -1036,16 +1039,18 @@ xfs_bmap_set_attrforkoff( int size, int *version) { + int default_size = xfs_default_attroffset(ip) >> 3; + switch (ip->i_df.if_format) { case XFS_DINODE_FMT_DEV: - ip->i_d.di_forkoff = roundup(sizeof(xfs_dev_t), 8) >> 3; + ip->i_d.di_forkoff = default_size; break; case XFS_DINODE_FMT_LOCAL: case XFS_DINODE_FMT_EXTENTS: case XFS_DINODE_FMT_BTREE: ip->i_d.di_forkoff = xfs_attr_shortform_bytesfit(ip, size); if (!ip->i_d.di_forkoff) - ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3; + ip->i_d.di_forkoff = default_size; else if ((ip->i_mount->m_flags & XFS_MOUNT_ATTR2) && version) *version = 2; break; -- 2.31.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 3/4] xfs: default attr fork size does not handle device inodes 2021-03-30 5:30 ` [PATCH 3/4] xfs: default attr fork size does not handle device inodes Dave Chinner @ 2021-03-30 18:10 ` Darrick J. Wong 2021-04-02 7:08 ` Christoph Hellwig 1 sibling, 0 replies; 20+ messages in thread From: Darrick J. Wong @ 2021-03-30 18:10 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-xfs On Tue, Mar 30, 2021 at 04:30:58PM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > Device inodes have a non-default data fork size of 8 bytes > as checked/enforced by xfs_repair. xfs_default_attroffset() doesn't > handle this, so lets do a minor refactor so it does. > > Fixes: e6a688c33238 ("xfs: initialise attr fork on inode create") > Signed-off-by: Dave Chinner <dchinner@redhat.com> Looks good to me, Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > fs/xfs/libxfs/xfs_bmap.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > index 2f72849c05f9..16c8730c140f 100644 > --- a/fs/xfs/libxfs/xfs_bmap.c > +++ b/fs/xfs/libxfs/xfs_bmap.c > @@ -195,6 +195,9 @@ xfs_default_attroffset( > struct xfs_mount *mp = ip->i_mount; > uint offset; > > + if (ip->i_df.if_format == XFS_DINODE_FMT_DEV) > + return roundup(sizeof(xfs_dev_t), 8); > + > if (mp->m_sb.sb_inodesize == 256) > offset = XFS_LITINO(mp) - XFS_BMDR_SPACE_CALC(MINABTPTRS); > else > @@ -1036,16 +1039,18 @@ xfs_bmap_set_attrforkoff( > int size, > int *version) > { > + int default_size = xfs_default_attroffset(ip) >> 3; > + > switch (ip->i_df.if_format) { > case XFS_DINODE_FMT_DEV: > - ip->i_d.di_forkoff = roundup(sizeof(xfs_dev_t), 8) >> 3; > + ip->i_d.di_forkoff = default_size; > break; > case XFS_DINODE_FMT_LOCAL: > case XFS_DINODE_FMT_EXTENTS: > case XFS_DINODE_FMT_BTREE: > ip->i_d.di_forkoff = xfs_attr_shortform_bytesfit(ip, size); > if (!ip->i_d.di_forkoff) > - ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3; > + ip->i_d.di_forkoff = default_size; > else if ((ip->i_mount->m_flags & XFS_MOUNT_ATTR2) && version) > *version = 2; > break; > -- > 2.31.0 > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/4] xfs: default attr fork size does not handle device inodes 2021-03-30 5:30 ` [PATCH 3/4] xfs: default attr fork size does not handle device inodes Dave Chinner 2021-03-30 18:10 ` Darrick J. Wong @ 2021-04-02 7:08 ` Christoph Hellwig 1 sibling, 0 replies; 20+ messages in thread From: Christoph Hellwig @ 2021-04-02 7:08 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-xfs On Tue, Mar 30, 2021 at 04:30:58PM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > Device inodes have a non-default data fork size of 8 bytes > as checked/enforced by xfs_repair. xfs_default_attroffset() doesn't > handle this, so lets do a minor refactor so it does. > > Fixes: e6a688c33238 ("xfs: initialise attr fork on inode create") > Signed-off-by: Dave Chinner <dchinner@redhat.com> Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/4] xfs: precalculate default inode attribute offset 2021-03-30 5:30 [PATCH 0/4] xfs: fix eager attr fork init regressions Dave Chinner ` (2 preceding siblings ...) 2021-03-30 5:30 ` [PATCH 3/4] xfs: default attr fork size does not handle device inodes Dave Chinner @ 2021-03-30 5:30 ` Dave Chinner 2021-03-30 18:10 ` Darrick J. Wong 2021-04-02 7:10 ` Christoph Hellwig 3 siblings, 2 replies; 20+ messages in thread From: Dave Chinner @ 2021-03-30 5:30 UTC (permalink / raw) To: linux-xfs From: Dave Chinner <dchinner@redhat.com> Default attr fork offset is based on inode size, so is a fixed geometry parameter of the inode. Move it to the xfs_ino_geometry structure and stop calculating it on every call to xfs_default_attroffset(). Signed-off-by: Dave Chinner <dchinner@redhat.com> --- fs/xfs/libxfs/xfs_bmap.c | 21 ++++++++++----------- fs/xfs/libxfs/xfs_bmap.h | 1 + fs/xfs/libxfs/xfs_shared.h | 4 ++++ fs/xfs/xfs_mount.c | 14 +++++++++++++- 4 files changed, 28 insertions(+), 12 deletions(-) diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c index 16c8730c140f..7930f11e4c54 100644 --- a/fs/xfs/libxfs/xfs_bmap.c +++ b/fs/xfs/libxfs/xfs_bmap.c @@ -94,6 +94,15 @@ xfs_bmap_compute_maxlevels( mp->m_bm_maxlevels[whichfork] = level; } +unsigned int +xfs_bmap_compute_attr_offset( + struct xfs_mount *mp) +{ + if (mp->m_sb.sb_inodesize == 256) + return XFS_LITINO(mp) - XFS_BMDR_SPACE_CALC(MINABTPTRS); + return XFS_BMDR_SPACE_CALC(6 * MINABTPTRS); +} + STATIC int /* error */ xfs_bmbt_lookup_eq( struct xfs_btree_cur *cur, @@ -192,19 +201,9 @@ uint xfs_default_attroffset( struct xfs_inode *ip) { - struct xfs_mount *mp = ip->i_mount; - uint offset; - if (ip->i_df.if_format == XFS_DINODE_FMT_DEV) return roundup(sizeof(xfs_dev_t), 8); - - if (mp->m_sb.sb_inodesize == 256) - offset = XFS_LITINO(mp) - XFS_BMDR_SPACE_CALC(MINABTPTRS); - else - offset = XFS_BMDR_SPACE_CALC(6 * MINABTPTRS); - - ASSERT(offset < XFS_LITINO(mp)); - return offset; + return M_IGEO(ip->i_mount)->attr_fork_offset; } /* diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h index 6747e97a7949..a49df4092c30 100644 --- a/fs/xfs/libxfs/xfs_bmap.h +++ b/fs/xfs/libxfs/xfs_bmap.h @@ -185,6 +185,7 @@ static inline bool xfs_bmap_is_written_extent(struct xfs_bmbt_irec *irec) void xfs_trim_extent(struct xfs_bmbt_irec *irec, xfs_fileoff_t bno, xfs_filblks_t len); +unsigned int xfs_bmap_compute_attr_offset(struct xfs_mount *mp); int xfs_bmap_add_attrfork(struct xfs_inode *ip, int size, int rsvd); int xfs_bmap_set_attrforkoff(struct xfs_inode *ip, int size, int *version); void xfs_bmap_local_to_extents_empty(struct xfs_trans *tp, diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h index 8c61a461bf7b..782fdd08f759 100644 --- a/fs/xfs/libxfs/xfs_shared.h +++ b/fs/xfs/libxfs/xfs_shared.h @@ -176,8 +176,12 @@ struct xfs_ino_geometry { unsigned int agino_log; /* #bits for agino in inum */ + /* precomputed default inode attribute fork offset */ + unsigned int attr_fork_offset; + /* precomputed value for di_flags2 */ uint64_t new_diflags2; + }; #endif /* __XFS_SHARED_H__ */ diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c index 1c97b155a8ee..cb1e2c4702c3 100644 --- a/fs/xfs/xfs_mount.c +++ b/fs/xfs/xfs_mount.c @@ -675,6 +675,18 @@ xfs_unmount_flush_inodes( xfs_health_unmount(mp); } +static void +xfs_mount_setup_inode_geom( + struct xfs_mount *mp) +{ + struct xfs_ino_geometry *igeo = M_IGEO(mp); + + igeo->attr_fork_offset = xfs_bmap_compute_attr_offset(mp); + ASSERT(igeo->attr_fork_offset < XFS_LITINO(mp)); + + xfs_ialloc_setup_geometry(mp); +} + /* * This function does the following on an initial mount of a file system: * - reads the superblock from disk and init the mount struct @@ -758,7 +770,7 @@ xfs_mountfs( xfs_alloc_compute_maxlevels(mp); xfs_bmap_compute_maxlevels(mp, XFS_DATA_FORK); xfs_bmap_compute_maxlevels(mp, XFS_ATTR_FORK); - xfs_ialloc_setup_geometry(mp); + xfs_mount_setup_inode_geom(mp); xfs_rmapbt_compute_maxlevels(mp); xfs_refcountbt_compute_maxlevels(mp); -- 2.31.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 4/4] xfs: precalculate default inode attribute offset 2021-03-30 5:30 ` [PATCH 4/4] xfs: precalculate default inode attribute offset Dave Chinner @ 2021-03-30 18:10 ` Darrick J. Wong 2021-04-02 7:10 ` Christoph Hellwig 1 sibling, 0 replies; 20+ messages in thread From: Darrick J. Wong @ 2021-03-30 18:10 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-xfs On Tue, Mar 30, 2021 at 04:30:59PM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > Default attr fork offset is based on inode size, so is a fixed > geometry parameter of the inode. Move it to the xfs_ino_geometry > structure and stop calculating it on every call to > xfs_default_attroffset(). > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/libxfs/xfs_bmap.c | 21 ++++++++++----------- > fs/xfs/libxfs/xfs_bmap.h | 1 + > fs/xfs/libxfs/xfs_shared.h | 4 ++++ > fs/xfs/xfs_mount.c | 14 +++++++++++++- > 4 files changed, 28 insertions(+), 12 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > index 16c8730c140f..7930f11e4c54 100644 > --- a/fs/xfs/libxfs/xfs_bmap.c > +++ b/fs/xfs/libxfs/xfs_bmap.c > @@ -94,6 +94,15 @@ xfs_bmap_compute_maxlevels( > mp->m_bm_maxlevels[whichfork] = level; > } > > +unsigned int > +xfs_bmap_compute_attr_offset( > + struct xfs_mount *mp) > +{ > + if (mp->m_sb.sb_inodesize == 256) > + return XFS_LITINO(mp) - XFS_BMDR_SPACE_CALC(MINABTPTRS); > + return XFS_BMDR_SPACE_CALC(6 * MINABTPTRS); > +} > + > STATIC int /* error */ > xfs_bmbt_lookup_eq( > struct xfs_btree_cur *cur, > @@ -192,19 +201,9 @@ uint > xfs_default_attroffset( > struct xfs_inode *ip) > { > - struct xfs_mount *mp = ip->i_mount; > - uint offset; > - > if (ip->i_df.if_format == XFS_DINODE_FMT_DEV) > return roundup(sizeof(xfs_dev_t), 8); > - > - if (mp->m_sb.sb_inodesize == 256) > - offset = XFS_LITINO(mp) - XFS_BMDR_SPACE_CALC(MINABTPTRS); > - else > - offset = XFS_BMDR_SPACE_CALC(6 * MINABTPTRS); > - > - ASSERT(offset < XFS_LITINO(mp)); > - return offset; > + return M_IGEO(ip->i_mount)->attr_fork_offset; > } > > /* > diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h > index 6747e97a7949..a49df4092c30 100644 > --- a/fs/xfs/libxfs/xfs_bmap.h > +++ b/fs/xfs/libxfs/xfs_bmap.h > @@ -185,6 +185,7 @@ static inline bool xfs_bmap_is_written_extent(struct xfs_bmbt_irec *irec) > > void xfs_trim_extent(struct xfs_bmbt_irec *irec, xfs_fileoff_t bno, > xfs_filblks_t len); > +unsigned int xfs_bmap_compute_attr_offset(struct xfs_mount *mp); > int xfs_bmap_add_attrfork(struct xfs_inode *ip, int size, int rsvd); > int xfs_bmap_set_attrforkoff(struct xfs_inode *ip, int size, int *version); > void xfs_bmap_local_to_extents_empty(struct xfs_trans *tp, > diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h > index 8c61a461bf7b..782fdd08f759 100644 > --- a/fs/xfs/libxfs/xfs_shared.h > +++ b/fs/xfs/libxfs/xfs_shared.h > @@ -176,8 +176,12 @@ struct xfs_ino_geometry { > > unsigned int agino_log; /* #bits for agino in inum */ > > + /* precomputed default inode attribute fork offset */ > + unsigned int attr_fork_offset; > + > /* precomputed value for di_flags2 */ > uint64_t new_diflags2; > + > }; > > #endif /* __XFS_SHARED_H__ */ > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c > index 1c97b155a8ee..cb1e2c4702c3 100644 > --- a/fs/xfs/xfs_mount.c > +++ b/fs/xfs/xfs_mount.c > @@ -675,6 +675,18 @@ xfs_unmount_flush_inodes( > xfs_health_unmount(mp); > } > > +static void > +xfs_mount_setup_inode_geom( > + struct xfs_mount *mp) > +{ > + struct xfs_ino_geometry *igeo = M_IGEO(mp); > + > + igeo->attr_fork_offset = xfs_bmap_compute_attr_offset(mp); > + ASSERT(igeo->attr_fork_offset < XFS_LITINO(mp)); Why not put these statements at the bottom of xfs_ialloc_setup_geometry and avoid creating an extra function? --D > + > + xfs_ialloc_setup_geometry(mp); > +} > + > /* > * This function does the following on an initial mount of a file system: > * - reads the superblock from disk and init the mount struct > @@ -758,7 +770,7 @@ xfs_mountfs( > xfs_alloc_compute_maxlevels(mp); > xfs_bmap_compute_maxlevels(mp, XFS_DATA_FORK); > xfs_bmap_compute_maxlevels(mp, XFS_ATTR_FORK); > - xfs_ialloc_setup_geometry(mp); > + xfs_mount_setup_inode_geom(mp); > xfs_rmapbt_compute_maxlevels(mp); > xfs_refcountbt_compute_maxlevels(mp); > > -- > 2.31.0 > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/4] xfs: precalculate default inode attribute offset 2021-03-30 5:30 ` [PATCH 4/4] xfs: precalculate default inode attribute offset Dave Chinner 2021-03-30 18:10 ` Darrick J. Wong @ 2021-04-02 7:10 ` Christoph Hellwig 2021-04-03 22:16 ` Dave Chinner 1 sibling, 1 reply; 20+ messages in thread From: Christoph Hellwig @ 2021-04-02 7:10 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-xfs On Tue, Mar 30, 2021 at 04:30:59PM +1100, Dave Chinner wrote: > +unsigned int > +xfs_bmap_compute_attr_offset( > + struct xfs_mount *mp) > +{ > + if (mp->m_sb.sb_inodesize == 256) > + return XFS_LITINO(mp) - XFS_BMDR_SPACE_CALC(MINABTPTRS); > + return XFS_BMDR_SPACE_CALC(6 * MINABTPTRS); > +} There isn't really anything bmap about this function. Maybe just merge it into xfs_mount_setup_inode_geom? Otherwise looks good: Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/4] xfs: precalculate default inode attribute offset 2021-04-02 7:10 ` Christoph Hellwig @ 2021-04-03 22:16 ` Dave Chinner 0 siblings, 0 replies; 20+ messages in thread From: Dave Chinner @ 2021-04-03 22:16 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-xfs On Fri, Apr 02, 2021 at 08:10:59AM +0100, Christoph Hellwig wrote: > On Tue, Mar 30, 2021 at 04:30:59PM +1100, Dave Chinner wrote: > > +unsigned int > > +xfs_bmap_compute_attr_offset( > > + struct xfs_mount *mp) > > +{ > > + if (mp->m_sb.sb_inodesize == 256) > > + return XFS_LITINO(mp) - XFS_BMDR_SPACE_CALC(MINABTPTRS); > > + return XFS_BMDR_SPACE_CALC(6 * MINABTPTRS); > > +} > > There isn't really anything bmap about this function. Maybe just merge > it into xfs_mount_setup_inode_geom? Sure there is - the XFS_BMDR_SPACE_CALC is a specific bmap btree root size calculation defined in xfs_bmap_btree.h. I left it here because I don't want to include xfs_bmap_btree.h into fs/xfs/xfs_mount.h. There is no reason for xfs_mount.c to know anything about the internals of bmap btrees, similar to how we call xfs_bmap_compute_maxlevels() to calculate the static characteristics of the bmap btrees... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 0/4 v2] xfs: fix eager attr fork init regressions @ 2021-04-06 11:59 Dave Chinner 2021-04-06 11:59 ` [PATCH 4/4] xfs: precalculate default inode attribute offset Dave Chinner 0 siblings, 1 reply; 20+ messages in thread From: Dave Chinner @ 2021-04-06 11:59 UTC (permalink / raw) To: linux-xfs Hi Folks, Update to the fixup patch set first posted here: https://lore.kernel.org/linux-xfs/20210330053059.1339949-1-david@fromorbit.com/ Really the only change is to patch 2, which has had the commit message reworked just to state the problem being fixed, along with a change in implementation that Christoph suggested. This version has passed through fstests on a SELinux enabled test machine without issues. Cheers, Dave. ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/4] xfs: precalculate default inode attribute offset 2021-04-06 11:59 [PATCH 0/4 v2] xfs: fix eager attr fork init regressions Dave Chinner @ 2021-04-06 11:59 ` Dave Chinner 2021-04-06 20:08 ` Allison Henderson 0 siblings, 1 reply; 20+ messages in thread From: Dave Chinner @ 2021-04-06 11:59 UTC (permalink / raw) To: linux-xfs From: Dave Chinner <dchinner@redhat.com> Default attr fork offset is based on inode size, so is a fixed geometry parameter of the inode. Move it to the xfs_ino_geometry structure and stop calculating it on every call to xfs_default_attroffset(). Signed-off-by: Dave Chinner <dchinner@redhat.com> Reviewed-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/libxfs/xfs_bmap.c | 21 ++++++++++----------- fs/xfs/libxfs/xfs_bmap.h | 1 + fs/xfs/libxfs/xfs_shared.h | 4 ++++ fs/xfs/xfs_mount.c | 14 +++++++++++++- 4 files changed, 28 insertions(+), 12 deletions(-) diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c index 414882ebcc8e..f937d3f05bc7 100644 --- a/fs/xfs/libxfs/xfs_bmap.c +++ b/fs/xfs/libxfs/xfs_bmap.c @@ -94,6 +94,15 @@ xfs_bmap_compute_maxlevels( mp->m_bm_maxlevels[whichfork] = level; } +unsigned int +xfs_bmap_compute_attr_offset( + struct xfs_mount *mp) +{ + if (mp->m_sb.sb_inodesize == 256) + return XFS_LITINO(mp) - XFS_BMDR_SPACE_CALC(MINABTPTRS); + return XFS_BMDR_SPACE_CALC(6 * MINABTPTRS); +} + STATIC int /* error */ xfs_bmbt_lookup_eq( struct xfs_btree_cur *cur, @@ -192,19 +201,9 @@ uint xfs_default_attroffset( struct xfs_inode *ip) { - struct xfs_mount *mp = ip->i_mount; - uint offset; - if (ip->i_df.if_format == XFS_DINODE_FMT_DEV) return roundup(sizeof(xfs_dev_t), 8); - - if (mp->m_sb.sb_inodesize == 256) - offset = XFS_LITINO(mp) - XFS_BMDR_SPACE_CALC(MINABTPTRS); - else - offset = XFS_BMDR_SPACE_CALC(6 * MINABTPTRS); - - ASSERT(offset < XFS_LITINO(mp)); - return offset; + return M_IGEO(ip->i_mount)->attr_fork_offset; } /* diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h index 6747e97a7949..a49df4092c30 100644 --- a/fs/xfs/libxfs/xfs_bmap.h +++ b/fs/xfs/libxfs/xfs_bmap.h @@ -185,6 +185,7 @@ static inline bool xfs_bmap_is_written_extent(struct xfs_bmbt_irec *irec) void xfs_trim_extent(struct xfs_bmbt_irec *irec, xfs_fileoff_t bno, xfs_filblks_t len); +unsigned int xfs_bmap_compute_attr_offset(struct xfs_mount *mp); int xfs_bmap_add_attrfork(struct xfs_inode *ip, int size, int rsvd); int xfs_bmap_set_attrforkoff(struct xfs_inode *ip, int size, int *version); void xfs_bmap_local_to_extents_empty(struct xfs_trans *tp, diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h index 8c61a461bf7b..782fdd08f759 100644 --- a/fs/xfs/libxfs/xfs_shared.h +++ b/fs/xfs/libxfs/xfs_shared.h @@ -176,8 +176,12 @@ struct xfs_ino_geometry { unsigned int agino_log; /* #bits for agino in inum */ + /* precomputed default inode attribute fork offset */ + unsigned int attr_fork_offset; + /* precomputed value for di_flags2 */ uint64_t new_diflags2; + }; #endif /* __XFS_SHARED_H__ */ diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c index 1c97b155a8ee..cb1e2c4702c3 100644 --- a/fs/xfs/xfs_mount.c +++ b/fs/xfs/xfs_mount.c @@ -675,6 +675,18 @@ xfs_unmount_flush_inodes( xfs_health_unmount(mp); } +static void +xfs_mount_setup_inode_geom( + struct xfs_mount *mp) +{ + struct xfs_ino_geometry *igeo = M_IGEO(mp); + + igeo->attr_fork_offset = xfs_bmap_compute_attr_offset(mp); + ASSERT(igeo->attr_fork_offset < XFS_LITINO(mp)); + + xfs_ialloc_setup_geometry(mp); +} + /* * This function does the following on an initial mount of a file system: * - reads the superblock from disk and init the mount struct @@ -758,7 +770,7 @@ xfs_mountfs( xfs_alloc_compute_maxlevels(mp); xfs_bmap_compute_maxlevels(mp, XFS_DATA_FORK); xfs_bmap_compute_maxlevels(mp, XFS_ATTR_FORK); - xfs_ialloc_setup_geometry(mp); + xfs_mount_setup_inode_geom(mp); xfs_rmapbt_compute_maxlevels(mp); xfs_refcountbt_compute_maxlevels(mp); -- 2.31.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 4/4] xfs: precalculate default inode attribute offset 2021-04-06 11:59 ` [PATCH 4/4] xfs: precalculate default inode attribute offset Dave Chinner @ 2021-04-06 20:08 ` Allison Henderson 0 siblings, 0 replies; 20+ messages in thread From: Allison Henderson @ 2021-04-06 20:08 UTC (permalink / raw) To: Dave Chinner, linux-xfs On 4/6/21 4:59 AM, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > Default attr fork offset is based on inode size, so is a fixed > geometry parameter of the inode. Move it to the xfs_ino_geometry > structure and stop calculating it on every call to > xfs_default_attroffset(). > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > Reviewed-by: Christoph Hellwig <hch@lst.de> Ok, makes sense Reviewed-by: Allison Henderson <allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_bmap.c | 21 ++++++++++----------- > fs/xfs/libxfs/xfs_bmap.h | 1 + > fs/xfs/libxfs/xfs_shared.h | 4 ++++ > fs/xfs/xfs_mount.c | 14 +++++++++++++- > 4 files changed, 28 insertions(+), 12 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > index 414882ebcc8e..f937d3f05bc7 100644 > --- a/fs/xfs/libxfs/xfs_bmap.c > +++ b/fs/xfs/libxfs/xfs_bmap.c > @@ -94,6 +94,15 @@ xfs_bmap_compute_maxlevels( > mp->m_bm_maxlevels[whichfork] = level; > } > > +unsigned int > +xfs_bmap_compute_attr_offset( > + struct xfs_mount *mp) > +{ > + if (mp->m_sb.sb_inodesize == 256) > + return XFS_LITINO(mp) - XFS_BMDR_SPACE_CALC(MINABTPTRS); > + return XFS_BMDR_SPACE_CALC(6 * MINABTPTRS); > +} > + > STATIC int /* error */ > xfs_bmbt_lookup_eq( > struct xfs_btree_cur *cur, > @@ -192,19 +201,9 @@ uint > xfs_default_attroffset( > struct xfs_inode *ip) > { > - struct xfs_mount *mp = ip->i_mount; > - uint offset; > - > if (ip->i_df.if_format == XFS_DINODE_FMT_DEV) > return roundup(sizeof(xfs_dev_t), 8); > - > - if (mp->m_sb.sb_inodesize == 256) > - offset = XFS_LITINO(mp) - XFS_BMDR_SPACE_CALC(MINABTPTRS); > - else > - offset = XFS_BMDR_SPACE_CALC(6 * MINABTPTRS); > - > - ASSERT(offset < XFS_LITINO(mp)); > - return offset; > + return M_IGEO(ip->i_mount)->attr_fork_offset; > } > > /* > diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h > index 6747e97a7949..a49df4092c30 100644 > --- a/fs/xfs/libxfs/xfs_bmap.h > +++ b/fs/xfs/libxfs/xfs_bmap.h > @@ -185,6 +185,7 @@ static inline bool xfs_bmap_is_written_extent(struct xfs_bmbt_irec *irec) > > void xfs_trim_extent(struct xfs_bmbt_irec *irec, xfs_fileoff_t bno, > xfs_filblks_t len); > +unsigned int xfs_bmap_compute_attr_offset(struct xfs_mount *mp); > int xfs_bmap_add_attrfork(struct xfs_inode *ip, int size, int rsvd); > int xfs_bmap_set_attrforkoff(struct xfs_inode *ip, int size, int *version); > void xfs_bmap_local_to_extents_empty(struct xfs_trans *tp, > diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h > index 8c61a461bf7b..782fdd08f759 100644 > --- a/fs/xfs/libxfs/xfs_shared.h > +++ b/fs/xfs/libxfs/xfs_shared.h > @@ -176,8 +176,12 @@ struct xfs_ino_geometry { > > unsigned int agino_log; /* #bits for agino in inum */ > > + /* precomputed default inode attribute fork offset */ > + unsigned int attr_fork_offset; > + > /* precomputed value for di_flags2 */ > uint64_t new_diflags2; > + > }; > > #endif /* __XFS_SHARED_H__ */ > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c > index 1c97b155a8ee..cb1e2c4702c3 100644 > --- a/fs/xfs/xfs_mount.c > +++ b/fs/xfs/xfs_mount.c > @@ -675,6 +675,18 @@ xfs_unmount_flush_inodes( > xfs_health_unmount(mp); > } > > +static void > +xfs_mount_setup_inode_geom( > + struct xfs_mount *mp) > +{ > + struct xfs_ino_geometry *igeo = M_IGEO(mp); > + > + igeo->attr_fork_offset = xfs_bmap_compute_attr_offset(mp); > + ASSERT(igeo->attr_fork_offset < XFS_LITINO(mp)); > + > + xfs_ialloc_setup_geometry(mp); > +} > + > /* > * This function does the following on an initial mount of a file system: > * - reads the superblock from disk and init the mount struct > @@ -758,7 +770,7 @@ xfs_mountfs( > xfs_alloc_compute_maxlevels(mp); > xfs_bmap_compute_maxlevels(mp, XFS_DATA_FORK); > xfs_bmap_compute_maxlevels(mp, XFS_ATTR_FORK); > - xfs_ialloc_setup_geometry(mp); > + xfs_mount_setup_inode_geom(mp); > xfs_rmapbt_compute_maxlevels(mp); > xfs_refcountbt_compute_maxlevels(mp); > > ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2021-04-06 20:08 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-30 5:30 [PATCH 0/4] xfs: fix eager attr fork init regressions Dave Chinner 2021-03-30 5:30 ` [PATCH 1/4] xfs: eager inode attr fork init needs attr feature awareness Dave Chinner 2021-03-30 18:10 ` Darrick J. Wong 2021-04-02 6:48 ` Christoph Hellwig 2021-03-30 5:30 ` [PATCH 2/4] xfs: inode fork allocation depends on XFS_IFEXTENT flag Dave Chinner 2021-03-30 18:06 ` Darrick J. Wong 2021-03-30 21:40 ` Dave Chinner 2021-04-02 7:02 ` Christoph Hellwig 2021-04-03 22:21 ` Dave Chinner 2021-04-04 3:25 ` Darrick J. Wong 2021-04-02 7:06 ` Christoph Hellwig 2021-03-30 5:30 ` [PATCH 3/4] xfs: default attr fork size does not handle device inodes Dave Chinner 2021-03-30 18:10 ` Darrick J. Wong 2021-04-02 7:08 ` Christoph Hellwig 2021-03-30 5:30 ` [PATCH 4/4] xfs: precalculate default inode attribute offset Dave Chinner 2021-03-30 18:10 ` Darrick J. Wong 2021-04-02 7:10 ` Christoph Hellwig 2021-04-03 22:16 ` Dave Chinner 2021-04-06 11:59 [PATCH 0/4 v2] xfs: fix eager attr fork init regressions Dave Chinner 2021-04-06 11:59 ` [PATCH 4/4] xfs: precalculate default inode attribute offset Dave Chinner 2021-04-06 20:08 ` Allison Henderson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).