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