From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:470:142:3::10]:55711) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1j3Cc5-0007R4-1N for guix-patches@gnu.org; Sun, 16 Feb 2020 00:37:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1j3Cc2-0002Ou-KB for guix-patches@gnu.org; Sun, 16 Feb 2020 00:37:04 -0500 Received: from debbugs.gnu.org ([209.51.188.43]:58928) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1j3Cc2-0002OS-GA for guix-patches@gnu.org; Sun, 16 Feb 2020 00:37:02 -0500 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1j3Cc2-0004Iv-E7 for guix-patches@gnu.org; Sun, 16 Feb 2020 00:37:02 -0500 Subject: [bug#37305] [PATCH V2] Allow booting from a Btrfs subvolume. Resent-Message-ID: From: Maxim Cournoyer References: <87sgpby4p9.fsf@gmail.com> <87y2yg3t3s.fsf@gnu.org> <87k14sfaz7.fsf@gmail.com> <87lfp6b5cs.fsf_-_@gmail.com> <8736bdf5il.fsf@gnu.org> Date: Sun, 16 Feb 2020 00:36:28 -0500 In-Reply-To: <8736bdf5il.fsf@gnu.org> ("Ludovic \=\?utf-8\?Q\?Court\=C3\=A8s\=22'\?\= \=\?utf-8\?Q\?s\?\= message of "Fri, 14 Feb 2020 18:22:26 +0100") Message-ID: <87blpzozz7.fsf@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+kyle=kyleam.com@gnu.org Sender: "Guix-patches" To: Ludovic =?UTF-8?Q?Court=C3=A8s?= Cc: 37305@debbugs.gnu.org Hello! Thanks for the prompt feedback. Ludovic Court=C3=A8s writes: > 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 sys= tem >> tests. >> >> When setting the GUIX_DEV_HACKS environment variable, the Guix package u= sed >> inside the instrumented VMs recycles the binaries already found in the G= uix >> 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 proc= edure. >> * build-aux/run-system-tests.scm (tests-for-channel-instance): Use it, w= hen >> 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=99= 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=E2=80=99s still room for improvement, but I=E2=80=99d rather work i= n 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 implementat= ion 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 >> 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.) 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 >> 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 procedur= e. >> * 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 sy= stem >> 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 t= he >> let* instead. Make "--root" take precendence over the device field stri= ng >> 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 ca= rrying > =E2=80=98--root=E2=80=99 and I=E2=80=99m not sure if I=E2=80=99m forgetti= ng 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=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->= string)) >> + ;; --root takes precedence over the 'device' field of the = root >> + ;; 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 s= hould 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. Done! > Also, the (or =E2=80=A6 "ext4") bit doesn=E2=80=99t sound great, but perh= aps it=E2=80=99ll 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 >> 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 par= se a >> string. >> >> * gnu/system/file-systems.scm (): 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 t= he >> '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= like > =E2=80=9CKEY1=3DVALUE1,KEY2=3DVALUE2=E2=80=9D, but it=E2=80=99s just a co= nvention. > > 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? 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 "=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. Good catch! >> 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" ker= nel >> argument, and use it when calling `mount-root-file-system'. Update doc. >> * doc/guix.texi (Initial RAM Disk): Document the use of the "--root-opti= ons" >> 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 > =E2=80=98--something=E2=80=99. Following the common naming convention, w= e should rather > call these options =E2=80=98gnu.something=E2=80=99.) Is this convention detailed somewhere? I haven't found it in 'Standards'. >> 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 understo= od 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 fil= ter >> 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" "norelat= ime" >> + "strictatime" "nostrictatime" "lazytime" "nolazytime" "suid" "nosui= d" >> + "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= should > pass options to the kernel without trying to be =E2=80=9Csmart=E2=80=9D. = It=E2=80=99s 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