diff mbox series

[RFC,3/3,RESEND] xfs: mark the inode for high-res timestamp update in getattr

Message ID 20230411143702.64495-4-jlayton@kernel.org (mailing list archive)
State New, archived
Headers show
Series fs: opportunistic high-res file timestamps | expand

Commit Message

Jeff Layton April 11, 2023, 2:37 p.m. UTC
When the mtime or ctime is being queried via getattr, ensure that we
mark the inode for a high-res timestamp update on the next pass. Also,
switch to current_cmtime for other c/mtime updates.

With this change, we're better off having the NFS server just ignore
the i_version field and have it use the ctime instead, so clear the
STATX_CHANGE_COOKIE flag in the result mask in ->getattr.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/xfs/libxfs/xfs_trans_inode.c |  2 +-
 fs/xfs/xfs_acl.c                |  2 +-
 fs/xfs/xfs_inode.c              |  2 +-
 fs/xfs/xfs_iops.c               | 15 ++++++++++++---
 4 files changed, 15 insertions(+), 6 deletions(-)

Comments

Darrick J. Wong April 11, 2023, 2:54 p.m. UTC | #1
On Tue, Apr 11, 2023 at 10:37:02AM -0400, Jeff Layton wrote:
> When the mtime or ctime is being queried via getattr, ensure that we
> mark the inode for a high-res timestamp update on the next pass. Also,
> switch to current_cmtime for other c/mtime updates.
> 
> With this change, we're better off having the NFS server just ignore
> the i_version field and have it use the ctime instead, so clear the
> STATX_CHANGE_COOKIE flag in the result mask in ->getattr.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_trans_inode.c |  2 +-
>  fs/xfs/xfs_acl.c                |  2 +-
>  fs/xfs/xfs_inode.c              |  2 +-
>  fs/xfs/xfs_iops.c               | 15 ++++++++++++---
>  4 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> index 8b5547073379..9ad7c229c617 100644
> --- a/fs/xfs/libxfs/xfs_trans_inode.c
> +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> @@ -63,7 +63,7 @@ xfs_trans_ichgtime(
>  	ASSERT(tp);
>  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>  
> -	tv = current_time(inode);
> +	tv = current_cmtime(inode);
>  
>  	if (flags & XFS_ICHGTIME_MOD)
>  		inode->i_mtime = tv;
> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
> index 791db7d9c849..461adc58cf8c 100644
> --- a/fs/xfs/xfs_acl.c
> +++ b/fs/xfs/xfs_acl.c
> @@ -233,7 +233,7 @@ xfs_acl_set_mode(
>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
>  	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
>  	inode->i_mode = mode;
> -	inode->i_ctime = current_time(inode);
> +	inode->i_ctime = current_cmtime(inode);

Hmm, now we're adding a spinlock to all these updates.
Does lockdep have anything exciting to say about this?

(I don't think it will, just wondering aloud...)

>  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>  
>  	if (xfs_has_wsync(mp))
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 5808abab786c..80f9d731e261 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -843,7 +843,7 @@ xfs_init_new_inode(
>  	ip->i_df.if_nextents = 0;
>  	ASSERT(ip->i_nblocks == 0);
>  
> -	tv = current_time(inode);
> +	tv = current_cmtime(inode);
>  	inode->i_mtime = tv;
>  	inode->i_atime = tv;
>  	inode->i_ctime = tv;
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 24718adb3c16..a0b07f90e16c 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -565,6 +565,15 @@ xfs_vn_getattr(
>  	if (xfs_is_shutdown(mp))
>  		return -EIO;
>  
> +	/*
> +	 * XFS uses the i_version infrastructure to track any change to
> +	 * the inode, including atime updates. This means that the i_version
> +	 * returned by getattr doesn't conform to what the callers expect.
> +	 * Clear it here so that nfsd will fake up a change cookie from the
> +	 * ctime instead.
> +	 */
> +	stat->result_mask &= ~STATX_CHANGE_COOKIE;
> +
>  	stat->size = XFS_ISIZE(ip);
>  	stat->dev = inode->i_sb->s_dev;
>  	stat->mode = inode->i_mode;
> @@ -573,8 +582,8 @@ xfs_vn_getattr(
>  	stat->gid = vfsgid_into_kgid(vfsgid);
>  	stat->ino = ip->i_ino;
>  	stat->atime = inode->i_atime;
> -	stat->mtime = inode->i_mtime;
> -	stat->ctime = inode->i_ctime;
> +	if (request_mask & (STATX_CTIME|STATX_MTIME))
> +		fill_cmtime_and_mark(inode, stat);

Should we be setting STATX_[CM]TIME in the result_mask, just in case the
caller asked for ctime and not mtime?

--D

>  	stat->blocks = XFS_FSB_TO_BB(mp, ip->i_nblocks + ip->i_delayed_blks);
>  
>  	if (xfs_has_v3inodes(mp)) {
> @@ -917,7 +926,7 @@ xfs_setattr_size(
>  	if (newsize != oldsize &&
>  	    !(iattr->ia_valid & (ATTR_CTIME | ATTR_MTIME))) {
>  		iattr->ia_ctime = iattr->ia_mtime =
> -			current_time(inode);
> +			current_cmtime(inode);
>  		iattr->ia_valid |= ATTR_CTIME | ATTR_MTIME;
>  	}
>  
> -- 
> 2.39.2
>
Christian Brauner April 11, 2023, 3:15 p.m. UTC | #2
On Tue, Apr 11, 2023 at 07:54:46AM -0700, Darrick J. Wong wrote:
> On Tue, Apr 11, 2023 at 10:37:02AM -0400, Jeff Layton wrote:
> > When the mtime or ctime is being queried via getattr, ensure that we
> > mark the inode for a high-res timestamp update on the next pass. Also,
> > switch to current_cmtime for other c/mtime updates.
> > 
> > With this change, we're better off having the NFS server just ignore
> > the i_version field and have it use the ctime instead, so clear the
> > STATX_CHANGE_COOKIE flag in the result mask in ->getattr.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/xfs/libxfs/xfs_trans_inode.c |  2 +-
> >  fs/xfs/xfs_acl.c                |  2 +-
> >  fs/xfs/xfs_inode.c              |  2 +-
> >  fs/xfs/xfs_iops.c               | 15 ++++++++++++---
> >  4 files changed, 15 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> > index 8b5547073379..9ad7c229c617 100644
> > --- a/fs/xfs/libxfs/xfs_trans_inode.c
> > +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> > @@ -63,7 +63,7 @@ xfs_trans_ichgtime(
> >  	ASSERT(tp);
> >  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> >  
> > -	tv = current_time(inode);
> > +	tv = current_cmtime(inode);
> >  
> >  	if (flags & XFS_ICHGTIME_MOD)
> >  		inode->i_mtime = tv;
> > diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
> > index 791db7d9c849..461adc58cf8c 100644
> > --- a/fs/xfs/xfs_acl.c
> > +++ b/fs/xfs/xfs_acl.c
> > @@ -233,7 +233,7 @@ xfs_acl_set_mode(
> >  	xfs_ilock(ip, XFS_ILOCK_EXCL);
> >  	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> >  	inode->i_mode = mode;
> > -	inode->i_ctime = current_time(inode);
> > +	inode->i_ctime = current_cmtime(inode);
> 
> Hmm, now we're adding a spinlock to all these updates.
> Does lockdep have anything exciting to say about this?
> 
> (I don't think it will, just wondering aloud...)
> 
> >  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> >  
> >  	if (xfs_has_wsync(mp))
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index 5808abab786c..80f9d731e261 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -843,7 +843,7 @@ xfs_init_new_inode(
> >  	ip->i_df.if_nextents = 0;
> >  	ASSERT(ip->i_nblocks == 0);
> >  
> > -	tv = current_time(inode);
> > +	tv = current_cmtime(inode);
> >  	inode->i_mtime = tv;
> >  	inode->i_atime = tv;
> >  	inode->i_ctime = tv;
> > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > index 24718adb3c16..a0b07f90e16c 100644
> > --- a/fs/xfs/xfs_iops.c
> > +++ b/fs/xfs/xfs_iops.c
> > @@ -565,6 +565,15 @@ xfs_vn_getattr(
> >  	if (xfs_is_shutdown(mp))
> >  		return -EIO;
> >  
> > +	/*
> > +	 * XFS uses the i_version infrastructure to track any change to
> > +	 * the inode, including atime updates. This means that the i_version
> > +	 * returned by getattr doesn't conform to what the callers expect.
> > +	 * Clear it here so that nfsd will fake up a change cookie from the
> > +	 * ctime instead.
> > +	 */
> > +	stat->result_mask &= ~STATX_CHANGE_COOKIE;
> > +
> >  	stat->size = XFS_ISIZE(ip);
> >  	stat->dev = inode->i_sb->s_dev;
> >  	stat->mode = inode->i_mode;
> > @@ -573,8 +582,8 @@ xfs_vn_getattr(
> >  	stat->gid = vfsgid_into_kgid(vfsgid);
> >  	stat->ino = ip->i_ino;
> >  	stat->atime = inode->i_atime;
> > -	stat->mtime = inode->i_mtime;
> > -	stat->ctime = inode->i_ctime;
> > +	if (request_mask & (STATX_CTIME|STATX_MTIME))
> > +		fill_cmtime_and_mark(inode, stat);
> 
> Should we be setting STATX_[CM]TIME in the result_mask, just in case the
> caller asked for ctime and not mtime?

I think the expectation is that everything in STATX_BASIC_MASK is always
returned to allow equivalence between stat() and statx(). So requesting
STATX_CTIME separately from STATX_MTIME isn't implemented widely, maybe
even not at atll?, yet.
Jeff Layton April 11, 2023, 3:58 p.m. UTC | #3
On Tue, 2023-04-11 at 07:54 -0700, Darrick J. Wong wrote:
> On Tue, Apr 11, 2023 at 10:37:02AM -0400, Jeff Layton wrote:
> > When the mtime or ctime is being queried via getattr, ensure that we
> > mark the inode for a high-res timestamp update on the next pass. Also,
> > switch to current_cmtime for other c/mtime updates.
> > 
> > With this change, we're better off having the NFS server just ignore
> > the i_version field and have it use the ctime instead, so clear the
> > STATX_CHANGE_COOKIE flag in the result mask in ->getattr.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/xfs/libxfs/xfs_trans_inode.c |  2 +-
> >  fs/xfs/xfs_acl.c                |  2 +-
> >  fs/xfs/xfs_inode.c              |  2 +-
> >  fs/xfs/xfs_iops.c               | 15 ++++++++++++---
> >  4 files changed, 15 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> > index 8b5547073379..9ad7c229c617 100644
> > --- a/fs/xfs/libxfs/xfs_trans_inode.c
> > +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> > @@ -63,7 +63,7 @@ xfs_trans_ichgtime(
> >  	ASSERT(tp);
> >  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> >  
> > -	tv = current_time(inode);
> > +	tv = current_cmtime(inode);
> >  
> >  	if (flags & XFS_ICHGTIME_MOD)
> >  		inode->i_mtime = tv;
> > diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
> > index 791db7d9c849..461adc58cf8c 100644
> > --- a/fs/xfs/xfs_acl.c
> > +++ b/fs/xfs/xfs_acl.c
> > @@ -233,7 +233,7 @@ xfs_acl_set_mode(
> >  	xfs_ilock(ip, XFS_ILOCK_EXCL);
> >  	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> >  	inode->i_mode = mode;
> > -	inode->i_ctime = current_time(inode);
> > +	inode->i_ctime = current_cmtime(inode);
> 
> Hmm, now we're adding a spinlock to all these updates.
> Does lockdep have anything exciting to say about this?
> 
> (I don't think it will, just wondering aloud...)
> 

I haven't seen anything, but I haven't done any extensive testing yet
either. Still, that might be a good reason to avoid locking in this
scheme if we can.

> >  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> >  
> >  	if (xfs_has_wsync(mp))
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index 5808abab786c..80f9d731e261 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -843,7 +843,7 @@ xfs_init_new_inode(
> >  	ip->i_df.if_nextents = 0;
> >  	ASSERT(ip->i_nblocks == 0);
> >  
> > -	tv = current_time(inode);
> > +	tv = current_cmtime(inode);
> >  	inode->i_mtime = tv;
> >  	inode->i_atime = tv;
> >  	inode->i_ctime = tv;
> > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > index 24718adb3c16..a0b07f90e16c 100644
> > --- a/fs/xfs/xfs_iops.c
> > +++ b/fs/xfs/xfs_iops.c
> > @@ -565,6 +565,15 @@ xfs_vn_getattr(
> >  	if (xfs_is_shutdown(mp))
> >  		return -EIO;
> >  
> > +	/*
> > +	 * XFS uses the i_version infrastructure to track any change to
> > +	 * the inode, including atime updates. This means that the i_version
> > +	 * returned by getattr doesn't conform to what the callers expect.
> > +	 * Clear it here so that nfsd will fake up a change cookie from the
> > +	 * ctime instead.
> > +	 */
> > +	stat->result_mask &= ~STATX_CHANGE_COOKIE;
> > +
> >  	stat->size = XFS_ISIZE(ip);
> >  	stat->dev = inode->i_sb->s_dev;
> >  	stat->mode = inode->i_mode;
> > @@ -573,8 +582,8 @@ xfs_vn_getattr(
> >  	stat->gid = vfsgid_into_kgid(vfsgid);
> >  	stat->ino = ip->i_ino;
> >  	stat->atime = inode->i_atime;
> > -	stat->mtime = inode->i_mtime;
> > -	stat->ctime = inode->i_ctime;
> > +	if (request_mask & (STATX_CTIME|STATX_MTIME))
> > +		fill_cmtime_and_mark(inode, stat);
> 
> Should we be setting STATX_[CM]TIME in the result_mask, just in case the
> caller asked for ctime and not mtime?
> 
> 

We already set the result_mask in vfs_getattr_nosec:

    stat->result_mask |= STATX_BASIC_STATS;

...and it's OK to return more attrs than were requested.

The bigger issue here is that it's not setting the ctime and mtime
fields while leaving the STATX_CTIME and STATX_MTIME flags set. I'll
plan to fix that up in some fashion in the next post.

Thanks!

> >  	stat->blocks = XFS_FSB_TO_BB(mp, ip->i_nblocks + ip->i_delayed_blks);
> >  
> >  	if (xfs_has_v3inodes(mp)) {
> > @@ -917,7 +926,7 @@ xfs_setattr_size(
> >  	if (newsize != oldsize &&
> >  	    !(iattr->ia_valid & (ATTR_CTIME | ATTR_MTIME))) {
> >  		iattr->ia_ctime = iattr->ia_mtime =
> > -			current_time(inode);
> > +			current_cmtime(inode);
> >  		iattr->ia_valid |= ATTR_CTIME | ATTR_MTIME;
> >  	}
> >  
> > -- 
> > 2.39.2
> >
Jeff Layton April 11, 2023, 4:05 p.m. UTC | #4
On Tue, 2023-04-11 at 17:15 +0200, Christian Brauner wrote:
> On Tue, Apr 11, 2023 at 07:54:46AM -0700, Darrick J. Wong wrote:
> > On Tue, Apr 11, 2023 at 10:37:02AM -0400, Jeff Layton wrote:
> > > When the mtime or ctime is being queried via getattr, ensure that we
> > > mark the inode for a high-res timestamp update on the next pass. Also,
> > > switch to current_cmtime for other c/mtime updates.
> > > 
> > > With this change, we're better off having the NFS server just ignore
> > > the i_version field and have it use the ctime instead, so clear the
> > > STATX_CHANGE_COOKIE flag in the result mask in ->getattr.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >  fs/xfs/libxfs/xfs_trans_inode.c |  2 +-
> > >  fs/xfs/xfs_acl.c                |  2 +-
> > >  fs/xfs/xfs_inode.c              |  2 +-
> > >  fs/xfs/xfs_iops.c               | 15 ++++++++++++---
> > >  4 files changed, 15 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> > > index 8b5547073379..9ad7c229c617 100644
> > > --- a/fs/xfs/libxfs/xfs_trans_inode.c
> > > +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> > > @@ -63,7 +63,7 @@ xfs_trans_ichgtime(
> > >  	ASSERT(tp);
> > >  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> > >  
> > > -	tv = current_time(inode);
> > > +	tv = current_cmtime(inode);
> > >  
> > >  	if (flags & XFS_ICHGTIME_MOD)
> > >  		inode->i_mtime = tv;
> > > diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
> > > index 791db7d9c849..461adc58cf8c 100644
> > > --- a/fs/xfs/xfs_acl.c
> > > +++ b/fs/xfs/xfs_acl.c
> > > @@ -233,7 +233,7 @@ xfs_acl_set_mode(
> > >  	xfs_ilock(ip, XFS_ILOCK_EXCL);
> > >  	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> > >  	inode->i_mode = mode;
> > > -	inode->i_ctime = current_time(inode);
> > > +	inode->i_ctime = current_cmtime(inode);
> > 
> > Hmm, now we're adding a spinlock to all these updates.
> > Does lockdep have anything exciting to say about this?
> > 
> > (I don't think it will, just wondering aloud...)
> > 
> > >  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> > >  
> > >  	if (xfs_has_wsync(mp))
> > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > > index 5808abab786c..80f9d731e261 100644
> > > --- a/fs/xfs/xfs_inode.c
> > > +++ b/fs/xfs/xfs_inode.c
> > > @@ -843,7 +843,7 @@ xfs_init_new_inode(
> > >  	ip->i_df.if_nextents = 0;
> > >  	ASSERT(ip->i_nblocks == 0);
> > >  
> > > -	tv = current_time(inode);
> > > +	tv = current_cmtime(inode);
> > >  	inode->i_mtime = tv;
> > >  	inode->i_atime = tv;
> > >  	inode->i_ctime = tv;
> > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > > index 24718adb3c16..a0b07f90e16c 100644
> > > --- a/fs/xfs/xfs_iops.c
> > > +++ b/fs/xfs/xfs_iops.c
> > > @@ -565,6 +565,15 @@ xfs_vn_getattr(
> > >  	if (xfs_is_shutdown(mp))
> > >  		return -EIO;
> > >  
> > > +	/*
> > > +	 * XFS uses the i_version infrastructure to track any change to
> > > +	 * the inode, including atime updates. This means that the i_version
> > > +	 * returned by getattr doesn't conform to what the callers expect.
> > > +	 * Clear it here so that nfsd will fake up a change cookie from the
> > > +	 * ctime instead.
> > > +	 */
> > > +	stat->result_mask &= ~STATX_CHANGE_COOKIE;
> > > +
> > >  	stat->size = XFS_ISIZE(ip);
> > >  	stat->dev = inode->i_sb->s_dev;
> > >  	stat->mode = inode->i_mode;
> > > @@ -573,8 +582,8 @@ xfs_vn_getattr(
> > >  	stat->gid = vfsgid_into_kgid(vfsgid);
> > >  	stat->ino = ip->i_ino;
> > >  	stat->atime = inode->i_atime;
> > > -	stat->mtime = inode->i_mtime;
> > > -	stat->ctime = inode->i_ctime;
> > > +	if (request_mask & (STATX_CTIME|STATX_MTIME))
> > > +		fill_cmtime_and_mark(inode, stat);
> > 
> > Should we be setting STATX_[CM]TIME in the result_mask, just in case the
> > caller asked for ctime and not mtime?
> 
> I think the expectation is that everything in STATX_BASIC_MASK is always
> returned to allow equivalence between stat() and statx(). So requesting
> STATX_CTIME separately from STATX_MTIME isn't implemented widely, maybe
> even not at atll?, yet.

Right. Probably we ought to be more selective with how the result_mask
gets set in vfs_getattr_nosec. Applications that use statx() effectively
are still pretty rare, so most calls will fetch both times anyway.
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
index 8b5547073379..9ad7c229c617 100644
--- a/fs/xfs/libxfs/xfs_trans_inode.c
+++ b/fs/xfs/libxfs/xfs_trans_inode.c
@@ -63,7 +63,7 @@  xfs_trans_ichgtime(
 	ASSERT(tp);
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 
-	tv = current_time(inode);
+	tv = current_cmtime(inode);
 
 	if (flags & XFS_ICHGTIME_MOD)
 		inode->i_mtime = tv;
diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
index 791db7d9c849..461adc58cf8c 100644
--- a/fs/xfs/xfs_acl.c
+++ b/fs/xfs/xfs_acl.c
@@ -233,7 +233,7 @@  xfs_acl_set_mode(
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
 	inode->i_mode = mode;
-	inode->i_ctime = current_time(inode);
+	inode->i_ctime = current_cmtime(inode);
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 
 	if (xfs_has_wsync(mp))
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 5808abab786c..80f9d731e261 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -843,7 +843,7 @@  xfs_init_new_inode(
 	ip->i_df.if_nextents = 0;
 	ASSERT(ip->i_nblocks == 0);
 
-	tv = current_time(inode);
+	tv = current_cmtime(inode);
 	inode->i_mtime = tv;
 	inode->i_atime = tv;
 	inode->i_ctime = tv;
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 24718adb3c16..a0b07f90e16c 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -565,6 +565,15 @@  xfs_vn_getattr(
 	if (xfs_is_shutdown(mp))
 		return -EIO;
 
+	/*
+	 * XFS uses the i_version infrastructure to track any change to
+	 * the inode, including atime updates. This means that the i_version
+	 * returned by getattr doesn't conform to what the callers expect.
+	 * Clear it here so that nfsd will fake up a change cookie from the
+	 * ctime instead.
+	 */
+	stat->result_mask &= ~STATX_CHANGE_COOKIE;
+
 	stat->size = XFS_ISIZE(ip);
 	stat->dev = inode->i_sb->s_dev;
 	stat->mode = inode->i_mode;
@@ -573,8 +582,8 @@  xfs_vn_getattr(
 	stat->gid = vfsgid_into_kgid(vfsgid);
 	stat->ino = ip->i_ino;
 	stat->atime = inode->i_atime;
-	stat->mtime = inode->i_mtime;
-	stat->ctime = inode->i_ctime;
+	if (request_mask & (STATX_CTIME|STATX_MTIME))
+		fill_cmtime_and_mark(inode, stat);
 	stat->blocks = XFS_FSB_TO_BB(mp, ip->i_nblocks + ip->i_delayed_blks);
 
 	if (xfs_has_v3inodes(mp)) {
@@ -917,7 +926,7 @@  xfs_setattr_size(
 	if (newsize != oldsize &&
 	    !(iattr->ia_valid & (ATTR_CTIME | ATTR_MTIME))) {
 		iattr->ia_ctime = iattr->ia_mtime =
-			current_time(inode);
+			current_cmtime(inode);
 		iattr->ia_valid |= ATTR_CTIME | ATTR_MTIME;
 	}