From debbugs-submit-bounces@debbugs.gnu.org Fri Feb 14 12:22:37 2020 Received: (at 37305) by debbugs.gnu.org; 14 Feb 2020 17:22:37 +0000 Received: from localhost ([127.0.0.1]:34640 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1j2efl-0005ZS-4r for submit@debbugs.gnu.org; Fri, 14 Feb 2020 12:22:37 -0500 Received: from eggs.gnu.org ([209.51.188.92]:35048) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1j2efi-0005ZG-LR for 37305@debbugs.gnu.org; Fri, 14 Feb 2020 12:22:35 -0500 Received: from fencepost.gnu.org ([2001:470:142:3::e]:36362) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1j2efd-0005Ld-HF; Fri, 14 Feb 2020 12:22:29 -0500 Received: from [2a01:e0a:1d:7270:af76:b9b:ca24:c465] (port=41592 helo=ribbon) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1j2efc-000259-TN; Fri, 14 Feb 2020 12:22:29 -0500 From: =?utf-8?Q?Ludovic_Court=C3=A8s?= To: Maxim Cournoyer Subject: Re: [bug#37305] [PATCH V2] Allow booting from a Btrfs subvolume. References: <87sgpby4p9.fsf@gmail.com> <87y2yg3t3s.fsf@gnu.org> <87k14sfaz7.fsf@gmail.com> <87lfp6b5cs.fsf_-_@gmail.com> X-URL: http://www.fdn.fr/~lcourtes/ X-Revolutionary-Date: 26 =?utf-8?Q?Pluvi=C3=B4se?= an 228 de la =?utf-8?Q?R=C3=A9volution?= X-PGP-Key-ID: 0x090B11993D9AEBB5 X-PGP-Key: http://www.fdn.fr/~lcourtes/ludovic.asc X-PGP-Fingerprint: 3CE4 6455 8A84 FDC6 9DB4 0CFB 090B 1199 3D9A EBB5 X-OS: x86_64-pc-linux-gnu Date: Fri, 14 Feb 2020 18:22:26 +0100 In-Reply-To: <87lfp6b5cs.fsf_-_@gmail.com> (Maxim Cournoyer's message of "Thu, 13 Feb 2020 15:27:15 -0500") Message-ID: <8736bdf5il.fsf@gnu.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Spam-Score: -0.7 (/) X-Debbugs-Envelope-To: 37305 Cc: 37305@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.7 (-) Hi Maxim! Great to see Btrfs support improving; many people will love that! Maxim Cournoyer skribis: > From 3640bea548826e1c1ec9b766da1fdfe4791d7452 Mon Sep 17 00:00:00 2001 > From: Maxim Cournoyer > Date: Sun, 17 Nov 2019 06:01:00 +0900 > Subject: [PATCH 1/9] gnu: tests: Reduce the time required to run the syst= em > tests. > > When setting the GUIX_DEV_HACKS environment variable, the Guix package us= ed > inside the instrumented VMs recycles the binaries already found in the Gu= ix > 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 proce= dure. > * build-aux/run-system-tests.scm (tests-for-channel-instance): Use it, wh= en > GUIX_DEV_HACKS is defined. I understand the need, but I=E2=80=99d really like to avoid that; it=E2=80= =99s too fragile IMO. But I have good news! First, commit 887fd835a7c90f720d36a211478012547feaead0 really improved things by avoiding the full =E2=80=98guix=E2=80=99 package rebuild (and we=E2=80=99re= 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=E2=80=99s still room for improvement, but I=E2=80=99d rather work in = those directions. WDYT? > From 97d8a635eba34c7cf0708e99bf77ef9bad1344bf Mon Sep 17 00:00:00 2001 > From: Maxim Cournoyer > 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 =E2=80=9Cgnu:=E2=80=9D prefix.) OK! > From e8d6642d3597207657842c9ca4849f8660d06638 Mon Sep 17 00:00:00 2001 > From: Maxim Cournoyer > 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 > 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 sys= tem > 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=3D@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=E2=80=99ve been carr= ying =E2=80=98--root=E2=80=99 and I=E2=80=99m not sure if I=E2=80=99m 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=3D> root-fs file-system-type) > + "ext4")) > + (root-device (and=3D> root-fs file-system-device)) > + (root-device-str (and=3D> root-device file-system-device->s= tring)) > + ;; --root takes precedence over the 'device' field of the r= oot > + ;; record. > + (root (or (find-long-option "--root" args) > + root-device-str)) > + (root-fs-flags (mount-flags->bit-mask > + (or (and=3D> 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 =E2=80=98file-system-device->string=E2=80=99 is lossy, I think we sho= uld do it the other way around: convert the =E2=80=98--root=E2=80=99 string to a . Perhaps that bit can be moved to a separate procedure, like =E2=80=98root-string->file-system=E2=80=99. Also, the (or =E2=80=A6 "ext4") bit doesn=E2=80=99t sound great, but perhap= s it=E2=80=99ll be unnecessary if we do with something as outlined above? > From af61745d8b686755a5d9deb9e21c9eac624fb43e Mon Sep 17 00:00:00 2001 > From: Maxim Cournoyer > 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 pars= e a > string. > > * gnu/system/file-systems.scm (): Update the default value o= f 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=E2=80=99s a convention to have those strings look l= ike =E2=80=9CKEY1=3DVALUE1,KEY2=3DVALUE2=E2=80=9D, but it=E2=80=99s just a conv= ention. As a rule of thumb, I=E2=80=99d 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 "=3D") > + (apply cons (string-split param #\=3D)) > + param)) > + option-list))) I think we=E2=80=99d want to split only on the first =E2=80=98=3D=E2=80=99 = sign, meaning we should use =E2=80=98string-index=E2=80=99 etc. instead of =E2=80=98string-split=E2= =80=99. > From 67135c925b07f2e077b4cd852e07178691a10164 Mon Sep 17 00:00:00 2001 > From: Maxim Cournoyer > 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" kern= el > argument, and use it when calling `mount-root-file-system'. Update doc. > * doc/guix.texi (Initial RAM Disk): Document the use of the "--root-optio= ns" > argument. Hmm do we really need this extra option? :-) (Also, in hindsight, I think it was a mistake to call them =E2=80=98--something=E2=80=99. Following the common naming convention, we = should rather call these options =E2=80=98gnu.something=E2=80=99.) > From cb060af5ea56427e1fd63ced5f9c9edd3ae61f76 Mon Sep 17 00:00:00 2001 > From: Maxim Cournoyer > 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 understoo= d by > the command line program "mount", are not understood by the system call o= f the > same name, which is used in the initial RAM disk. > > * gnu/system/file-systems.scm (%file-system-independent-mount-options): N= ew variable. > (file-system-independent-mount-option?): New predicate. > * gnu/build/linux-boot.scm (boot-system): Use the above predicate to filt= er > 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" "norelati= me" > + "strictatime" "nostrictatime" "lazytime" "nolazytime" "suid" "nosuid" > + "silent" "loud" "owner" "remount" "ro" "rw" "sync" "user" "nouser" "= users")) I=E2=80=99d rather avoid it. In general, as much as possible, I think we s= hould pass options to the kernel without trying to be =E2=80=9Csmart=E2=80=9D. I= t=E2=80=99s often the safe strategy. WDYT? > From 6cf2ece21683e98544f8f46675aef58d5a7231fd Mon Sep 17 00:00:00 2001 > From: Maxim Cournoyer > Date: Sun, 14 Jul 2019 20:50:23 +0900 > Subject: [PATCH 8/9] bootloader: grub: Allow booting from a Btrfs subvolu= me. > > * 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 n= ew > BTRFS-SUBVOLUME-PATH argument. > * doc/guix.texi (File Systems): Add a Btrfs subsection to document the us= e of > subvolumes. Document the new `properties' field of the `' > 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 > object, and where the store is available at > STORE-FS, a 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 resid= es." (Nitpick: s/path/file name/ :-)) It=E2=80=99s 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=E2=80=99t have a better idea, but it=E2=80=99d be great if Btrfs supp= ort 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 p= airs > + (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)))) >=20=20 > +(define (btrfs-subvolume? fs) > + "Predicate to check if FS, a file-system object, is a Btrfs subvolume." > + (and-let* ((btrfs-file-system? (string=3D "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 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 =E2=80=98raise=E2=80=99 with =E2=80=98&message=E2=80=99 and =E2= =80=98&fix-hint=E2=80=99 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=E2=80=99.