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