On Tue, Oct 07, 2008 at 12:30:06PM +1100, Timothy Shimmin wrote:
> Timothy Shimmin wrote:
> > Dave Chinner wrote:
> >> On Tue, Oct 07, 2008 at 11:04:32AM +1100, Barry Naujok wrote:
> >>> I seem to have traced this down to xfs_bmap_add_attrfork not calling
> >>> xfs_trans_ihold after calling xfs_trans_ijoin like other similar
> >>> functions.
> >>> BUT, it does call IHOLD(ip).
> >> The difference between the two is kinda subtle. IHOLD() increments
> >> the reference count to ensure the transaction commit doesn't drop
> >> the last reference to the inode when it unlocks it and hence
> >> cause us to enter reclaim in the commit code.
> >>
> >> OTOH, xfs_trans_ihold() holds the inode across the transaction
> >> commit so that it is still locked when xfs_trans_commit() completes.
> >> This is needed for rolling transactions to be able to continue
> >> across duplication and commit without needing to relock inodes.
> >>
> > Oh okay.
> > Want a reference held in both cases, but don't always want it locked
> > after commit.
> > One way, we take an extra reference and then drop it at commit,
> > the other we just don't drop the reference at commit.
>
> This sounds like a very implicit way of doing things IMHO
> (i.e. not clear from the hold that it is about a reference
> being dropped at commit time).
> It almost seems like a different kind of trans-ihold flag
> would have made things clearer (one for unlock, one for rele).
Go look in fs/xfs/xfs_vnodeops.c - every IHOLD is called during a
transaction there is a different reason given, but they all boil
down to one thing - ensuring the transaction commit doesn't
drop the final reference on the inode. e.g. in xfs_link():
2141 /*
2142 * Increment vnode ref counts since xfs_trans_commit &
2143 * xfs_trans_cancel will both unlock the inodes and
2144 * decrement the associated ref counts.
2145 */
2146 IHOLD(sip);
2147 IHOLD(tdp);
2148 xfs_trans_ijoin(tp, sip, XFS_ILOCK_EXCL);
2149 xfs_trans_ijoin(tp, tdp, XFS_ILOCK_EXCL);
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
|