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

Systemd stage 1 fsck #208269

Merged
merged 4 commits into from
Feb 17, 2023
Merged

Systemd stage 1 fsck #208269

merged 4 commits into from
Feb 17, 2023

Conversation

ElvishJerricco
Copy link
Contributor

@ElvishJerricco ElvishJerricco commented Dec 29, 2022

Description of changes

Until now, systemd-stage-1 has not supported fsck, which is a major problem.

Incidentally, this will also technically help with hibernation. Currently, the unit ordering isn't quite right, so you'll find /sysroot mounting at the same time that systemd-hibernate-resume is starting, which is very dangerous. The problem is that mount units in stage 1 don't get the After=local-fs-pre.target dependency, which is the sync point that systemd-hibernate-resume orders itself before. But systemd-fsck is ordered after local-fs-pre.target, so it works if fsck is working. Realistically, I think the lack of this ordering on the mount units should be considered a bug in systemd, and the patch to fix that bug is like one line, but I haven't opened an issue/PR about that yet.

Targeting staging because the systemd patch that changes how systemd finds fsck had to be modified.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.05 Release Notes (or backporting 22.11 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ElvishJerricco ElvishJerricco requested a review from a team as a code owner December 29, 2022 19:32
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: systemd 8.has: module (update) This PR changes an existing module in `nixos/` labels Dec 29, 2022
@ElvishJerricco ElvishJerricco marked this pull request as draft December 29, 2022 19:33
@ofborg ofborg bot requested review from Mic92, kloenk and flokli December 29, 2022 20:55
@flokli
Copy link
Contributor

flokli commented Jan 2, 2023

Marking as draft because there's a systemd patch file. It has been merged upstream, but I haven't asked them to backport it to stable. (This is also why it's targeting staging)

There's currently no link to the upstream PR, making it very hard to check if upstream has been poked in the meantime or not.

We normally try to not increase our patch count, so please ask upstream first.

@ElvishJerricco
Copy link
Contributor Author

@flokli Ah, sorry. I've updated the OP with a link to the PR, and I've asked there if it could be backported to systemd-stable

@ElvishJerricco
Copy link
Contributor Author

(Note that although that PR was a followup to systemd/systemd#25710, it is not required, because NixOS currently doesn't use the cmdline for root and usrt. But systemd-fsck is generated differently for root and usr solely based on the mountpoint path rather than whether they were provided by the cmdline.)

@ElvishJerricco ElvishJerricco marked this pull request as ready for review February 8, 2023 05:44
@ElvishJerricco
Copy link
Contributor Author

Now that v252.5 is in staging, the new patch file that was needed has been backported and is no longer needed.

Copy link
Member

@lilyinstarlight lilyinstarlight left a comment

Choose a reason for hiding this comment

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

The diff looks reasonable to me, and the new fsck test passes on my system

I've been running this on my machine (only patching systemdStage1 to avoid too many rebuilds) for a few days now and have not noticed issues, but I admittedly don't have any filesystems marked neededForBoot that need fsck

Other systemd-stage-1 tests I ran all still succeeded as well

Thank you for this work!

@flokli flokli merged commit ab566b8 into NixOS:staging Feb 17, 2023
Comment on lines +334 to +336
boot.initrd.systemd.storePaths = [initrdFstab];
boot.initrd.systemd.managerEnvironment.SYSTEMD_SYSROOT_FSTAB = initrdFstab;
boot.initrd.systemd.services.initrd-parse-etc.environment.SYSTEMD_SYSROOT_FSTAB = initrdFstab;
Copy link
Contributor

Choose a reason for hiding this comment

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

@ElvishJerricco Could you share the reason for removing /etc/fstab from the initrd filesystem? I found that this broke one of my custom initrd units, which had a mount <mountpoint> line. I have a workaround, but I'm curious to understand the reason for this change so that I can choose the best workaround in my config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Majiir /etc/fstab was never the right choice; the systemd tooling wants you to use root= and mount.usr= on the cmdline. The service initrd-parse-etc.service reads /sysroot/etc/fstab, and so does the systemd-fstab-generator, which is what we're overriding with this environment variable.

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 6.topic: systemd 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 101-500 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants