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

From: Marius Bakke <[email-masked]>
Date: Sat, 25 Jul 2020 19:00:54 +0200

Efraim Flashner <[email-masked]> 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?

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

> +@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.

> +@item @code{mem_limit} (default @var{"0"})

"mem_limit" should instead be "memory-limit" or just "limit".

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

> +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?
  
> +
> +;;;
> +;;; 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?

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.

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