Re: [PATCH 06/11] xfs: refactor inode timestamp coding

From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Amir Goldstein <amir73il@gmail.com>
Cc: linux-xfs <linux-xfs@vger.kernel.org>,
	Eric Sandeen <sandeen@sandeen.net>
Subject: Re: [PATCH 06/11] xfs: refactor inode timestamp coding
Date: Tue, 18 Aug 2020 08:42:03 -0700	[thread overview]
Message-ID: <20200818154203.GT6096@magnolia> (raw)
In-Reply-To: <CAOQ4uxhcf1-MKsyenuHnJ5WhpZk7eM53DSmyKYr-EGoSCieTSg@mail.gmail.com>

On Tue, Aug 18, 2020 at 02:20:22PM +0300, Amir Goldstein wrote:
> On Tue, Aug 18, 2020 at 1:57 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> >
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > Refactor inode timestamp encoding and decoding into helper functions so
> > that we can add extra behaviors in subsequent patches.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> 
> With nit below...
> 
> ...
> 
> > diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> > index d95a00376fad..c2f9a0adeed2 100644
> > --- a/fs/xfs/xfs_inode_item.c
> > +++ b/fs/xfs/xfs_inode_item.c
> > @@ -295,6 +295,15 @@ xfs_inode_item_format_attr_fork(
> >         }
> >  }
> >
> > +static inline void
> > +xfs_from_log_timestamp(
> > +       struct xfs_timestamp            *ts,
> > +       const struct xfs_ictimestamp    *its)
> > +{
> > +       ts->t_sec = cpu_to_be32(its->t_sec);
> > +       ts->t_nsec = cpu_to_be32(its->t_nsec);
> > +}
> > +
> >  void
> >  xfs_log_dinode_to_disk(
> 
> Following convention of xfs_inode_{to,from}_disk_timestamp()
> I think it would be less confusing to name these helpers
> xfs_log_to_disk_timestamp()
> 
> and...
> 
> >
> > +static inline void
> > +xfs_to_log_timestamp(
> > +       struct xfs_ictimestamp          *its,
> > +       const struct timespec64         *ts)
> > +{
> > +       its->t_sec = ts->tv_sec;
> > +       its->t_nsec = ts->tv_nsec;
> > +}
> > +
> >  static void
> >  xfs_inode_to_log_dinode(
> 
> xfs_inode_to_log_timestamp()
> 
> Because xfs_{to,from}_log_timestamp() may sound like a matching pair,
> to your average code reviewer, but they are not.

Ok, will do.

--D

> Thanks,
> Amir.