[RFC PATCH] xfs: don't bump the i_version on an atime update in xfs_vn_update_time

[RFC PATCH] xfs: don't bump the i_version on an atime update in xfs_vn_update_time

[Jeff Layton]

xfs will update the i_version when updating only the atime value, which
is not desirable for any of the current consumers of i_version. Doing so
leads to unnecessary cache invalidations on NFS and extra measurement
activity in IMA.

Add a new XFS_ILOG_NOIVER flag, and use that to indicate that the
transaction should not update the i_version. Set that value in
xfs_vn_update_time if we're only updating the atime.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/xfs/libxfs/xfs_log_format.h  |  2 +-
 fs/xfs/libxfs/xfs_trans_inode.c |  2 +-
 fs/xfs/xfs_iops.c               | 10 +++++++---
 3 files changed, 9 insertions(+), 5 deletions(-)

Dave,

How about this for an alternate approach? This just explicitly ensures
that we don't bump the i_version on an atime-only update, and seems to
fix the testcase I have.

diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
index b351b9dc6561..866a4c5cf70c 100644
--- a/fs/xfs/libxfs/xfs_log_format.h
+++ b/fs/xfs/libxfs/xfs_log_format.h
@@ -323,7 +323,7 @@ struct xfs_inode_log_format_32 {
 #define	XFS_ILOG_ABROOT	0x100	/* log i_af.i_broot */
 #define XFS_ILOG_DOWNER	0x200	/* change the data fork owner on replay */
 #define XFS_ILOG_AOWNER	0x400	/* change the attr fork owner on replay */
-
+#define XFS_ILOG_NOIVER	0x800	/* don't bump i_version */
 
 /*
  * The timestamps are dirty, but not necessarily anything else in the inode
diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
index 8b5547073379..ffe6d296e7f9 100644
--- a/fs/xfs/libxfs/xfs_trans_inode.c
+++ b/fs/xfs/libxfs/xfs_trans_inode.c
@@ -126,7 +126,7 @@ xfs_trans_log_inode(
 	 * unconditionally.
 	 */
 	if (!test_and_set_bit(XFS_LI_DIRTY, &iip->ili_item.li_flags)) {
-		if (IS_I_VERSION(inode) &&
+		if (!(flags & XFS_ILOG_NOIVER) && IS_I_VERSION(inode) &&
 		    inode_maybe_inc_iversion(inode, flags & XFS_ILOG_CORE))
 			iversion_flags = XFS_ILOG_CORE;
 	}
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 45518b8c613c..54db85a43dfb 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -1021,7 +1021,7 @@ xfs_vn_update_time(
 {
 	struct xfs_inode	*ip = XFS_I(inode);
 	struct xfs_mount	*mp = ip->i_mount;
-	int			log_flags = XFS_ILOG_TIMESTAMP;
+	int			log_flags = XFS_ILOG_TIMESTAMP|XFS_ILOG_NOIVER;
 	struct xfs_trans	*tp;
 	int			error;
 
@@ -1041,10 +1041,14 @@ xfs_vn_update_time(
 		return error;
 
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	if (flags & S_CTIME)
+	if (flags & S_CTIME) {
 		inode->i_ctime = *now;
-	if (flags & S_MTIME)
+		log_flags &= ~XFS_ILOG_NOIVER;
+	}
+	if (flags & S_MTIME) {
 		inode->i_mtime = *now;
+		log_flags &= ~XFS_ILOG_NOIVER;
+	}
 	if (flags & S_ATIME)
 		inode->i_atime = *now;
 
-- 
2.37.2

Re: [RFC PATCH] xfs: don't bump the i_version on an atime update in xfs_vn_update_time

[Darrick J. Wong]

On Wed, Aug 17, 2022 at 09:00:02AM -0400, Jeff Layton wrote:
> xfs will update the i_version when updating only the atime value, which
> is not desirable for any of the current consumers of i_version. Doing so
> leads to unnecessary cache invalidations on NFS and extra measurement
> activity in IMA.
> 
> Add a new XFS_ILOG_NOIVER flag, and use that to indicate that the
> transaction should not update the i_version. Set that value in
> xfs_vn_update_time if we're only updating the atime.

I suppose that would work, since explicit atime updates from futimens()
go through notify_change/setattr and not ->update_time.

--D

> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_log_format.h  |  2 +-
>  fs/xfs/libxfs/xfs_trans_inode.c |  2 +-
>  fs/xfs/xfs_iops.c               | 10 +++++++---
>  3 files changed, 9 insertions(+), 5 deletions(-)
> 
> Dave,
> 
> How about this for an alternate approach? This just explicitly ensures
> that we don't bump the i_version on an atime-only update, and seems to
> fix the testcase I have.
> 
> diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
> index b351b9dc6561..866a4c5cf70c 100644
> --- a/fs/xfs/libxfs/xfs_log_format.h
> +++ b/fs/xfs/libxfs/xfs_log_format.h
> @@ -323,7 +323,7 @@ struct xfs_inode_log_format_32 {
>  #define	XFS_ILOG_ABROOT	0x100	/* log i_af.i_broot */
>  #define XFS_ILOG_DOWNER	0x200	/* change the data fork owner on replay */
>  #define XFS_ILOG_AOWNER	0x400	/* change the attr fork owner on replay */
> -
> +#define XFS_ILOG_NOIVER	0x800	/* don't bump i_version */
>  
>  /*
>   * The timestamps are dirty, but not necessarily anything else in the inode
> diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> index 8b5547073379..ffe6d296e7f9 100644
> --- a/fs/xfs/libxfs/xfs_trans_inode.c
> +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> @@ -126,7 +126,7 @@ xfs_trans_log_inode(
>  	 * unconditionally.
>  	 */
>  	if (!test_and_set_bit(XFS_LI_DIRTY, &iip->ili_item.li_flags)) {
> -		if (IS_I_VERSION(inode) &&
> +		if (!(flags & XFS_ILOG_NOIVER) && IS_I_VERSION(inode) &&
>  		    inode_maybe_inc_iversion(inode, flags & XFS_ILOG_CORE))
>  			iversion_flags = XFS_ILOG_CORE;
>  	}
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 45518b8c613c..54db85a43dfb 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -1021,7 +1021,7 @@ xfs_vn_update_time(
>  {
>  	struct xfs_inode	*ip = XFS_I(inode);
>  	struct xfs_mount	*mp = ip->i_mount;
> -	int			log_flags = XFS_ILOG_TIMESTAMP;
> +	int			log_flags = XFS_ILOG_TIMESTAMP|XFS_ILOG_NOIVER;
>  	struct xfs_trans	*tp;
>  	int			error;
>  
> @@ -1041,10 +1041,14 @@ xfs_vn_update_time(
>  		return error;
>  
>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
> -	if (flags & S_CTIME)
> +	if (flags & S_CTIME) {
>  		inode->i_ctime = *now;
> -	if (flags & S_MTIME)
> +		log_flags &= ~XFS_ILOG_NOIVER;
> +	}
> +	if (flags & S_MTIME) {
>  		inode->i_mtime = *now;
> +		log_flags &= ~XFS_ILOG_NOIVER;
> +	}
>  	if (flags & S_ATIME)
>  		inode->i_atime = *now;
>  
> -- 
> 2.37.2
> 

Re: [RFC PATCH] xfs: don't bump the i_version on an atime update in xfs_vn_update_time

[Dave Chinner]

On Wed, Aug 17, 2022 at 09:00:02AM -0400, Jeff Layton wrote:
> xfs will update the i_version when updating only the atime value, which
> is not desirable for any of the current consumers of i_version. Doing so
> leads to unnecessary cache invalidations on NFS and extra measurement
> activity in IMA.
> 
> Add a new XFS_ILOG_NOIVER flag, and use that to indicate that the
> transaction should not update the i_version. Set that value in
> xfs_vn_update_time if we're only updating the atime.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_log_format.h  |  2 +-
>  fs/xfs/libxfs/xfs_trans_inode.c |  2 +-
>  fs/xfs/xfs_iops.c               | 10 +++++++---
>  3 files changed, 9 insertions(+), 5 deletions(-)
> 
> Dave,
> 
> How about this for an alternate approach? This just explicitly ensures
> that we don't bump the i_version on an atime-only update, and seems to
> fix the testcase I have.

This just duplicates lazytime functionality, only now users
can't opt-in or out.

atime update filtering is a VFS function so behaviour is common
across all filesystems. Deficiencies in VFS filtering behaviour
should not be hacked around by individual filesystems, the VFS
filtering should be fixed.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com