[PATCH v4 2/2] tools/xenstore: set open file descriptor limit for xenstored

Juergen Gross posted 2 patches 3 years, 2 months ago
There is a newer version of this series
[PATCH v4 2/2] tools/xenstore: set open file descriptor limit for xenstored
Posted by Juergen Gross 3 years, 2 months ago
Add a configuration item for the maximum number of open file
descriptors xenstored should be allowed to have.

The default should be "unlimited" in order not to restrict xenstored
in the number of domains it can support, but unfortunately the prlimit
command requires specification of a real value for the number of files,
so use 262144 as the default value. As an aid for the admin configuring
the value add a comment specifying the common needs of xenstored for
the different domain types.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- set ulimit form launch script (Julien Grall)
- split off from original patch (Julien Grall)
V4:
- switch to directly configuring the limit of file descriptors instead
  of domains (Ian Jackson)
---
 tools/hotplug/Linux/init.d/sysconfig.xencommons.in | 13 +++++++++++++
 tools/hotplug/Linux/launch-xenstore.in             |  2 ++
 2 files changed, 15 insertions(+)

diff --git a/tools/hotplug/Linux/init.d/sysconfig.xencommons.in b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
index b83101ab7e..ad020b7a50 100644
--- a/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
+++ b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
@@ -32,6 +32,19 @@
 # Changing this requires a reboot to take effect.
 #XENSTORED=@XENSTORED@
 
+## Type: integer
+## Default: 262144
+#
+# Select maximum number of file descriptors xenstored is allowed to have
+# opened at one time.
+# For each HVM domain xenstored might need up to 5 open file descriptors,
+# PVH and PV domains will require up to 3 open file descriptors. Additionally
+# 20-30 file descriptors will be opened for internal uses. The default of
+# 262144 allows for about 8 open files for the theoretical maximum of 32752
+# domains.
+# Only evaluated if XENSTORETYPE is "daemon".
+#XENSTORED_MAX_OPEN_FDS=262144
+
 ## Type: string
 ## Default: ""
 #
diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in
index 1747c96065..2bc41bb4f0 100644
--- a/tools/hotplug/Linux/launch-xenstore.in
+++ b/tools/hotplug/Linux/launch-xenstore.in
@@ -54,6 +54,7 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF
 
 [ "$XENSTORETYPE" = "daemon" ] && {
 	[ -z "$XENSTORED_TRACE" ] || XENSTORED_ARGS="$XENSTORED_ARGS -T @XEN_LOG_DIR@/xenstored-trace.log"
+	[ -z "$XENSTORED_MAX_OPEN_FDS" ] && XENSTORED_MAX_OPEN_FDS=262144
 	[ -z "$XENSTORED" ] && XENSTORED=@XENSTORED@
 	[ -x "$XENSTORED" ] || {
 		echo "No xenstored found"
@@ -70,6 +71,7 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF
 	systemd-notify --booted 2>/dev/null || timeout_xenstore $XENSTORED || exit 1
 	XS_PID=`cat @XEN_RUN_DIR@/xenstored.pid`
 	echo $XS_OOM_SCORE >/proc/$XS_PID/oom_score_adj
+	prlimit --pid $XS_PID --nofile=$XENSTORED_MAX_OPEN_FDS
 
 	exit 0
 }
-- 
2.26.2


Re: [PATCH v4 2/2] tools/xenstore: set open file descriptor limit for xenstored
Posted by Andrew Cooper 3 years, 2 months ago
On 27/09/2021 11:48, Juergen Gross wrote:
> Add a configuration item for the maximum number of open file
> descriptors xenstored should be allowed to have.
>
> The default should be "unlimited" in order not to restrict xenstored
> in the number of domains it can support, but unfortunately the prlimit
> command requires specification of a real value for the number of files,
> so use 262144 as the default value.

Citation needed.

prlimit -nunlimited

prlimit --nofile=unlimited

both work fine, and strace confirms they issue correct system calls.

Support for "unlimited" as a parameter has existed for the entire
lifetime of the utility,
https://github.com/karelzak/util-linux/commit/6bac2825af7216c5471148e219dbcf62ec5ede84

~Andrew


Re: [PATCH v4 2/2] tools/xenstore: set open file descriptor limit for xenstored
Posted by Juergen Gross 3 years, 2 months ago
On 27.09.21 16:13, Andrew Cooper wrote:
> On 27/09/2021 11:48, Juergen Gross wrote:
>> Add a configuration item for the maximum number of open file
>> descriptors xenstored should be allowed to have.
>>
>> The default should be "unlimited" in order not to restrict xenstored
>> in the number of domains it can support, but unfortunately the prlimit
>> command requires specification of a real value for the number of files,
>> so use 262144 as the default value.
> 
> Citation needed.
> 
> prlimit -nunlimited
> 
> prlimit --nofile=unlimited
> 
> both work fine, and strace confirms they issue correct system calls.

Not on my test system:

# prlimit --pid 734 --nofile=unlimited
prlimit: failed to set the NOFILE resource limit: Operation not permitted
# prlimit --pid 734 --nofile=262144
#

> Support for "unlimited" as a parameter has existed for the entire
> lifetime of the utility,
> https://github.com/karelzak/util-linux/commit/6bac2825af7216c5471148e219dbcf62ec5ede84

Yes, but not all systems seem to support raising the limit to
"unlimited".


Juergen
Re: [PATCH v4 2/2] tools/xenstore: set open file descriptor limit for xenstored
Posted by Andrew Cooper 3 years, 2 months ago
On 27/09/2021 15:24, Juergen Gross wrote:
> On 27.09.21 16:13, Andrew Cooper wrote:
>> On 27/09/2021 11:48, Juergen Gross wrote:
>>> Add a configuration item for the maximum number of open file
>>> descriptors xenstored should be allowed to have.
>>>
>>> The default should be "unlimited" in order not to restrict xenstored
>>> in the number of domains it can support, but unfortunately the prlimit
>>> command requires specification of a real value for the number of files,
>>> so use 262144 as the default value.
>>
>> Citation needed.
>>
>> prlimit -nunlimited
>>
>> prlimit --nofile=unlimited
>>
>> both work fine, and strace confirms they issue correct system calls.
>
> Not on my test system:
>
> # prlimit --pid 734 --nofile=unlimited
> prlimit: failed to set the NOFILE resource limit: Operation not permitted
> # prlimit --pid 734 --nofile=262144
> #

What does strace say in both of these cases?

>
>> Support for "unlimited" as a parameter has existed for the entire
>> lifetime of the utility,
>> https://github.com/karelzak/util-linux/commit/6bac2825af7216c5471148e219dbcf62ec5ede84
>>
>
> Yes, but not all systems seem to support raising the limit to
> "unlimited".

That's as maybe, but

prlimit64(0, RLIMIT_NOFILE, {rlim_cur=RLIM64_INFINITY,
rlim_max=RLIM64_INFINITY}, NULL) = -1 EPERM (Operation not permitted)

is a Linux issue, not a prlimit bug.

~Andrew


Re: [PATCH v4 2/2] tools/xenstore: set open file descriptor limit for xenstored
Posted by Juergen Gross 3 years, 2 months ago
On 27.09.21 16:40, Andrew Cooper wrote:
> On 27/09/2021 15:24, Juergen Gross wrote:
>> On 27.09.21 16:13, Andrew Cooper wrote:
>>> On 27/09/2021 11:48, Juergen Gross wrote:
>>>> Add a configuration item for the maximum number of open file
>>>> descriptors xenstored should be allowed to have.
>>>>
>>>> The default should be "unlimited" in order not to restrict xenstored
>>>> in the number of domains it can support, but unfortunately the prlimit
>>>> command requires specification of a real value for the number of files,
>>>> so use 262144 as the default value.
>>>
>>> Citation needed.
>>>
>>> prlimit -nunlimited
>>>
>>> prlimit --nofile=unlimited
>>>
>>> both work fine, and strace confirms they issue correct system calls.
>>
>> Not on my test system:
>>
>> # prlimit --pid 734 --nofile=unlimited
>> prlimit: failed to set the NOFILE resource limit: Operation not permitted
>> # prlimit --pid 734 --nofile=262144
>> #
> 
> What does strace say in both of these cases?

prlimit64(734, RLIMIT_NOFILE, {rlim_cur=RLIM64_INFINITY, 
rlim_max=RLIM64_INFINITY}, NULL) = -1 EPERM (Operation not permitted)
write(2, "prlimit: ", 9prlimit: )                = 9

vs.

prlimit64(734, RLIMIT_NOFILE, {rlim_cur=256*1024, rlim_max=256*1024}, 
NULL) = 0

> 
>>
>>> Support for "unlimited" as a parameter has existed for the entire
>>> lifetime of the utility,
>>> https://github.com/karelzak/util-linux/commit/6bac2825af7216c5471148e219dbcf62ec5ede84
>>>
>>
>> Yes, but not all systems seem to support raising the limit to
>> "unlimited".
> 
> That's as maybe, but
> 
> prlimit64(0, RLIMIT_NOFILE, {rlim_cur=RLIM64_INFINITY,
> rlim_max=RLIM64_INFINITY}, NULL) = -1 EPERM (Operation not permitted)
> 
> is a Linux issue, not a prlimit bug.

Nevertheless it isn't a good idea to use this setting in case it isn't
supported in all Linux distros/versions we care about.


Juergen
Re: [PATCH v4 2/2] tools/xenstore: set open file descriptor limit for xenstored
Posted by Andrew Cooper 3 years, 2 months ago
On 27/09/2021 15:52, Juergen Gross wrote:
> On 27.09.21 16:40, Andrew Cooper wrote:
>> On 27/09/2021 15:24, Juergen Gross wrote:
>>> On 27.09.21 16:13, Andrew Cooper wrote:
>>>> On 27/09/2021 11:48, Juergen Gross wrote:
>>>>> Add a configuration item for the maximum number of open file
>>>>> descriptors xenstored should be allowed to have.
>>>>>
>>>>> The default should be "unlimited" in order not to restrict xenstored
>>>>> in the number of domains it can support, but unfortunately the
>>>>> prlimit
>>>>> command requires specification of a real value for the number of
>>>>> files,
>>>>> so use 262144 as the default value.
>>>>
>>>> Citation needed.
>>>>
>>>> prlimit -nunlimited
>>>>
>>>> prlimit --nofile=unlimited
>>>>
>>>> both work fine, and strace confirms they issue correct system calls.
>>>
>>> Not on my test system:
>>>
>>> # prlimit --pid 734 --nofile=unlimited
>>> prlimit: failed to set the NOFILE resource limit: Operation not
>>> permitted
>>> # prlimit --pid 734 --nofile=262144
>>> #
>>
>> What does strace say in both of these cases?
>
> prlimit64(734, RLIMIT_NOFILE, {rlim_cur=RLIM64_INFINITY,
> rlim_max=RLIM64_INFINITY}, NULL) = -1 EPERM (Operation not permitted)
> write(2, "prlimit: ", 9prlimit: )                = 9
>
> vs.
>
> prlimit64(734, RLIMIT_NOFILE, {rlim_cur=256*1024, rlim_max=256*1024},
> NULL) = 0
>
>>
>>>
>>>> Support for "unlimited" as a parameter has existed for the entire
>>>> lifetime of the utility,
>>>> https://github.com/karelzak/util-linux/commit/6bac2825af7216c5471148e219dbcf62ec5ede84
>>>>
>>>>
>>>
>>> Yes, but not all systems seem to support raising the limit to
>>> "unlimited".
>>
>> That's as maybe, but
>>
>> prlimit64(0, RLIMIT_NOFILE, {rlim_cur=RLIM64_INFINITY,
>> rlim_max=RLIM64_INFINITY}, NULL) = -1 EPERM (Operation not permitted)
>>
>> is a Linux issue, not a prlimit bug.
>
> Nevertheless it isn't a good idea to use this setting in case it isn't
> supported in all Linux distros/versions we care about.

Ok, but at a minimum you need s/prlimit/Linux/ in the commit message.

~Andrew

Re: [PATCH v4 2/2] tools/xenstore: set open file descriptor limit for xenstored
Posted by Juergen Gross 3 years, 2 months ago
On 27.09.21 16:54, Andrew Cooper wrote:
> On 27/09/2021 15:52, Juergen Gross wrote:
>> On 27.09.21 16:40, Andrew Cooper wrote:
>>> On 27/09/2021 15:24, Juergen Gross wrote:
>>>> On 27.09.21 16:13, Andrew Cooper wrote:
>>>>> On 27/09/2021 11:48, Juergen Gross wrote:
>>>>>> Add a configuration item for the maximum number of open file
>>>>>> descriptors xenstored should be allowed to have.
>>>>>>
>>>>>> The default should be "unlimited" in order not to restrict xenstored
>>>>>> in the number of domains it can support, but unfortunately the
>>>>>> prlimit
>>>>>> command requires specification of a real value for the number of
>>>>>> files,
>>>>>> so use 262144 as the default value.
>>>>>
>>>>> Citation needed.
>>>>>
>>>>> prlimit -nunlimited
>>>>>
>>>>> prlimit --nofile=unlimited
>>>>>
>>>>> both work fine, and strace confirms they issue correct system calls.
>>>>
>>>> Not on my test system:
>>>>
>>>> # prlimit --pid 734 --nofile=unlimited
>>>> prlimit: failed to set the NOFILE resource limit: Operation not
>>>> permitted
>>>> # prlimit --pid 734 --nofile=262144
>>>> #
>>>
>>> What does strace say in both of these cases?
>>
>> prlimit64(734, RLIMIT_NOFILE, {rlim_cur=RLIM64_INFINITY,
>> rlim_max=RLIM64_INFINITY}, NULL) = -1 EPERM (Operation not permitted)
>> write(2, "prlimit: ", 9prlimit: )                = 9
>>
>> vs.
>>
>> prlimit64(734, RLIMIT_NOFILE, {rlim_cur=256*1024, rlim_max=256*1024},
>> NULL) = 0
>>
>>>
>>>>
>>>>> Support for "unlimited" as a parameter has existed for the entire
>>>>> lifetime of the utility,
>>>>> https://github.com/karelzak/util-linux/commit/6bac2825af7216c5471148e219dbcf62ec5ede84
>>>>>
>>>>>
>>>>
>>>> Yes, but not all systems seem to support raising the limit to
>>>> "unlimited".
>>>
>>> That's as maybe, but
>>>
>>> prlimit64(0, RLIMIT_NOFILE, {rlim_cur=RLIM64_INFINITY,
>>> rlim_max=RLIM64_INFINITY}, NULL) = -1 EPERM (Operation not permitted)
>>>
>>> is a Linux issue, not a prlimit bug.
>>
>> Nevertheless it isn't a good idea to use this setting in case it isn't
>> supported in all Linux distros/versions we care about.
> 
> Ok, but at a minimum you need s/prlimit/Linux/ in the commit message.

Okay.

Should I send out another version, or can this be done when committing?


Juergen
Re: [PATCH v4 2/2] tools/xenstore: set open file descriptor limit for xenstored
Posted by Ian Jackson 3 years, 2 months ago
Juergen Gross writes ("Re: [PATCH v4 2/2] tools/xenstore: set open file descriptor limit for xenstored"):
> Should I send out another version, or can this be done when committing?

In any case I think you should send another version.

If you don't agree with my suggestion to check /proc/sys/fs/nr_open,
then you could usefully explain in the commit message why we're not
doing that.  If you do agree please include the links from my mail.

Ian.

Re: [PATCH v4 2/2] tools/xenstore: set open file descriptor limit for xenstored
Posted by Ian Jackson 3 years, 2 months ago
Juergen Gross writes ("Re: [PATCH v4 2/2] tools/xenstore: set open file descriptor limit for xenstored"):
> On 27.09.21 16:13, Andrew Cooper wrote:
> > both work fine, and strace confirms they issue correct system calls.
> 
> Not on my test system:
> 
> # prlimit --pid 734 --nofile=unlimited
> prlimit: failed to set the NOFILE resource limit: Operation not permitted
> # prlimit --pid 734 --nofile=262144
> #
> 
> > Support for "unlimited" as a parameter has existed for the entire
> > lifetime of the utility,
> > https://github.com/karelzak/util-linux/commit/6bac2825af7216c5471148e219dbcf62ec5ede84
> 
> Yes, but not all systems seem to support raising the limit to
> "unlimited".

This is strange.  Are you running in some kind of restricted
environment ?  <strike>Or maybe prlimit is trying to adjust the soft
limit (only) but failing to remove the hard limit too ?  I confess
that I never use prlimit, just ulimit.</strike>

I just tried this on my laptop:

root(ian)@zealot:~> ulimit -Hn 1048577
bash: ulimit: open files: cannot modify limit: Operation not permitted
root(ian)@zealot:~> ulimit -Hn 1048576
root(ian)@zealot:~>

????

1048576 is 0x100000.
1048577 is 0x100001.

The intertubes caused me to check sysctl fs.file-max (1591013),
/etc/security/limits.conf (nothing uncommented).  Eventually a helpful
person pointed me to /proc/sys/fs/nr_open.

root(ian)@zealot:~> cat /proc/sys/fs/nr_open
1048576

This is completely deranged.  The internet is full of people working
around this (if you are unluckloy you try to set nofile unlimited in
some file in /etc/security and then you can't log in!).

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=60fd760fb9ff7034360bab7137c917c0330628c2
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0c2d64fb6cae9aae480f6a46cfe79f8d7d48b59f

^ that explains why things are like this.  Oh woe is us what madness
have we collectively perpetrated.

I suggest the following workaround for our scripts: try to read
/proc/sys/fs/nr_open and use the value from there if there is one;
otherwise use "unlimited".

I think that is better than sort-of-guessing 262144.  What you do you
think ?

Thanks,
Ian.