xfs
[Top] [All Lists]

RE: [PATCH] xfs: simplify xfs_trans_iget

To: "Christoph Hellwig" <hch@xxxxxxxxxxxxx>
Subject: RE: [PATCH] xfs: simplify xfs_trans_iget
From: "Alex Elder" <aelder@xxxxxxx>
Date: Fri, 28 Aug 2009 16:58:14 -0500
Cc: <xfs@xxxxxxxxxxx>
In-reply-to: <20090828194243.GA6204@xxxxxxxxxxxxx>
Thread-index: AcooF7gcaViwS7OuRNy9DYcRm31isQAElI8A
Thread-topic: [PATCH] xfs: simplify xfs_trans_iget
Christoph Hellwig wrote:
> 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

That alone answers just about everything...

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

Looks good.  Thanks for the explanation.

Reviewed-by: Alex Elder <aelder@xxxxxxx>

<Prev in Thread] Current Thread [Next in Thread>