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

zramSwap: allow configure compression algorithm + cleanups #52991

Open
wants to merge 2 commits into
base: master
from

Conversation

@danbst
Copy link
Contributor

danbst commented Dec 27, 2018

Motivation for this change

As I mentioned in https://discourse.nixos.org/t/install-nixos-on-512-mb-ram/1713, I'm using zstd compression, but current module doesn't allow to change compressor declaratively. Here we go, and fix few problems with this module.

Things done
  • add zramSwap.algorithm option, which allows to change compressor
    declaratively
  • some documentation changes
  • replaced /sys/block/zram* handling with zramctl, because I had occasional
    "Device is busy" error (looks like zram has to be configured in predefined order)
  • added memoryPercent and algorithm as restart triggers. I think, it was
    a bug that changing memoryPercent in configuration wasn't applied immediately.
  • removed a bind to .swap device. While it looks natural (when swap device goes
    off, so should zram device), it wasn't implemented properly. This caused problems
    with swapon/swapoff:
$ cat /proc/swaps
Filename                                Type            Size    Used    Priority
/dev/zram0                              partition       8166024 0       -2
/var/swapfile                           file            5119996 5120    1

$ sudo swapoff -a

$ sudo swapon -a
swapon: /dev/zram0: read swap header failed

$ cat /proc/swaps
Filename                                Type            Size    Used    Priority
/var/swapfile                           file            5119996 0       1
  • 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

cc @Mic92

@worldofpeace

This comment has been minimized.

Copy link
Member

worldofpeace commented Dec 27, 2018

I'm using zstd compression, but current module doesn't allow to change compressor declaratively.

I came across this also, and what I was doing for a while was that I added an algorithm option but used an additional udev rule to implement it

services.udev.extraRules = ''
  KERNEL=="zram[0-9]*", ENV{SYSTEMD_WANTS}="zram-init-%k.service", TAG+="systemd", ATTR{comp_algorithm}="${cfg.algorithm}"
'';

I also think there's a disksize attribute as well.

@danbst

This comment has been minimized.

Copy link
Contributor

danbst commented Dec 28, 2018

@worldofpeace yeah, according to gist https://gist.github.com/asssaf/f9bef956a755e67ed742e3265765e48d it is possible to do only with udev. However I'm not sure udev solution will apply required changes when options are changed. For example, when you change compressor and rebuild system, does it rebuild zram devices with new algo?

@worldofpeace

This comment has been minimized.

Copy link
Member

worldofpeace commented Dec 28, 2018

@worldofpeace yeah, according to gist https://gist.github.com/asssaf/f9bef956a755e67ed742e3265765e48d it is possible to do only with udev. However I'm not sure udev solution will apply required changes when options are changed. For example, when you change compressor and rebuild system, does it rebuild zram devices with new algo?

It would rebuild the udev rules and I'm pretty sure those are monitored for changes so it would just reapply them once changed.

@danbst

This comment has been minimized.

Copy link
Contributor

danbst commented Dec 28, 2018

Also, just learned that numDevices description (which I've added) is not actual since linux 3.16 (sick). So maybe I should go and deprecate numDevices altogether (we don't do per-device configuration).

@danbst danbst changed the title zramSwap: allow configure compression algorithm + cleanups [WIP] zramSwap: allow configure compression algorithm + cleanups Dec 28, 2018

@danbst danbst force-pushed the danbst:zram-zstd branch from d8ff5ec to 7d0b691 Jan 1, 2019

@danbst danbst changed the title [WIP] zramSwap: allow configure compression algorithm + cleanups zramSwap: allow configure compression algorithm + cleanups Jan 1, 2019

@danbst

This comment has been minimized.

Copy link
Contributor

danbst commented Jan 2, 2019

