[OpenZFS Developer] a ZFS SA bug and my patch

[James Pan]

Hi Ned,
Thanks very much for your comments.
Actually 'done' is not necessary and can be completely removed.
The value of *will_spill is initialized only when buftype is SA_BONUS, so to remove buftype == SA_BONUS from the check, we need to set it to False for SA_SPILL buftype as well.

I've revised the patch to reflect these changes, could you help review it again and get it checked in to the main branch if it is OK?
Thanks a lot.

James Pan



On Thursday, November 21, 2013 4:43 AM, Ned Bass <bass6 at llnl.gov> wrote:
 
On Wed, Nov 20, 2013 at 04:15:21AM -0800, James Pan wrote:
> Hi,
> I hit a ZFS SA problem on FreeBSD 9.2, but I believe the issue exists on other
> platform too. Here is the description of the bug.
> 
> 
> PROBLEM:
> run the attached script on a ZFS, after a few seconds, run zdb -vvv on the ZFS,
> zdb will crash at the following assertion:
> 
> Assertion failed: (IS_SA_BONUSTYPE(bonustype) && SA_HDR_SIZE_MATCH_LAYOUT(hdr,
> tb) || !IS_SA_BONUSTYPE(bonustype) || (IS_SA_BONUSTYPE(bonustype) && hdr->
> sa_layout_info == 0)), file /usr/src/cddl/lib/libzpool/../../../sys/cddl/
> contrib/opensolaris/uts/common/fs/zfs/sa.c, line 1509.
> Abort (core dumped)
> 
> the reason is the SA's header size does not match its layout.
> 
> 
> ROOT CAUSE:
> The issue will be hit when a file has more than 2 variable-length SA and the
> total SA size is larger than the bonus buffer's length -  sizeof (blkptr_t),
> but less the bonus buffer's length.
> 
> in sa_find_sizes(), done is set to TRUE if the SA size + header > the bonus
> buffer's length - sizeof (blkptr_t), then hdrsize += sizeof (uint16_t) will be
> skipped for the second variable-length SA. If finally all SA can fit in the
> bonus buffer and no spill block is needed, we will get a wrong hdrsize.
> 
> MY FIX:
> I've also attached my simple fix for this issue, anyone who might have interest
> could you please take a look? Thanks a lot!
> 

Good catch.  I'm responsible for the first attempt to correct hdrsize in
the case of spill-over with variable-sized SAs.  But I clearly didn't
get it quite right.

I haven't tested your patch, but it looks correct to me.

+                               if (done)
+                                       extra_hdrsize += sizeof (uint16_t);
                        } else {
-                               done = B_TRUE;
-                               *index = i;
+                               if (!done) {
+                                       done = B_TRUE;
+                                       *index = i;
+                               }

It's not very obvious to the casual observer what we're "done" with when
done == B_TRUE.  It seems to mean we've found the index of the first SA
that will spill over _if_ we need to use a spill buffer.  While we're
revising this code, this could be made much more clear with a comment
and a self-documenting name (spill_index_found, perhaps?).


+       if (buftype == SA_BONUS && *will_spill)
+               hdrsize -= extra_hdrsize;

*will_spill implies buftype == SA_BONUS. This doesn't hurt to
double-check, but we could make it more explicit with an assertion:

    if (*will_spill) {
        ASSERT3U(buftype, ==, SA_BONUS);
        hdrsize -= extra_hdrsize;
    }

Ned

_______________________________________________
developer mailing list
developer at open-zfs.org
http://lists.open-zfs.org/mailman/listinfo/developer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sa.c.diff2
Type: application/octet-stream
Size: 2472 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/zfs-devel/attachments/20131122/b24003a1/attachment.obj>