Always reset btree cursor after an insert

After a btree insert operation a cursor can be invalid due to block splits and a maybe a new root block. We reset the cursor in xfs_bmbt_insert() in the cases where we think we need to but it isn't enough as we still see assertions. Just do what we do elsewhere and reset the cursor unconditionally.

Quoted 5 lines · Lachlan McIlroy

After a btree insert operation a cursor can be invalid due to block splits and a maybe a new root block. We reset the cursor in xfs_bmbt_insert() in the cases where we think we need to but it isn't enough as we still see assertions. Just do what we do elsewhere and reset the cursor unconditionally.

Ok, so you should also kill the new code in the btree insert that revalidates the btree cursor. IIRC, this was the only place it was needed for....

xfs/xfs_bmap.c xfs_bmap_add_extent_unwritten_real( · line 1751

Quoted 12 lines

xfs/xfs_bmap.c renamed from xfs/xfs_bmap.c_1.392 +4 -1

@@ -1745,11 +1745,17 @@ xfs_bmap_add_extent_unwritten_real(
 			if ((error = xfs_bmbt_insert(cur, &i)))
 				goto done;
 			ASSERT(i == 1);
-			if ((error = xfs_bmbt_increment(cur, 0, &i)))
+			/*
+			 * Reset the cursor, don't trust it after any insert
+			 * operation.
+			 */

/*
* reset the cursor to the position of the new extent we are about
* to insert as we can't trust it after the previous insert */

xfs/xfs_bmap.c xfs_bmap_add_extent_unwritten_real( · line 1755

Quoted 5 lines

xfs/xfs_bmap.c renamed from xfs/xfs_bmap.c_1.392 +3 -0

+			if ((error = xfs_bmbt_lookup_eq(cur, new->br_startoff,
+					new->br_startblock, new->br_blockcount,
+					&i)))
 				goto done;

			error = xfs_bmbt_lookup_eq(cur, new->br_startoff,
					br_startblock, new->br_blockcount, &i);
			if (error)
				goto done;

xfs/xfs_bmap.c xfs_bmap_add_extent_unwritten_real( · line 1756

Quoted 2 lines

xfs/xfs_bmap.c renamed from xfs/xfs_bmap.c_1.392 +1 -1

-			ASSERT(i == 1);
+			ASSERT(i == 0);

ASSERT? How about a WANT_CORRUPTED_GOTO()?

xfs/xfs_bmap.c xfs_bmap_add_extent_unwritten_real( · line 1761

Quoted 7 lines

xfs/xfs_bmap.c renamed from xfs/xfs_bmap.c_1.392 +1 -1

 			/* new middle extent - newext */
-			cur->bc_rec.b = *new;
+			cur->bc_rec.b.br_state = new->br_state;
 			if ((error = xfs_bmbt_insert(cur, &i)))
 				goto done;
 			ASSERT(i == 1);

Cheers,

Dave.

Quoted 7 lines · Dave Chinner

Quoted 5 lines · Lachlan McIlroy

After a btree insert operation a cursor can be invalid due to block splits and a maybe a new root block. We reset the cursor in xfs_bmbt_insert() in the cases where we think we need to but it isn't enough as we still see assertions. Just do what we do elsewhere and reset the cursor unconditionally.

Quoted 3 lines

Ok, so you should also kill the new code in the btree insert that revalidates the btree cursor. IIRC, this was the only place it was needed for....

That was the plan.

Quoted 11 lines

Quoted 4 lines

(quoted diff) +3 -0

+			if ((error = xfs_bmbt_lookup_eq(cur, new->br_startoff,
+					new->br_startblock, new->br_blockcount,
+					&i)))

				goto done;

			error = xfs_bmbt_lookup_eq(cur, new->br_startoff,
					br_startblock, new->br_blockcount, &i);
			if (error)
				goto done;

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +1 -1

-			ASSERT(i == 1);
+			ASSERT(i == 0);

ASSERT? How about a WANT_CORRUPTED_GOTO()?

I was just being consistent with the rest of the code. If you think this ASSERT should be changed then what about all of them?

Quoted 11 lines · Lachlan McIlroy

Quoted 9 lines · Dave Chinner

Quoted 5 lines · Lachlan McIlroy

After a btree insert operation a cursor can be invalid due to block splits and a maybe a new root block. We reset the cursor in xfs_bmbt_insert() in the cases where we think we need to but it isn't enough as we still see assertions. Just do what we do elsewhere and reset the cursor unconditionally.

Ok, so you should also kill the new code in the btree insert that revalidates the btree cursor. IIRC, this was the only place it was needed for....

That was the plan.

Cool. Please include it in the next version of the patch.

Quoted 10 lines

Quoted 10 lines

Quoted 4 lines

(quoted diff) +3 -0

+			if ((error = xfs_bmbt_lookup_eq(cur, new->br_startoff,
+					new->br_startblock, new->br_blockcount,
+					&i)))

				goto done;

			error = xfs_bmbt_lookup_eq(cur, new->br_startoff,
					br_startblock, new->br_blockcount, &i);
			if (error)
				goto done;

Quoted 5 lines

Quoted 3 lines

Review

Quoted 2 lines

(quoted diff) +1 -1

-			ASSERT(i == 1);
+			ASSERT(i == 0);

ASSERT? How about a WANT_CORRUPTED_GOTO()?

I was just being consistent with the rest of the code. If you think this ASSERT should be changed then what about all of them?

Well, the ASSERT means silent failure on a production system. Given that failure here indicates a corrupt btree, then we really should be treating it as such. i.e. shut down the filesystem. This might have saved us a whole heap of trouble tracking down these problems as a shutdown here would have pointed us right at the source of the problem...

Quoted 11 lines · Dave Chinner

Quoted 3 lines · Lachlan McIlroy

Quoted 1 line

ASSERT? How about a WANT_CORRUPTED_GOTO()?

I was just being consistent with the rest of the code. If you think this ASSERT should be changed then what about all of them?

Well, the ASSERT means silent failure on a production system. Given that failure here indicates a corrupt btree, then we really should be treating it as such. i.e. shut down the filesystem. This might have saved us a whole heap of trouble tracking down these problems as a shutdown here would have pointed us right at the source of the problem...

Dave, what I was asking is what about the rest of the ASSERTs in this file - should we change them too? There's a lot of them. After this change they are all equally likely to trigger so if it makes sense to change one then the same argument applies to all of them.