Re: [PATCH] xfs: simplify xfs_trans_iget

[Christoph Hellwig <hch@infradead.org>]

On Fri, Aug 28, 2009 at 12:02:06PM -0500, Alex Elder wrote:
> This function was only ever called as a helper by xfs_trans_iget().
> And it does the same things as xfs_iget() does, but in a more
> restricted  context.  One difference I see is that this verifies
> the transaction pointers match--and if they do not, it returns NULL.
> 
> In xfs_iget(), meanwhile, there is no such check.  I'm not familiar
> enough with the transaction code to know what circumstances that
> might lead a hashed inode without a matching transaction pointer,
> but thought I'd point it out anyway in hopes you maybe did...

I'm always happy about people actually looking deep into the code in
reviews.

Let's got through the above case in detail:

To have i_tansp set the inode must be locked.  So assuming we have an
inode in a transaction that is not your.  We will no go on to the
xfs_iget once the xfs_inode_incore is gone.  But as part of xfs_iget
we will lock the inode exclusively, which means we have to wait
until the other transaction unlocks the inode.  Also if this was
for some reason no true we would immediately hit the ASSERT in
xfs_trans_ijoin which chcks that the i_transp pointer is NULL.

> >  		xfs_inode_item_init(ip, ip->i_mount);
> >  	iip = ip->i_itemp;
> >  	ASSERT(iip->ili_flags == 0);
> > -	ASSERT(iip->ili_ilock_recur == 0);
> > -	ASSERT(iip->ili_iolock_recur == 0);
> 
> These are the only other references, and (assuming we've done
> a lot of testing with ASSERT() enabled) it is evident that
> these are always zero at this point.  In any case, what this
> means is that xfs_trans_ijoin() must never be called after
> xfs_trans_iget() has been called on a hashed inode.  Again,
> I'm not familiar enough with how we manage transactions to
> to verify by inspection this is the case, but I presume it is.

I think this is where the above invariant that an inode in a transaction
must always be locked kicks in again.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs