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@xxxxxxxxxx>
> > ---
> > 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 <jlayton@xxxxxxxxxx>
