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