Re: [PATCH 02/24] xfs: add an inode item lock

Christoph Hellwig
2020-05-23T08:45:48Z

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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 "|".