Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zfs: default disable zfs_dmu_offset_next_sync to avoid data corruption #270275

Merged

Conversation

amarshall
Copy link
Member

This helps mitigate a data corruption bug. This was previously defaulted to zero prior to upstream commit
05b3eb6d232009db247882a39d518e7282630753, and it is already a tunable, so doing this seems safe. Initially data corruption was thought to be introduced with v2.2, but further upstream investigation currently believes that changes in v2.2 only exacerbated the issue that already existed.

A longer-term fix is likely to be
openzfs/zfs#15571, though that is not yet merged. The zfs_2_1 package has already backported that, so do not apply the tunable default change there.

Positioning of extraPatches is to avoid merge conflicts with #269097.

Patch is nearly identical to the Gentoo patch, but better patch formatting.

See openzfs/zfs#11900
See openzfs/zfs#15526

Description of changes

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Priorities

Add a 👍 reaction to pull requests you find important.

This helps mitigate a data corruption bug. This was previously defaulted
to zero prior to upstream commit
05b3eb6d232009db247882a39d518e7282630753, and it is already a tunable,
so doing this seems safe. Initially data corruption was thought to be
introduced with v2.2, but further upstream investigation currently
believes that changes in v2.2 only exacerbated the issue that already
existed.

A longer-term fix is likely to be
openzfs/zfs#15571, though that is not yet
merged. The zfs_2_1 package has already backported that, so do not apply
the tunable default change there.

Positioning of `extraPatches` is to avoid merge conflicts with
NixOS#269097.

Patch is nearly identical to the [Gentoo][1] patch, but better patch
formatting.

See openzfs/zfs#11900
See openzfs/zfs#15526

[1]: https://gitweb.gentoo.org/repo/gentoo.git/tree/sys-fs/zfs-kmod/files/zfs-kmod-2.2.1-Disable-zfs_dmu_offset_next_sync-tunable-by-default.patch
@numinit
Copy link
Contributor

numinit commented Nov 26, 2023

Agreed with this; it cuts the incidence rate of this corruption to something like 1 in 10 million in my testing from 1 in 10,000 (obviously while trying to force it)

openzfs/zfs#15526 (comment)

@adamcstephens adamcstephens merged commit c9b87a5 into NixOS:master Nov 27, 2023
25 of 26 checks passed
Copy link
Contributor

Backport failed for release-23.11, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-23.11
git worktree add -d .worktree/backport-270275-to-release-23.11 origin/release-23.11
cd .worktree/backport-270275-to-release-23.11
git switch --create backport-270275-to-release-23.11
git cherry-pick -x 9670ddc54389828b70447860821ed6521124bdd1

@adamcstephens
Copy link
Contributor

#270295 backport still hasn't been merged yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants