{"thread":{"id":"56024","subject":"[PATCH 0/4] gracefully handling mmap failures","startedAt":"2021-06-29T08:11:12Z","lastAt":"2021-06-30T17:18:23Z","messageCount":15,"participants":["Eric Wong","Junio C Hamano","Jeff King","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"428724","messageId":"20210629081108.28657-1-e@80x24.org","threadId":"56024","inReplyTo":null,"subject":"[PATCH 0/4] gracefully handling mmap failures","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2021-06-29T08:11:04Z","receivedAt":"2021-06-29T08:11:12Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"With an out-of-the-box Linux kernel, I encountered failures at\nall of these mmap call sites via \"git cat-file --batch\" on a\ntest repo with 100K total alternates.\n\nWhile upping sys.vm.max_map_count and/or RLIMIT_DATA solves the\nproblem, not all users have administrative privileges to do so.\n\nEric Wong (4):\n  use_pack: attempt to handle ENOMEM from mmap\n  map_loose_object_1: attempt to handle ENOMEM from mmap\n  check_packed_git_idx: attempt to handle ENOMEM from mmap\n  xmmap: inform Linux users of tuning knobs on ENOMEM\n\n object-file.c | 16 ++++++++++++++--\n packfile.c    | 17 ++++++++++++-----\n packfile.h    |  2 ++\n 3 files changed, 28 insertions(+), 7 deletions(-)\n"},{"id":"428725","messageId":"20210629081108.28657-3-e@80x24.org","threadId":"56024","inReplyTo":"20210629081108.28657-1-e@80x24.org","subject":"[PATCH 2/4] map_loose_object_1: attempt to handle ENOMEM from mmap","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2021-06-29T08:11:06Z","receivedAt":"2021-06-29T08:11:21Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"This benefits unprivileged users who lack permissions to raise\nthe `sys.vm.max_map_count' sysctl and/or RLIMIT_DATA resource\nlimit.\n\nMulti-threaded callers to map_loose_object_1 (e.g. \"git grep\")\nappear to guard against thread-safety problems.  Other\nmulti-core aware pieces of code (e.g. parallel-checkout) uses\nchild processes\n\nForcing a call to unuse_one_window() once before xmmap_gently()\nrevealed no test suite failures:\n\n\t--- a/object-file.c\n\t+++ b/object-file.c\n\t@@ -1197,6 +1197,7 @@ static void *map_loose_object_1(struct repository *r, const char *path,\n\t\t\t\t\treturn NULL;\n\t\t\t\t}\n\t\t\t\tdo {\n\t+\t\t\t\tunuse_one_window(NULL);\n\t\t\t\t\tmap = xmmap_gently(NULL, *size, PROT_READ,\n\t\t\t\t\t\t\tMAP_PRIVATE, fd, 0);\n\t\t\t\t} while (map == MAP_FAILED && errno == ENOMEM\n\nSigned-off-by: Eric Wong <e@80x24.org>\n---\n object-file.c | 8 +++++++-\n packfile.c    | 2 +-\n packfile.h    | 2 ++\n 3 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex f233b440b2..4c043f1f3c 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1196,7 +1196,13 @@ static void *map_loose_object_1(struct repository *r, const char *path,\n \t\t\t\tclose(fd);\n \t\t\t\treturn NULL;\n \t\t\t}\n-\t\t\tmap = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\t\t\tdo {\n+\t\t\t\tmap = xmmap_gently(NULL, *size, PROT_READ,\n+\t\t\t\t\t\tMAP_PRIVATE, fd, 0);\n+\t\t\t} while (map == MAP_FAILED && errno == ENOMEM\n+\t\t\t\t&& unuse_one_window(NULL));\n+\t\t\tif (map == MAP_FAILED)\n+\t\t\t\tdie_errno(\"%s cannot be mapped\", path);\n \t\t}\n \t\tclose(fd);\n \t}\ndiff --git a/packfile.c b/packfile.c\nindex a0da790fb4..4bc7fa0f36 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -265,7 +265,7 @@ static void scan_windows(struct packed_git *p,\n \t}\n }\n \n-static int unuse_one_window(struct packed_git *current)\n+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;\ndiff --git a/packfile.h b/packfile.h\nindex 3ae117a8ae..da9a061e30 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -196,4 +196,6 @@ int is_promisor_object(const struct object_id *oid);\n int load_idx(const char *path, const unsigned int hashsz, void *idx_map,\n \t     size_t idx_size, struct packed_git *p);\n \n+int unuse_one_window(struct packed_git *);\n+\n #endif\n"},{"id":"428726","messageId":"20210629081108.28657-2-e@80x24.org","threadId":"56024","inReplyTo":"20210629081108.28657-1-e@80x24.org","subject":"[PATCH 1/4] use_pack: attempt to handle ENOMEM from mmap","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2021-06-29T08:11:05Z","receivedAt":"2021-06-29T08:11:21Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Since use_pack() can already safely munmap packs to respect\ncore.packedGitLimit, attempt to gracefully handle ENOMEM\nerrors the same way by unmapping a window and retrying.\n\nThis benefits unprivileged users who lack permissions to raise\nthe `sys.vm.max_map_count' sysctl and/or RLIMIT_DATA resource\nlimit.\n\nI've also verified it is safe to release a pack here by\nunconditionally calling unuse_one_window() before\nxmmap_gently():\n\n\t--- a/packfile.c\n\t+++ b/packfile.c\n\t@@ -649,6 +649,7 @@ unsigned char *use_pack(struct packed_git *p,\n\t\t\t\t\t&& unuse_one_window(p))\n\t\t\t\t\t; /* nothing */\n\t\t\t\tdo {\n\t+\t\t\t\tunuse_one_window(p);\n\t\t\t\t\twin->base = xmmap_gently(NULL, win->len,\n\t\t\t\t\t\tPROT_READ, MAP_PRIVATE,\n\t\t\t\t\t\tp->pack_fd, win->offset);\n\nSigned-off-by: Eric Wong <e@80x24.org>\n---\n packfile.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex 755aa7aec5..a0da790fb4 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -648,9 +648,12 @@ unsigned char *use_pack(struct packed_git *p,\n \t\t\twhile (packed_git_limit < pack_mapped\n \t\t\t\t&& unuse_one_window(p))\n \t\t\t\t; /* nothing */\n-\t\t\twin->base = xmmap_gently(NULL, win->len,\n-\t\t\t\tPROT_READ, MAP_PRIVATE,\n-\t\t\t\tp->pack_fd, win->offset);\n+\t\t\tdo {\n+\t\t\t\twin->base = xmmap_gently(NULL, win->len,\n+\t\t\t\t\tPROT_READ, MAP_PRIVATE,\n+\t\t\t\t\tp->pack_fd, win->offset);\n+\t\t\t} while (win->base == MAP_FAILED && errno == ENOMEM\n+\t\t\t\t&& unuse_one_window(p));\n \t\t\tif (win->base == MAP_FAILED)\n \t\t\t\tdie_errno(\"packfile %s cannot be mapped\",\n \t\t\t\t\t  p->pack_name);\n"},{"id":"428727","messageId":"20210629081108.28657-4-e@80x24.org","threadId":"56024","inReplyTo":"20210629081108.28657-1-e@80x24.org","subject":"[PATCH 3/4] check_packed_git_idx: attempt to handle ENOMEM from mmap","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2021-06-29T08:11:07Z","receivedAt":"2021-06-29T08:11:21Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"This benefits unprivileged users who lack permissions to raise\nthe `sys.vm.max_map_count' sysctl and/or RLIMIT_DATA resource\nlimit.\n\nIt appears none of our multithreaded code is calling this,\nor is doing so while holding appropropriate locks to serialize\naccess to the object database.\n\nUnconditionally calling unuse_one_window() before xmmap_gently()\nrevealed no test suite failures:\n\n\t--- a/packfile.c\n\t+++ b/packfile.c\n\t@@ -98,6 +98,7 @@ static int check_packed_git_idx(const char *path, struct packed_git *p)\n\t\t\treturn error(\"index file %s is too small\", path);\n\t\t}\n\t\tdo {\n\t+\t\tunuse_one_window(p);\n\t\t\tidx_map = xmmap_gently(NULL, idx_size, PROT_READ, MAP_PRIVATE,\n\t\t\t\t\t\tfd, 0);\n\t\t} while (idx_map == MAP_FAILED && errno == ENOMEM\n\nSigned-off-by: Eric Wong <e@80x24.org>\n---\n packfile.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex 4bc7fa0f36..2904560f52 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -97,7 +97,11 @@ static int check_packed_git_idx(const char *path, struct packed_git *p)\n \t\tclose(fd);\n \t\treturn error(\"index file %s is too small\", path);\n \t}\n-\tidx_map = xmmap(NULL, idx_size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\tdo {\n+\t\tidx_map = xmmap_gently(NULL, idx_size, PROT_READ, MAP_PRIVATE,\n+\t\t\t\t\tfd, 0);\n+\t} while (idx_map == MAP_FAILED && errno == ENOMEM\n+\t\t&& unuse_one_window(p));\n \tclose(fd);\n \n \tret = load_idx(path, hashsz, idx_map, idx_size, p);\n"},{"id":"428728","messageId":"20210629081108.28657-5-e@80x24.org","threadId":"56024","inReplyTo":"20210629081108.28657-1-e@80x24.org","subject":"[PATCH 4/4] xmmap: inform Linux users of tuning knobs on ENOMEM","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2021-06-29T08:11:08Z","receivedAt":"2021-06-29T08:11:23Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"In cases where we can't (or haven't gotten around to) attempting\ngraceful handling of mmap failures, Linux users may benefit from\nadditional information on how to avoid the problem.\n\nSigned-off-by: Eric Wong <e@80x24.org>\n---\n object-file.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 4c043f1f3c..4139a5ebe6 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1027,8 +1027,14 @@ void *xmmap(void *start, size_t length,\n \tint prot, int flags, int fd, off_t offset)\n {\n \tvoid *ret = xmmap_gently(start, length, prot, flags, fd, offset);\n-\tif (ret == MAP_FAILED)\n+\tif (ret == MAP_FAILED) {\n+#if defined(__linux__)\n+\t\tif (errno == ENOMEM)\n+\t\t\tdie_errno(_(\n+\"mmap failed, check sys.vm.max_map_count and/or RLIMIT_DATA\"));\n+#endif /* OS-specific bits */\n \t\tdie_errno(_(\"mmap failed\"));\n+\t}\n \treturn ret;\n }\n \n"},{"id":"428774","messageId":"20210629202101.GA18279@dcvr","threadId":"56024","inReplyTo":"20210629081108.28657-4-e@80x24.org","subject":"[HOLD 3/4] check_packed_git_idx: attempt to handle ENOMEM from mmap","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2021-06-29T20:21:01Z","receivedAt":"2021-06-29T20:21:04Z","isPatch":false,"sender":{"key":"e@80x24.org","avatar":null},"body":"Eric Wong <e@80x24.org> wrote:\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -97,7 +97,11 @@ static int check_packed_git_idx(const char *path, struct packed_git *p)\n>  \t\tclose(fd);\n>  \t\treturn error(\"index file %s is too small\", path);\n>  \t}\n> -\tidx_map = xmmap(NULL, idx_size, PROT_READ, MAP_PRIVATE, fd, 0);\n> +\tdo {\n> +\t\tidx_map = xmmap_gently(NULL, idx_size, PROT_READ, MAP_PRIVATE,\n> +\t\t\t\t\tfd, 0);\n> +\t} while (idx_map == MAP_FAILED && errno == ENOMEM\n> +\t\t&& unuse_one_window(p));\n\nOops, I dropped extra error handling here :x\n\n>  \tclose(fd);\n>  \n>  \tret = load_idx(path, hashsz, idx_map, idx_size, p);\n"},{"id":"428782","messageId":"xmqqbl7owe2q.fsf@gitster.g","threadId":"56024","inReplyTo":"20210629202101.GA18279@dcvr","subject":"Re: [HOLD 3/4] check_packed_git_idx: attempt to handle ENOMEM from mmap","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-29T21:33:49Z","receivedAt":"2021-06-29T21:33:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <e@80x24.org> writes:\n\n> Eric Wong <e@80x24.org> wrote:\n>> --- a/packfile.c\n>> +++ b/packfile.c\n>> @@ -97,7 +97,11 @@ static int check_packed_git_idx(const char *path, struct packed_git *p)\n>>  \t\tclose(fd);\n>>  \t\treturn error(\"index file %s is too small\", path);\n>>  \t}\n>> -\tidx_map = xmmap(NULL, idx_size, PROT_READ, MAP_PRIVATE, fd, 0);\n>> +\tdo {\n>> +\t\tidx_map = xmmap_gently(NULL, idx_size, PROT_READ, MAP_PRIVATE,\n>> +\t\t\t\t\tfd, 0);\n>> +\t} while (idx_map == MAP_FAILED && errno == ENOMEM\n>> +\t\t&& unuse_one_window(p));\n>\n> Oops, I dropped extra error handling here :x\n>\n>>  \tclose(fd);\n>>  \n>>  \tret = load_idx(path, hashsz, idx_map, idx_size, p);\n\n\nSomething like this, perhaps?  You'd also need _() around the error\nmessage you added to object-file.c in [2/4], I would think.\n\n packfile.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git c/packfile.c w/packfile.c\nindex 2904560f52..b31f14ecb7 100644\n--- c/packfile.c\n+++ w/packfile.c\n@@ -102,6 +102,10 @@ static int check_packed_git_idx(const char *path, struct packed_git *p)\n \t\t\t\t\tfd, 0);\n \t} while (idx_map == MAP_FAILED && errno == ENOMEM\n \t\t&& unuse_one_window(p));\n+\tif (idx_map == MAP_FAILED) {\n+\t\tclose(fd);\n+\t\treturn error_errno(_(\"%s cannot be mapped\"), path);\n+\t}\n \tclose(fd);\n \n \tret = load_idx(path, hashsz, idx_map, idx_size, p);\n"},{"id":"428785","messageId":"20210629223142.GA20945@dcvr","threadId":"56024","inReplyTo":"xmqqbl7owe2q.fsf@gitster.g","subject":"Re: [HOLD 3/4] check_packed_git_idx: attempt to handle ENOMEM from mmap","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2021-06-29T22:31:42Z","receivedAt":"2021-06-29T22:31:46Z","isPatch":false,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> Something like this, perhaps?  You'd also need _() around the error\n> message you added to object-file.c in [2/4], I would think.\n\nI think I need to drop 1-3 of this series because it seems\nimpossible to account for the mmap-via-malloc cases that\nstill bumps into vm.max_map_count limitations under Linux.\n"},{"id":"428790","messageId":"20210630000132.GA2653@dcvr","threadId":"56024","inReplyTo":"20210629081108.28657-1-e@80x24.org","subject":"[PATCH v2] xmmap: inform Linux users of tuning knobs on ENOMEM","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2021-06-30T00:01:32Z","receivedAt":"2021-06-30T00:01:37Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"This series is now down to a single patch.\n\nI wanted to make things more transparent to users without\nprivileges to raise sys.vm.max_map_count and/or RLIMIT_DATA;\nbut it doesn't seem possible to account for libc/zlib/etc. doing\nmmap() without our knowledge (usually via malloc).\n\nSo I think giving users some information to feed their sysadmins\nis the best we can do in this situation:\n\n--------------8<-----------\nSubject: [PATCH] xmmap: inform Linux users of tuning knobs on ENOMEM\n\nLinux users may benefit from additional information on how to\navoid ENOMEM from mmap despite the system having enough RAM to\naccomodate them.  We can't reliably unmap pack windows to work\naround the issue since malloc and other library routines may\nmmap without our knowledge.\n\nSigned-off-by: Eric Wong <e@80x24.org>\n---\n config.c          |  3 ++-\n git-compat-util.h |  1 +\n object-file.c     | 16 +++++++++++++++-\n packfile.c        |  4 ++--\n read-cache.c      |  3 ++-\n 5 files changed, 22 insertions(+), 5 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex f9c400ad30..79ae9f2dea 100644\n--- a/config.c\n+++ b/config.c\n@@ -3051,7 +3051,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\tif (contents == MAP_FAILED) {\n \t\t\tif (errno == ENODEV && S_ISDIR(st.st_mode))\n \t\t\t\terrno = EISDIR;\n-\t\t\terror_errno(_(\"unable to mmap '%s'\"), config_filename);\n+\t\t\terror_errno(_(\"unable to mmap '%s'%s\"),\n+\t\t\t\t\tconfig_filename, mmap_os_err());\n \t\t\tret = CONFIG_INVALID_FILE;\n \t\t\tcontents = NULL;\n \t\t\tgoto out_free;\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex fb6e9af76b..fa6dd92219 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -876,6 +876,7 @@ char *xstrndup(const char *str, size_t len);\n void *xrealloc(void *ptr, size_t size);\n void *xcalloc(size_t nmemb, size_t size);\n void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);\n+const char *mmap_os_err(void);\n void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_t offset);\n int xopen(const char *path, int flags, ...);\n ssize_t xread(int fd, void *buf, size_t len);\ndiff --git a/object-file.c b/object-file.c\nindex f233b440b2..b9c3219793 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1023,12 +1023,26 @@ void *xmmap_gently(void *start, size_t length,\n \treturn ret;\n }\n \n+const char *mmap_os_err(void)\n+{\n+\tstatic const char blank[] = \"\";\n+#if defined(__linux__)\n+\tif (errno == ENOMEM) {\n+\t\t/* this continues an existing error message: */\n+\t\tstatic const char enomem[] =\n+\", check sys.vm.max_map_count and/or RLIMIT_DATA\";\n+\t\treturn enomem;\n+\t}\n+#endif /* OS-specific bits */\n+\treturn blank;\n+}\n+\n void *xmmap(void *start, size_t length,\n \tint prot, int flags, int fd, off_t offset)\n {\n \tvoid *ret = xmmap_gently(start, length, prot, flags, fd, offset);\n \tif (ret == MAP_FAILED)\n-\t\tdie_errno(_(\"mmap failed\"));\n+\t\tdie_errno(_(\"mmap failed%s\"), mmap_os_err());\n \treturn ret;\n }\n \ndiff --git a/packfile.c b/packfile.c\nindex 755aa7aec5..9ef6d98292 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -652,8 +652,8 @@ unsigned char *use_pack(struct packed_git *p,\n \t\t\t\tPROT_READ, MAP_PRIVATE,\n \t\t\t\tp->pack_fd, win->offset);\n \t\t\tif (win->base == MAP_FAILED)\n-\t\t\t\tdie_errno(\"packfile %s cannot be mapped\",\n-\t\t\t\t\t  p->pack_name);\n+\t\t\t\tdie_errno(_(\"packfile %s cannot be mapped%s\"),\n+\t\t\t\t\t  p->pack_name, mmap_os_err());\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_pack_fd(p);\ndiff --git a/read-cache.c b/read-cache.c\nindex 77961a3885..a80902155c 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2236,7 +2236,8 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)\n \n \tmmap = xmmap_gently(NULL, mmap_size, PROT_READ, MAP_PRIVATE, fd, 0);\n \tif (mmap == MAP_FAILED)\n-\t\tdie_errno(_(\"%s: unable to map index file\"), path);\n+\t\tdie_errno(_(\"%s: unable to map index file%s\"), path,\n+\t\t\tmmap_os_err());\n \tclose(fd);\n \n \thdr = (const struct cache_header *)mmap;\n-- \nIt's probably not safe to feed sysadmins after midnight, though :>\n"},{"id":"428799","messageId":"YNvXTf6PG2zhbjdh@coredump.intra.peff.net","threadId":"56024","inReplyTo":"20210629081108.28657-2-e@80x24.org","subject":"Re: [PATCH 1/4] use_pack: attempt to handle ENOMEM from mmap","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-30T02:30:37Z","receivedAt":"2021-06-30T02:30:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 29, 2021 at 08:11:05AM +0000, Eric Wong wrote:\n\n> Since use_pack() can already safely munmap packs to respect\n> core.packedGitLimit, attempt to gracefully handle ENOMEM\n> errors the same way by unmapping a window and retrying.\n> \n> This benefits unprivileged users who lack permissions to raise\n> the `sys.vm.max_map_count' sysctl and/or RLIMIT_DATA resource\n> limit.\n> \n> I've also verified it is safe to release a pack here by\n> unconditionally calling unuse_one_window() before\n> xmmap_gently():\n> \n> \t--- a/packfile.c\n> \t+++ b/packfile.c\n> \t@@ -649,6 +649,7 @@ unsigned char *use_pack(struct packed_git *p,\n> \t\t\t\t\t&& unuse_one_window(p))\n> \t\t\t\t\t; /* nothing */\n> \t\t\t\tdo {\n> \t+\t\t\t\tunuse_one_window(p);\n> \t\t\t\t\twin->base = xmmap_gently(NULL, win->len,\n> \t\t\t\t\t\tPROT_READ, MAP_PRIVATE,\n> \t\t\t\t\t\tp->pack_fd, win->offset);\n\nI don't find that test-diff all that compelling, because we don't know\nwhich window will get unused. I.e., if there is one that will get racily\nunused, we might not hit it. I think it would be a lot more interesting\nfor finding problems if it did:\n\n  while (unuse_one_window(p)) ;\n\nto clear them all.\n\nThat said, I think this must be obviously correct because the code above\nwill potentially have just called unuse_one_window(p) already. So at\nleast if not obviously correct, no more buggy than the previous code. :)\n\n-Peff\n"},{"id":"428800","messageId":"YNvZ3lA6J8DIx2r4@coredump.intra.peff.net","threadId":"56024","inReplyTo":"20210629081108.28657-3-e@80x24.org","subject":"Re: [PATCH 2/4] map_loose_object_1: attempt to handle ENOMEM from mmap","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-30T02:41:34Z","receivedAt":"2021-06-30T02:41:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 29, 2021 at 08:11:06AM +0000, Eric Wong wrote:\n\n> This benefits unprivileged users who lack permissions to raise\n> the `sys.vm.max_map_count' sysctl and/or RLIMIT_DATA resource\n> limit.\n> \n> Multi-threaded callers to map_loose_object_1 (e.g. \"git grep\")\n> appear to guard against thread-safety problems.  Other\n> multi-core aware pieces of code (e.g. parallel-checkout) uses\n> child processes\n\nThis one makes me slightly more nervous than the last, because we don't\nknow if somebody higher up the call stack might be expecting to use that\nwindow again.\n\nI _think_ this one is OK, because we would never read a loose object in\nthe process of reading a packed one. I thought we might in some rare\ncases with corruption (e.g., B is a delta against A; we fail to read A\nbecause it's corrupt, so we look elsewhere for a copy). I don't think\nthe current code is quite that clever, though. If B were corrupted, we'd\nlook elsewhere for a duplicate, but that logic doesn't apply in the\nmiddle of a delta resolution (even though there are corruption cases it\nwould help recover from).\n\nSo I don't think this is introducing any bugs. But it worries me a bit\nthat it creates an opportunity for a subtle and hard-to-trigger\nsituation if we do change seemingly-unrelated code.\n\nBut looking more carefully at unuse_one_window(), I think the worst case\nis a slight performance drop, as we unmap the window and then re-map\nwhen somebody higher up the call-stack needs it again. My real worry was\nthat we could trigger a case where somebody was holding onto a pointer\nto the bytes. But I think use_pack() will mark those bytes via\ninuse_cnt, and then unuse_one_window() will never drop a window that has\na non-zero inuse_count.\n\nSo in that sense all of your patches should be correct without thinking\ntoo hard no them. Sure, a performance drop from having to re-map the\nwindow later isn't great, but if the alternative in each case is calling\ndie(), it's a strict improvement.\n\n-Peff\n"},{"id":"428801","messageId":"YNva8PWFJ24QX2yF@coredump.intra.peff.net","threadId":"56024","inReplyTo":"20210630000132.GA2653@dcvr","subject":"Re: [PATCH v2] xmmap: inform Linux users of tuning knobs on ENOMEM","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-30T02:46:08Z","receivedAt":"2021-06-30T02:46:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 30, 2021 at 12:01:32AM +0000, Eric Wong wrote:\n\n> This series is now down to a single patch.\n> \n> I wanted to make things more transparent to users without\n> privileges to raise sys.vm.max_map_count and/or RLIMIT_DATA;\n> but it doesn't seem possible to account for libc/zlib/etc. doing\n> mmap() without our knowledge (usually via malloc).\n\nOh, I should have read this one before reviewing the inner parts of v1. :)\n\nIn general I agree that trying to manage our map count can never be\nfoolproof. As you note, other parts of the system may contribute to that\ncount. But even within Git, we don't have any mechanism for unmapping\nmany non-packfiles. So if you have 30,000 packs, you may hit the limit\npurely via the .idx files (and ditto for the new .rev files, and\nprobably commit-graph files, etc).\n\nThat said, I'm not opposed to handling xmmap() failures more gracefully,\nas your series did. It's not foolproof, but it might help in some cases.\n\n> So I think giving users some information to feed their sysadmins\n> is the best we can do in this situation:\n\nThis seems OK to me, too. Translators might complain a bit about the\nmessage-lego. I don't have a strong opinion.\n\n-Peff\n"},{"id":"428805","messageId":"87czs3ga1f.fsf@evledraar.gmail.com","threadId":"56024","inReplyTo":"20210629081108.28657-3-e@80x24.org","subject":"Re: [PATCH 2/4] map_loose_object_1: attempt to handle ENOMEM from mmap","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-30T06:06:21Z","receivedAt":"2021-06-30T06:07:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jun 29 2021, Eric Wong wrote:\n\n> This benefits unprivileged users who lack permissions to raise\n> the `sys.vm.max_map_count' sysctl and/or RLIMIT_DATA resource\n> limit.\n>\n> Multi-threaded callers to map_loose_object_1 (e.g. \"git grep\")\n> appear to guard against thread-safety problems.  Other\n> multi-core aware pieces of code (e.g. parallel-checkout) uses\n> child processes\n\ns/uses/use/ & missing full-stop.\n"},{"id":"428818","messageId":"20210630100722.GB2653@dcvr","threadId":"56024","inReplyTo":"YNva8PWFJ24QX2yF@coredump.intra.peff.net","subject":"Re: [PATCH v2] xmmap: inform Linux users of tuning knobs on ENOMEM","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2021-06-30T10:07:22Z","receivedAt":"2021-06-30T10:07:26Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jeff King <peff@peff.net> wrote:\n> On Wed, Jun 30, 2021 at 12:01:32AM +0000, Eric Wong wrote:\n> \n> > This series is now down to a single patch.\n> > \n> > I wanted to make things more transparent to users without\n> > privileges to raise sys.vm.max_map_count and/or RLIMIT_DATA;\n> > but it doesn't seem possible to account for libc/zlib/etc. doing\n> > mmap() without our knowledge (usually via malloc).\n> \n> Oh, I should have read this one before reviewing the inner parts of v1. :)\n> \n> In general I agree that trying to manage our map count can never be\n> foolproof. As you note, other parts of the system may contribute to that\n> count. But even within Git, we don't have any mechanism for unmapping\n> many non-packfiles. So if you have 30,000 packs, you may hit the limit\n> purely via the .idx files (and ditto for the new .rev files, and\n> probably commit-graph files, etc).\n\nYeah, the most annoying thing with my original series was when\nI hit \"inflate: out of memory\" once I stopped xmmap from dying.\nI suspect that would be a worse far error message for users who\naren't familiar with how malloc works.\n\n> That said, I'm not opposed to handling xmmap() failures more gracefully,\n> as your series did. It's not foolproof, but it might help in some cases.\n\nI've also been wondering if we can maintain a watermark based\non reading the contents of /proc/sys/vm/max_map_count and the\nmappings we make.   Then we could start dropping mappings\nwhen we hit half (or some other threshold) of that sysctl.\nSimilar for RLIMIT_DATA (though that defaults to unlimited\non my Debian system).\n\nOTOH, I also wonder if we're overusing mmap when we could be\njust as well served with pread.\n\nI'm not up-to-date on modern mmap performance and maybe CPU\nvulnerability mitigations nowadays make mmap more compelling.\nHowever, once upon a time in 2006, pread could be a hair quicker:\n\nhttps://lore.kernel.org/git/Pine.LNX.4.64.0612182234260.3479@woody.osdl.org/T/#u\n(But that info could be out-of-date...)\n\nI'm also somewhat paranoid when it comes to mmap since rogue\nprocesses could be truncating mmap-ed files to cause bus errors.\n\n> > So I think giving users some information to feed their sysadmins\n> > is the best we can do in this situation:\n> \n> This seems OK to me, too. Translators might complain a bit about the\n> message-lego. I don't have a strong opinion.\n\n*shrug*  I saw my original patches already ended up in `seen'\n(commit 7b79212a93c375365c06cab5c0018ab97a4185cf)\n"},{"id":"428835","messageId":"YNynXZkDOXJDeR4P@coredump.intra.peff.net","threadId":"56024","inReplyTo":"20210630100722.GB2653@dcvr","subject":"Re: [PATCH v2] xmmap: inform Linux users of tuning knobs on ENOMEM","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-30T17:18:21Z","receivedAt":"2021-06-30T17:18:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 30, 2021 at 10:07:22AM +0000, Eric Wong wrote:\n\n> > In general I agree that trying to manage our map count can never be\n> > foolproof. As you note, other parts of the system may contribute to that\n> > count. But even within Git, we don't have any mechanism for unmapping\n> > many non-packfiles. So if you have 30,000 packs, you may hit the limit\n> > purely via the .idx files (and ditto for the new .rev files, and\n> > probably commit-graph files, etc).\n> \n> Yeah, the most annoying thing with my original series was when\n> I hit \"inflate: out of memory\" once I stopped xmmap from dying.\n> I suspect that would be a worse far error message for users who\n> aren't familiar with how malloc works.\n\nYep. When I've seen this problem in the wild, that was exactly the\nmessage I hit, too.\n\n> > That said, I'm not opposed to handling xmmap() failures more gracefully,\n> > as your series did. It's not foolproof, but it might help in some cases.\n> \n> I've also been wondering if we can maintain a watermark based\n> on reading the contents of /proc/sys/vm/max_map_count and the\n> mappings we make.   Then we could start dropping mappings\n> when we hit half (or some other threshold) of that sysctl.\n> Similar for RLIMIT_DATA (though that defaults to unlimited\n> on my Debian system).\n\nMaybe. You'd still need to teach Git to start closing pack idx files.\n\nOne downside of closing any maps is that it leads to races with\nsimultaneous repacks. Usually if a pack goes away we'll re-scan the\nobjects directory and look for new packs. But in pack-objects, we commit\nourselves to a certain packed representation. If that pack goes away\n(and we aren't holding on to it via an mmap or open descriptor), then we\nhave to bail.\n\nIf we hit a point where the alternative is calling die() because we\ncouldn't map or allocate something, then risking the race is OK. But\nunmapping at a more conservative level opens us up to that race even\nwhen we would have otherwise succeeded.\n\n> OTOH, I also wonder if we're overusing mmap when we could be\n> just as well served with pread.\n> \n> I'm not up-to-date on modern mmap performance and maybe CPU\n> vulnerability mitigations nowadays make mmap more compelling.\n> However, once upon a time in 2006, pread could be a hair quicker:\n\nYou can compile with NO_MMAP now to emulate it with pread(), but I doubt\nit performs that well. If you shrink the packedGitWindowSize, then I\nthink use_pack() would bring in and operate on reasonable amounts of\nbytes at a time.\n\nBut lots of other code will mmap whole files (.idx code, packed-refs\nlookup, the index). We'd end up copying those files entirely into\nmemory, rather than accessing them directly page-wise.  That increases\nRAM usage (we are copying into our own heap) and makes operations\ninherently O(n) in the file sizes (one of the big reasons we mmap the\npacked-refs file is so that we can binary search without having to\nprocess the whole thing).\n\nThose cases could be converted to use pread() more directly. Doing so\nfor .idx files might not be too bad, as the records are fixed-length and\nwe know where each step of the binary search will land. For packed-refs\nit would get pretty hairy; we jump to a particular offset and then have\nto scan around (sometimes backwards) for the beginning of the record.\n\n> > > So I think giving users some information to feed their sysadmins\n> > > is the best we can do in this situation:\n> > \n> > This seems OK to me, too. Translators might complain a bit about the\n> > message-lego. I don't have a strong opinion.\n> \n> *shrug*  I saw my original patches already ended up in `seen'\n> (commit 7b79212a93c375365c06cab5c0018ab97a4185cf)\n\nThat doesn't necessarily mean much. Only that the topic was \"seen\" by\nthe maintainer and didn't look like complete garbage. :)\n\n-Peff\n"}]}