Re: REVIEW: Fix for incore extent corruption.

[Lachlan McIlroy <lachlan@xxxxxxx>]

Eric Sandeen wrote:
> Lachlan McIlroy wrote:
> > Eric Sandeen wrote:
> > > Lachlan McIlroy wrote:
> > > > Russell, this fixes xfs_iext_irec_compact_full().  If we don't move
> > > > all the records from the next page into the current page then we need
> > > > to update the er_extoff of the modified page as we move the remaining
> > > > extents up.  Would you mind giving it a go?
> > > >
> > > > --- a/fs/xfs/xfs_inode.c        2008-09-18 18:48:46.000000000 +1000
> > > > +++ b/fs/xfs/xfs_inode.c        2008-09-18 18:57:18.000000000 +1000
> > > > @@ -4623,6 +4623,7 @@ xfs_iext_irec_compact_full(
> > > >                                         (XFS_LINEAR_EXTS -
> > > >                                                 erp_next->er_extcount) *
> > > >                                         sizeof(xfs_bmbt_rec_t));
> > > > +                               erp_next->er_extoff += ext_diff;
> > > >                         }
> > > >                 }
> > > Lachlan, I concur.  I spent way too long last night looking at this and
> > > arrived at the same conclusion about the root cause of the problem, but
> > > didn't hae *quite* the right solution.  I blame it on 2am ;)  Your fix
> > > looks right.
> > >
> > > (though I'd probably move the erp_next changes into the else clause? 
> > > Otherwise you're changing it then freeing it.)
> > I don't understand what you mean by that.  Could you elaborate?
>
> Sorry I mis-read where the above hunk went... that makes sense as-is above.
>
> For clarity having the erp_next->er_extoff and er_extcount adjustments
> together *might* make sense but no big deal.

That did occur to me.  I thought if I put it with the er_extcount adjustment
then it might look like the er_extoff line was missing from
xfs_iext_irec_compact_pages() because it does the er_extcount adjustment too
but never needs to adjust er_extoff.  Doesn't really matter if we get rid of
xfs_iext_irec_compact_full().