On Sun, Aug 30, 2020 at 11:07:39PM -0700, Darrick J. Wong wrote:
> +static inline uint64_t xfs_inode_encode_bigtime(struct timespec64 tv)
> +{
> + return (xfs_unix_to_bigtime(tv.tv_sec) * NSEC_PER_SEC) + tv.tv_nsec;
Nit: no need for the braces due to operator precedence.
> + return xfs_sb_version_hasbigtime(&ip->i_mount->m_sb) &&
> + !xfs_inode_has_bigtime(ip);
> +}
> +
> /*
> * This is called to mark the fields indicated in fieldmask as needing to be
> * logged when the transaction is committed. The inode must already be
> @@ -131,6 +137,16 @@ xfs_trans_log_inode(
> iversion_flags = XFS_ILOG_CORE;
> }
>
> + /*
> + * If we're updating the inode core or the timestamps and it's possible
> + * to upgrade this inode to bigtime format, do so now.
> + */
> + if ((flags & (XFS_ILOG_CORE | XFS_ILOG_TIMESTAMP)) &&
> + xfs_inode_want_bigtime_upgrade(ip)) {
> + ip->i_d.di_flags2 |= XFS_DIFLAG2_BIGTIME;
> + flags |= XFS_ILOG_CORE;
> + }
Despite the disagree with Dave I find it very confusing to use
both a direct reference to XFS_DIFLAG2_BIGTIME and one hidden under
two layers of abstraction in the direct same piece of code.
Otherwise looks good:
Reviewed-by: Christoph Hellwig <hch@xxxxxx>
