{"thread":{"id":"35547","subject":"RLIMIT_NOFILE fallback","startedAt":"2013-12-18T17:14:46Z","lastAt":"2013-12-20T14:43:51Z","messageCount":16,"participants":["Joey Hess","Junio C Hamano","Jeff King","Torsten Bögershausen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"232172","messageId":"20131218171446.GA19657@kitenet.net","threadId":"35547","inReplyTo":null,"subject":"RLIMIT_NOFILE fallback","fromName":"Joey Hess","fromEmail":"joey@kitenet.net","sentAt":"2013-12-18T17:14:46Z","receivedAt":"2013-12-18T17:14:46Z","isPatch":false,"sender":{"key":"joey@kitenet.net","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"In sha1_file.c, when git is built on linux, it will use \ngetrlimit(RLIMIT_NOFILE). I've been deploying git binaries to some\nunusual systems, like embedded NAS devices, and it seems some with older\nkernels like 2.6.33 fail with \"fatal: cannot get RLIMIT_NOFILE: Bad address\".\n\nI could work around this by building git without RLIMIT_NOFILE defined,\nbut perhaps it would make sense to improve the code to fall back\nto one of the other methods for getting the limit, and/or return the\nhardcoded 1 as a fallback. This would make git binaries more robust\nagainst old/broken/misconfigured kernels.\n\n-- \nsee shy jo\n"},{"id":"232178","messageId":"xmqqy53ihwe4.fsf@gitster.dls.corp.google.com","threadId":"35547","inReplyTo":"20131218171446.GA19657@kitenet.net","subject":"Re: RLIMIT_NOFILE fallback","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-18T18:00:35Z","receivedAt":"2013-12-18T18:00:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joey Hess writes:\n\n> In sha1_file.c, when git is built on linux, it will use \n> getrlimit(RLIMIT_NOFILE). I've been deploying git binaries to some\n> unusual systems, like embedded NAS devices, and it seems some with older\n> kernels like 2.6.33 fail with \"fatal: cannot get RLIMIT_NOFILE: Bad address\".\n>\n> I could work around this by building git without RLIMIT_NOFILE defined,\n> but perhaps it would make sense to improve the code to fall back\n> to one of the other methods for getting the limit, and/or return the\n> hardcoded 1 as a fallback. This would make git binaries more robust\n> against old/broken/misconfigured kernels.\n\nHmph, perhaps you are right. Like this?\n\n sha1_file.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex daacc0c..a3a0014 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -809,8 +809,12 @@ static unsigned int get_max_fd_limit(void)\n #ifdef RLIMIT_NOFILE\n \tstruct rlimit lim;\n \n-\tif (getrlimit(RLIMIT_NOFILE, &lim))\n-\t\tdie_errno(\"cannot get RLIMIT_NOFILE\");\n+\tif (getrlimit(RLIMIT_NOFILE, &lim)) {\n+\t\tstatic int warn_only_once;\n+\t\tif (!warn_only_once++)\n+\t\t\twarning(\"cannot get RLIMIT_NOFILE: %s\", strerror(errno));\n+\t\treturn 1; /* see the caller ;-) */\n+\t}\n \n \treturn lim.rlim_cur;\n #elif defined(_SC_OPEN_MAX)\n"},{"id":"232181","messageId":"20131218184122.GB7936@kitenet.net","threadId":"35547","inReplyTo":"xmqqy53ihwe4.fsf@gitster.dls.corp.google.com","subject":"Re: RLIMIT_NOFILE fallback","fromName":"Joey Hess","fromEmail":"joey@kitenet.net","sentAt":"2013-12-18T18:41:22Z","receivedAt":"2013-12-18T18:41:22Z","isPatch":false,"sender":{"key":"joey@kitenet.net","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"Junio C Hamano wrote:\n> Hmph, perhaps you are right. Like this?\n\nWorks for me.\n\n-- \nsee shy jo\n"},{"id":"232184","messageId":"20131218191702.GA9083@sigill.intra.peff.net","threadId":"35547","inReplyTo":"xmqqy53ihwe4.fsf@gitster.dls.corp.google.com","subject":"Re: RLIMIT_NOFILE fallback","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-18T19:17:02Z","receivedAt":"2013-12-18T19:17:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 18, 2013 at 10:00:35AM -0800, Junio C Hamano wrote:\n\n> Joey Hess writes:\n> \n> > In sha1_file.c, when git is built on linux, it will use \n> > getrlimit(RLIMIT_NOFILE). I've been deploying git binaries to some\n> > unusual systems, like embedded NAS devices, and it seems some with older\n> > kernels like 2.6.33 fail with \"fatal: cannot get RLIMIT_NOFILE: Bad address\".\n> >\n> > I could work around this by building git without RLIMIT_NOFILE defined,\n> > but perhaps it would make sense to improve the code to fall back\n> > to one of the other methods for getting the limit, and/or return the\n> > hardcoded 1 as a fallback. This would make git binaries more robust\n> > against old/broken/misconfigured kernels.\n> \n> Hmph, perhaps you are right. Like this?\n> \n> sha1_file.c | 8 ++++++--\n> 1 file changed, 6 insertions(+), 2 deletions(-)\n> \n> diff --git a/sha1_file.c b/sha1_file.c\n> index daacc0c..a3a0014 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -809,8 +809,12 @@ static unsigned int get_max_fd_limit(void)\n> #ifdef RLIMIT_NOFILE\n> \tstruct rlimit lim;\n> \n> -\tif (getrlimit(RLIMIT_NOFILE, &lim))\n> -\t\tdie_errno(\"cannot get RLIMIT_NOFILE\");\n> +\tif (getrlimit(RLIMIT_NOFILE, &lim)) {\n> +\t\tstatic int warn_only_once;\n> +\t\tif (!warn_only_once++)\n> +\t\t\twarning(\"cannot get RLIMIT_NOFILE: %s\", strerror(errno));\n> +\t\treturn 1; /* see the caller ;-) */\n> +\t}\n\nI wish we understood why getrlimit was failing. Returning EFAULT seems\nlike an odd choice if it is not implemented for the system. On such a\nsystem, do the other fallbacks actually work? Would it work to do:\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex daacc0c..ab38795 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -809,11 +809,11 @@ static unsigned int get_max_fd_limit(void)\n #ifdef RLIMIT_NOFILE\n \tstruct rlimit lim;\n \n-\tif (getrlimit(RLIMIT_NOFILE, &lim))\n-\t\tdie_errno(\"cannot get RLIMIT_NOFILE\");\n+\tif (!getrlimit(RLIMIT_NOFILE, &lim))\n+\t\treturn lim.rlim_cur;\n+#endif\n \n-\treturn lim.rlim_cur;\n-#elif defined(_SC_OPEN_MAX)\n+#if defined(_SC_OPEN_MAX)\n \treturn sysconf(_SC_OPEN_MAX);\n #elif defined(OPEN_MAX)\n \treturn OPEN_MAX;\n\nThat is, does sysconf actually work on such a system (or does it need a\nsimilar run-time fallback)? And either way, we should try falling back\nto OPEN_MAX rather than 1 if we have it.\n\nAs far as the warning, I am not sure I see a point. The user does not\nhave any useful recourse, and git should continue to operate as normal.\nHaving every single git invocation print \"by the way, RLIMIT_NOFILE does\nnot work on your system\" seems like it would get annoying.\n\n-Peff\n"},{"id":"232187","messageId":"xmqq61qmhrb3.fsf@gitster.dls.corp.google.com","threadId":"35547","inReplyTo":"20131218191702.GA9083@sigill.intra.peff.net","subject":"Re: RLIMIT_NOFILE fallback","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-18T19:50:24Z","receivedAt":"2013-12-18T19:50:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> That is, does sysconf actually work on such a system (or does it need a\n> similar run-time fallback)? And either way, we should try falling back\n> to OPEN_MAX rather than 1 if we have it.\n\nInteresting.\n\n> As far as the warning, I am not sure I see a point. The user does not\n> have any useful recourse, and git should continue to operate as normal.\n> Having every single git invocation print \"by the way, RLIMIT_NOFILE does\n> not work on your system\" seems like it would get annoying.\n\nVery true. That makes the resulting function look like this:\n\n-------------------------------- 8< ------------------------------\n\nstatic unsigned int get_max_fd_limit(void)\n{\n#ifdef RLIMIT_NOFILE\n\tstruct rlimit lim;\n\n\tif (!getrlimit(RLIMIT_NOFILE, &lim))\n\t\treturn lim.rlim_cur;\n#endif\n\n#if defined(_SC_OPEN_MAX)\n\t{\n\t\tlong sc_open_max = sysconf(_SC_OPEN_MAX);\n\t\tif (0 < sc_open_max)\n\t\t\treturn sc_open_max;\n\t}\n\n#if defined(OPEN_MAX)\n\treturn OPEN_MAX;\n#else\n\treturn 1; /* see the caller ;-) */\n#endif\n}\n\n-------------------------------- >8 ------------------------------\n\nBut the sysconf part makes me wonder; here is what we see in\nhttp://pubs.opengroup.org/onlinepubs/9699919799/functions/sysconf.html\n\n If name is an invalid value, sysconf() shall return -1 and set errno\n to indicate the error. If the variable corresponding to name is\n described in as a maximum or minimum value and the\n variable has no limit, sysconf() shall return -1 without changing\n the value of errno. Note that indefinite limits do not imply\n infinite limits; see .\n\nFor a broken system (like RLIMIT_NOFILE defined for the compiler,\nbut the actual call returns a bogus error), the compiler may see the\n_SC_OPEN_MAX defined, while sysconf() may say \"I've never heard of\nsuch a name\" and return -1, or the system, whether broken or not,\nmay want to say \"Unlimited\" and return -1. The caller takes\nanything unreasonable as a positive value capped to 25 or something,\nso there isn't a real harm if we returned a bogus value from here,\nbut I am not sure what the safe default behaviour of this function\nshould be to help such a broken system while not harming systems\nthat are functioning correctly.\n"},{"id":"232191","messageId":"20131218200349.GA14532@kitenet.net","threadId":"35547","inReplyTo":"20131218191702.GA9083@sigill.intra.peff.net","subject":"Re: RLIMIT_NOFILE fallback","fromName":"Joey Hess","fromEmail":"joey@kitenet.net","sentAt":"2013-12-18T20:03:49Z","receivedAt":"2013-12-18T20:03:49Z","isPatch":false,"sender":{"key":"joey@kitenet.net","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"Jeff King wrote:\n> I wish we understood why getrlimit was failing. Returning EFAULT seems\n> like an odd choice if it is not implemented for the system. On such a\n> system, do the other fallbacks actually work? Would it work to do:\n> \n> That is, does sysconf actually work on such a system (or does it need a\n> similar run-time fallback)? And either way, we should try falling back\n> to OPEN_MAX rather than 1 if we have it.\n\nFor what it's worth, the system this happened on was a QNAP TS-219PII\nLinux willow 2.6.33.2 #1 Fri Mar 1 04:41:48 CST 2013 armv5tel unknown\n\nI don't have access to it to run tests of sysconf. (I already suggested its\nowner upgrade its firmware.)\n\n> As far as the warning, I am not sure I see a point. The user does not\n> have any useful recourse, and git should continue to operate as normal.\n> Having every single git invocation print \"by the way, RLIMIT_NOFILE does\n> not work on your system\" seems like it would get annoying.\n\nI agree with that.\n\n-- \nsee shy jo\n"},{"id":"232195","messageId":"xmqqlhzhhq0i.fsf@gitster.dls.corp.google.com","threadId":"35547","inReplyTo":"xmqq61qmhrb3.fsf@gitster.dls.corp.google.com","subject":"Re: RLIMIT_NOFILE fallback","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-18T20:18:21Z","receivedAt":"2013-12-18T20:18:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> That is, does sysconf actually work on such a system (or does it need a\n>> similar run-time fallback)? And either way, we should try falling back\n>> to OPEN_MAX rather than 1 if we have it.\n>\n> Interesting.\n>\n>> As far as the warning, I am not sure I see a point. The user does not\n>> have any useful recourse, and git should continue to operate as normal.\n>> Having every single git invocation print \"by the way, RLIMIT_NOFILE does\n>> not work on your system\" seems like it would get annoying.\n>\n> Very true. That makes the resulting function look like this:\n>\n>\n> -------------------------------- 8< ------------------------------\n>\n> static unsigned int get_max_fd_limit(void)\n> {\n> #ifdef RLIMIT_NOFILE\n> \tstruct rlimit lim;\n>\n> \tif (!getrlimit(RLIMIT_NOFILE, &lim))\n> \t\treturn lim.rlim_cur;\n> #endif\n>\n> #if defined(_SC_OPEN_MAX)\n> \t{\n> \t\tlong sc_open_max = sysconf(_SC_OPEN_MAX);\n> \t\tif (0 < sc_open_max)\n> \t\t\treturn sc_open_max;\n> \t}\n\nerr, here we need\n\n#endif /* defined(_SC_OPEN_MAX) */\n\nto truly implement the structure \"try all the available functions,\nand then fall back to OPEN_MAX\".\n\n\n\n>\n> #if defined(OPEN_MAX)\n> \treturn OPEN_MAX;\n> #else\n> \treturn 1; /* see the caller ;-) */\n> #endif\n> }\n>\n> -------------------------------- >8 ------------------------------\n>\n>\n> But the sysconf part makes me wonder; here is what we see in\n> http://pubs.opengroup.org/onlinepubs/9699919799/functions/sysconf.html\n>\n> If name is an invalid value, sysconf() shall return -1 and set errno\n> to indicate the error. If the variable corresponding to name is\n> described in as a maximum or minimum value and the\n> variable has no limit, sysconf() shall return -1 without changing\n> the value of errno. Note that indefinite limits do not imply\n> infinite limits; see .\n>\n> For a broken system (like RLIMIT_NOFILE defined for the compiler,\n> but the actual call returns a bogus error), the compiler may see the\n> _SC_OPEN_MAX defined, while sysconf() may say \"I've never heard of\n> such a name\" and return -1, or the system, whether broken or not,\n> may want to say \"Unlimited\" and return -1. The caller takes\n> anything unreasonable as a positive value capped to 25 or something,\n> so there isn't a real harm if we returned a bogus value from here,\n> but I am not sure what the safe default behaviour of this function\n> should be to help such a broken system while not harming systems\n> that are functioning correctly.\n"},{"id":"232203","messageId":"20131218212847.GA13685@sigill.intra.peff.net","threadId":"35547","inReplyTo":"xmqq61qmhrb3.fsf@gitster.dls.corp.google.com","subject":"Re: RLIMIT_NOFILE fallback","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-18T21:28:48Z","receivedAt":"2013-12-18T21:28:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 18, 2013 at 11:50:24AM -0800, Junio C Hamano wrote:\n\n> -------------------------------- 8< ------------------------------\n> \n> static unsigned int get_max_fd_limit(void)\n> {\n> #ifdef RLIMIT_NOFILE\n> \tstruct rlimit lim;\n> \n> \tif (!getrlimit(RLIMIT_NOFILE, &lim))\n> \t\treturn lim.rlim_cur;\n> #endif\n> \n> #if defined(_SC_OPEN_MAX)\n> \t{\n> \t\tlong sc_open_max = sysconf(_SC_OPEN_MAX);\n> \t\tif (0 < sc_open_max)\n> \t\t\treturn sc_open_max;\n> \t}\n> \n> #if defined(OPEN_MAX)\n> \treturn OPEN_MAX;\n> #else\n> \treturn 1; /* see the caller ;-) */\n> #endif\n> }\n> \n> -------------------------------- >8 ------------------------------\n\nYeah, with the #endif followup you posted, this is what I had in mind.\n\n> But the sysconf part makes me wonder; here is what we see in\n> http://pubs.opengroup.org/onlinepubs/9699919799/functions/sysconf.html\n> \n> If name is an invalid value, sysconf() shall return -1 and set errno\n> to indicate the error. If the variable corresponding to name is\n> described in as a maximum or minimum value and the\n> variable has no limit, sysconf() shall return -1 without changing\n> the value of errno. Note that indefinite limits do not imply\n> infinite limits; see .\n> \n> For a broken system (like RLIMIT_NOFILE defined for the compiler,\n> but the actual call returns a bogus error), the compiler may see the\n> _SC_OPEN_MAX defined, while sysconf() may say \"I've never heard of\n> such a name\" and return -1, or the system, whether broken or not,\n> may want to say \"Unlimited\" and return -1. The caller takes\n> anything unreasonable as a positive value capped to 25 or something,\n> so there isn't a real harm if we returned a bogus value from here,\n> but I am not sure what the safe default behaviour of this function\n> should be to help such a broken system while not harming systems\n> that are functioning correctly.\n\nAccording to the POSIX quote above, it sounds like we could do:\n\n #if defined (_SC_OPEN_MAX)\n {\n long max;\n errno = 0;\n max = sysconf(_SC_OPEN_MAX);\n if (0 < max) /* got the limit */\n return max;\n else if (!errno) /* unlimited, cast to int-max */\n return max;\n /* otherwise, fall through */\n }\n #endif\n\nObviously you could collapse the two branches of the conditional, though\nI think it deserves at least a comment to explain what is going on.\n\n-Peff\n"},{"id":"232204","messageId":"xmqqd2kthmcr.fsf@gitster.dls.corp.google.com","threadId":"35547","inReplyTo":"20131218212847.GA13685@sigill.intra.peff.net","subject":"Re: RLIMIT_NOFILE fallback","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-18T21:37:24Z","receivedAt":"2013-12-18T21:37:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> According to the POSIX quote above, it sounds like we could do:\n>\n> #if defined (_SC_OPEN_MAX)\n> {\n> long max;\n> errno = 0;\n> max = sysconf(_SC_OPEN_MAX);\n> if (0 < max) /* got the limit */\n> return max;\n> else if (!errno) /* unlimited, cast to int-max */\n> return max;\n> /* otherwise, fall through */\n> }\n> #endif\n>\n> Obviously you could collapse the two branches of the conditional, though\n> I think it deserves at least a comment to explain what is going on.\n\nYes, that is locally OK, but depending on how the caller behaves, we\nmight need to have an extra saved_errno dance here, which I didn't\nwant to get into...\n"},{"id":"232205","messageId":"20131218214001.GA14354@sigill.intra.peff.net","threadId":"35547","inReplyTo":"xmqqd2kthmcr.fsf@gitster.dls.corp.google.com","subject":"Re: RLIMIT_NOFILE fallback","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-18T21:40:01Z","receivedAt":"2013-12-18T21:40:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 18, 2013 at 01:37:24PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > According to the POSIX quote above, it sounds like we could do:\n> >\n> > #if defined (_SC_OPEN_MAX)\n> > {\n> > long max;\n> > errno = 0;\n> > max = sysconf(_SC_OPEN_MAX);\n> > if (0 < max) /* got the limit */\n> > return max;\n> > else if (!errno) /* unlimited, cast to int-max */\n> > return max;\n> > /* otherwise, fall through */\n> > }\n> > #endif\n> >\n> > Obviously you could collapse the two branches of the conditional, though\n> > I think it deserves at least a comment to explain what is going on.\n> \n> Yes, that is locally OK, but depending on how the caller behaves, we\n> might need to have an extra saved_errno dance here, which I didn't\n> want to get into...\n\nI think we are fine. The only caller is about to clobber errno by\nclosing packs anyway.\n\n-Peff\n"},{"id":"232212","messageId":"xmqqzjnxg3zz.fsf@gitster.dls.corp.google.com","threadId":"35547","inReplyTo":"20131218214001.GA14354@sigill.intra.peff.net","subject":"Re: RLIMIT_NOFILE fallback","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-18T22:59:12Z","receivedAt":"2013-12-18T22:59:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Dec 18, 2013 at 01:37:24PM -0800, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > According to the POSIX quote above, it sounds like we could do:\n>> >\n>> > #if defined (_SC_OPEN_MAX)\n>> > {\n>> > long max;\n>> > errno = 0;\n>> > max = sysconf(_SC_OPEN_MAX);\n>> > if (0 < max) /* got the limit */\n>> > return max;\n>> > else if (!errno) /* unlimited, cast to int-max */\n>> > return max;\n>> > /* otherwise, fall through */\n>> > }\n>> > #endif\n>> >\n>> > Obviously you could collapse the two branches of the conditional, though\n>> > I think it deserves at least a comment to explain what is going on.\n>> \n>> Yes, that is locally OK, but depending on how the caller behaves, we\n>> might need to have an extra saved_errno dance here, which I didn't\n>> want to get into...\n>\n> I think we are fine. The only caller is about to clobber errno by\n> closing packs anyway.\n>\n> -Peff\n\nOK.\n\n-- >8 --\nSubject: [PATCH] get_max_fd_limit(): fall back to OPEN_MAX upon getrlimit/sysconf failure\n\nOn broken systems where RLIMIT_NOFILE is visible by the compliers\nbut underlying getrlimit() system call does not behave, we used to\nsimply die() when we are trying to decide how many file descriptors\nto allocate for keeping packfiles open. Instead, allow the fallback\ncodepath to take over when we get such a failure from getrlimit().\n\nThe same issue exists with _SC_OPEN_MAX and sysconf(); restructure\nthe code in a similar way to prepare for a broken sysconf() as well.\n\nNoticed-by: Joey Hess \nHelped-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n sha1_file.c | 37 ++++++++++++++++++++++++++++++-------\n 1 file changed, 30 insertions(+), 7 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 760dd60..288badd 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -807,15 +807,38 @@ void free_pack_by_name(const char *pack_name)\n static unsigned int get_max_fd_limit(void)\n {\n #ifdef RLIMIT_NOFILE\n-\tstruct rlimit lim;\n+\t{\n+\t\tstruct rlimit lim;\n \n-\tif (getrlimit(RLIMIT_NOFILE, &lim))\n-\t\tdie_errno(\"cannot get RLIMIT_NOFILE\");\n+\t\tif (!getrlimit(RLIMIT_NOFILE, &lim))\n+\t\t\treturn lim.rlim_cur;\n+\t}\n+#endif\n+\n+#ifdef _SC_OPEN_MAX\n+\t{\n+\t\tlong open_max = sysconf(_SC_OPEN_MAX);\n+\t\tif (0 < open_max)\n+\t\t\treturn open_max;\n+\t\t/*\n+\t\t * Otherwise, we got -1 for one of the two\n+\t\t * reasons:\n+\t\t *\n+\t\t * (1) sysconf() did not understand _SC_OPEN_MAX\n+\t\t * and signaled an error with -1; or\n+\t\t * (2) sysconf() said there is no limit.\n+\t\t *\n+\t\t * We _could_ clear errno before calling sysconf() to\n+\t\t * tell these two cases apart and return a huge number\n+\t\t * in the latter case to let the caller cap it to a\n+\t\t * value that is not so selfish, but letting the\n+\t\t * fallback OPEN_MAX codepath take care of these cases\n+\t\t * is a lot simpler.\n+\t\t */\n+\t}\n+#endif\n \n-\treturn lim.rlim_cur;\n-#elif defined(_SC_OPEN_MAX)\n-\treturn sysconf(_SC_OPEN_MAX);\n-#elif defined(OPEN_MAX)\n+#ifdef OPEN_MAX\n \treturn OPEN_MAX;\n #else\n \treturn 1; /* see the caller ;-) */\n-- \n1.8.5.2-297-g3e57c29\n"},{"id":"232216","messageId":"20131219001519.GB17420@sigill.intra.peff.net","threadId":"35547","inReplyTo":"xmqqzjnxg3zz.fsf@gitster.dls.corp.google.com","subject":"Re: RLIMIT_NOFILE fallback","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-19T00:15:19Z","receivedAt":"2013-12-19T00:15:19Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 18, 2013 at 02:59:12PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n>\n> >> Yes, that is locally OK, but depending on how the caller behaves, we\n> >> might need to have an extra saved_errno dance here, which I didn't\n> >> want to get into...\n> >\n> > I think we are fine. The only caller is about to clobber errno by\n> > closing packs anyway.\n\nAlso, I do not think we would be any worse off than the current code.\ngetrlimit almost certainly just clobbered errno anyway. Either it is\nworth saving for the whole function, or not at all (and I think not at\nall).\n\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 760dd60..288badd 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -807,15 +807,38 @@ void free_pack_by_name(const char *pack_name)\n> static unsigned int get_max_fd_limit(void)\n> {\n> #ifdef RLIMIT_NOFILE\n> -\tstruct rlimit lim;\n> +\t{\n> +\t\tstruct rlimit lim;\n> \n> -\tif (getrlimit(RLIMIT_NOFILE, &lim))\n> -\t\tdie_errno(\"cannot get RLIMIT_NOFILE\");\n> +\t\tif (!getrlimit(RLIMIT_NOFILE, &lim))\n> +\t\t\treturn lim.rlim_cur;\n> +\t}\n> +#endif\n\nYeah, I think pulling the variable into its own block makes this more\nreadable.\n\n> +#ifdef _SC_OPEN_MAX\n> +\t{\n> +\t\tlong open_max = sysconf(_SC_OPEN_MAX);\n> +\t\tif (0 < open_max)\n> +\t\t\treturn open_max;\n> +\t\t/*\n> +\t\t * Otherwise, we got -1 for one of the two\n> +\t\t * reasons:\n> +\t\t *\n> +\t\t * (1) sysconf() did not understand _SC_OPEN_MAX\n> +\t\t * and signaled an error with -1; or\n> +\t\t * (2) sysconf() said there is no limit.\n> +\t\t *\n> +\t\t * We _could_ clear errno before calling sysconf() to\n> +\t\t * tell these two cases apart and return a huge number\n> +\t\t * in the latter case to let the caller cap it to a\n> +\t\t * value that is not so selfish, but letting the\n> +\t\t * fallback OPEN_MAX codepath take care of these cases\n> +\t\t * is a lot simpler.\n> +\t\t */\n> +\t}\n> +#endif\n\nThis is probably OK. I assume sane systems actually provide OPEN_MAX,\nand/or have a working getrlimit in the first place.\n\nThe fallback of \"1\" is actually quite low and can have an impact. Both\nfor performance, but also for concurrent use. We used to run into a\nproblem at GitHub where pack-objects serving a clone would have its\npackfile removed from under it (by a concurrent repack), and then would\ndie. The normal code paths are able to just retry the object lookup and\nfind the new pack, but the pack-objects code is a bit more intimate with\nthe particular packfile and cannot (currently) do so. With a large\nenough mmap window and descriptor limit, we just keep the packfiles\nopen. But if we have to close them for resource limits (like a too-low\ndescriptor limit), then we can end up in the die() situation above.\n\n-Peff\n"},{"id":"232240","messageId":"52B32D18.80400@web.de","threadId":"35547","inReplyTo":"20131219001519.GB17420@sigill.intra.peff.net","subject":"Re: RLIMIT_NOFILE fallback","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-12-19T17:30:00Z","receivedAt":"2013-12-19T17:30:00Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2013-12-19 01.15, Jeff King wrote:\n> On Wed, Dec 18, 2013 at 02:59:12PM -0800, Junio C Hamano wrote:\n> \n>> Jeff King writes:\n>>\n>>>> Yes, that is locally OK, but depending on how the caller behaves, we\n>>>> might need to have an extra saved_errno dance here, which I didn't\n>>>> want to get into...\n>>>\n>>> I think we are fine. The only caller is about to clobber errno by\n>>> closing packs anyway.\n> \n> Also, I do not think we would be any worse off than the current code.\n> getrlimit almost certainly just clobbered errno anyway. Either it is\n> worth saving for the whole function, or not at all (and I think not at\n> all).\n> \n>> diff --git a/sha1_file.c b/sha1_file.c\n>> index 760dd60..288badd 100644\n>> --- a/sha1_file.c\n>> +++ b/sha1_file.c\n>> @@ -807,15 +807,38 @@ void free_pack_by_name(const char *pack_name)\n>> static unsigned int get_max_fd_limit(void)\n>> {\n>> #ifdef RLIMIT_NOFILE\n>> -\tstruct rlimit lim;\n>> +\t{\n>> +\t\tstruct rlimit lim;\n>> \n>> -\tif (getrlimit(RLIMIT_NOFILE, &lim))\n>> -\t\tdie_errno(\"cannot get RLIMIT_NOFILE\");\n>> +\t\tif (!getrlimit(RLIMIT_NOFILE, &lim))\n>> +\t\t\treturn lim.rlim_cur;\n>> +\t}\n>> +#endif\n> \n> Yeah, I think pulling the variable into its own block makes this more\n> readable.\n> \n>> +#ifdef _SC_OPEN_MAX\n>> +\t{\n>> +\t\tlong open_max = sysconf(_SC_OPEN_MAX);\n>> +\t\tif (0 < open_max)\n>> +\t\t\treturn open_max;\n>> +\t\t/*\n>> +\t\t * Otherwise, we got -1 for one of the two\n>> +\t\t * reasons:\n>> +\t\t *\n>> +\t\t * (1) sysconf() did not understand _SC_OPEN_MAX\n>> +\t\t * and signaled an error with -1; or\n>> +\t\t * (2) sysconf() said there is no limit.\n>> +\t\t *\n>> +\t\t * We _could_ clear errno before calling sysconf() to\n>> +\t\t * tell these two cases apart and return a huge number\n>> +\t\t * in the latter case to let the caller cap it to a\n>> +\t\t * value that is not so selfish, but letting the\n>> +\t\t * fallback OPEN_MAX codepath take care of these cases\n>> +\t\t * is a lot simpler.\n>> +\t\t */\n>> +\t}\n>> +#endif\n> \n> This is probably OK. I assume sane systems actually provide OPEN_MAX,\n> and/or have a working getrlimit in the first place.\n> \n> The fallback of \"1\" is actually quite low and can have an impact. Both\n> for performance, but also for concurrent use. We used to run into a\n> problem at GitHub where pack-objects serving a clone would have its\n> packfile removed from under it (by a concurrent repack), and then would\n> die. The normal code paths are able to just retry the object lookup and\n> find the new pack, but the pack-objects code is a bit more intimate with\n> the particular packfile and cannot (currently) do so. With a large\n> enough mmap window and descriptor limit, we just keep the packfiles\n> open. But if we have to close them for resource limits (like a too-low\n> descriptor limit), then we can end up in the die() situation above.\n> \n> -Peff\n\nThanks for an interesting reading,\nplease allow a side question:\nCould it be, that \"-1 == unlimited\" is Linux specific?\nAnd therefore not 100% portable ?\n\nAnd doesn't \"unlimited\" number of files call for trouble,\nhaving the risk to starve the machine ?\n\nBTW: cygwin returns 256.\n\n------------\nhttp://pubs.opengroup.org/onlinepubs/007908799/xsh/sysconf.html\nRETURN VALUE\n\n 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. \n\n---------- Mac OS, based on BSD (?): ---------- \nRETURN VALUES\n If the call to sysconf() is not successful, -1 is returned and errno is\n set appropriately. Otherwise, if the variable is associated with func-\n tionality that is not supported, -1 is returned and errno is not modi-\n fied. Otherwise, the current variable value is returned.\n\nERRORS\n The sysconf() function may fail and set errno for any of the errors spec-\n ified for the library function sysctl(3). In addition, the following\n error may be reported:\n\n [EINVAL] The value of the name argument is invalid.\n[snip]\n The sysconf() function first appeared in 4.4BSD.\n\n-----------\nLinux, Debian:\n OPEN_MAX - _SC_OPEN_MAX\n The maximum number of files that a process can have open at any\n time. Must not be less than _POSIX_OPEN_MAX (20).\n[snip]\nRETURN VALUE\n If name is invalid, -1 is returned, and errno is set to EINVAL. Other‐\n wise, the value returned is the value of the system resource and errno\n is not changed. In the case of options, a positive value is returned\n if a queried option is available, and -1 if it is not. In the case of\n limits, -1 means that there is no definite limit.\n"},{"id":"232241","messageId":"xmqqmwjwg2ok.fsf@gitster.dls.corp.google.com","threadId":"35547","inReplyTo":"52B32D18.80400@web.de","subject":"Re: RLIMIT_NOFILE fallback","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-19T17:39:55Z","receivedAt":"2013-12-19T17:39:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> Thanks for an interesting reading,\n> please allow a side question:\n> Could it be, that \"-1 == unlimited\" is Linux specific?\n> And therefore not 100% portable ?\n>\n> And doesn't \"unlimited\" number of files call for trouble,\n> having the risk to starve the machine ?\n>\n> BTW: cygwin returns 256.\n\nIf you look at the caller, you will see that we do cap the value\nreturned from this helper function down to a more reasonable and not\nso selfish maximum, exactly for the purpose of avoiding the risk of\nstarving other processes.\n\n>\n> ------------\n> http://pubs.opengroup.org/onlinepubs/007908799/xsh/sysconf.html\n> RETURN VALUE\n>\n> 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. \n\nThat is a rather dated document. POSIX.1-2013 (look for the URL to\nthe corresponding page in an earlier message from me) has a bit\ntighter wording than that to clarify the \"there is no limit\" case.\n\nIn addition, the final version Peff and I worked out does not even\nlook at the value of errno, in order not to rely on possibly\nambiguous interpretations of negative return values. So I think we\nare good.\n\nThanks.\n\n> ---------- Mac OS, based on BSD (?): ---------- \n> RETURN VALUES\n> If the call to sysconf() is not successful, -1 is returned and errno is\n> set appropriately. Otherwise, if the variable is associated with func-\n> tionality that is not supported, -1 is returned and errno is not modi-\n> fied. Otherwise, the current variable value is returned.\n>\n> ERRORS\n> The sysconf() function may fail and set errno for any of the errors spec-\n> ified for the library function sysctl(3). In addition, the following\n> error may be reported:\n>\n> [EINVAL] The value of the name argument is invalid.\n> [snip]\n> The sysconf() function first appeared in 4.4BSD.\n>\n> -----------\n> Linux, Debian:\n> OPEN_MAX - _SC_OPEN_MAX\n> The maximum number of files that a process can have open at any\n> time. Must not be less than _POSIX_OPEN_MAX (20).\n> [snip]\n> RETURN VALUE\n> If name is invalid, -1 is returned, and errno is set to EINVAL. Other‐\n> wise, the value returned is the value of the system resource and errno\n> is not changed. In the case of options, a positive value is returned\n> if a queried option is available, and -1 if it is not. In the case of\n> limits, -1 means that there is no definite limit.\n"},{"id":"232280","messageId":"20131220091232.GA9637@sigill.intra.peff.net","threadId":"35547","inReplyTo":"xmqqmwjwg2ok.fsf@gitster.dls.corp.google.com","subject":"Re: RLIMIT_NOFILE fallback","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-20T09:12:33Z","receivedAt":"2013-12-20T09:12:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 19, 2013 at 09:39:55AM -0800, Junio C Hamano wrote:\n\n> Torsten Bögershausen writes:\n> \n> > Thanks for an interesting reading,\n> > please allow a side question:\n> > Could it be, that \"-1 == unlimited\" is Linux specific?\n> > And therefore not 100% portable ?\n> >\n> > And doesn't \"unlimited\" number of files call for trouble,\n> > having the risk to starve the machine ?\n> >\n> > BTW: cygwin returns 256.\n> \n> If you look at the caller, you will see that we do cap the value\n> returned from this helper function down to a more reasonable and not\n> so selfish maximum, exactly for the purpose of avoiding the risk of\n> starving other processes.\n\nI am not sure you are reading the capping in the right direction. We do\nnot cap at 25, but rather keep 25 open for \"other stuff\". So at\nunlimited, we are consuming a mere UINT_MAX-25 descriptors. :)\n\nI think that 25 is not for the benefit of the rest of the system, but\nrather for _us_ to avoid running out of descriptors for normal\noperations. I do not think we need to be careful about starving other\nprocesses at all. That is the job of the ulimit in the first place, and\nwe respect it. If the sysadmin turns off the limit, then we are just\nfollowing their instructions.\n\nIn practice, I'd be shocked if git behaved reasonably above about 500\npacks anyway, so that puts a practical cap on our fd use. :)\n\nNone of that impacts the patch under discussion, though. The only thing\nI was trying to bring up earlier is that on a system with:\n\n 1. No (or broken) getrlimit\n\n 2. No OPEN_MAX defined\n\n 3. sysconf that works, and returns -1 for unlimited\n\n 4. a sysadmin who has set the descriptor limit to \"unlimited\"\n\nWe will end up at \"1\". Which is not great, but I am skeptical that a\nsystem matching the above 4 constraints actually exists. So I think the\npatch is fine in practice.\n\n-Peff\n"},{"id":"232285","messageId":"52B457A7.4050901@web.de","threadId":"35547","inReplyTo":"20131220091232.GA9637@sigill.intra.peff.net","subject":"Re: RLIMIT_NOFILE fallback","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-12-20T14:43:51Z","receivedAt":"2013-12-20T14:43:51Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2013-12-20 10.12, Jeff King wrote:\n> On Thu, Dec 19, 2013 at 09:39:55AM -0800, Junio C Hamano wrote:\n> \n>> Torsten Bögershausen writes:\n>>\n>>> Thanks for an interesting reading,\n>>> please allow a side question:\n>>> Could it be, that \"-1 == unlimited\" is Linux specific?\n>>> And therefore not 100% portable ?\n>>>\n>>> And doesn't \"unlimited\" number of files call for trouble,\n>>> having the risk to starve the machine ?\n>>>\n>>> BTW: cygwin returns 256.\n>>\n>> If you look at the caller, you will see that we do cap the value\n>> returned from this helper function down to a more reasonable and not\n>> so selfish maximum, exactly for the purpose of avoiding the risk of\n>> starving other processes.\n> \n> I am not sure you are reading the capping in the right direction. We do\n> not cap at 25, but rather keep 25 open for \"other stuff\". So at\n> unlimited, we are consuming a mere UINT_MAX-25 descriptors. :)\n> \n> I think that 25 is not for the benefit of the rest of the system, but\n> rather for _us_ to avoid running out of descriptors for normal\n> operations. I do not think we need to be careful about starving other\n> processes at all. That is the job of the ulimit in the first place, and\n> we respect it. If the sysadmin turns off the limit, then we are just\n> following their instructions.\n> \n> In practice, I'd be shocked if git behaved reasonably above about 500\n> packs anyway, so that puts a practical cap on our fd use. :)\n> \n> None of that impacts the patch under discussion, though. The only thing\n> I was trying to bring up earlier is that on a system with:\n> \n> 1. No (or broken) getrlimit\n> \n> 2. No OPEN_MAX defined\n> \n> 3. sysconf that works, and returns -1 for unlimited\n> \n> 4. a sysadmin who has set the descriptor limit to \"unlimited\"\n> \n> We will end up at \"1\". Which is not great, but I am skeptical that a\n> system matching the above 4 constraints actually exists. So I think the\n> patch is fine in practice.\n> \n> -Peff\n\nMy wrong: I was carefully reading the wrong version of the patch :-(\nSorry for the noise.\n/torsten\n"}]}