search

NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
17.56k stars 13.72k forks source link

Revert #16903 zfs: Force sync on shutdown #343014

Open matdibu opened 1 week ago

matdibu commented 1 week ago

Issue description

Half of my zpool history is the zfs set nixos:shutdown-time=... message set in #16903 by @Baughn

And that PR explicitly modifies the behaviour of ZFS' sync=disabled; I don't think we should go against the users that decide to use that feature.

And for the large majority who left sync enabled, it's not doing much aside from setting a timestamp

Overall, I suggest removing this overriding behaviour.

Steps to reproduce

  1. use ZFS on NixOS
  2. reboot
  3. check the output of zpool history
Baughn commented 1 week ago

That would be a data-loss regression for anyone relying on the current behaviour, so I'm heavily against that. The idea is to allow performance improvements at the cost of time reversal if the system crashes; not to allow it at the cost of data loss on every single reboot.

I'm not sure if zpool sync will do the job, but that's testable. If it does not, how about making the forced sync only happen if sync happens to be disabled on some filesystem?

matdibu commented 1 week ago

I understand the risks, but so should anyone who goes out of their way to set sync=disabled, it's not a default setting and it comes with its own documented caveats.

As it stands, we override what the user set in their zfs config, just because we know better than them.

In my mind, this is in the same spirit as shipping a patch to openzfs that outright disables the feature, but still accepts the flag and doesn't notify the user of the non-upstream behaviour.