xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 3/4] [PATCH 3/4] xfs: remove wrapper for the fsync file operation
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Wed, 17 Feb 2010 03:33:34 -0500
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <20100217040944.GK28392@xxxxxxxxxxxxxxxx>
References: <20100215094445.528696829@xxxxxxxxxxxxxxxxxxxxxx> <20100215094604.640912740@xxxxxxxxxxxxxxxxxxxxxx> <20100217040944.GK28392@xxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.19 (2009-01-05)
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.

<Prev in Thread] Current Thread [Next in Thread>