Re: [PATCH] Unlock inode before calling xfs_idestroy()

[Lachlan McIlroy <lachlan@sgi.com>]

Christoph Hellwig wrote:
> On Fri, Sep 19, 2008 at 01:15:08PM +1000, Lachlan McIlroy wrote:
>> Lock debugging reported the ilock was being destroyed
>> without being unlocked.
>>
>> --- a/fs/xfs/xfs_iget.c	2008-09-19 13:03:57.000000000 +1000
>> +++ b/fs/xfs/xfs_iget.c	2008-09-19 13:12:38.000000000 +1000
>> @@ -214,6 +214,7 @@ finish_inode:
>> 		xfs_ilock(ip, lock_flags);
>>
>> 	if ((ip->i_d.di_mode == 0) && !(flags & XFS_IGET_CREATE)) {
>> +		xfs_iunlock(ip, lock_flags);
>> 		xfs_idestroy(ip);
>> 		xfs_put_perag(mp, pag);
>> 		return ENOENT;
>> @@ -224,6 +225,7 @@ finish_inode:
>> 	 * write spinlock.
>> 	 */
>> 	if (radix_tree_preload(GFP_KERNEL)) {
>> +		xfs_iunlock(ip, lock_flags);
>> 		xfs_idestroy(ip);
>> 		delay(1);
>> 		goto again;
> 
> Just move the xfs_ilock call after these two statements, there is no
> need to have it locked before inserting it into the radix tree.
Okay, thanks.  I thought we may need it locked while doing the mode
check so I left it as is.

> 
>> @@ -239,6 +241,7 @@ finish_inode:
>> 		BUG_ON(error != -EEXIST);
>> 		write_unlock(&pag->pag_ici_lock);
>> 		radix_tree_preload_end();
>> +		xfs_iunlock(ip, lock_flags);
>> 		xfs_idestroy(ip);
>> 		XFS_STATS_INC(xs_ig_dup);
>> 		goto again;
> 
> But here we still need the fix.  But as Tim mention we need to check
> for lock_flags != 0 first.  Long-term it might make sense to just make
> xfs_iunlock a no-op if lock_flags == 0, but let's do that separately.
>