Re: [PATCH v5 5/8] xfs: XFS_ICHGTIME_CREATE is unused

["Darrick J. Wong" <djwong@kernel.org>]

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.

--D

> > > 
> > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > > ---
> > > > >  fs/xfs/libxfs/xfs_shared.h      | 2 --
> > > > >  fs/xfs/libxfs/xfs_trans_inode.c | 2 --
> > > > >  2 files changed, 4 deletions(-)
> > > > > 
> > > > > diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
> > > > > index c4381388c0c1..8989fff21723 100644
> > > > > --- a/fs/xfs/libxfs/xfs_shared.h
> > > > > +++ b/fs/xfs/libxfs/xfs_shared.h
> > > > > @@ -126,8 +126,6 @@ void	xfs_log_get_max_trans_res(struct xfs_mount *mp,
> > > > >   */
> > > > >  #define	XFS_ICHGTIME_MOD	0x1	/* data fork modification timestamp */
> > > > >  #define	XFS_ICHGTIME_CHG	0x2	/* inode field change timestamp */
> > > > > -#define	XFS_ICHGTIME_CREATE	0x4	/* inode create timestamp */
> > > > > -
> > > > >  
> > > > >  /*
> > > > >   * Symlink decoding/encoding functions
> > > > > diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> > > > > index 6b2296ff248a..0c9df8df6d4a 100644
> > > > > --- a/fs/xfs/libxfs/xfs_trans_inode.c
> > > > > +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> > > > > @@ -68,8 +68,6 @@ xfs_trans_ichgtime(
> > > > >  		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;
> > > > >  }
> > > > >  
> > > > >  /*
> > > > > 
> > > > > -- 
> > > > > 2.41.0
> > > > > 
> > > > > 
> > > 
> > > 
> > > -- 
> > > Jeff Layton <jlayton@kernel.org>
> 
> -- 
> Jeff Layton <jlayton@kernel.org>