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.
Cc: Dave Chinner <[email protected]>
Cc: NeilBrown <[email protected]>
Cc: Trond Myklebust <[email protected]>
Cc: David Wysochanski <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
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(-)
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
On Fri, 19 Aug 2022, 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.
>
> Cc: Dave Chinner <[email protected]>
> Cc: NeilBrown <[email protected]>
> Cc: Trond Myklebust <[email protected]>
> Cc: David Wysochanski <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> 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(-)
>
> 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;
I think you should also clear XFS_ILOG_NOIVER is S_VERSION is set, but
otherwise this looks sane to me.
Thanks,
NeilBrown
>
> --
> 2.37.2
>
>
On Mon, 2022-08-22 at 09:57 +1000, NeilBrown wrote:
> On Fri, 19 Aug 2022, 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.
> >
> > Cc: Dave Chinner <[email protected]>
> > Cc: NeilBrown <[email protected]>
> > Cc: Trond Myklebust <[email protected]>
> > Cc: David Wysochanski <[email protected]>
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > 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(-)
> >
> > 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;
>
> I think you should also clear XFS_ILOG_NOIVER is S_VERSION is set, but
> otherwise this looks sane to me.
>
> Thanks,
> NeilBrown
Good point. I'll spin up a v2.
Thanks,
--
Jeff Layton <[email protected]>