| Message ID | 20210324142129.1011766-6-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:16PM +0100, Christoph Hellwig wrote: > The legacy DMAPI fields were never set by upstream Linux XFS, and have no > way to be read using the kernel APIs. So instead of bloating the in-core > inode for them just copy them from the on-disk inode into the log when > logging the inode. The only caveat is that we need to make sure to zero > the fields for newly read or deleted inodes, which is solved using a new > flag in the inode. How long ago /did/ non-upstream XFS have DMAPI support? Does it still have it now? What's the cost of zeroing the fields? (Really what I'm saying is that I have so little clue of what dmevmask and dmstate do that I don't really know what Magic Smoke comes out if these fields get zeroed by an upstream kernel.) > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/libxfs/xfs_inode_buf.c | 7 +++---- > fs/xfs/libxfs/xfs_inode_buf.h | 2 -- > fs/xfs/xfs_inode.c | 5 ++--- > fs/xfs/xfs_inode.h | 1 + > fs/xfs/xfs_inode_item.c | 31 +++++++++++++++++++++++++++++-- > fs/xfs/xfs_log_recover.c | 6 ------ > 6 files changed, 35 insertions(+), 17 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c > index af5ee8bd7e6ac9..062157dcfdfcd3 100644 > --- a/fs/xfs/libxfs/xfs_inode_buf.c > +++ b/fs/xfs/libxfs/xfs_inode_buf.c > @@ -230,10 +230,11 @@ xfs_inode_from_disk( > to->di_nblocks = be64_to_cpu(from->di_nblocks); > to->di_extsize = be32_to_cpu(from->di_extsize); > to->di_forkoff = from->di_forkoff; > - to->di_dmevmask = be32_to_cpu(from->di_dmevmask); > - to->di_dmstate = be16_to_cpu(from->di_dmstate); > to->di_flags = be16_to_cpu(from->di_flags); > > + if (from->di_dmevmask || from->di_dmstate) > + xfs_iflags_set(ip, XFS_IDMAPI); My prefix-happy brain sees this as IDMAP-I, not I-DMAPI. ISAVEDMAPI? No... I-SAVED-MAPI makes no sense... I_PRESERVE_DMAPI? > + > if (xfs_sb_version_has_v3inode(&ip->i_mount->m_sb)) { > inode_set_iversion_queried(inode, > be64_to_cpu(from->di_changecount)); > @@ -311,8 +312,6 @@ xfs_inode_to_disk( > to->di_anextents = cpu_to_be16(xfs_ifork_nextents(ip->i_afp)); > to->di_forkoff = from->di_forkoff; > to->di_aformat = xfs_ifork_format(ip->i_afp); > - to->di_dmevmask = cpu_to_be32(from->di_dmevmask); > - to->di_dmstate = cpu_to_be16(from->di_dmstate); > to->di_flags = cpu_to_be16(from->di_flags); > > if (xfs_sb_version_has_v3inode(&ip->i_mount->m_sb)) { > diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h > index b3097ea8b53366..d7a019df05d647 100644 > --- a/fs/xfs/libxfs/xfs_inode_buf.h > +++ b/fs/xfs/libxfs/xfs_inode_buf.h > @@ -22,8 +22,6 @@ struct xfs_icdinode { > xfs_rfsblock_t di_nblocks; /* # of direct & btree blocks used */ > xfs_extlen_t di_extsize; /* basic/minimum extent size for file */ > uint8_t di_forkoff; /* attr fork offs, <<3 for 64b align */ > - uint32_t di_dmevmask; /* DMIG event mask */ > - uint16_t di_dmstate; /* DMIG state info */ > uint16_t di_flags; /* random flags, XFS_DIFLAG_... */ > > uint64_t di_flags2; /* more random flags */ > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index 093689996f06f3..0954ad0d12ec48 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -839,8 +839,6 @@ xfs_init_new_inode( > inode->i_ctime = tv; > > ip->i_d.di_extsize = 0; > - ip->i_d.di_dmevmask = 0; > - ip->i_d.di_dmstate = 0; > ip->i_d.di_flags = 0; > > if (xfs_sb_version_has_v3inode(&mp->m_sb)) { > @@ -2591,9 +2589,10 @@ xfs_ifree( > VFS_I(ip)->i_mode = 0; /* mark incore inode as free */ > ip->i_d.di_flags = 0; > ip->i_d.di_flags2 = ip->i_mount->m_ino_geo.new_diflags2; > - ip->i_d.di_dmevmask = 0; > ip->i_d.di_forkoff = 0; /* mark the attr fork not in use */ > ip->i_df.if_format = XFS_DINODE_FMT_EXTENTS; > + if (xfs_iflags_test(ip, XFS_IDMAPI)) > + xfs_iflags_clear(ip, XFS_IDMAPI); > > /* Don't attempt to replay owner changes for a deleted inode */ > spin_lock(&iip->ili_lock); > diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h > index 88ee4c3930ae70..e97957f5309281 100644 > --- a/fs/xfs/xfs_inode.h > +++ b/fs/xfs/xfs_inode.h > @@ -212,6 +212,7 @@ static inline bool xfs_inode_has_bigtime(struct xfs_inode *ip) > #define XFS_IRECLAIM (1 << 0) /* started reclaiming this inode */ > #define XFS_ISTALE (1 << 1) /* inode has been staled */ > #define XFS_IRECLAIMABLE (1 << 2) /* inode can be reclaimed */ > +#define XFS_IDMAPI (1 << 4) /* has legacy DMAPI fields set */ > #define __XFS_INEW_BIT 3 /* inode has just been allocated */ > #define XFS_INEW (1 << __XFS_INEW_BIT) > #define XFS_ITRUNCATED (1 << 5) /* truncated down so flush-on-close */ > diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c > index 17e20a6d8b4e27..4c3d1d829753b2 100644 > --- a/fs/xfs/xfs_inode_item.c > +++ b/fs/xfs/xfs_inode_item.c > @@ -317,6 +317,33 @@ xfs_inode_to_log_dinode_ts( > return its; > } > > +/* > + * The legacy DMAPI fields are only present in the on-disk and in-log inodes, > + * but in the in-memory one. But we are guaranteed to have an inode buffer in "but not in" ? --D > + * memory when logging an inode, so we can just copy it from the on-disk inode > + * to the in-log inode here so that recovery of file system with these fields > + * set to non-zero values doesn't lose them. For all other cases we zero the > + * fields. > + */ > +static void > +xfs_copy_dm_fields_to_log_dinode( > + struct xfs_inode *ip, > + struct xfs_log_dinode *to) > +{ > + struct xfs_dinode *dip; > + > + dip = xfs_buf_offset(ip->i_itemp->ili_item.li_buf, > + ip->i_imap.im_boffset); > + > + if (xfs_iflags_test(ip, XFS_IDMAPI)) { > + to->di_dmevmask = be32_to_cpu(dip->di_dmevmask); > + to->di_dmstate = be16_to_cpu(dip->di_dmstate); > + } else { > + to->di_dmevmask = 0; > + to->di_dmstate = 0; > + } > +} > + > static void > xfs_inode_to_log_dinode( > struct xfs_inode *ip, > @@ -349,10 +376,10 @@ xfs_inode_to_log_dinode( > to->di_anextents = xfs_ifork_nextents(ip->i_afp); > to->di_forkoff = from->di_forkoff; > to->di_aformat = xfs_ifork_format(ip->i_afp); > - to->di_dmevmask = from->di_dmevmask; > - to->di_dmstate = from->di_dmstate; > to->di_flags = from->di_flags; > > + xfs_copy_dm_fields_to_log_dinode(ip, to); > + > /* log a dummy value to ensure log structure is fully initialised */ > to->di_next_unlinked = NULLAGINO; > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > index 31348e660cd65b..507c31f6b1a8be 100644 > --- a/fs/xfs/xfs_log_recover.c > +++ b/fs/xfs/xfs_log_recover.c > @@ -2696,12 +2696,6 @@ xlog_recover_process_one_iunlink( > agino = be32_to_cpu(dip->di_next_unlinked); > xfs_buf_relse(ibp); > > - /* > - * Prevent any DMAPI event from being sent when the reference on > - * the inode is dropped. > - */ > - ip->i_d.di_dmevmask = 0; > - > xfs_irele(ip); > return agino; > > -- > 2.30.1 >
On Wed, Mar 24, 2021 at 11:17:59AM -0700, Darrick J. Wong wrote: > On Wed, Mar 24, 2021 at 03:21:16PM +0100, Christoph Hellwig wrote: > > The legacy DMAPI fields were never set by upstream Linux XFS, and have no > > way to be read using the kernel APIs. So instead of bloating the in-core > > inode for them just copy them from the on-disk inode into the log when > > logging the inode. The only caveat is that we need to make sure to zero > > the fields for newly read or deleted inodes, which is solved using a new > > flag in the inode. > > How long ago /did/ non-upstream XFS have DMAPI support? Does it still > have it now? What's the cost of zeroing the fields? > > (Really what I'm saying is that I have so little clue of what dmevmask > and dmstate do that I don't really know what Magic Smoke comes out if > these fields get zeroed by an upstream kernel.) SuSE is shipping SGI patches for it in SLES12, their second newest release. Which is still supported AFAIK.
On Wed, Mar 24, 2021 at 07:20:42PM +0100, Christoph Hellwig wrote: > On Wed, Mar 24, 2021 at 11:17:59AM -0700, Darrick J. Wong wrote: > > On Wed, Mar 24, 2021 at 03:21:16PM +0100, Christoph Hellwig wrote: > > > The legacy DMAPI fields were never set by upstream Linux XFS, and have no > > > way to be read using the kernel APIs. So instead of bloating the in-core > > > inode for them just copy them from the on-disk inode into the log when > > > logging the inode. The only caveat is that we need to make sure to zero > > > the fields for newly read or deleted inodes, which is solved using a new > > > flag in the inode. > > > > How long ago /did/ non-upstream XFS have DMAPI support? Does it still > > have it now? What's the cost of zeroing the fields? > > > > (Really what I'm saying is that I have so little clue of what dmevmask > > and dmstate do that I don't really know what Magic Smoke comes out if > > these fields get zeroed by an upstream kernel.) > > SuSE is shipping SGI patches for it in SLES12, their second newest > release. Which is still supported AFAIK. Indeed SLE12-SP3 was the last release to ship with DMAPI. The general support has ended (although LTSS is still supported till mid-2022). Either way booting/rescue from a non-SLE12 medium isn't supported, and this cleanup is not going to be backported (LTSS only receives critical fixes anyway), so it's probably safe to keep those fields always zeroed in upstream kernels. Perhaps even renaming those fields on xfs_dinode and xfs_log_dinode to unused/pads would make sense, since they could be repurposed. Needs though a couple of cleanups on the xfsprogs side too, so probably outside the scope of this series (I have a bunch of patches to that effect that I need to send sometime). Regards, Anthony
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c index af5ee8bd7e6ac9..062157dcfdfcd3 100644 --- a/fs/xfs/libxfs/xfs_inode_buf.c +++ b/fs/xfs/libxfs/xfs_inode_buf.c @@ -230,10 +230,11 @@ xfs_inode_from_disk( to->di_nblocks = be64_to_cpu(from->di_nblocks); to->di_extsize = be32_to_cpu(from->di_extsize); to->di_forkoff = from->di_forkoff; - to->di_dmevmask = be32_to_cpu(from->di_dmevmask); - to->di_dmstate = be16_to_cpu(from->di_dmstate); to->di_flags = be16_to_cpu(from->di_flags); + if (from->di_dmevmask || from->di_dmstate) + xfs_iflags_set(ip, XFS_IDMAPI); + if (xfs_sb_version_has_v3inode(&ip->i_mount->m_sb)) { inode_set_iversion_queried(inode, be64_to_cpu(from->di_changecount)); @@ -311,8 +312,6 @@ xfs_inode_to_disk( to->di_anextents = cpu_to_be16(xfs_ifork_nextents(ip->i_afp)); to->di_forkoff = from->di_forkoff; to->di_aformat = xfs_ifork_format(ip->i_afp); - to->di_dmevmask = cpu_to_be32(from->di_dmevmask); - to->di_dmstate = cpu_to_be16(from->di_dmstate); to->di_flags = cpu_to_be16(from->di_flags); if (xfs_sb_version_has_v3inode(&ip->i_mount->m_sb)) { diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h index b3097ea8b53366..d7a019df05d647 100644 --- a/fs/xfs/libxfs/xfs_inode_buf.h +++ b/fs/xfs/libxfs/xfs_inode_buf.h @@ -22,8 +22,6 @@ struct xfs_icdinode { xfs_rfsblock_t di_nblocks; /* # of direct & btree blocks used */ xfs_extlen_t di_extsize; /* basic/minimum extent size for file */ uint8_t di_forkoff; /* attr fork offs, <<3 for 64b align */ - uint32_t di_dmevmask; /* DMIG event mask */ - uint16_t di_dmstate; /* DMIG state info */ uint16_t di_flags; /* random flags, XFS_DIFLAG_... */ uint64_t di_flags2; /* more random flags */ diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index 093689996f06f3..0954ad0d12ec48 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -839,8 +839,6 @@ xfs_init_new_inode( inode->i_ctime = tv; ip->i_d.di_extsize = 0; - ip->i_d.di_dmevmask = 0; - ip->i_d.di_dmstate = 0; ip->i_d.di_flags = 0; if (xfs_sb_version_has_v3inode(&mp->m_sb)) { @@ -2591,9 +2589,10 @@ xfs_ifree( VFS_I(ip)->i_mode = 0; /* mark incore inode as free */ ip->i_d.di_flags = 0; ip->i_d.di_flags2 = ip->i_mount->m_ino_geo.new_diflags2; - ip->i_d.di_dmevmask = 0; ip->i_d.di_forkoff = 0; /* mark the attr fork not in use */ ip->i_df.if_format = XFS_DINODE_FMT_EXTENTS; + if (xfs_iflags_test(ip, XFS_IDMAPI)) + xfs_iflags_clear(ip, XFS_IDMAPI); /* Don't attempt to replay owner changes for a deleted inode */ spin_lock(&iip->ili_lock); diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h index 88ee4c3930ae70..e97957f5309281 100644 --- a/fs/xfs/xfs_inode.h +++ b/fs/xfs/xfs_inode.h @@ -212,6 +212,7 @@ static inline bool xfs_inode_has_bigtime(struct xfs_inode *ip) #define XFS_IRECLAIM (1 << 0) /* started reclaiming this inode */ #define XFS_ISTALE (1 << 1) /* inode has been staled */ #define XFS_IRECLAIMABLE (1 << 2) /* inode can be reclaimed */ +#define XFS_IDMAPI (1 << 4) /* has legacy DMAPI fields set */ #define __XFS_INEW_BIT 3 /* inode has just been allocated */ #define XFS_INEW (1 << __XFS_INEW_BIT) #define XFS_ITRUNCATED (1 << 5) /* truncated down so flush-on-close */ diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c index 17e20a6d8b4e27..4c3d1d829753b2 100644 --- a/fs/xfs/xfs_inode_item.c +++ b/fs/xfs/xfs_inode_item.c @@ -317,6 +317,33 @@ xfs_inode_to_log_dinode_ts( return its; } +/* + * The legacy DMAPI fields are only present in the on-disk and in-log inodes, + * but in the in-memory one. But we are guaranteed to have an inode buffer in + * memory when logging an inode, so we can just copy it from the on-disk inode + * to the in-log inode here so that recovery of file system with these fields + * set to non-zero values doesn't lose them. For all other cases we zero the + * fields. + */ +static void +xfs_copy_dm_fields_to_log_dinode( + struct xfs_inode *ip, + struct xfs_log_dinode *to) +{ + struct xfs_dinode *dip; + + dip = xfs_buf_offset(ip->i_itemp->ili_item.li_buf, + ip->i_imap.im_boffset); + + if (xfs_iflags_test(ip, XFS_IDMAPI)) { + to->di_dmevmask = be32_to_cpu(dip->di_dmevmask); + to->di_dmstate = be16_to_cpu(dip->di_dmstate); + } else { + to->di_dmevmask = 0; + to->di_dmstate = 0; + } +} + static void xfs_inode_to_log_dinode( struct xfs_inode *ip, @@ -349,10 +376,10 @@ xfs_inode_to_log_dinode( to->di_anextents = xfs_ifork_nextents(ip->i_afp); to->di_forkoff = from->di_forkoff; to->di_aformat = xfs_ifork_format(ip->i_afp); - to->di_dmevmask = from->di_dmevmask; - to->di_dmstate = from->di_dmstate; to->di_flags = from->di_flags; + xfs_copy_dm_fields_to_log_dinode(ip, to); + /* log a dummy value to ensure log structure is fully initialised */ to->di_next_unlinked = NULLAGINO; diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c index 31348e660cd65b..507c31f6b1a8be 100644 --- a/fs/xfs/xfs_log_recover.c +++ b/fs/xfs/xfs_log_recover.c @@ -2696,12 +2696,6 @@ xlog_recover_process_one_iunlink( agino = be32_to_cpu(dip->di_next_unlinked); xfs_buf_relse(ibp); - /* - * Prevent any DMAPI event from being sent when the reference on - * the inode is dropped. - */ - ip->i_d.di_dmevmask = 0; - xfs_irele(ip); return agino;
The legacy DMAPI fields were never set by upstream Linux XFS, and have no way to be read using the kernel APIs. So instead of bloating the in-core inode for them just copy them from the on-disk inode into the log when logging the inode. The only caveat is that we need to make sure to zero the fields for newly read or deleted inodes, which is solved using a new flag in the inode. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/libxfs/xfs_inode_buf.c | 7 +++---- fs/xfs/libxfs/xfs_inode_buf.h | 2 -- fs/xfs/xfs_inode.c | 5 ++--- fs/xfs/xfs_inode.h | 1 + fs/xfs/xfs_inode_item.c | 31 +++++++++++++++++++++++++++++-- fs/xfs/xfs_log_recover.c | 6 ------ 6 files changed, 35 insertions(+), 17 deletions(-)