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

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

Hi Ludovic!

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

> Hi Maxim,
>
> Sorry for dropping the ball for sooo long.

No worries :-)

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

[...]

>> +++ b/gnu/build/linux-boot.scm
>> @@ -498,25 +498,13 @@ upon error."
>>    (define (root-mount-point? fs)
>>      (string=? (file-system-mount-point fs) "/"))
>>  
>> -  (define root-fs-type
>> -    (or (any (lambda (fs)
>> -               (and (root-mount-point? fs)
>> -                    (file-system-type fs)))
>> -             mounts)
>> -        "ext4"))
>> -
>> -  (define root-fs-flags
>> -    (mount-flags->bit-mask (or (any (lambda (fs)
>> -                                      (and (root-mount-point? fs)
>> -                                           (file-system-flags fs)))
>> -                                    mounts)
>> -                               '())))
>> -
>> -  (define root-fs-options
>> -    (any (lambda (fs)
>> -           (and (root-mount-point? fs)
>> -                (file-system-options fs)))
>> -         mounts))
>
> [...]
>
>> +             (root-fs (find root-mount-point? mounts))
>> +             (root-fs-type (or (and=> root-fs file-system-type)
>> +                               "ext4"))
>> +             (root-fs-device (and=> root-fs file-system-device))
>> +             (root-fs-flags (mount-flags->bit-mask
>> +                             (or (and=> root-fs file-system-flags)
>> +                                 '())))
>> +             (root-options (if root-fs
>> +                               (file-system-options root-fs)
>> +                               #f))
>
> I would tend to keep these as defines to make the ‘let*’ less
> intimidating, but it’s a detail.

It would only *appear* less intimidating ;-).  I personally prefer the
let* versions as the logic is more succinctly expressed (there is no
need for 'any' + lambdas, for example).

>> +        ;; XXX: Importing (guix utils) and using &fix-hint causes the
>> +        ;; following error when booting the init RAM disk: "ERROR: In
>> +        ;; procedure dynamic-func:\nIn procedure dynamic-pointer: Symbol not
>> +        ;; found: strverscmp", so we just embed the hint in the message.
>
> I think it should just be “FIXME: Use &fix-hint once it no longer pulls
> in (guix utils).”

Done!

>>>From 082934db68964890ebd2a2118fb44d66911844d3 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 4/4] bootloader: grub: Allow booting from a Btrfs subvolume.
>>
>> * gnu/bootloader/grub.scm (strip-mount-point): Remove procedure.
>> (normalize-file): Add procedure.
>> (grub-configuration-file): New BTRFS-SUBVOLUME-FILE-NAME parameter.  When
>> defined, prepend its value to the kernel and initrd file names, using the
>> NORMALIZE-FILE procedure.  Adjust the call to EYE-CANDY to pass the
>> BTRFS-SUBVOLUME-FILE-NAME argument.  Normalize the KEYMAP file as well.
>> (eye-candy): Add a BTRFS-SUBVOLUME-FILE-NAME parameter, and use it, along with
>> the NORMALIZE-FILE procedure, to normalize the FONT-FILE and IMAGE nested
>> variables.  Adjust doc.
>> * 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-file-name): New procedures.
>> * gnu/system.scm (operating-system-bootcfg): Specify the Btrfs
>> subvolume file name the store resides on to the
>> `operating-system-bootcfg' procedure, using the new
>> BTRFS-SUBVOLUME-FILE-NAME argument.
>> * doc/guix.texi (File Systems): Add a Btrfs subsection to document the use of
>> subvolumes.
>> * gnu/tests/install.scm: Add test "btrfs-root-on-subvolume-os".
>
> Please list the entities added to ‘install.scm’.

Done!

>>  (define* (eye-candy config store-device store-mount-point
>> +                    btrfs-store-subvolume-file-name
>>                      #:key system port)
>
> I think ‘btrfs-store-subvolume-file-name’ should be a keyword argument.

Done!

>>  (define* (grub-configuration-file config entries
>>                                    #:key
>>                                    (system (%current-system))
>> -                                  (old-entries '()))
>> +                                  (old-entries '())
>> +                                  btrfs-subvolume-file-name)
>
> I wonder if we should just call it ‘store-directory-prefix’ or similar
> since, after all, it’s just about prepending a prefix, which could
> perhaps be useful for file systems other than Btrfs.
>
> Thoughts?

Perhaps, but given it's not yet clear whether another file system will
require similar support from GRUB, I'd rather keep things as explicit as
possible for now.

> Anyway, that’s great work, so I’ll be happy to finally see it committed
> in the coming days!

Thanks for the great words and for having a last look :-).

I've added a news entry and pushed to master as:

--8<---------------cut here---------------start------------->8---
489699c456 allow-booting-from-btrfs-subvolume news: Add entry for Btrfs subvolume boot support.
b460ba7992 bootloader: grub: Allow booting from a Btrfs subvolume.
fa35fb58c8 file-systems: Add helpers for parsing the options string into an alist.
281d80d8e5 linux-boot: Refactor boot-system.
--8<---------------cut here---------------end--------------->8---

Closing!  Thanks to Pierre and Ludovic for testing and reviewing.

Maxim