Skip to content

Fixing 781 and it's btrfs mountpoints and kernel parameters#905

Merged
Torxed merged 38 commits intov2.3.1-devfrom
torxed-fix-btrfs-mounts
Jan 26, 2022
Merged

Fixing 781 and it's btrfs mountpoints and kernel parameters#905
Torxed merged 38 commits intov2.3.1-devfrom
torxed-fix-btrfs-mounts

Conversation

@Torxed
Copy link
Copy Markdown
Member

@Torxed Torxed commented Jan 25, 2022

  • Before running the equivalent of pacstrap, create the subvols on the toplevel
  • Umount the toplevel and mount the rootfs subvol (/@) into /mnt/archinstall
  • Mount other subvolumes such as /@homeinto their place inside the /mnt/archinstall
  • Run pacstrap in /mnt/archinstall
  • Run genfstab -U >> /mnt/archinstall/etc/fstab (the currently generated /etc/fstab is incorrect, but it should fix itself after solving the items above).
  • Make sure the kernel cmdline has the rootflags=subvol=/@ parameter, so it mounts the correct subvolume as root without having to use btrfs subvolume set-default. To accomplish this, configure the bootloader to pass that flag to the Linux kernel, example for systemd-boot:
title   Arch Linux
linux   /vmlinuz-linux
initrd  /intel-ucode.img
initrd  /initramfs-linux.img
options  rootflags=subvol=/@

Tested

  • systemd-boot with subvolumes
  • systemd-boot + luks2 with subvolumes
  • grub in BIOS with subvolumes
  • grub in BIOS + luks2 with subvolumes
  • grub in UEFI with subvolumes
  • grub in UEFI + luks2 with subvolumes

… it. Improved some logging and decreased the usage of try/except where unessecary (we can catch it higher up without catching and raising the same thing.
@Torxed Torxed linked an issue Jan 25, 2022 that may be closed by this pull request
@Torxed
Copy link
Copy Markdown
Member Author

Torxed commented Jan 25, 2022

Current state is that it installs.
But the mountpoints are still not there:

[root@bigrigv2 archinstall]# findmnt --real
TARGET                    SOURCE               FSTYPE      OPTIONS
└─/mnt/archinstall        /dev/loop0p2[/@]     btrfs       rw,relatime,ssd,space_cache=v2,subvolid=256,subvol=/@
  └─/mnt/archinstall/boot /dev/loop0p1         vfat        rw,relatime,fmask=0022,dmask=0022,codepage=437,iocharset=ascii,shortname=mixed,utf8,errors=remount-ro
[root@bigrigv2 archinstall]# ls /mnt/archinstall/
bin  boot  dev	etc  home  lib	lib64  mnt  opt  proc  root  run  sbin	srv  sys  tmp  usr  var

For some reason the manage_btrfs_subvolumes return statement doesn't update the mountpoints variable:

{
    "/boot": {
        "partition": {
            "boot": true,
            "encrypted": false,
            "filesystem": {
                "format": "fat32"
            },
            "format": true,
            "mountpoint": "/boot",
            "size": "513MB",
            "start": "5MB",
            "type": "primary",
            "device_instance": "Partition(path=/dev/loop0p1",
            "size=0.5": null,
            "PARTUUID=db071b01-b344-4a76-b44c-6f2dba3b169e": null,
            "fs=vfat)": null
        }
    },
    "/": {
        "partition": {
            "btrfs": {
                "subvolumes": {
                    "@": "/",
                    "@.snapshots": "/.snapshots",
                    "@home": "/home",
                    "@log": "/var/log",
                    "@pkg": "/var/cache/pacman/pkg"
                }
            },
            "encrypted": false,
            "filesystem": {
                "format": "btrfs"
            },
            "format": true,
            "mountpoint": "/",
            "size": "100%",
            "start": "518MB",
            "type": "primary",
            "device_instance": "Partition(path=/dev/loop0p2",
            "size=19.5": null,
            "PARTUUID=5ea67532-dc42-4076-a732-27eb2b8d6ad4": null,
            "fs=btrfs)": null
        },
        "mount_options": [
            "subvol=@/"
        ]
    }
}

fs=vfat) is also a bit alarming. Not sure where the ) in vfat) comes from.

Copy link
Copy Markdown
Contributor

@wllacer wllacer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my phone.
I think you don't need the mountpoints.update at line 213 (and others) of installer.py
In the worst case, i've seen you ported manage_btrs_subvolumes from máster. IIRC i made It to behave slightly diferent. That could imply you need to Port mount_ordered_partitions also. And this is a lap of faith.
I'd rather revert this patch and try with #906 only

…all subvolumes are mountpoint targets, but with relevant information. Had to work that into Partition().mount to support re-mounting a partition multiple times. That will have to be cleaned up.
@Torxed
Copy link
Copy Markdown
Member Author

Torxed commented Jan 25, 2022

In #906 the subvolumes doesn't get mounted when I tried. Or so I'm 90% sure of.
I've just now got v2.3.1-dev to a state where it completes an installation and the default subvolume layout is supported and works.

There's still some questionable code, will sleep on it before I merge this.
For instance, a subvolume definition of partition = Partition(target['source'], partition.real_device, filesystem=target.get('fstype', None), mountpoint=target['target']) in get_partitions_in_use breaks because it will try to auto-mount a device that is potentially already mounted. I put a auto_mount = False in the class and the call for now.

As this logic only appear to happen surrounding btrfs subvolumes, and we've fixed this in master.
I would deem this stable enough and a improvement to v2.3.0. I don't think people can go to crazy with subvolumes using v2.3.1, but at least it will have a better default layout and that works. + all the other disk related issues that got fixed too.

The PR is ready for review and testing, but again, keep in mind:

@Torxed
Copy link
Copy Markdown
Member Author

Torxed commented Jan 25, 2022

This is the current state of the code in v2.3.1-dev -fix branch:

[root@bigrigv2 archinstall]# findmnt --real
TARGET                                    SOURCE                     FSTYPE      OPTIONS
└─/mnt/archinstall                        /dev/loop0p2[/@]           btrfs       rw,relatime,ssd,space_cache=v2,subvolid=256,subvol=/@
  ├─/mnt/archinstall/boot                 /dev/loop0p1               vfat        rw,relatime,fmask=0022,dmask=0022,codepage=437,iocharset=ascii,shortname=mixed,utf8,errors=remount-ro
  ├─/mnt/archinstall/.snapshots           /dev/loop0p2[/@.snapshots] btrfs       rw,relatime,ssd,space_cache=v2,subvolid=257,subvol=/@.snapshots
  ├─/mnt/archinstall/home                 /dev/loop0p2[/@home]       btrfs       rw,relatime,ssd,space_cache=v2,subvolid=258,subvol=/@home
  ├─/mnt/archinstall/var/cache/pacman/pkg /dev/loop0p2[/@pkg]        btrfs       rw,relatime,ssd,space_cache=v2,subvolid=260,subvol=/@pkg
  └─/mnt/archinstall/var/log              /dev/loop0p2[/@log]        btrfs       rw,relatime,ssd,space_cache=v2,subvolid=259,subvol=/@log

Not sure this is enough, I'm missing the @home when doing ls -l /mnt/archinstall

@Torxed Torxed marked this pull request as ready for review January 25, 2022 20:10
@wllacer
Copy link
Copy Markdown
Contributor

wllacer commented Jan 25, 2022

This is the current state of the code in v2.3.1-dev -fix branch:

[root@bigrigv2 archinstall]# findmnt --real
TARGET                                    SOURCE                     FSTYPE      OPTIONS
└─/mnt/archinstall                        /dev/loop0p2[/@]           btrfs       rw,relatime,ssd,space_cache=v2,subvolid=256,subvol=/@
  ├─/mnt/archinstall/boot                 /dev/loop0p1               vfat        rw,relatime,fmask=0022,dmask=0022,codepage=437,iocharset=ascii,shortname=mixed,utf8,errors=remount-ro
  ├─/mnt/archinstall/.snapshots           /dev/loop0p2[/@.snapshots] btrfs       rw,relatime,ssd,space_cache=v2,subvolid=257,subvol=/@.snapshots
  ├─/mnt/archinstall/home                 /dev/loop0p2[/@home]       btrfs       rw,relatime,ssd,space_cache=v2,subvolid=258,subvol=/@home
  ├─/mnt/archinstall/var/cache/pacman/pkg /dev/loop0p2[/@pkg]        btrfs       rw,relatime,ssd,space_cache=v2,subvolid=260,subvol=/@pkg
  └─/mnt/archinstall/var/log              /dev/loop0p2[/@log]        btrfs       rw,relatime,ssd,space_cache=v2,subvolid=259,subvol=/@log

Not sure this is enough, I'm missing the @home when doing ls -l /mnt/archinstall

I think the output is totally Ok. You have the @home subvolume correctly mounted
To see which subvolumes are defined do
sudo btrfs subvolume list /mnt/archinstall

@Torxed
Copy link
Copy Markdown
Member Author

Torxed commented Jan 25, 2022

Tested with and without encryption using systemd-boot.
I have not tested with grub yet, but so far un-encrypted works then but encryption fails on a BlockDevice lookup.

