Want tunable to ignore hole_birth

[rincebrain]

Between 0.6.5.7 and prior still being affected by illumos #6513, and the newly-discovered Illumos #7176, a tunable to ignore hole_birth for safe sends seems a reasonable tool.

This is just a minor improvement on @pcd1193182's patch to make it a runtime-tunable module parameter.

I tested it, and it functions correctly on ignoring hole_birth at least on the test dataset I have from #4809.

[behlendorf]

@rincebrain I don't object to adding a tunable but I'd suggest disabling this by default as in the original patch. Please also add a section to zfs-module-parameters.5 describing the option.

[tjikkun]

Would https://gist.github.com/tjikkun/df46ae6b84acc73c6f4000b19ffd9ba0 be the correct way to do this for 0.6.4.2?

[rincebrain]

@behlendorf Okay, I've added an entry for it to the man page, and changed the default to 0, though I'm really not fond of that.

I also proposed a feature flag, hole_birth_fix, in Illumos #7175, which has no purpose other than to let you safely use hole_birth data newer than the txg that feature flag was enabled, with the presumption that the fixes for illumos 6513 and 7176 are both merged at the same time/before that flag is integrated. I've not made a pull req because I wanted to get people's opinions on using a feature flag for something like this, but I don't see a cleaner option for letting people safely use hole_birth after a certain point while preventing you from "accidentally" importing r/w on an unfixed system.

If there's interest, I have the code for this sitting in a branch and could cut a pull request for it, I just figured it'd be less acrimonious to discuss it with people before trying to get it merged somewhere.

[behlendorf]

@rincebrain thanks for refreshing this. I've squashed and merged the updated patch to master and I do think it would be prudent to cherry pick this change for the 0.6.5.8 release and enable it by default as you originally proposed. I've tagged this 0.6.5.8 so we don't forget. We'll definitely want to get this fixed correctly on the master development branch before the next major release gets tagged.

Thanks for opening an issue on the illumos about adding a feature flag for this. Personally, I'd like to avoid that if possible since it's code we'll need to maintain forever and poses a potential portability issue.

[rincebrain]

I'm not too fond of the idea either, TBH, but I don't see a more reasonable way to safely delineate "after this txg, we promise we fixed the bugs" without requiring administrator intervention and manual detection of the issue.

[ilovezfs]

This doesn't really make sense to me. Bugs are bugs. We need to move on.

[Ringdingcoder]

Isn't this also necessary so that it also works on @hole_birth enabled pools?

--- a/module/zfs/dmu_traverse.c
+++ b/module/zfs/dmu_traverse.c
@@ -256,7 +256,7 @@ traverse_visitbp(traverse_data_t *td, const dnode_phys_t *dnp,
                        zb->zb_object == DMU_META_DNODE_OBJECT) &&
                        td->td_hole_birth_enabled_txg <= td->td_min_txg)
                        return (0);
-       } else if (bp->blk_birth <= td->td_min_txg) {
+       } else if (!ignore_hole_birth && bp->blk_birth <= td->td_min_txg) {
                return (0);
        }

[Ringdingcoder]

Hmm, forget what I said ;). I guess this would make all sends non-incremental. I missed that it is called blk_birth, not hole_birth.

[pcd1193182]

Just as an FYI to folks, OpenZFS is getting a version of this patch that defaults to 1; the community there feels that correctness needs to be job 1, so we're effectively disabling hole birth by default until a solution can be found. There are some good ideas for a solution already (which I will probably be implementing, if the community agrees with them); if you have any, feel free to join the discussion in the openzfs dev list, or in the PR at openzfs/openzfs#188