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 kernel
is normally limiting the maximum value via /proc/sys/fs/nr_open [1],
[2]. So check that file to exist and if it does, limit the maximum
value to the one specified by /proc/sys/fs/nr_open.

As an aid for the admin configuring the value add a comment specifying
the common needs of xenstored for the different domain types.

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

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)
V5:
- use /proc/sys/fs/nr_open (Ian Jackson)
---
 .../Linux/init.d/sysconfig.xencommons.in      | 13 ++++++++++++
 tools/hotplug/Linux/launch-xenstore.in        | 20 +++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/tools/hotplug/Linux/init.d/sysconfig.xencommons.in b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
index b83101ab7e..433e4849af 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: string
+## Default: unlimited
+#
+# 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 specified value (including "unlimited") will be capped by the contents
+# of /proc/sys/fs/nr_open if existing.
+# Only evaluated if XENSTORETYPE is "daemon".
+#XENSTORED_MAX_OPEN_FDS=unlimited
+
 ## Type: string
 ## Default: ""
 #
diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in
index 1747c96065..7a0334d880 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=unlimited
 	[ -z "$XENSTORED" ] && XENSTORED=@XENSTORED@
 	[ -x "$XENSTORED" ] || {
 		echo "No xenstored found"
@@ -62,6 +63,24 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF
 	[ -z "$XENSTORED_OOM_MEM_THRESHOLD" ] || XENSTORED_OOM_MEM_THRESHOLD=50
 	XS_OOM_SCORE=-$(($XENSTORED_OOM_MEM_THRESHOLD * 10))
 
+	[ "$XENSTORED_MAX_OPEN_FDS" = "unlimited" ] || {
+		[ -z "${XENSTORED_MAX_OPEN_FDS//[0-9]}" ] &&
+		[ -n "$XENSTORED_MAX_OPEN_FDS" ] || {
+			echo "XENSTORED_MAX_OPEN_FDS=$XENSTORED_MAX_OPEN_FDS invalid"
+			echo "Setting to default \"unlimited\"."
+			XENSTORED_MAX_OPEN_FDS=unlimited
+		}
+	}
+	[ -r /proc/sys/fs/nr_open ] && {
+		MAX_FDS=`cat /proc/sys/fs/nr_open`
+		[ "$XENSTORED_MAX_OPEN_FDS" = "unlimited" ] && XENSTORED_MAX_OPEN_FDS=$MAX_FDS
+		[ $XENSTORED_MAX_OPEN_FDS -gt $MAX_FDS ] && {
+			echo "XENSTORED_MAX_OPEN_FDS exceeds system limit."
+			echo "Setting to \"$MAX_FDS\"."
+			XENSTORED_MAX_OPEN_FDS=$MAX_FDS
+		}
+	}
+
 	rm -f @XEN_RUN_DIR@/xenstored.pid
 
 	echo -n Starting $XENSTORED...
@@ -70,6 +89,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

Juergen Gross writes ("[PATCH v5 2/2] tools/xenstore: set open file descriptor limit for xenstored"):
> 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 kernel
> is normally limiting the maximum value via /proc/sys/fs/nr_open [1],
> [2]. So check that file to exist and if it does, limit the maximum
> value to the one specified by /proc/sys/fs/nr_open.
> 
> As an aid for the admin configuring the value add a comment specifying
> the common needs of xenstored for the different domain types.
...
>  	echo -n Starting $XENSTORED...
> @@ -70,6 +89,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

Thanks for this.  I have one comment/question, which I regret making
rather late:

I am uncomfortable with the use of prlimit here, because identifying
processes by pid is typically inherently not 100% reliable.

AIUI you are using it here because perhaps otherwise you would have to
mess about with both systemd and non-systemd approaches.  But in fact
this script "launch-xenstore" is simply a parent of xenstore.  It is
run either by systemd or from the init script, and it runs $XENSTORED
directly (so not via systemd or another process supervisor).

fd limits are inherited, so I think you can use ulimit rather than
prlimit ?

If you use ulimit I think you must set the hard and soft limits,
which requires two calls.

If you can't use ulimit then we should try to make some argument that
the prlimit can't target the wrong process eg due to a
misconfiguration or stale pid file or soemthing.  I think I see a way
that such an argument could be construted but it would be better just
to use ulimit.

Ian.

On 28.09.21 14:02, Ian Jackson wrote:
> Juergen Gross writes ("[PATCH v5 2/2] tools/xenstore: set open file descriptor limit for xenstored"):
>> 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 kernel
>> is normally limiting the maximum value via /proc/sys/fs/nr_open [1],
>> [2]. So check that file to exist and if it does, limit the maximum
>> value to the one specified by /proc/sys/fs/nr_open.
>>
>> As an aid for the admin configuring the value add a comment specifying
>> the common needs of xenstored for the different domain types.
> ...
>>   	echo -n Starting $XENSTORED...
>> @@ -70,6 +89,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
> 
> Thanks for this.  I have one comment/question, which I regret making
> rather late:
> 
> I am uncomfortable with the use of prlimit here, because identifying
> processes by pid is typically inherently not 100% reliable.
> 
> AIUI you are using it here because perhaps otherwise you would have to
> mess about with both systemd and non-systemd approaches.  But in fact
> this script "launch-xenstore" is simply a parent of xenstore.  It is
> run either by systemd or from the init script, and it runs $XENSTORED
> directly (so not via systemd or another process supervisor).
> 
> fd limits are inherited, so I think you can use ulimit rather than
> prlimit ?
> 
> If you use ulimit I think you must set the hard and soft limits,
> which requires two calls.
> 
> If you can't use ulimit then we should try to make some argument that
> the prlimit can't target the wrong process eg due to a
> misconfiguration or stale pid file or soemthing.  I think I see a way
> that such an argument could be construted but it would be better just
> to use ulimit.

Hmm, maybe I should just use:

prlimit --nofile=$XENSTORED_MAX_OPEN_FDS \
    $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS


Juergen

Juergen Gross writes ("Re: [PATCH v5 2/2] tools/xenstore: set open file descriptor limit for xenstored"):
> Hmm, maybe I should just use:
> 
> prlimit --nofile=$XENSTORED_MAX_OPEN_FDS \
>     $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS

I guess that would probably work (although it involves another
exec) but I don't understand what's wrong with ulimit, which is a
shell builtin.

I think this script has to run only on Linux and all reasonable Linux
/bin/sh have `ulimit`.  (I have checked dash and bash.)

So I think just

  ulimit -n $XENSTORED_MAX_OPEN_FDS

  $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS

will DTRT.  You could also do this

  ulimit -n $XENSTORED_MAX_OPEN_FDS || true

which will arrange that if, somehow, this fails, the system is likely
to continue to mostly-work despite the error.  Whether that would be
desirable is a matter of taste I think.

(I have RTFM again, and setting -H and -S separately is not needed;
omitting -H or -S means to set both.)

Ian.

On 28.09.21 17:26, Ian Jackson wrote:
> Juergen Gross writes ("Re: [PATCH v5 2/2] tools/xenstore: set open file descriptor limit for xenstored"):
>> Hmm, maybe I should just use:
>>
>> prlimit --nofile=$XENSTORED_MAX_OPEN_FDS \
>>      $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS
> 
> I guess that would probably work (although it involves another
> exec) but I don't understand what's wrong with ulimit, which is a
> shell builtin.

My main concern with ulimit is that this would set the open file limit
for _all_ children of the script. I don't think right now this is a real
problem, but it feels wrong to me (systemd-notify ought to be fine, but
you never know ...).


Juergen

Juergen Gross writes ("Re: [PATCH v5 2/2] tools/xenstore: set open file descriptor limit for xenstored"):
> On 28.09.21 17:26, Ian Jackson wrote:
> > Juergen Gross writes ("Re: [PATCH v5 2/2] tools/xenstore: set open file descriptor limit for xenstored"):
> >> Hmm, maybe I should just use:
> >>
> >> prlimit --nofile=$XENSTORED_MAX_OPEN_FDS \
> >>      $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS
> > 
> > I guess that would probably work (although it involves another
> > exec) but I don't understand what's wrong with ulimit, which is a
> > shell builtin.
> 
> My main concern with ulimit is that this would set the open file limit
> for _all_ children of the script. I don't think right now this is a real
> problem, but it feels wrong to me (systemd-notify ought to be fine, but
> you never know ...).

Oh, I see.  Yes, that is a good point.

So, I think your suggest (quoted above) is good.

Thanks,
Ian.