Re: RLIMIT_NOFILE fallback

["Torsten Bögershausen" <tboegi@web.de>]

On 2013-12-19 01.15, Jeff King wrote:
> On Wed, Dec 18, 2013 at 02:59:12PM -0800, Junio C Hamano wrote:
> 
>> Jeff King <peff@peff.net> writes:
>>
>>>> Yes, that is locally OK, but depending on how the caller behaves, we
>>>> might need to have an extra saved_errno dance here, which I didn't
>>>> want to get into...
>>>
>>> I think we are fine. The only caller is about to clobber errno by
>>> closing packs anyway.
> 
> Also, I do not think we would be any worse off than the current code.
> getrlimit almost certainly just clobbered errno anyway. Either it is
> worth saving for the whole function, or not at all (and I think not at
> all).
> 
>> diff --git a/sha1_file.c b/sha1_file.c
>> index 760dd60..288badd 100644
>> --- a/sha1_file.c
>> +++ b/sha1_file.c
>> @@ -807,15 +807,38 @@ void free_pack_by_name(const char *pack_name)
>>  static unsigned int get_max_fd_limit(void)
>>  {
>>  #ifdef RLIMIT_NOFILE
>> -	struct rlimit lim;
>> +	{
>> +		struct rlimit lim;
>>  
>> -	if (getrlimit(RLIMIT_NOFILE, &lim))
>> -		die_errno("cannot get RLIMIT_NOFILE");
>> +		if (!getrlimit(RLIMIT_NOFILE, &lim))
>> +			return lim.rlim_cur;
>> +	}
>> +#endif
> 
> Yeah, I think pulling the variable into its own block makes this more
> readable.
> 
>> +#ifdef _SC_OPEN_MAX
>> +	{
>> +		long open_max = sysconf(_SC_OPEN_MAX);
>> +		if (0 < open_max)
>> +			return open_max;
>> +		/*
>> +		 * Otherwise, we got -1 for one of the two
>> +		 * reasons:
>> +		 *
>> +		 * (1) sysconf() did not understand _SC_OPEN_MAX
>> +		 *     and signaled an error with -1; or
>> +		 * (2) sysconf() said there is no limit.
>> +		 *
>> +		 * We _could_ clear errno before calling sysconf() to
>> +		 * tell these two cases apart and return a huge number
>> +		 * in the latter case to let the caller cap it to a
>> +		 * value that is not so selfish, but letting the
>> +		 * fallback OPEN_MAX codepath take care of these cases
>> +		 * is a lot simpler.
>> +		 */
>> +	}
>> +#endif
> 
> This is probably OK. I assume sane systems actually provide OPEN_MAX,
> and/or have a working getrlimit in the first place.
> 
> The fallback of "1" is actually quite low and can have an impact. Both
> for performance, but also for concurrent use. We used to run into a
> problem at GitHub where pack-objects serving a clone would have its
> packfile removed from under it (by a concurrent repack), and then would
> die. The normal code paths are able to just retry the object lookup and
> find the new pack, but the pack-objects code is a bit more intimate with
> the particular packfile and cannot (currently) do so. With a large
> enough mmap window and descriptor limit, we just keep the packfiles
> open. But if we have to close them for resource limits (like a too-low
> descriptor limit), then we can end up in the die() situation above.
> 
> -Peff

Thanks for an interesting reading,
please allow a side question:
Could it be, that "-1 == unlimited" is Linux specific?
And therefore not 100% portable ?

And doesn't "unlimited" number of files call for trouble,
having the risk to starve the machine ?

BTW: cygwin returns 256.

------------
http://pubs.opengroup.org/onlinepubs/007908799/xsh/sysconf.html
RETURN VALUE

    If name is an invalid value, sysconf() returns -1 and sets errno to indicate the error. If the variable corresponding to name is associated with functionality that is not supported by the system, sysconf() returns -1 without changing the value of errno. 

---------- Mac OS, based on BSD (?): ---------- 
RETURN VALUES
     If the call to sysconf() is not successful, -1 is returned and errno is
     set appropriately.  Otherwise, if the variable is associated with func-
     tionality that is not supported, -1 is returned and errno is not modi-
     fied.  Otherwise, the current variable value is returned.

ERRORS
     The sysconf() function may fail and set errno for any of the errors spec-
     ified for the library function sysctl(3).  In addition, the following
     error may be reported:

     [EINVAL]           The value of the name argument is invalid.
[snip]
     The sysconf() function first appeared in 4.4BSD.

-----------
Linux, Debian:
      OPEN_MAX - _SC_OPEN_MAX
              The  maximum number of files that a process can have open at any
              time.  Must not be less than _POSIX_OPEN_MAX (20).
[snip]
RETURN VALUE
       If name is invalid, -1 is returned, and errno is set to EINVAL.  Other‐
       wise, the value returned is the value of the system resource and  errno
       is  not  changed.  In the case of options, a positive value is returned
       if a queried option is available, and -1 if it is not.  In the case  of
       limits, -1 means that there is no definite limit.