Caculate header size correctly in sa_find_sizes()

[dweeezil]

In the case where a variable-sized SA overlaps the spill block pointer and
a new variable-sized SA is being added, the header size was improperly
calculated to include the to-be-moved SA. This problem could be
reproduced when xattr=sa enabled as follows:

ln -s xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx blah
setfattr -n security.selinux -v blahblah -h blah

The symlink is large enough to interfere with the spill block pointer
and has a typical SA registration of:

[ 5  6  4  12  13  7  11  0  1  2  3  8  16  19  17 ]

Adding the SA xattr will, effectively require a registration of:

[ 5  6  4  12  13  7  11  0  1  2  3  8  16  19  17  20 ]

Since the symlink (17) interferes with the spill block pointer, it must
also be moved to the spill block which should have a registration of:

[ 17 20 ]

This patch simply re-uses the logic within sa_find_sizes() in which the
"split point" is calculated (near the comment "find index of where spill
could occur" to update extra_hdrsize in this particular case.

[dweeezil]

This should fix #2316 and a number of other SA corruption cases. I'll try to track down the other issues to which it may apply.

I'm not real thrilled with the patch. The problem was likely introduced by 472e7c6 which fixed a similar issue which was typically manifested as corrupted symlinks.

I also may well have the description of the problem messed up a bit but I think I'm close.

@nedbass I'm sure you'll want to look at this since you've dealt with other SA-related issues.

[nedbass]

Nice find @dweeezil. The patch looks correct to me, but this function is so hard to review and become convinced that it's right (hence all the buggy corner cases). This is just screaming for better test coverage.

I'm not crazy about duplicating that nasty boolean expression though. Perhaps we should factor it out to make things slightly more readable:

index fcc5f3b..0af9631 100644
--- a/module/zfs/sa.c
+++ b/module/zfs/sa.c
@@ -595,6 +595,7 @@ sa_find_sizes(sa_os_t *sa, sa_bulk_attr_t *attr_desc, int attr_count,

        for (i = 0; i != attr_count; i++) {
                boolean_t is_var_sz;
+               boolean_t might_spill_here;

                *total = P2ROUNDUP(*total, 8);
                *total += attr_desc[i].sa_length;
@@ -606,6 +607,10 @@ sa_find_sizes(sa_os_t *sa, sa_bulk_attr_t *attr_desc, int attr_count,
                        var_size++;
                }

+               might_spill_here = buftype == SA_BONUS && *index == -1 &&
+                   (*total + P2ROUNDUP(hdrsize, 8)) >
+                   (full_space - sizeof (blkptr_t));
+
                if (is_var_sz && var_size > 1) {
                        /*
                         * Don't worry that the spill block might overflow.
@@ -622,7 +627,7 @@ sa_find_sizes(sa_os_t *sa, sa_bulk_attr_t *attr_desc, int attr_count,
                                 * spill-over.
                                 */
                                hdrsize += sizeof (uint16_t);
-                               if (*index != -1)
+                               if (*index != -1 || might_spill_here)
                                        extra_hdrsize += sizeof (uint16_t);
                        } else {
                                ASSERT(buftype == SA_BONUS);
@@ -639,11 +644,8 @@ sa_find_sizes(sa_os_t *sa, sa_bulk_attr_t *attr_desc, int attr_count,
                 * space.  The sum is used later for sizing bonus
                 * and spill buffer.
                 */
-               if (buftype == SA_BONUS && *index == -1 &&
-                   (*total + P2ROUNDUP(hdrsize, 8)) >
-                   (full_space - sizeof (blkptr_t))) {
+               if (might_spill_here)
                        *index = i;
-               }

                if ((*total + P2ROUNDUP(hdrsize, 8)) > full_space &&
                    buftype == SA_BONUS)

The reproducer command could be shortened to fit conventional line wrapping and self-document the length of the symlink target name:

ln -s `perl -e 'print "x" x 120'` blah

[dweeezil]

Thanks for the eyes on this, @nedbass. You hit on exactly the aspect of my patch that I was not very happy with. I will factor the conditional into a boolean as you suggested.

[behlendorf]

Thanks guys, I'm looking forward to the updated patch.

[dweeezil]

I've updated the patch based on @nedbass's comments and also refreshed the commit message.

[behlendorf]

Thanks guys, merged as:

83021b4 Calculate header size correctly in sa_find_sizes()