One thing that has me concerned is that every subvolume gets Adding additional key-file ....
Because it's now treated as one separate "partition". This should be corrected as we probably will end up with too many keys (?) at some point.

I'm not far away at least.
I know why that happens, the lookup for BlockDevice when creating Partition() in helpers were not sophisticated enough.
Think I found a solution.

@wllacer
Copy link
Copy Markdown
Contributor

wllacer commented Jan 25, 2022

Tested with and without encryption using systemd-boot. I have not tested with grub yet, but so far un-encrypted works then but encryption fails on a BlockDevice lookup.

One thing that has me concerned is that every subvolume gets Adding additional key-file .... Because it's now treated as one separate "partition". This should be corrected as we probably will end up with too many keys (?) at some point.

I'm not far away at least.

The btrfs code in 2.3.1 DOES NOT support encrypted partitions. It was clearly documented, and i thought i had a explicit branching in the code. If you want it, you need to put the whole #838.

btw. i'm trying to test v.2.3.1.dev but I've run in an exception

 File "/home/werner/projects/archinstall/archinstall/lib/disk/partition.py", line 220, in partprobe
    if self.block_device and SysCommand(f'partprobe {self.block_device.device}').exit_code == 0:
AttributeError: 'float' object has no attribute 'device'

Do you remember having anything like that ?

@Torxed
Copy link
Copy Markdown
Member Author

Torxed commented Jan 25, 2022

I got it supporting encryption now. So far it encrypts the root partition /dev/sda2 and then unlocks it, creates subvolumes inside it, unmounts it and with the unlocked volume mounts all the subolumes and continues with the installation.

I do not get AttributeError: 'float' object has no attribute 'device'. I don't know how self.block_device would be a float? humm

@wllacer
Copy link
Copy Markdown
Contributor

wllacer commented Jan 25, 2022

A correction, We managed to support encryption but not the generation of key-files for btrfs. It was the last thing that came out in #787

The reason why we didn't support encryption, was that encryption and key handling has to be done, in btrfs as in the rest of the filesystems, at the physical partition level.
To achieve this i had to totally rewrite mount_ordered_partitions, handling the encryption prior to subvol definition and mounting, and creating the keys after everything else was done

@Torxed
Copy link
Copy Markdown
Member Author

Torxed commented Jan 25, 2022

I solved that by checking if the keyfile already exists on the desired destination (which is based on loopdev name).
Still running a few last tests. keyfile is only really supposed to be used when there's multiple partitions that are individually encrypted. I'm still not sure why it's even creating a keyfile for /dev/sda2 as that's the encrypted volume. But it appears generate-encryption-key-file is set somewhere.

Something that we don't really do on btrfs with subvolumes.
As they are all virtually the same logical encrypted(device).

@Torxed
Copy link
Copy Markdown
Member Author

Torxed commented Jan 25, 2022

Old function:

def select_encrypted_partitions(block_devices :dict, password :str) -> dict:
for device in block_devices:
for partition in block_devices[device]['partitions']:
if partition.get('mountpoint', None) != '/boot':
partition['encrypted'] = True
partition['!password'] = password
if partition['mountpoint'] != '/':
# Tell the upcoming steps to generate a key-file for non root mounts.
partition['generate-encryption-key-file'] = True

This sets all partitions that are not / to generate a keyfile.
This has to be changed before this PR can go in.

@wllacer
Copy link
Copy Markdown
Contributor

wllacer commented Jan 25, 2022

I solved that by checking if the keyfile already exists on the desired destination (which is based on loopdev name).

Good idea

Still running a few last tests. keyfile is only really supposed to be used when there's multiple partitions that are individually encrypted. I'm still not sure why it's even creating a keyfile for /dev/sda2 as that's the encrypted volume. But it appears generate-encryption-key-file is set somewhere.

I remember the same problem, I solved it checking if the partitiion didn't contain the root / directory

Something that we don't really do on btrfs with subvolumes. As they are all virtually the same logical encrypted(device).
btrfs handles partitions as encryption unit, not subvolumes

@Torxed
Copy link
Copy Markdown
Member Author

Torxed commented Jan 25, 2022

Apparemtly systemd-run --machine={self.container_name} --pty /bin/bash -c "shutdown now" always exits with exit code 256 when it's doen good. (wraparound to 0?).

Instead, we check if either shutdown or the session exit code is 0, which should mean all is well.
Boot() now works as intended.

I'll have a look at GRUB + UEFI tomorrow.
That's the only thing that doesn't work right now.

@wllacer
Copy link
Copy Markdown
Contributor

wllacer commented Jan 25, 2022

