archlinux / archinstall

Arch Linux installer - guided, templates etc.
GNU General Public License v3.0
5.95k stars 514 forks source link

Fix Btrfs mount options #2404

Closed codefiles closed 5 months ago

codefiles commented 5 months ago

Fixes #1571

In btrfs(5) under MOUNT OPTIONS there is this note:

Most mount options apply to the whole filesystem and only options in the first mounted subvolume will take effect. This is due to lack of implementation and may change in the future. This means that (for example) you can't set per-subvolume nodatacow, nodatasum, or compress using mount options. This should eventually be fixed, but it has proved to be difficult to implement correctly within the Linux VFS framework.

This pull request removes the ability to apply the compress and nodatacow mount options for individual subvolumes for this reason.

Selecting and toggling the nodatacow mount option for the file system has been added. Both the selection and toggle restrict compress and nodatacow from both being present since they conflict.

svartkanin commented 5 months ago

I just realized, it would be better to keep the mount options as types rather than strings

def select_mount_options() -> List[str]:
    prompt = str(_('Would you like to use compression or disable CoW?'))
    options = [str(_('Use compression')), str(_('Disable Copy-on-Write'))]
    choice = Menu(prompt, options, sort=False).run()

    if choice.type_ == MenuSelectionType.Selection:
        if choice.single_value == options[0]:
            return [BtrfsMountOption.compress.value]
        else:
            return [BtrfsMountOption.nodatacow.value]

    return []

If they need to be distinguished later on we have to do string comparison which is unsafe and annoying

codefiles commented 5 months ago

It is easier to follow along if you properly link to the code like this:

https://github.com/archlinux/archinstall/blob/df2884085dc06f71f2bb201a316828c23b6299dd/archinstall/lib/interactions/disk_conf.py#L218-L229

I just realized, it would be better to keep the mount options as types rather than strings

Would making that change interfere with allowing the user to provide mount options in the configuration file or make this more complicated?

    "disk_config": {
        ...
        "device_modifications": [
            {
                ...
                "partitions": [
                    {
                        ...
                        "mount_options": [
                            "example_option"
                        ],
                        ...
                    }
                ],
            }

If they need to be distinguished later on we have to do string comparison which is unsafe and annoying

Could you provide an example of why they would need to be distinguished later? Seems like everything is already taken care of but I might be missing something.

svartkanin commented 5 months ago

I just had another look and I missed that previously we were also storing plain strings, so please disregard my comment. If we wanted to change to this it would need to be something with a bigger picture