Re: Adding attr, inode reference query

[Timothy Shimmin <tes@sgi.com>]

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