crash with latest code drop.

Hi Dave and list, I hit the following crash with the latest code drop that was pushed in yesterday
while running test 177 in a loop, after 4-5 loops it crashed as follows:
"<0>general protection fault: 0000 [1] SMP"

[1]kdb> bt
Stack traceback for pid 5425
0xffff88007b9b38d0 5425 5242 1 1 R 0xffff88007b9b3c38 *sync
sp ip Function (args)
0xffff88006c45fde8 0xffffffff80320990 radix_tree_tagged+0xa (0x6b6b6b6b6b6b6b73, 0x0)
0xffff88006c45fe10 0xffffffffa01f36c6 [xfs]xfs_sync_inodes_ag+0x197 (0xffff88007d4025c8, invalid, invalid)
0xffff88006c45fe80 0xffffffffa01f38d1 [xfs]xfs_sync_inodes+0x63 (0xffff88007d4025c8, invalid)
0xffff88006c45fec0 0xffffffffa01f3997 [xfs]xfs_quiesce_data+0x13 (0xffff88007d4025c8)
0xffff88006c45fee0 0xffffffffa01f1800 [xfs]xfs_fs_sync_super+0x2b (0xffff88007d9f10b8)
0xffff88006c45ff40 0xffffffff80292fd2 sync_filesystems+0xae (invalid) 0xffff88006c45ff60 0xffffffff802af48b do_sync+0x2f (0x1) 0xffff88006c45ff70 0xffffffff802af4c4 sys_sync+0xe
bb_special_case: Invalid bb_reg_state.memory, missing trailing entries
bb_special_case: on transfer to int_with_check
Assuming system_call_fastpath is 'pass through' with 6 register parameters
kdb_bb: 0xffffffff8020be0b [kernel]system_call_fastpath failed at 0xffffffff8020be98

Using old style backtrace, unreliable with no arguments
sp ip Function (args)
0xffff88006c45fdb0 0xffffffffa01f369a [xfs]xfs_sync_inodes_ag+0x16b 0xffff88006c45fde8 0xffffffff80320990 radix_tree_tagged+0xa 0xffff88006c45fe10 0xffffffffa01f36c6 [xfs]xfs_sync_inodes_ag+0x197 0xffff88006c45fe80 0xffffffffa01f38d1 [xfs]xfs_sync_inodes+0x63 0xffff88006c45fec0 0xffffffffa01f3997 [xfs]xfs_quiesce_data+0x13 0xffff88006c45fec8 0xffffffff802452b9 autoremove_wake_function 0xffff88006c45fee0 0xffffffffa01f1800 [xfs]xfs_fs_sync_super+0x2b 0xffff88006c45ff00 0xffffffff8043b871 __down_read+0x12 0xffff88006c45ff10 0xffffffffa024d395 [ext3]ext3_sync_fs+0x46 0xffff88006c45ff40 0xffffffff80292fd2 sync_filesystems+0xae 0xffff88006c45ff60 0xffffffff802af48b do_sync+0x2f 0xffff88006c45ff70 0xffffffff802af4c4 sys_sync+0xe

[1]kdb> rd

    r15 = 0x0000000000000002      r14 = 0x0000000000000000
    r13 = 0x000000000000000a      r12 = 0x0000000000000040
     bp = 0xffff88003793a9d8       bx = 0xffff880055c10250
    r11 = 0x0000000000000001      r10 = 0xffff880055d0ade8
     r9 = 0x000000000002309f       r8 = 0xffffffffa01f369a
     ax = 0x0000000000200000       cx = 0x0000000000000015
     dx = 0x0000000000000000       si = 0x0000000000000000
     di = 0x6b6b6b6b6b6b6b73  orig_ax = 0xffffffffffffffff
     ip = 0xffffffff80320990       cs = 0x0000000000000010
  flags = 0x0000000000010206       sp = 0xffff88006c45fe00
     ss = 0x0000000000000018 &regs = 0xffff88006c45fd68

[1]kdb> id %ip
0xffffffff80320990 radix_tree_tagged+0xa: and 0x4(%rdi),%eax
0xffffffff80320993 radix_tree_tagged+0xd: retq

0xffffffff80320994 radix_tree_callback:         cmp    $0x7,%rsi
0xffffffff80320998 radix_tree_callback+0x4:     push   %rbx
0xffffffff80320999 radix_tree_callback+0x5:     je      

