Add writeback-dev=, corresponding to /s/b/z0/backing_dev

systemd / zram-generator

Systemd unit generator for zram devices

MIT License

575 stars 48 forks source link

Add writeback-dev=, corresponding to /s/b/z0/backing_dev #124

Closed nabijaczleweli closed 2 years ago

nabijaczleweli commented 2 years ago

CC: @fignew, please test if this works in your setup

Also, fun fact, the documentation says, i.a. "If admin wants to use incompressible page writeback, they could do via:" – this is a lie! Those are all triggers, not toggles. This is all we can configure.

nabijaczleweli commented 2 years ago

rebased

keszybz commented 2 years ago

Two issues I noticed in testing:

I think this also needs an After=<backing-device>. I didn't test it, but w/o After, we could try to write the name too early, before the device node exists and the kernel can resolve the path to a device. On shutdown, systemd could also try to destroy the backing device before the zram device.
There is a disagreement about escaping: I tried /dev/disk/by-path/virtio-pci-0000:08:00.0, and we escape this as "dev-disk-by\\x2dpath-virtio\\x2dpci\\x2d0000\\x3a08\\x3a00.0.device", but systemd has "dev-disk-by\\x2dpath-virtio\\x2dpci\\x2d0000:08:00.0.device". I didn't yet figure out who misimplemented the spec, but I expect that it doesn't matter: the systemd implementation is probably the de-facto "standard", and even if the docs disagree, they might be adjusted to fit it.

keszybz commented 2 years ago

Oh, no, we get this wrong. OK, so this is an easy fix, we just need to adjust the algorithm that was added recently.

nabijaczleweli commented 2 years ago

Oh, lol, I was pretty sure I got this right, and I did! For systemd 247.3-6 (bullseye): screenshot of man systemd.unit against systemd.unit.xml from the systemd repo Apparently this got fixed in https://github.com/systemd/systemd/commit/aa6dc3ec337b04308a5dfe3b962fa88088b2c82e, which github says landed in v249

keszybz commented 2 years ago

Oh, indeed. So it's as I wrote… systemd was in the wrong, but docs got adjusted ;) For us here that's good, because the code will with new systemd and old both.

nabijaczleweli commented 2 years ago

As for the After=, you're probably right, since we could try to use it before the device is Active (what this strictly entails is beyond me for the time being), but fixed that, too.

nabijaczleweli commented 2 years ago

Rebased

keszybz commented 2 years ago

Seems to work great. I'll go ahead and merge the first three commits so this can get more testing. We can discuss the other two commits, but I'm not in favour, as described above.

nabijaczleweli commented 2 years ago

yeah, these would be gains if the features I wanted were actually stable, the dependency on once-cell is meh since it's being stabilised verbatim, but it's taking time, and writev() would be beneficial since we already have the buffers, but we end up allocating into BufWriter anyway, so maybe Next Year™