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.

Sign up for GitHub

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

Jump to bottom

nixos/zfs: only enable trim if zfs is enabled #70601

Merged
merged 4 commits into from
Oct 14, 2019
Merged

nixos/zfs: only enable trim if zfs is enabled #70601

merged 4 commits into from
Oct 14, 2019
+19 −10

Conversation

Mic92
Copy link
Member

@Mic92 Mic92 commented Oct 7, 2019

Also don't fail the service if there are no pools yet.
This might happen on installation ISOs.

See also: #69672 (comment)
cc @delroth

This has been not tested yet.

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

Sorry, something went wrong.

@Mic92
nixos/zfs: only enable trim if zfs is enabled
a412d90 
Also don't fail the service if there are no pools yet.
This might happen on installation ISOs.
@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Oct 7, 2019
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Oct 7, 2019
delroth
delroth approved these changes Oct 7, 2019
View reviewed changes
Copy link
Contributor

@delroth delroth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested that zpool-trim.service is now not started on my non-ZFS systems. Thanks!

Note that this makes the logic for zpool-trim different than the one implemented for the other ZFS services in that file. For example, enableZfs is defined to be enabled if inuse || enableAutoSnapshots || enableAutoScrub, without taking zpool-trim into account. IMHO enableAutoSnapshots and enableAutoScrub should be removed from the enableZfs boolean, but I don't have a clear understanding of the consequences or how much it would break.

delroth
delroth reviewed Oct 7, 2019
View reviewed changes
nixos/modules/tasks/filesystems/zfs.nix Outdated
script = ''
zpool list -H -o name | xargs -n1 zpool trim
'';
ExecStart = "${pkgs.runtimeShell} -c 'zpool list -H -o name | xargs --no-run-if-empty -n1 zpool trim'";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be serviceConfig.ExecStart.

@Mic92
nixos/zfs: avoid script derivation for trim service
692656d 
Since we only have a single pipe we can save the overhead of building a derivation
when creating the zfs trim service file when building the system.
@Mic92
Copy link
Member Author

Mic92 commented Oct 8, 2019

Note that this makes the logic for zpool-trim different than the one implemented for the other ZFS services in that file. For example, enableZfs is defined to be enabled if inuse || enableAutoSnapshots || enableAutoScrub, without taking zpool-trim into account. IMHO enableAutoSnapshots and enableAutoScrub should be removed from the enableZfs boolean, but I don't have a clear understanding of the consequences or how much it would break.

The Autosnapshot/autoscrub services are now only enabled if zfs is somehow used in the system.

@ofborg ofborg bot added 8.has: changelog 8.has: documentation This PR adds or changes documentation labels Oct 8, 2019
Mic92 added 2 commits October 14, 2019 10:30
@Mic92
nixos/zfs: simplify logic for scrub/autosnapshot service
9a89467 
This makes them consistent with the way zfs.trim is enabled
and allow to enable them by default in future.
@Mic92
nixos/zfs: mention trim support in the release notes
12880e5
@Mic92 Mic92 merged commit d633ec9 into NixOS:master Oct 14, 2019
dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Oct 15, 2019
@Mic92 @dtzWill
Merge pull request NixOS#70601 from Mic92/zfs-trim
2361768 
nixos/zfs: only enable trim if zfs is enabled

(cherry picked from commit d633ec9)
yorickvP pushed a commit to serokell/nixpkgs that referenced this pull request Jan 8, 2020
@Mic92 @yorickvP
Merge pull request NixOS#70601 from Mic92/zfs-trim
7010948 
nixos/zfs: only enable trim if zfs is enabled
@Mic92 Mic92 deleted the zfs-trim branch July 23, 2021 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@delroth delroth

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Mic92 @delroth @nixos-discourse