Re: REVIEW: Fix for incore extent corruption.

[Lachlan McIlroy <lachlan@sgi.com>]

Eric Sandeen wrote:
> Russell Cattelan 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;
>>>             }
>>>         }
>>>
>> Cool I'll give it some run through when I done traveling.
>>
>> I still think compact_full should simply be eliminated since
>> it really doesn't help, and it's obviously confusing code.
>> Or we should make sure it works and get rid of compact_pages
>> since compact_full behaves just like compact_pages when not
>> doing partial moves.
> 
> I'd agree with that, at least as far as reevaluating this packing stuff -
> given the seriousness of the bug when you do hit it, and how rarely it's
> ever hit, apparently this chunk of code is almost never run ....
> 

I agree too.  If any code is difficult to reach it's also difficult to test.
We've had numerous reports of extent corruption that could be explained by
this bug but we have not been able to reproduce the symptoms let alone devise
a reliable test case.

What real benefit does compact_full have over compact_pages?
Are there corner cases where compact_pages is not good enough?