[bug#37305] [PATCH V2] Allow booting from a Btrfs subvolume.

["Ludovic Courtès" <ludo@gnu.org>]

Hi Maxim!

Great to see Btrfs support improving; many people will love that!

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

> From 3640bea548826e1c1ec9b766da1fdfe4791d7452 Mon Sep 17 00:00:00 2001
> From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
> Date: Sun, 17 Nov 2019 06:01:00 +0900
> Subject: [PATCH 1/9] gnu: tests: Reduce the time required to run the system
>  tests.
>
> When setting the GUIX_DEV_HACKS environment variable, the Guix package used
> inside the instrumented VMs recycles the binaries already found in the Guix
> checkout of the developer instead of rebuilding Guix from scratch.  This
> brings the time required for this component from 20+ minutes down to 2-3
> minutes on an X200 machine.
>
> * gnu/packages/package-management.scm (current-guix/pre-built): New procedure.
> * build-aux/run-system-tests.scm (tests-for-channel-instance): Use it, when
> GUIX_DEV_HACKS is defined.

I understand the need, but I’d really like to avoid that; it’s too
fragile IMO.

But I have good news!  First, commit
887fd835a7c90f720d36a211478012547feaead0 really improved things by
avoiding the full ‘guix’ package rebuild (and we’re only talking about
the installation tests; other tests are just fine.)  Second, there are
improvements to Guile that will appear in 3.0.1/2.2.7 that make
compilation of big files roughly twice as fast.

There’s still room for improvement, but I’d rather work in those
directions.  WDYT?

> From 97d8a635eba34c7cf0708e99bf77ef9bad1344bf Mon Sep 17 00:00:00 2001
> From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
> Date: Tue, 11 Feb 2020 12:57:29 -0500
> Subject: [PATCH 2/9] gnu: linux-boot: Ensure volatile root is mounted
>  read-only.
>
> * gnu/build/linux-boot.scm (mount-root-file-system): Ensure MS_RDONLY is
> present among the root file system flags when VOLATILE-ROOT? is #t.

(You can drop the “gnu:” prefix.)

OK!

> From e8d6642d3597207657842c9ca4849f8660d06638 Mon Sep 17 00:00:00 2001
> From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
> Date: Tue, 11 Feb 2020 23:56:45 -0500
> Subject: [PATCH 3/9] file-systems: Add a 'file-system-device->string'
>  procedure.
>
> * gnu/system/file-systems.scm (file-system-device->string): New procedure.
> * gnu/system.scm (bootable-kernel-arguments): Use it.
> * gnu/system/vm.scm (operating-system-uuid): Likewise.
> * guix/scripts/system.scm (display-system-generation): Likewise.

OK!

> From 4f6e3955957beb5287e9d5a5d33b74725836e1ac Mon Sep 17 00:00:00 2001
> From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
> Date: Tue, 11 Feb 2020 14:00:06 -0500
> Subject: [PATCH 4/9] gnu: linux-boot: Refactor boot-system.
>
> The --root option can now be omitted, and inferred from the root file system
> declaration instead.
>
> * gnu/build/linux-boot.scm (boot-system): Remove nested definitions for
> root-fs-type, root-fs-flags and root-fs-options, and bind those inside the
> let* instead.  Make "--root" take precendence over the device field string
> representation of the root file system.
> * doc/guix.texi (Initial RAM Disk): Document that "--root" can be left
> unspecified.

[...]

>  @item --root=@var{root}
> -Mount @var{root} as the root file system.  @var{root} can be a
> -device name like @code{/dev/sda1}, a file system label, or a file system
> -UUID.
> +Mount @var{root} as the root file system.  @var{root} can be a device
> +name like @code{/dev/sda1}, a file system label, or a file system UUID.
> +When unspecified, the device name from the root file system of the
> +operating system declaration is used.

Oh!  Does it always work?  That makes me wonder why we’ve been carrying
‘--root’ and I’m not sure if I’m forgetting a good reason to do it that
way.

>        (mount-essential-file-systems)
>        (let* ((args    (linux-command-line))
>               (to-load (find-long-option "--load" args))
> -             (root    (find-long-option "--root" args)))
> +             (root-fs (find root-mount-point? mounts))
> +             (root-fs-type (or (and=> root-fs file-system-type)
> +                               "ext4"))
> +             (root-device (and=> root-fs file-system-device))
> +             (root-device-str (and=> root-device file-system-device->string))
> +             ;; --root takes precedence over the 'device' field of the root
> +             ;; <file-system> record.
> +             (root (or (find-long-option "--root" args)
> +                       root-device-str))
> +             (root-fs-flags (mount-flags->bit-mask
> +                             (or (and=> root-fs file-system-flags)
> +                                 '())))
> +             (root-fs-options (if root-fs
> +                                  (file-system-options root-fs)
> +                                  '()))
> +             (root-options (if (null? root-fs-options)
> +                               #f
> +                               (file-system-options->str
> +                                root-fs-options))))

Since ‘file-system-device->string’ is lossy, I think we should do it the
other way around: convert the ‘--root’ string to a <file-system>.
Perhaps that bit can be moved to a separate procedure, like
‘root-string->file-system’.

Also, the (or … "ext4") bit doesn’t sound great, but perhaps it’ll be
unnecessary if we do with something as outlined above?

> From af61745d8b686755a5d9deb9e21c9eac624fb43e Mon Sep 17 00:00:00 2001
> From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
> Date: Wed, 25 Sep 2019 22:43:41 +0900
> Subject: [PATCH 5/9] file-systems: Represent the file system options as an
>  alist.
>
> This allows accessing the parameter values easily, without having to parse a
> string.
>
> * gnu/system/file-systems.scm (<file-system>): Update the default value of the
> OPTIONS field, doc.
> (%file-system-options): Field accessor renamed from `file-system-options'.
> (file-system-options, file-system-options->string): New procedures.
> * gnu/build/file-systems.scm (mount-file-system): Adapt.
> * gnu/services/base.scm (file-system->fstab-entry): Likewise.
> * tests/file-systems.scm: New tests.
> * doc/guix.texi (File Systems): Document the modified default value of the
> 'file-system-options' field.

The main issue I see with this change is that mount(2) takes raw strings
for the options.  There’s a convention to have those strings look like
“KEY1=VALUE1,KEY2=VALUE2”, but it’s just a convention.

As a rule of thumb, I’d rather have our interface be as close as
possible to the actual mount(2) interface, which means taking strings.

Now, we can surely add helper procedures to parse options that follow
the above conventions.

WDYT?

> +  ;; Support the deprecated options format (a string).
> +  (define (options-string->options-list str)
> +    (let ((option-list (string-split str #\,)))
> +      (map (lambda (param)
> +             (if (string-contains param "=")
> +                 (apply cons (string-split param #\=))
> +                 param))
> +           option-list)))

I think we’d want to split only on the first ‘=’ sign, meaning we should
use ‘string-index’ etc. instead of ‘string-split’.

> From 67135c925b07f2e077b4cd852e07178691a10164 Mon Sep 17 00:00:00 2001
> From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
> Date: Tue, 11 Feb 2020 14:14:36 -0500
> Subject: [PATCH 6/9] gnu: linux-boot: Honor the "--root-options" kernel
>  argument.
>
> * gnu/build/linux-boot.scm (boot-system): Parse the "--root-options" kernel
> argument, and use it when calling `mount-root-file-system'.  Update doc.
> * doc/guix.texi (Initial RAM Disk): Document the use of the "--root-options"
> argument.

Hmm do we really need this extra option?  :-)

(Also, in hindsight, I think it was a mistake to call them
‘--something’.  Following the common naming convention, we should rather
call these options ‘gnu.something’.)

> From cb060af5ea56427e1fd63ced5f9c9edd3ae61f76 Mon Sep 17 00:00:00 2001
> From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
> Date: Tue, 11 Feb 2020 14:27:19 -0500
> Subject: [PATCH 7/9] gnu: linux-boot: Filter out file system independent
>  options.
>
> This fixes an issue where options such as "defaults", which are understood by
> the command line program "mount", are not understood by the system call of the
> same name, which is used in the initial RAM disk.
>
> * gnu/system/file-systems.scm (%file-system-independent-mount-options): New variable.
> (file-system-independent-mount-option?): New predicate.
> * gnu/build/linux-boot.scm (boot-system): Use the above predicate to filter
> out system independent mount options.

[...]

> +(define %file-system-independent-mount-options
> +  ;; Taken from 'man 8 mount'.
> +  '("async" "atime" "auto" "noatime" "noauto" "context" "defaults" "dev" "nodev"
> +    "diratime" "nodiratime" "dirsync" "exec" "noexec" "group" "iversion"
> +    "noiversion" "mand" "nomand" "_netdev" "nofail" "relatime" "norelatime"
> +    "strictatime" "nostrictatime" "lazytime" "nolazytime" "suid" "nosuid"
> +    "silent" "loud" "owner" "remount" "ro" "rw" "sync" "user" "nouser" "users"))

I’d rather avoid it.  In general, as much as possible, I think we should
pass options to the kernel without trying to be “smart”.  It’s often the
safe strategy.

WDYT?

> From 6cf2ece21683e98544f8f46675aef58d5a7231fd Mon Sep 17 00:00:00 2001
> From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
> Date: Sun, 14 Jul 2019 20:50:23 +0900
> Subject: [PATCH 8/9] bootloader: grub: Allow booting from a Btrfs subvolume.
>
> * gnu/bootloader/grub.scm (grub-configuration-file) [btrfs-subvolume-path]:
> New parameter.  When it is defined, prepend its value to the kernel and
> initrd file paths.
> * gnu/bootloader/depthcharge.scm (depthcharge-configuration-file): Adapt.
> * gnu/bootloader/extlinux.scm (extlinux-configuration-file): Likewise.
> * gnu/system/file-systems.scm (btrfs-subvolume?)
> (btrfs-store-subvolume-path): New procedures.
> * gnu/system.scm (operating-system-bootcfg): Specify the Btrfs subvolume path
> of the GNU store to the `operating-system-bootcfg' procedure, using the new
> BTRFS-SUBVOLUME-PATH argument.
> * doc/guix.texi (File Systems): Add a Btrfs subsection to document the use of
> subvolumes.  Document the new `properties' field of the `<file-system>'
> record.
> * gnu/tests/install.scm: Add test "btrfs-root-on-subvolume-os".

Neat!

>  (define* (grub-configuration-file config entries
>                                    #:key
>                                    (system (%current-system))
> -                                  (old-entries '()))
> +                                  (old-entries '())
> +                                  btrfs-subvolume-path)
>    "Return the GRUB configuration file corresponding to CONFIG, a
>  <bootloader-configuration> object, and where the store is available at
>  STORE-FS, a <file-system> object.  OLD-ENTRIES is taken to be a list of menu
> -entries corresponding to old generations of the system."
> +entries corresponding to old generations of the system.  BTRFS-SUBVOLUME-PATH
> +may be used to specify on which subvolume a Btrfs root file system resides."

(Nitpick: s/path/file name/ :-))

It’s a bit problematic that (1) GRUB needs explicit Btrfs support, and
(2) other bootloaders will silently ignore the option, due to
#:allow-other-keys.

I don’t have a better idea, but it’d be great if Btrfs support could be
made bootloader-independent, and if it could be somewhat
not-too-btrfs-specific, if that is possible at all.

Thoughts?

> +  (properties       file-system-properties        ; list of name-value pairs
> +                    (default '()))
>    (location         file-system-location
>                      (default (current-source-location))
>                      (innate)))
> @@ -582,4 +589,48 @@ system has the given TYPE."
>      (or (string-prefix-ci? "x-" option-name)
>          (member option-name %file-system-independent-mount-options))))
>  
> +(define (btrfs-subvolume? fs)
> +  "Predicate to check if FS, a file-system object, is a Btrfs subvolume."
> +  (and-let* ((btrfs-file-system? (string= "btrfs" (file-system-type fs)))
> +             (option-keys (map (match-lambda
> +                                 ((key . value) key)
> +                                 (key key))
> +                               (file-system-options fs))))
> +    (find (cut string-prefix? "subvol" <>) option-keys)))

I wonder if we can avoid special support in the <file-system> API for
Btrfs.

> +              (error "The store is on a Btrfs subvolume, but the \
> +subvolume name is unknown.
> +Hint: Define the \"btrfs-subvolume-path\" file system property or
> +use the \"subvol\" Btrfs file system option."))))

Rather use ‘raise’ with ‘&message’ and ‘&fix-hint’ conditions.

Pheeew, that was a long patch series!

Perhaps we can split the continuation of this thread in sizable chunks!

Thanks for working on this,
Ludo’.