Re: REVIEW: Fix for incore extent corruption.

Lachlan McIlroy
2008-09-19T03:15:13Z

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.

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