Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Btrfs mount options #2404

Merged
merged 1 commit into from
Mar 11, 2024
Merged

Conversation

codefiles
Copy link
Contributor

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.

@codefiles codefiles requested a review from Torxed as a code owner March 11, 2024 01:10
@codefiles codefiles force-pushed the fix-btrfs-mnt-options branch 2 times, most recently from c239194 to b7ff652 Compare March 11, 2024 03:00
@Torxed Torxed merged commit c210cdc into archlinux:master Mar 11, 2024
6 of 7 checks passed
@codefiles codefiles deleted the fix-btrfs-mnt-options branch March 11, 2024 13:10
@svartkanin
Copy link
Collaborator

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
Copy link
Contributor Author

codefiles commented Mar 13, 2024

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

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 []

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
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants