[OpenZFS Developer] a ZFS SA bug and my patch

[Ned Bass]

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