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

[Ludovic Courtès]

Hi Maxim,

Resuming review of this series…  Sorry for the delay!

Maxim Cournoyer <address@hidden> skribis:

>>> From 97d8a635eba34c7cf0708e99bf77ef9bad1344bf Mon Sep 17 00:00:00 2001
>>> From: Maxim Cournoyer <address@hidden>
>>> 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? :-)

“gnu:” is for changes to (gnu packages).  The idea is that the prefix
should reflect what subsystem the commit is modifying.  But yeah,
looking at ‘git log’ can be inspiring.  :-)

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

Yes, ‘--root’ can resolve labels and UUIDs; my question was more about
why we have it in the first place.

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

Yes, that makes sense.  It’s certainly useful to have ‘--root’ at least
as an option.

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

Right, mount(8) documents file system options that can be passed to
mount(2).

What does it mean to target mount(8) vs. mount(2)?  To me, mount(8) is a
CLI to mount(2) that provides additional features to make the CLI more
convenient: the “defaults” option, a way to pass mount(2) flags as
options (like “ro”, “remount”, “bind”), /etc/fstab handling, etc.

Guix System handles /etc/fstab differently and “defaults” makes little
sense in our API (one can just use leave the default value of the
‘options’ field.)

I think mount(8) is actually a good illustration of what not to do.  It
ends up mixing things that are separate in the mount(2) API, and that
doesn’t improve clarity and future-proof-ness (what if a file system has
a “bind” option, etc.).

But again, I think the helper procedures that you propose to move back
and forth between the string and the alist representations are very
welcome.  I just wouldn’t hard-code that directly in our API.

WDYT?

>>> From 67135c925b07f2e077b4cd852e07178691a10164 Mon Sep 17 00:00:00 2001
>>> From: Maxim Cournoyer <address@hidden>
>>> 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?

I’d rather avoid it for now.  Less code is better.  :-)

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

It’s a convention of the Linux kernel, I don’t know if it’s documented.

That’s it!

Ludo’.