| Message ID | 20220819113450.11885-1-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs: don't bump the i_version on an atime update in xfs_vn_update_time | expand |
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 <david@fromorbit.com> > Cc: NeilBrown <neilb@suse.de> > Cc: Trond Myklebust <trondmy@hammerspace.com> > Cc: David Wysochanski <dwysocha@redhat.com> > 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(-) > > 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 <david@fromorbit.com> > > Cc: NeilBrown <neilb@suse.de> > > Cc: Trond Myklebust <trondmy@hammerspace.com> > > Cc: David Wysochanski <dwysocha@redhat.com> > > 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(-) > > > > 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,
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;
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 <david@fromorbit.com> Cc: NeilBrown <neilb@suse.de> Cc: Trond Myklebust <trondmy@hammerspace.com> Cc: David Wysochanski <dwysocha@redhat.com> 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(-)