[PATCH 3/4] [PATCH 3/4] xfs: remove wrapper for the fsync file operation

[Christoph Hellwig]

On Wed, Feb 17, 2010 at 03:09:44PM +1100, Dave Chinner wrote:
> > +		xfs_iunlock(ip, XFS_ILOCK_SHARED);
> > +		if (xfs_ipincount(ip)) {
> > +			if (ip->i_itemp->ili_last_lsn) {
> > +				error = _xfs_log_force_lsn(ip->i_mount,
> > +						ip->i_itemp->ili_last_lsn,
> > +						XFS_LOG_SYNC, &log_flushed);
> > +			} else {
> > +				error = _xfs_log_force(ip->i_mount,
> > +						XFS_LOG_SYNC, &log_flushed);
> > +			}
> > +		}
> 
> To be technically correct, the ilock should be held over the
> pincount check and log force, as is done in xfs_iunpin_wait().
> That way we can guarantee the inode was correctly forced and not
> unpinned between the unlock/check/log force being issued. I know
> this is just a copy of the existing fsync code, but I think that
> the existing code is wrong, too. ;)

Yes, no changes to the code semantics here.  But that ipincount check
is a relatively recent addition from me, too - so thanks for the
slightly delayed review as the comment is absolutely correct.  I'll
prepare a patch for it

> Also, if the inode is pinned while we have it locked, then
> ip->i_itemp->ili_last_lsn is guaranteed to be set as it is updated
> in IOP_COMMITTING() which is called during transaction commit.
> 
> As it is, ili_last_lsn is never reset to zero after a transaction,
> so i think the _xfs_log_force() branch will never be executed,
> either.

True, the same also applies to __xfs_iunpit_wait, I'll look into
that in another patch.  This will also apply to Ben's nfsd
commit_metadata patch.