Re: Adding attr, inode reference query

[Timothy Shimmin <tes@sgi.com>]

Timothy Shimmin wrote:
> Dave Chinner wrote:
>> 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.
>>
> 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.
> 
> --Tim

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

--Tim