Re: REVIEW: Fix for incore extent corruption.

[Lachlan McIlroy <lachlan@sgi.com>]

Russell Cattelan wrote:
> Lachlan McIlroy wrote:
>> Here's a patch to remove xfs_iext_irec_compact_full() like Russell
>> did in his original patch - are you guys happy with this?
>>
>> I'm putting it through it's paces now and so far it looks good.
> I guess looking at my original patch it would have made sense to
> drop the call to xfs_iext_irec_irec_compact_pages/full in
> xfs_iext_indirect_to_direct  or do what you did and keep the
> if else logic.
Ah, I see what you did now.  That does simplify the code a bit but
on the other hand it will result in calling xfs_iext_irec_compact_pages()
more often - every time we remove an extent instead of when the buffer
utilisation drops below 50%.

> 
> So ya this look good.
> 
> 
>>
>> --- a/fs/xfs/xfs_inode.c    2008-09-19 13:08:08.000000000 +1000
>> +++ b/fs/xfs/xfs_inode.c    2008-09-19 13:16:34.000000000 +1000
>> @@ -4157,7 +4166,7 @@ xfs_iext_indirect_to_direct(
>>     ASSERT(nextents <= XFS_LINEAR_EXTS);
>>     size = nextents * sizeof(xfs_bmbt_rec_t);
>>
>> -    xfs_iext_irec_compact_full(ifp);
>> +    xfs_iext_irec_compact_pages(ifp);
>>     ASSERT(ifp->if_real_bytes == XFS_IEXT_BUFSZ);
>>
>>     ep = ifp->if_u1.if_ext_irec->er_extbuf;
>> @@ -4510,8 +4519,6 @@ xfs_iext_irec_compact(
>>         xfs_iext_direct_to_inline(ifp, nextents);
>>     } else if (nextents <= XFS_LINEAR_EXTS) {
>>         xfs_iext_indirect_to_direct(ifp);
>> -    } else if (nextents < (nlists * XFS_LINEAR_EXTS) >> 3) {
>> -        xfs_iext_irec_compact_full(ifp);
>>     } else if (nextents < (nlists * XFS_LINEAR_EXTS) >> 1) {
>>         xfs_iext_irec_compact_pages(ifp);
>>     }
>> @@ -4555,91 +4562,6 @@ xfs_iext_irec_compact_pages(
>> }
>>
>> /*
>> - * Fully compact the extent records managed by the indirection array.
>> - */
>> -void
>> -xfs_iext_irec_compact_full(
>> -    xfs_ifork_t    *ifp)            /* inode fork pointer */
>> -{
>> -    xfs_bmbt_rec_host_t *ep, *ep_next;    /* extent record pointers */
>> -    xfs_ext_irec_t    *erp, *erp_next;    /* extent irec pointers */
>> -    int        erp_idx = 0;        /* extent irec index */
>> -    int        ext_avail;        /* empty entries in ex list */
>> -    int        ext_diff;        /* number of exts to add */
>> -    int        nlists;            /* number of irec's (ex lists) */
>> -
>> -    ASSERT(ifp->if_flags & XFS_IFEXTIREC);
>> -
>> -    nlists = ifp->if_real_bytes / XFS_IEXT_BUFSZ;
>> -    erp = ifp->if_u1.if_ext_irec;
>> -    ep = &erp->er_extbuf[erp->er_extcount];
>> -    erp_next = erp + 1;
>> -    ep_next = erp_next->er_extbuf;
>> -
>> -    while (erp_idx < nlists - 1) {
>> -        /*
>> -         * Check how many extent records are available in this irec.
>> -         * If there is none skip the whole exercise.
>> -         */
>> -        ext_avail = XFS_LINEAR_EXTS - erp->er_extcount;
>> -        if (ext_avail) {
>> -
>> -            /*
>> -             * Copy over as many as possible extent records into
>> -             * the previous page.
>> -             */
>> -            ext_diff = MIN(ext_avail, erp_next->er_extcount);
>> -            memcpy(ep, ep_next, ext_diff * sizeof(xfs_bmbt_rec_t));
>> -            erp->er_extcount += ext_diff;
>> -            erp_next->er_extcount -= ext_diff;
>> -
>> -            /*
>> -             * If the next irec is empty now we can simply
>> -             * remove it.
>> -             */
>> -            if (erp_next->er_extcount == 0) {
>> -                /*
>> -                 * Free page before removing extent record
>> -                 * so er_extoffs don't get modified in
>> -                 * xfs_iext_irec_remove.
>> -                 */
>> -                kmem_free(erp_next->er_extbuf);
>> -                erp_next->er_extbuf = NULL;
>> -                xfs_iext_irec_remove(ifp, erp_idx + 1);
>> -                erp = &ifp->if_u1.if_ext_irec[erp_idx];
>> -                nlists = ifp->if_real_bytes / XFS_IEXT_BUFSZ;
>> -
>> -            /*
>> -             * If the next irec is not empty move up the content
>> -             * that has not been copied to the previous page to
>> -             * the beggining of this one.
>> -             */
>> -            } else {
>> -                memmove(erp_next->er_extbuf, &ep_next[ext_diff],
>> -                    erp_next->er_extcount *
>> -                    sizeof(xfs_bmbt_rec_t));
>> -                ep_next = erp_next->er_extbuf;
>> -                memset(&ep_next[erp_next->er_extcount], 0,
>> -                    (XFS_LINEAR_EXTS -
>> -                        erp_next->er_extcount) *
>> -                    sizeof(xfs_bmbt_rec_t));
>> -            }
>> -        }
>> -
>> -        if (erp->er_extcount == XFS_LINEAR_EXTS) {
>> -            erp_idx++;
>> -            if (erp_idx < nlists)
>> -                erp = &ifp->if_u1.if_ext_irec[erp_idx];
>> -            else
>> -                break;
>> -        }
>> -        ep = &erp->er_extbuf[erp->er_extcount];
>> -        erp_next = erp + 1;
>> -        ep_next = erp_next->er_extbuf;
>> -    }
>> -}
>> -
>> -/*
>>  * This is called to update the er_extoff field in the indirection
>>  * array when extents have been added or removed from one of the
>>  * extent lists. erp_idx contains the irec index to begin updating
>>
>>
>> Lachlan McIlroy wrote:
>>> 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?
>>>
>>>
>>>
>>>
>>
> 
>