Bug 1469726 - Review Request: zram - Enable compressed swap in memory
Review Request: zram - Enable compressed swap in memory
Status: NEW
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
: 1169926 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2017-07-11 14:12 EDT by Raphael Groner
Modified: 2017-08-14 08:25 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Raphael Groner 2017-07-11 14:12:18 EDT
Spec URL: http://raphgro.fedorapeople.org//zram.spec
SRPM URL: http://raphgro.fedorapeople.org//zram-0-0.1.20170206git3a7a278.fc26.src.rpm

Description:
zram compresses swap partitions into RAM for performance.
Comment 1 Raphael Groner 2017-07-11 14:12:22 EDT
This package built on koji:  https://koji.fedoraproject.org/koji/taskinfo?taskID=20463801
Comment 2 Raphael Groner 2017-07-11 14:13:46 EDT
May replace the (stalled?) review in bug #1169926.
Comment 3 Raphael Groner 2017-07-11 14:29:20 EDT
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
Comment 4 František Zatloukal 2017-07-27 09:50:43 EDT
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?
Comment 5 Zbigniew Jędrzejewski-Szmek 2017-08-04 15:25:08 EDT
*** Bug 1169926 has been marked as a duplicate of this bug. ***
Comment 6 Zbigniew Jędrzejewski-Szmek 2017-08-04 15:44:52 EDT
> # 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.
Comment 7 Raphael Groner 2017-08-14 03:03:04 EDT
(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.
Comment 8 Raphael Groner 2017-08-14 03:05:10 EDT
(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.
Comment 9 Zbigniew Jędrzejewski-Szmek 2017-08-14 08:22:01 EDT
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.
Comment 10 Zbigniew Jędrzejewski-Szmek 2017-08-14 08:25:31 EDT
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.

Note You need to log in before you can comment on or make changes to this bug.