0xffffffff803209a1 radix_tree_callback+0xd
0xffffffff8032099b radix_tree_callback+0x7: cmp $0x17,%rsi
0xffffffff8032099f radix_tree_callback+0xb: jne
0xffffffff803209e1 radix_tree_callback+0x4d
0xffffffff803209a1 radix_tree_callback+0xd: movslq %edx,%rax
0xffffffff803209a4 radix_tree_callback+0x10: mov
3961501(%rip),%rdx # 0xffffffff806e7c48 _cpu_pda
0xffffffff803209ab radix_tree_callback+0x17: mov
$0xffffffff80796480,%rbx

0xffffffff803209b2 radix_tree_callback+0x1e:    mov    (%rdx,%rax,8),%rax
0xffffffff803209b6 radix_tree_callback+0x22:    add    0x8(%rax),%rbx
0xffffffff803209ba radix_tree_callback+0x26:    jmp     

0xffffffff803209db radix_tree_callback+0x47
0xffffffff803209bc radix_tree_callback+0x28: cltq
0xffffffff803209be radix_tree_callback+0x2a: mov
5545627(%rip),%rdi # 0xffffffff8086a860 __key.8127
0xffffffff803209c5 radix_tree_callback+0x31: mov (%rbx,%rax,8),%rsi

The back trace is a little busted xfs_sync_inodes_ag appears to be calling:
xfs_sync_inodes_ag->xfs_flush_pages->mapping_tagged->radix_tree_tagged()

Review

Quoted 10 lines

(quoted diff) +3 -0

+		 * in reclaim. Leave it for the reclaim code to flush.
 		 */
 		if (igrab(inode)) {
 			read_unlock(&pag->pag_ici_lock);
 			xfs_ilock(ip, lock_flags);
 		} else {
+			/* leave it to reclaim */
 			read_unlock(&pag->pag_ici_lock);
+			continue;
 		}

Might be betters as

		if (!igrab(inode)) {
			/* leave it to reclaim */
			read_unlock(&pag->pag_ici_lock);
			continue;
		}

		read_unlock(&pag->pag_ici_lock);
		xfs_ilock(ip, lock_flags);

which then also shows that we could stop doing the ilock at all for the DELWRI case, thas is after fixing the last caller doing SYNC_ATTR|SYNC_DELWRI. Well yeah, lots of things still to sort out in this area.

Review

Quoted 2 lines · Dave Chinner

(quoted diff) +2 -0

+		xfs_iunlock(ip, lock_flags);
+		IRELE(ip);

Aka xfs_iput(ip, lock_flags);

Update below.

Cheers,

Dave.

Quoted 6 lines · Dave Chinner

Update below.

Cheers,

Dave.

The original patch appeared to fix the issue, however the latest one Oops as follows:

Quoted 6 lines · Peter Leckie

Quoted 4 lines · Dave Chinner

Update below.

Cheers,

Quoted 5 lines

Quoted 3 lines

Dave.

The original patch appeared to fix the issue, however the latest one Oops as follows:

Well, I think the problem dhould be obvious

- it's the same as
the first report - deferencing the linux inode without first having a refernce on it.

Quoted 5 lines

- it's the same as
the first report - deferencing the linux inode without first having a refernce on it.

Yes it resolves the issue.

Quoted 7 lines

Quoted 5 lines

- it's the same as
the first report - deferencing the linux inode without first having a refernce on it.

Yes it resolves the issue.

I spoke to soon, Ooops as follows:

