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@xxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html