GNU bug report logs - #61964
[PATCH] services: Add fstrim-service-type.

Previous Next

Full log

View this message in rfc822 format

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Bruno Victal <mirai <at> makinata.eu>
Cc: 61964 <at> debbugs.gnu.org
Subject: [bug#61964] [PATCH] services: Add fstrim-service-type.
Date: Tue, 21 Mar 2023 22:35:18 -0400

Hi!

Bruno Victal <mirai <at> makinata.eu> writes:

> * gnu/services/linux.scm (fstrim-service-type): New variable.
> (fstrim-mcron-job, serialize-fstrim-configuration)
> (fstrim-serialize-list-of-strings, fstrim-serialize-boolean): New procedure.
> (mcron-time?): New predicate.
> (fstrim-configuration): New record.
> * doc/guix.texi (Linux Services): Document new fstrim-service-type.

Thanks!  This looks nice.

> ---
>  doc/guix.texi          |  62 +++++++++++++++++++++++
>  gnu/services/linux.scm | 109 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 171 insertions(+)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index 74658dbc86..d5a83e387f 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -37436,6 +37436,68 @@ Linux Services
>  @end table
>  @end deftp
>  
> +@cindex fstrim service
> +@cindex solid state drives, periodic trim
> +@cindex solid state drives, trim
> +@subsubheading fstrim Service
> +
> +The command @command{fstrim} can be used to discard (or @dfn{trim})
> +unused blocks on a mounted filesystem.

Please s/filesystem/file system/, which is the preferred spelling in the
GNU project.

> +
> +@c This was copied from the fstrim manpage, with some texinfo touch-ups.
>                                                        Texinfo

> +@quotation Warning
> +Running @command{fstrim} frequently, or even using
> +@command{mount -o discard}, might negatively affect the lifetime of
> +poor-quality SSD devices.  For most desktop and server systems a
> +sufficient trimming frequency is once a week.  Note that not all devices
> +support a queued trim, so each trim command incurs a performance penalty
> +on whatever else might be trying to use the disk at the time.
> +@end quotation
> +
> +@defvar fstrim-service-type
> +Type for a service that periodically runs @command{fstrim}, whose value must
> +be a @code{<fstrim-configuration>} object.  The service can be instantiated
> +in its default configuration with:
> +
> +@lisp
> +(service fstrim-service-type)
> +@end lisp
> +@end defvar
> +
> +@c %start of fragment
> +@deftp {Data Type} fstrim-configuration
> +Available @code{fstrim-configuration} fields are:
> +
> +@table @asis
> +@item @code{package} (default: @code{util-linux}) (type: file-like)
> +The package providing @command{fstrim}.
> +
> +@item @code{schedule} (default: @code{"0 0 * * 0"}) (type: mcron-time)
> +Schedule for launching @command{fstrim}.  This can be a procedure, a
> +list or a string.  For additional information, @pxref{Guile Syntax,, Job
> +specification, mcron,the mcron manual}.  By default this is set to run
> +weekly on Sunday at 00:00.

pxref is supposed to be used in between parentheses (Parenthetical
Cross-Reference); I think you can use just "see: @ref{...}" instead,
without parentheses.

> +@item @code{listed-in} (default: @code{("/etc/fstab" "/proc/self/mountinfo")}) (type: maybe-list-of-strings)
> +List of files in fstab or kernel mountinfo format.  All missing or empty
> +files are silently ignored.  The evaluation of the list @emph{stops}
> +after the first non-empty file.  Filesystems with @code{X-fstrim.notrim}

File systems

> +mount option in fstab are skipped.
> +
> +@item @code{verbose?} (default: @code{#t}) (type: boolean)
> +Verbose execution.
> +
> +@item @code{quiet-unsupported?} (default: @code{#t}) (type: boolean)
> +Suppress error messages if trim operation (ioctl) is unsupported.
> +
> +@item @code{extra-arguments} (type: maybe-list-of-strings)
> +Extra options to append to @command{fstrim} command.@footnote{Run
> +@command{man fstrim} for more information.}

I think @command is to denote a single command, not a command line
(command + arguments); I'd use @samp{man fstrim} instead, and replace
the footnote by (see @samp{man fstrim} for more information).

> +@end table
> +@end deftp
> +@c %end of fragment
> +
>  @cindex modprobe
>  @cindex kernel module loader
>  @subsubheading Kernel Module Loader Service
> diff --git a/gnu/services/linux.scm b/gnu/services/linux.scm
> index 60e2093e1d..f5ec5fec48 100644
> --- a/gnu/services/linux.scm
> +++ b/gnu/services/linux.scm
> @@ -5,6 +5,7 @@
>  ;;; Copyright © 2021 raid5atemyhomework <raid5atemyhomework <at> protonmail.com>
>  ;;; Copyright © 2021 B. Wilson <elaexuotee <at> wilsonb.com>
>  ;;; Copyright © 2022 Josselin Poiret <dev <at> jpoiret.xyz>
> +;;; Copyright © 2023 Bruno Victal <mirai <at> makinata.eu>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -30,12 +31,15 @@ (define-module (gnu services linux)
>    #:use-module (guix ui)
>    #:use-module (gnu services)
>    #:use-module (gnu services base)
> +  #:use-module (gnu services configuration)
> +  #:use-module (gnu services mcron)
>    #:use-module (gnu services shepherd)
>    #:use-module (gnu packages linux)
>    #:use-module (srfi srfi-1)
>    #:use-module (srfi srfi-26)
>    #:use-module (srfi srfi-34)
>    #:use-module (srfi srfi-35)
> +  #:use-module (ice-9 format)
>    #:use-module (ice-9 match)
>    #:export (earlyoom-configuration
>              earlyoom-configuration?
> @@ -50,6 +54,16 @@ (define-module (gnu services linux)
>              earlyoom-configuration-send-notification-command
>              earlyoom-service-type
>  
> +            fstrim-configuration
> +            fstrim-configuration?
> +            fstrim-configuration-package
> +            fstrim-configuration-schedule
> +            fstrim-configuration-listed-in
> +            fstrim-configuration-verbose?
> +            fstrim-configuration-quiet-unsupported?
> +            fstrim-configuration-extra-arguments
> +            fstrim-service-type
> +
>              kernel-module-loader-service-type
>  
>              rasdaemon-configuration
> @@ -150,6 +164,101 @@ (define earlyoom-service-type
>                               (compose list earlyoom-shepherd-service))))
>     (description "Run @command{earlyoom}, the Early OOM daemon.")))
>  
> +
> +;;;
> +;;; fstrim
> +;;;
> +
> +(define (mcron-time? x)
> +  (or (procedure? x) (string? x) (list? x)))
> +
> +(define-maybe list-of-strings (prefix fstrim-))
> +
> +(define (fstrim-serialize-boolean field-name value)
> +  (list (format #f "~:[~;--~a~]" value
> +                ;; drop trailing '?' character

Use full sentence for standalone comment (;; Drop [...] character.)

> +                (string-drop-right (symbol->string field-name) 1))))
> +
> +(define (fstrim-serialize-list-of-strings field-name value)
> +  (list (string-append "--" (symbol->string field-name))
> +        #~(string-join '#$value ":")))
> +
> +(define-configuration fstrim-configuration
> +  (package
> +   (file-like util-linux)
> +   "The package providing @command{fstrim}."

providing the @command{fstrim} command.

> +   empty-serializer)
> +
> +  (schedule
> +   (mcron-time "0 0 * * 0")
> +   "Schedule for launching @command{fstrim}. This can be a procedure, a list
> +or a string. For additional information, @pxref{Guile Syntax,,
> +Job specification, mcron, the mcron manual}. By default this is set to run
> +weekly on Sunday at 00:00."
> +   empty-serializer)

From here on, the text started to use single sentence spacing.  Please
make it double sentence spacing.

> +  ;; fstrim options
> +  (listed-in
> +   (maybe-list-of-strings '("/etc/fstab" "/proc/self/mountinfo"))
> +   ;; XXX: documentation sourced from the fstrim manpage.

What is "dirty" about the above comment?  I'd just use ;; Note: [...].

> +   "List of files in fstab or kernel mountinfo format. All missing or
> +empty files are silently ignored. The evaluation of the list @emph{stops}
> +after the first non-empty file. Filesystems with @code{X-fstrim.notrim} mount
> +option in fstab are skipped.")

File systems.

> +
> +  (verbose?
> +   (boolean #t)
> +   "Verbose execution.")
> +
> +  (quiet-unsupported?
> +   (boolean #t)
> +   "Suppress error messages if trim operation (ioctl) is unsupported.")
> +
> +  (extra-arguments
> +   maybe-list-of-strings
> +   ;; FIXME <at> GUILE(TEXINFO): @footnote causes errors when calling
> +   ;;                       configuration->documentation.
> +   ;; > Throw to key `parser-error' with args `(#f "Unknown command" footnote)'

Please take the time to report the issue upstream (bug-guile <at> gnu.org)
and link to it here.

> +   "Extra options to append to @command{fstrim} command.@footnote{Run
> +@command{man fstrim} for more information.}"
> +   (lambda (_ value)
> +     (if (maybe-value-set? value)
> +         value '())))
> +
> +  (prefix fstrim-))
> +
> +(define (serialize-fstrim-configuration config)
> +  (concatenate
> +   (filter list?
> +           (map (lambda (field)
> +                  ((configuration-field-serializer field)
> +                   (configuration-field-name field)
> +                   ((configuration-field-getter field) config)))
> +                fstrim-configuration-fields))))
> +
> +(define (fstrim-mcron-job config)
> +  (match-record config <fstrim-configuration> (package schedule)
> +    #~(job
> +       ;; XXX: The “if” below is to ensure that
> +       ;; lists are ungexp'd correctly since @var{schedule}
> +       ;; can be either a procedure, a string or a list.

I'd turn the XXX into a 'Note' here as well.  XXX is for ugly hacks that
should be eventually replaced with something more elegant, when someone
finds a way to do so.

> +       #$(if (list? schedule)
> +             `(list ,@schedule)
> +             schedule)
> +       (lambda ()
> +         (system* #$(file-append package "/sbin/fstrim")
> +                  #$@(serialize-fstrim-configuration config)))
> +       "fstrim")))
> +
> +(define fstrim-service-type
> +  (service-type
> +   (name 'fstrim)
> +   (extensions
> +    (list (service-extension mcron-service-type
> +                             (compose list fstrim-mcron-job))))
> +   (description "Discard unused blocks from filesystems.")

I think the main takeaway from my review is this: file systems!  Eh.
More seriously, thanks, this looks good!

-- 
Thanks,
Maxim

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.