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

Amir Goldstein
2020-08-18T11:31:13Z

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.

Thanks,
Amir.