Review xfs_sync_inodes_ag( · line 108

Quoted 12 lines · Peter Leckie

Adding the following resolved the issue however you may wish to solve it in another manner.

(quoted diff) +1 -1

@@ -102,7 +102,7 @@ xfs_sync_inodes_ag(
                * in reclaim. Leave it for the reclaim code to flush.
                */
               inode = VFS_I(ip);
-               if (!igrab(inode)) {
+               if (!inode || !igrab(inode)) {
                       read_unlock(&pag->pag_ici_lock);
                       continue;
               }

This would be the correct fix for the current code, but can you please put in the Inode/XFS Inode unification patches? At least I have only QAed the whole patchkit, and from the issues here it seems like Dave did the same.

And with that it would also be very good if there was any chance Dave and me (and all others) could actually access the current tree. The ptools -> CVS export has been broken for mor than two days, and there is no uptodate git tree eiter, so us external developers are completely tapping in the dark vs the current tree.

Quoted 9 lines · Peter Leckie

Quoted 6 lines · Dave Chinner

Update below.

Cheers,

Dave.

The original patch appeared to fix the issue, however the latest one Oops as follows:

Well, I think the problem dhould be obvious - it's the same as the first report - deferencing the linux inode without first having a refernce on it.

FWIW, if you apply the "combine linux/xfs inode" patch series, this failure will go away.

Updated patch below.

Cheers,

Dave.

Signature · 31 lines

Dave Chinner
[email protected]

XFS: avoid all reclaimable inodes in xfs_sync_inodes_ag

If we are syncing data in xfs_sync_inodes_ag(), the VFS
inode must still be referencable as the dirty data state
is carried on the VFS inode. hence if we can't get a
reference via igrab(), the inode must be in reclaim which
implies that it has no dirty data attached.

Leave such inodes to the reclaim code to flush the dirty
inode state to disk and so avoid attempting to access the
VFS inode when it may not exist in xfs_sync_inodes_ag().

Version 4:
o don't reference liinux inode untiil after igrab() succeeds

Version 3:
o converted unlock/rele to an xfs_iput() call.

Version 2:
o change igrab logic to be more linear
o remove initial reclaimable inode check now that we are using
  igrab() failure to find reclaimable inodes
o assert that igrab failure occurs only on reclaimable inodes
o clean up inode locking - only grab the iolock if we are doing
  a SYNC_DELWRI call and we have a dirty inode.
---
 fs/xfs/linux-2.6/xfs_sync.c |   75 ++++++++++--------------------------------
 1 files changed, 18 insertions(+), 57 deletions(-)

fs/xfs/linux-2.6/xfs_sync.c +18 -57

index 08b2acf..39ed7f3 100644
@@ -63,25 +63,16 @@ xfs_sync_inodes_ag(
 	int		error = 0;
 	int		last_error = 0;
 	int		fflag = XFS_B_ASYNC;
-	int		lock_flags = XFS_ILOCK_SHARED;
 
 	if (flags & SYNC_DELWRI)
 		fflag = XFS_B_DELWRI;
 	if (flags & SYNC_WAIT)
 		fflag = 0;		/* synchronous overrides all */
 
-	if (flags & SYNC_DELWRI) {
-		/*
-		 * We need the I/O lock if we're going to call any of
-		 * the flush/inval routines.
-		 */
-		lock_flags |= XFS_IOLOCK_SHARED;
-	}
-
 	do {
 		struct inode	*inode;
-		boolean_t	inode_refed;
 		xfs_inode_t	*ip = NULL;
+		int		lock_flags = XFS_ILOCK_SHARED;
 
 		/*
 		 * use a gang lookup to find the next inode in the tree

@@ -109,22 +100,6 @@ xfs_sync_inodes_ag(
 			break;
 		}
 
-		/*
-		 * skip inodes in reclaim. Let xfs_syncsub do that for
-		 * us so we don't need to worry.
-		 */
-		if (xfs_iflags_test(ip, (XFS_IRECLAIM|XFS_IRECLAIMABLE))) {
-			read_unlock(&pag->pag_ici_lock);
-			continue;
-		}
-
-		/* bad inodes are dealt with elsewhere */
-		inode = VFS_I(ip);
-		if (is_bad_inode(inode)) {
-			read_unlock(&pag->pag_ici_lock);
-			continue;
-		}
-
 		/* nothing to sync during shutdown */
 		if (XFS_FORCED_SHUTDOWN(mp)) {
 			read_unlock(&pag->pag_ici_lock);

@@ -132,42 +107,34 @@ xfs_sync_inodes_ag(
 		}
 
 		/*
-		 * If we can't get a reference on the VFS_I, the inode must be
-		 * in reclaim. If we can get the inode lock without blocking,
-		 * it is safe to flush the inode because we hold the tree lock
-		 * and xfs_iextract will block right now. Hence if we lock the
-		 * inode while holding the tree lock, xfs_ireclaim() is
-		 * guaranteed to block on the inode lock we now hold and hence
-		 * it is safe to reference the inode until we drop the inode
-		 * locks completely.
+		 * If we can't get a reference on the inode, it must be
+		 * in reclaim. Leave it for the reclaim code to flush.
 		 */
-		inode_refed = B_FALSE;
-		if (igrab(inode)) {
-			read_unlock(&pag->pag_ici_lock);
-			xfs_ilock(ip, lock_flags);
-			inode_refed = B_TRUE;
-		} else {
-			if (!xfs_ilock_nowait(ip, lock_flags)) {
-				/* leave it to reclaim */
-				read_unlock(&pag->pag_ici_lock);
-				continue;
-			}
+		inode = VFS_I(ip);
+		if (!igrab(inode)) {
 			read_unlock(&pag->pag_ici_lock);
+			continue;
+		}
+		read_unlock(&pag->pag_ici_lock);
+
+		/* bad inodes are dealt with elsewhere */
+		if (is_bad_inode(inode)) {
+			IRELE(ip);
+			continue;
 		}
 
 		/*
 		 * If we have to flush data or wait for I/O completion
-		 * we need to drop the ilock that we currently hold.
-		 * If we need to drop the lock, insert a marker if we
-		 * have not already done so.
+		 * we need to hold the iolock.
 		 */
 		if ((flags & SYNC_DELWRI) && VN_DIRTY(inode)) {
-			xfs_iunlock(ip, XFS_ILOCK_SHARED);
+			xfs_ilock(ip, XFS_IOLOCK_SHARED);
+			lock_flags |= XFS_IOLOCK_SHARED;
 			error = xfs_flush_pages(ip, 0, -1, fflag, FI_NONE);
 			if (flags & SYNC_IOWAIT)
 				vn_iowait(ip);
-			xfs_ilock(ip, XFS_ILOCK_SHARED);
 		}
+		xfs_ilock(ip, XFS_ILOCK_SHARED);
 
 		if ((flags & SYNC_ATTR) && !xfs_inode_clean(ip)) {
 			if (flags & SYNC_WAIT) {

@@ -183,13 +150,7 @@ xfs_sync_inodes_ag(
 					xfs_ifunlock(ip);
 			}
 		}
-
-		if (lock_flags)
-			xfs_iunlock(ip, lock_flags);
-
-		if (inode_refed) {
-			IRELE(ip);
-		}
+		xfs_iput(ip, lock_flags);
 
 		if (error)
 			last_error = error;

Ping?

Can this patch be applied to the new sync code before it is pushed to Linus?

Cheers,

Dave.

Quoted 190 lines · Dave Chinner

Quoted 11 lines · Peter Leckie

Quoted 7 lines · Dave Chinner

Update below.

Cheers,

Dave.

The original patch appeared to fix the issue, however the latest one Oops as follows:

Well, I think the problem dhould be obvious - it's the same as the first report - deferencing the linux inode without first having a refernce on it.

FWIW, if you apply the "combine linux/xfs inode" patch series, this failure will go away.

Updated patch below.

Cheers,

Dave.

Signature · 31 lines

Dave Chinner
[email protected]

XFS: avoid all reclaimable inodes in xfs_sync_inodes_ag

If we are syncing data in xfs_sync_inodes_ag(), the VFS
inode must still be referencable as the dirty data state
is carried on the VFS inode. hence if we can't get a
reference via igrab(), the inode must be in reclaim which
implies that it has no dirty data attached.

Leave such inodes to the reclaim code to flush the dirty
inode state to disk and so avoid attempting to access the
VFS inode when it may not exist in xfs_sync_inodes_ag().

Version 4:
o don't reference liinux inode untiil after igrab() succeeds

Version 3:
o converted unlock/rele to an xfs_iput() call.

Version 2:
o change igrab logic to be more linear
o remove initial reclaimable inode check now that we are using
  igrab() failure to find reclaimable inodes
o assert that igrab failure occurs only on reclaimable inodes
o clean up inode locking - only grab the iolock if we are doing
  a SYNC_DELWRI call and we have a dirty inode.
---
 fs/xfs/linux-2.6/xfs_sync.c |   75 ++++++++++--------------------------------
 1 files changed, 18 insertions(+), 57 deletions(-)

fs/xfs/linux-2.6/xfs_sync.c +18 -57

index 08b2acf..39ed7f3 100644
@@ -63,25 +63,16 @@ xfs_sync_inodes_ag(
 	int		error = 0;
 	int		last_error = 0;
 	int		fflag = XFS_B_ASYNC;
-	int		lock_flags = XFS_ILOCK_SHARED;
 
 	if (flags & SYNC_DELWRI)
 		fflag = XFS_B_DELWRI;
 	if (flags & SYNC_WAIT)
 		fflag = 0;		/* synchronous overrides all */
 
-	if (flags & SYNC_DELWRI) {
-		/*
-		 * We need the I/O lock if we're going to call any of
-		 * the flush/inval routines.
-		 */
-		lock_flags |= XFS_IOLOCK_SHARED;
-	}
-
 	do {
 		struct inode	*inode;
-		boolean_t	inode_refed;
 		xfs_inode_t	*ip = NULL;
+		int		lock_flags = XFS_ILOCK_SHARED;
 
 		/*
 		 * use a gang lookup to find the next inode in the tree

@@ -109,22 +100,6 @@ xfs_sync_inodes_ag(
 			break;
 		}
 
-		/*
-		 * skip inodes in reclaim. Let xfs_syncsub do that for
-		 * us so we don't need to worry.
-		 */
-		if (xfs_iflags_test(ip, (XFS_IRECLAIM|XFS_IRECLAIMABLE))) {
-			read_unlock(&pag->pag_ici_lock);
-			continue;
-		}
-
-		/* bad inodes are dealt with elsewhere */
-		inode = VFS_I(ip);
-		if (is_bad_inode(inode)) {
-			read_unlock(&pag->pag_ici_lock);
-			continue;
-		}
-
 		/* nothing to sync during shutdown */
 		if (XFS_FORCED_SHUTDOWN(mp)) {
 			read_unlock(&pag->pag_ici_lock);

@@ -132,42 +107,34 @@ xfs_sync_inodes_ag(
 		}
 
 		/*
-		 * If we can't get a reference on the VFS_I, the inode must be
-		 * in reclaim. If we can get the inode lock without blocking,
-		 * it is safe to flush the inode because we hold the tree lock
-		 * and xfs_iextract will block right now. Hence if we lock the
-		 * inode while holding the tree lock, xfs_ireclaim() is
-		 * guaranteed to block on the inode lock we now hold and hence
-		 * it is safe to reference the inode until we drop the inode
-		 * locks completely.
+		 * If we can't get a reference on the inode, it must be
+		 * in reclaim. Leave it for the reclaim code to flush.
 		 */
-		inode_refed = B_FALSE;
-		if (igrab(inode)) {
-			read_unlock(&pag->pag_ici_lock);
-			xfs_ilock(ip, lock_flags);
-			inode_refed = B_TRUE;
-		} else {
-			if (!xfs_ilock_nowait(ip, lock_flags)) {
-				/* leave it to reclaim */
-				read_unlock(&pag->pag_ici_lock);
-				continue;
-			}
+		inode = VFS_I(ip);
+		if (!igrab(inode)) {
 			read_unlock(&pag->pag_ici_lock);
+			continue;
+		}
+		read_unlock(&pag->pag_ici_lock);
+
+		/* bad inodes are dealt with elsewhere */
+		if (is_bad_inode(inode)) {
+			IRELE(ip);
+			continue;
 		}
 
 		/*
 		 * If we have to flush data or wait for I/O completion
-		 * we need to drop the ilock that we currently hold.
-		 * If we need to drop the lock, insert a marker if we
-		 * have not already done so.
+		 * we need to hold the iolock.
 		 */
 		if ((flags & SYNC_DELWRI) && VN_DIRTY(inode)) {
-			xfs_iunlock(ip, XFS_ILOCK_SHARED);
+			xfs_ilock(ip, XFS_IOLOCK_SHARED);
+			lock_flags |= XFS_IOLOCK_SHARED;
 			error = xfs_flush_pages(ip, 0, -1, fflag, FI_NONE);
 			if (flags & SYNC_IOWAIT)
 				vn_iowait(ip);
-			xfs_ilock(ip, XFS_ILOCK_SHARED);
 		}
+		xfs_ilock(ip, XFS_ILOCK_SHARED);
 
 		if ((flags & SYNC_ATTR) && !xfs_inode_clean(ip)) {
 			if (flags & SYNC_WAIT) {

@@ -183,13 +150,7 @@ xfs_sync_inodes_ag(
 					xfs_ifunlock(ip);
 			}
 		}
-
-		if (lock_flags)
-			xfs_iunlock(ip, lock_flags);
-
-		if (inode_refed) {
-			IRELE(ip);
-		}
+		xfs_iput(ip, lock_flags);
 
 		if (error)
 			last_error = error;

Hey Dave I'll push it now, I assume it's ok to go in with out the check for a inode first
as the combine linux/XFS inode patch is in.

Can you confirm that the xfs_inode has either the I_RECLAIM or I_RECLAIMABLE flag set on it when the panic occurred? If this is the case, then the patch below will probably fix it.