On Mon, Aug 31, 2020 at 05:08:10PM +0100, Christoph Hellwig wrote:
> 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.
Fixed.
> > + 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.
Same here, I'll change it back to:
/*
* 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_sb_version_hasbigtime(&ip->i_mount->m_sb) &&
!xfs_inode_has_bigtime(ip)) {
ip->i_d.di_flags2 |= XFS_DIFLAG2_BIGTIME;
flags |= XFS_ILOG_CORE;
}
I'm not a huge fan of the single-line inode-has-bigtime predicate
either, but at least it's consistent stylistically with the predicate
for the dinode and the log dinode...
Anyway, thanks for the review! Now on to the buffer disposition v2
series....
--D
> Otherwise looks good:
>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
