Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <fc87931a003d7a911ba1ab7088e559e6b46f6299.camel@kernel.org>
Subject: Re: [PATCH] xfs: don't bump the i_version on an atime update in
 xfs_vn_update_time
From:   Jeff Layton <jlayton@kernel.org>
To:     NeilBrown <neilb@suse.de>
Cc:     "Darrick J . Wong" <djwong@kernel.org>, linux-xfs@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, linux-nfs@vger.kernel.org,
        linux-integrity@vger.kernel.org,
        Dave Chinner <david@fromorbit.com>,
        Trond Myklebust <trondmy@hammerspace.com>,
        David Wysochanski <dwysocha@redhat.com>
Date:   Mon, 22 Aug 2022 06:40:16 -0400
In-Reply-To: <166112626820.23264.11718948914253988812@noble.neil.brown.name>
References: <20220819113450.11885-1-jlayton@kernel.org>
         <166112626820.23264.11718948914253988812@noble.neil.brown.name>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 (3.44.4-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

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 s=
o
> > leads to unnecessary cache invalidations on NFS and extra measurement
> > activity in IMA.
> >=20
> > 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.
> >=20
> > 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(-)
> >=20
> > diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_for=
mat.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 */
> > =20
> >  /*
> >   * 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 =3D 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 =3D XFS_I(inode);
> >  	struct xfs_mount	*mp =3D ip->i_mount;
> > -	int			log_flags =3D XFS_ILOG_TIMESTAMP;
> > +	int			log_flags =3D XFS_ILOG_TIMESTAMP|XFS_ILOG_NOIVER;
> >  	struct xfs_trans	*tp;
> >  	int			error;
> > =20
> > @@ -1041,10 +1041,14 @@ xfs_vn_update_time(
> >  		return error;
> > =20
> >  	xfs_ilock(ip, XFS_ILOCK_EXCL);
> > -	if (flags & S_CTIME)
> > +	if (flags & S_CTIME) {
> >  		inode->i_ctime =3D *now;
> > -	if (flags & S_MTIME)
> > +		log_flags &=3D ~XFS_ILOG_NOIVER;
> > +	}
> > +	if (flags & S_MTIME) {
> >  		inode->i_mtime =3D *now;
> > +		log_flags &=3D ~XFS_ILOG_NOIVER;
> > +	}
> >  	if (flags & S_ATIME)
> >  		inode->i_atime =3D *now;
>=20
> I think you should also clear XFS_ILOG_NOIVER is S_VERSION is set, but
> otherwise this looks sane to me.
>=20
> Thanks,
> NeilBrown

Good point. I'll spin up a v2.

Thanks,
--=20
Jeff Layton <jlayton@kernel.org>