Alright, I've fixed things up.

  • one more old race condition hunted!
  • replaced integer math with floating math!
  • introduced swapDevices. In general, zram and swap are not same, you can allocate zram device and use it as regular block device, including compressed tmpfs usage. So, one can set 1 device for swap, and 1 for other things. Previously it required lots of hacking.
  • for those who use several zram devices added a warning that this may be misusage. The warning can be disabled by explicitly setting zramSwap.swapDevices.
  • documentation, documentation
  • and finally, zstd is default now!

cc for review @worldofpeace @Infinisil @bjornfor @wmertens @wizeman

@nixos-discourse

This comment has been minimized.

Copy link

nixos-discourse commented Jan 2, 2019

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/install-nixos-on-512-mb-ram/1713/13

description = ''
Compression algorithm. <literal>lzo</literal> has good compression,
but is slow. <literal>lz4</literal> has bad compression, but is fast.
<literal>zstd</literal> is both good compression and fast (kernel 4.18+).

This comment has been minimized.

@Mic92

Mic92 Jan 14, 2019

Contributor

There could be an assertion generated if the kernel is too old (config.boot.kernelPackages.kernel.version)

This comment has been minimized.

@danbst

danbst Jan 17, 2019

Contributor

I'd better remove this section. Current kernel is already capable of zstd zram.

totalmem=$(${pkgs.gnugrep}/bin/grep 'MemTotal: ' /proc/meminfo | ${pkgs.gawk}/bin/awk '{print $2}')
mem=$(((totalmem * ${toString cfg.memoryPercent} / 100 / ${toString cfg.numDevices}) * 1024))
totalmem=$(${pkgs.gawk}/bin/awk '/MemTotal: /{print $2}' /proc/meminfo)
mem=$(echo "$totalmem*${toString cfg.memoryPercent}/100.0/${toString devicesCount}*1024" | ${bc})

This comment has been minimized.

@Mic92

Mic92 Jan 14, 2019

Contributor

Do we depend on bc in the minimal closure?

This comment has been minimized.

@Mic92

Mic92 Jan 14, 2019

Contributor

Otherwise gawk could also perform floating point calculation I think.

zramSwap: allow configure compression algorithm + cleanups
- add `zramSwap.algorithm` option, which allows to change compressor
declaratively. zstd as default
- add `zramSwap.swapDevices` option, which allows to define how many zram
devices will be used as swap. Rest devices can be managed freely
- simpler floating calculations
- fix udev race condition
- some documentation changes
- replaced `/sys/block/zram*` handling with `zramctl`, because I had occasional
"Device is busy" error (looks like zram has to be configured in predefined order)
- added `memoryPercent` and `algorithm` as restart triggers. I think, it was
a bug that changing `memoryPercent` in configuration wasn't applied immediately.
- removed a bind to .swap device. While it looks natural (when swap device goes
off, so should zram device), it wasn't implemented properly. This caused problems
with swapon/swapoff:
```
$ cat /proc/swaps
Filename                                Type            Size    Used    Priority
/dev/zram0                              partition       8166024 0       -2
/var/swapfile                           file            5119996 5120    1

$ sudo swapoff -a

$ sudo swapon -a
swapon: /dev/zram0: read swap header failed

$ cat /proc/swaps
Filename                                Type            Size    Used    Priority
/var/swapfile                           file            5119996 0       1
```

@danbst danbst force-pushed the danbst:zram-zstd branch from 862f2fc to 8d8a721 Jan 17, 2019

@danbst

This comment has been minimized.

Copy link
Contributor

danbst commented Jan 17, 2019

@Mic92 addressed your issues. Also, added some release notes.

I'll merge in a week if nobody beats me

@danbst danbst referenced this pull request Jan 17, 2019

Open

Unit ordering cycle #47474

zramSwap: remove basic.target for zram devices
This creates a dependency cycle when used with boot.tmpOnTmpfs:
basic.target <- tmp.mount <- swap.target <- zram-init-dev0 <- basic.target

This same fix is done already for tmp.mount

Fixes #47474
@xaverdh

This comment has been minimized.

Copy link
Contributor

xaverdh commented Jan 17, 2019

I can confirm, that the patch resolves the dependency cycle.

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