{"thread":{"id":"7827","subject":"[PATCH] Actually handle some-low memory conditions","startedAt":"2007-04-25T08:02:27Z","lastAt":"2007-04-25T08:02:27Z","messageCount":1,"participants":["Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"40383","messageId":"20070425080227.GA8368@spearce.org","threadId":"7827","inReplyTo":null,"subject":"[PATCH] Actually handle some-low memory conditions","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-04-25T08:02:27Z","receivedAt":"2007-04-25T08:02:27Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Tim Ansell discovered his Debian server didn't permit git-daemon to\nuse as much memory as it needed to handle cloning a project with\na 128 MiB packfile. Filtering the strace provided by Tim of the\nrev-list child showed this gem of a sequence:\n\n open(\"./objects/pack/pack-*.pack\", O_RDONLY|O_LARGEFILE \n <... open resumed> ) = 5\n\nOK, so the packfile is fd 5...\n\n mmap2(NULL, 33554432, PROT_READ, MAP_PRIVATE, 5, 0 \n <... mmap2 resumed> ) = 0xb5e2d000\n\nand we mapped one 32 MiB window from it at position 0...\n\n mmap2(NULL, 31020635, PROT_READ, MAP_PRIVATE, 5, 0x6000 \n <... mmap2 resumed> ) = -1 ENOMEM (Cannot allocate memory)\n\nAnd we asked for another window further into the file. But got\ndenied. In Tim's case this was due to a resource limit on the\ngit-daemon process, and its children.\n\nNow where are we in the code? We're down inside use_pack(),\nafter we have called unuse_one_window() enough times to make sure\nwe stay within our allowed maximum window size. However since we\ndidn't unmap the prior window at 0xb5e2d000 we aren't exceeding\nthe current limit (which probably was just the defaults).\n\nBut we're actually down inside xmmap()...\n\nSo we release the window we do have (by calling release_pack_memory),\nassuming there is some memory pressure...\n\n munmap(0xb5e2d000, 33554432 \n <... munmap resumed> ) = 0\n close(5 \n <... close resumed> ) = 0\n\nAnd that was the last window in this packfile. So we closed it.\nWay to go us. Our xmmap did not expect release_pack_memory to\nclose the fd its about to map...\n\n mmap2(NULL, 31020635, PROT_READ, MAP_PRIVATE, 5, 0x6000 \n <... mmap2 resumed> ) = -1 EBADF (Bad file descriptor)\n\nAnd so the Linux kernel happily tells us f' off.\n\n write(2, \"fatal: \", 7 \n <... write resumed> ) = 7\n write(2, \"Out of memory? mmap failed: Bad \"..., 47 \n <... write resumed> ) = 47\n\nAnd we report the bad file descriptor error, and not the ENOMEM,\nand die, claiming we are out of memory. But actually that mmap\nshould have succeeded, as we had enough memory for that window,\nseeing as how we released the prior one.\n\nOriginally when I developed the sliding window mmap feature I had\nthis exact same bug in fast-import, and I dealt with it by handing\nin the struct packed_git* we want to open the new window for, as the\ncaller wasn't prepared to reopen the packfile if unuse_one_window\nclosed it. The same is true here from xmmap, but the caller doesn't\nhave the struct packed_git* handy. So I'm using the file descriptor\ninstead to perform the same test.\n\nSigned-off-by: Shawn O. Pearce \n---\n\n The patch is obviously correct. But I don't have the CPU cycles\n right now to compile this thing, or test it. (Sorry, another job\n is higher priority, has been running 9 hours now and has another\n ~11 hours to go.)\n\n git-compat-util.h | 12 ++++++------\n sha1_file.c | 10 +++++-----\n 2 files changed, 11 insertions(+), 11 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 5f6a281..e3cf370 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -156,13 +156,13 @@ extern size_t gitstrlcpy(char *, const char *, size_t);\n extern uintmax_t gitstrtoumax(const char *, char **, int);\n #endif\n \n-extern void release_pack_memory(size_t);\n+extern void release_pack_memory(size_t, int);\n \n static inline char* xstrdup(const char *str)\n {\n \tchar *ret = strdup(str);\n \tif (!ret) {\n-\t\trelease_pack_memory(strlen(str) + 1);\n+\t\trelease_pack_memory(strlen(str) + 1, -1);\n \t\tret = strdup(str);\n \t\tif (!ret)\n \t\t\tdie(\"Out of memory, strdup failed\");\n@@ -176,7 +176,7 @@ static inline void *xmalloc(size_t size)\n \tif (!ret && !size)\n \t\tret = malloc(1);\n \tif (!ret) {\n-\t\trelease_pack_memory(size);\n+\t\trelease_pack_memory(size, -1);\n \t\tret = malloc(size);\n \t\tif (!ret && !size)\n \t\t\tret = malloc(1);\n@@ -195,7 +195,7 @@ static inline void *xrealloc(void *ptr, size_t size)\n \tif (!ret && !size)\n \t\tret = realloc(ptr, 1);\n \tif (!ret) {\n-\t\trelease_pack_memory(size);\n+\t\trelease_pack_memory(size, -1);\n \t\tret = realloc(ptr, size);\n \t\tif (!ret && !size)\n \t\t\tret = realloc(ptr, 1);\n@@ -211,7 +211,7 @@ static inline void *xcalloc(size_t nmemb, size_t size)\n \tif (!ret && (!nmemb || !size))\n \t\tret = calloc(1, 1);\n \tif (!ret) {\n-\t\trelease_pack_memory(nmemb * size);\n+\t\trelease_pack_memory(nmemb * size, -1);\n \t\tret = calloc(nmemb, size);\n \t\tif (!ret && (!nmemb || !size))\n \t\t\tret = calloc(1, 1);\n@@ -228,7 +228,7 @@ static inline 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);\n+\t\trelease_pack_memory(length, fd);\n \t\tret = mmap(start, length, prot, flags, fd, offset);\n \t\tif (ret == MAP_FAILED)\n \t\t\tdie(\"Out of memory? mmap failed: %s\", strerror(errno));\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 9c26038..3717c7e 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -516,7 +516,7 @@ static void scan_windows(struct packed_git *p,\n \t}\n }\n \n-static int unuse_one_window(struct packed_git *current)\n+static int unuse_one_window(struct packed_git *current, int keep_fd)\n {\n \tstruct packed_git *p, *lru_p = NULL;\n \tstruct pack_window *lru_w = NULL, *lru_l = NULL;\n@@ -532,7 +532,7 @@ static int unuse_one_window(struct packed_git *current)\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 != current) {\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\tlru_p->pack_fd = -1;\n \t\t\t}\n@@ -544,10 +544,10 @@ static int unuse_one_window(struct packed_git *current)\n \treturn 0;\n }\n \n-void release_pack_memory(size_t need)\n+void release_pack_memory(size_t need, int fd)\n {\n \tsize_t cur = pack_mapped;\n-\twhile (need >= (cur - pack_mapped) && unuse_one_window(NULL))\n+\twhile (need >= (cur - pack_mapped) && unuse_one_window(NULL, fd))\n \t\t; /* nothing */\n }\n \n@@ -680,7 +680,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))\n+\t\t\t\t&& unuse_one_window(p, p->pack_fd))\n \t\t\t\t; /* nothing */\n \t\t\twin->base = xmmap(NULL, win->len,\n \t\t\t\tPROT_READ, MAP_PRIVATE,\n-- \n1.5.1.1.135.gf948\n"}]}