RLIMIT_NOFILE fallback

RLIMIT_NOFILE fallback

[Joey Hess]

[-- Attachment #1: Type: text/plain, Size: 621 bytes --]

In sha1_file.c, when git is built on linux, it will use 
getrlimit(RLIMIT_NOFILE). I've been deploying git binaries to some
unusual systems, like embedded NAS devices, and it seems some with older
kernels like 2.6.33 fail with "fatal: cannot get RLIMIT_NOFILE: Bad address".

I could work around this by building git without RLIMIT_NOFILE defined,
but perhaps it would make sense to improve the code to fall back
to one of the other methods for getting the limit, and/or return the
hardcoded 1 as a fallback. This would make git binaries more robust
against old/broken/misconfigured kernels.

-- 
see shy jo

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: RLIMIT_NOFILE fallback

[Junio C Hamano]

Joey Hess <joey@kitenet.net> writes:

> In sha1_file.c, when git is built on linux, it will use 
> getrlimit(RLIMIT_NOFILE). I've been deploying git binaries to some
> unusual systems, like embedded NAS devices, and it seems some with older
> kernels like 2.6.33 fail with "fatal: cannot get RLIMIT_NOFILE: Bad address".
>
> I could work around this by building git without RLIMIT_NOFILE defined,
> but perhaps it would make sense to improve the code to fall back
> to one of the other methods for getting the limit, and/or return the
> hardcoded 1 as a fallback. This would make git binaries more robust
> against old/broken/misconfigured kernels.

Hmph, perhaps you are right.  Like this?

 sha1_file.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index daacc0c..a3a0014 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -809,8 +809,12 @@ static unsigned int get_max_fd_limit(void)
 #ifdef RLIMIT_NOFILE
 	struct rlimit lim;
 
-	if (getrlimit(RLIMIT_NOFILE, &lim))
-		die_errno("cannot get RLIMIT_NOFILE");
+	if (getrlimit(RLIMIT_NOFILE, &lim)) {
+		static int warn_only_once;
+		if (!warn_only_once++)
+			warning("cannot get RLIMIT_NOFILE: %s", strerror(errno));
+		return 1; /* see the caller ;-) */
+	}
 
 	return lim.rlim_cur;
 #elif defined(_SC_OPEN_MAX)

Re: RLIMIT_NOFILE fallback

[Joey Hess]

[-- Attachment #1: Type: text/plain, Size: 103 bytes --]

Junio C Hamano wrote:
> Hmph, perhaps you are right.  Like this?

Works for me.

-- 
see shy jo

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: RLIMIT_NOFILE fallback

[Jeff King]

On Wed, Dec 18, 2013 at 10:00:35AM -0800, Junio C Hamano wrote:

> Joey Hess <joey@kitenet.net> writes:
> 
> > In sha1_file.c, when git is built on linux, it will use 
> > getrlimit(RLIMIT_NOFILE). I've been deploying git binaries to some
> > unusual systems, like embedded NAS devices, and it seems some with older
> > kernels like 2.6.33 fail with "fatal: cannot get RLIMIT_NOFILE: Bad address".
> >
> > I could work around this by building git without RLIMIT_NOFILE defined,
> > but perhaps it would make sense to improve the code to fall back
> > to one of the other methods for getting the limit, and/or return the
> > hardcoded 1 as a fallback. This would make git binaries more robust
> > against old/broken/misconfigured kernels.
> 
> Hmph, perhaps you are right.  Like this?
> 
>  sha1_file.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/sha1_file.c b/sha1_file.c
> index daacc0c..a3a0014 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -809,8 +809,12 @@ static unsigned int get_max_fd_limit(void)
>  #ifdef RLIMIT_NOFILE
>  	struct rlimit lim;
>  
> -	if (getrlimit(RLIMIT_NOFILE, &lim))
> -		die_errno("cannot get RLIMIT_NOFILE");
> +	if (getrlimit(RLIMIT_NOFILE, &lim)) {
> +		static int warn_only_once;
> +		if (!warn_only_once++)
> +			warning("cannot get RLIMIT_NOFILE: %s", strerror(errno));
> +		return 1; /* see the caller ;-) */
> +	}

I wish we understood why getrlimit was failing. Returning EFAULT seems
like an odd choice if it is not implemented for the system. On such a
system, do the other fallbacks actually work? Would it work to do:

diff --git a/sha1_file.c b/sha1_file.c
index daacc0c..ab38795 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -809,11 +809,11 @@ static unsigned int get_max_fd_limit(void)
 #ifdef RLIMIT_NOFILE
 	struct rlimit lim;
 
-	if (getrlimit(RLIMIT_NOFILE, &lim))
-		die_errno("cannot get RLIMIT_NOFILE");
+	if (!getrlimit(RLIMIT_NOFILE, &lim))
+		return lim.rlim_cur;
+#endif
 
-	return lim.rlim_cur;
-#elif defined(_SC_OPEN_MAX)
+#if defined(_SC_OPEN_MAX)
 	return sysconf(_SC_OPEN_MAX);
 #elif defined(OPEN_MAX)
 	return OPEN_MAX;

That is, does sysconf actually work on such a system (or does it need a
similar run-time fallback)? And either way, we should try falling back
to OPEN_MAX rather than 1 if we have it.

As far as the warning, I am not sure I see a point. The user does not
have any useful recourse, and git should continue to operate as normal.
Having every single git invocation print "by the way, RLIMIT_NOFILE does
not work on your system" seems like it would get annoying.

-Peff

Re: RLIMIT_NOFILE fallback

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> That is, does sysconf actually work on such a system (or does it need a
> similar run-time fallback)? And either way, we should try falling back
> to OPEN_MAX rather than 1 if we have it.

Interesting.

> As far as the warning, I am not sure I see a point. The user does not
> have any useful recourse, and git should continue to operate as normal.
> Having every single git invocation print "by the way, RLIMIT_NOFILE does
> not work on your system" seems like it would get annoying.

Very true.  That makes the resulting function look like this:

-------------------------------- 8< ------------------------------

static unsigned int get_max_fd_limit(void)
{
#ifdef RLIMIT_NOFILE
	struct rlimit lim;

	if (!getrlimit(RLIMIT_NOFILE, &lim))
		return lim.rlim_cur;
#endif

#if defined(_SC_OPEN_MAX)
	{
		long sc_open_max = sysconf(_SC_OPEN_MAX);
		if (0 < sc_open_max)
			return sc_open_max;
	}

#if defined(OPEN_MAX)
	return OPEN_MAX;
#else
	return 1; /* see the caller ;-) */
#endif
}

-------------------------------- >8 ------------------------------

But the sysconf part makes me wonder; here is what we see in
http://pubs.opengroup.org/onlinepubs/9699919799/functions/sysconf.html

    If name is an invalid value, sysconf() shall return -1 and set errno
    to indicate the error. If the variable corresponding to name is
    described in <limits.h> as a maximum or minimum value and the
    variable has no limit, sysconf() shall return -1 without changing
    the value of errno. Note that indefinite limits do not imply
    infinite limits; see <limits.h>.

For a broken system (like RLIMIT_NOFILE defined for the compiler,
but the actual call returns a bogus error), the compiler may see the
_SC_OPEN_MAX defined, while sysconf() may say "I've never heard of
such a name" and return -1, or the system, whether broken or not,
may want to say "Unlimited" and return -1.  The caller takes
anything unreasonable as a positive value capped to 25 or something,
so there isn't a real harm if we returned a bogus value from here,
but I am not sure what the safe default behaviour of this function
should be to help such a broken system while not harming systems
that are functioning correctly.

Re: RLIMIT_NOFILE fallback

[Joey Hess]

[-- Attachment #1: Type: text/plain, Size: 1014 bytes --]

Jeff King wrote:
> I wish we understood why getrlimit was failing. Returning EFAULT seems
> like an odd choice if it is not implemented for the system. On such a
> system, do the other fallbacks actually work? Would it work to do:
> 
> That is, does sysconf actually work on such a system (or does it need a
> similar run-time fallback)? And either way, we should try falling back
> to OPEN_MAX rather than 1 if we have it.

For what it's worth, the system this happened on was a QNAP TS-219PII
Linux willow 2.6.33.2 #1 Fri Mar 1 04:41:48 CST 2013 armv5tel unknown

I don't have access to it to run tests of sysconf. (I already suggested its
owner upgrade its firmware.)

> As far as the warning, I am not sure I see a point. The user does not
> have any useful recourse, and git should continue to operate as normal.
> Having every single git invocation print "by the way, RLIMIT_NOFILE does
> not work on your system" seems like it would get annoying.

I agree with that.

-- 
see shy jo

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: RLIMIT_NOFILE fallback

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> That is, does sysconf actually work on such a system (or does it need a
>> similar run-time fallback)? And either way, we should try falling back
>> to OPEN_MAX rather than 1 if we have it.
>
> Interesting.
>
>> As far as the warning, I am not sure I see a point. The user does not
>> have any useful recourse, and git should continue to operate as normal.
>> Having every single git invocation print "by the way, RLIMIT_NOFILE does
>> not work on your system" seems like it would get annoying.
>
> Very true.  That makes the resulting function look like this:
>
>
> -------------------------------- 8< ------------------------------
>
> static unsigned int get_max_fd_limit(void)
> {
> #ifdef RLIMIT_NOFILE
> 	struct rlimit lim;
>
> 	if (!getrlimit(RLIMIT_NOFILE, &lim))
> 		return lim.rlim_cur;
> #endif
>
> #if defined(_SC_OPEN_MAX)
> 	{
> 		long sc_open_max = sysconf(_SC_OPEN_MAX);
> 		if (0 < sc_open_max)
> 			return sc_open_max;
> 	}

err, here we need

#endif /* defined(_SC_OPEN_MAX) */

to truly implement the structure "try all the available functions,
and then fall back to OPEN_MAX".



>
> #if defined(OPEN_MAX)
> 	return OPEN_MAX;
> #else
> 	return 1; /* see the caller ;-) */
> #endif
> }
>
> -------------------------------- >8 ------------------------------
>
>
> But the sysconf part makes me wonder; here is what we see in
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/sysconf.html
>
>     If name is an invalid value, sysconf() shall return -1 and set errno
>     to indicate the error. If the variable corresponding to name is
>     described in <limits.h> as a maximum or minimum value and the
>     variable has no limit, sysconf() shall return -1 without changing
>     the value of errno. Note that indefinite limits do not imply
>     infinite limits; see <limits.h>.
>
> For a broken system (like RLIMIT_NOFILE defined for the compiler,
> but the actual call returns a bogus error), the compiler may see the
> _SC_OPEN_MAX defined, while sysconf() may say "I've never heard of
> such a name" and return -1, or the system, whether broken or not,
> may want to say "Unlimited" and return -1.  The caller takes
> anything unreasonable as a positive value capped to 25 or something,
> so there isn't a real harm if we returned a bogus value from here,
> but I am not sure what the safe default behaviour of this function
> should be to help such a broken system while not harming systems
> that are functioning correctly.

Re: RLIMIT_NOFILE fallback

[Jeff King]

On Wed, Dec 18, 2013 at 11:50:24AM -0800, Junio C Hamano wrote:

> -------------------------------- 8< ------------------------------
> 
> static unsigned int get_max_fd_limit(void)
> {
> #ifdef RLIMIT_NOFILE
> 	struct rlimit lim;
> 
> 	if (!getrlimit(RLIMIT_NOFILE, &lim))
> 		return lim.rlim_cur;
> #endif
> 
> #if defined(_SC_OPEN_MAX)
> 	{
> 		long sc_open_max = sysconf(_SC_OPEN_MAX);
> 		if (0 < sc_open_max)
> 			return sc_open_max;
> 	}
> 
> #if defined(OPEN_MAX)
> 	return OPEN_MAX;
> #else
> 	return 1; /* see the caller ;-) */
> #endif
> }
> 
> -------------------------------- >8 ------------------------------

Yeah, with the #endif followup you posted, this is what I had in mind.

> But the sysconf part makes me wonder; here is what we see in
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/sysconf.html
> 
>     If name is an invalid value, sysconf() shall return -1 and set errno
>     to indicate the error. If the variable corresponding to name is
>     described in <limits.h> as a maximum or minimum value and the
>     variable has no limit, sysconf() shall return -1 without changing
>     the value of errno. Note that indefinite limits do not imply
>     infinite limits; see <limits.h>.
> 
> For a broken system (like RLIMIT_NOFILE defined for the compiler,
> but the actual call returns a bogus error), the compiler may see the
> _SC_OPEN_MAX defined, while sysconf() may say "I've never heard of
> such a name" and return -1, or the system, whether broken or not,
> may want to say "Unlimited" and return -1.  The caller takes
> anything unreasonable as a positive value capped to 25 or something,
> so there isn't a real harm if we returned a bogus value from here,
> but I am not sure what the safe default behaviour of this function
> should be to help such a broken system while not harming systems
> that are functioning correctly.

According to the POSIX quote above, it sounds like we could do:

  #if defined (_SC_OPEN_MAX)
  {
          long max;
          errno = 0;
          max = sysconf(_SC_OPEN_MAX);
          if (0 < max) /* got the limit */
                  return max;
          else if (!errno) /* unlimited, cast to int-max */
                  return max;
          /* otherwise, fall through */
  }
  #endif

Obviously you could collapse the two branches of the conditional, though
I think it deserves at least a comment to explain what is going on.

-Peff

Re: RLIMIT_NOFILE fallback

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> According to the POSIX quote above, it sounds like we could do:
>
>   #if defined (_SC_OPEN_MAX)
>   {
>           long max;
>           errno = 0;
>           max = sysconf(_SC_OPEN_MAX);
>           if (0 < max) /* got the limit */
>                   return max;
>           else if (!errno) /* unlimited, cast to int-max */
>                   return max;
>           /* otherwise, fall through */
>   }
>   #endif
>
> Obviously you could collapse the two branches of the conditional, though
> I think it deserves at least a comment to explain what is going on.

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...

Re: RLIMIT_NOFILE fallback

[Jeff King]

On Wed, Dec 18, 2013 at 01:37:24PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > According to the POSIX quote above, it sounds like we could do:
> >
> >   #if defined (_SC_OPEN_MAX)
> >   {
> >           long max;
> >           errno = 0;
> >           max = sysconf(_SC_OPEN_MAX);
> >           if (0 < max) /* got the limit */
> >                   return max;
> >           else if (!errno) /* unlimited, cast to int-max */
> >                   return max;
> >           /* otherwise, fall through */
> >   }
> >   #endif
> >
> > Obviously you could collapse the two branches of the conditional, though
> > I think it deserves at least a comment to explain what is going on.
> 
> 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.

-Peff

Re: RLIMIT_NOFILE fallback

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Dec 18, 2013 at 01:37:24PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > According to the POSIX quote above, it sounds like we could do:
>> >
>> >   #if defined (_SC_OPEN_MAX)
>> >   {
>> >           long max;
>> >           errno = 0;
>> >           max = sysconf(_SC_OPEN_MAX);
>> >           if (0 < max) /* got the limit */
>> >                   return max;
>> >           else if (!errno) /* unlimited, cast to int-max */
>> >                   return max;
>> >           /* otherwise, fall through */
>> >   }
>> >   #endif
>> >
>> > Obviously you could collapse the two branches of the conditional, though
>> > I think it deserves at least a comment to explain what is going on.
>> 
>> 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.
>
> -Peff

OK.

-- >8 --
Subject: [PATCH] get_max_fd_limit(): fall back to OPEN_MAX upon getrlimit/sysconf failure

On broken systems where RLIMIT_NOFILE is visible by the compliers
but underlying getrlimit() system call does not behave, we used to
simply die() when we are trying to decide how many file descriptors
to allocate for keeping packfiles open.  Instead, allow the fallback
codepath to take over when we get such a failure from getrlimit().

The same issue exists with _SC_OPEN_MAX and sysconf(); restructure
the code in a similar way to prepare for a broken sysconf() as well.

Noticed-by: Joey Hess <joey@kitenet.net>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sha1_file.c | 37 ++++++++++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 7 deletions(-)

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
+
+#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
 
-	return lim.rlim_cur;
-#elif defined(_SC_OPEN_MAX)
-	return sysconf(_SC_OPEN_MAX);
-#elif defined(OPEN_MAX)
+#ifdef OPEN_MAX
 	return OPEN_MAX;
 #else
 	return 1; /* see the caller ;-) */
-- 
1.8.5.2-297-g3e57c29

Re: RLIMIT_NOFILE fallback

[Jeff King]

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

Re: RLIMIT_NOFILE fallback

[Torsten Bögershausen]

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.

Re: RLIMIT_NOFILE fallback

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> 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.

If you look at the caller, you will see that we do cap the value
returned from this helper function down to a more reasonable and not
so selfish maximum, exactly for the purpose of avoiding the risk of
starving other processes.

>
> ------------
> 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. 

That is a rather dated document.  POSIX.1-2013 (look for the URL to
the corresponding page in an earlier message from me) has a bit
tighter wording than that to clarify the "there is no limit" case.

In addition, the final version Peff and I worked out does not even
look at the value of errno, in order not to rely on possibly
ambiguous interpretations of negative return values.  So I think we
are good.

Thanks.

> ---------- 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.

Re: RLIMIT_NOFILE fallback

[Jeff King]

On Thu, Dec 19, 2013 at 09:39:55AM -0800, Junio C Hamano wrote:

> Torsten Bögershausen <tboegi@web.de> writes:
> 
> > 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.
> 
> If you look at the caller, you will see that we do cap the value
> returned from this helper function down to a more reasonable and not
> so selfish maximum, exactly for the purpose of avoiding the risk of
> starving other processes.

I am not sure you are reading the capping in the right direction. We do
not cap at 25, but rather keep 25 open for "other stuff". So at
unlimited, we are consuming a mere UINT_MAX-25 descriptors. :)

I think that 25 is not for the benefit of the rest of the system, but
rather for _us_ to avoid running out of descriptors for normal
operations. I do not think we need to be careful about starving other
processes at all. That is the job of the ulimit in the first place, and
we respect it. If the sysadmin turns off the limit, then we are just
following their instructions.

In practice, I'd be shocked if git behaved reasonably above about 500
packs anyway, so that puts a practical cap on our fd use. :)

