On Wed, 2024-02-28 at 15:28 +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> The redefinition of how NFS wants inode->i_version to be updated is
> incomaptible with the XFS i_version mechanism. The VFS now wants
> inode->i_version to only change when ctime changes (i.e. it has
> become a ctime change counter, not an inode change counter). XFS has
> fine grained timestamps, so it can just use ctime for the NFS change
> cookie like it still does for V4 XFS filesystems.
>
Are you saying that XFS has timestamp granularity finer than
current_time() reports? I thought XFS used the same clocksource as
everyone else.
At LPC, you mentioned you had some patches in progress to use the unused
bits in the tv_nsec field as a change counter to track changes that
occurred within the same timer tick.
Did that not pan out for some reason? I'd like to understand why if so.
It sounded like a reasonable solution to the problem.
>
> We still want XFS to update the inode change counter as it currently
> does, so convert all the code that checks SB_I_VERSION to check for
> v5 format support. Then we can remove the SB_I_VERSION flag from the
> VFS superblock to indicate that inode->i_version is not a valid
> change counter and should not be used as such.
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_trans_inode.c | 15 +++++----------
> fs/xfs/xfs_iops.c | 16 +++-------------
> fs/xfs/xfs_super.c | 8 --------
> 3 files changed, 8 insertions(+), 31 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> index 70e97ea6eee7..8071aefad728 100644
> --- a/fs/xfs/libxfs/xfs_trans_inode.c
> +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> @@ -97,17 +97,12 @@ xfs_trans_log_inode(
>
> /*
> * First time we log the inode in a transaction, bump the inode change
> - * counter if it is configured for this to occur. While we have the
> - * inode locked exclusively for metadata modification, we can usually
> - * avoid setting XFS_ILOG_CORE if no one has queried the value since
> - * the last time it was incremented. If we have XFS_ILOG_CORE already
> - * set however, then go ahead and bump the i_version counter
> - * unconditionally.
> + * counter if it is configured for this to occur.
> */
> - if (!test_and_set_bit(XFS_LI_DIRTY, &iip->ili_item.li_flags)) {
> - if (IS_I_VERSION(inode) &&
> - inode_maybe_inc_iversion(inode, flags & XFS_ILOG_CORE))
> - flags |= XFS_ILOG_IVERSION;
> + if (!test_and_set_bit(XFS_LI_DIRTY, &iip->ili_item.li_flags) &&
> + xfs_has_crc(ip->i_mount)) {
> + inode->i_version++;
> + flags |= XFS_ILOG_IVERSION;
> }
>
> iip->ili_dirty_flags |= flags;
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index be102fd49560..97e792d9d79a 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -584,11 +584,6 @@ xfs_vn_getattr(
> }
> }
>
> - if ((request_mask & STATX_CHANGE_COOKIE) && IS_I_VERSION(inode)) {
> - stat->change_cookie = inode_query_iversion(inode);
> - stat->result_mask |= STATX_CHANGE_COOKIE;
> - }
> -
> /*
> * Note: If you add another clause to set an attribute flag, please
> * update attributes_mask below.
> @@ -1044,16 +1039,11 @@ xfs_vn_update_time(
> struct timespec64 now;
>
> trace_xfs_update_time(ip);
> + ASSERT(!(flags & S_VERSION));
>
> if (inode->i_sb->s_flags & SB_LAZYTIME) {
> - if (!((flags & S_VERSION) &&
> - inode_maybe_inc_iversion(inode, false))) {
> - generic_update_time(inode, flags);
> - return 0;
> - }
> -
> - /* Capture the iversion update that just occurred */
> - log_flags |= XFS_ILOG_CORE;
> + generic_update_time(inode, flags);
> + return 0;
> }
>
> error = xfs_trans_alloc(mp, &M_RES(mp)->tr_fsyncts, 0, 0, 0, &tp);
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 6ce1e6deb7ec..657ce0423f1d 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1692,10 +1692,6 @@ xfs_fs_fill_super(
>
> set_posix_acl_flag(sb);
>
> - /* version 5 superblocks support inode version counters. */
> - if (xfs_has_crc(mp))
> - sb->s_flags |= SB_I_VERSION;
> -
> if (xfs_has_dax_always(mp)) {
> error = xfs_setup_dax_always(mp);
> if (error)
> @@ -1917,10 +1913,6 @@ xfs_fs_reconfigure(
> int flags = fc->sb_flags;
> int error;
>
> - /* version 5 superblocks always support version counters. */
> - if (xfs_has_crc(mp))
> - fc->sb_flags |= SB_I_VERSION;
> -
> error = xfs_fs_validate_params(new_mp);
> if (error)
> return error;
Acked-by: Jeff Layton <jlayton@xxxxxxxxxx>
