[bug#42478] [PATCH] services: Add zram-device-service.

[Efraim Flashner]

On Sat, Jul 25, 2020 at 07:00:54PM +0200, Marius Bakke wrote:
> Efraim Flashner <efraim@flashner.co.il> writes:
> 
> > * gnu/services/linux.scm (<zram-device-configuration>): New record.
> > (zram-device-service-type): New variable.
> > * doc/guix.texi (Linux Services): Document it.
> > * tests/services/linux.scm (zram-device-configuration->udev-string): New
> > test.
> 
> [...]
> 
> > diff --git a/doc/guix.texi b/doc/guix.texi
> > index 8696a9b554..f656c31fab 100644
> > --- a/doc/guix.texi
> > +++ b/doc/guix.texi
> > @@ -27127,6 +27127,51 @@ parameters, can be done as follow:
> >  @end lisp
> >  @end deffn
> >  
> > +@cindex zram
> > +@cindex compressed swap
> > +@cindex Compressed RAM-based block devices
> > +@subsubheading Zram Device Service
> > +
> > +The Zram device service provides a compressed swap device in system
> > +memory.  The Linux Kernel documentation has more information about
> > +@uref{https://www.kernel.org/doc/html/latest/admin-guide/blockdev/zram.html,zram}
> > +devices.
> > +
> > +@deffn {Scheme Variable} zram-device-service-type
> > +This service creates the zram block device, formats it as swap and
> > +enables it as a swap device.  The service's value is a
> > +@code{zram-device-configuration} record.
> > +
> > +@deftp {Data Type} zram-device-configuration
> > +This is the data type representing the configuration for the zram-device
> > +service.
> > +
> > +@table @asis
> > +@item @code{disksize} (default @var{"0"})
> > +This is the amount of space you wish to provide for the zram device.  It
> > +accepts a string and can be a number of bytes or use a suffix, eg.:
> > +@var{2G}.
> 
> Perhaps this could accept both an integer and a string?  What does a
> size 0 device do, would it make sense to not have a default here?

An integer or a string would certainly be more convenient for users, it
just needs a bit more logic to add in number->string as needed.

A size 0 device is pretty useless. It still creates the zram device but
with a size of 0. I put in 0 as the default because that's the default
if you don't choose anything when creating it. I suppose it would be
better to make the default 1G or 1024**3

> 
> Also, would it make sense to name it "size" instad of "disksize"?

That's the internal name but I'm not opposed to changing it to 'size'.

> 
> > +@item @code{comp_algorithm} (default @var{"lzo"})
> > +This is the compression algorithm you wish to use.  It is difficult to
> > +list all the possible compression options, but common ones supported by
> > +Guix's Linux Libre Kernel include @var{lzo}, @var{lz4} and @var{zstd}.
> 
> "comp_algorithm" is not very idiomatic :-)
> 
> Either "compression-algorithm" or just "compression" IMO.  I'd also
> prefer a symbol instead of a string (or both!), but no strong opinion.

compression-algorithm does seem better. I'll change it to a symbol.

> 
> > +@item @code{mem_limit} (default @var{"0"})
> 
> "mem_limit" should instead be "memory-limit" or just "limit".

I like memory-limit, limit would make me wonder what the difference is
between this an size.

> 
> Accepting an integer here too would be nice!  :-)

Noted :)

> 
> > +This is the maximum amount of memory which the zram device can use.
> > +Setting it to '0' disables the limit.  While it is generally expected
> > +that compression will be 2:1, it is possible that uncompressable data
> > +can be written to swap and this is a method to limit how much memory can
> > +be used.  It accepts a string and can be a number of bytes or use a
> > +suffix, eg.: @var{2G}.
> > +@item @code{priority} (default @var{"-1"})
> 
> Just an integer I suppose?

It would make more sense than a string.

>   
> > +
> > +;;;
> > +;;; Zram swap device.
> > +;;;
> > +
> > +(define zram-device-configuration->udev-string
> > +  (@@ (gnu services linux) zram-device-configuration->udev-string))
> 
> Would it make sense to export this, or is it strictly for internal use?

I'm not opposed to exporting it but I'm not really sure what else you'd
do with it. I think it would be nice to not hardcode zram0 so there can
be more than one or to make it so it can be mounted as /tmp but there's
nothing currently exposed as a variable to change how it works.

> 
> Great that you were able to add unit tests for the functionality.  I
> think in this case we could also have a system test that checks that a
> zram device was created?  But it can come later.

I think it'd be good to add a system test to make sure there's actually
42 MB of swap or something. I'll have to think about how to do that.

> 
> Other than the cosmetic/idiomatic issues looks great to me!

Thanks

-- 
Efraim Flashner   <efraim@flashner.co.il>   אפרים פלשנר
GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted

signature.asc
Description: PGP signature