[PATCH v2 0/2] xfs: drop SB_I_VERSION support

[PATCH v2 0/2] xfs: drop SB_I_VERSION support

[Dave Chinner]

Hi folks,

This is a followup to my original patch to drop SB_I_VERSION support
that adds a second patch to clean up all the remaining usage of
inode->i_version and completely internalise the inode change counter
as Darrick requested. I haven't seen any issues or failures as a
result of this change, and NFS doesn't appear to complain or
misbehave over the reversion to ctime based change attributes,
either.

-Dave.

[PATCH 1/2] xfs: stop advertising SB_I_VERSION

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

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.

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@redhat.com>
Acked-by: Jeff Layton <jlayton@kernel.org>
---
 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 69fc5b981352..b82f9c7ff2d5 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)) {
+		atomic64_inc(&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 66f8c47642e8..3940ad1ee66e 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.
@@ -1043,16 +1038,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 c21f10ab0f5d..bfc5f3a27451 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1690,10 +1690,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)
@@ -1915,10 +1911,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;
-- 
2.43.0

Re: [PATCH 1/2] xfs: stop advertising SB_I_VERSION

[Darrick J. Wong]

On Tue, Mar 19, 2024 at 09:51:00AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> 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.
> 
> 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@redhat.com>
> Acked-by: Jeff Layton <jlayton@kernel.org>

Looks good to me,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  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 69fc5b981352..b82f9c7ff2d5 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)) {
> +		atomic64_inc(&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 66f8c47642e8..3940ad1ee66e 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.
> @@ -1043,16 +1038,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 c21f10ab0f5d..bfc5f3a27451 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1690,10 +1690,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)
> @@ -1915,10 +1911,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;
> -- 
> 2.43.0
> 
> 

Re: [PATCH 1/2] xfs: stop advertising SB_I_VERSION

[Christoph Hellwig]

A bit sad, but with the curren tstate of affairs it's the right thing
to do:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 2/2] xfs: internalise all remaining i_version support

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Now that we don't support SB_I_VERSION, completely internalise the
remaining usage of inode->i_version. We use our own internal change
counter now, and leave inode->i_version completely unused. This
grows the xfs_inode by 8 bytes, but also allows us to use a normal
uint64_t rather than an expensive atomic64_t for the counter.

This clears the way for implementing different inode->i_version
functionality in the future whilst still maintaining the internal
XFS change counters as they currently stand.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_inode_buf.c   | 7 ++-----
 fs/xfs/libxfs/xfs_trans_inode.c | 5 +----
 fs/xfs/xfs_icache.c             | 4 ----
 fs/xfs/xfs_inode.c              | 4 +---
 fs/xfs/xfs_inode.h              | 1 +
 fs/xfs/xfs_inode_item.c         | 4 +---
 fs/xfs/xfs_iops.c               | 1 -
 7 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index 68989f4bf793..cadd8be83cc4 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -20,8 +20,6 @@
 #include "xfs_dir2.h"
 #include "xfs_health.h"
 
-#include <linux/iversion.h>
-
 /*
  * If we are doing readahead on an inode buffer, we might be in log recovery
  * reading an inode allocation buffer that hasn't yet been replayed, and hence
@@ -244,8 +242,7 @@ xfs_inode_from_disk(
 		xfs_iflags_set(ip, XFS_IPRESERVE_DM_FIELDS);
 
 	if (xfs_has_v3inodes(ip->i_mount)) {
-		inode_set_iversion_queried(inode,
-					   be64_to_cpu(from->di_changecount));
+		ip->i_changecount = be64_to_cpu(from->di_changecount);
 		ip->i_crtime = xfs_inode_from_disk_ts(from, from->di_crtime);
 		ip->i_diflags2 = be64_to_cpu(from->di_flags2);
 		ip->i_cowextsize = be32_to_cpu(from->di_cowextsize);
@@ -339,7 +336,7 @@ xfs_inode_to_disk(
 
 	if (xfs_has_v3inodes(ip->i_mount)) {
 		to->di_version = 3;
-		to->di_changecount = cpu_to_be64(inode_peek_iversion(inode));
+		to->di_changecount = cpu_to_be64(ip->i_changecount);
 		to->di_crtime = xfs_inode_to_disk_ts(ip, ip->i_crtime);
 		to->di_flags2 = cpu_to_be64(ip->i_diflags2);
 		to->di_cowextsize = cpu_to_be32(ip->i_cowextsize);
diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
index b82f9c7ff2d5..f9196eff6bab 100644
--- a/fs/xfs/libxfs/xfs_trans_inode.c
+++ b/fs/xfs/libxfs/xfs_trans_inode.c
@@ -15,8 +15,6 @@
 #include "xfs_trans_priv.h"
 #include "xfs_inode_item.h"
 
-#include <linux/iversion.h>
-
 /*
  * Add a locked inode to the transaction.
  *
@@ -87,7 +85,6 @@ xfs_trans_log_inode(
 	uint			flags)
 {
 	struct xfs_inode_log_item *iip = ip->i_itemp;
-	struct inode		*inode = VFS_I(ip);
 
 	ASSERT(iip);
 	xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
@@ -101,7 +98,7 @@ xfs_trans_log_inode(
 	 */
 	if (!test_and_set_bit(XFS_LI_DIRTY, &iip->ili_item.li_flags) &&
 	    xfs_has_crc(ip->i_mount)) {
-		atomic64_inc(&inode->i_version);
+		ip->i_changecount++;
 		flags |= XFS_ILOG_IVERSION;
 	}
 
diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 74f1812b03cb..6c87b90754c4 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -26,8 +26,6 @@
 #include "xfs_log_priv.h"
 #include "xfs_health.h"
 
-#include <linux/iversion.h>
-
 /* Radix tree tags for incore inode tree. */
 
 /* inode is to be reclaimed */
@@ -309,7 +307,6 @@ xfs_reinit_inode(
 	int			error;
 	uint32_t		nlink = inode->i_nlink;
 	uint32_t		generation = inode->i_generation;
-	uint64_t		version = inode_peek_iversion(inode);
 	umode_t			mode = inode->i_mode;
 	dev_t			dev = inode->i_rdev;
 	kuid_t			uid = inode->i_uid;
@@ -319,7 +316,6 @@ xfs_reinit_inode(
 
 	set_nlink(inode, nlink);
 	inode->i_generation = generation;
-	inode_set_iversion_queried(inode, version);
 	inode->i_mode = mode;
 	inode->i_rdev = dev;
 	inode->i_uid = uid;
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index e7a724270423..3ca8e905dbd4 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -3,8 +3,6 @@
  * Copyright (c) 2000-2006 Silicon Graphics, Inc.
  * All Rights Reserved.
  */
-#include <linux/iversion.h>
-
 #include "xfs.h"
 #include "xfs_fs.h"
 #include "xfs_shared.h"
@@ -828,7 +826,7 @@ xfs_init_new_inode(
 	ip->i_diflags = 0;
 
 	if (xfs_has_v3inodes(mp)) {
-		inode_set_iversion(inode, 1);
+		ip->i_changecount = 1;
 		ip->i_cowextsize = 0;
 		ip->i_crtime = tv;
 	}
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index ab46ffb3ac19..0f9d32cbae72 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -42,6 +42,7 @@ typedef struct xfs_inode {
 	struct rw_semaphore	i_lock;		/* inode lock */
 	atomic_t		i_pincount;	/* inode pin count */
 	struct llist_node	i_gclist;	/* deferred inactivation list */
+	uint64_t		i_changecount;	/* # of attribute changes */
 
 	/*
 	 * Bitsets of inode metadata that have been checked and/or are sick.
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index f28d653300d1..9ec88a84edfa 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -21,8 +21,6 @@
 #include "xfs_error.h"
 #include "xfs_rtbitmap.h"
 
-#include <linux/iversion.h>
-
 struct kmem_cache	*xfs_ili_cache;		/* inode log item */
 
 static inline struct xfs_inode_log_item *INODE_ITEM(struct xfs_log_item *lip)
@@ -546,7 +544,7 @@ xfs_inode_to_log_dinode(
 
 	if (xfs_has_v3inodes(ip->i_mount)) {
 		to->di_version = 3;
-		to->di_changecount = inode_peek_iversion(inode);
+		to->di_changecount = ip->i_changecount;
 		to->di_crtime = xfs_inode_to_log_dinode_ts(ip, ip->i_crtime);
 		to->di_flags2 = ip->i_diflags2;
 		to->di_cowextsize = ip->i_cowextsize;
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 3940ad1ee66e..8a145ca7d380 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -28,7 +28,6 @@
 
 #include <linux/posix_acl.h>
 #include <linux/security.h>
-#include <linux/iversion.h>
 #include <linux/fiemap.h>
 
 /*
-- 
2.43.0

Re: [PATCH 2/2] xfs: internalise all remaining i_version support

[Darrick J. Wong]

On Tue, Mar 19, 2024 at 09:51:01AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Now that we don't support SB_I_VERSION, completely internalise the
> remaining usage of inode->i_version. We use our own internal change
> counter now, and leave inode->i_version completely unused. This
> grows the xfs_inode by 8 bytes, but also allows us to use a normal
> uint64_t rather than an expensive atomic64_t for the counter.
> 
> This clears the way for implementing different inode->i_version
> functionality in the future whilst still maintaining the internal
> XFS change counters as they currently stand.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_inode_buf.c   | 7 ++-----
>  fs/xfs/libxfs/xfs_trans_inode.c | 5 +----
>  fs/xfs/xfs_icache.c             | 4 ----
>  fs/xfs/xfs_inode.c              | 4 +---
>  fs/xfs/xfs_inode.h              | 1 +
>  fs/xfs/xfs_inode_item.c         | 4 +---
>  fs/xfs/xfs_iops.c               | 1 -
>  7 files changed, 6 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 68989f4bf793..cadd8be83cc4 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -20,8 +20,6 @@
>  #include "xfs_dir2.h"
>  #include "xfs_health.h"
>  
> -#include <linux/iversion.h>
> -
>  /*
>   * If we are doing readahead on an inode buffer, we might be in log recovery
>   * reading an inode allocation buffer that hasn't yet been replayed, and hence
> @@ -244,8 +242,7 @@ xfs_inode_from_disk(
>  		xfs_iflags_set(ip, XFS_IPRESERVE_DM_FIELDS);
>  
>  	if (xfs_has_v3inodes(ip->i_mount)) {
> -		inode_set_iversion_queried(inode,
> -					   be64_to_cpu(from->di_changecount));
> +		ip->i_changecount = be64_to_cpu(from->di_changecount);
>  		ip->i_crtime = xfs_inode_from_disk_ts(from, from->di_crtime);
>  		ip->i_diflags2 = be64_to_cpu(from->di_flags2);
>  		ip->i_cowextsize = be32_to_cpu(from->di_cowextsize);
> @@ -339,7 +336,7 @@ xfs_inode_to_disk(
>  
>  	if (xfs_has_v3inodes(ip->i_mount)) {
>  		to->di_version = 3;
> -		to->di_changecount = cpu_to_be64(inode_peek_iversion(inode));
> +		to->di_changecount = cpu_to_be64(ip->i_changecount);
>  		to->di_crtime = xfs_inode_to_disk_ts(ip, ip->i_crtime);
>  		to->di_flags2 = cpu_to_be64(ip->i_diflags2);
>  		to->di_cowextsize = cpu_to_be32(ip->i_cowextsize);
> diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> index b82f9c7ff2d5..f9196eff6bab 100644
> --- a/fs/xfs/libxfs/xfs_trans_inode.c
> +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> @@ -15,8 +15,6 @@
>  #include "xfs_trans_priv.h"
>  #include "xfs_inode_item.h"
>  
> -#include <linux/iversion.h>
> -
>  /*
>   * Add a locked inode to the transaction.
>   *
> @@ -87,7 +85,6 @@ xfs_trans_log_inode(
>  	uint			flags)
>  {
>  	struct xfs_inode_log_item *iip = ip->i_itemp;
> -	struct inode		*inode = VFS_I(ip);
>  
>  	ASSERT(iip);
>  	xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
> @@ -101,7 +98,7 @@ xfs_trans_log_inode(
>  	 */
>  	if (!test_and_set_bit(XFS_LI_DIRTY, &iip->ili_item.li_flags) &&
>  	    xfs_has_crc(ip->i_mount)) {
> -		atomic64_inc(&inode->i_version);
> +		ip->i_changecount++;
>  		flags |= XFS_ILOG_IVERSION;
>  	}
>  
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 74f1812b03cb..6c87b90754c4 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -26,8 +26,6 @@
>  #include "xfs_log_priv.h"
>  #include "xfs_health.h"
>  
> -#include <linux/iversion.h>
> -
>  /* Radix tree tags for incore inode tree. */
>  
>  /* inode is to be reclaimed */
> @@ -309,7 +307,6 @@ xfs_reinit_inode(
>  	int			error;
>  	uint32_t		nlink = inode->i_nlink;
>  	uint32_t		generation = inode->i_generation;
> -	uint64_t		version = inode_peek_iversion(inode);
>  	umode_t			mode = inode->i_mode;
>  	dev_t			dev = inode->i_rdev;
>  	kuid_t			uid = inode->i_uid;
> @@ -319,7 +316,6 @@ xfs_reinit_inode(
>  
>  	set_nlink(inode, nlink);
>  	inode->i_generation = generation;
> -	inode_set_iversion_queried(inode, version);
>  	inode->i_mode = mode;
>  	inode->i_rdev = dev;
>  	inode->i_uid = uid;
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index e7a724270423..3ca8e905dbd4 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3,8 +3,6 @@
>   * Copyright (c) 2000-2006 Silicon Graphics, Inc.
>   * All Rights Reserved.
>   */
> -#include <linux/iversion.h>
> -
>  #include "xfs.h"
>  #include "xfs_fs.h"
>  #include "xfs_shared.h"
> @@ -828,7 +826,7 @@ xfs_init_new_inode(
>  	ip->i_diflags = 0;
>  
>  	if (xfs_has_v3inodes(mp)) {
> -		inode_set_iversion(inode, 1);
> +		ip->i_changecount = 1;
>  		ip->i_cowextsize = 0;
>  		ip->i_crtime = tv;
>  	}
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index ab46ffb3ac19..0f9d32cbae72 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -42,6 +42,7 @@ typedef struct xfs_inode {
>  	struct rw_semaphore	i_lock;		/* inode lock */
>  	atomic_t		i_pincount;	/* inode pin count */
>  	struct llist_node	i_gclist;	/* deferred inactivation list */
> +	uint64_t		i_changecount;	/* # of attribute changes */

Now that we've separated this from i_version, should we export this via
bulkstat or something?

The code itself looks fine so
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

>  
>  	/*
>  	 * Bitsets of inode metadata that have been checked and/or are sick.
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index f28d653300d1..9ec88a84edfa 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -21,8 +21,6 @@
>  #include "xfs_error.h"
>  #include "xfs_rtbitmap.h"
>  
> -#include <linux/iversion.h>
> -
>  struct kmem_cache	*xfs_ili_cache;		/* inode log item */
>  
>  static inline struct xfs_inode_log_item *INODE_ITEM(struct xfs_log_item *lip)
> @@ -546,7 +544,7 @@ xfs_inode_to_log_dinode(
>  
>  	if (xfs_has_v3inodes(ip->i_mount)) {
>  		to->di_version = 3;
> -		to->di_changecount = inode_peek_iversion(inode);
> +		to->di_changecount = ip->i_changecount;
>  		to->di_crtime = xfs_inode_to_log_dinode_ts(ip, ip->i_crtime);
>  		to->di_flags2 = ip->i_diflags2;
>  		to->di_cowextsize = ip->i_cowextsize;
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 3940ad1ee66e..8a145ca7d380 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -28,7 +28,6 @@
>  
>  #include <linux/posix_acl.h>
>  #include <linux/security.h>
> -#include <linux/iversion.h>
>  #include <linux/fiemap.h>
>  
>  /*
> -- 
> 2.43.0
> 
> 

Re: [PATCH 2/2] xfs: internalise all remaining i_version support

[Christoph Hellwig]

Is there much of a good reason to do this now vs oing it whenverer
we actually sort out i_version semantics?  It adds size to the inode
and I don't see how the atomics actually cost us much.  The patch
itself does looks good, though.

Re: [PATCH 2/2] xfs: internalise all remaining i_version support

[Darrick J. Wong]

On Tue, Mar 19, 2024 at 04:06:28PM -0700, Christoph Hellwig wrote:
> Is there much of a good reason to do this now vs oing it whenverer
> we actually sort out i_version semantics?  It adds size to the inode
> and I don't see how the atomics actually cost us much.  The patch
> itself does looks good, though.

I /think/ it's safe because all the callers I found gated their
inode*inc_iversion calls on IS_I_VERSION(), but as di_changecount is now
a totally internal inode attribute, I don't think it should be out where
the VFS can mess with it.

--D

Re: [PATCH 2/2] xfs: internalise all remaining i_version support

[Dave Chinner]

On Tue, Mar 19, 2024 at 04:06:28PM -0700, Christoph Hellwig wrote:
> Is there much of a good reason to do this now vs oing it whenverer
> we actually sort out i_version semantics?  It adds size to the inode
> and I don't see how the atomics actually cost us much.  The patch
> itself does looks good, though.

I'm kinda with Darrick on this - yes, it burns an extra 8 bytes per
inode, but it doesn't leave us exposed to someone misusing the
inode->i_version field. I also don't like the idea of leaving the
disablement half done - that just makes it more work for whoever
comes later to do something with this again...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com