nixos/sysctl: dynamic inotify watches or higher defaults #126777

blaggacao · 2021-06-13T18:54:46Z

Nowadays most applications require a good amount of inotify watches,
so raise our default to what other distros do. If kernel supports it
enable dynamic setting.

nixos/modules/config/sysctl.nix

nh2

Great find, change looks sensible to me.

blaggacao · 2021-06-15T16:11:22Z

ping: this is supposed to be ready 🤷

nixos/modules/config/sysctl.nix

blaggacao · 2021-06-18T16:25:08Z

@GrahamcOfBorg test bittorrent
@GrahamcOfBorg test boot.biosCdrom
@GrahamcOfBorg test boot.biosNetboot
@GrahamcOfBorg test boot.biosUsb
@GrahamcOfBorg test boot.uefiCdrom
@GrahamcOfBorg test boot.uefiNetboot
@GrahamcOfBorg test boot.uefiUsb
@GrahamcOfBorg test installer.bcache
@GrahamcOfBorg test installer.btrfsSimple
@GrahamcOfBorg test installer.btrfsSubvolDefault
@GrahamcOfBorg test installer.btrfsSubvols
@GrahamcOfBorg test installer.encryptedFSWithKeyfile
@GrahamcOfBorg test installer.grub1
@GrahamcOfBorg test installer.luksroot
@GrahamcOfBorg test installer.lvm
@GrahamcOfBorg test installer.separateBoot
@GrahamcOfBorg test installer.simple
@GrahamcOfBorg test installer.swraid
@GrahamcOfBorg test installer.fsroot

blaggacao · 2021-06-21T16:11:44Z

@alyssais Can we merge this? All feedback seems to be duly attended.

fzakaria · 2021-06-23T03:36:12Z

This change LGTM.

fzakaria

I think it's great.
Small nit would be making it a let variable or a re-usable variable for INT_MAX.
(Probably re-used elsewhere)

fzakaria · 2021-06-23T04:00:05Z

nixos/modules/config/sysctl.nix

+    # https://github.com/torvalds/linux/commit/ac7b79fd190b02e7151bc7d2b9da692f537657f3
+    boot.kernel.sysctl."fs.inotify.max_user_instances" =
+      if (config.boot.kernelPackages.kernel.kernelAtLeast "5.12")
+      then mkDefault 2147483647 # INT_MAX, dynamically limited based on available memory


Can we make boot.kernel.INT_MAX a variable?

I guess, it would be more like in the stdenv, though or lib or even builtin. I can make a quick follow-up PR once this is merged.

maralorn · 2021-06-23T13:46:15Z

I don‘t understand the max_user_instances default.

I guess it makes sense for max_user_watches. The commit explains that the default value changes with 5.11 so we set that same value for everything below 5.11. Seems reasonable.

For max_user_instances we only change the default for everything above 5.12, if I understand kernelAtLeast correctly. Why is that?

It might be just right, but it looks like different cases. So describing them both with "Dynamic since X" confuses me. Can you maybe elaborate that?

blaggacao · 2021-06-23T14:29:39Z

Can you maybe elaborate that?

The upstream commit formulation is so intricate that I don't dare to give an approximate interpretation here inline.

torvalds/linux@ac7b79f

It reads:

With inotify instances charged to memcg [since 5.12], the admin can simply set max_user_instances to INT_MAX and let the memcg limits of the jobs limit their inotify instances.

everything above 5.12

... and including 5.12.

maralorn · 2021-06-23T14:40:05Z

Okay, do you know what harm it would do do raise that limit unconditionally?

And related: Back porting a new default is one thing, but are we sure it’s a good idea to override an existing default for current and future kernels?

Edit: For reference I routinely increase max_user_watches. I never needed to increase max_user_instances.

blaggacao · 2021-06-23T15:31:16Z

Thanks for the continued discussion!

Okay, do you know what harm it would do do raise that limit unconditionally?

I don't for sure, but I assume that it's an int value, so it cannot receive more than MAX_INT.

And related: Back porting a new default is one thing, but are we sure it’s a good idea to override an existing default for current and future kernels?

This was inspired by how it was argued in the related issue: #36214, seems to be a requirement for docker or rather: in general name-spacing. I innocently assume because of those namespaces actually count towards this maximum.

