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

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

[Dave Chinner]

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.

[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>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 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>

Due to confusion on when the XFS_IFEXTENT needs to be set, the
changes in e6a688c33238 ("xfs: initialise attr fork on inode
create") failed to set the flag when initialising the empty
attribute fork at inode creation. Set this flag the same way
xfs_bmap_add_attrfork() does after attry fork allocation.

Fixes: e6a688c33238 ("xfs: initialise attr fork on inode create")
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_inode.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 3b516ab7f22b..6e3fbe25ef57 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -890,6 +890,7 @@ xfs_init_new_inode(
 	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);
+		ip->i_afp->if_flags = XFS_IFEXTENTS;
 	}
 
 	/*
-- 
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>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 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 5574d345d066..414882ebcc8e 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>
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

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

[Christoph Hellwig]

On Tue, Apr 06, 2021 at 09:59:21PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Due to confusion on when the XFS_IFEXTENT needs to be set, the
> changes in e6a688c33238 ("xfs: initialise attr fork on inode
> create") failed to set the flag when initialising the empty
> attribute fork at inode creation. Set this flag the same way
> xfs_bmap_add_attrfork() does after attry fork allocation.
> 
> 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 0/4 v2] xfs: fix eager attr fork init regressions

[Brian Foster]

On Tue, Apr 06, 2021 at 09:59:19PM +1000, Dave Chinner wrote:
> 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.
> 

This series seems to resolve the issues I was hitting previously. FWIW,
for the set:

Tested-by: Brian Foster <bfoster@redhat.com>

> Cheers,
> 
> Dave.
> 

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

[Christoph Hellwig]

On Tue, Apr 06, 2021 at 09:59:20PM +1000, 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.

This doesn't apply to the current xfs/for-next tree to me, with
rejects in xfs_default_attroffset.

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

[Allison Henderson]

On 4/6/21 4:59 AM, 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>
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
Looks ok
Reviewed-by: Allison Henderson <allison.henderson@oracle.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);
>   	}
> 

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

[Allison Henderson]

On 4/6/21 4:59 AM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Due to confusion on when the XFS_IFEXTENT needs to be set, the
> changes in e6a688c33238 ("xfs: initialise attr fork on inode
> create") failed to set the flag when initialising the empty
> attribute fork at inode creation. Set this flag the same way
> xfs_bmap_add_attrfork() does after attry fork allocation.
> 
> Fixes: e6a688c33238 ("xfs: initialise attr fork on inode create")
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
Ok, looks good
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

> ---
>   fs/xfs/xfs_inode.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 3b516ab7f22b..6e3fbe25ef57 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -890,6 +890,7 @@ xfs_init_new_inode(
>   	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);
> +		ip->i_afp->if_flags = XFS_IFEXTENTS;
>   	}
>   
>   	/*
> 

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

[Allison Henderson]

On 4/6/21 4:59 AM, 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>
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
Looks fine
Reviewed-by: Allison Henderson <allison.henderson@oracle.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 5574d345d066..414882ebcc8e 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;
> 

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

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

[Dave Chinner]

On Tue, Apr 06, 2021 at 04:40:16PM +0100, Christoph Hellwig wrote:
> On Tue, Apr 06, 2021 at 09:59:20PM +1000, 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.
> 
> This doesn't apply to the current xfs/for-next tree to me, with
> rejects in xfs_default_attroffset.

Not sure why you'd get rejects in xfs_default_attroffset() given
this patch doesn't change that function at all.

The whole series applies fine here on 5.12-rc6 + xfs/for-next. Head
of the xfs/for-next branch I'm using is commit 25dfa65f8149 ("xfs:
fix xfs_trans slab cache name") which matches the head commit in the
kernel.org tree...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Christoph Hellwig]

On Wed, Apr 07, 2021 at 07:29:05AM +1000, Dave Chinner wrote:
> On Tue, Apr 06, 2021 at 04:40:16PM +0100, Christoph Hellwig wrote:
> > On Tue, Apr 06, 2021 at 09:59:20PM +1000, 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.
> > 
> > This doesn't apply to the current xfs/for-next tree to me, with
> > rejects in xfs_default_attroffset.
> 
> Not sure why you'd get rejects in xfs_default_attroffset() given
> this patch doesn't change that function at all.
> 
> The whole series applies fine here on 5.12-rc6 + xfs/for-next. Head
> of the xfs/for-next branch I'm using is commit 25dfa65f8149 ("xfs:
> fix xfs_trans slab cache name") which matches the head commit in the
> kernel.org tree...

Yes. that's the one I tried to apply it to.  Oh, bloddy git-am trying
to sort by something applies "xfs: precalculate default inode attribute
offset" first.  This has been happening to me a bit lately.  Sorry for
the noise.

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

[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