Skip to content
New issue

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

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

Already on GitHub? # to your account

tool: only sync ESP filesystem #219

Merged
merged 1 commit into from
Sep 13, 2023
Merged

Conversation

tilpner
Copy link
Contributor

@tilpner tilpner commented Aug 17, 2023

I noticed lanzaboote was adding quite a bit of time to my nixos-rebuild time, and tracked it to the sync() call. That currently syncs all filesystems, even if it only cares about the ESP filesystem. This commit opens the root directory of the ESP filesystem, passes it to syncfs instead, which should result in just syncing the ESP filesystem.

  • This is mostly untested, but it passes all flake checks
    • I have no idea how to (automatically) test that this still reliably syncs anything
  • Stop most overwriting in the ESP #204 probably helps more in reducing sync time than this PR will.

@@ -161,7 +162,8 @@ impl Installer {
// Sync files to persistent storage. This may improve the
// chance of a consistent boot directory in case the system
// crashes.
sync();
let boot = File::open(&self.esp_paths.esp).context("Failed to open ESP root directory.")?;
syncfs(boot.as_raw_fd()).context("Failed to sync ESP filesystem.")?;
Copy link
Contributor Author

@tilpner tilpner Aug 17, 2023

Choose a reason for hiding this comment

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

I was going to format these two lines to match the other .context(...)? lines, but rustfmt disagrees.

@Mic92 Mic92 requested a review from nikstur August 30, 2023 19:14
@dasJ
Copy link
Member

dasJ commented Aug 30, 2023

Just a little pointer, switch-to-configuration.pl implements a variable NIXOS_NO_SYNC (as outlined in the docs).
I don't expect this to be added in this PR, just wanted to let everyone know that there may be people who want to set this

@Mic92
Copy link
Member

Mic92 commented Aug 30, 2023

Works on my machine.

Copy link
Member

@RaitoBezarius RaitoBezarius left a comment

Choose a reason for hiding this comment

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

This has waited enough, LGTM.

@RaitoBezarius RaitoBezarius added this pull request to the merge queue Sep 13, 2023
Merged via the queue into nix-community:master with commit 9a9b096 Sep 13, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants