Re: Adding attr, inode reference query

[Dave Chinner <david@fromorbit.com>]

On Tue, Oct 07, 2008 at 11:04:32AM +1100, Barry Naujok wrote:
> I'm doing a bit of debugging with attr creation in xfs_repair which uses
> libxfs which has it's own simple cache/ref counting/transaction mechanism
> for inodes and buffers.
>
> I came across a refcounting issue when adding an extended attribute to an
> inode, calling xfs_attr_set_int (indirectly in Phase 6):
>   - if there are no extended attributes, a attr fork area is created within
>     the inode (calling xfs_bmap_add_attrfork). After this call in libxfs,
>     the inode is derefenced.
>   - if extended attributes already exist, the inode isn't dereferenced
>     after calling xfs_attr_set_int.
>
> 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.

> It seems most other routines do call xfs_trans_ihold, esp in the attr code.

That's because most of those are in the scope of rolling
transactions, whereas xfs_bmap_add_attrfork() runs a completely
self-contained transaction. Hence we are only concerned about
reference counts to prevent inode reclaim, not continuing to hold
the inode locked for a rolling transaction.

> Also, it seems IHOLD isn't normally called in these routine in the
> core XFS code.
>
> Is this a bug in xfs_bmap_add_attrfork?

No - the inode should exit xfs_bmap_add_attrfork() with the same
reference count it entered with.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com