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

preallocateContents option: disable by default #4094

Merged
merged 1 commit into from Nov 26, 2020
Merged

Conversation

@martinetd
Copy link
Contributor

@martinetd martinetd commented Sep 29, 2020

On btrfs, fallocate preallocate file space and basically make it no-cow,
thus disabling compression which is probably desirable for nix store.

Change the preallocateContents to tristate true/false/auto and if
auto disable preallocation on btrfs.

Fixes #3550.


set as draft because I'm not happy with this:

  • I think it'd make sense to have the tristate enum somewhere common, config.hh/config.cc ? would that make more sense? if not I'll keep it here and rename the enum to make it specific to that option like SandboxMode that @cole-h suggested.
  • @Ericson2314 I tried an enum struct (enum class appears elsewhere in the code so I used that, it's apparently the same) but it wouldn't let me reuse true/false/auto keywords -- I'm very sad, what's the point of namespacing then :P erm, sorry, I mean I'll rename that when I move the option off.

That aside I ended up going for a "check the first time it's used and update the option" -- it works well enough. The daemon forks when a request comes in so it basically checks with statfs everytime a new request comes in, but that's probably reasonable enough.

@martinetd martinetd force-pushed the btrfs branch 3 times, most recently from 3c59d27 to 5221790 Sep 30, 2020
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Oct 21, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/good-filesystem-for-the-nix-store/3566/13

@martinetd martinetd marked this pull request as ready for review Oct 23, 2020
@martinetd
Copy link
Contributor Author

@martinetd martinetd commented Oct 23, 2020

As stated in the related issue, in the absence of feedback I did what I thought was best and removed draft:

  • first commit adds a tristate AutoOption::{True,False,Auto} in config.cc/hh
  • second commit uses it and does fstype check

I'll be glad to fix things if requested, let's get this issue finally closed! and I'll let you folks decide what to backport, the first PR might be useful on stable nix or we can just tell people to use nixUnstable if they want compression on btrfs

@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Oct 28, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/good-filesystem-for-the-nix-store/3566/26

@symphorien
Copy link
Member

@symphorien symphorien commented Nov 5, 2020

It looks like it works :)

I rebuilt my nixos system with nix from this commit, and fetched ghc from the binary cache on a zstd-compressed btrfs file system: compression works:

$  sudo compsize /nix/store/v04hbr8l861w5w3i05mb8n2xbp9jaa1i-ghc-8.8.4-doc
Processed 3681 files, 4181 regular extents (4320 refs), 664 inline.
Type       Perc     Disk Usage   Uncompressed Referenced  
TOTAL       14%       39M         273M         276M       
none       100%      195K         195K         579K       
zstd        14%       39M         272M         275M 

@martinetd
Copy link
Contributor Author

@martinetd martinetd commented Nov 22, 2020

btw if this is not merged because too complex I'd suggest just disabling fallocate by default, here's a rant from #xfs@freenode earlier today (TL;DR: don't use fallocate()):

22:07:55 < dchinner> file-by-file fallocate() is actively harmful to the filesystem because 
                     it defeats delayed allocation.
22:08:15 < dchinner> the filesystem is far smarter about allocation than user applications think they 
                     are.
22:08:42 < dchinner> in general, fallocate() should be considered harmful and not the right thing to 
                     do....
22:10:48 < dchinner> IIRC, it was first introduced in about 1990 with EFS - the filesytem 
                     that XFS replaced on Irix....
22:10:59 < dchinner> XFS has had it since day zero....
22:11:16 < dchinner> ext4 and btrfs now use it as well
22:11:32 < dchinner> (and have since 2008-2009)
22:12:56 < dchinner> But because everyone is a filesystem expert they think they can do better than 
                     filesystems with up front fallocate()
22:13:50 < dchinner> without understanding that delayed allocation solves all the problems up front 
                     allocation causes (which is what all filesystems did before delalloc)

disabling fallocate by default is a one line change from current code and would probably make everyone happy...

@edolstra
Copy link
Member

@edolstra edolstra commented Nov 26, 2020

Yes I wouldn't mind disabling it by default. Contiguous allocation is also a lot less important now that everybody uses SSDs.

using fallocate() to preallocate files space does more harm than good:
 - breaks compression on btrfs
 - has been called "not the right thing to do" by xfs developers
(because delayed allocation that most filesystems implement leads to smarter
allocation than what the filesystem needs to do if we upfront fallocate files)
@martinetd martinetd changed the title preallocateContents option: automatically set on btrfs preallocateContents option: disable by default Nov 26, 2020
@martinetd
Copy link
Contributor Author

@martinetd martinetd commented Nov 26, 2020

Yes I wouldn't mind disabling it by default. Contiguous allocation is also a lot less important now that everybody uses SSDs.

Ok, updated this branch to just toggle the boolean value.

Thanks!

@edolstra edolstra merged commit 2458270 into NixOS:master Nov 26, 2020
2 checks passed
@edolstra
Copy link
Member

@edolstra edolstra commented Nov 26, 2020

Thanks!

@bbigras
Copy link

@bbigras bbigras commented Dec 1, 2020

Is this change usable in nixos-unstable or do we need to wait for a nix release?

@martinetd
Copy link
Contributor Author

@martinetd martinetd commented Dec 1, 2020

Is this change usable in nixos-unstable or do we need to wait for a nix release?

Last update to nixUnstable in nixpkgs master was around Nov 18, so it's not there yet -- I'm not sure how "good commits" are decided but you can probably just open a PR like NixOS/nixpkgs#104289 that just bumps the suffix/rev/hash to today's commits :)

@bbigras
Copy link

@bbigras bbigras commented Dec 1, 2020

Done, thanks.

@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Jan 7, 2021

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/good-filesystem-for-the-nix-store/3566/27

@stephanos
Copy link

@stephanos stephanos commented Jan 23, 2021

Sorry to jump into this so late, but I'm seeing the error

preallocating file of 34 bytes: Operation not permitted

and that led me here.

According to nix-channel --list, I'm on nixpkgs https://nixos.org/channels/nixpkgs-unstable. I'm using Nix 2.3.10. So if I understand this thread correctly, it should have made its way there by now.

What confuses me is that I can't seem to disable it using preallocate-contents = false in /etc/nix/nix.conf. It just shows me warning: unknown setting 'preallocate-contents'.

I must be clearly missing something. If you have any pointers, I'd appreciate that :)

@martinetd
Copy link
Contributor Author

@martinetd martinetd commented Jan 23, 2021

According to nix-channel --list, I'm on nixpkgs https://nixos.org/channels/nixpkgs-unstable. I'm using Nix 2.3.10. So if I understand this thread correctly, it should have made its way there by now.

What confuses me is that I can't seem to disable it using preallocate-contents = false in /etc/nix/nix.conf. It just shows me warning: unknown setting 'preallocate-contents'.

I must be clearly missing something. If you have any pointers, I'd appreciate that :)

2.3 is the stable branch, you need to be on 2.4 (e.g. having opted it to nixUnstable or for example nix flakes that pull it) to have this option.
I don't know what's the plan for when the switch to 2.4 as stable will happen, if people are interested it might be worth backporting now the change has gotten simple? OTOH it's simple enough to say nix.package = pkgs.nixUnstable in your configuration once you know you need it.

@stephanos
Copy link

@stephanos stephanos commented Jan 23, 2021

aah, that worked! thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants