Undirty removed spill blocks.

[dweeezil]

If a spill block's dbuf hasn't yet been written when a the spill block
is removed, the unwritten version will still be written. This patch
undirties the dbuf in this case to prevent it to be written.

The most common case in which this could happen is when xattr=sa is being
used and a long xattr is immediately replaced by a short xattr as in:

setfattr -n user.test -v very_very_very..._long_value  <file>
setfattr -n user.test -v short_value  <file>

The first value must be sufficiently long that a spill block is generated
and the second value must be short enough to not require a spill block.
In practice, this would typically happen due to internal xattr operations
as a result of setting acltype=posixacl.

[dweeezil]

This should fix the recently reported cases of dnode corruption which typically occur when xattr=sa and acltype=posixacl are set:
#2663
#2700
#2717
#2701
#2863
and maybe others.

[dweeezil]

A little more explanation is likely in order because this problem doesn't necessarily directly cause the problems seen in the referenced issues. This problem will cause an obsolete version of an SA (typically an SA xattr) to be stored in a spill block simultaneously with a newer version of the same SA in the dnode (which ends up corrupted). This corruption is often harmless and manifests itself as (typically the) SA xattr being "invisible". Subsequent modifications to the dnode, however, will generally lead to more severe corruption and it's these latter stages that appear to have happened in some cases.

[b333z]

wow, amazing work @dweeezil...

[tomposmiko]

+1, amazing work

@dweeezil
What can we do with already corrupted files?
Should we just remove them, or scrub will fix after applying the patch, or do we need to recreate the filesystem?

[dweeezil]

@tomposmiko If you can remove the corrupted files, you should be OK. Unfortunately, there are a number of different types of corruption resulting from follow-on access to the bug. During my testing, I was able to create corrupted filesystems which in some cases would cause the entire pool to become un-importable and in other cases would cause the filesystem to be un-deletable. I'd say that if you can remove the corrupted files, you should be pretty safe with the possible fallout being the potential of a bit of leaked space.

Due to my inability to recreate the problem as well as the number of problem reports, I had started working on a gross hack to work around the exact type of corruption seen in #2863. One of my hopes was that working on a workaround would give me an idea as to the nature of the problem. That is, in fact, exactly what happened. I don't plan on working on the workaround any more because I'm not convinced it can be implemented very reliably.

[dweeezil]

@behlendorf Yes, that's a better structure. I was so happy to get to the bottom of this problem that I kinda just jammed it into the "early return" part because I knew it would work there and had intended on looking into a cleaner implementation once the overall concept got some review.

[behlendorf]

@dweeezil I completely understand! The buildbot was done for a few days but it's back online now and should get your patch tested in the next day or so. If you're happy with the suggested restructuring you could refresh the pull request after giving it a good review to make sure I didn't overlook something.

[dweeezil]

I've restructured the code as suggested by @behlendorf. Interestingly, a version of the code in which I had introduced a lot of instrumentation, I did use a flag to indicate whether a spill was being freed but I called it freespill. In homage to that debugging code, I've used the same name. This patch does pass my test case.

[behlendorf]

This looks good to me. I like freespill better as a variable name too, isspill was hard to read. Once the buildbot has finished testing this change I'll get it merged.

[behlendorf]

Thanks everybody for getting to the bottom of this. It's been merged to master as:

4254acb Undirty freed spill blocks.