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

Add options for swap discarding (TRIM) #108948

Closed
wants to merge 2 commits into from
Closed

Add options for swap discarding (TRIM) #108948

wants to merge 2 commits into from

Conversation

viric
Copy link
Member

@viric viric commented Jan 10, 2021 •
edited

Motivation for this change

swap on ssd will benefit from TRIM/discard so the writes to the ssd go faster with less wearing.

It works in my encrypted swap0.

lsblk -D:
sdb                    0      512B       2G         0
├─sdb1                 0      512B       2G         0
├─sdb2                 0      512B       2G         0
├─sdb3                 0      512B       2G         0
│ └─root               0      512B       2G         0
└─sdb4                 0      512B       2G         0
  └─swap0              0      512B       2G         0
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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.

@bb2020
Copy link
Member

bb2020 commented Jan 10, 2021 •
edited

Isn't this change the same as adding following configuration? (unless swap partition is in a different device)

fileSystems."swap".options = [ "discard" ];
boot.initrd.luks.devices."cryptroot".allowDiscards = true;

If TRIM support is not properly implemented by an SSD vendor, there is a risk of data loss. There should be a
warning if this PR is merged. Also enabling TRIM on an encrypted area is a potential security concern.
This has been explained here very well: http://asalor.blogspot.com/2011/08/trim-dm-crypt-problems.html.

symphorien
symphorien reviewed Jan 10, 2021
View reviewed changes
nixos/modules/tasks/encrypted-devices.nix Outdated
allowDiscards = mkOption {
default = false;
type = types.bool;
description = "cryptsetup argument --allow-discard on open";
Copy link
Member

Choose a reason for hiding this comment

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

this explains how it works but not what it does.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now I copied the allowDiscards description from luksroot. Thank you!

@viric
Rework description of options
403c61f 
I copied allowDiscards from luksroot
@viric
Copy link
Member Author

viric commented Jan 10, 2021

Isn't this change the same as adding following configuration? (unless swap partition is in a different device)

fileSystems."swap".options = [ "discard" ];
boot.initrd.luks.devices."cryptroot".allowDiscards = true;

Only luksroot had allowDiscards, not the general 'encrypted-devices.nix'. I didn't know I could override options for swap that way either. Does that work? Maybe it should say 'swap0'?

I'm using this change this way:

  swapDevices = [ {
       device = "/dev/mapper/swap0";
       discard = true;
       encrypted.enable = true;
       encrypted.blkDev = "/dev/disk/by-id/ata-WDC_WDS120G2G0A-00JH30_1806BB803081-part4";
       encrypted.label = "swap0";
       encrypted.allowDiscards = true;
       } ];

If TRIM support is not properly implemented by an SSD vendor, there is a risk of data loss. There should be a
warning if this PR is merged. Also enabling TRIM on an encrypted area is a potential security concern.
This has been explained here very well: http://asalor.blogspot.com/2011/08/trim-dm-crypt-problems.html.

I agree. I copied the documentation from luksroot.nix

@bb2020
Copy link
Member

bb2020 commented Jan 10, 2021

Only luksroot had allowDiscards, not the general 'encrypted-devices.nix'. I didn't know I could override options for swap that way either. Does that work? Maybe it should say 'swap0'?

You should change the names according to your partition naming. I haven't tested it, but it should work. fsType = "swap" attribute should be valid for fileSystems option.

Is it a good idea to mount swap with discard? It may stall disk operations. I would prefer mounting / with discard and running periodic fstrim on swap area.

@viric
Copy link
Member Author

viric commented Jan 11, 2021 via email

symphorien
symphorien reviewed Jan 13, 2021
View reviewed changes
nixos/modules/config/swap.nix
with --allow-discards.

If you use encrypted.enable for this swap device, you should enable
there encrypted.allowDiscards.
Copy link
Member

Choose a reason for hiding this comment

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

this could be done automatically, right ?

Copy link
Member Author

Choose a reason for hiding this comment

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

hm I'm not sure how. Any suggestion?

@ajs124
Copy link
Member

ajs124 commented Feb 26, 2021

How does this relate to #107728?

@bb2020
Copy link
Member

bb2020 commented Feb 26, 2021

How does this relate to #107728?

Oh, that looks like the official way to achieve this.

@kurnevsky
Copy link
Member

Looks like changes in #107728 will work only with unencrypted swap, so discards still should be enabled in in luks.

@bb2020
Copy link
Member

bb2020 commented Jul 6, 2021

It is good to see #107728 is merged.

Looks like changes in #107728 will work only with unencrypted swap, so discards still should be enabled in in luks.

I suppose you can use LVM to create swap and root areas on a cryptroot LUKS partition and then use this to enable discard on both. boot.initrd.luks.devices."cryptroot".allowDiscards = true;

TRIM on LUKS should not be enabled by default as there are security implications. It is explained here. It should be up to the user.

@kurnevsky
Copy link
Member

kurnevsky commented Jul 6, 2021 •
edited

Here I added PR for this: #129408

I suppose you can use LVM to create swap and root areas on a cryptroot LUKS partition and then use this to enable discard on both.

I'd prefer not to use LVM as I don't need it and use btrfs subvolumes instead.

TRIM on LUKS should not be enabled by default as there are security implications. It is explained here. It should be up to the user.

Right, it's disabled by default via discardPolicy. With my PR it will be enabled only if user specify this option explicitly.

@Artturin
Copy link
Member

superseded by #107728 and #129408

additionally #128032 will allow people to set their own swap options

@Artturin Artturin closed this Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@symphorien symphorien symphorien left review comments

Assignees
No one assigned
Labels
2.status: merge conflict 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@viric @bb2020 @ajs124 @kurnevsky @Artturin @symphorien @SuperSandro2000