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

[Dave Chinner <dchinner@agami.com>]

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.