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

[Maxim Cournoyer <maxim.cournoyer@gmail.com>]

Hello!

Thanks for the prompt feedback.

Ludovic Courtès <ludo@gnu.org> writes:

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

With a little bit more love (inheriting from the Guix package from the
inferior captured at build time), I don't see the hack being any less
fragile than the Guix checkout compiled and ran with ./pre-inst-env.

There's no arguing that it *is* a hack, but:

1) Being labeled as such (GUIX_DEV_HACKS)
2) Being undocumented
3) Only being enabled explicitly (through that GUIX_DEV_HACKS
environment variable)
4) Can be reverted easily in the future when the default, clean implementation is
fast enough to obsolete it.

To me means its targeted to developers who understand the nature of the
hack and is provided without any warranty.

And while it's exciting that Guile's compilation time is going to be
improved, no compiler's going to be as efficient as "no compilation"
;-).

>> 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.)

Done.

I never know before looking at past logs (and then sometimes it's a
mixed bag).  Is there any mechanical process for selecting the right
commit 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.

If the documentation is accurate, it should :-), given that --root gets
written as a string to the GRUB configuration file, and that the doc
says it's possible to give it as a device name, label or UUID.

About why providing options such as --root or --root-options in the
first place; I pondered about this as well, especially after making the
file systems from operating system able to be mounted with all their
(file system independent -- more on that later) options.  A reason I
came up with was that it allows to experiment at the GRUB command line
and change the root device, or perhaps the root options.  One use case
would be debugging the right options to pass to a file system driver in
case of a mistake in the operating system declaration.

>
>>        (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’.

Done!

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

That bit was already there; it exists because we allow an operating
system definition to be made without any root file system (for use with
the 'guix system container' or 'guix system vm' commands IIRC).  What
could be simpler would be to always require such information to be
embedded in the operating system declaration: when an interactive user
command would need to modify the root file system, it could create a new
operating-system object, inheriting from the original one.

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

To me, it's an implementation detail that I'd rather abstract away (or
make optional, like in this patch).  Just like we provide a higher level
configuration for services instead of requiring the user to input the
configuration in the native format of the tool (or allowing for both).
The idea for this format was taken from a discussion here:
http://issues.guix.info/issue/33517#3.

Are we really targeting mount(2)?  The commit
9d3053819dfd834a1c29a03427c41d8524b8a7d5 (which you co-authored :-))
mentions 'man 8 mount' for the file system options. This should be
stressed in the manual, as someone attempting to pass file system
independent options (perhaps taking their options line from a previous
/etc/fstab config), could prevent the system from booting.

I mistakenly was under the impression that mount(8) was targeted, since
the main interaction I observed between a file system object and my
system was the /etc/fstab file getting produced.

Doesn't targeting mount(2) mean that it becomes impossible to give all
the options someone would possibly want to produce their /etc/fstab
file?  Suppose you want the 'ro' option for one of your partition in
/etc/fstab; that wouldn't fly currently, unless I'm missing something.
It seems to me we should target mount(8), and internally translate the
file system independent mount(8) options into mount(2) flags or
vice-versa but targeting the higher level and going down makes more
sense to me.

>> +  ;; 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’.

Good catch!

>> 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?  :-)

It is not strictly needed but allows the user to experiment/troubleshoot
with the init RAM disk from GRUB as discussed earlier for --root.  Do
you think it has enough value to be kept?

> (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’.)

Is this convention detailed somewhere?  I haven't found it in 'Standards'.

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

Let's revisit this topic after we've sorted the mount(8) vs mount(2)
problem above.

I'll address the rest in a part 2.

Thank you for your time and patience!

Maxim