Looks good:
Reviewed-by: Christoph Hellwig <hch@xxxxxx>
just a few minor style nipicks below:
> - if (!test_and_set_bit(XFS_LI_DIRTY, &ip->i_itemp->ili_item.li_flags) &&
> - IS_I_VERSION(VFS_I(ip))) {
> - if (inode_maybe_inc_iversion(VFS_I(ip), flags & XFS_ILOG_CORE))
> - flags |= XFS_ILOG_CORE;
> + if (!test_and_set_bit(XFS_LI_DIRTY, &iip->ili_item.li_flags) &&
> + IS_I_VERSION(inode)) {
> + if (inode_maybe_inc_iversion(inode, flags & XFS_ILOG_CORE))
> + iversion_flags = XFS_ILOG_CORE;
I find the ordering of the conditionals here weird (and yes I know this
comes from the old code), given that test_and_set_bit is an action
applicable independend of IS_I_VERSION. Maybe reshuffle this to:
if (!test_and_set_bit(XFS_LI_DIRTY, &iip->ili_item.li_flags) {
if (IS_I_VERSION(inode) &&
inode_maybe_inc_iversion(inode, flags & XFS_ILOG_CORE))
iversion_flags = XFS_ILOG_CORE;
}
> - tp->t_flags |= XFS_TRANS_DIRTY;
> + /*
> + * Record the specific change for fdatasync optimisation. This
> + * allows fdatasync to skip log forces for inodes that are only
> + * timestamp dirty. We do this before the change count so that
> + * the core being logged in this case does not impact on fdatasync
> + * behaviour.
> + */
This comment could save a precious line by using up all 80 characters :)
> + flags |= iip->ili_last_fields | iversion_flags;
> + iip->ili_fields |= flags;
> + spin_unlock(&iip->ili_lock);
> }
Maybe something like this:
iip->ili_fields |= (flags | iip->ili_last_fields | iversion_flags);
would make more sense as the flags isn't used anywhere below.
> + spin_lock(&iip->ili_lock);
> iip->ili_last_fields = iip->ili_fields;
> iip->ili_fields = 0;
> iip->ili_fsync_fields = 0;
> + spin_unlock(&iip->ili_lock);
> xfs_trans_ail_copy_lsn(mp->m_ail, &iip->ili_flush_lsn,
> &iip->ili_item.li_lsn);
We have to copies of the exact above code sequence. Maybe it makes
sense to be factored into a little helper?
> + spin_lock(&iip->ili_lock);
> + iip->ili_fields &= ~(XFS_ILOG_AOWNER|XFS_ILOG_DOWNER);
Please add whitespaces around the "|".
