* [PATCH 0/4 v2] xfs: fix eager attr fork init regressions
@ 2021-04-06 11:59 Dave Chinner
2021-04-06 11:59 ` [PATCH 1/4] xfs: eager inode attr fork init needs attr feature awareness Dave Chinner
` (4 more replies)
0 siblings, 5 replies; 17+ 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] 17+ messages in thread
* [PATCH 1/4] xfs: eager inode attr fork init needs attr feature awareness
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 15:40 ` Christoph Hellwig
2021-04-06 20:07 ` Allison Henderson
2021-04-06 11:59 ` [PATCH 2/4] xfs: inode fork allocation depends on XFS_IFEXTENT flag Dave Chinner
` (3 subsequent siblings)
4 siblings, 2 replies; 17+ messages in thread
From: Dave Chinner @ 2021-04-06 11:59 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>
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/4] xfs: inode fork allocation depends on XFS_IFEXTENT flag
2021-04-06 11:59 [PATCH 0/4 v2] xfs: fix eager attr fork init regressions Dave Chinner
2021-04-06 11:59 ` [PATCH 1/4] xfs: eager inode attr fork init needs attr feature awareness Dave Chinner
@ 2021-04-06 11:59 ` Dave Chinner
2021-04-06 13:10 ` Christoph Hellwig
2021-04-06 20:07 ` Allison Henderson
2021-04-06 11:59 ` [PATCH 3/4] xfs: default attr fork size does not handle device inodes Dave Chinner
` (2 subsequent siblings)
4 siblings, 2 replies; 17+ messages in thread
From: Dave Chinner @ 2021-04-06 11:59 UTC (permalink / raw)
To: linux-xfs
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 3/4] xfs: default attr fork size does not handle device inodes
2021-04-06 11:59 [PATCH 0/4 v2] xfs: fix eager attr fork init regressions Dave Chinner
2021-04-06 11:59 ` [PATCH 1/4] xfs: eager inode attr fork init needs attr feature awareness Dave Chinner
2021-04-06 11:59 ` [PATCH 2/4] xfs: inode fork allocation depends on XFS_IFEXTENT flag Dave Chinner
@ 2021-04-06 11:59 ` Dave Chinner
2021-04-06 20:07 ` Allison Henderson
2021-04-06 11:59 ` [PATCH 4/4] xfs: precalculate default inode attribute offset Dave Chinner
2021-04-06 13:48 ` [PATCH 0/4 v2] xfs: fix eager attr fork init regressions Brian Foster
4 siblings, 1 reply; 17+ messages in thread
From: Dave Chinner @ 2021-04-06 11:59 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>
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
^ permalink raw reply related [flat|nested] 17+ 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
` (2 preceding siblings ...)
2021-04-06 11:59 ` [PATCH 3/4] xfs: default attr fork size does not handle device inodes Dave Chinner
@ 2021-04-06 11:59 ` Dave Chinner
2021-04-06 20:08 ` Allison Henderson
2021-04-06 13:48 ` [PATCH 0/4 v2] xfs: fix eager attr fork init regressions Brian Foster
4 siblings, 1 reply; 17+ 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] 17+ messages in thread
* Re: [PATCH 2/4] xfs: inode fork allocation depends on XFS_IFEXTENT flag
2021-04-06 11:59 ` [PATCH 2/4] xfs: inode fork allocation depends on XFS_IFEXTENT flag Dave Chinner
@ 2021-04-06 13:10 ` Christoph Hellwig
2021-04-06 20:07 ` Allison Henderson
1 sibling, 0 replies; 17+ messages in thread
From: Christoph Hellwig @ 2021-04-06 13:10 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
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>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/4 v2] xfs: fix eager attr fork init regressions
2021-04-06 11:59 [PATCH 0/4 v2] xfs: fix eager attr fork init regressions Dave Chinner
` (3 preceding siblings ...)
2021-04-06 11:59 ` [PATCH 4/4] xfs: precalculate default inode attribute offset Dave Chinner
@ 2021-04-06 13:48 ` Brian Foster
4 siblings, 0 replies; 17+ messages in thread
From: Brian Foster @ 2021-04-06 13:48 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
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.
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/4] xfs: eager inode attr fork init needs attr feature awareness
2021-04-06 11:59 ` [PATCH 1/4] xfs: eager inode attr fork init needs attr feature awareness Dave Chinner
@ 2021-04-06 15:40 ` Christoph Hellwig
2021-04-06 21:29 ` Dave Chinner
2021-04-06 20:07 ` Allison Henderson
1 sibling, 1 reply; 17+ messages in thread
From: Christoph Hellwig @ 2021-04-06 15:40 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/4] xfs: eager inode attr fork init needs attr feature awareness
2021-04-06 11:59 ` [PATCH 1/4] xfs: eager inode attr fork init needs attr feature awareness Dave Chinner
2021-04-06 15:40 ` Christoph Hellwig
@ 2021-04-06 20:07 ` Allison Henderson
1 sibling, 0 replies; 17+ messages in thread
From: Allison Henderson @ 2021-04-06 20:07 UTC (permalink / raw)
To: Dave Chinner, linux-xfs
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);
> }
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/4] xfs: inode fork allocation depends on XFS_IFEXTENT flag
2021-04-06 11:59 ` [PATCH 2/4] xfs: inode fork allocation depends on XFS_IFEXTENT flag Dave Chinner
2021-04-06 13:10 ` Christoph Hellwig
@ 2021-04-06 20:07 ` Allison Henderson
1 sibling, 0 replies; 17+ messages in thread
From: Allison Henderson @ 2021-04-06 20:07 UTC (permalink / raw)
To: Dave Chinner, linux-xfs
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;
> }
>
> /*
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/4] xfs: default attr fork size does not handle device inodes
2021-04-06 11:59 ` [PATCH 3/4] xfs: default attr fork size does not handle device inodes Dave Chinner
@ 2021-04-06 20:07 ` Allison Henderson
0 siblings, 0 replies; 17+ messages in thread
From: Allison Henderson @ 2021-04-06 20:07 UTC (permalink / raw)
To: Dave Chinner, linux-xfs
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;
>
^ permalink raw reply [flat|nested] 17+ 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; 17+ 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] 17+ messages in thread
* Re: [PATCH 1/4] xfs: eager inode attr fork init needs attr feature awareness
2021-04-06 15:40 ` Christoph Hellwig
@ 2021-04-06 21:29 ` Dave Chinner
2021-04-07 5:08 ` Christoph Hellwig
0 siblings, 1 reply; 17+ messages in thread
From: Dave Chinner @ 2021-04-06 21:29 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/4] xfs: eager inode attr fork init needs attr feature awareness
2021-04-06 21:29 ` Dave Chinner
@ 2021-04-07 5:08 ` Christoph Hellwig
0 siblings, 0 replies; 17+ messages in thread
From: Christoph Hellwig @ 2021-04-07 5:08 UTC (permalink / raw)
To: Dave Chinner; +Cc: Christoph Hellwig, linux-xfs
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.
^ permalink raw reply [flat|nested] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ messages in thread
* [PATCH 3/4] xfs: default attr fork size does not handle device inodes
2021-03-30 5:30 [PATCH 0/4] " Dave Chinner
@ 2021-03-30 5:30 ` Dave Chinner
2021-03-30 18:10 ` Darrick J. Wong
2021-04-02 7:08 ` Christoph Hellwig
0 siblings, 2 replies; 17+ 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] 17+ messages in thread
end of thread, other threads:[~2021-04-07 5:09 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-06 11:59 [PATCH 0/4 v2] xfs: fix eager attr fork init regressions Dave Chinner
2021-04-06 11:59 ` [PATCH 1/4] xfs: eager inode attr fork init needs attr feature awareness Dave Chinner
2021-04-06 15:40 ` Christoph Hellwig
2021-04-06 21:29 ` Dave Chinner
2021-04-07 5:08 ` Christoph Hellwig
2021-04-06 20:07 ` Allison Henderson
2021-04-06 11:59 ` [PATCH 2/4] xfs: inode fork allocation depends on XFS_IFEXTENT flag Dave Chinner
2021-04-06 13:10 ` Christoph Hellwig
2021-04-06 20:07 ` Allison Henderson
2021-04-06 11:59 ` [PATCH 3/4] xfs: default attr fork size does not handle device inodes Dave Chinner
2021-04-06 20:07 ` Allison Henderson
2021-04-06 11:59 ` [PATCH 4/4] xfs: precalculate default inode attribute offset Dave Chinner
2021-04-06 20:08 ` Allison Henderson
2021-04-06 13:48 ` [PATCH 0/4 v2] xfs: fix eager attr fork init regressions Brian Foster
-- strict thread matches above, loose matches on Subject: below --
2021-03-30 5:30 [PATCH 0/4] " Dave Chinner
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
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.