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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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