Unlock inode before calling xfs_idestroy()

PATCH Superseded Lachlan McIlroy 4 participants 7 messages Sep 19, 2008 – Sep 22, 2008 XFS filesystem

lore

Share & tools

lore

Lachlan McIlroy

author

2008-09-19 03:05 UTC #0 lore raw

PATCH 1/1 Unlock inode before calling xfs_idestroy()

Lock debugging reported the ilock was being destroyed without being unlocked.

fs/xfs/xfs_iget.c +0 -0

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

Timothy Shimmin

2008-09-19 04:11 UTC #1 lore raw

Quoted 22 lines · Lachlan McIlroy

Lock debugging reported the ilock was being destroyed without being unlocked.

fs/xfs/xfs_iget.c 2008-09-19 13:12:38.000000000 +1000 renamed from fs/xfs/xfs_iget.c 2008-09-19 13:03:57.000000000 +1000 +2 -0

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

fs/xfs/xfs_iget.c 2008-09-19 13:12:38.000000000 +1000 finish_inode: · line 247

Quoted 7 lines

fs/xfs/xfs_iget.c 2008-09-19 13:12:38.000000000 +1000 renamed from fs/xfs/xfs_iget.c 2008-09-19 13:03:57.000000000 +1000 +1 -0

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

Christoph Hellwig

2008-09-19 07:24 UTC #3 lore raw

In reply to Lachlan McIlroy

Quoted 14 lines · Lachlan McIlroy

Lock debugging reported the ilock was being destroyed without being unlocked.

fs/xfs/xfs_iget.c +1 -0

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

fs/xfs/xfs_iget.c finish_inode: · line 231

Quoted 7 lines

fs/xfs/xfs_iget.c +1 -0

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

fs/xfs/xfs_iget.c finish_inode: · line 247

Quoted 8 lines

fs/xfs/xfs_iget.c +1 -0

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

Dave Chinner

2008-09-20 07:00 UTC #5 lore raw

In reply to Lachlan McIlroy

Quoted 2 lines · Lachlan McIlroy

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.

Signature · 2 lines

Dave Chinner
[email protected]

Lachlan McIlroy

author

2008-09-22 03:42 UTC #2 lore raw

In reply to Timothy Shimmin

Quoted 26 lines · Timothy Shimmin

Quoted 25 lines · Lachlan McIlroy

Lock debugging reported the ilock was being destroyed without being unlocked.

fs/xfs/xfs_iget.c 2008-09-19 13:12:38.000000000 +1000 renamed from fs/xfs/xfs_iget.c 2008-09-19 13:03:57.000000000 +1000 +2 -0

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

Quoted 8 lines

fs/xfs/xfs_iget.c 2008-09-19 13:12:38.000000000 +1000 finish_inode: · line 247
Quoted 4 lines
fs/xfs/xfs_iget.c 2008-09-19 13:12:38.000000000 +1000 renamed from fs/xfs/xfs_iget.c 2008-09-19 13:03:57.000000000 +1000 +1 -0
+        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.

Quoted 34 lines

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

Lachlan McIlroy

author

2008-09-22 03:47 UTC #4 lore raw

In reply to Christoph Hellwig

Quoted 9 lines · Christoph Hellwig

Quoted 8 lines · Lachlan McIlroy
Lock debugging reported the ilock was being destroyed without being unlocked.
fs/xfs/xfs_iget.c +0 -0
@@ -214,6 +214,7 @@ finish_inode:
 		xfs_ilock(ip, lock_flags);
 

Quoted 16 lines

fs/xfs/xfs_iget.c finish_inode: · line 231
Quoted 13 lines
fs/xfs/xfs_iget.c +2 -0
 	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.

Quoted 14 lines

fs/xfs/xfs_iget.c finish_inode: · line 247
Quoted 9 lines
fs/xfs/xfs_iget.c +1 -0
@@ -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.

Lachlan McIlroy

author

2008-09-22 03:49 UTC #6 lore raw

In reply to Dave Chinner

Quoted 9 lines · Dave Chinner

Quoted 2 lines · Lachlan McIlroy
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.