Clearer wording on Errata #4 for non-destructive remedy

[JMoVS]

Errata #4 doesn’t mention a non-destructive way to clear it. This commits fixes #8682

Motivation and Context

Users of existing pools, especially pools with top-level encrypted datasets could run into trouble trying to work around Errata #4. After @behlendorf's clarification, there is a non-destructive way that is possible. This commit mentions that.

This commit attempts to fix #8682.

Description

Just a documentation change where previous Errata #4 changes were made-

How Has This Been Tested?

only documentation changes, haven't tested them myself.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the ZFS on Linux code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• All new and existing tests passed.
• All commit messages are properly formatted and contain Signed-off-by.

[rlaager]

You have this:

Enable the bookmark_v2 feature, backup these datasets to new encrypted datasets, and destroy the old ones or create new snapshots and destroy all snapshots that were created before enabling bookmark_v2.

It wasn't clear to me the precedence of that "or". I read it a couple times and it wasn't making sense, as new datasets wouldn't have snapshots to delete. In reading the referenced issue, it sounds like deleting the snapshots is an alternative to creating new datasets.

Also, from what I can see, creating snapshots is not required to fix the issue. The deletion is the important part.

How about something like this:

Enable the bookmark_v2 feature. Then, copy these datasets to new encrypted datasets, and destroy the old datasets. Alternatively, destroy all snapshots (on encrypted datasets) that were created before enabling bookmark_v2.

Or focus on the snapshot deletion first, which might be even better:

Enable the bookmark_v2 feature. Destroy all snapshots (on encrypted datasets) that were created before enabling bookmark_v2. If preserving those snapshots is required, copy them to new encrypted datasets with zfs send.

[JMoVS]

Enable the bookmark_v2 feature. Destroy all snapshots (on encrypted datasets) that were created before enabling bookmark_v2. If preserving those snapshots is required, copy them to new encrypted datasets with zfs send.

I like that wording a lot. And you're right: It's about getting rid of existing snapshots.

[JMoVS]

there's a very weird mixture of tabs vs spaces going on in that code section...

[rlaager]

The wording sounds good to me. This is an approval for that much.

I have not personally confirmed that deleting snapshots is sufficient. I am relying on the quotes from Brian to that effect.

I also have not personally run this code to verify the zpool status output is correctly line-wrapped and tabbed. @JMoVS, can you paste in an example of the zpool status output?

[behlendorf]

Let's make sure to get @tcaputi's feedback on the updated wording here.

[JMoVS]

The wording sounds good to me. This is an approval for that much.

I have not personally confirmed that deleting snapshots is sufficient. I am relying on the quotes from Brian to that effect.

I also have not personally run this code to verify the zpool status output is correctly line-wrapped and tabbed. @JMoVS, can you paste in an example of the zpool status output?

I verified that getting rid of snapshots (and a subsequent reimport of the pool which is always necessary) was sufficient to get rid of the Errata on O3X (I'm an O3X user). I'll see if I can directly pull in that commit into the O3X codebase and test it directly, otherwise I very welcome if somebody else can more easily confirm correct formatting of the zpool status output ;-)

[tcaputi]

LGTM. The sentiment here is accurate. The wording feels a little awkward to me, but its not a big deal.

[tcaputi]

Small correction, any bookmarks created before enabling bookmark_v2 will also need to be destroyed.

[codecov]

Codecov Report

Merging #8683 into master will decrease coverage by 0.73%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #8683      +/-   ##
==========================================
- Coverage   79.34%   78.61%   -0.74%     
==========================================
  Files         262      381     +119     
  Lines       77786   117580   +39794     
==========================================
+ Hits        61723    92434   +30711     
- Misses      16063    25146    +9083

Flag	Coverage Δ
#kernel	79.35% <ø> (ø)	⬆️
#user	67% <66.66%> (?)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a15c00...129e9ee. Read the comment docs.

[JMoVS]

@tcaputi I changed it to include bookmarks in the language.

@ALL please check again that I didn't mess it up now. If you want me to rebase and squash it to 1 commit again, let me know. There is a weird mix of tabs and spaces in that code section - I maintained how it was

[behlendorf]

@JMoVS @rlaager @tcaputi I think the proposed wording is a definite improvement but still a little awkward. How about this alternate proposed wording:

"Existing encrypted snapshots and bookmarks contain an on-disk incompatibility. This may cause on-disk corruption if they are used with 'zfs recv'. To correct the issue enable the bookmark_v2 feature. No additional action is needed if there are no encrypted snapshots or bookmarks. If preserving the encrypted snapshots and bookmarks is required use a non-raw send to backup and restore them. Alternately, they may be removed to resolve the incompatibility."

[JMoVS]

@JMoVS @rlaager @tcaputi I think the proposed wording is a different improvement but still a little awkward. How about this alternate proposed wording:

"Existing encrypted snapshots and bookmarks contain an on-disk incompatibility. This may cause on-disk corruption if they are used with 'zfs recv'. To correct the issue enable the bookmark_v2 feature. No additional action is needed if there are no encrypted snapshots or bookmarks. If preserving the encrypted snapshots and bookmarks is required use a non-raw send to backup and restore them. Alternately, they may be removed to resolve the incompatibility."

I like it, though my German (not sure if same rules apply in English) mind would put 2 additional commas in:

...To correct the issue[,] enable the bookmark_v2 feature... and If preserving the encrypted snapshots and bookmarks is required[,] use a non-raw send to backup and restore them.

[rlaager]

I agree with adding those commas.

[JMoVS]

@rlaager I take that as an "ok go".

@behlendorf I split your text up a bit for the status_callback part as it separates status from action. Aside from that splitting still the exact same text.

And I think I finally figured out the new lines and limited it to 80 characters. Still, please test it.

[JMoVS]

ha! This time I even got the STYLE test to give me a green check mark! 💪🏼

[behlendorf]

Thanks!

[JMoVS]

You’re welcome;-)
Should I squash it down to a single commit or will you squash and merge it?