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

bcachefs: add support for subvolumes #499

Closed
wants to merge 1 commit into from

Conversation

adamcstephens
Copy link

@adamcstephens adamcstephens commented Jan 8, 2024

Picked up the work started by Raito. 6.7 was just released, but isn't in unstable yet.

Closes: #449

lib/types/bcachefs.nix Outdated Show resolved Hide resolved
@Lassulus
Copy link
Collaborator

Lassulus commented Jan 9, 2024

do we want to extend this also for multidisks? if yes then we need to split the bcachefs type into a type which marks the disk for including it in a bcachefs array and another toplevel type to assemble the array. The toplevel type would be mostly the same as the bcachefs type here, but with support for multiple disks.

@Mic92
Copy link
Member

Mic92 commented Jan 9, 2024

I think even if we don't implement this feature fully just now, we should already prepare it in this direction. Otherwise we need to break compatibility with people soon again.

@adamcstephens
Copy link
Author

The docs provide a complex example of how one formats a multi-disk filesystem, https://bcachefs-docs.readthedocs.io/en/latest/mgmt-formatting.html . Based on that, I would probably suggest we create our own construct, a multi-disk toplevel type named something like bcachefs_multi to clearly distinguish it from a simple filesystem. I would then suggest we duplicate the subvolume section into that new type instead of trying to overload a single bcachefs type in the two very different use cases. I'm also not excited about requiring users of bcachefs to build a toplevel config when they just want a simple filesystem, maybe with subvolumes, that the fs clearly targets as well.

In that case, I'm failing to see how what is done here would break compatibility for a separate type. Thoughts?

name = "root";
end = "-0";
content = {
type = "bcachefs";
Copy link
Member

Choose a reason for hiding this comment

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

If we have two types, than we should rename this one to bcachefs_single or similar. Otherwise it creates confusion when people try to use bcachefs for multi disk setups.

Copy link

@Sntx626 Sntx626 left a comment

Choose a reason for hiding this comment

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

Looks mostly good.

I agree that we should start separating between single- and multi-disk bcachefs types with this PR, even if it doesn't implement the functionality.

Also I'm not sure, but is there an assertion that allows us to check whether the user has defined a bcachefs enabled kernel (i.e. by comparing versions, even if inputs.nixpkgs is overwritten)?
I think that would be better than just overwriting whatever the user defines.

nixpkgs.overlays = [
(_final: super: { zfs = super.zfs.overrideAttrs (_: { meta.platforms = [ ]; }); })
];
boot.kernelPackages = pkgs.lib.mkForce pkgs.linuxPackages_testing;
Copy link

Choose a reason for hiding this comment

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

This can be changed to pkgs.linuxPackages_latest - testing is no longer required on unstable

nixpkgs.overlays = [
(_final: super: { zfs = super.zfs.overrideAttrs (_: { meta.platforms = [ ]; }); })
];
boot.kernelPackages = pkgs.lib.mkForce pkgs.linuxPackages_testing;
Copy link

Choose a reason for hiding this comment

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

This can be changed to pkgs.linuxPackages_latest - testing is no longer required on unstable

@raj-magesh
Copy link

raj-magesh commented Mar 6, 2024

Would this PR enable declarative configuration of a single bcachefs filesystem spanning:

  • disk A
  • partition B1 on disk B
  • LUKS-encrypted partition /dev/mapper/<label> on disk C

I could help test it if so.

@adamcstephens
Copy link
Author

Would this PR enable declarative configuration of a single bcachefs filesystem spanning

No, that would be different effort to extend multi disk support.

I’m actually going to close this as I won’t be finishing it any time soon. Someone else should feel free to pick it up.

@Lassulus
Copy link
Collaborator

Lassulus commented Mar 6, 2024

We probably need a generic solution for mutli device support which integrates nicely with the single disk usecase.

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.

None yet

6 participants