{"thread":{"id":"26622","subject":"[PATCH] Limit file descriptors used by packs","startedAt":"2011-02-28T20:27:15Z","lastAt":"2011-03-02T18:01:54Z","messageCount":10,"participants":["Shawn O. Pearce","Bernhard R. Link","Junio C Hamano","Erik Faye-Lund","Shawn Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"162466","messageId":"1298924835-23413-1-git-send-email-spearce@spearce.org","threadId":"26622","inReplyTo":null,"subject":"[PATCH] Limit file descriptors used by packs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-02-28T20:27:15Z","receivedAt":"2011-02-28T20:27:15Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Rather than using 'errno == EMFILE' after a failed open() call\nto indicate the process is out of file descriptors and an LRU\npack window should be closed, place a hard upper limit on the\nnumber of open packs based on the actual rlimit of the process.\n\nBy using a hard upper limit that is below the rlimit of the current\nprocess, it is not necessary to check for EMFILE on every single\nfd-allocating system call.  Instead reserving 8 file descriptors\nmakes it safe to assume the system call won't fail due to being\nover limit in the filedescriptor limit.\n\nThis fixes a case where running `git gc --auto` in a repository\nwith more than 1024 packs (but an rlimit of 1024 open fds) fails\ndue to the temporary output file not being able to allocate a\nfile descriptor.  The output file is opened by pack-objects after\nobject enumeration and delete compression are done, both of which\nhave already opened all of the packs and fully populated the file\ndescriptor table.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n sha1_file.c |   49 ++++++++++++++++++++++++++++++++++++-------------\n 1 files changed, 36 insertions(+), 13 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex d949b35..8863ff6 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -418,6 +418,8 @@ static unsigned int pack_used_ctr;\n static unsigned int pack_mmap_calls;\n static unsigned int peak_pack_open_windows;\n static unsigned int pack_open_windows;\n+static unsigned int pack_open_fds;\n+static unsigned int pack_max_fds;\n static size_t peak_pack_mapped;\n static size_t pack_mapped;\n struct packed_git *packed_git;\n@@ -597,6 +599,7 @@ static int unuse_one_window(struct packed_git *current, int keep_fd)\n \t\t\tlru_p->windows = lru_w->next;\n \t\t\tif (!lru_p->windows && lru_p->pack_fd != keep_fd) {\n \t\t\t\tclose(lru_p->pack_fd);\n+\t\t\t\tpack_open_fds--;\n \t\t\t\tlru_p->pack_fd = -1;\n \t\t\t}\n \t\t}\n@@ -681,8 +684,10 @@ void free_pack_by_name(const char *pack_name)\n \t\tif (strcmp(pack_name, p->pack_name) == 0) {\n \t\t\tclear_delta_base_cache();\n \t\t\tclose_pack_windows(p);\n-\t\t\tif (p->pack_fd != -1)\n+\t\t\tif (p->pack_fd != -1) {\n \t\t\t\tclose(p->pack_fd);\n+\t\t\t\tpack_open_fds--;\n+\t\t\t}\n \t\t\tclose_pack_index(p);\n \t\t\tfree(p->bad_object_sha1);\n \t\t\t*pp = p->next;\n@@ -708,9 +713,35 @@ static int open_packed_git_1(struct packed_git *p)\n \tif (!p->index_data && open_pack_index(p))\n \t\treturn error(\"packfile %s index unavailable\", p->pack_name);\n \n+\tif (!pack_max_fds) {\n+\t\tstruct rlimit lim;\n+\t\tunsigned int max_fds;\n+\n+\t\tif (getrlimit(RLIMIT_NOFILE, &lim))\n+\t\t\tdie_errno(\"cannot get RLIMIT_NOFILE\");\n+\n+\t\tmax_fds = lim.rlim_cur;\n+\n+\t\tif (lim.rlim_cur < lim.rlim_max) {\n+\t\t\tlim.rlim_cur = lim.rlim_max;\n+\t\t\tif (!setrlimit(RLIMIT_NOFILE, &lim))\n+\t\t\t\tmax_fds = lim.rlim_max;\n+\t\t}\n+\n+\t\t/* Save 3 for stdin/stdout/stderr, 5 for work */\n+\t\tif (8 < max_fds)\n+\t\t\tpack_max_fds = max_fds - 8;\n+\t\telse\n+\t\t\tpack_max_fds = 1;\n+\t}\n+\n+\twhile (pack_max_fds <= pack_open_fds && unuse_one_window(NULL, -1))\n+\t  /* nothing */;\n+\n \tp->pack_fd = git_open_noatime(p->pack_name, p);\n \tif (p->pack_fd < 0 || fstat(p->pack_fd, &st))\n \t\treturn -1;\n+\tpack_open_fds++;\n \n \t/* If we created the struct before we had the pack we lack size. */\n \tif (!p->pack_size) {\n@@ -762,6 +793,7 @@ static int open_packed_git(struct packed_git *p)\n \t\treturn 0;\n \tif (p->pack_fd != -1) {\n \t\tclose(p->pack_fd);\n+\t\tpack_open_fds--;\n \t\tp->pack_fd = -1;\n \t}\n \treturn -1;\n@@ -919,6 +951,9 @@ struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path)\n \n void install_packed_git(struct packed_git *pack)\n {\n+\tif (pack->pack_fd != -1)\n+\t\tpack_open_fds++;\n+\n \tpack->next = packed_git;\n \tpacked_git = pack;\n }\n@@ -936,8 +971,6 @@ static void prepare_packed_git_one(char *objdir, int local)\n \tsprintf(path, \"%s/pack\", objdir);\n \tlen = strlen(path);\n \tdir = opendir(path);\n-\twhile (!dir && errno == EMFILE && unuse_one_window(NULL, -1))\n-\t\tdir = opendir(path);\n \tif (!dir) {\n \t\tif (errno != ENOENT)\n \t\t\terror(\"unable to open object pack directory: %s: %s\",\n@@ -1093,14 +1126,6 @@ static int git_open_noatime(const char *name, struct packed_git *p)\n \t\tif (fd >= 0)\n \t\t\treturn fd;\n \n-\t\t/* Might the failure be insufficient file descriptors? */\n-\t\tif (errno == EMFILE) {\n-\t\t\tif (unuse_one_window(p, -1))\n-\t\t\t\tcontinue;\n-\t\t\telse\n-\t\t\t\treturn -1;\n-\t\t}\n-\n \t\t/* Might the failure be due to O_NOATIME? */\n \t\tif (errno != ENOENT && sha1_file_open_flag) {\n \t\t\tsha1_file_open_flag = 0;\n@@ -2360,8 +2385,6 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \n \tfilename = sha1_file_name(sha1);\n \tfd = create_tmpfile(tmpfile, sizeof(tmpfile), filename);\n-\twhile (fd < 0 && errno == EMFILE && unuse_one_window(NULL, -1))\n-\t\tfd = create_tmpfile(tmpfile, sizeof(tmpfile), filename);\n \tif (fd < 0) {\n \t\tif (errno == EACCES)\n \t\t\treturn error(\"insufficient permission for adding an object to repository database %s\\n\", get_object_directory());\n-- \n1.7.4.1.249.g4aa7\n"},{"id":"162467","messageId":"20110228203557.GA8189@pcpool00.mathematik.uni-freiburg.de","threadId":"26622","inReplyTo":"1298924835-23413-1-git-send-email-spearce@spearce.org","subject":"Re: [PATCH] Limit file descriptors used by packs","fromName":"Bernhard R. Link","fromEmail":"brl+ccmadness@pcpool00.mathematik.uni-freiburg.de","sentAt":"2011-02-28T20:35:57Z","receivedAt":"2011-02-28T20:35:57Z","isPatch":true,"sender":{"key":"brl+ccmadness@pcpool00.mathematik.uni-freiburg.de","avatar":null},"body":"* Shawn O. Pearce <spearce@spearce.org> [110228 21:27]:\n> By using a hard upper limit that is below the rlimit of the current\n> process, it is not necessary to check for EMFILE on every single\n> fd-allocating system call.  Instead reserving 8 file descriptors\n> makes it safe to assume the system call won't fail due to being\n> over limit in the filedescriptor limit.\n\nIsn't 8 quite a bit low for a reserve? Couldn't some libc stuff\n(especially nss modules perhaps activated by something) easily surpass\nthat?\n\n\tBernhard R. Link\n"},{"id":"162468","messageId":"7vwrkjhp27.fsf@alter.siamese.dyndns.org","threadId":"26622","inReplyTo":"1298924835-23413-1-git-send-email-spearce@spearce.org","subject":"Re: [PATCH] Limit file descriptors used by packs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-28T20:38:56Z","receivedAt":"2011-02-28T20:38:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> writes:\n\n> ...  The output file is opened by pack-objects after\n> object enumeration and delete compression are done, ...\n\ns/delete/deflate/, I guess.\n\n> diff --git a/sha1_file.c b/sha1_file.c\n> index d949b35..8863ff6 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -708,9 +713,35 @@ static int open_packed_git_1(struct packed_git *p)\n>  \tif (!p->index_data && open_pack_index(p))\n>  \t\treturn error(\"packfile %s index unavailable\", p->pack_name);\n>  \n> +\tif (!pack_max_fds) {\n> + ...\n> +\t\tif (lim.rlim_cur < lim.rlim_max) {\n> +\t\t\tlim.rlim_cur = lim.rlim_max;\n> +\t\t\tif (!setrlimit(RLIMIT_NOFILE, &lim))\n> +\t\t\t\tmax_fds = lim.rlim_max;\n> +\t\t}\n\nThis is somewhat questionable, isn't it?  We don't know why the user chose\nto ulimit the process yet forcibly bust that limit without telling him?\n\nOther than that it looks sensible.  Thanks.\n"},{"id":"162469","messageId":"20110228204406.GA26052@spearce.org","threadId":"26622","inReplyTo":"20110228203557.GA8189@pcpool00.mathematik.uni-freiburg.de","subject":"Re: [PATCH] Limit file descriptors used by packs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-02-28T20:44:06Z","receivedAt":"2011-02-28T20:44:06Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"\"Bernhard R. Link\" <brl+ccmadness@pcpool00.mathematik.uni-freiburg.de> wrote:\n> * Shawn O. Pearce <spearce@spearce.org> [110228 21:27]:\n> > By using a hard upper limit that is below the rlimit of the current\n> > process, it is not necessary to check for EMFILE on every single\n> > fd-allocating system call.  Instead reserving 8 file descriptors\n> > makes it safe to assume the system call won't fail due to being\n> > over limit in the filedescriptor limit.\n> \n> Isn't 8 quite a bit low for a reserve? Couldn't some libc stuff\n> (especially nss modules perhaps activated by something) easily surpass\n> that?\n\nOriginally I proposed 25 to Junio, but he scoffed and said that\nwas quite high. So I went with 8, 3 for std{in,out,err} and 5 as\na WAG for everything else.\n\nIts arbitrary, 25 might be a better WAG than 8...\n\n-- \nShawn.\n"},{"id":"162470","messageId":"20110228204727.GB26052@spearce.org","threadId":"26622","inReplyTo":"7vwrkjhp27.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Limit file descriptors used by packs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-02-28T20:47:27Z","receivedAt":"2011-02-28T20:47:27Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> \"Shawn O. Pearce\" <spearce@spearce.org> writes:\n> \n> > ...  The output file is opened by pack-objects after\n> > object enumeration and delete compression are done, ...\n> \n> s/delete/deflate/, I guess.\n\ns/delete/delta/ is what I meant. \"Compressing objects\" is the\ndelta compression phase. The fact that we save some small deltas in\ndeflated format during this phase is an uninteresting implementation\ndetail not worth mentioning in the comments.\n \n> > diff --git a/sha1_file.c b/sha1_file.c\n> > index d949b35..8863ff6 100644\n> > --- a/sha1_file.c\n> > +++ b/sha1_file.c\n> > @@ -708,9 +713,35 @@ static int open_packed_git_1(struct packed_git *p)\n> >  \tif (!p->index_data && open_pack_index(p))\n> >  \t\treturn error(\"packfile %s index unavailable\", p->pack_name);\n> >  \n> > +\tif (!pack_max_fds) {\n> > + ...\n> > +\t\tif (lim.rlim_cur < lim.rlim_max) {\n> > +\t\t\tlim.rlim_cur = lim.rlim_max;\n> > +\t\t\tif (!setrlimit(RLIMIT_NOFILE, &lim))\n> > +\t\t\t\tmax_fds = lim.rlim_max;\n> > +\t\t}\n> \n> This is somewhat questionable, isn't it?  We don't know why the user chose\n> to ulimit the process yet forcibly bust that limit without telling him?\n\nMaybe you are right.\n\nIn network server code is somewhat common to push the rlim_cur to\nrlim_max if its not already there, since you might need to use a\nlot of fds to handle a lot of concurrent clients. So habit sort of\ncaused me to just do this out of instinct.\n\nIn this particular part of C Git, if we are bumping up against the\nhard pack_max_fds limit we're already into some pretty difficult\ncomputation. Trying to push the rlimit higher in order to avoid\nclose/open calls as we cycle through fds isn't really going to make\na huge difference on end-user latency for the command to finish\nits task. So maybe we are better off honoring the rlim_cur that we\ninherited from the user/environment.\n\nI'll respin a v2 for you.\n\n-- \nShawn.\n"},{"id":"162471","messageId":"1298926359-26438-1-git-send-email-spearce@spearce.org","threadId":"26622","inReplyTo":"20110228204727.GB26052@spearce.org","subject":"[PATCH v2] Limit file descriptors used by packs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-02-28T20:52:39Z","receivedAt":"2011-02-28T20:52:39Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Rather than using 'errno == EMFILE' after a failed open() call\nto indicate the process is out of file descriptors and an LRU\npack window should be closed, place a hard upper limit on the\nnumber of open packs based on the actual rlimit of the process.\n\nBy using a hard upper limit that is below the rlimit of the current\nprocess it is not necessary to check for EMFILE on every single\nfd-allocating system call.  Instead reserving 25 file descriptors\nmakes it safe to assume the system call won't fail due to being over\nthe filedescriptor limit.  Here 25 is chosen as a WAG, but considers\n3 for stdin/stdout/stderr, and at least a few for other Git code\nto operate on temporary files.  An additional 20 is reserved as it\nis not known what the C library needs to perform other services on\nGit's behalf, such as nsswitch or name resolution.\n\nThis fixes a case where running `git gc --auto` in a repository\nwith more than 1024 packs (but an rlimit of 1024 open fds) fails\ndue to the temporary output file not being able to allocate a\nfile descriptor.  The output file is opened by pack-objects after\nobject enumeration and delta compression are done, both of which\nhave already opened all of the packs and fully populated the file\ndescriptor table.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n sha1_file.c |   43 ++++++++++++++++++++++++++++++-------------\n 1 files changed, 30 insertions(+), 13 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex d949b35..7850c18 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -418,6 +418,8 @@ static unsigned int pack_used_ctr;\n static unsigned int pack_mmap_calls;\n static unsigned int peak_pack_open_windows;\n static unsigned int pack_open_windows;\n+static unsigned int pack_open_fds;\n+static unsigned int pack_max_fds;\n static size_t peak_pack_mapped;\n static size_t pack_mapped;\n struct packed_git *packed_git;\n@@ -597,6 +599,7 @@ static int unuse_one_window(struct packed_git *current, int keep_fd)\n \t\t\tlru_p->windows = lru_w->next;\n \t\t\tif (!lru_p->windows && lru_p->pack_fd != keep_fd) {\n \t\t\t\tclose(lru_p->pack_fd);\n+\t\t\t\tpack_open_fds--;\n \t\t\t\tlru_p->pack_fd = -1;\n \t\t\t}\n \t\t}\n@@ -681,8 +684,10 @@ void free_pack_by_name(const char *pack_name)\n \t\tif (strcmp(pack_name, p->pack_name) == 0) {\n \t\t\tclear_delta_base_cache();\n \t\t\tclose_pack_windows(p);\n-\t\t\tif (p->pack_fd != -1)\n+\t\t\tif (p->pack_fd != -1) {\n \t\t\t\tclose(p->pack_fd);\n+\t\t\t\tpack_open_fds--;\n+\t\t\t}\n \t\t\tclose_pack_index(p);\n \t\t\tfree(p->bad_object_sha1);\n \t\t\t*pp = p->next;\n@@ -708,9 +713,29 @@ static int open_packed_git_1(struct packed_git *p)\n \tif (!p->index_data && open_pack_index(p))\n \t\treturn error(\"packfile %s index unavailable\", p->pack_name);\n \n+\tif (!pack_max_fds) {\n+\t\tstruct rlimit lim;\n+\t\tunsigned int max_fds;\n+\n+\t\tif (getrlimit(RLIMIT_NOFILE, &lim))\n+\t\t\tdie_errno(\"cannot get RLIMIT_NOFILE\");\n+\n+\t\tmax_fds = lim.rlim_cur;\n+\n+\t\t/* Save 3 for stdin/stdout/stderr, 22 for work */\n+\t\tif (25 < max_fds)\n+\t\t\tpack_max_fds = max_fds - 25;\n+\t\telse\n+\t\t\tpack_max_fds = 1;\n+\t}\n+\n+\twhile (pack_max_fds <= pack_open_fds && unuse_one_window(NULL, -1))\n+\t  /* nothing */;\n+\n \tp->pack_fd = git_open_noatime(p->pack_name, p);\n \tif (p->pack_fd < 0 || fstat(p->pack_fd, &st))\n \t\treturn -1;\n+\tpack_open_fds++;\n \n \t/* If we created the struct before we had the pack we lack size. */\n \tif (!p->pack_size) {\n@@ -762,6 +787,7 @@ static int open_packed_git(struct packed_git *p)\n \t\treturn 0;\n \tif (p->pack_fd != -1) {\n \t\tclose(p->pack_fd);\n+\t\tpack_open_fds--;\n \t\tp->pack_fd = -1;\n \t}\n \treturn -1;\n@@ -919,6 +945,9 @@ struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path)\n \n void install_packed_git(struct packed_git *pack)\n {\n+\tif (pack->pack_fd != -1)\n+\t\tpack_open_fds++;\n+\n \tpack->next = packed_git;\n \tpacked_git = pack;\n }\n@@ -936,8 +965,6 @@ static void prepare_packed_git_one(char *objdir, int local)\n \tsprintf(path, \"%s/pack\", objdir);\n \tlen = strlen(path);\n \tdir = opendir(path);\n-\twhile (!dir && errno == EMFILE && unuse_one_window(NULL, -1))\n-\t\tdir = opendir(path);\n \tif (!dir) {\n \t\tif (errno != ENOENT)\n \t\t\terror(\"unable to open object pack directory: %s: %s\",\n@@ -1093,14 +1120,6 @@ static int git_open_noatime(const char *name, struct packed_git *p)\n \t\tif (fd >= 0)\n \t\t\treturn fd;\n \n-\t\t/* Might the failure be insufficient file descriptors? */\n-\t\tif (errno == EMFILE) {\n-\t\t\tif (unuse_one_window(p, -1))\n-\t\t\t\tcontinue;\n-\t\t\telse\n-\t\t\t\treturn -1;\n-\t\t}\n-\n \t\t/* Might the failure be due to O_NOATIME? */\n \t\tif (errno != ENOENT && sha1_file_open_flag) {\n \t\t\tsha1_file_open_flag = 0;\n@@ -2360,8 +2379,6 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \n \tfilename = sha1_file_name(sha1);\n \tfd = create_tmpfile(tmpfile, sizeof(tmpfile), filename);\n-\twhile (fd < 0 && errno == EMFILE && unuse_one_window(NULL, -1))\n-\t\tfd = create_tmpfile(tmpfile, sizeof(tmpfile), filename);\n \tif (fd < 0) {\n \t\tif (errno == EACCES)\n \t\t\treturn error(\"insufficient permission for adding an object to repository database %s\\n\", get_object_directory());\n-- \n1.7.4.1.249.g4aa7\n"},{"id":"162475","messageId":"AANLkTikpBSi9CDHBsThGyumJ0CLd2xP+wD18vr1NQr3J@mail.gmail.com","threadId":"26622","inReplyTo":"1298926359-26438-1-git-send-email-spearce@spearce.org","subject":"Re: [PATCH v2] Limit file descriptors used by packs","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-02-28T21:13:22Z","receivedAt":"2011-02-28T21:13:22Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Mon, Feb 28, 2011 at 9:52 PM, Shawn O. Pearce <spearce@spearce.org> wrote:\n> Rather than using 'errno == EMFILE' after a failed open() call\n> to indicate the process is out of file descriptors and an LRU\n> pack window should be closed, place a hard upper limit on the\n> number of open packs based on the actual rlimit of the process.\n>\n> By using a hard upper limit that is below the rlimit of the current\n> process it is not necessary to check for EMFILE on every single\n> fd-allocating system call.  Instead reserving 25 file descriptors\n> makes it safe to assume the system call won't fail due to being over\n> the filedescriptor limit.  Here 25 is chosen as a WAG, but considers\n> 3 for stdin/stdout/stderr, and at least a few for other Git code\n> to operate on temporary files.  An additional 20 is reserved as it\n> is not known what the C library needs to perform other services on\n> Git's behalf, such as nsswitch or name resolution.\n>\n> This fixes a case where running `git gc --auto` in a repository\n> with more than 1024 packs (but an rlimit of 1024 open fds) fails\n> due to the temporary output file not being able to allocate a\n> file descriptor.  The output file is opened by pack-objects after\n> object enumeration and delta compression are done, both of which\n> have already opened all of the packs and fully populated the file\n> descriptor table.\n>\n> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>\n> ---\n>  sha1_file.c |   43 ++++++++++++++++++++++++++++++-------------\n>  1 files changed, 30 insertions(+), 13 deletions(-)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index d949b35..7850c18 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -418,6 +418,8 @@ static unsigned int pack_used_ctr;\n>  static unsigned int pack_mmap_calls;\n>  static unsigned int peak_pack_open_windows;\n>  static unsigned int pack_open_windows;\n> +static unsigned int pack_open_fds;\n> +static unsigned int pack_max_fds;\n>  static size_t peak_pack_mapped;\n>  static size_t pack_mapped;\n>  struct packed_git *packed_git;\n> @@ -597,6 +599,7 @@ static int unuse_one_window(struct packed_git *current, int keep_fd)\n>                        lru_p->windows = lru_w->next;\n>                        if (!lru_p->windows && lru_p->pack_fd != keep_fd) {\n>                                close(lru_p->pack_fd);\n> +                               pack_open_fds--;\n>                                lru_p->pack_fd = -1;\n>                        }\n>                }\n> @@ -681,8 +684,10 @@ void free_pack_by_name(const char *pack_name)\n>                if (strcmp(pack_name, p->pack_name) == 0) {\n>                        clear_delta_base_cache();\n>                        close_pack_windows(p);\n> -                       if (p->pack_fd != -1)\n> +                       if (p->pack_fd != -1) {\n>                                close(p->pack_fd);\n> +                               pack_open_fds--;\n> +                       }\n>                        close_pack_index(p);\n>                        free(p->bad_object_sha1);\n>                        *pp = p->next;\n> @@ -708,9 +713,29 @@ static int open_packed_git_1(struct packed_git *p)\n>        if (!p->index_data && open_pack_index(p))\n>                return error(\"packfile %s index unavailable\", p->pack_name);\n>\n> +       if (!pack_max_fds) {\n> +               struct rlimit lim;\n> +               unsigned int max_fds;\n> +\n> +               if (getrlimit(RLIMIT_NOFILE, &lim))\n> +                       die_errno(\"cannot get RLIMIT_NOFILE\");\n> +\n\nWe don't have getrlimit on Windows :(\n\nI guess something like should work, but untested. Limit of 2048 taken from MSDN:\n\nhttp://msdn.microsoft.com/en-us/library/6e3b887c(v=vs.71).aspx\n\n---8<---\n\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex 9c00e75..9155ce3 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -234,6 +234,22 @@ int mingw_getpagesize(void);\n #define getpagesize mingw_getpagesize\n #endif\n\n+struct rlimit {\n+\tunsigned int rlim_cur;\n+};\n+#define RLIMIT_NOFILE 0\n+\n+static inline int getrlimit(int resource, struct rlimit *rlp)\n+{\n+\tif (resource != RLIMIT_NOFILE) {\n+\t\terrno = EINVAL;\n+\t\treturn -1;\n+\t}\n+\n+\trlp->rlim_cur = 2048;\n+\treturn 0;\n+}\n+\n /* Use mingw_lstat() instead of lstat()/stat() and\n  * mingw_fstat() instead of fstat() on Windows.\n  */\n\n---8<---\n"},{"id":"162550","messageId":"7vwrkiex62.fsf@alter.siamese.dyndns.org","threadId":"26622","inReplyTo":"20110228204727.GB26052@spearce.org","subject":"Re: [PATCH] Limit file descriptors used by packs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-01T14:24:21Z","receivedAt":"2011-03-01T14:24:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> writes:\n\n> In this particular part of C Git, if we are bumping up against the\n> hard pack_max_fds limit we're already into some pretty difficult\n> computation. Trying to push the rlimit higher in order to avoid\n> close/open calls as we cycle through fds isn't really going to make\n> a huge difference on end-user latency for the command to finish\n> its task. So maybe we are better off honoring the rlim_cur that we\n> inherited from the user/environment.\n\nLet's step back a bit.\n\nYou are holding too many file descriptors open because you have too many\npack-files in your repository.\n\nI am not going to question why they aren't repacked before their number\ngets too large---that is not a valid question to ask in the context of\nthis discussion.  But it is a valid question to ask why we need an open\nfile descriptor for each of them to begin with, and if we really need to,\nisn't it?\n\nWe keep one file descriptor open for each .pack in which we have pack\nwindow(s).  The reason we keep one file descriptor open is because we\nmight want to mmap() different portions of a .pack file that is already in\nuse through the file descriptor to open new window(s) on demand.\n\nFor a .pack that fits inside a single pack window, however, can't we close\nthe file descriptor immediately after mmap() it to obtain a sole window\ninto it?  For such a .pack, we would either have one window into it, or\nthe .pack is not in use and have no window into it.  When the number of\nwindows drops to zero, the current code closes the file descriptor, and\nupon next use, we already let the caller access the .pack correctly, so\nwe already should know when to re-open a file descriptor to it as needed.\n\nI am wondering if it is a viable approach to, inside open_packed_git_1(),\n\n - mark a packfile that is small enough (i.e. st.st_size aka p->pack_size\n   is smaller than packed_git_window_size) as \"persistently mapped\";\n\n - keep a window that covers the entire thing in p->windows as a single\n   and sole window into it for such a pack; and\n\n - close the file descriptor when we did the above\n\nand have use_pack() notice that single window and use it.\n\nThis is not an alternate proposal to your patch, but the approach may\nalleviate the resource pressure in the first place, no?\n"},{"id":"162553","messageId":"AANLkTik=FiwsQUg89MRXZX1-jR-fkF7uyJAimSXVSLvR@mail.gmail.com","threadId":"26622","inReplyTo":"7vwrkiex62.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Limit file descriptors used by packs","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-03-01T14:58:49Z","receivedAt":"2011-03-01T14:58:49Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Tue, Mar 1, 2011 at 06:24, Junio C Hamano <gitster@pobox.com> wrote:\n> \"Shawn O. Pearce\" <spearce@spearce.org> writes:\n>\n>> In this particular part of C Git, if we are bumping up against the\n>> hard pack_max_fds limit we're already into some pretty difficult\n>> computation. Trying to push the rlimit higher in order to avoid\n>> close/open calls as we cycle through fds isn't really going to make\n>> a huge difference on end-user latency for the command to finish\n>> its task. So maybe we are better off honoring the rlim_cur that we\n>> inherited from the user/environment.\n>\n> Let's step back a bit.\n...\n> For a .pack that fits inside a single pack window, however, can't we close\n> the file descriptor immediately after mmap() it to obtain a sole window\n> into it?\n\nYes. And its unrelated to this patch. You can still run out of file\ndescriptors because you have a lot of large packs. :-)\n\nI've considered this in the past, but avoided it because I thought the\nunuse_one_window() function might become more complex. But its not, we\ncan just keep popping windows until the condition is met, which for a\nfile descriptor is that we are below the limit.\n\nI'll send a follow-up patch that builds on top of this one to close\nthe pack fd if the entire thing fits into one window.\n\n-- \nShawn.\n"},{"id":"162657","messageId":"1299088914-3468-1-git-send-email-spearce@spearce.org","threadId":"26622","inReplyTo":"AANLkTik=FiwsQUg89MRXZX1-jR-fkF7uyJAimSXVSLvR@mail.gmail.com","subject":"[PATCH 2/1] sha1_file.c: Don't retain open fds on small packs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-03-02T18:01:54Z","receivedAt":"2011-03-02T18:01:54Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"If a pack file is small enough that its entire contents fits within\none mmap window, mmap the file and then immediately close its file\ndescriptor.  This reduces the number of file descriptors that are\nneeded to read from repositories with many tiny pack files, such\nas one that has received 1000 pushes (and created 1000 small pack\nfiles) since its last repack.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n cache.h       |    3 ++-\n fast-import.c |    1 +\n sha1_file.c   |   41 ++++++++++++++++++++++++++++++++++++-----\n 3 files changed, 39 insertions(+), 6 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 08a9022..1d362c4 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -914,7 +914,8 @@ extern struct packed_git {\n \ttime_t mtime;\n \tint pack_fd;\n \tunsigned pack_local:1,\n-\t\t pack_keep:1;\n+\t\t pack_keep:1,\n+\t\t do_not_close:1;\n \tunsigned char sha1[20];\n \t/* something like \".git/objects/pack/xxxxx.pack\" */\n \tchar pack_name[FLEX_ARRAY]; /* more */\ndiff --git a/fast-import.c b/fast-import.c\nindex 3886a1b..4916a9d 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -872,6 +872,7 @@ static void start_packfile(void)\n \tp = xcalloc(1, sizeof(*p) + strlen(tmpfile) + 2);\n \tstrcpy(p->pack_name, tmpfile);\n \tp->pack_fd = pack_fd;\n+\tp->do_not_close = 1;\n \tpack_file = sha1fd(pack_fd, p->pack_name);\n \n \thdr.hdr_signature = htonl(PACK_SIGNATURE);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 7850c18..fbb178a 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -597,7 +597,8 @@ static int unuse_one_window(struct packed_git *current, int keep_fd)\n \t\t\tlru_l->next = lru_w->next;\n \t\telse {\n \t\t\tlru_p->windows = lru_w->next;\n-\t\t\tif (!lru_p->windows && lru_p->pack_fd != keep_fd) {\n+\t\t\tif (!lru_p->windows && lru_p->pack_fd != -1\n+\t\t\t\t&& lru_p->pack_fd != keep_fd) {\n \t\t\t\tclose(lru_p->pack_fd);\n \t\t\t\tpack_open_fds--;\n \t\t\t\tlru_p->pack_fd = -1;\n@@ -813,14 +814,13 @@ unsigned char *use_pack(struct packed_git *p,\n {\n \tstruct pack_window *win = *w_cursor;\n \n-\tif (p->pack_fd == -1 && open_packed_git(p))\n-\t\tdie(\"packfile %s cannot be accessed\", p->pack_name);\n-\n \t/* Since packfiles end in a hash of their content and it's\n \t * pointless to ask for an offset into the middle of that\n \t * hash, and the in_window function above wouldn't match\n \t * don't allow an offset too close to the end of the file.\n \t */\n+\tif (!p->pack_size && p->pack_fd == -1 && open_packed_git(p))\n+\t\tdie(\"packfile %s cannot be accessed\", p->pack_name);\n \tif (offset > (p->pack_size - 20))\n \t\tdie(\"offset beyond end of packfile (truncated pack?)\");\n \n@@ -834,6 +834,10 @@ unsigned char *use_pack(struct packed_git *p,\n \t\tif (!win) {\n \t\t\tsize_t window_align = packed_git_window_size / 2;\n \t\t\toff_t len;\n+\n+\t\t\tif (p->pack_fd == -1 && open_packed_git(p))\n+\t\t\t\tdie(\"packfile %s cannot be accessed\", p->pack_name);\n+\n \t\t\twin = xcalloc(1, sizeof(*win));\n \t\t\twin->offset = (offset / window_align) * window_align;\n \t\t\tlen = p->pack_size - win->offset;\n@@ -851,6 +855,12 @@ unsigned char *use_pack(struct packed_git *p,\n \t\t\t\tdie(\"packfile %s cannot be mapped: %s\",\n \t\t\t\t\tp->pack_name,\n \t\t\t\t\tstrerror(errno));\n+\t\t\tif (!win->offset && win->len == p->pack_size\n+\t\t\t\t&& !p->do_not_close) {\n+\t\t\t\tclose(p->pack_fd);\n+\t\t\t\tpack_open_fds--;\n+\t\t\t\tp->pack_fd = -1;\n+\t\t\t}\n \t\t\tpack_mmap_calls++;\n \t\t\tpack_open_windows++;\n \t\t\tif (pack_mapped > peak_pack_mapped)\n@@ -1951,6 +1961,27 @@ off_t find_pack_entry_one(const unsigned char *sha1,\n \treturn 0;\n }\n \n+static int is_pack_valid(struct packed_git *p)\n+{\n+\t/* An already open pack is known to be valid. */\n+\tif (p->pack_fd != -1)\n+\t\treturn 1;\n+\n+\t/* If the pack has one window completely covering the\n+\t * file size, the pack is known to be valid even if\n+\t * the descriptor is not currently open.\n+\t */\n+\tif (p->windows) {\n+\t\tstruct pack_window *w = p->windows;\n+\n+\t\tif (!w->offset && w->len == p->pack_size)\n+\t\t\treturn 1;\n+\t}\n+\n+\t/* Force the pack to open to prove its valid. */\n+\treturn !open_packed_git(p);\n+}\n+\n static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n {\n \tstatic struct packed_git *last_found = (void *)1;\n@@ -1980,7 +2011,7 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n \t\t\t * it may have been deleted since the index\n \t\t\t * was loaded!\n \t\t\t */\n-\t\t\tif (p->pack_fd == -1 && open_packed_git(p)) {\n+\t\t\tif (!is_pack_valid(p)) {\n \t\t\t\terror(\"packfile %s cannot be accessed\", p->pack_name);\n \t\t\t\tgoto next;\n \t\t\t}\n-- \n1.7.4.1.249.g4aa7\n"}]}