Bug 1469726

This package built on koji:  https://koji.fedoraproject.org/koji/taskinfo?taskID=20463801

May replace the (stalled?) review in bug #1169926.

In case of any relevance, I explicitly decided to use the package name zram instead of the upstream name systemd-zram because this project is not part of the systemd core and so to avoid any confusion, though guidelines [1] miss a special part for systemd addons as we have for other base packages.
[1] https://fedoraproject.org/wiki/Packaging:Naming?rd=Packaging:NamingGuidelines#Addon_Packages

Packaging looks good to me.

I see upstream recommends disabling swap after installation but I don't see the package doing it. Is it an issue if swap remains enabled? It would make sense to at least print warning to the terminal during installation (I'd be against disabling swap in a completely automatic manner).

The other potential issue - I see documented incompatibility with encrypted /home . Does it break anything after installing this package to such system?

*** Bug 1169926 has been marked as a duplicate of this bug. ***

> # need Linux kernel version 2.6.37.1 or better to use zram
> Requires: kernel >= 2.6.37.1

First, nowadays kernel has been split into kernel-core and other subpackages, so you can easily boot a machine without kernel.rpm (which also pulls in kernel-modules). Second, the fact that you have a specific kernel.rpm installed does not mean that you're running with that version so this is not effective. And third, there's no way to install a kernel rpm for 2.6.x in any Fedora. So you should just drop this line.

> BuildRequires: systemd-units

> Requires(post): systemd-sysv
> Requires(post): systemd-units
> Requires(preun): systemd-units
> Requires(postun): systemd-units
> Requires: systemd
> Requires: initscripts

This should be replaced by
%{?systemd_requires}
BuildRequires: systemd
[https://fedoraproject.org/wiki/Packaging:Scriptlets#Systemd]

> %systemd_postun_with_restart mkzram.service
Hmm, that could be dangerous. Disabling swap during an upgrade could bring the machine down. %systemd_postun seems more reasonable.

> After=multi-user.target
That's certainly the wrong place to order this. Not only it causes a dependency loop with swap.target which is ordered much earlier, it might also disable swap at the beginning of shutdown, crashing the machine.

> sleep 1
Hmm. swapoff is synchronous so this is either masking a bug in the kernel or just plain unnecessary.

I'm not particularly impressed by how this is all put together. The problem starts with the stupid kernel interface which requires ~50 lines of shell to bring up swap. That's just bizarre. But anyway, zram is useful, so we should make it easy to use, so I think it's worth to package this, so it'd be great to finally get this package into Fedora.

(In reply to Zbigniew Jędrzejewski-Szmek from comment #6)
> > # need Linux kernel version 2.6.37.1 or better to use zram
> > Requires: kernel >= 2.6.37.1
> 
> First, nowadays kernel has been split into kernel-core and other
> subpackages, so you can easily boot a machine without kernel.rpm (which also
> pulls in kernel-modules). Second, the fact that you have a specific
> kernel.rpm installed does not mean that you're running with that version so
> this is not effective. And third, there's no way to install a kernel rpm for
> 2.6.x in any Fedora. So you should just drop this line.

OK. Good point about several kernels available to boot on a system.

> > BuildRequires: systemd-units
> 
> > Requires(post): systemd-sysv
> > Requires(post): systemd-units
> > Requires(preun): systemd-units
> > Requires(postun): systemd-units
> > Requires: systemd
> > Requires: initscripts
> 
> This should be replaced by
> %{?systemd_requires}
> BuildRequires: systemd
> [https://fedoraproject.org/wiki/Packaging:Scriptlets#Systemd]

OK

> > %systemd_postun_with_restart mkzram.service
> Hmm, that could be dangerous. Disabling swap during an upgrade could bring
> the machine down. %systemd_postun seems more reasonable.

OK

> > After=multi-user.target
> That's certainly the wrong place to order this. Not only it causes a
> dependency loop with swap.target which is ordered much earlier, it might
> also disable swap at the beginning of shutdown, crashing the machine.

Drop it?

> > sleep 1
> Hmm. swapoff is synchronous so this is either masking a bug in the kernel or
> just plain unnecessary.

Drop it?

> I'm not particularly impressed by how this is all put together. The problem
> starts with the stupid kernel interface which requires ~50 lines of shell to
> bring up swap. That's just bizarre. But anyway, zram is useful, so we should
> make it easy to use, so I think it's worth to package this, so it'd be great
> to finally get this package into Fedora.

How can they talk on reddit about available systemd integration without an official package? Is this review still needed?
https://www.reddit.com/r/Fedora/comments/4y6n5t/how_to_enable_zram_on_fedora_24/

What about storaged-zram? Is it stilled developed? I see it's part of general storaged, merged into systemd or somehow actual plans to merge.

(In reply to František Zatloukal from comment #4)
> Packaging looks good to me.
> 
> I see upstream recommends disabling swap after installation but I don't see
> the package doing it. Is it an issue if swap remains enabled? It would make
> sense to at least print warning to the terminal during installation (I'd be
> against disabling swap in a completely automatic manner).

OK

> The other potential issue - I see documented incompatibility with encrypted
> /home . Does it break anything after installing this package to such system?

Please provide any link for more information and reproducibility.

Oh, /usr/lib/systemd/system/zram.service is part of anaconda. It seems to be tailored for anaconda (see the part at the end about configuring /tmp).
It also has no support for enabling (hence the reddit thread).
So it seems that this package is still useful, and in the long run anaconda should switch to depending on it.


>>> After=multi-user.target
> Drop it?

Yes. I think the following should work:
[Unit]
DefaultDependencies=no
Before=swap.target
Conflicts=umount.target

[Install]
WantedBy=swap.target

>>> sleep 1
>> Hmm. swapoff is synchronous so this is either masking a bug in the kernel or
>> just plain unnecessary.
> Drop it?
Yeah. If anything breaks, there's a bug somewhere.

Hmm, it seems that the anaconda service is smarter than the one being packaged here. Instead of making one swap device per cpu, it just does "echo $num_cpus > /sys/block/zram0/max_comp_streams". If this is updated, not much is left of the original, I don't know what you want to do with that.

Thanks for your comments. Will update soonish, please be still patient.

Raphael do you plan to continue?

Less time available for development currently and so I can't promise anything, sorry. Do you have interest to take over for this request? We could close here and please feel free to take this as a base for your new request. You can add me as co-maintainer after imported the package.

Just fyi, seems like several people are currently interested in zram on Fedora:
https://wiki.gnome.org/Hackfests/Performance2018/Agenda
https://github.com/systemd/systemd/issues/8990
https://fedoraproject.org/wiki/Changes/ZRAMforARMimages

I'm going to drive this, feel free to add feedback to the new review.

> https://wiki.gnome.org/Hackfests/Performance2018/Agenda
> https://github.com/systemd/systemd/issues/8990
> https://fedoraproject.org/wiki/Changes/ZRAMforARMimages

I was involved in all of the above.

*** This bug has been marked as a duplicate of bug 1602846 ***

(In reply to Teppo Turtiainen from comment #14)
> Just fyi, seems like several people are currently interested in zram on
> Fedora:
> https://wiki.gnome.org/Hackfests/Performance2018/Agenda
> https://github.com/systemd/systemd/issues/8990
> https://fedoraproject.org/wiki/Changes/ZRAMforARMimages

See also bug #1469767.