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