Re: RLIMIT_NOFILE fallback

2013-12-20 Thread Jeff King 
On Thu, Dec 19, 2013 at 09:39:55AM -0800, Junio C Hamano wrote:

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

Re: RLIMIT_NOFILE fallback

2013-12-20 Thread 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 tbo...@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

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RLIMIT_NOFILE fallback

2013-12-19 Thread 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 p...@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.


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at

Re: RLIMIT_NOFILE fallback

2013-12-19 Thread Junio C Hamano 
Torsten Bögershausen tbo...@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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RLIMIT_NOFILE fallback

2013-12-18 Thread Junio C Hamano 
Joey Hess j...@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)
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RLIMIT_NOFILE fallback

2013-12-18 Thread Joey Hess 
Junio C Hamano wrote:
 Hmph, perhaps you are right.  Like this?

Works for me.

-- 
see shy jo


signature.asc
Description: Digital signature

Re: RLIMIT_NOFILE fallback

2013-12-18 Thread Junio C Hamano 
Jeff King p...@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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RLIMIT_NOFILE fallback

2013-12-18 Thread Joey Hess 
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


signature.asc
Description: Digital signature

Re: RLIMIT_NOFILE fallback

2013-12-18 Thread Junio C Hamano 
Junio C Hamano gits...@pobox.com writes:

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

Re: RLIMIT_NOFILE fallback

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

Re: RLIMIT_NOFILE fallback

2013-12-18 Thread Junio C Hamano 
Jeff King p...@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...
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RLIMIT_NOFILE fallback

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

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

Re: RLIMIT_NOFILE fallback

2013-12-18 Thread Junio C Hamano 
Jeff King p...@peff.net writes:

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

 Jeff King p...@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 j...@kitenet.net
Helped-by: Jeff King p...@peff.net
Signed-off-by: Junio C Hamano gits...@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

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RLIMIT_NOFILE fallback

2013-12-18 Thread Jeff King 
On Wed, Dec 18, 2013 at 02:59:12PM -0800, Junio C Hamano wrote:

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