[PATCH] Unlock inode before calling xfs_idestroy()

[PATCH] Unlock inode before calling xfs_idestroy()

[Lachlan McIlroy]

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

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

[Timothy Shimmin]

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

I'm just wondering about the case where lock_flags==0
and the inode is not locked.
I think it would fail an assert in xfs_iunlock().


--Tim


	if (lock_flags)
		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;
	}

void
xfs_iunlock(
        xfs_inode_t             *ip,
        uint                    lock_flags)
{
        /*
         * You can't set both SHARED and EXCL for the same lock,
         * and only XFS_IOLOCK_SHARED, XFS_IOLOCK_EXCL, XFS_ILOCK_SHARED,
         * and XFS_ILOCK_EXCL are valid values to set in lock_flags.
         */
        ASSERT((lock_flags & (XFS_IOLOCK_SHARED | XFS_IOLOCK_EXCL)) !=
               (XFS_IOLOCK_SHARED | XFS_IOLOCK_EXCL));
        ASSERT((lock_flags & (XFS_ILOCK_SHARED | XFS_ILOCK_EXCL)) !=
               (XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
        ASSERT((lock_flags & ~(XFS_LOCK_MASK | XFS_IUNLOCK_NONOTIFY |
                        XFS_LOCK_DEP_MASK)) == 0);
        ASSERT(lock_flags != 0);

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

[Christoph Hellwig]

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.

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

[Dave Chinner]

On Fri, Sep 19, 2008 at 01:15:08PM +1000, Lachlan McIlroy wrote:
> Lock debugging reported the ilock was being destroyed
> without being unlocked.

Is this going to go in before or after my changes to refactor
this function? If it is before< can you please push it out to
the master git tree ASAP (and all the other outstanding changes)
so I can rediff all the outstanding patches I have before I
send them again?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Lachlan McIlroy]

Timothy Shimmin wrote:
> 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;
>> @@ -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;
> 
> I'm just wondering about the case where lock_flags==0
> and the inode is not locked.
> I think it would fail an assert in xfs_iunlock().
Good catch Tim, thanks.

> 
> 
> --Tim
> 
> 
> 	if (lock_flags)
> 		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;
> 	}
> 
> void
> xfs_iunlock(
>         xfs_inode_t             *ip,
>         uint                    lock_flags)
> {
>         /*
>          * You can't set both SHARED and EXCL for the same lock,
>          * and only XFS_IOLOCK_SHARED, XFS_IOLOCK_EXCL, XFS_ILOCK_SHARED,
>          * and XFS_ILOCK_EXCL are valid values to set in lock_flags.
>          */
>         ASSERT((lock_flags & (XFS_IOLOCK_SHARED | XFS_IOLOCK_EXCL)) !=
>                (XFS_IOLOCK_SHARED | XFS_IOLOCK_EXCL));
>         ASSERT((lock_flags & (XFS_ILOCK_SHARED | XFS_ILOCK_EXCL)) !=
>                (XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
>         ASSERT((lock_flags & ~(XFS_LOCK_MASK | XFS_IUNLOCK_NONOTIFY |
>                         XFS_LOCK_DEP_MASK)) == 0);
>         ASSERT(lock_flags != 0);
> 
> 

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

[Lachlan McIlroy]

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

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

[Lachlan McIlroy]

Dave Chinner 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.
> 
> Is this going to go in before or after my changes to refactor
> this function? If it is before< can you please push it out to
> the master git tree ASAP (and all the other outstanding changes)
> so I can rediff all the outstanding patches I have before I
> send them again?
Yeah sure.  I need to get Christoph's btree patch set in real soon
so I'll do that then update oss/master.