On Fri, 2023-07-14 at 07:16 -0700, Darrick J. Wong wrote:
> On Fri, Jul 14, 2023 at 06:53:45AM -0400, Jeff Layton wrote:
> > On Thu, 2023-07-13 at 23:35 -0700, Darrick J. Wong wrote:
> > > On Thu, Jul 13, 2023 at 08:15:21PM -0400, Jeff Layton wrote:
> > > > On Fri, 2023-07-14 at 01:53 +0200, Anthony Iliopoulos wrote:
> > > > > On Thu, Jul 13, 2023 at 07:00:54PM -0400, Jeff Layton wrote:
> > > > > > Nothing ever sets this flag, which makes sense since the create time is
> > > > > > set at inode instantiation and is never changed. Remove it and the
> > > > > > handling of it in xfs_trans_ichgtime.
> > > > >
> > > > > It is currently used by xfs_repair during recreating the root inode and
> > > > > the internal realtime inodes when needed (libxfs is exported to xfsprogs
> > > > > so there are userspace consumers of this code).
> > > > >
> > > >
> > > > Ahh thanks. I didn't think to look at userland for this. We can drop
> > > > this patch, and I'll respin #6.
> > > >
> > > > Looking briefly at xfsprogs, it looks like XFS_ICHGTIME_CREATE is never
> > > > set without also setting XFS_ICHGTIME_CHG. Is that safe assumption?
> > >
> > > There are four timestamps in an xfs inode and an ICHGTIME flag for each:
> > > MOD is mtime, CHG is ctime, CREATE is crtime/btime, and ACCESS is atime.
> > > I'd rather leave it that way than tie flags together.
> > >
> > >
> >
> > I wasn't suggesting to tie any flags together. I just don't see any
> > scenario where it's OK to call xfs_trans_ichgtime() without
> > XFS_ICHGTIME_CHG set. It has to change if either of the other times
> > change.
>
> Oh! That's correct, I don't know of any place where the [bam]time get
> updated without also bumping ctime.
>
Great! Thanks for confirming. With that, patch #6 has this delta in it
instead:
diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
index 6b2296ff248a..3ac1fcca7c52 100644
--- a/fs/xfs/libxfs/xfs_trans_inode.c
+++ b/fs/xfs/libxfs/xfs_trans_inode.c
@@ -62,12 +62,12 @@ xfs_trans_ichgtime(
ASSERT(tp);
ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
- tv = current_time(inode);
+ /* If the mtime or btime change, then ctime must also change */
+ WARN_ON_ONCE(!(flags & XFS_ICHGTIME_CHG));
+ tv = inode_set_ctime_current(inode);
if (flags & XFS_ICHGTIME_MOD)
inode->i_mtime = tv;
- if (flags & XFS_ICHGTIME_CHG)
- inode_set_ctime_to_ts(inode, tv);
if (flags & XFS_ICHGTIME_CREATE)
ip->i_crtime = tv;
}
