| Message ID | 20210324142129.1011766-14-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [01/18] xfs: split xfs_imap_to_bp | expand |
On Wed, Mar 24, 2021 at 03:21:24PM +0100, Christoph Hellwig wrote: > The i_cowextsize field is only used for v3 inodes, and the i_flushiter > field is only used for v1/v2 inodes. Use a union to pack the inode a > littler better after adding a few missing guards around their usage. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Yes, that's a nice compression opportunity. Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > fs/xfs/libxfs/xfs_inode_buf.c | 3 ++- > fs/xfs/xfs_inode.c | 6 ++++-- > fs/xfs/xfs_inode.h | 7 +++++-- > fs/xfs/xfs_ioctl.c | 6 +++++- > 4 files changed, 16 insertions(+), 6 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c > index d090274fb8a152..96db2649f6b2fe 100644 > --- a/fs/xfs/libxfs/xfs_inode_buf.c > +++ b/fs/xfs/libxfs/xfs_inode_buf.c > @@ -193,7 +193,8 @@ xfs_inode_from_disk( > * inode. If the inode is unused, mode is zero and we shouldn't mess > * with the uninitialized part of it. > */ > - ip->i_flushiter = be16_to_cpu(from->di_flushiter); > + if (!xfs_sb_version_has_v3inode(&ip->i_mount->m_sb)) > + ip->i_flushiter = be16_to_cpu(from->di_flushiter); > inode->i_generation = be32_to_cpu(from->di_gen); > inode->i_mode = be16_to_cpu(from->di_mode); > if (!inode->i_mode) > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index e951ea48b3a276..b4b6fddccd1ca0 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -3459,8 +3459,10 @@ xfs_iflush( > xfs_inode_to_disk(ip, dip, iip->ili_item.li_lsn); > > /* Wrap, we never let the log put out DI_MAX_FLUSH */ > - if (ip->i_flushiter == DI_MAX_FLUSH) > - ip->i_flushiter = 0; > + if (!xfs_sb_version_has_v3inode(&mp->m_sb)) { > + if (ip->i_flushiter == DI_MAX_FLUSH) > + ip->i_flushiter = 0; > + } > > xfs_iflush_fork(ip, dip, iip, XFS_DATA_FORK); > if (XFS_IFORK_Q(ip)) > diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h > index 6246ee8a4359ab..7ba0ffa50ede20 100644 > --- a/fs/xfs/xfs_inode.h > +++ b/fs/xfs/xfs_inode.h > @@ -58,8 +58,11 @@ typedef struct xfs_inode { > xfs_rfsblock_t i_nblocks; /* # of direct & btree blocks */ > uint32_t i_projid; /* owner's project id */ > xfs_extlen_t i_extsize; /* basic/minimum extent size */ > - xfs_extlen_t i_cowextsize; /* basic cow extent size */ > - uint16_t i_flushiter; /* incremented on flush */ > + /* cowextsize is only used for v3 inodes, flushiter for v1/2 */ > + union { > + xfs_extlen_t i_cowextsize; /* basic cow extent size */ > + uint16_t i_flushiter; /* incremented on flush */ > + }; > > struct xfs_icdinode i_d; /* most of ondisk inode */ > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > index e45bce9b11082c..3405a5f5bacfda 100644 > --- a/fs/xfs/xfs_ioctl.c > +++ b/fs/xfs/xfs_ioctl.c > @@ -1121,7 +1121,11 @@ xfs_fill_fsxattr( > > simple_fill_fsxattr(fa, xfs_ip2xflags(ip)); > fa->fsx_extsize = ip->i_extsize << ip->i_mount->m_sb.sb_blocklog; > - fa->fsx_cowextsize = ip->i_cowextsize << ip->i_mount->m_sb.sb_blocklog; > + if (xfs_sb_version_has_v3inode(&ip->i_mount->m_sb) && > + (ip->i_d.di_flags2 & XFS_DIFLAG2_COWEXTSIZE)) { > + fa->fsx_cowextsize = > + ip->i_cowextsize << ip->i_mount->m_sb.sb_blocklog; > + } > fa->fsx_projid = ip->i_projid; > if (ifp && (ifp->if_flags & XFS_IFEXTENTS)) > fa->fsx_nextents = xfs_iext_count(ifp); > -- > 2.30.1 >
On Wed, Mar 24, 2021 at 03:21:24PM +0100, Christoph Hellwig wrote: > The i_cowextsize field is only used for v3 inodes, and the i_flushiter > field is only used for v1/v2 inodes. Use a union to pack the inode a > littler better after adding a few missing guards around their usage. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Hmm, so this patch caused a regression on V4 filesystems xfs/051. It looks like the flush iter gets set to zero and then log recovery forgets to replay the inode(?) The following patch fixes it for me, FWIW... --D xfs: fix regression in xfs/051 with this patch Signed-off-by: Darrick J. Wong <djwong@kernel.org> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index 205ee7da8fa5..795c23e5c655 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -1525,11 +1525,12 @@ xfs_ioctl_setattr( ip->i_extsize = fa->fsx_extsize >> mp->m_sb.sb_blocklog; else ip->i_extsize = 0; - if (xfs_sb_version_has_v3inode(&mp->m_sb) && - (ip->i_d.di_flags2 & XFS_DIFLAG2_COWEXTSIZE)) - ip->i_cowextsize = fa->fsx_cowextsize >> mp->m_sb.sb_blocklog; - else - ip->i_cowextsize = 0; + if (xfs_sb_version_has_v3inode(&mp->m_sb)) { + if (ip->i_d.di_flags2 & XFS_DIFLAG2_COWEXTSIZE) + ip->i_cowextsize = XFS_B_TO_FSBT(mp, fa->fsx_cowextsize); + else + ip->i_cowextsize = 0; + } error = xfs_trans_commit(tp);
On Wed, Mar 24, 2021 at 08:06:28PM -0700, Darrick J. Wong wrote: > On Wed, Mar 24, 2021 at 03:21:24PM +0100, Christoph Hellwig wrote: > > The i_cowextsize field is only used for v3 inodes, and the i_flushiter > > field is only used for v1/v2 inodes. Use a union to pack the inode a > > littler better after adding a few missing guards around their usage. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > Hmm, so this patch caused a regression on V4 filesystems xfs/051. It > looks like the flush iter gets set to zero and then log recovery forgets > to replay the inode(?) > > The following patch fixes it for me, FWIW... Indeed. I've folded something similar in.
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c index d090274fb8a152..96db2649f6b2fe 100644 --- a/fs/xfs/libxfs/xfs_inode_buf.c +++ b/fs/xfs/libxfs/xfs_inode_buf.c @@ -193,7 +193,8 @@ xfs_inode_from_disk( * inode. If the inode is unused, mode is zero and we shouldn't mess * with the uninitialized part of it. */ - ip->i_flushiter = be16_to_cpu(from->di_flushiter); + if (!xfs_sb_version_has_v3inode(&ip->i_mount->m_sb)) + ip->i_flushiter = be16_to_cpu(from->di_flushiter); inode->i_generation = be32_to_cpu(from->di_gen); inode->i_mode = be16_to_cpu(from->di_mode); if (!inode->i_mode) diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index e951ea48b3a276..b4b6fddccd1ca0 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -3459,8 +3459,10 @@ xfs_iflush( xfs_inode_to_disk(ip, dip, iip->ili_item.li_lsn); /* Wrap, we never let the log put out DI_MAX_FLUSH */ - if (ip->i_flushiter == DI_MAX_FLUSH) - ip->i_flushiter = 0; + if (!xfs_sb_version_has_v3inode(&mp->m_sb)) { + if (ip->i_flushiter == DI_MAX_FLUSH) + ip->i_flushiter = 0; + } xfs_iflush_fork(ip, dip, iip, XFS_DATA_FORK); if (XFS_IFORK_Q(ip)) diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h index 6246ee8a4359ab..7ba0ffa50ede20 100644 --- a/fs/xfs/xfs_inode.h +++ b/fs/xfs/xfs_inode.h @@ -58,8 +58,11 @@ typedef struct xfs_inode { xfs_rfsblock_t i_nblocks; /* # of direct & btree blocks */ uint32_t i_projid; /* owner's project id */ xfs_extlen_t i_extsize; /* basic/minimum extent size */ - xfs_extlen_t i_cowextsize; /* basic cow extent size */ - uint16_t i_flushiter; /* incremented on flush */ + /* cowextsize is only used for v3 inodes, flushiter for v1/2 */ + union { + xfs_extlen_t i_cowextsize; /* basic cow extent size */ + uint16_t i_flushiter; /* incremented on flush */ + }; struct xfs_icdinode i_d; /* most of ondisk inode */ diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index e45bce9b11082c..3405a5f5bacfda 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -1121,7 +1121,11 @@ xfs_fill_fsxattr( simple_fill_fsxattr(fa, xfs_ip2xflags(ip)); fa->fsx_extsize = ip->i_extsize << ip->i_mount->m_sb.sb_blocklog; - fa->fsx_cowextsize = ip->i_cowextsize << ip->i_mount->m_sb.sb_blocklog; + if (xfs_sb_version_has_v3inode(&ip->i_mount->m_sb) && + (ip->i_d.di_flags2 & XFS_DIFLAG2_COWEXTSIZE)) { + fa->fsx_cowextsize = + ip->i_cowextsize << ip->i_mount->m_sb.sb_blocklog; + } fa->fsx_projid = ip->i_projid; if (ifp && (ifp->if_flags & XFS_IFEXTENTS)) fa->fsx_nextents = xfs_iext_count(ifp);
The i_cowextsize field is only used for v3 inodes, and the i_flushiter field is only used for v1/v2 inodes. Use a union to pack the inode a littler better after adding a few missing guards around their usage. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/libxfs/xfs_inode_buf.c | 3 ++- fs/xfs/xfs_inode.c | 6 ++++-- fs/xfs/xfs_inode.h | 7 +++++-- fs/xfs/xfs_ioctl.c | 6 +++++- 4 files changed, 16 insertions(+), 6 deletions(-)