xfs
[Top] [All Lists]

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

To: Lachlan McIlroy <lachlan@xxxxxxx>
Subject: Re: [PATCH] Unlock inode before calling xfs_idestroy()
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Fri, 19 Sep 2008 03:26:03 -0400
Cc: xfs-oss <xfs@xxxxxxxxxxx>, xfs-dev <xfs-dev@xxxxxxx>
In-reply-to: <48D3193C.6060201@xxxxxxx>
References: <48D3193C.6060201@xxxxxxx>
User-agent: Mutt/1.5.18 (2008-05-17)
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.

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

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