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

nixos/profiles/base: support bcachefs #361461

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MakiseKurisu
Copy link
Contributor

@MakiseKurisu MakiseKurisu commented Dec 3, 2024

Add bcachefs support in the base profile.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: changelog 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Dec 3, 2024
@MakiseKurisu
Copy link
Contributor Author

MakiseKurisu commented Dec 9, 2024

Test condition was updated to check the actual underlying kernel package:

nix-repl> pkgs.linuxPackages.kernel == pkgs.linuxPackages_6_6.kernel
true

nix-repl> pkgs.linuxPackages == pkgs.linuxPackages_6_6              
false

However, evaluating with the current bcachefs ISO sample code sans the explicit bcachefs enablement is still causing error:

       … while evaluating 'config.boot.kernelPackages' to select 'kernel' on it
         at /nix/store/py8fn4zjmy5fmfrchrml4rljs0wjngvm-source/nixos/modules/profiles/base.nix:51:19:
           50|     # when bcachefs module no longer defaults to latest kernel
           51|     lib.optional (config.boot.kernelPackages.kernel == pkgs.linuxPackages_latest.kernel) "bcachefs" ++
             |                   ^
           52|     lib.optional (lib.meta.availableOn pkgs.stdenv.hostPlatform config.boot.zfs.package) "zfs";

       … from call site
         at /nix/store/py8fn4zjmy5fmfrchrml4rljs0wjngvm-source/nixos/modules/profiles/base.nix:51:19:
           50|     # when bcachefs module no longer defaults to latest kernel
           51|     lib.optional (config.boot.kernelPackages.kernel == pkgs.linuxPackages_latest.kernel) "bcachefs" ++
             |                   ^
           52|     lib.optional (lib.meta.availableOn pkgs.stdenv.hostPlatform config.boot.zfs.package) "zfs";

       … while calling anonymous lambda
         at /nix/store/py8fn4zjmy5fmfrchrml4rljs0wjngvm-source/lib/attrsets.nix:1204:18:
         1203|         mapAttrs
         1204|           (name: value:
             |                  ^
         1205|             if isAttrs value && cond value

       … from call site
         at /nix/store/py8fn4zjmy5fmfrchrml4rljs0wjngvm-source/lib/attrsets.nix:1207:18:
         1206|             then recurse (path ++ [ name ]) value
         1207|             else f (path ++ [ name ]) value);
             |                  ^
         1208|     in

       … while calling anonymous lambda
         at /nix/store/py8fn4zjmy5fmfrchrml4rljs0wjngvm-source/lib/modules.nix:254:72:
          253|           # For definitions that have an associated option
          254|           declaredConfig = mapAttrsRecursiveCond (v: ! isOption v) (_: v: v.value) options;
             |                                                                        ^
          255|

       … while evaluating the attribute 'value'
         at /nix/store/py8fn4zjmy5fmfrchrml4rljs0wjngvm-source/lib/modules.nix:816:9:
          815|     in warnDeprecation opt //
          816|       { value = addErrorContext "while evaluating the option `${showOption loc}':" value;
             |         ^
          817|         inherit (res.defsFinal') highestPrio;

       error: infinite recursion encountered
       at /nix/store/py8fn4zjmy5fmfrchrml4rljs0wjngvm-source/lib/modules.nix:816:9:
          815|     in warnDeprecation opt //
          816|       { value = addErrorContext "while evaluating the option `${showOption loc}':" value;
             |         ^
          817|         inherit (res.defsFinal') highestPrio;

@Mic92
Copy link
Member

Mic92 commented Dec 9, 2024

Test condition was updated to check the actual underlying kernel package:

nix-repl> pkgs.linuxPackages.kernel == pkgs.linuxPackages_6_6.kernel
true

nix-repl> pkgs.linuxPackages == pkgs.linuxPackages_6_6              
false

However, evaluating with the current bcachefs ISO sample code sans the explicit bcachefs enablement is still causing error:

       … while evaluating 'config.boot.kernelPackages' to select 'kernel' on it
         at /nix/store/py8fn4zjmy5fmfrchrml4rljs0wjngvm-source/nixos/modules/profiles/base.nix:51:19:
           50|     # when bcachefs module no longer defaults to latest kernel
           51|     lib.optional (config.boot.kernelPackages.kernel == pkgs.linuxPackages_latest.kernel) "bcachefs" ++
             |                   ^
           52|     lib.optional (lib.meta.availableOn pkgs.stdenv.hostPlatform config.boot.zfs.package) "zfs";

       … from call site
         at /nix/store/py8fn4zjmy5fmfrchrml4rljs0wjngvm-source/nixos/modules/profiles/base.nix:51:19:
           50|     # when bcachefs module no longer defaults to latest kernel
           51|     lib.optional (config.boot.kernelPackages.kernel == pkgs.linuxPackages_latest.kernel) "bcachefs" ++
             |                   ^
           52|     lib.optional (lib.meta.availableOn pkgs.stdenv.hostPlatform config.boot.zfs.package) "zfs";

       … while calling anonymous lambda
         at /nix/store/py8fn4zjmy5fmfrchrml4rljs0wjngvm-source/lib/attrsets.nix:1204:18:
         1203|         mapAttrs
         1204|           (name: value:
             |                  ^
         1205|             if isAttrs value && cond value

       … from call site
         at /nix/store/py8fn4zjmy5fmfrchrml4rljs0wjngvm-source/lib/attrsets.nix:1207:18:
         1206|             then recurse (path ++ [ name ]) value
         1207|             else f (path ++ [ name ]) value);
             |                  ^
         1208|     in

       … while calling anonymous lambda
         at /nix/store/py8fn4zjmy5fmfrchrml4rljs0wjngvm-source/lib/modules.nix:254:72:
          253|           # For definitions that have an associated option
          254|           declaredConfig = mapAttrsRecursiveCond (v: ! isOption v) (_: v: v.value) options;
             |                                                                        ^
          255|

       … while evaluating the attribute 'value'
         at /nix/store/py8fn4zjmy5fmfrchrml4rljs0wjngvm-source/lib/modules.nix:816:9:
          815|     in warnDeprecation opt //
          816|       { value = addErrorContext "while evaluating the option `${showOption loc}':" value;
             |         ^
          817|         inherit (res.defsFinal') highestPrio;

       error: infinite recursion encountered
       at /nix/store/py8fn4zjmy5fmfrchrml4rljs0wjngvm-source/lib/modules.nix:816:9:
          815|     in warnDeprecation opt //
          816|       { value = addErrorContext "while evaluating the option `${showOption loc}':" value;
             |         ^
          817|         inherit (res.defsFinal') highestPrio;

If you build an image with bcache support based on the installer profile, you can disable zfs like this:

# take from: cd-dvd/installation-cd-minimal-new-kernel-no-zfs.nix
{
  imports = [ ./installation-cd-minimal-new-kernel.nix ]; 
 
  boot.supportedFilesystems.zfs = lib.mkForce false;
} 

Or enable zfsUnstable (which supports a kernel also supported by bcachefs.
However this is not something we can solve in the bcachefs module.
There exist constellations which support both zfs and bcachefs (we have that https://github.com/nix-community/nixos-images). Therefore we should not disable zfs in the bcachefs module.

@MakiseKurisu
Copy link
Contributor Author

Here is the full code that I'm testing with that got the above infinite recursion, which is already using no-zfs profile:

# nix build --show-trace .#nixosConfigurations.exampleIso.config.system.build.isoImage
{
  description = "Bcachefs enabled installation media";
  inputs.nixos.url = "github:MakiseKurisu/nixpkgs/bcachefs-base";
  outputs = { self, nixos }: {
    nixosConfigurations = {
      exampleIso = nixos.lib.nixosSystem {
        system = "x86_64-linux";
        modules = [
          "${nixos}/nixos/modules/installer/cd-dvd/installation-cd-minimal-new-kernel-no-zfs.nix"
          ({ lib, pkgs, ... }: {
            # Might be required as a workaround for bug
            # https://github.com/NixOS/nixpkgs/issues/32279
            environment.systemPackages = [ pkgs.keyutils ];
            # boot.supportedFilesystems = [ "bcachefs" ];
            # boot.kernelPackages = pkgs.linuxPackages_latest;
          })
        ];
      };
    };
  };
}

Either way I don't think this is related to ZFS, but I'm also not sure how to fix this infinite recursion issue.

@Mic92
Copy link
Member

Mic92 commented Dec 9, 2024

Ah. Actually I noticed you removed the force override filesystems. I think now it's fine actually.

@MakiseKurisu
Copy link
Contributor Author

Um this PR currently does not build the sample I provided. My guess is that I'm referencing config.boot.kernelPackages which is also touched by bcachefs module if it is enabled. I'm currently out of idea of how to fix it.

@Mic92
Copy link
Member

Mic92 commented Dec 10, 2024

Um this PR currently does not build the sample I provided. My guess is that I'm referencing config.boot.kernelPackages which is also touched by bcachefs module if it is enabled. I'm currently out of idea of how to fix it.

Maybe just enable it in the installer profile that adds the latest kernel?

Comment on lines 12 to 14
boot.postBootCommands = ''
${lib.getExe pkgs.keyutils "keyctl"} link @u @s
'';
Copy link
Member

Choose a reason for hiding this comment

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

But this should be part of the bcachefs module no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In keyctl's man page it says:

       Session keyring: @s or -3
              Each process subscribes to a session keyring that is
              inherited across (v)fork, exec and clone. This is searched
              after the process keyring. Session keyrings can be named
              and an extant keyring can be joined in place of a
              process's current session keyring.

       User specific keyring: @u or -4
              This keyring is shared between all the processes owned by
              a particular user. It isn't searched directly, but is
              **normally linked to from the session keyring.**

So it looks like this might be a more general configuration that are not limited to bcachefs usage. But I'm not sure why we didn't link those 2 keyrings in the first place, and if that actually should be a general setting. So I'm just doing the safe thing as listed in the NixOS Wiki.

@@ -6,6 +6,8 @@

- The default PHP version has been updated to 8.3.

- `bcachefs` is now enabled in the installation media with a suitable kernel.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `bcachefs` is now enabled in the installation media with a suitable kernel.
- `bcachefs` is now enabled in the installation media with the latest kernel available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can see 25.05 releases with 6.12 as the default, and hopefully bcachefs module can drop its version override. In that case we just go back to the version check (>=6.7) so a suitable kernel will be more appropriate.

But then it turned out currently it is quite difficult to check if the config.boot.kernelPackages is actually pkgs.linuxPackages_latest, so this PR is no longer the one-liner I was expecting. Might be better to actually wait for those blockages to be fixed first before continuing.

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 31, 2024
@MakiseKurisu
Copy link
Contributor Author

6.12 is the default now: #370410

@Mic92
Copy link
Member

Mic92 commented Jan 23, 2025

Yeah that means we could just enable it without any hacks.

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 23, 2025
@MakiseKurisu MakiseKurisu force-pushed the bcachefs-base branch 5 times, most recently from a192bd0 to 3de2b11 Compare January 23, 2025 16:20
@MakiseKurisu
Copy link
Contributor Author

Locally built the ISO for 6.6 and default, and confirmed nix-store.squashfs contains bcachefs stuff if kernelPackages is commented out. Too lazy to spin up a VM for the night though.

# nix build --show-trace .#nixosConfigurations.exampleIso.config.system.build.isoImage
{
  description = "Bcachefs enabled installation media";
  inputs.nixos.url = "github:MakiseKurisu/nixpkgs/bcachefs-base";
  outputs = { self, nixos }: {
    nixosConfigurations = {
      exampleIso = nixos.lib.nixosSystem {
        system = "x86_64-linux";
        modules = [
          "${nixos}/nixos/modules/installer/cd-dvd/installation-cd-minimal.nix"
          ({ lib, pkgs, ... }: {
            boot.kernelPackages = pkgs.linuxPackages_6_6;
          })
        ];
      };
    };
  };
}

@MakiseKurisu MakiseKurisu marked this pull request as ready for review January 23, 2025 16:49
@MakiseKurisu MakiseKurisu requested a review from Mic92 January 23, 2025 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants