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