[bug#37305] Allow booting from a Btrfs subvolume [review part 2]

[Maxim Cournoyer]

Hello Ludovic,

Here comes part 2!

Ludovic Courtès <address@hidden> writes:

>> From 6cf2ece21683e98544f8f46675aef58d5a7231fd Mon Sep 17 00:00:00 2001
>> From: Maxim Cournoyer <address@hidden>
>> 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/ :-))

I stand corrected!  From '(Standards)GNU Manuals':

       Please do not use the term "pathname" that is used in Unix
    documentation; use "file name" (two words) instead.  We use the term
    "path" only for search paths, which are lists of directory names.

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

I have no idea how Btrfs subvolumes are handled (if at all) on U-Boot or
other bootloaders than GRUB.  All I know is that for GRUB they need to
handle subvolumes in a special manner in their own grub-mkconfig tool
(which we bypass).

Also, I'm afraid subvolumes are very Btrfs specific :-).  It doesn't
exist in traditional file systems like EXT4.  I think ZFS must have
something similar, though.

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

I tried this, but importing (guix utils) to acces &fix-hint caused the init
RAM disk to fail mysteriously:

--8<---------------cut here---------------start------------->8---
[    0.614503] Run /init as init process
GC Warning: pthread_getattr_np or pthread_attr_getstack failed for main thread
GC Warning: Couldn't read /proc/stat
Backtrace:
In ice-9/boot-9.scm:
   222:29 19 (map1 (((ice-9 match)) ((rnrs bytevectors)) ((srfi 
   222:29 18 (map1 (((rnrs bytevectors)) ((srfi srfi-1)) ((srfi 
   222:29 17 (map1 (((srfi srfi-1)) ((srfi srfi-2)) ((srfi #)) (#) 
   222:29 16 (map1 (((srfi srfi-2)) ((srfi srfi-9)) ((srfi #)) (#) 
   222:29 15 (map1 (((srfi srfi-9)) ((srfi srfi-26)) ((srfi #)) (#) 
   222:29 14 (map1 (((srfi srfi-26)) ((srfi srfi-35)) ((srfi # #)) 
   222:29 13 (map1 (((srfi srfi-35)) ((srfi srfi-9 gnu)) ((guix 
))[    0.657578] random: fast init done
   222:29 12 (map1 (((srfi srfi-9 gnu)) ((guix records)) ((guix 
   222:29 11 (map1 (((guix records)) ((guix utils)) ((gnu system 
))))
   222:17 10 (map1 (((guix utils)) ((gnu system uuid))))
  2800:17  9 (resolve-interface (guix utils) #:select _ #:hide _ # _ 
In ice-9/threads.scm:
    390:8  8 (_ _)
In ice-9/boot-9.scm:
  2726:13  7 (_)
In ice-9/threads.scm:
    390:8  6 (_ _)
In ice-9/boot-9.scm:
  2994:20  5 (_)
   2312:4  4 (save-module-excursion #<procedure 7f455b6d48d0 at ice-
  3014:26  3 (_)
In unknown file:
           2 (primitive-load-path "guix/utils" #<procedure 7f455b8fd
In guix/utils.scm:
   508:24  1 (_)
In unknown file:
           0 (dynamic-func "strverscmp" #<dynamic-object #f>)
ERROR: In procedure dynamic-func:
In procedure dynamic-pointer: Symbol not found: strverscmp
[    0.697002] Kernel panic - not syncing: Attempted to kill init! 
exitcode=0x00000100
[    0.697894] CPU: 0 PID: 1 Comm: init Not tainted 5.4.18-gnu #1
[    0.698592] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
rel-1.12.0-59-gc9ba5276e321-prebuilt.qemu.org 04/01/2014
[    0.699938] Call Trace:
[    0.700240]  dump_stack+0x6d/0x8d
[    0.700640]  panic+0x10b/0x2f4
[    0.701010]  do_exit+0x7e2/0xb80
[    0.701398]  ? wake_up_state+0x1f/0x30
[    0.701847]  ? signal_wake_up_state+0x24/0x40
[    0.702374]  do_group_exit+0x44/0xa0
[    0.702805]  __x64_sys_exit_group+0x1c/0x20
[    0.703306]  do_syscall_64+0x5a/0x190
[    0.703748]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[    0.704347] RIP: 0033:0x5df9c6
[    0.704716] Code: 00 00 00 be 3c 00 00 00 eb 19 66 2e 0f 1f 84 00 00 00 00 
00 89 d7 89 f0 0f 05 48 3d 00 f0 ff ff 77 22 f4 89 d7 44 89 c0 0f 05 <48> 3d 00 
f0 ff ff 76 e2 f7 d8 64 41 89 01 eb da 66 2e 0f 1f 84 00
[    0.706919] RSP: 002b:00007ffd9fa65228 EFLAGS: 00000246 ORIG_RAX: 
00000000000000e7
[    0.707808] RAX: ffffffffffffffda RBX: 00000000007c51f0 RCX: 00000000005df9c6
[    0.708643] RDX: 0000000000000001 RSI: 000000000000003c RDI: 0000000000000001
[    0.709482] RBP: 0000000000000001 R08: 00000000000000e7 R09: ffffffffffffffb0
[    0.710328] R10: 0000000000436340 R11: 0000000000000246 R12: 00000000007c51f0
[    0.711166] R13: 0000000000000001 R14: 0000000000000000 R15: 0000000000000003
[    0.712026] Kernel Offset: 0x13000000 from 0xffffffff81000000 (relocation 
range: 0xffffffff80000000-0xffffffffbfffffff)
[    0.713288] Rebooting in 1 seconds..
file-size: 
/gnu/store/zfi66vny0h10d180xajgm4pq2vnvmc2z-nss-certs-3.49.1/etc/ssl/certs/NetLock_Arany_=Class_Gold=_F??tan??s??tv??ny:2.6.73.65.44.228.0.16.pem:
 No such file or directory
file-size: 
/gnu/store/04xpkgf9zlhcngyr6gnhl4rb8g6v6i1i-profile/etc/ssl/certs/NetLock_Arany_=Class_Gold=_F??tan??s??tv??ny:2.6.73.65.44.228.0.16.pem:
 No such file or directory
--8<---------------cut here---------------end--------------->8---

The exception I had refactored to use with &fix-hint looked like:

--8<---------------cut here---------------start------------->8---
(raise (condition
                      (&message
                       (message "The store is on a Btrfs subvolume, \
but the subvolume name is unknown."))
                      (&fix-hint
                       (hint "Define the \"btrfs-subvolume-file-name\" \
file system property or use the \"subvol\" Btrfs file system
option."))))
--8<---------------cut here---------------end--------------->8---

So, I ended up using just a &message condition with the hint embedded in it.
The attached patch (v3) incorporate the agreed changes so far.

The btrfs-root-on-subvolume-os test still passes.

Thank you for your patience,

Maxim

0001-gnu-tests-Reduce-the-time-required-to-run-the-system.patch
Description: Text Data

0002-linux-boot-Ensure-volatile-root-is-mounted-read-only.patch
Description: Text Data

0003-file-systems-Add-a-file-system-device-string-procedu.patch
Description: Text Data

0004-linux-boot-Refactor-boot-system.patch
Description: Text Data

0005-file-systems-Represent-the-file-system-options-as-an.patch
Description: Text Data

0006-linux-boot-Honor-the-root-options-kernel-argument.patch
Description: Text Data

0007-linux-boot-Filter-out-file-system-independent-option.patch
Description: Text Data

0008-bootloader-grub-Allow-booting-from-a-Btrfs-subvolume.patch
Description: Text Data