{"thread":{"id":"34566","subject":"[PATCH] sha1_file: introduce close_one_pack() to close packs on fd pressure","startedAt":"2013-07-30T04:05:13Z","lastAt":"2013-08-02T17:12:08Z","messageCount":23,"participants":["Brandon Casey","Eric Sunshine","Junio C Hamano","Jeff King","Antoine Pelisse","Fredrik Gustafsson","Thomas Rast"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"224280","messageId":"1375157113-608-1-git-send-email-bcasey@nvidia.com","threadId":"34566","inReplyTo":null,"subject":"[PATCH] sha1_file: introduce close_one_pack() to close packs on fd pressure","fromName":"Brandon Casey","fromEmail":"bcasey@nvidia.com","sentAt":"2013-07-30T04:05:13Z","receivedAt":"2013-07-30T04:05:13Z","isPatch":true,"sender":{"key":"bcasey@nvidia.com","avatar":null},"body":"From: Brandon Casey \n\nWhen the number of open packs exceeds pack_max_fds, unuse_one_window()\nis called repeatedly to attempt to release the least-recently-used\npack windows, which, as a side-effect, will also close a pack file\nafter closing its last open window. If a pack file has been opened,\nbut no windows have been allocated into it, it will never be selected\nby unuse_one_window() and hence its file descriptor will not be\nclosed. When this happens, git may exceed the number of file\ndescriptors permitted by the system.\n\nThis latter situation can occur in show-ref or receive-pack during ref\nadvertisement. During ref advertisement, receive-pack will iterate\nover every ref in the repository and advertise it to the client after\nensuring that the ref exists in the local repository. If the ref is\nlocated inside a pack, then the pack is opened to ensure that it\nexists, but since the object is not actually read from the pack, no\nmmap windows are allocated. When the number of open packs exceeds\npack_max_fds, unuse_one_window() will not able to find any windows to\nfree and will not be able to close any packs. Once the per-process\nfile descriptor limit is exceeded, receive-pack will produce a warning,\nnot an error, for each pack it cannot open, and will then most likely\nfail with an error to spawn rev-list or index-pack like:\n\n error: cannot create standard input pipe for rev-list: Too many open files\n error: Could not run 'git rev-list'\n\nThis is not likely to occur during upload-pack since upload-pack\nreads each object from the pack so that it can peel tags and\nadvertise the exposed object. So during upload-pack, mmap windows\nwill be allocated for each pack that is opened and unuse_one_window()\nwill eventually be able to close unused packs after freeing all of\ntheir windows.\n\nWhen we have file descriptor pressure, in contrast to memory pressure,\nwe need to free all windows and close the pack file descriptor so that\na new pack can be opened. Let's introduce a new function\nclose_one_pack() designed specifically for this purpose to search\nfor and close the least-recently-used pack, where LRU is defined as\n\n * pack with oldest mtime and no allocated mmap windows or\n * pack with the least-recently-used windows, i.e. the pack\n with the oldest most-recently-used window\n\nSigned-off-by: Brandon Casey \n---\n sha1_file.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 62 insertions(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 8e27db1..7731ab1 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -682,6 +682,67 @@ void close_pack_windows(struct packed_git *p)\n \t}\n }\n \n+/*\n+ * The LRU pack is the one with the oldest MRU window or the oldest mtime\n+ * if it has no windows allocated.\n+ */\n+static void find_lru_pack(struct packed_git *p, struct packed_git **lru_p, struct pack_window **mru_w)\n+{\n+\tstruct pack_window *w, *this_mru_w;\n+\n+\t/*\n+\t * Reject this pack if it has windows and the previously selected\n+\t * one does not. If this pack does not have windows, reject\n+\t * it if the pack file is newer than the previously selected one.\n+\t */\n+\tif (*lru_p && !*mru_w && (p->windows || p->mtime > (*lru_p)->mtime))\n+\t\treturn;\n+\n+\tfor (w = this_mru_w = p->windows; w; w = w->next) {\n+\t\t/* Reject this pack if any of its windows are in use */\n+\t\tif (w->inuse_cnt)\n+\t\t\treturn;\n+\t\t/*\n+\t\t * Reject this pack if it has windows that have been\n+\t\t * used more recently than the previously selected pack.\n+\t\t */\n+\t\tif (*mru_w && w->last_used > (*mru_w)->last_used)\n+\t\t\treturn;\n+\t\tif (w->last_used > this_mru_w->last_used)\n+\t\t\tthis_mru_w = w;\n+\t}\n+\n+\t/*\n+\t * Select this pack.\n+\t */\n+\t*mru_w = this_mru_w;\n+\t*lru_p = p;\n+}\n+\n+static int close_one_pack(void)\n+{\n+\tstruct packed_git *p, *lru_p = NULL;\n+\tstruct pack_window *mru_w = NULL;\n+\n+\tfor (p = packed_git; p; p = p->next) {\n+\t\tif (p->pack_fd == -1)\n+\t\t\tcontinue;\n+\t\tfind_lru_pack(p, &lru_p, &mru_w);\n+\t}\n+\n+\tif (lru_p) {\n+\t\tclose_pack_windows(lru_p);\n+\t\tclose(lru_p->pack_fd);\n+\t\tpack_open_fds--;\n+\t\tlru_p->pack_fd = -1;\n+\t\tif (lru_p == last_found_pack)\n+\t\t\tlast_found_pack = NULL;\n+\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n void unuse_pack(struct pack_window **w_cursor)\n {\n \tstruct pack_window *w = *w_cursor;\n@@ -777,7 +838,7 @@ static int open_packed_git_1(struct packed_git *p)\n \t\t\tpack_max_fds = 1;\n \t}\n \n-\twhile (pack_max_fds <= pack_open_fds && unuse_one_window(NULL, -1))\n+\twhile (pack_max_fds <= pack_open_fds && close_one_pack())\n \t\t; /* nothing */\n \n \tp->pack_fd = git_open_noatime(p->pack_name);\n-- \n1.8.3.1.440.gc2bf105\n\n\n-----------------------------------------------------------------------------------\nThis email message is for the sole use of the intended recipient(s) and may contain\nconfidential information. Any unauthorized review, use, disclosure or distribution\nis prohibited. If you are not the intended recipient, please contact the sender by\nreply email and destroy all copies of the original message.\n-----------------------------------------------------------------------------------\n"},{"id":"224283","messageId":"CAPig+cR68PQ62JC266iGBidD-udBq2BaL-BWaTpY1JkWWBOp0Q@mail.gmail.com","threadId":"34566","inReplyTo":"1375157113-608-1-git-send-email-bcasey@nvidia.com","subject":"Re: [PATCH] sha1_file: introduce close_one_pack() to close packs on fd pressure","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-07-30T07:51:59Z","receivedAt":"2013-07-30T07:51:59Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 30, 2013 at 12:05 AM, Brandon Casey wrote:\n> When the number of open packs exceeds pack_max_fds, unuse_one_window()\n> is called repeatedly to attempt to release the least-recently-used\n> pack windows, which, as a side-effect, will also close a pack file\n> after closing its last open window. If a pack file has been opened,\n> but no windows have been allocated into it, it will never be selected\n> by unuse_one_window() and hence its file descriptor will not be\n> closed. When this happens, git may exceed the number of file\n> descriptors permitted by the system.\n>\n> This latter situation can occur in show-ref or receive-pack during ref\n> advertisement. During ref advertisement, receive-pack will iterate\n> over every ref in the repository and advertise it to the client after\n> ensuring that the ref exists in the local repository. If the ref is\n> located inside a pack, then the pack is opened to ensure that it\n> exists, but since the object is not actually read from the pack, no\n> mmap windows are allocated. When the number of open packs exceeds\n> pack_max_fds, unuse_one_window() will not able to find any windows to\n\ns/not able/not be able/\n...or...\ns/not able to find/not find/\n\n> free and will not be able to close any packs. Once the per-process\n> file descriptor limit is exceeded, receive-pack will produce a warning,\n> not an error, for each pack it cannot open, and will then most likely\n> fail with an error to spawn rev-list or index-pack like:\n>\n> error: cannot create standard input pipe for rev-list: Too many open files\n> error: Could not run 'git rev-list'\n>\n> This is not likely to occur during upload-pack since upload-pack\n> reads each object from the pack so that it can peel tags and\n> advertise the exposed object. So during upload-pack, mmap windows\n> will be allocated for each pack that is opened and unuse_one_window()\n> will eventually be able to close unused packs after freeing all of\n> their windows.\n>\n> When we have file descriptor pressure, in contrast to memory pressure,\n> we need to free all windows and close the pack file descriptor so that\n> a new pack can be opened. Let's introduce a new function\n> close_one_pack() designed specifically for this purpose to search\n> for and close the least-recently-used pack, where LRU is defined as\n>\n> * pack with oldest mtime and no allocated mmap windows or\n> * pack with the least-recently-used windows, i.e. the pack\n> with the oldest most-recently-used window\n>\n> Signed-off-by: Brandon Casey \n"},{"id":"224301","messageId":"7v61vsxdiz.fsf@alter.siamese.dyndns.org","threadId":"34566","inReplyTo":"1375157113-608-1-git-send-email-bcasey@nvidia.com","subject":"Re: [PATCH] sha1_file: introduce close_one_pack() to close packs on fd pressure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-30T15:39:48Z","receivedAt":"2013-07-30T15:39:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> From: Brandon Casey \n>\n> When the number of open packs exceeds pack_max_fds, unuse_one_window()\n> is called repeatedly to attempt to release the least-recently-used\n> pack windows, which, as a side-effect, will also close a pack file\n> after closing its last open window. If a pack file has been opened,\n> but no windows have been allocated into it, it will never be selected\n> by unuse_one_window() and hence its file descriptor will not be\n> closed. When this happens, git may exceed the number of file\n> descriptors permitted by the system.\n\nAn interesting find. The patch from a cursory look reads OK.\n\nThanks.\n\n> This is not likely to occur during upload-pack since upload-pack\n> reads each object from the pack so that it can peel tags and\n> advertise the exposed object.\n\nAnother interesting find. Perhaps there is a room for improvements,\nas packed-refs file knows what objects the tags peel to? I vaguely\nrecall Peff was actively reducing the object access during ref\nenumeration in not so distant past...\n"},{"id":"224320","messageId":"20130730195257.GA16247@sigill.intra.peff.net","threadId":"34566","inReplyTo":"7v61vsxdiz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] sha1_file: introduce close_one_pack() to close packs on fd pressure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-30T19:52:57Z","receivedAt":"2013-07-30T19:52:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 30, 2013 at 08:39:48AM -0700, Junio C Hamano wrote:\n\n> Brandon Casey writes:\n> \n> > From: Brandon Casey \n> >\n> > When the number of open packs exceeds pack_max_fds, unuse_one_window()\n> > is called repeatedly to attempt to release the least-recently-used\n> > pack windows, which, as a side-effect, will also close a pack file\n> > after closing its last open window. If a pack file has been opened,\n> > but no windows have been allocated into it, it will never be selected\n> > by unuse_one_window() and hence its file descriptor will not be\n> > closed. When this happens, git may exceed the number of file\n> > descriptors permitted by the system.\n> \n> An interesting find. The patch from a cursory look reads OK.\n\nYeah. I wonder if unuse_one_window() should actually leave the pack fd\nopen now in general.\n\nIf you close packfile descriptors, you can run into racy situations\nwhere somebody else is repacking and deleting packs, and they go away\nwhile you are trying to access them. If you keep a descriptor open,\nyou're fine; they last to the end of the process. If you don't, then\nthey disappear from under you.\n\nFor normal object access, this isn't that big a deal; we just rescan the\npacks and retry. But if you are packing yourself (e.g., because you are\na pack-objects started by upload-pack for a clone or fetch), it's much\nharder to recover (and we print some warnings).\n\nWe had our core.packedGitWindowSize lowered on GitHub for a while, and\nwe ran into this warning on busy repositories when we were running \"git\ngc\" on the server. We solved it by bumping the window size so we never\nrelease memory.\n\nBut just not closing the descriptor wouldn't work until Brandon's patch,\nbecause we used the same function to release memory and descriptor\npressure. Now we could do them separately (and progressively if we need\nto).\n\n> > This is not likely to occur during upload-pack since upload-pack\n> > reads each object from the pack so that it can peel tags and\n> > advertise the exposed object.\n> \n> Another interesting find. Perhaps there is a room for improvements,\n> as packed-refs file knows what objects the tags peel to? I vaguely\n> recall Peff was actively reducing the object access during ref\n> enumeration in not so distant past...\n\nYeah, we should be reading almost no objects these days due to the\npacked-refs peel lines. I just did a double-check on what \"git\nupload-pack . /dev/null\" reads on my git.git repo, and it is\nonly three objects: the v1.8.3.3, v1.8.3.4, and v1.8.4-rc0 tag objects.\nIn other words, the tags I got since the last time I ran \"git gc\". So I\nthink all is working as designed.\n\nWe could give receive-pack the same treatment; I've spent less time\nmicro-optimizing it because because we (and most sites, I would think)\nget an order of magnitude more fetches than pushes.\n\n-Peff\n"},{"id":"224328","messageId":"CA+sFfMe1GTDqtgGs3NXoB0OBYTtyHxLDYgy0TmOe+3r=tMXS0A@mail.gmail.com","threadId":"34566","inReplyTo":"20130730195257.GA16247@sigill.intra.peff.net","subject":"Re: [PATCH] sha1_file: introduce close_one_pack() to close packs on fd pressure","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-07-30T22:59:54Z","receivedAt":"2013-07-30T22:59:54Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Tue, Jul 30, 2013 at 12:52 PM, Jeff King wrote:\n> On Tue, Jul 30, 2013 at 08:39:48AM -0700, Junio C Hamano wrote:\n>\n>> Brandon Casey writes:\n>>\n>> > From: Brandon Casey \n>> >\n>> > When the number of open packs exceeds pack_max_fds, unuse_one_window()\n>> > is called repeatedly to attempt to release the least-recently-used\n>> > pack windows, which, as a side-effect, will also close a pack file\n>> > after closing its last open window. If a pack file has been opened,\n>> > but no windows have been allocated into it, it will never be selected\n>> > by unuse_one_window() and hence its file descriptor will not be\n>> > closed. When this happens, git may exceed the number of file\n>> > descriptors permitted by the system.\n>>\n>> An interesting find. The patch from a cursory look reads OK.\n>\n> Yeah. I wonder if unuse_one_window() should actually leave the pack fd\n> open now in general.\n>\n> If you close packfile descriptors, you can run into racy situations\n> where somebody else is repacking and deleting packs, and they go away\n> while you are trying to access them. If you keep a descriptor open,\n> you're fine; they last to the end of the process. If you don't, then\n> they disappear from under you.\n>\n> For normal object access, this isn't that big a deal; we just rescan the\n> packs and retry. But if you are packing yourself (e.g., because you are\n> a pack-objects started by upload-pack for a clone or fetch), it's much\n> harder to recover (and we print some warnings).\n>\n> We had our core.packedGitWindowSize lowered on GitHub for a while, and\n> we ran into this warning on busy repositories when we were running \"git\n> gc\" on the server. We solved it by bumping the window size so we never\n> release memory.\n>\n> But just not closing the descriptor wouldn't work until Brandon's patch,\n> because we used the same function to release memory and descriptor\n> pressure. Now we could do them separately (and progressively if we need\n> to).\n\nI had thought about whether to stop closing the pack file in\nunuse_one_window(), but didn't have a reason to do so. I think the\nscenario you described provides a justification. If we're not under\nfile descriptor pressure and we can possibly avoid rescanning the pack\ndirectory, it sounds like a net win.\n\n>> > This is not likely to occur during upload-pack since upload-pack\n>> > reads each object from the pack so that it can peel tags and\n>> > advertise the exposed object.\n>>\n>> Another interesting find. Perhaps there is a room for improvements,\n>> as packed-refs file knows what objects the tags peel to? I vaguely\n>> recall Peff was actively reducing the object access during ref\n>> enumeration in not so distant past...\n>\n> Yeah, we should be reading almost no objects these days due to the\n> packed-refs peel lines. I just did a double-check on what \"git\n> upload-pack . /dev/null\" reads on my git.git repo, and it is\n> only three objects: the v1.8.3.3, v1.8.3.4, and v1.8.4-rc0 tag objects.\n> In other words, the tags I got since the last time I ran \"git gc\". So I\n> think all is working as designed.\n\nOk, looks like this has been the case since your 435c8332 which taught\nupload-pack to use peel_ref(). So looks like we do avoid reaching\ninto the pack for any ref that was read from a (modern) packed-refs\nfile. The repository I was testing with had mostly loose refs.\nIndeed, after packing refs, upload-pack encounters the same problem as\nreceive-pack and runs out of file descriptors.\n\nSo my comment about upload-pack is not completely accurate.\nUpload-pack _can_ run into this problem, but the refs must be packed,\nas well as there being enough of them that exist in enough different\npack files to exceed the processes fd limit.\n\n> We could give receive-pack the same treatment; I've spent less time\n> micro-optimizing it because because we (and most sites, I would think)\n> get an order of magnitude more fetches than pushes.\n\nI don't think it would need the 435c8332 treatment since receive-pack\ndoesn't peel refs when it advertises them to the client and hence does\nnot need to load the ref object from the pack file during ref\nadvertisement, but possibly some of the other stuff you did would be\napplicable. But like you said, the number of fetches far exceed the\nnumber of pushes.\n\n-Brandon\n"},{"id":"224382","messageId":"1375300297-6744-1-git-send-email-bcasey@nvidia.com","threadId":"34566","inReplyTo":"CA+sFfMe1GTDqtgGs3NXoB0OBYTtyHxLDYgy0TmOe+3r=tMXS0A@mail.gmail.com","subject":"[PATCH v2 1/2] sha1_file: introduce close_one_pack() to close packs on fd pressure","fromName":"Brandon Casey","fromEmail":"bcasey@nvidia.com","sentAt":"2013-07-31T19:51:36Z","receivedAt":"2013-07-31T19:51:36Z","isPatch":true,"sender":{"key":"bcasey@nvidia.com","avatar":null},"body":"From: Brandon Casey \n\nWhen the number of open packs exceeds pack_max_fds, unuse_one_window()\nis called repeatedly to attempt to release the least-recently-used\npack windows, which, as a side-effect, will also close a pack file\nafter closing its last open window. If a pack file has been opened,\nbut no windows have been allocated into it, it will never be selected\nby unuse_one_window() and hence its file descriptor will not be\nclosed. When this happens, git may exceed the number of file\ndescriptors permitted by the system.\n\nThis latter situation can occur in show-ref or receive-pack during ref\nadvertisement. During ref advertisement, receive-pack will iterate\nover every ref in the repository and advertise it to the client after\nensuring that the ref exists in the local repository. If the ref is\nlocated inside a pack, then the pack is opened to ensure that it\nexists, but since the object is not actually read from the pack, no\nmmap windows are allocated. When the number of open packs exceeds\npack_max_fds, unuse_one_window() will not be able to find any windows to\nfree and will not be able to close any packs. Once the per-process\nfile descriptor limit is exceeded, receive-pack will produce a warning,\nnot an error, for each pack it cannot open, and will then most likely\nfail with an error to spawn rev-list or index-pack like:\n\n error: cannot create standard input pipe for rev-list: Too many open files\n error: Could not run 'git rev-list'\n\nThis may also occur during upload-pack when refs are packed (in the\npacked-refs file) and the number of packs that must be opened to\nverify that these packed refs exist exceeds the file descriptor limit.\nIf the refs are loose, then upload-pack will read each ref from the\npack (allocating one or more mmap windows) so it can peel tags and\nadvertise the underlying object. If the refs are packed and peeled,\nthen upload-pack will use the peeled sha1 in the packed-refs file and\nwill not need to read from the pack files, so no mmap windows will be\nallocated and just like with receive-pack, unuse_one_window() will\nnever select these opened packs to close.\n\nWhen we have file descriptor pressure, in contrast to memory pressure,\nwe need to free all windows and close the pack file descriptor so that\na new pack can be opened. Let's introduce a new function\nclose_one_pack() designed specifically for this purpose to search\nfor and close the least-recently-used pack, where LRU is defined as\n\n * pack with oldest mtime and no allocated mmap windows or\n * pack with the least-recently-used windows, i.e. the pack\n with the oldest most-recently-used window\n\nSigned-off-by: Brandon Casey \n---\n\nThe commit message was updated to fix the grammatical error that Eric\nSunshine pointed out, and to correct the paragraph about upload-pack.\n\n-Brandon\n\n sha1_file.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 62 insertions(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 8e27db1..7731ab1 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -682,6 +682,67 @@ void close_pack_windows(struct packed_git *p)\n \t}\n }\n \n+/*\n+ * The LRU pack is the one with the oldest MRU window or the oldest mtime\n+ * if it has no windows allocated.\n+ */\n+static void find_lru_pack(struct packed_git *p, struct packed_git **lru_p, struct pack_window **mru_w)\n+{\n+\tstruct pack_window *w, *this_mru_w;\n+\n+\t/*\n+\t * Reject this pack if it has windows and the previously selected\n+\t * one does not. If this pack does not have windows, reject\n+\t * it if the pack file is newer than the previously selected one.\n+\t */\n+\tif (*lru_p && !*mru_w && (p->windows || p->mtime > (*lru_p)->mtime))\n+\t\treturn;\n+\n+\tfor (w = this_mru_w = p->windows; w; w = w->next) {\n+\t\t/* Reject this pack if any of its windows are in use */\n+\t\tif (w->inuse_cnt)\n+\t\t\treturn;\n+\t\t/*\n+\t\t * Reject this pack if it has windows that have been\n+\t\t * used more recently than the previously selected pack.\n+\t\t */\n+\t\tif (*mru_w && w->last_used > (*mru_w)->last_used)\n+\t\t\treturn;\n+\t\tif (w->last_used > this_mru_w->last_used)\n+\t\t\tthis_mru_w = w;\n+\t}\n+\n+\t/*\n+\t * Select this pack.\n+\t */\n+\t*mru_w = this_mru_w;\n+\t*lru_p = p;\n+}\n+\n+static int close_one_pack(void)\n+{\n+\tstruct packed_git *p, *lru_p = NULL;\n+\tstruct pack_window *mru_w = NULL;\n+\n+\tfor (p = packed_git; p; p = p->next) {\n+\t\tif (p->pack_fd == -1)\n+\t\t\tcontinue;\n+\t\tfind_lru_pack(p, &lru_p, &mru_w);\n+\t}\n+\n+\tif (lru_p) {\n+\t\tclose_pack_windows(lru_p);\n+\t\tclose(lru_p->pack_fd);\n+\t\tpack_open_fds--;\n+\t\tlru_p->pack_fd = -1;\n+\t\tif (lru_p == last_found_pack)\n+\t\t\tlast_found_pack = NULL;\n+\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n void unuse_pack(struct pack_window **w_cursor)\n {\n \tstruct pack_window *w = *w_cursor;\n@@ -777,7 +838,7 @@ static int open_packed_git_1(struct packed_git *p)\n \t\t\tpack_max_fds = 1;\n \t}\n \n-\twhile (pack_max_fds <= pack_open_fds && unuse_one_window(NULL, -1))\n+\twhile (pack_max_fds <= pack_open_fds && close_one_pack())\n \t\t; /* nothing */\n \n \tp->pack_fd = git_open_noatime(p->pack_name);\n-- \n1.8.4.rc0.2.g6cf5c31\n\n\n-----------------------------------------------------------------------------------\nThis email message is for the sole use of the intended recipient(s) and may contain\nconfidential information. Any unauthorized review, use, disclosure or distribution\nis prohibited. If you are not the intended recipient, please contact the sender by\nreply email and destroy all copies of the original message.\n-----------------------------------------------------------------------------------\n"},{"id":"224383","messageId":"1375300297-6744-2-git-send-email-bcasey@nvidia.com","threadId":"34566","inReplyTo":"1375300297-6744-1-git-send-email-bcasey@nvidia.com","subject":"[PATCH 2/2] Don't close pack fd when free'ing pack windows","fromName":"Brandon Casey","fromEmail":"bcasey@nvidia.com","sentAt":"2013-07-31T19:51:37Z","receivedAt":"2013-07-31T19:51:37Z","isPatch":true,"sender":{"key":"bcasey@nvidia.com","avatar":null},"body":"From: Brandon Casey \n\nNow that close_one_pack() has been introduced to handle file\ndescriptor pressure, it is not strictly necessary to close the\npack file descriptor in unuse_one_window() when we're under memory\npressure.\n\nJeff King provided a justification for leaving the pack file open:\n\n If you close packfile descriptors, you can run into racy situations\n where somebody else is repacking and deleting packs, and they go away\n while you are trying to access them. If you keep a descriptor open,\n you're fine; they last to the end of the process. If you don't, then\n they disappear from under you.\n\n For normal object access, this isn't that big a deal; we just rescan\n the packs and retry. But if you are packing yourself (e.g., because\n you are a pack-objects started by upload-pack for a clone or fetch),\n it's much harder to recover (and we print some warnings).\n\nLet's do so (or uh, not do so).\n\nSigned-off-by: Brandon Casey \n---\n builtin/pack-objects.c | 2 +-\n git-compat-util.h | 2 +-\n sha1_file.c | 21 +++++++--------------\n 3 files changed, 9 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex f069462..4eb0521 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1809,7 +1809,7 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,\n static void try_to_free_from_threads(size_t size)\n {\n \tread_lock();\n-\trelease_pack_memory(size, -1);\n+\trelease_pack_memory(size);\n \tread_unlock();\n }\n \ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex cc4ba4d..29b1ee3 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -517,7 +517,7 @@ int inet_pton(int af, const char *src, void *dst);\n const char *inet_ntop(int af, const void *src, char *dst, size_t size);\n #endif\n \n-extern void release_pack_memory(size_t, int);\n+extern void release_pack_memory(size_t);\n \n typedef void (*try_to_free_t)(size_t);\n extern try_to_free_t set_try_to_free_routine(try_to_free_t);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 7731ab1..d26121a 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -614,7 +614,7 @@ static void scan_windows(struct packed_git *p,\n \t}\n }\n \n-static int unuse_one_window(struct packed_git *current, int keep_fd)\n+static int unuse_one_window(struct packed_git *current)\n {\n \tstruct packed_git *p, *lru_p = NULL;\n \tstruct pack_window *lru_w = NULL, *lru_l = NULL;\n@@ -628,15 +628,8 @@ static int unuse_one_window(struct packed_git *current, int keep_fd)\n \t\tpack_mapped -= lru_w->len;\n \t\tif (lru_l)\n \t\t\tlru_l->next = lru_w->next;\n-\t\telse {\n+\t\telse\n \t\t\tlru_p->windows = lru_w->next;\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-\t\t\t}\n-\t\t}\n \t\tfree(lru_w);\n \t\tpack_open_windows--;\n \t\treturn 1;\n@@ -644,10 +637,10 @@ static int unuse_one_window(struct packed_git *current, int keep_fd)\n \treturn 0;\n }\n \n-void release_pack_memory(size_t need, int fd)\n+void release_pack_memory(size_t need)\n {\n \tsize_t cur = pack_mapped;\n-\twhile (need >= (cur - pack_mapped) && unuse_one_window(NULL, fd))\n+\twhile (need >= (cur - pack_mapped) && unuse_one_window(NULL))\n \t\t; /* nothing */\n }\n \n@@ -658,7 +651,7 @@ void *xmmap(void *start, size_t length,\n \tif (ret == MAP_FAILED) {\n \t\tif (!length)\n \t\t\treturn NULL;\n-\t\trelease_pack_memory(length, fd);\n+\t\trelease_pack_memory(length);\n \t\tret = mmap(start, length, prot, flags, fd, offset);\n \t\tif (ret == MAP_FAILED)\n \t\t\tdie_errno(\"Out of memory? mmap failed\");\n@@ -954,7 +947,7 @@ unsigned char *use_pack(struct packed_git *p,\n \t\t\twin->len = (size_t)len;\n \t\t\tpack_mapped += win->len;\n \t\t\twhile (packed_git_limit < pack_mapped\n-\t\t\t\t&& unuse_one_window(p, p->pack_fd))\n+\t\t\t\t&& unuse_one_window(p))\n \t\t\t\t; /* nothing */\n \t\t\twin->base = xmmap(NULL, win->len,\n \t\t\t\tPROT_READ, MAP_PRIVATE,\n@@ -1000,7 +993,7 @@ static struct packed_git *alloc_packed_git(int extra)\n \n static void try_to_free_pack_memory(size_t size)\n {\n-\trelease_pack_memory(size, -1);\n+\trelease_pack_memory(size);\n }\n \n struct packed_git *add_packed_git(const char *path, int path_len, int local)\n-- \n1.8.4.rc0.2.g6cf5c31\n\n\n-----------------------------------------------------------------------------------\nThis email message is for the sole use of the intended recipient(s) and may contain\nconfidential information. Any unauthorized review, use, disclosure or distribution\nis prohibited. If you are not the intended recipient, please contact the sender by\nreply email and destroy all copies of the original message.\n-----------------------------------------------------------------------------------\n"},{"id":"224393","messageId":"CALWbr2wR2cN8dcOtW2bV3p7FC3ymdXgfp61A4pNKvOWhP6WU_Q@mail.gmail.com","threadId":"34566","inReplyTo":"1375300297-6744-2-git-send-email-bcasey@nvidia.com","subject":"Re: [PATCH 2/2] Don't close pack fd when free'ing pack windows","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-07-31T21:08:21Z","receivedAt":"2013-07-31T21:08:21Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"On Wed, Jul 31, 2013 at 9:51 PM, Brandon Casey wrote:\n> -----------------------------------------------------------------------------------\n> This email message is for the sole use of the intended recipient(s) and may contain\n> confidential information. Any unauthorized review, use, disclosure or distribution\n> is prohibited. If you are not the intended recipient, please contact the sender by\n> reply email and destroy all copies of the original message.\n> -----------------------------------------------------------------------------------\n\nI'm certainly not a lawyer, and I'm sorry for not reviewing the\ncontent of the patch instead, but is that not a problem from a legal\npoint of view ?\nI remember a video of Greg Kroah-Hartman where he talked about that\n(the video was posted by Junio on G+).\n"},{"id":"224394","messageId":"20130731212114.GG19369@paksenarrion.iveqy.com","threadId":"34566","inReplyTo":"CALWbr2wR2cN8dcOtW2bV3p7FC3ymdXgfp61A4pNKvOWhP6WU_Q@mail.gmail.com","subject":"Re: [PATCH 2/2] Don't close pack fd when free'ing pack windows","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2013-07-31T21:21:14Z","receivedAt":"2013-07-31T21:21:14Z","isPatch":true,"sender":{"key":"iveqy@iveqy.com","avatar":"https://avatars.githubusercontent.com/u/761743?v=4"},"body":"On Wed, Jul 31, 2013 at 11:08:21PM +0200, Antoine Pelisse wrote:\n> On Wed, Jul 31, 2013 at 9:51 PM, Brandon Casey wrote:\n> > -----------------------------------------------------------------------------------\n> > This email message is for the sole use of the intended recipient(s) and may contain\n> > confidential information. Any unauthorized review, use, disclosure or distribution\n> > is prohibited. If you are not the intended recipient, please contact the sender by\n> > reply email and destroy all copies of the original message.\n> > -----------------------------------------------------------------------------------\n> \n> I'm certainly not a lawyer, and I'm sorry for not reviewing the\n> content of the patch instead, but is that not a problem from a legal\n> point of view ?\n\nTalking about legal, is it a problem if a commit isn't signed-off by\nit's committer or author e-mail? Like in this case where the sign-off is\nfrom gmail.com and the committer from nvidia.com?\n\n-- \nMed vänliga hälsningar\nFredrik Gustafsson\n\ntel: 0733-608274\ne-post: iveqy@iveqy.com\n"},{"id":"224395","messageId":"CA+sFfMe935imbvt=XvaU2jZhf=KSf0xZdnrBDgYfUop0CtyWrA@mail.gmail.com","threadId":"34566","inReplyTo":"CALWbr2wR2cN8dcOtW2bV3p7FC3ymdXgfp61A4pNKvOWhP6WU_Q@mail.gmail.com","subject":"Re: [PATCH 2/2] Don't close pack fd when free'ing pack windows","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-07-31T21:23:43Z","receivedAt":"2013-07-31T21:23:43Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Wed, Jul 31, 2013 at 2:08 PM, Antoine Pelisse wrote:\n> On Wed, Jul 31, 2013 at 9:51 PM, Brandon Casey wrote:\n>> -----------------------------------------------------------------------------------\n>> This email message is for the sole use of the intended recipient(s) and may contain\n>> confidential information. Any unauthorized review, use, disclosure or distribution\n>> is prohibited. If you are not the intended recipient, please contact the sender by\n>> reply email and destroy all copies of the original message.\n>> -----------------------------------------------------------------------------------\n>\n> I'm certainly not a lawyer, and I'm sorry for not reviewing the\n> content of the patch instead, but is that not a problem from a legal\n> point of view ?\n> I remember a video of Greg Kroah-Hartman where he talked about that\n> (the video was posted by Junio on G+).\n\nMe either thank God. Are those footers even enforceable? I mean,\nreally, if someone mistakenly sends me their corporate financial\nnumbers am I supposed to be under some legal obligation not to share\nit? I always assumed it was a scare tactic that lawyers like to use.\n\nTo address the text of the footer, I'd say the \"intended recipient(s)\"\nare those on the \"to\" line which includes git@vger.kernel.org and the\nimplicit use is for inclusion and distribution in the git source code.\n\nAnyway, I doubt I would have any influence on getting the footer\nremoved. If Junio would rather me not submit patches with that\nfooter, then I'd try to find a workaround.\n\n-Brandon\n"},{"id":"224396","messageId":"87r4eee7x3.fsf@hexa.v.cablecom.net","threadId":"34566","inReplyTo":"CALWbr2wR2cN8dcOtW2bV3p7FC3ymdXgfp61A4pNKvOWhP6WU_Q@mail.gmail.com","subject":"Re: [PATCH 2/2] Don't close pack fd when free'ing pack windows","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-07-31T21:28:08Z","receivedAt":"2013-07-31T21:28:08Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Antoine Pelisse writes:\n\n> On Wed, Jul 31, 2013 at 9:51 PM, Brandon Casey wrote:\n>> -----------------------------------------------------------------------------------\n>> This email message is for the sole use of the intended recipient(s) and may contain\n>> confidential information. Any unauthorized review, use, disclosure or distribution\n>> is prohibited. If you are not the intended recipient, please contact the sender by\n>> reply email and destroy all copies of the original message.\n>> -----------------------------------------------------------------------------------\n>\n> I'm certainly not a lawyer, and I'm sorry for not reviewing the\n> content of the patch instead, but is that not a problem from a legal\n> point of view ?\n> I remember a video of Greg Kroah-Hartman where he talked about that\n> (the video was posted by Junio on G+).\n\nIt's this video:\n\n http://www.youtube.com/watch?v=fMeH7wqOwXA\n\nThe comment starts at 13:55.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"224397","messageId":"CA+sFfMfzHh5kbyv673e6V=Md14DZBqDaLFwspfcZNBGomZBV9g@mail.gmail.com","threadId":"34566","inReplyTo":"20130731212114.GG19369@paksenarrion.iveqy.com","subject":"Re: [PATCH 2/2] Don't close pack fd when free'ing pack windows","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-07-31T21:31:34Z","receivedAt":"2013-07-31T21:31:34Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Wed, Jul 31, 2013 at 2:21 PM, Fredrik Gustafsson wrote:\n> On Wed, Jul 31, 2013 at 11:08:21PM +0200, Antoine Pelisse wrote:\n>> On Wed, Jul 31, 2013 at 9:51 PM, Brandon Casey wrote:\n>> > -----------------------------------------------------------------------------------\n>> > This email message is for the sole use of the intended recipient(s) and may contain\n>> > confidential information. Any unauthorized review, use, disclosure or distribution\n>> > is prohibited. If you are not the intended recipient, please contact the sender by\n>> > reply email and destroy all copies of the original message.\n>> > -----------------------------------------------------------------------------------\n>>\n>> I'm certainly not a lawyer, and I'm sorry for not reviewing the\n>> content of the patch instead, but is that not a problem from a legal\n>> point of view ?\n>\n> Talking about legal, is it a problem if a commit isn't signed-off by\n> it's committer or author e-mail? Like in this case where the sign-off is\n> from gmail.com and the committer from nvidia.com?\n\nIt never has been. My commits should have the author and committer\nset to my gmail address actually.\n\nOthers have sometimes used the two fields to distinguish between a\ncorporate identity (i.e. me@somecompany.com) that represents the\nfunder of the work and a canonical identity (me@personalemail.com)\nthat identifies the person that performed the work.\n\n-Brandon\n"},{"id":"224399","messageId":"20130731214400.GH19369@paksenarrion.iveqy.com","threadId":"34566","inReplyTo":"CA+sFfMfzHh5kbyv673e6V=Md14DZBqDaLFwspfcZNBGomZBV9g@mail.gmail.com","subject":"Re: [PATCH 2/2] Don't close pack fd when free'ing pack windows","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2013-07-31T21:44:00Z","receivedAt":"2013-07-31T21:44:00Z","isPatch":true,"sender":{"key":"iveqy@iveqy.com","avatar":"https://avatars.githubusercontent.com/u/761743?v=4"},"body":"On Wed, Jul 31, 2013 at 02:31:34PM -0700, Brandon Casey wrote:\n> On Wed, Jul 31, 2013 at 2:21 PM, Fredrik Gustafsson wrote:\n> > On Wed, Jul 31, 2013 at 11:08:21PM +0200, Antoine Pelisse wrote:\n> >> On Wed, Jul 31, 2013 at 9:51 PM, Brandon Casey wrote:\n> >> > -----------------------------------------------------------------------------------\n> >> > This email message is for the sole use of the intended recipient(s) and may contain\n> >> > confidential information. Any unauthorized review, use, disclosure or distribution\n> >> > is prohibited. If you are not the intended recipient, please contact the sender by\n> >> > reply email and destroy all copies of the original message.\n> >> > -----------------------------------------------------------------------------------\n> >>\n> >> I'm certainly not a lawyer, and I'm sorry for not reviewing the\n> >> content of the patch instead, but is that not a problem from a legal\n> >> point of view ?\n> >\n> > Talking about legal, is it a problem if a commit isn't signed-off by\n> > it's committer or author e-mail? Like in this case where the sign-off is\n> > from gmail.com and the committer from nvidia.com?\n> \n> It never has been. My commits should have the author and committer\n> set to my gmail address actually.\n\nOh, that's why the extra \"From: \" - field below the header is for.\n\n> \n> Others have sometimes used the two fields to distinguish between a\n> corporate identity (i.e. me@somecompany.com) that represents the\n> funder of the work and a canonical identity (me@personalemail.com)\n> that identifies the person that performed the work.\n> \n\nIn some contries your work when you're employed does not belong\nto you but to your employer and when you're acting for your employer\nyou're representing the corporate legal person. Therefore two different\ne-mails can be seen as two different (legal not physical) persons.\n\nAt least that's how I understand those \"legal tips for developers\" I've\ngot.\n\n-- \nMed vänliga hälsningar\nFredrik Gustafsson\n\ntel: 0733-608274\ne-post: iveqy@iveqy.com\n"},{"id":"224421","messageId":"7vsiyts5bb.fsf@alter.siamese.dyndns.org","threadId":"34566","inReplyTo":"1375300297-6744-1-git-send-email-bcasey@nvidia.com","subject":"Re: [PATCH v2 1/2] sha1_file: introduce close_one_pack() to close packs on fd pressure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-01T17:12:56Z","receivedAt":"2013-08-01T17:12:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> If the refs are loose, then upload-pack will read each ref from the\n> pack (allocating one or more mmap windows) so it can peel tags and\n> advertise the underlying object. If the refs are packed and peeled,\n> then upload-pack will use the peeled sha1 in the packed-refs file and\n> will not need to read from the pack files, so no mmap windows will be\n> allocated and just like with receive-pack, unuse_one_window() will\n\nEven though what it says is not incorrect, the phrasing around here,\nespecially \"so it can\", confused me in my first reading. It reads\nobjects \"in order to\" peel and advertise (and as a side-effect it\ncan lead to windows into packs that eventually help relieaving the\nfd pressure), but a quick scan led me to misread it as \"so it can do\npeel and advertise just fine\", which misses the point, because it is\nnot like we are having trouble peeling and advertising.\n\nAlso, the objects at the tips of refs and the objects they point at\nmay be loose objects, which is very likely for branch tips. The fd\npressure will not be relieved in such a case even if these refs were\npacked.\n\nI've tentatively reworded the above section like so:\n\n ... If the refs are loose, then upload-pack will read each ref\n from the object database (if the object is in a pack, allocating\n one or more mmap windows for it) in order to peel tags and\n advertise the underlying object. But when the refs are packed\n and peeled, upload-pack will use the peeled sha1 in the\n packed-refs file and will not need to read from the pack files,\n so no mmap windows will be allocated ...\n\n> +static int close_one_pack(void)\n> +{\n> +\tstruct packed_git *p, *lru_p = NULL;\n> +\tstruct pack_window *mru_w = NULL;\n> +\n> +\tfor (p = packed_git; p; p = p->next) {\n> +\t\tif (p->pack_fd == -1)\n> +\t\t\tcontinue;\n> +\t\tfind_lru_pack(p, &lru_p, &mru_w);\n> +\t}\n> +\n> +\tif (lru_p) {\n> +\t\tclose_pack_windows(lru_p);\n> +\t\tclose(lru_p->pack_fd);\n> +\t\tpack_open_fds--;\n> +\t\tlru_p->pack_fd = -1;\n> +\t\tif (lru_p == last_found_pack)\n> +\t\t\tlast_found_pack = NULL;\n> +\t\treturn 1;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n\nOK, so in this codepath where we know we are under fd pressure, we\nfind the pack that is least recently used that can be closed, and\nuse close_pack_windows() to reclaim all of its open windows (if\nany), which takes care of the accounting for pack_mapped and\npack_open_windows, but we need to do the pack_open_fds accounting\nhere ourselves. Makes sense to me.\n\nThanks.\n\n> void unuse_pack(struct pack_window **w_cursor)\n> {\n> \tstruct pack_window *w = *w_cursor;\n> @@ -777,7 +838,7 @@ static int open_packed_git_1(struct packed_git *p)\n> \t\t\tpack_max_fds = 1;\n> \t}\n> \n> -\twhile (pack_max_fds <= pack_open_fds && unuse_one_window(NULL, -1))\n> +\twhile (pack_max_fds <= pack_open_fds && close_one_pack())\n> \t\t; /* nothing */\n> \n> \tp->pack_fd = git_open_noatime(p->pack_name);\n"},{"id":"224424","messageId":"CA+sFfMdp9j4LL4eocbsJu5DCEfhoE=uEN_wJ3o8VBW+hUVFVLQ@mail.gmail.com","threadId":"34566","inReplyTo":"7vsiyts5bb.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 1/2] sha1_file: introduce close_one_pack() to close packs on fd pressure","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-08-01T18:01:52Z","receivedAt":"2013-08-01T18:01:52Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Thu, Aug 1, 2013 at 10:12 AM, Junio C Hamano wrote:\n> Brandon Casey writes:\n>\n>> If the refs are loose, then upload-pack will read each ref from the\n>> pack (allocating one or more mmap windows) so it can peel tags and\n>> advertise the underlying object. If the refs are packed and peeled,\n>> then upload-pack will use the peeled sha1 in the packed-refs file and\n>> will not need to read from the pack files, so no mmap windows will be\n>> allocated and just like with receive-pack, unuse_one_window() will\n>\n> Even though what it says is not incorrect, the phrasing around here,\n> especially \"so it can\", confused me in my first reading. It reads\n> objects \"in order to\" peel and advertise (and as a side-effect it\n> can lead to windows into packs that eventually help relieaving the\n> fd pressure), but a quick scan led me to misread it as \"so it can do\n> peel and advertise just fine\", which misses the point, because it is\n> not like we are having trouble peeling and advertising.\n>\n> Also, the objects at the tips of refs and the objects they point at\n> may be loose objects, which is very likely for branch tips. The fd\n> pressure will not be relieved in such a case even if these refs were\n> packed.\n>\n> I've tentatively reworded the above section like so:\n>\n> ... If the refs are loose, then upload-pack will read each ref\n> from the object database (if the object is in a pack, allocating\n> one or more mmap windows for it) in order to peel tags and\n> advertise the underlying object. But when the refs are packed\n> and peeled, upload-pack will use the peeled sha1 in the\n> packed-refs file and will not need to read from the pack files,\n> so no mmap windows will be allocated ...\n\nThanks.\n\n>> +static int close_one_pack(void)\n>> +{\n>> + struct packed_git *p, *lru_p = NULL;\n>> + struct pack_window *mru_w = NULL;\n>> +\n>> + for (p = packed_git; p; p = p->next) {\n>> + if (p->pack_fd == -1)\n>> + continue;\n>> + find_lru_pack(p, &lru_p, &mru_w);\n>> + }\n>> +\n>> + if (lru_p) {\n>> + close_pack_windows(lru_p);\n>> + close(lru_p->pack_fd);\n>> + pack_open_fds--;\n>> + lru_p->pack_fd = -1;\n>> + if (lru_p == last_found_pack)\n>> + last_found_pack = NULL;\n>> + return 1;\n>> + }\n>> +\n>> + return 0;\n>> +}\n>\n> OK, so in this codepath where we know we are under fd pressure, we\n> find the pack that is least recently used that can be closed, and\n> use close_pack_windows() to reclaim all of its open windows (if\n> any),\n\nI've been looking closer at uses of p->windows everywhere, and it\nseems that we always open_packed_git() before we try to create new\nwindows. There doesn't seem to be any reason that we can't continue\nto use the existing open windows even after closing the pack file. We\nobviously do this when the window spans the entire file.\n\nSo, I'm thinking we can drop the close_pack_windows() and refrain from\nresetting last_found_pack, so the last block will become simply:\n\n + if (lru_p) {\n + close(lru_p->pack_fd);\n + pack_open_fds--;\n + lru_p->pack_fd = -1;\n + return 1;\n + }\n\nIf the pack file needs to be reopened later and it has been rewritten\nin the mean time, open_packed_git_1() should notice when it compares\neither the file size or the pack's sha1 checksum to what was\npreviously read from the pack index. So this seems safe.\n\nIf we don't need to close_pack_windows(), find_lru_pack() doesn't\nstrictly need to reject packs that have windows in use. I think the\nalgorithm can be tweaked to prefer to close packs that have no windows\nin use, but still select them for closing if not. The order of\npreference would look like:\n\n 1. pack with no open windows, oldest mtime\n 2. pack with oldest MRU window but none in use\n 3. pack with oldest MRU window\n\n> which takes care of the accounting for pack_mapped and\n> pack_open_windows, but we need to do the pack_open_fds accounting\n> here ourselves. Makes sense to me.\n>\n> Thanks.\n\nSorry about the additional reroll. I'll make the above changes and resubmit.\n\n-Brandon\n"},{"id":"224427","messageId":"7v4nb9s1az.fsf@alter.siamese.dyndns.org","threadId":"34566","inReplyTo":"CA+sFfMdp9j4LL4eocbsJu5DCEfhoE=uEN_wJ3o8VBW+hUVFVLQ@mail.gmail.com","subject":"Re: [PATCH v2 1/2] sha1_file: introduce close_one_pack() to close packs on fd pressure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-01T18:39:32Z","receivedAt":"2013-08-01T18:39:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> I've been looking closer at uses of p->windows everywhere, and it\n> seems that we always open_packed_git() before we try to create new\n> windows. There doesn't seem to be any reason that we can't continue\n> to use the existing open windows even after closing the pack file.\n> ...\n> If we don't need to close_pack_windows(), find_lru_pack() doesn't\n> strictly need to reject packs that have windows in use.\n\nThat makes me feel somewhat uneasy. Yes, you can open/mmap/close\nand hold onto the contents of a file still mapped in-core, and it\nmay not count as \"open filedescriptor\", but do OSes allow infinite\nsuch mmapped regions to us? We do keep track of number of open\nwindows, but is there a way for us to learn how close we are to the\nlimit?\n"},{"id":"224429","messageId":"CA+sFfMer+5bhqxF=_zAQhZ+8sQD6EAYb8HBtYpuhQY_0uj-m9A@mail.gmail.com","threadId":"34566","inReplyTo":"7v4nb9s1az.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 1/2] sha1_file: introduce close_one_pack() to close packs on fd pressure","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-08-01T19:16:57Z","receivedAt":"2013-08-01T19:16:57Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Thu, Aug 1, 2013 at 11:39 AM, Junio C Hamano wrote:\n> Brandon Casey writes:\n>\n>> I've been looking closer at uses of p->windows everywhere, and it\n>> seems that we always open_packed_git() before we try to create new\n>> windows. There doesn't seem to be any reason that we can't continue\n>> to use the existing open windows even after closing the pack file.\n>> ...\n>> If we don't need to close_pack_windows(), find_lru_pack() doesn't\n>> strictly need to reject packs that have windows in use.\n>\n> That makes me feel somewhat uneasy. Yes, you can open/mmap/close\n> and hold onto the contents of a file still mapped in-core, and it\n> may not count as \"open filedescriptor\", but do OSes allow infinite\n> such mmapped regions to us? We do keep track of number of open\n> windows, but is there a way for us to learn how close we are to the\n> limit?\n\nNot that I know of, but xmmap() does already try to unmap existing\nwindows when mmap() fails, and then retries the mmap. It calls\nrelease_pack_memory() which calls unuse_one_window(). mmap returns\nENOMEM when either there is no available memory or if the limit of\nmmap mappings has been exceeded.\n\nSo, I think we'll be ok. It's the same situation we'd be in if there\nwere many large packs (but fewer than pack_max_fds) and a small\npackedGitWindowSize, requiring many mmap windows. We'd try to map an\nadditional segment, fail, release some unused segments, and retry.\n\nThe memory usage of all mmap segments would still be bounded by\npackedGitLimit. It's just that now, when we're only under file\ndescriptor pressure, we won't close the mmap windows unnecessarily\nwhen they may be needed again.\n\n-Brandon\n"},{"id":"224430","messageId":"CA+sFfMe=RfNxE54Jt3QSJjVSOHsXs3jqFi7DbG28vRqCYhPOEQ@mail.gmail.com","threadId":"34566","inReplyTo":"CA+sFfMer+5bhqxF=_zAQhZ+8sQD6EAYb8HBtYpuhQY_0uj-m9A@mail.gmail.com","subject":"Re: [PATCH v2 1/2] sha1_file: introduce close_one_pack() to close packs on fd pressure","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-08-01T19:23:01Z","receivedAt":"2013-08-01T19:23:01Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Thu, Aug 1, 2013 at 12:16 PM, Brandon Casey wrote:\n> On Thu, Aug 1, 2013 at 11:39 AM, Junio C Hamano wrote:\n>> Brandon Casey writes:\n>>\n>>> I've been looking closer at uses of p->windows everywhere, and it\n>>> seems that we always open_packed_git() before we try to create new\n>>> windows. There doesn't seem to be any reason that we can't continue\n>>> to use the existing open windows even after closing the pack file.\n>>> ...\n>>> If we don't need to close_pack_windows(), find_lru_pack() doesn't\n>>> strictly need to reject packs that have windows in use.\n>>\n>> That makes me feel somewhat uneasy. Yes, you can open/mmap/close\n>> and hold onto the contents of a file still mapped in-core, and it\n>> may not count as \"open filedescriptor\", but do OSes allow infinite\n>> such mmapped regions to us? We do keep track of number of open\n>> windows, but is there a way for us to learn how close we are to the\n>> limit?\n>\n> Not that I know of, but xmmap() does already try to unmap existing\n> windows when mmap() fails, and then retries the mmap. It calls\n> release_pack_memory() which calls unuse_one_window(). mmap returns\n> ENOMEM when either there is no available memory or if the limit of\n> mmap mappings has been exceeded.\n>\n> So, I think we'll be ok. It's the same situation we'd be in if there\n> were many large packs (but fewer than pack_max_fds) and a small\n> packedGitWindowSize, requiring many mmap windows. We'd try to map an\n> additional segment, fail, release some unused segments, and retry.\n\nAlso, it's the same situation we'd be in if there were many small\npacks that were smaller than packedGitWindowSize. We'd mmap the\nentire pack file into memory and then close the file descriptor,\nallowing us to have many more pack files mapped into memory than\npack_max_fds would allow us to have open. With enough small packs,\nwe'd eventually reach the mmap limit and xmmap would try to release\nsome mappings.\n\n-Brandon\n"},{"id":"224431","messageId":"7vy58lqiwd.fsf@alter.siamese.dyndns.org","threadId":"34566","inReplyTo":"CA+sFfMer+5bhqxF=_zAQhZ+8sQD6EAYb8HBtYpuhQY_0uj-m9A@mail.gmail.com","subject":"Re: [PATCH v2 1/2] sha1_file: introduce close_one_pack() to close packs on fd pressure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-01T20:02:26Z","receivedAt":"2013-08-01T20:02:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> On Thu, Aug 1, 2013 at 11:39 AM, Junio C Hamano wrote:\n>> That makes me feel somewhat uneasy. Yes, you can open/mmap/close\n>> and hold onto the contents of a file still mapped in-core, and it\n>> may not count as \"open filedescriptor\", but do OSes allow infinite\n>> such mmapped regions to us? We do keep track of number of open\n>> windows, but is there a way for us to learn how close we are to the\n>> limit?\n>\n> Not that I know of, but xmmap() does already try to unmap existing\n> windows when mmap() fails, and then retries the mmap. It calls\n> release_pack_memory() which calls unuse_one_window(). mmap returns\n> ENOMEM when either there is no available memory or if the limit of\n> mmap mappings has been exceeded.\n\nOK, so if there were such an OS limit, the unuse_one_window() will\nhopefully reduce the number of open windows and as a side effect we\nmay go below that limit.\n\nWhat I was worried about was if there was a limit on the number of\nfiles we have windows into (i.e. having one window each in N files,\nwith fds all closed, somehow costs more than having N window in one\nfile with the fd closed). We currently have knobs for total number\nof windows and number of open fds consumed for packs, and the latter\nindirectly controls the number of active packfiles we have windows\ninto. Your proposed change will essentially make the number of\nactive packfiles unlimited by any of our knobs, and that was where\nmy uneasiness was coming from.\n"},{"id":"224432","messageId":"CA+sFfMdJgQaEBx_FsYPz1rC3--jknnb4Zwr+FOaL+9gNe4xwyw@mail.gmail.com","threadId":"34566","inReplyTo":"7vy58lqiwd.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 1/2] sha1_file: introduce close_one_pack() to close packs on fd pressure","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-08-01T20:37:31Z","receivedAt":"2013-08-01T20:37:31Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Thu, Aug 1, 2013 at 1:02 PM, Junio C Hamano wrote:\n> Brandon Casey writes:\n>\n>> On Thu, Aug 1, 2013 at 11:39 AM, Junio C Hamano wrote:\n>>> That makes me feel somewhat uneasy. Yes, you can open/mmap/close\n>>> and hold onto the contents of a file still mapped in-core, and it\n>>> may not count as \"open filedescriptor\", but do OSes allow infinite\n>>> such mmapped regions to us? We do keep track of number of open\n>>> windows, but is there a way for us to learn how close we are to the\n>>> limit?\n>>\n>> Not that I know of, but xmmap() does already try to unmap existing\n>> windows when mmap() fails, and then retries the mmap. It calls\n>> release_pack_memory() which calls unuse_one_window(). mmap returns\n>> ENOMEM when either there is no available memory or if the limit of\n>> mmap mappings has been exceeded.\n>\n> OK, so if there were such an OS limit, the unuse_one_window() will\n> hopefully reduce the number of open windows and as a side effect we\n> may go below that limit.\n>\n> What I was worried about was if there was a limit on the number of\n> files we have windows into (i.e. having one window each in N files,\n> with fds all closed, somehow costs more than having N window in one\n> file with the fd closed).\n\nAh, yeah, I've never heard of that type of limit and I do not know if\nthere is one.\n\nIf there is such a limit, like you said unuse_one_window() will\n_hopefully_ release enough windows to reduce the number of packs we\nhave windows into, but it is certainly not guaranteed.\n\n> We currently have knobs for total number\n> of windows and number of open fds consumed for packs, and the latter\n> indirectly controls the number of active packfiles we have windows\n> into. Your proposed change will essentially make the number of\n> active packfiles unlimited by any of our knobs, and that was where\n> my uneasiness was coming from.\n\nYes and no. The limit on the number of open fds used for packs only\nindirectly controls the number of active packfiles we have windows\ninto for the packs that are larger than packedGitWindowSize. For pack\nfiles smaller than packedGitWindowSize, the number was unlimited too\nsince we close the file descriptor if the whole pack fits within one\nwindow.\n\n-Brandon\n"},{"id":"224442","messageId":"1375421793-32224-1-git-send-email-drafnel@gmail.com","threadId":"34566","inReplyTo":"CA+sFfMdJgQaEBx_FsYPz1rC3--jknnb4Zwr+FOaL+9gNe4xwyw@mail.gmail.com","subject":"[PATCH v3] sha1_file: introduce close_one_pack() to close packs on fd pressure","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-08-02T05:36:33Z","receivedAt":"2013-08-02T05:36:33Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"When the number of open packs exceeds pack_max_fds, unuse_one_window()\nis called repeatedly to attempt to release the least-recently-used\npack windows, which, as a side-effect, will also close a pack file\nafter closing its last open window. If a pack file has been opened,\nbut no windows have been allocated into it, it will never be selected\nby unuse_one_window() and hence its file descriptor will not be\nclosed. When this happens, git may exceed the number of file\ndescriptors permitted by the system.\n\nThis latter situation can occur in show-ref or receive-pack during ref\nadvertisement. During ref advertisement, receive-pack will iterate\nover every ref in the repository and advertise it to the client after\nensuring that the ref exists in the local repository. If the ref is\nlocated inside a pack, then the pack is opened to ensure that it\nexists, but since the object is not actually read from the pack, no\nmmap windows are allocated. When the number of open packs exceeds\npack_max_fds, unuse_one_window() will not be able to find any windows to\nfree and will not be able to close any packs. Once the per-process\nfile descriptor limit is exceeded, receive-pack will produce a warning,\nnot an error, for each pack it cannot open, and will then most likely\nfail with an error to spawn rev-list or index-pack like:\n\n error: cannot create standard input pipe for rev-list: Too many open files\n error: Could not run 'git rev-list'\n\nThis may also occur during upload-pack when refs are packed (in the\npacked-refs file) and the number of packs that must be opened to\nverify that these packed refs exist exceeds the file descriptor\nlimit. If the refs are loose, then upload-pack will read each ref\nfrom the object database (if the object is in a pack, allocating one\nor more mmap windows for it) in order to peel tags and advertise the\nunderlying object. But when the refs are packed and peeled,\nupload-pack will use the peeled sha1 in the packed-refs file and\nwill not need to read from the pack files, so no mmap windows will\nbe allocated and just like with receive-pack, unuse_one_window()\nwill never select these opened packs to close.\n\nWhen we have file descriptor pressure, we just need to find an open\npack to close. We can leave the existing mmap windows open. If\nadditional windows need to be mapped into the pack file, it will be\nreopened when necessary. If the pack file has been rewritten in the\nmean time, open_packed_git_1() should notice when it compares the file\nsize or the pack's sha1 checksum to what was previously read from the\npack index, and reject it.\n\nLet's introduce a new function close_one_pack() designed specifically\nfor this purpose to search for and close the least-recently-used pack,\nwhere LRU is defined as (in order of preference):\n\n * pack with oldest mtime and no allocated mmap windows\n * pack with the least-recently-used windows, i.e. the pack\n with the oldest most-recently-used window, where none of\n the windows are in use\n * pack with the least-recently-used windows\n\nSigned-off-by: Brandon Casey \n---\n\nHere's the version that leaves the mmap windows open after closing\nthe pack file descriptor.\n\n-Brandon\n\n sha1_file.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 78 insertions(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 40b2329..263cf71 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -673,6 +673,83 @@ void close_pack_windows(struct packed_git *p)\n \t}\n }\n \n+/*\n+ * The LRU pack is the one with the oldest MRU window, preferring packs\n+ * with no used windows, or the oldest mtime if it has no windows allocated.\n+ */\n+static void find_lru_pack(struct packed_git *p, struct packed_git **lru_p, struct pack_window **mru_w, int *accept_windows_inuse)\n+{\n+\tstruct pack_window *w, *this_mru_w;\n+\tint has_windows_inuse = 0;\n+\n+\t/*\n+\t * Reject this pack if it has windows and the previously selected\n+\t * one does not. If this pack does not have windows, reject\n+\t * it if the pack file is newer than the previously selected one.\n+\t */\n+\tif (*lru_p && !*mru_w && (p->windows || p->mtime > (*lru_p)->mtime))\n+\t\treturn;\n+\n+\tfor (w = this_mru_w = p->windows; w; w = w->next) {\n+\t\t/*\n+\t\t * Reject this pack if any of its windows are in use,\n+\t\t * but the previously selected pack did not have any\n+\t\t * inuse windows. Otherwise, record that this pack\n+\t\t * has windows in use.\n+\t\t */\n+\t\tif (w->inuse_cnt) {\n+\t\t\tif (*accept_windows_inuse)\n+\t\t\t\thas_windows_inuse = 1;\n+\t\t\telse\n+\t\t\t\treturn;\n+\t\t}\n+\n+\t\tif (w->last_used > this_mru_w->last_used)\n+\t\t\tthis_mru_w = w;\n+\n+\t\t/*\n+\t\t * Reject this pack if it has windows that have been\n+\t\t * used more recently than the previously selected pack.\n+\t\t * If the previously selected pack had windows inuse and\n+\t\t * we have not encountered a window in this pack that is\n+\t\t * inuse, skip this check since we prefer a pack with no\n+\t\t * inuse windows to one that has inuse windows.\n+\t\t */\n+\t\tif (*mru_w && *accept_windows_inuse == has_windows_inuse &&\n+\t\t this_mru_w->last_used > (*mru_w)->last_used)\n+\t\t\treturn;\n+\t}\n+\n+\t/*\n+\t * Select this pack.\n+\t */\n+\t*mru_w = this_mru_w;\n+\t*lru_p = p;\n+\t*accept_windows_inuse = has_windows_inuse;\n+}\n+\n+static int close_one_pack(void)\n+{\n+\tstruct packed_git *p, *lru_p = NULL;\n+\tstruct pack_window *mru_w = NULL;\n+\tint accept_windows_inuse = 1;\n+\n+\tfor (p = packed_git; p; p = p->next) {\n+\t\tif (p->pack_fd == -1)\n+\t\t\tcontinue;\n+\t\tfind_lru_pack(p, &lru_p, &mru_w, &accept_windows_inuse);\n+\t}\n+\n+\tif (lru_p) {\n+\t\tclose(lru_p->pack_fd);\n+\t\tpack_open_fds--;\n+\t\tlru_p->pack_fd = -1;\n+\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n void unuse_pack(struct pack_window **w_cursor)\n {\n \tstruct pack_window *w = *w_cursor;\n@@ -768,7 +845,7 @@ static int open_packed_git_1(struct packed_git *p)\n \t\t\tpack_max_fds = 1;\n \t}\n \n-\twhile (pack_max_fds <= pack_open_fds && unuse_one_window(NULL, -1))\n+\twhile (pack_max_fds <= pack_open_fds && close_one_pack())\n \t\t; /* nothing */\n \n \tp->pack_fd = git_open_noatime(p->pack_name);\n-- \n1.8.1.1.252.gdb33759\n"},{"id":"224454","messageId":"7v38qsqcs8.fsf@alter.siamese.dyndns.org","threadId":"34566","inReplyTo":"1375421793-32224-1-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH v3] sha1_file: introduce close_one_pack() to close packs on fd pressure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-02T16:26:47Z","receivedAt":"2013-08-02T16:26:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> +/*\n> + * The LRU pack is the one with the oldest MRU window, preferring packs\n> + * with no used windows, or the oldest mtime if it has no windows allocated.\n> + */\n> +static void find_lru_pack(struct packed_git *p, struct packed_git **lru_p, struct pack_window **mru_w, int *accept_windows_inuse)\n> +{\n> +\tstruct pack_window *w, *this_mru_w;\n> +\tint has_windows_inuse = 0;\n> +\n> +\t/*\n> +\t * Reject this pack if it has windows and the previously selected\n> +\t * one does not. If this pack does not have windows, reject\n> +\t * it if the pack file is newer than the previously selected one.\n> +\t */\n> +\tif (*lru_p && !*mru_w && (p->windows || p->mtime > (*lru_p)->mtime))\n> +\t\treturn;\n> +\n> +\tfor (w = this_mru_w = p->windows; w; w = w->next) {\n> +\t\t/*\n> +\t\t * Reject this pack if any of its windows are in use,\n> +\t\t * but the previously selected pack did not have any\n> +\t\t * inuse windows. Otherwise, record that this pack\n> +\t\t * has windows in use.\n> +\t\t */\n> +\t\tif (w->inuse_cnt) {\n> +\t\t\tif (*accept_windows_inuse)\n> +\t\t\t\thas_windows_inuse = 1;\n> +\t\t\telse\n> +\t\t\t\treturn;\n> +\t\t}\n> +\n> +\t\tif (w->last_used > this_mru_w->last_used)\n> +\t\t\tthis_mru_w = w;\n> +\n> +\t\t/*\n> +\t\t * Reject this pack if it has windows that have been\n> +\t\t * used more recently than the previously selected pack.\n> +\t\t * If the previously selected pack had windows inuse and\n> +\t\t * we have not encountered a window in this pack that is\n> +\t\t * inuse, skip this check since we prefer a pack with no\n> +\t\t * inuse windows to one that has inuse windows.\n> +\t\t */\n> +\t\tif (*mru_w && *accept_windows_inuse == has_windows_inuse &&\n> +\t\t this_mru_w->last_used > (*mru_w)->last_used)\n> +\t\t\treturn;\n\nThe \"*accept_windows_inuse == has_windows_inuse\" part is hard to\ngrok, together with the fact that this statement is evaluated for\neach and every \"w\", even though it is about this_mru_w and that\nvariable is not updated in every iteration of the loop. Can you\nclarify/simplify this part of the code a bit more?\n\nFor example, would the above be equivalent to this?\n\n\t\tif (w->last_used < this_mru_w->last_used)\n\t\t\tcontinue;\n\n\t\tthis_mru_w = w;\n if (has_windows_inuse && *mru_w &&\n w->last_used > (*mru_w)->last_used)\n\t\t\treturn;\n\nThat is, if we already know a more recently used window in this\npack, we do not have to do anything to maintain mru_w. Otherwise,\nremember that this window is the most recently used one in this\npack, and if it is newer than the newest one from the pack we are\ngoing to pick, we refrain from picking this pack.\n\nBut we do not reject ourselves if we haven't seen a window that is\nin use (yet).\n\n> +\t}\n> +\n> +\t/*\n> +\t * Select this pack.\n> +\t */\n> +\t*mru_w = this_mru_w;\n> +\t*lru_p = p;\n> +\t*accept_windows_inuse = has_windows_inuse;\n> +}\n> +\n> +static int close_one_pack(void)\n> +{\n> +\tstruct packed_git *p, *lru_p = NULL;\n> +\tstruct pack_window *mru_w = NULL;\n> +\tint accept_windows_inuse = 1;\n> +\n> +\tfor (p = packed_git; p; p = p->next) {\n> +\t\tif (p->pack_fd == -1)\n> +\t\t\tcontinue;\n> +\t\tfind_lru_pack(p, &lru_p, &mru_w, &accept_windows_inuse);\n> +\t}\n> +\n> +\tif (lru_p) {\n> +\t\tclose(lru_p->pack_fd);\n> +\t\tpack_open_fds--;\n> +\t\tlru_p->pack_fd = -1;\n> +\t\treturn 1;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> void unuse_pack(struct pack_window **w_cursor)\n> {\n> \tstruct pack_window *w = *w_cursor;\n> @@ -768,7 +845,7 @@ static int open_packed_git_1(struct packed_git *p)\n> \t\t\tpack_max_fds = 1;\n> \t}\n> \n> -\twhile (pack_max_fds <= pack_open_fds && unuse_one_window(NULL, -1))\n> +\twhile (pack_max_fds <= pack_open_fds && close_one_pack())\n> \t\t; /* nothing */\n> \n> \tp->pack_fd = git_open_noatime(p->pack_name);\n"},{"id":"224458","messageId":"CA+sFfMfZCAmPNzD8cBY0-1CePi5VM3dBqaxxNV=hvxkfStXK4g@mail.gmail.com","threadId":"34566","inReplyTo":"7v38qsqcs8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] sha1_file: introduce close_one_pack() to close packs on fd pressure","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-08-02T17:12:08Z","receivedAt":"2013-08-02T17:12:08Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Fri, Aug 2, 2013 at 9:26 AM, Junio C Hamano wrote:\n> Brandon Casey writes:\n>\n>> +/*\n>> + * The LRU pack is the one with the oldest MRU window, preferring packs\n>> + * with no used windows, or the oldest mtime if it has no windows allocated.\n>> + */\n>> +static void find_lru_pack(struct packed_git *p, struct packed_git **lru_p, struct pack_window **mru_w, int *accept_windows_inuse)\n>> +{\n>> + struct pack_window *w, *this_mru_w;\n>> + int has_windows_inuse = 0;\n>> +\n>> + /*\n>> + * Reject this pack if it has windows and the previously selected\n>> + * one does not. If this pack does not have windows, reject\n>> + * it if the pack file is newer than the previously selected one.\n>> + */\n>> + if (*lru_p && !*mru_w && (p->windows || p->mtime > (*lru_p)->mtime))\n>> + return;\n>> +\n>> + for (w = this_mru_w = p->windows; w; w = w->next) {\n>> + /*\n>> + * Reject this pack if any of its windows are in use,\n>> + * but the previously selected pack did not have any\n>> + * inuse windows. Otherwise, record that this pack\n>> + * has windows in use.\n>> + */\n>> + if (w->inuse_cnt) {\n>> + if (*accept_windows_inuse)\n>> + has_windows_inuse = 1;\n>> + else\n>> + return;\n>> + }\n>> +\n>> + if (w->last_used > this_mru_w->last_used)\n>> + this_mru_w = w;\n>> +\n>> + /*\n>> + * Reject this pack if it has windows that have been\n>> + * used more recently than the previously selected pack.\n>> + * If the previously selected pack had windows inuse and\n>> + * we have not encountered a window in this pack that is\n>> + * inuse, skip this check since we prefer a pack with no\n>> + * inuse windows to one that has inuse windows.\n>> + */\n>> + if (*mru_w && *accept_windows_inuse == has_windows_inuse &&\n>> + this_mru_w->last_used > (*mru_w)->last_used)\n>> + return;\n>\n> The \"*accept_windows_inuse == has_windows_inuse\" part is hard to\n> grok, together with the fact that this statement is evaluated for\n> each and every \"w\", even though it is about this_mru_w and that\n> variable is not updated in every iteration of the loop. Can you\n> clarify/simplify this part of the code a bit more?\n>\n> For example, would the above be equivalent to this?\n>\n> if (w->last_used < this_mru_w->last_used)\n> continue;\n>\n> this_mru_w = w;\n> if (has_windows_inuse && *mru_w &&\n> w->last_used > (*mru_w)->last_used)\n> return;\n>\n> That is, if we already know a more recently used window in this\n> pack, we do not have to do anything to maintain mru_w. Otherwise,\n> remember that this window is the most recently used one in this\n> pack, and if it is newer than the newest one from the pack we are\n> going to pick, we refrain from picking this pack.\n>\n> But we do not reject ourselves if we haven't seen a window that is\n> in use (yet).\n\nNo that wouldn't be the same. The function of \"*accept_windows_inuse\n== has_windows_inuse\" and the testing of this_mru_w in every loop\nrather than w, is too subtle. I tried to draw attention to it in the\ncomment, but I agree it's not enough.\n\nThe case that your example would not catch is when the new pack's mru\nwindow has already been found, but has_windows_inuse is not set until\nlater. When has_windows_inuse is later set, we need to test\nthis_mru_w regardless of whether we have just assigned it.\n\nFor example, if mru_w points to a pack with last_used == 11 and\n*accept_windows_inuse = 1, and p->windows looks like this:\n\n last_used in_use\n 12 0\n 10 1\n\nThen the first time through the loop, this_mru_w would be set to the\nfirst window with last_used equal to 12. The if statement that tests\n\"this_mru_w->last_used > (*mru_w)->last_used\" would be skipped since\nhas_windows_inuse would still be 0. The second time through the loop,\nthis_mru_w would _not_ be reset, but has_windows_inuse _would_ be set.\n This time, we would want to enter the last for loop so that we can\nreject the pack.\n\nI'll try to rework this loop or add comments to clarify.\n\n-Brandon\n\n\n>> + }\n>> +\n>> + /*\n>> + * Select this pack.\n>> + */\n>> + *mru_w = this_mru_w;\n>> + *lru_p = p;\n>> + *accept_windows_inuse = has_windows_inuse;\n>> +}\n>> +\n>> +static int close_one_pack(void)\n>> +{\n>> + struct packed_git *p, *lru_p = NULL;\n>> + struct pack_window *mru_w = NULL;\n>> + int accept_windows_inuse = 1;\n>> +\n>> + for (p = packed_git; p; p = p->next) {\n>> + if (p->pack_fd == -1)\n>> + continue;\n>> + find_lru_pack(p, &lru_p, &mru_w, &accept_windows_inuse);\n>> + }\n>> +\n>> + if (lru_p) {\n>> + close(lru_p->pack_fd);\n>> + pack_open_fds--;\n>> + lru_p->pack_fd = -1;\n>> + return 1;\n>> + }\n>> +\n>> + return 0;\n>> +}\n>> +\n>> void unuse_pack(struct pack_window **w_cursor)\n>> {\n>> struct pack_window *w = *w_cursor;\n>> @@ -768,7 +845,7 @@ static int open_packed_git_1(struct packed_git *p)\n>> pack_max_fds = 1;\n>> }\n>>\n>> - while (pack_max_fds <= pack_open_fds && unuse_one_window(NULL, -1))\n>> + while (pack_max_fds <= pack_open_fds && close_one_pack())\n>> ; /* nothing */\n>>\n>> p->pack_fd = git_open_noatime(p->pack_name);\n"}]}