GRUB with encryption installs, but I can't get it to boot to the boot loader, it skips the HDD and goes straight to ISO.

what kind of target disk are you using? I wasn't able to boot into Grub with a loopback device, IIRC not even with ext4. In fact the only way i could make it booting was within a virtualbox session, with an vdi target

@Torxed
Copy link
Copy Markdown
Member Author

Torxed commented Jan 26, 2022

GRUB with encryption installs, but I can't get it to boot to the boot loader, it skips the HDD and goes straight to ISO.

what kind of target disk are you using? I wasn't able to boot into Grub with a loopback device, IIRC not even with ext4. In fact the only way i could make it booting was within a virtualbox session, with an vdi target

I'm using archtest --branch torxed-fix-btrfs-mounts --rebuild --bios --new-drives to install and then archtest.py --bios --boot 'hdd' to test it. So it's KVM, two normal block devices. Skipping --bios when testing UEFI.

…installed before installing it (to avoid multiple calls to pacstrap even tho it doesn't hurt)
@Torxed
Copy link
Copy Markdown
Member Author

Torxed commented Jan 26, 2022

screenshot

Grub installs on UEFI, but isn't bootable. I'm wondering if it's because ovmf and grub some how.

@Torxed
Copy link
Copy Markdown
Member Author

Torxed commented Jan 26, 2022

Think I found the cause, these two lines doesn't do what it's supposed to:

add_to_CMDLINE_LINUX = f"sed -i 's/GRUB_CMDLINE_LINUX=\"\"/GRUB_CMDLINE_LINUX=\"{options_entry}\"/'"
SysCommand(f"/usr/bin/arch-chroot {self.target} {add_to_CMDLINE_LINUX} {_file}")

It's called (verified through cmd_history.txt), but it doesn't replace the file content as it should. Not sure how this ever worked. Might be because subvol=/ where / escapes the string. Yepp that was it..

@Torxed
Copy link
Copy Markdown
Member Author

Torxed commented Jan 26, 2022

Everything appears to work correctly, commands are executed as they should.
But for whatever reason, it doesn't work yet.

screenshot

screenshot1

screenshot2

screenshot3

screenshot4

Checked the output of grub-install and it seems fine:
screenshot

@Torxed
Copy link
Copy Markdown
Member Author

Torxed commented Jan 26, 2022

GRUB on EFI without subvolumes is also broken. Even in v2.3.1-dev.
I'm tempted to merge this as it's probably better to try to solve GRUB + UEFI issues in a separate PR. This PR will not worsen, only make other aspects work better.

Torxed and others added 2 commits January 26, 2022 09:38
Added "--removable" after "--bootloader-id=GRUB" on Line 669, because it would throw an input/output error without it on my laptop
@Torxed
Copy link
Copy Markdown
Member Author

Torxed commented Jan 26, 2022

Works now with the latest change from #793 :D

@Torxed
Copy link
Copy Markdown
Member Author

Torxed commented Jan 26, 2022

screenshot

If everyone is fine with this PR, I'm merging it. Pinging a few that I know has experience with these things: @wllacer @dylanmtaylor

@wllacer
Copy link
Copy Markdown
Contributor

wllacer commented Jan 26, 2022

I like what you did on the naming of the encrypted devices ... as for the rest, it feels a lot better
With a 2.3.1-dev from yesterday I've tested Grub+btrfs+ Uefi under VirtualBox. I could boot into Archlinux, almost ... It hung while loading the initial ram disk

@Torxed
Copy link
Copy Markdown
Member Author

Torxed commented Jan 26, 2022

I like what you did on the naming of the encrypted devices ... as for the rest, it feels a lot better With a 2.3.1-dev from yesterday I've tested Grub+btrfs+ Uefi under VirtualBox. I could boot into Archlinux, almost ... It hung while loading the initial ram disk

What specifically do you not like? I can change things around and modify it.
But I would need to know what doesn't feel great.

@wllacer
Copy link
Copy Markdown
Contributor

wllacer commented Jan 26, 2022 via email

@Torxed
Copy link
Copy Markdown
Member Author

Torxed commented Jan 26, 2022

Just the opposite. I find the final result very good. Sorry for Haven't been able to follow the grub issue further. Today isn't the BEST day

Ah apologies, my English skills sometimes messes with me hehe. Read your message as if there was something wrong with v2.3.1-dev heh. My bad, and I hope your day gets better! :D

@Torxed Torxed merged commit aa3b79c into v2.3.1-dev Jan 26, 2022
@Torxed Torxed deleted the torxed-fix-btrfs-mounts branch January 26, 2022 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consider changing the BTRFS subvolume layout

3 participants