Since this is a maximum boundary and implemented as a default, it is in any case as close to a pareto-improvement as we can get (doesn't do harm to anybody, but good to many). The maximum is only hit, if you actually use it.

maralorn · 2021-06-23T18:07:42Z

Okay, do you know what harm it would do do raise that limit unconditionally?

I don't for sure, but I assume that it's an int value, so it cannot receive more than MAX_INT.

That’s not what I meant. I was wondering why we only do the override from 5.12 and newer.

But since then I think I understood it. I suggest adding a comment of the form:
From version 5.12 inotify watches count against a RAM resource limit, so it’s safe to increase this limit to the maximum
(If this is factually correct.)

And related: Back porting a new default is one thing, but are we sure it’s a good idea to override an existing default for current and future kernels?

This was inspired by how it was argued in the related issue: #36214, seems to be a requirement for docker or rather: in general name-spacing. I innocently assume because of those namespaces actually count towards this maximum.

Since this is a maximum boundary and implemented as a default, it is in any case as close to a pareto-improvement as we can get (doesn't do harm to anybody, but good to many). The maximum is only hit, if you actually use it.

Well the resulting harm would be if the user creates a resource exhaustion they don‘t want. So this wouldn‘t be a "pareto-improvement". The kernel has these limits for a reason. But if my phrasing above is correct, it’s fine to raise it.

maralorn · 2021-06-23T20:33:23Z

nixos/modules/config/sysctl.nix

+    # Dynamic since kernel v5.12
+    # https://github.com/torvalds/linux/commit/ac7b79fd190b02e7151bc7d2b9da692f537657f3
+    # From version 5.12 inotify watches count against a RAM resource limit, so kernel
+    # mainatainers (see commit) suggest to increase this limit to the maximum


Suggested change

# mainatainers (see commit) suggest to increase this limit to the maximum

# maintainers (see commit) suggest to increase this limit to the maximum

Nowadays most applications require a good amount of inotify watches, so raise our default to what other distros do. If kernel supports it enable dynamic setting. fixes NixOS#36214, NixOS#65001 (re-fix) - NixOS/nixops#890 - divnix/digga#209 replaces: NixOS#112472

maralorn · 2021-06-26T13:53:09Z

I think I am 100% in favor of the instances change. It does basically what the kernel devs recommend.

I am a little bit uncertain about the watches change. The kernel wants to establish a new, system dependent default here and this PR would raise the limit for older kernels higher then what new kernels would pick on most systems. I don‘t think this is likely to cause any problems, but maybe someone more confident can merge this.

blaggacao · 2021-06-28T22:20:28Z

Cleaning up my follow-up list. Please anyone feel free to pick this up.

github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jun 13, 2021

blaggacao force-pushed the raise-global-inotify-watches branch 3 times, most recently from f40a877 to cbe2715 Compare June 13, 2021 18:58

alyssais reviewed Jun 13, 2021

View reviewed changes

nixos/modules/config/sysctl.nix Show resolved Hide resolved

nixos/modules/config/sysctl.nix Outdated Show resolved Hide resolved

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jun 13, 2021

euank mentioned this pull request Jun 13, 2021

nixos/docker: increase inotify user instances #112472

Closed

10 tasks

nh2 approved these changes Jun 14, 2021

View reviewed changes

blaggacao force-pushed the raise-global-inotify-watches branch 2 times, most recently from c366d5e to 654173b Compare June 14, 2021 21:29

blaggacao requested review from alyssais and jonringer June 14, 2021 21:41

alyssais requested changes Jun 15, 2021

View reviewed changes

nixos/modules/config/sysctl.nix Outdated Show resolved Hide resolved

blaggacao force-pushed the raise-global-inotify-watches branch from 654173b to de892e5 Compare June 15, 2021 18:57

alyssais reviewed Jun 16, 2021

View reviewed changes

nixos/modules/config/sysctl.nix Outdated Show resolved Hide resolved

blaggacao force-pushed the raise-global-inotify-watches branch 4 times, most recently from c2dcc7b to 8c967c1 Compare June 16, 2021 16:21

blaggacao requested a review from alyssais June 16, 2021 17:33

blaggacao force-pushed the raise-global-inotify-watches branch from 8c967c1 to 0e8af3e Compare June 20, 2021 20:02

fzakaria approved these changes Jun 23, 2021

View reviewed changes

blaggacao force-pushed the raise-global-inotify-watches branch from 0e8af3e to 24f2ff6 Compare June 23, 2021 20:10

maralorn reviewed Jun 23, 2021

View reviewed changes

blaggacao force-pushed the raise-global-inotify-watches branch from 24f2ff6 to 7336ba3 Compare June 23, 2021 20:49

blaggacao closed this Jun 28, 2021

betaboon mentioned this pull request Apr 27, 2024

inotify.max_user_instances default very low for using containers #36214

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/sysctl: dynamic inotify watches or higher defaults #126777

nixos/sysctl: dynamic inotify watches or higher defaults #126777

blaggacao commented Jun 13, 2021

nh2 left a comment

blaggacao commented Jun 15, 2021

blaggacao commented Jun 18, 2021

blaggacao commented Jun 21, 2021

fzakaria commented Jun 23, 2021

fzakaria left a comment

fzakaria Jun 23, 2021

blaggacao Jun 23, 2021

maralorn commented Jun 23, 2021 •

edited

Loading

blaggacao commented Jun 23, 2021 •

edited

Loading

maralorn commented Jun 23, 2021 •

edited

Loading

blaggacao commented Jun 23, 2021 •

edited

Loading

maralorn commented Jun 23, 2021 •

edited

Loading

maralorn Jun 23, 2021

maralorn commented Jun 26, 2021

blaggacao commented Jun 28, 2021

	# mainatainers (see commit) suggest to increase this limit to the maximum
	# maintainers (see commit) suggest to increase this limit to the maximum

nixos/sysctl: dynamic inotify watches or higher defaults #126777

nixos/sysctl: dynamic inotify watches or higher defaults #126777

Conversation

blaggacao commented Jun 13, 2021

nh2 left a comment

Choose a reason for hiding this comment

blaggacao commented Jun 15, 2021

blaggacao commented Jun 18, 2021

blaggacao commented Jun 21, 2021

fzakaria commented Jun 23, 2021

fzakaria left a comment

Choose a reason for hiding this comment

fzakaria Jun 23, 2021

Choose a reason for hiding this comment

blaggacao Jun 23, 2021

Choose a reason for hiding this comment

maralorn commented Jun 23, 2021 • edited Loading

blaggacao commented Jun 23, 2021 • edited Loading

maralorn commented Jun 23, 2021 • edited Loading

blaggacao commented Jun 23, 2021 • edited Loading

maralorn commented Jun 23, 2021 • edited Loading

maralorn Jun 23, 2021

Choose a reason for hiding this comment

maralorn commented Jun 26, 2021

blaggacao commented Jun 28, 2021

maralorn commented Jun 23, 2021 •

edited

Loading

blaggacao commented Jun 23, 2021 •

edited

Loading

maralorn commented Jun 23, 2021 •

edited

Loading

blaggacao commented Jun 23, 2021 •

edited

Loading

maralorn commented Jun 23, 2021 •

edited

Loading