[PATCH] Always reset btree cursor after an insert

[PATCH] Always reset btree cursor after an insert

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

Lachlan

--- fs/xfs/xfs_bmap.c_1.392	2008-06-03 12:20:14.000000000 +1000
+++ fs/xfs/xfs_bmap.c	2008-06-16 12:11:47.000000000 +1000
@@ -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.
+			 */
+			if ((error = xfs_bmbt_lookup_eq(cur, new->br_startoff,
+					new->br_startblock, new->br_blockcount,
+					&i)))
 				goto done;
-			ASSERT(i == 1);
+			ASSERT(i == 0);
 			/* 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);

Re: [PATCH] Always reset btree cursor after an insert

[Dave Chinner]

On Mon, Jun 16, 2008 at 12:21:33PM +1000, Lachlan McIlroy wrote:
> 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....

> --- fs/xfs/xfs_bmap.c_1.392	2008-06-03 12:20:14.000000000 +1000
> +++ fs/xfs/xfs_bmap.c	2008-06-16 12:11:47.000000000 +1000
> @@ -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
 */

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

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

ASSERT? How about a WANT_CORRUPTED_GOTO()?

> 			/* 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.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] Always reset btree cursor after an insert

[Lachlan McIlroy]

Dave Chinner wrote:
> On Mon, Jun 16, 2008 at 12:21:33PM +1000, Lachlan McIlroy wrote:
>> 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.

> 
>> --- fs/xfs/xfs_bmap.c_1.392	2008-06-03 12:20:14.000000000 +1000
>> +++ fs/xfs/xfs_bmap.c	2008-06-16 12:11:47.000000000 +1000
>> @@ -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
>  */

Fair enough.

> 
>> +			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;
> 
>> -			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?

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

Re: [PATCH] Always reset btree cursor after an insert

[Dave Chinner]

On Sunday 15 June 2008 11:41 pm, Lachlan McIlroy wrote:
> Dave Chinner wrote:
> > On Mon, Jun 16, 2008 at 12:21:33PM +1000, Lachlan McIlroy wrote:
> >> 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.

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

Cheers,

Dave.

Re: [PATCH] Always reset btree cursor after an insert

[Lachlan McIlroy]

Dave Chinner wrote:
> On Sunday 15 June 2008 11:41 pm, Lachlan McIlroy wrote:
>> Dave Chinner wrote:
>>> On Mon, Jun 16, 2008 at 12:21:33PM +1000, Lachlan McIlroy wrote:
>>>> 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.
> 
>>>> +			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;
>>>
>>>> -			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...
> 

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.

Re: [PATCH] Always reset btree cursor after an insert

[Dave Chinner]

On Tue, Jun 17, 2008 at 11:24:17AM +1000, Lachlan McIlroy wrote:
> Dave Chinner wrote:
>> On Sunday 15 June 2008 11:41 pm, Lachlan McIlroy wrote:
>>>> 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.

In the past we've only changed the bits of the code that have been
needed for debugging problems. e.g. there's WANT_CORRUPTED
throughout the alloc code, but only some bits of the bmbt code and
almost none in the inobt. Really we should be consistent with
our catching and handling of errors.

IOWs, I'd say we probably should change them all, but it's going
to touch lots of code. For the btree code, I'd say it should be done
with the factoring (get them all in one go), but xfs_bmap.c code is
separate and could be done separately.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com