{"thread":{"id":"53302","subject":"[PATCH 0/4] commit-graph: handle file descriptor exhaustion","startedAt":"2020-04-23T21:41:02Z","lastAt":"2021-06-24T15:53:02Z","messageCount":14,"participants":["Taylor Blau","Junio C Hamano","Jeff King","Derrick Stolee","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"396045","messageId":"cover.1587677671.git.me@ttaylorr.com","threadId":"53302","inReplyTo":null,"subject":"[PATCH 0/4] commit-graph: handle file descriptor exhaustion","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-23T21:40:57Z","receivedAt":"2020-04-23T21:41:02Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\nThis is another series to handle an issue we started seeing at GitHub on\nrepositories that have too many commit-graph layers.\n\nThis issue evaded me for several days, but the gist is that having too\nmany descriptors open caused us to fail 'git_mkstemp_mode', which in\nturn left some values in 'ctx->commit_graph_filenames_after'\nuninitialized. This in turn caused a general protection fault when\ntrying to free these uninitialized entries. Frustratingly, it causes\nstray 'commit-graph-chain.lock' files to stick around even after the\nprocess has gone away, causing all subsequent commit-graph write\noperations to fail.\n\nThe first two patches are semi-related cleanups and other preparation.\nThe third patch fixes the main issue described here. Peff contributed\nthe fourth patch which teaches the commit-graph machinery to be more\nwilling to close file descriptors.\n\nThese patches are very much owed to the help that Stolee and Peff\nprovided while writing them and for their willingness to let me bounce\nsome ideas off of them.\n\nThanks in advance for your review.\n\nJeff King (1):\n commit-graph: close descriptors after mmap\n\nTaylor Blau (3):\n commit-graph.c: don't use discarded graph_name in error\n t/test-lib.sh: make ULIMIT_FILE_DESCRIPTORS available to tests\n commit-graph.c: gracefully handle file descriptor exhaustion\n\n commit-graph.c | 21 ++++++++-------------\n commit-graph.h | 3 +--\n fuzz-commit-graph.c | 5 ++---\n t/t1400-update-ref.sh | 9 ---------\n t/t5324-split-commit-graph.sh | 13 +++++++++++++\n t/test-lib.sh | 9 +++++++++\n 6 files changed, 33 insertions(+), 27 deletions(-)\n\n--\n2.26.0.113.ge9739cdccc\n"},{"id":"396046","messageId":"17dff6142e62d202794986f0bd51e4a8604f8e49.1587677671.git.me@ttaylorr.com","threadId":"53302","inReplyTo":"cover.1587677671.git.me@ttaylorr.com","subject":"[PATCH 1/4] commit-graph.c: don't use discarded graph_name in error","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-23T21:41:02Z","receivedAt":"2020-04-23T21:41:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When writing a commit-graph layer, we do so in a temporary file which is\nrenamed into place. If we fail to create a temporary file, for e.g.,\nbecause we have too many open files, then 'git_mkstemp_mode' sets the\npattern to the empty string, in which case we get an error something\nalong the lines of:\n\n error: unable to create ''\n\nIt's not useful to show the pattern here at all, since we (1) know the\npattern is well-formed, and (2) would have already shown the dirname\nwhen trying to create the leading directories. So, replace this error\nwith something friendlier.\n\nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex b8737f0ce9..afde075962 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1397,7 +1397,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \n \t\tfd = git_mkstemp_mode(ctx->graph_name, 0444);\n \t\tif (fd < 0) {\n-\t\t\terror(_(\"unable to create '%s'\"), ctx->graph_name);\n+\t\t\terror(_(\"unable to create temporary graph layer\"));\n \t\t\treturn -1;\n \t\t}\n \n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"396047","messageId":"b374ff0490a9de2708943f41921e55e6a36d72e1.1587677671.git.me@ttaylorr.com","threadId":"53302","inReplyTo":"cover.1587677671.git.me@ttaylorr.com","subject":"[PATCH 2/4] t/test-lib.sh: make ULIMIT_FILE_DESCRIPTORS available to tests","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-23T21:41:06Z","receivedAt":"2020-04-23T21:41:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In t1400 the prerequisite 'ULIMIT_FILE_DESCRIPTORS' is defined and used\nto effectively guard the helper function 'run_with_limited_open_files'\nfrom being used on systems that do not satisfy this prerequisite.\n\nIn the subsequent patch, we will introduce another test outside of t1400\nthat would benefit from using this prerequisite. So, move it to\n'test-lib.sh' instead so that it can be used by multiple tests.\n\nSigned-off-by: Taylor Blau \n---\n t/t1400-update-ref.sh | 9 ---------\n t/test-lib.sh | 9 +++++++++\n 2 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex a6224ef65f..1e7428a379 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -1354,15 +1354,6 @@ test_expect_success 'fails with duplicate ref update via symref' '\n \ttest_cmp expect actual\n '\n \n-run_with_limited_open_files () {\n-\t(ulimit -n 32 && \"$@\")\n-}\n-\n-test_lazy_prereq ULIMIT_FILE_DESCRIPTORS '\n-\ttest_have_prereq !MINGW,!CYGWIN &&\n-\trun_with_limited_open_files true\n-'\n-\n test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '\n (\n \tfor i in $(test_seq 33)\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 9fe390bd5a..03bf2a4b93 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1618,6 +1618,15 @@ test_lazy_prereq ULIMIT_STACK_SIZE '\n \trun_with_limited_stack true\n '\n \n+run_with_limited_open_files () {\n+\t(ulimit -n 32 && \"$@\")\n+}\n+\n+test_lazy_prereq ULIMIT_FILE_DESCRIPTORS '\n+\ttest_have_prereq !MINGW,!CYGWIN &&\n+\trun_with_limited_open_files true\n+'\n+\n build_option () {\n \tgit version --build-options |\n \tsed -ne \"s/^$1: //p\"\n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"396048","messageId":"e05db264cb50760cab222157b436e82adeaeadc8.1587677671.git.me@ttaylorr.com","threadId":"53302","inReplyTo":"cover.1587677671.git.me@ttaylorr.com","subject":"[PATCH 4/4] commit-graph: close descriptors after mmap","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-23T21:41:13Z","receivedAt":"2020-04-23T21:41:20Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"From: Jeff King \n\nWe don't ever refer to the descriptor after mmap-ing it. And keeping it\nopen means we can run out of descriptors in degenerate cases (e.g.,\nthousands of split chain files). Let's close it as soon as possible.\n\nSigned-off-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 15 +++++----------\n commit-graph.h | 3 +--\n fuzz-commit-graph.c | 5 ++---\n 3 files changed, 8 insertions(+), 15 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex b2d2fdfe3d..e9b458539f 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -69,7 +69,6 @@ static uint8_t oid_version(void)\n static struct commit_graph *alloc_commit_graph(void)\n {\n \tstruct commit_graph *g = xcalloc(1, sizeof(*g));\n-\tg->graph_fd = -1;\n \n \treturn g;\n }\n@@ -123,14 +122,13 @@ struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st,\n \t\treturn NULL;\n \t}\n \tgraph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);\n-\tret = parse_commit_graph(graph_map, fd, graph_size);\n+\tclose(fd);\n+\tret = parse_commit_graph(graph_map, graph_size);\n \n \tif (ret)\n \t\tret->odb = odb;\n-\telse {\n+\telse\n \t\tmunmap(graph_map, graph_size);\n-\t\tclose(fd);\n-\t}\n \n \treturn ret;\n }\n@@ -165,8 +163,7 @@ static int verify_commit_graph_lite(struct commit_graph *g)\n \treturn 0;\n }\n \n-struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n-\t\t\t\t\tsize_t graph_size)\n+struct commit_graph *parse_commit_graph(void *graph_map, size_t graph_size)\n {\n \tconst unsigned char *data, *chunk_lookup;\n \tuint32_t i;\n@@ -209,7 +206,6 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \n \tgraph->hash_len = the_hash_algo->rawsz;\n \tgraph->num_chunks = *(unsigned char*)(data + 6);\n-\tgraph->graph_fd = fd;\n \tgraph->data = graph_map;\n \tgraph->data_len = graph_size;\n \n@@ -2125,10 +2121,9 @@ void free_commit_graph(struct commit_graph *g)\n {\n \tif (!g)\n \t\treturn;\n-\tif (g->graph_fd >= 0) {\n+\tif (g->data) {\n \t\tmunmap((void *)g->data, g->data_len);\n \t\tg->data = NULL;\n-\t\tclose(g->graph_fd);\n \t}\n \tfree(g->filename);\n \tfree(g);\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 98ef121924..a0a2c4a1e5 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -66,8 +66,7 @@ struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st,\n \t\t\t\t\t\t struct object_directory *odb);\n struct commit_graph *read_commit_graph_one(struct repository *r,\n \t\t\t\t\t struct object_directory *odb);\n-struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n-\t\t\t\t\tsize_t graph_size);\n+struct commit_graph *parse_commit_graph(void *graph_map, size_t graph_size);\n \n /*\n * Return 1 if and only if the repository has a commit-graph\ndiff --git a/fuzz-commit-graph.c b/fuzz-commit-graph.c\nindex 0157acbf2e..9fd1c04edd 100644\n--- a/fuzz-commit-graph.c\n+++ b/fuzz-commit-graph.c\n@@ -1,8 +1,7 @@\n #include \"commit-graph.h\"\n #include \"repository.h\"\n \n-struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n-\t\t\t\t\tsize_t graph_size);\n+struct commit_graph *parse_commit_graph(void *graph_map, size_t graph_size);\n \n int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);\n \n@@ -11,7 +10,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)\n \tstruct commit_graph *g;\n \n \tinitialize_the_repository();\n-\tg = parse_commit_graph((void *)data, -1, size);\n+\tg = parse_commit_graph((void *)data, size);\n \trepo_clear(the_repository);\n \tfree(g);\n \n-- \n2.26.0.113.ge9739cdccc\n"},{"id":"396049","messageId":"2b8ee726690861405f41adede5582b96749e98c5.1587677671.git.me@ttaylorr.com","threadId":"53302","inReplyTo":"cover.1587677671.git.me@ttaylorr.com","subject":"[PATCH 3/4] commit-graph.c: gracefully handle file descriptor exhaustion","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-23T21:41:09Z","receivedAt":"2020-04-23T21:42:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When writing a layered commit-graph, the commit-graph machinery uses\n'commit_graph_filenames_after' and 'commit_graph_hash_after' to keep\ntrack of the layers in the chain that we are in the process of writing.\n\nWhen the number of commit-graph layers shrinks, we initialize all\nentries in the aforementioned arrays, because we know the structure of\nthe new commit-graph chain immediately (since there are no new layers,\nthere are no unknown hash values).\n\nBut when the number of commit-graph layers grows (i.e., that\n'num_commit_graphs_after > num_commit_graphs_before'), then we leave\nsome entries in the filenames and hashes arrays as uninitialized,\nbecause we will fill them in later as those values become available.\n\nFor instance, we rely on 'write_commit_graph_file's to store the\nfilename and hash of the last layer in the new chain, which is the one\nthat it is responsible for writing. But, it's possible that\n'write_commit_graph_file' may fail, e.g., from file descriptor\nexhaustion. In this case it is possible that 'git_mkstemp_mode' will\nfail, and that function will return early *before* setting the values\nfor the last commit-graph layer's filename and hash.\n\nThis causes a number of upleasant side-effects. For instance, trying to\n'free()' each entry in 'ctx->commit_graph_filenames_after' (and\nsimilarly for the hashes array) causes us to 'free()' uninitialized\nmemory, since the area is allocated with 'malloc()' and is therefore\nsubject to contain garbage (which is left alone when\n'write_commit_graph_file' returns early).\n\nThis can manifest in other issues, like a general protection fault,\nand/or leaving a stray 'commit-graph-chain.lock' around after the\nprocess dies. (The reasoning for this is still a mystery to me, since\nwe'd otherwise usually expect the kernel to run tempfile.c's 'atexit()'\nhandlers in the case of a normal death...)\n\nTo resolve this, initialize the memory with 'CALLOC_ARRAY' so that\nuninitialized entries are filled with zeros, and can thus be 'free()'d\nas a noop instead of causing a fault.\n\nHelped-by: Jeff King \nHelped-by: Derrick Stolee \nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 4 ++--\n t/t5324-split-commit-graph.sh | 13 +++++++++++++\n 2 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex afde075962..b2d2fdfe3d 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1602,8 +1602,8 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n \t\tfree(old_graph_name);\n \t}\n \n-\tALLOC_ARRAY(ctx->commit_graph_filenames_after, ctx->num_commit_graphs_after);\n-\tALLOC_ARRAY(ctx->commit_graph_hash_after, ctx->num_commit_graphs_after);\n+\tCALLOC_ARRAY(ctx->commit_graph_filenames_after, ctx->num_commit_graphs_after);\n+\tCALLOC_ARRAY(ctx->commit_graph_hash_after, ctx->num_commit_graphs_after);\n \n \tfor (i = 0; i < ctx->num_commit_graphs_after &&\n \t\t i < ctx->num_commit_graphs_before; i++)\ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex e5d8d64170..0d1db31b0a 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -381,4 +381,17 @@ test_expect_success '--split=replace replaces the chain' '\n \tgraph_read_expect 2\n '\n \n+test_expect_success ULIMIT_FILE_DESCRIPTORS 'handles file descriptor exhaustion' '\n+\tgit init ulimit &&\n+\t(\n+\t\tcd ulimit &&\n+\t\tfor i in $(test_seq 64)\n+\t\tdo\n+\t\t\ttest_commit $i &&\n+\t\t\ttest_might_fail run_with_limited_open_files git commit-graph write \\\n+\t\t\t\t--split=no-merge --reachable || return 1\n+\t\tdone\n+\t)\n+'\n+\n test_done\n-- \n2.26.0.113.ge9739cdccc\n\n"},{"id":"396050","messageId":"20200423214305.GA48325@syl.local","threadId":"53302","inReplyTo":"cover.1587677671.git.me@ttaylorr.com","subject":"Re: [PATCH 0/4] commit-graph: handle file descriptor exhaustion","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-23T21:43:05Z","receivedAt":"2020-04-23T21:43:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"...I should mention that this is based on my series in [1], since it's\nmuch easier to express the new test in t5324 with '--split=no-merge'.\n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/cover.1586836700.git.me@ttaylorr.com/\n"},{"id":"396055","messageId":"xmqq368tg8po.fsf@gitster.c.googlers.com","threadId":"53302","inReplyTo":"e05db264cb50760cab222157b436e82adeaeadc8.1587677671.git.me@ttaylorr.com","subject":"Re: [PATCH 4/4] commit-graph: close descriptors after mmap","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-23T22:04:19Z","receivedAt":"2020-04-23T22:04:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> From: Jeff King \n>\n> We don't ever refer to the descriptor after mmap-ing it. And keeping it\n> open means we can run out of descriptors in degenerate cases (e.g.,\n> thousands of split chain files). Let's close it as soon as possible.\n\nYikes. \n\nSorry, I should have looked at the use of mmap in this topioc more\ncarefully when we queued the series. It is an easy mistake to make\nby anybody new to the API to leave it open while the region is in\nuse.\n\nWith this fix, with or without the other topics still in flight, I\ndo not think no code touches graph_fd. Should we remove the\ngraph_fd field from the structure as well?\n\n\n\n\n commit-graph.h | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/commit-graph.h b/commit-graph.h\nindex a0a2c4a1e5..1254eae948 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -40,8 +40,6 @@ struct tree *get_commit_tree_in_graph(struct repository *r,\n \t\t\t\t const struct commit *c);\n \n struct commit_graph {\n-\tint graph_fd;\n-\n \tconst unsigned char *data;\n \tsize_t data_len;\n \n"},{"id":"396093","messageId":"20200424035628.GA1648190@coredump.intra.peff.net","threadId":"53302","inReplyTo":"xmqq368tg8po.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 4/4] commit-graph: close descriptors after mmap","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-24T03:56:28Z","receivedAt":"2020-04-24T03:56:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 23, 2020 at 03:04:19PM -0700, Junio C Hamano wrote:\n\n> With this fix, with or without the other topics still in flight, I\n> do not think no code touches graph_fd. Should we remove the\n> graph_fd field from the structure as well?\n\nOops, I thought I did.\n\n> diff --git a/commit-graph.h b/commit-graph.h\n> index a0a2c4a1e5..1254eae948 100644\n> --- a/commit-graph.h\n> +++ b/commit-graph.h\n> @@ -40,8 +40,6 @@ struct tree *get_commit_tree_in_graph(struct repository *r,\n> \t\t\t\t const struct commit *c);\n> \n> struct commit_graph {\n> -\tint graph_fd;\n> -\n\nYes, this should definitely be squashed in.\n\n-Peff\n"},{"id":"396129","messageId":"2232c379-d0ec-0b52-96b4-379438642785@gmail.com","threadId":"53302","inReplyTo":"xmqq368tg8po.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 4/4] commit-graph: close descriptors after mmap","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-04-24T13:17:16Z","receivedAt":"2020-04-24T13:17:21Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/23/2020 6:04 PM, Junio C Hamano wrote:\n> Taylor Blau writes:\n> \n>> From: Jeff King \n>>\n>> We don't ever refer to the descriptor after mmap-ing it. And keeping it\n>> open means we can run out of descriptors in degenerate cases (e.g.,\n>> thousands of split chain files). Let's close it as soon as possible.\n> \n> Yikes. \n> \n> Sorry, I should have looked at the use of mmap in this topioc more\n> carefully when we queued the series. It is an easy mistake to make\n> by anybody new to the API to leave it open while the region is in\n> use.\n\nYou are right. I was new when first contributing the commit-graph. It\nwas also easier to miss because we only had one commit-graph open at\nthe time. Adding in the incremental file format led to multiple file\ndescriptors being open.\n\nHowever, this idea of closing a descriptor after an mmap is new to\nme. So I thought about other situations where I made the same mistake.\nPlease see the patch below.\n\n> With this fix, with or without the other topics still in flight, I\n> do not think no code touches graph_fd. Should we remove the\n> graph_fd field from the structure as well?\n\nI agree that this should be done.\n\nThanks,\n-Stolee\n\n-->8--\n\nFrom: Derrick Stolee \nDate: Fri, 24 Apr 2020 13:11:13 +0000\nSubject: [PATCH] multi-pack-index: close file descriptor after mmap\n\nWe recently discovered that the commit-graph was not closing its\nfile descriptor after memory-mapping the file contents. After this\nmmap() succeeds, there is no need to keep the file descriptor open.\nIn fact, there is signficant reason to close it so we do not run\nout of descriptors.\n\nThis is entirely my fault for not knowing that we can have an open\nmmap while also closing the descriptor. Some could blame this on\nbeing a new contributor when the series was first submitted, but\neven years later this is still new information to me.\n\nThat made me realize that I used the same pattern when opening a\nmulti-pack-index. Since this file is not (yet) incremental, there\nwill be at most one descriptor open for this reason. It is still\nworth fixing, especially if we extend the format to be incremental\nlike the commit-graph.\n\nSigned-off-by: Derrick Stolee \n---\n midx.c | 4 +---\n midx.h | 2 --\n 2 files changed, 1 insertion(+), 5 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 1527e464a7..60d30e873b 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -72,9 +72,9 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n \tFREE_AND_NULL(midx_name);\n \n \tmidx_map = xmmap(NULL, midx_size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\tclose(fd);\n \n \tFLEX_ALLOC_STR(m, object_dir, object_dir);\n-\tm->fd = fd;\n \tm->data = midx_map;\n \tm->data_len = midx_size;\n \tm->local = local;\n@@ -190,8 +190,6 @@ void close_midx(struct multi_pack_index *m)\n \t\treturn;\n \n \tmunmap((unsigned char *)m->data, m->data_len);\n-\tclose(m->fd);\n-\tm->fd = -1;\n \n \tfor (i = 0; i < m->num_packs; i++) {\n \t\tif (m->packs[i])\ndiff --git a/midx.h b/midx.h\nindex e6fa356b5c..b18cf53bc4 100644\n--- a/midx.h\n+++ b/midx.h\n@@ -12,8 +12,6 @@ struct repository;\n struct multi_pack_index {\n \tstruct multi_pack_index *next;\n \n-\tint fd;\n-\n \tconst unsigned char *data;\n \tsize_t data_len;\n \n-- \n2.26.2\n\n"},{"id":"396145","messageId":"20200424163553.GA58219@syl.local","threadId":"53302","inReplyTo":"2232c379-d0ec-0b52-96b4-379438642785@gmail.com","subject":"Re: [PATCH 4/4] commit-graph: close descriptors after mmap","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-24T16:35:53Z","receivedAt":"2020-04-24T16:36:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Apr 24, 2020 at 09:17:16AM -0400, Derrick Stolee wrote:\n> On 4/23/2020 6:04 PM, Junio C Hamano wrote:\n> > Taylor Blau writes:\n> >\n> >> From: Jeff King \n> >>\n> >> We don't ever refer to the descriptor after mmap-ing it. And keeping it\n> >> open means we can run out of descriptors in degenerate cases (e.g.,\n> >> thousands of split chain files). Let's close it as soon as possible.\n> >\n> > Yikes.\n> >\n> > Sorry, I should have looked at the use of mmap in this topioc more\n> > carefully when we queued the series. It is an easy mistake to make\n> > by anybody new to the API to leave it open while the region is in\n> > use.\n>\n> You are right. I was new when first contributing the commit-graph. It\n> was also easier to miss because we only had one commit-graph open at\n> the time. Adding in the incremental file format led to multiple file\n> descriptors being open.\n>\n> However, this idea of closing a descriptor after an mmap is new to\n> me. So I thought about other situations where I made the same mistake.\n> Please see the patch below.\n\nIt's new to me, too :). If I had known it beforehand, then I would have\nwritten the fourth patch here myself. But, I didn't, so I am grateful to\nPeff for teaching me something new here.\n\n> > With this fix, with or without the other topics still in flight, I\n> > do not think no code touches graph_fd. Should we remove the\n> > graph_fd field from the structure as well?\n>\n> I agree that this should be done.\n>\n> Thanks,\n> -Stolee\n>\n> -->8--\n\nFor what it's worth, this didn't apply quite right with 'git am -3 -c',\nsince it didn't seem to recognize that this was your scissors line. If I\nedit your mail myself by replacing this line with '-- >8 --', then 'git\nam' applies it just fine.\n\n> From: Derrick Stolee \n> Date: Fri, 24 Apr 2020 13:11:13 +0000\n> Subject: [PATCH] multi-pack-index: close file descriptor after mmap\n>\n> We recently discovered that the commit-graph was not closing its\n> file descriptor after memory-mapping the file contents. After this\n> mmap() succeeds, there is no need to keep the file descriptor open.\n> In fact, there is signficant reason to close it so we do not run\n> out of descriptors.\n>\n> This is entirely my fault for not knowing that we can have an open\n> mmap while also closing the descriptor. Some could blame this on\n> being a new contributor when the series was first submitted, but\n> even years later this is still new information to me.\n>\n> That made me realize that I used the same pattern when opening a\n> multi-pack-index. Since this file is not (yet) incremental, there\n> will be at most one descriptor open for this reason. It is still\n> worth fixing, especially if we extend the format to be incremental\n> like the commit-graph.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> midx.c | 4 +---\n> midx.h | 2 --\n> 2 files changed, 1 insertion(+), 5 deletions(-)\n>\n> diff --git a/midx.c b/midx.c\n> index 1527e464a7..60d30e873b 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -72,9 +72,9 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n> \tFREE_AND_NULL(midx_name);\n>\n> \tmidx_map = xmmap(NULL, midx_size, PROT_READ, MAP_PRIVATE, fd, 0);\n> +\tclose(fd);\n\nRight, we want to close as soon as we have mmaped the file.\n\n>\n> \tFLEX_ALLOC_STR(m, object_dir, object_dir);\n> -\tm->fd = fd;\n> \tm->data = midx_map;\n> \tm->data_len = midx_size;\n> \tm->local = local;\n> @@ -190,8 +190,6 @@ void close_midx(struct multi_pack_index *m)\n> \t\treturn;\n>\n> \tmunmap((unsigned char *)m->data, m->data_len);\n> -\tclose(m->fd);\n> -\tm->fd = -1;\n\n...and not down here. Thanks.\n\n> \tfor (i = 0; i < m->num_packs; i++) {\n> \t\tif (m->packs[i])\n> diff --git a/midx.h b/midx.h\n> index e6fa356b5c..b18cf53bc4 100644\n> --- a/midx.h\n> +++ b/midx.h\n> @@ -12,8 +12,6 @@ struct repository;\n> struct multi_pack_index {\n> \tstruct multi_pack_index *next;\n>\n> -\tint fd;\n> -\n\n:). Even better!\n\n> \tconst unsigned char *data;\n> \tsize_t data_len;\n>\n> --\n> 2.26.2\n\nThis looks great to me, and thanks for being proactive about the fix.\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"396172","messageId":"xmqqmu70d547.fsf@gitster.c.googlers.com","threadId":"53302","inReplyTo":"2232c379-d0ec-0b52-96b4-379438642785@gmail.com","subject":"Re: [PATCH 4/4] commit-graph: close descriptors after mmap","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-24T20:02:32Z","receivedAt":"2020-04-24T20:02:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> From: Derrick Stolee \n> Date: Fri, 24 Apr 2020 13:11:13 +0000\n> Subject: [PATCH] multi-pack-index: close file descriptor after mmap\n>\n> We recently discovered that the commit-graph was not closing its\n> file descriptor after memory-mapping the file contents. After this\n> mmap() succeeds, there is no need to keep the file descriptor open.\n> In fact, there is signficant reason to close it so we do not run\n> out of descriptors.\n\nThe above is sufficient a justification. Let's leave the remaining\ntwo paragraphs under three-dashes.\n\n> This is entirely my fault for not knowing that we can have an open\n> mmap while also closing the descriptor. Some could blame this on\n> being a new contributor when the series was first submitted, but\n> even years later this is still new information to me.\n>\n> That made me realize that I used the same pattern when opening a\n> multi-pack-index. Since this file is not (yet) incremental, there\n> will be at most one descriptor open for this reason. It is still\n> worth fixing, especially if we extend the format to be incremental\n> like the commit-graph.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> midx.c | 4 +---\n> midx.h | 2 --\n> 2 files changed, 1 insertion(+), 5 deletions(-)\n>\n> diff --git a/midx.c b/midx.c\n> index 1527e464a7..60d30e873b 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -72,9 +72,9 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n> \tFREE_AND_NULL(midx_name);\n> \n> \tmidx_map = xmmap(NULL, midx_size, PROT_READ, MAP_PRIVATE, fd, 0);\n> +\tclose(fd);\n> \n> \tFLEX_ALLOC_STR(m, object_dir, object_dir);\n> -\tm->fd = fd;\n> \tm->data = midx_map;\n> \tm->data_len = midx_size;\n> \tm->local = local;\n> @@ -190,8 +190,6 @@ void close_midx(struct multi_pack_index *m)\n> \t\treturn;\n> \n> \tmunmap((unsigned char *)m->data, m->data_len);\n> -\tclose(m->fd);\n> -\tm->fd = -1;\n> \n> \tfor (i = 0; i < m->num_packs; i++) {\n> \t\tif (m->packs[i])\n> diff --git a/midx.h b/midx.h\n> index e6fa356b5c..b18cf53bc4 100644\n> --- a/midx.h\n> +++ b/midx.h\n> @@ -12,8 +12,6 @@ struct repository;\n> struct multi_pack_index {\n> \tstruct multi_pack_index *next;\n> \n> -\tint fd;\n> -\n> \tconst unsigned char *data;\n> \tsize_t data_len;\n"},{"id":"396322","messageId":"7f344c3c-98e1-fc3c-4018-46e64f8010c0@gmail.com","threadId":"53302","inReplyTo":"xmqqmu70d547.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 4/4] commit-graph: close descriptors after mmap","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-04-27T10:57:54Z","receivedAt":"2020-04-27T10:57:58Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/24/2020 4:02 PM, Junio C Hamano wrote:\n> Derrick Stolee writes:\n> \n>> From: Derrick Stolee \n>> Date: Fri, 24 Apr 2020 13:11:13 +0000\n>> Subject: [PATCH] multi-pack-index: close file descriptor after mmap\n>>\n>> We recently discovered that the commit-graph was not closing its\n>> file descriptor after memory-mapping the file contents. After this\n>> mmap() succeeds, there is no need to keep the file descriptor open.\n>> In fact, there is signficant reason to close it so we do not run\n>> out of descriptors.\n> \n> The above is sufficient a justification. Let's leave the remaining\n> two paragraphs under three-dashes.\n\nWorks for me! I also thought there were too many first-person pronouns,\nbut erred on the side of reporting \"how did we figure out this was an\nissue?\" in the message.\n\n-Stolee\n"},{"id":"428365","messageId":"87im231sj6.fsf@evledraar.gmail.com","threadId":"53302","inReplyTo":"2b8ee726690861405f41adede5582b96749e98c5.1587677671.git.me@ttaylorr.com","subject":"t5324-split-commit-graph.sh flaky due to assumptions about ulimit behavior","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-24T09:51:09Z","receivedAt":"2021-06-24T10:11:30Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Apr 23 2020, Taylor Blau wrote:\n\n> When writing a layered commit-graph, the commit-graph machinery uses\n> 'commit_graph_filenames_after' and 'commit_graph_hash_after' to keep\n> track of the layers in the chain that we are in the process of writing.\n>\n> When the number of commit-graph layers shrinks, we initialize all\n> entries in the aforementioned arrays, because we know the structure of\n> the new commit-graph chain immediately (since there are no new layers,\n> there are no unknown hash values).\n>\n> But when the number of commit-graph layers grows (i.e., that\n> 'num_commit_graphs_after > num_commit_graphs_before'), then we leave\n> some entries in the filenames and hashes arrays as uninitialized,\n> because we will fill them in later as those values become available.\n>\n> For instance, we rely on 'write_commit_graph_file's to store the\n> filename and hash of the last layer in the new chain, which is the one\n> that it is responsible for writing. But, it's possible that\n> 'write_commit_graph_file' may fail, e.g., from file descriptor\n> exhaustion. In this case it is possible that 'git_mkstemp_mode' will\n> fail, and that function will return early *before* setting the values\n> for the last commit-graph layer's filename and hash.\n>\n> This causes a number of upleasant side-effects. For instance, trying to\n> 'free()' each entry in 'ctx->commit_graph_filenames_after' (and\n> similarly for the hashes array) causes us to 'free()' uninitialized\n> memory, since the area is allocated with 'malloc()' and is therefore\n> subject to contain garbage (which is left alone when\n> 'write_commit_graph_file' returns early).\n>\n> This can manifest in other issues, like a general protection fault,\n> and/or leaving a stray 'commit-graph-chain.lock' around after the\n> process dies. (The reasoning for this is still a mystery to me, since\n> we'd otherwise usually expect the kernel to run tempfile.c's 'atexit()'\n> handlers in the case of a normal death...)\n>\n> To resolve this, initialize the memory with 'CALLOC_ARRAY' so that\n> uninitialized entries are filled with zeros, and can thus be 'free()'d\n> as a noop instead of causing a fault.\n> [...]\n> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\n> index e5d8d64170..0d1db31b0a 100755\n> --- a/t/t5324-split-commit-graph.sh\n> +++ b/t/t5324-split-commit-graph.sh\n> @@ -381,4 +381,17 @@ test_expect_success '--split=replace replaces the chain' '\n> \tgraph_read_expect 2\n> '\n> \n> +test_expect_success ULIMIT_FILE_DESCRIPTORS 'handles file descriptor exhaustion' '\n> +\tgit init ulimit &&\n> +\t(\n> +\t\tcd ulimit &&\n> +\t\tfor i in $(test_seq 64)\n> +\t\tdo\n> +\t\t\ttest_commit $i &&\n> +\t\t\ttest_might_fail run_with_limited_open_files git commit-graph write \\\n> +\t\t\t\t--split=no-merge --reachable || return 1\n> +\t\tdone\n> +\t)\n> +'\n> +\n> test_done\n\nThis test started failing for me, and now I can't reproduce it anymore,\nbut I reproduced enough of it to think it was odd that it hadn't failed\nbefore.\n\nI.e. here we do an \"ulimit -n 32\" and then run a command, which makes a\nlot of assumptions about how git is compiled, starts up etc, a lot of\nwhich are outside of our control and up to the OS. It's not 32 files we\nopen, but 32 everything. When I could reproduce this it was failing on\nopening some libpcre file or other, so maybe I linked to one too many\nlibraries.\n\nThe one other test that uses this pattern seems like it could be\nsimilarly affected, but I haven't had that fail: d415ad022d8\n(update-ref: test handling large transactions properly, 2015-04-14)\n\nSince I can't reproduce this anymore maybe I'm reporting a\nnothingburger. I just wonder if this can really work reliably in the\ngeneral case, and whether a reliably version of this pattern doesn't\nneed to be one/some of:\n\n 1. Some GIT_TEST_* mode that sets the (unportable) ulimit itself in the\n code, after we reach some point. This is how e.g. web-based REPLs\n often work, load all your known libraries, forbid any file openings\n (or just N number) after that.\n\n 2. Ditto, but have the GIT_TEST_* print to stderr if we reach some\n \"checkpoint\", have the test only run under limit N if we can reach\n that point (i.e. binary search or brute force to find the exact N\n limit).\n\n 3. Maybe we can assume this would work reliably in cases of a really\n high limit of N, i.e. the d415ad022d8 test doesn't do this, but my\n understanding of it is that we're trying to guard against having all\n loose refs opened at once. So if we create e.g. 2k refs and operate\n on them we can set the limit to \"1999\".\n\n That's still assuming the same things about ulimit that make/made\n this test flaky, but we can probably safely assume that just getting\n to \"git \" being invoked won't open >1k files, but maybe\n not >32.\n\n\n"},{"id":"428384","messageId":"YNSqW3G5Sc/gBWAs@coredump.intra.peff.net","threadId":"53302","inReplyTo":"87im231sj6.fsf@evledraar.gmail.com","subject":"Re: t5324-split-commit-graph.sh flaky due to assumptions about ulimit behavior","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-24T15:52:59Z","receivedAt":"2021-06-24T15:53:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 24, 2021 at 11:51:09AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> I.e. here we do an \"ulimit -n 32\" and then run a command, which makes a\n> lot of assumptions about how git is compiled, starts up etc, a lot of\n> which are outside of our control and up to the OS. It's not 32 files we\n> open, but 32 everything. When I could reproduce this it was failing on\n> opening some libpcre file or other, so maybe I linked to one too many\n> libraries.\n> \n> The one other test that uses this pattern seems like it could be\n> similarly affected, but I haven't had that fail: d415ad022d8\n> (update-ref: test handling large transactions properly, 2015-04-14)\n> \n> Since I can't reproduce this anymore maybe I'm reporting a\n> nothingburger. I just wonder if this can really work reliably in the\n> general case, and whether a reliably version of this pattern doesn't\n> need to be one/some of:\n> \n> 1. Some GIT_TEST_* mode that sets the (unportable) ulimit itself in the\n> code, after we reach some point. This is how e.g. web-based REPLs\n> often work, load all your known libraries, forbid any file openings\n> (or just N number) after that.\n> \n> 2. Ditto, but have the GIT_TEST_* print to stderr if we reach some\n> \"checkpoint\", have the test only run under limit N if we can reach\n> that point (i.e. binary search or brute force to find the exact N\n> limit).\n> \n> 3. Maybe we can assume this would work reliably in cases of a really\n> high limit of N, i.e. the d415ad022d8 test doesn't do this, but my\n> understanding of it is that we're trying to guard against having all\n> loose refs opened at once. So if we create e.g. 2k refs and operate\n> on them we can set the limit to \"1999\".\n> \n> That's still assuming the same things about ulimit that make/made\n> this test flaky, but we can probably safely assume that just getting\n> to \"git \" being invoked won't open >1k files, but maybe\n> not >32.\n\nYes, we could probably just set the \"32\" a bit higher. Something like\n\"128\" may be more conservative. I'd be hesitant to go as high as\nsomething like 1999; system defaults are often much lower than that\nanyway and may get rejected. We may have to adjust the tests to match\nthe idea of what's \"a lot of descriptors\". (Likewise, the \"ulimit -s\"\nstuff has to make the same guess).\n\nThe prereq also tries running with the lower ulimit, but it only runs\n\"true\". Perhaps running something like \"git version\" would give us a\nmore realistic idea of the baseline for running a git command (in which\ncase the test would be auto-skipped if somehow 32 descriptors isn't\nenough on some system). That's not foolproof, though (it might take 31\nto run \"git version\", but 33 to run \"update-ref\" or something).\n\nI'm willing to let it lie unless you have a current problem. This is all\nknown to be guesswork/heuristics, and the hope was that it simply\nwouldn't come up. If it's causing even occasional pain we might need to\ndeal with it. But if it's an ephemeral thing that maybe went away, I'm\ncontent to stick my fingers back in my ears. :)\n\n-Peff\n"}]}