bug#54783: ZRAM default priority wrong

[Maxim Cournoyer]

Hi,

Josselin Poiret <dev@jpoiret.xyz> writes:

> * gnu/services/linux.scm
> (zram-device-configuration) [priority]: Adapt to use #f or an integer
> from 0 to 32767.  Add sanitizer to warn for the change and delay the
> field.
> (zram-device-configuration->udev-string): Adapt as above.
> * doc/guix.texi (Zram Device Service): Change priority description to
> refer to the Swap Space one, and suggest not leaving the default #f on
> to properly use zram.
> ---
>  doc/guix.texi          | 10 +++++-----
>  gnu/services/linux.scm | 26 +++++++++++++++++++++++---
>  2 files changed, 28 insertions(+), 8 deletions(-)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index b7005f0ef1..31f391357d 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -96,7 +96,7 @@ Copyright @copyright{} 2021 Domagoj Stolfa@*
>  Copyright @copyright{} 2021 Hui Lu@*
>  Copyright @copyright{} 2021 pukkamustard@*
>  Copyright @copyright{} 2021 Alice Brenon@*
> -Copyright @copyright{} 2021 Josselin Poiret@*
> +Copyright @copyright{} 2021, 2022 Josselin Poiret@*
>  Copyright @copyright{} 2021 Andrew Tropin@*
>  Copyright @copyright{} 2021 Sarah Morgensen@*
>  Copyright @copyright{} 2021 Josselin Poiret@*
> @@ -34650,11 +34650,11 @@ 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.: @code{"2G"}.
> -@item @code{priority} (default @code{-1})
> +@item @code{priority} (default @code{#f})
>  This is the priority of the swap device created from the zram device.
> -@code{swapon} accepts values between -1 and 32767, with higher values
> -indicating higher priority.  Higher priority swap will generally be used
> -first.
> +@xref{Swap Space} for a description of swap priorities.  You might want
> +to set a specific priority for the zram device, otherwise it could end
> +up not being used much for the reasons described there.
>  @end table
>  
>  @end deftp
> diff --git a/gnu/services/linux.scm b/gnu/services/linux.scm
> index 2eb02ac5a3..9f598b2826 100644
> --- a/gnu/services/linux.scm
> +++ b/gnu/services/linux.scm
> @@ -4,6 +4,7 @@
>  ;;; Copyright © 2020 Efraim Flashner <efraim@flashner.co.il>
>  ;;; Copyright © 2021 raid5atemyhomework <raid5atemyhomework@protonmail.com>
>  ;;; Copyright © 2021 B. Wilson <elaexuotee@wilsonb.com>
> +;;; Copyright © 2022 Josselin Poiret <dev@jpoiret.xyz>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -21,9 +22,11 @@
>  ;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
>  
>  (define-module (gnu services linux)
> +  #:use-module (guix diagnostics)
>    #:use-module (guix gexp)
>    #:use-module (guix records)
>    #:use-module (guix modules)
> +  #:use-module (guix i18n)
>    #:use-module (gnu services)
>    #:use-module (gnu services base)
>    #:use-module (gnu services shepherd)
> @@ -252,7 +255,19 @@ (define-record-type* <zram-device-configuration>
>    (memory-limit             zram-device-configuration-memory-limit
>                              (default 0))        ; string or integer
>    (priority                 zram-device-configuration-priority
> -                            (default -1)))      ; integer
> +                            (default #f)        ; integer | #f
> +                            (delayed)

I'm curious, what does delaying the field buys us here?  Is it to avoid
printing the warning multiple times when the record is evaluated?

> +                            (sanitize warn-zram-priority-change)))
> +
> +(define-with-syntax-properties
> +  (warn-zram-priority-change (priority properties))
> +  (if (eqv? priority -1)
> +      (begin
> +        (warning (source-properties->location properties)
> +                 (G_ "Using -1 for zram priority is deprecated to align with 
> \
> +the corresponding swap-space field, please use #f from now on.~%"))
> +        #f)
> +      priority))

By convention, a warning message should not be a complete sentence (no
capitalized first letter nor last period) and be short.  To provide a
human friendly hint/message, you could use 'display-hint' (combined with
a more succinct warning).

The rest LGTM.

Maxim