None of that impacts the patch under discussion, though. The only thing
I was trying to bring up earlier is that on a system with:

  1. No (or broken) getrlimit

  2. No OPEN_MAX defined

  3. sysconf that works, and returns -1 for unlimited

  4. a sysadmin who has set the descriptor limit to "unlimited"

We will end up at "1". Which is not great, but I am skeptical that a
system matching the above 4 constraints actually exists. So I think the
patch is fine in practice.

-Peff

Re: RLIMIT_NOFILE fallback

[Torsten Bögershausen]

On 2013-12-20 10.12, Jeff King wrote:
> On Thu, Dec 19, 2013 at 09:39:55AM -0800, Junio C Hamano wrote:
> 
>> Torsten Bögershausen <tboegi@web.de> writes:
>>
>>> 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.
>>
>> If you look at the caller, you will see that we do cap the value
>> returned from this helper function down to a more reasonable and not
>> so selfish maximum, exactly for the purpose of avoiding the risk of
>> starving other processes.
> 
> I am not sure you are reading the capping in the right direction. We do
> not cap at 25, but rather keep 25 open for "other stuff". So at
> unlimited, we are consuming a mere UINT_MAX-25 descriptors. :)
> 
> I think that 25 is not for the benefit of the rest of the system, but
> rather for _us_ to avoid running out of descriptors for normal
> operations. I do not think we need to be careful about starving other
> processes at all. That is the job of the ulimit in the first place, and
> we respect it. If the sysadmin turns off the limit, then we are just
> following their instructions.
> 
> In practice, I'd be shocked if git behaved reasonably above about 500
> packs anyway, so that puts a practical cap on our fd use. :)
> 
> None of that impacts the patch under discussion, though. The only thing
> I was trying to bring up earlier is that on a system with:
> 
>   1. No (or broken) getrlimit
> 
>   2. No OPEN_MAX defined
> 
>   3. sysconf that works, and returns -1 for unlimited
> 
>   4. a sysadmin who has set the descriptor limit to "unlimited"
> 
> We will end up at "1". Which is not great, but I am skeptical that a
> system matching the above 4 constraints actually exists. So I think the
> patch is fine in practice.
> 
> -Peff

My wrong: I was carefully reading the wrong version of the patch :-(
Sorry for the noise.
/torsten