Adding attr, inode reference query

Adding attr, inode reference query

[Barry Naujok]

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

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

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?

Re: Adding attr, inode reference query

[Timothy Shimmin]

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).
> 
> It seems most other routines do call xfs_trans_ihold, esp in the attr code.
> 
> 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?
Yeah, it looks wrong to me too.

doucette  |1.153|                            |  ASSERT(ip->i_d.di_anextents == 0);
doucette  |1.148|                            |  VN_HOLD(XFS_ITOV(ip));
doucette  |1.146|                            |  xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
doucette  |1.213|                            |  xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);

----------------------------
revision 1.148
date: 1995/05/19 22:41:52;  author: doucette;  state: Exp;  lines: +5 -4
More progress on attributes: fix the transaction reservation in
xfs_bmap_add_attrfork (to be permanent).  Hold the vnode so it won't
go away at transaction commit.
----------------------------

So the xfs_trans_ihold sets a flag on item:
ip->i_itemp->ili_flags |= XFS_ILI_HOLD;
preventing an xfs_iput (unlock, irele) from xfs_inode_item_unlock, from IOP_UNLOCK,
from xfs_trans_unlock_chunk() from xfs_trans_unlock_items() from
_xfs_trans_commit etc...
i.e. from unlocking,unrefing on transaction commit.

So I guess, in current code we are taking an extra reference here instead of just stopping
the inode from being unlocked, and a ref dropped at commit time.

Be interesting to see where we do this throughout the code -> audit.
And why have we normally gotten away with this?

--Tim

Re: Adding attr, inode reference query

[Dave Chinner]

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

Re: Adding attr, inode reference query

[Timothy Shimmin]

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

Re: Adding attr, inode reference query

[Timothy Shimmin]

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

Re: Adding attr, inode reference query

[Dave Chinner]

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@fromorbit.com