[PATCH 0/4] xfs: fix eager attr fork init regressions

[PATCH 0/4] xfs: fix eager attr fork init regressions

[Dave Chinner]

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.

[PATCH 1/4] xfs: eager inode attr fork init needs attr feature awareness

[Dave Chinner]

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

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

[Dave Chinner]

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

[PATCH 3/4] xfs: default attr fork size does not handle device inodes

[Dave Chinner]

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

[PATCH 4/4] xfs: precalculate default inode attribute offset

[Dave Chinner]

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

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

[Darrick J. Wong]

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
> 

Re: [PATCH 4/4] xfs: precalculate default inode attribute offset

[Darrick J. Wong]

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
> 

Re: [PATCH 3/4] xfs: default attr fork size does not handle device inodes

[Darrick J. Wong]

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
> 

Re: [PATCH 1/4] xfs: eager inode attr fork init needs attr feature awareness

[Darrick J. Wong]

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
> 

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

[Dave Chinner]

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

Re: [PATCH 1/4] xfs: eager inode attr fork init needs attr feature awareness

[Christoph Hellwig]

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>

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

[Christoph Hellwig]

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.

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

[Christoph Hellwig]

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.

Re: [PATCH 3/4] xfs: default attr fork size does not handle device inodes

[Christoph Hellwig]

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>

Re: [PATCH 4/4] xfs: precalculate default inode attribute offset

[Christoph Hellwig]

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>

Re: [PATCH 4/4] xfs: precalculate default inode attribute offset

[Dave Chinner]

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

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

[Dave Chinner]

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

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

[Darrick J. Wong]

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

Re: [PATCH 4/4] xfs: precalculate default inode attribute offset

[Allison Henderson]

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);
>   
> 

[PATCH 4/4] xfs: precalculate default inode attribute offset

[Dave Chinner]

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