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

From: Ian Jackson <iwj@xenproject.org>
To: Juergen Gross <jgross@suse.com>
Cc: xen-devel@lists.xenproject.org, Wei Liu <wl@xen.org>
Subject: [PATCH v5 2/2] tools/xenstore: set open file descriptor limit for xenstored
Date: Tue, 28 Sep 2021 13:02:41 +0100	[thread overview]
Message-ID: <24915.1121.8356.288414@mariner.uk.xensource.com> (raw)
In-Reply-To: <20210928091517.9761-3-jgross@suse.com>

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.