{"thread":{"id":"60701","subject":"[PATCH 0/4] reftable: optimize I/O patterns","startedAt":"2024-01-08T12:18:32Z","lastAt":"2024-01-16T16:44:22Z","messageCount":22,"participants":["Patrick Steinhardt","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"486392","messageId":"cover.1704714575.git.ps@pks.im","threadId":"60701","inReplyTo":null,"subject":"[PATCH 0/4] reftable: optimize I/O patterns","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-08T12:18:27Z","receivedAt":"2024-01-08T12:18:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis patch series optimizes some I/O patterns that the reftable library\nuses:\n\n - Establish a stat(3P)-based caching mechanism to avoid re-reading\n \"tables.list\" all the time.\n\n - Convert the block source to use mmap(3P) instead of read(3P) to read\n data.\n\nCombined these lead to a ~2.7x speedup when writing many refs.\n\nPatrick\n\nPatrick Steinhardt (4):\n reftable/stack: refactor stack reloading to have common exit path\n reftable/stack: refactor reloading to use file descriptor\n reftable/stack: use stat info to avoid re-reading stack list\n reftable/blocksource: use mmap to read tables\n\n reftable/blocksource.c | 48 +++++++++++++-----\n reftable/stack.c | 111 ++++++++++++++++++++++++-----------------\n reftable/stack.h | 1 +\n reftable/system.h | 1 +\n 4 files changed, 103 insertions(+), 58 deletions(-)\n\n-- \n2.43.GIT\n\n"},{"id":"486393","messageId":"01ece2626dd4cb494829e146d99c172fa8428478.1704714575.git.ps@pks.im","threadId":"60701","inReplyTo":"cover.1704714575.git.ps@pks.im","subject":"[PATCH 1/4] reftable/stack: refactor stack reloading to have common exit path","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-08T12:18:31Z","receivedAt":"2024-01-08T12:18:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `reftable_stack_reload_maybe_reuse()` function is responsible for\nreloading the reftable list from disk. The function is quite hard to\nfollow though because it has a bunch of different exit paths, many of\nwhich have to free the same set of resources.\n\nRefactor the function to have a common exit path. While at it, touch up\nthe style of this function a bit to match our usual coding style better.\n---\n reftable/stack.c | 86 +++++++++++++++++++++++-------------------------\n 1 file changed, 42 insertions(+), 44 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 16bab82063..bf869a6772 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -304,69 +304,67 @@ static int tv_cmp(struct timeval *a, struct timeval *b)\n static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n \t\t\t\t\t int reuse_open)\n {\n-\tstruct timeval deadline = { 0 };\n-\tint err = gettimeofday(&deadline, NULL);\n+\tchar **names = NULL, **names_after = NULL;\n+\tstruct timeval deadline;\n \tint64_t delay = 0;\n-\tint tries = 0;\n-\tif (err < 0)\n-\t\treturn err;\n+\tint tries = 0, err;\n \n+\terr = gettimeofday(&deadline, NULL);\n+\tif (err < 0)\n+\t\tgoto out;\n \tdeadline.tv_sec += 3;\n+\n \twhile (1) {\n-\t\tchar **names = NULL;\n-\t\tchar **names_after = NULL;\n-\t\tstruct timeval now = { 0 };\n-\t\tint err = gettimeofday(&now, NULL);\n-\t\tint err2 = 0;\n-\t\tif (err < 0) {\n-\t\t\treturn err;\n-\t\t}\n+\t\tstruct timeval now;\n \n-\t\t/* Only look at deadlines after the first few times. This\n-\t\t simplifies debugging in GDB */\n+\t\terr = gettimeofday(&now, NULL);\n+\t\tif (err < 0)\n+\t\t\tgoto out;\n+\n+\t\t/*\n+\t\t * Only look at deadlines after the first few times. This\n+\t\t * simplifies debugging in GDB.\n+\t\t */\n \t\ttries++;\n-\t\tif (tries > 3 && tv_cmp(&now, &deadline) >= 0) {\n-\t\t\tbreak;\n-\t\t}\n+\t\tif (tries > 3 && tv_cmp(&now, &deadline) >= 0)\n+\t\t\tgoto out;\n \n \t\terr = read_lines(st->list_file, &names);\n-\t\tif (err < 0) {\n-\t\t\tfree_names(names);\n-\t\t\treturn err;\n-\t\t}\n+\t\tif (err < 0)\n+\t\t\tgoto out;\n+\n \t\terr = reftable_stack_reload_once(st, names, reuse_open);\n-\t\tif (err == 0) {\n-\t\t\tfree_names(names);\n+\t\tif (!err)\n \t\t\tbreak;\n-\t\t}\n-\t\tif (err != REFTABLE_NOT_EXIST_ERROR) {\n-\t\t\tfree_names(names);\n-\t\t\treturn err;\n-\t\t}\n-\n-\t\t/* err == REFTABLE_NOT_EXIST_ERROR can be caused by a concurrent\n-\t\t writer. Check if there was one by checking if the name list\n-\t\t changed.\n-\t\t*/\n-\t\terr2 = read_lines(st->list_file, &names_after);\n-\t\tif (err2 < 0) {\n-\t\t\tfree_names(names);\n-\t\t\treturn err2;\n-\t\t}\n-\n+\t\tif (err != REFTABLE_NOT_EXIST_ERROR)\n+\t\t\tgoto out;\n+\n+\t\t/*\n+\t\t * REFTABLE_NOT_EXIST_ERROR can be caused by a concurrent\n+\t\t * writer. Check if there was one by checking if the name list\n+\t\t * changed.\n+\t\t */\n+\t\terr = read_lines(st->list_file, &names_after);\n+\t\tif (err < 0)\n+\t\t\tgoto out;\n \t\tif (names_equal(names_after, names)) {\n-\t\t\tfree_names(names);\n-\t\t\tfree_names(names_after);\n-\t\t\treturn err;\n+\t\t\terr = REFTABLE_NOT_EXIST_ERROR;\n+\t\t\tgoto out;\n \t\t}\n+\n \t\tfree_names(names);\n+\t\tnames = NULL;\n \t\tfree_names(names_after);\n+\t\tnames_after = NULL;\n \n \t\tdelay = delay + (delay * rand()) / RAND_MAX + 1;\n \t\tsleep_millisec(delay);\n \t}\n \n-\treturn 0;\n+out:\n+\tfree_names(names);\n+\tfree_names(names_after);\n+\treturn err;\n }\n \n /* -1 = error\n-- \n2.43.GIT\n\n"},{"id":"486394","messageId":"726d302d7b21a5b948575492c717cfdb701de5cd.1704714575.git.ps@pks.im","threadId":"60701","inReplyTo":"cover.1704714575.git.ps@pks.im","subject":"[PATCH 2/4] reftable/stack: refactor reloading to use file descriptor","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-08T12:18:35Z","receivedAt":"2024-01-08T12:18:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We're about to introduce a stat(3P)-based caching mechanism to reload\nthe list of stacks only when it has changed. In order to avoid race\nconditions this requires us to have a file descriptor available that we\ncan use to call fstat(3P) on.\n\nPrepare for this by converting the code to use `fd_read_lines()` so that\nwe have the file descriptor readily available.\n---\n reftable/stack.c | 21 ++++++++++++++++++---\n 1 file changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex bf869a6772..b1ee247601 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -308,6 +308,7 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n \tstruct timeval deadline;\n \tint64_t delay = 0;\n \tint tries = 0, err;\n+\tint fd = -1;\n \n \terr = gettimeofday(&deadline, NULL);\n \tif (err < 0)\n@@ -329,9 +330,19 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n \t\tif (tries > 3 && tv_cmp(&now, &deadline) >= 0)\n \t\t\tgoto out;\n \n-\t\terr = read_lines(st->list_file, &names);\n-\t\tif (err < 0)\n-\t\t\tgoto out;\n+\t\tfd = open(st->list_file, O_RDONLY);\n+\t\tif (fd < 0) {\n+\t\t\tif (errno != ENOENT) {\n+\t\t\t\terr = REFTABLE_IO_ERROR;\n+\t\t\t\tgoto out;\n+\t\t\t}\n+\n+\t\t\tnames = reftable_calloc(sizeof(char *));\n+\t\t} else {\n+\t\t\terr = fd_read_lines(fd, &names);\n+\t\t\tif (err < 0)\n+\t\t\t\tgoto out;\n+\t\t}\n \n \t\terr = reftable_stack_reload_once(st, names, reuse_open);\n \t\tif (!err)\n@@ -356,12 +367,16 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n \t\tnames = NULL;\n \t\tfree_names(names_after);\n \t\tnames_after = NULL;\n+\t\tclose(fd);\n+\t\tfd = -1;\n \n \t\tdelay = delay + (delay * rand()) / RAND_MAX + 1;\n \t\tsleep_millisec(delay);\n \t}\n \n out:\n+\tif (fd >= 0)\n+\t\tclose(fd);\n \tfree_names(names);\n \tfree_names(names_after);\n \treturn err;\n-- \n2.43.GIT\n\n"},{"id":"486395","messageId":"4fabdc3d8016dbc1e20fbe90058ee7320a5f770b.1704714575.git.ps@pks.im","threadId":"60701","inReplyTo":"cover.1704714575.git.ps@pks.im","subject":"[PATCH 3/4] reftable/stack: use stat info to avoid re-reading stack list","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-08T12:18:39Z","receivedAt":"2024-01-08T12:18:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Whenever we call into the refs interfaces we potentially have to reload\nrefs in case they have been concurrently modified, either in-process or\nexternally. While this happens somewhat automatically for loose refs\nbecause we simply try to re-read the files, the \"packed\" backend will\nreload its snapshot of the packed-refs file in case its stat info has\nchanged since last reading it.\n\nIn the reftable backend we have a similar mechanism that is provided by\n`reftable_stack_reload()`. This function will read the list of stacks\nfrom \"tables.list\" and, if they have changed from the currently stored\nlist, reload the stacks. This is heavily inefficient though, as we have\nto check whether the stack is up-to-date on basically every read and\nthus keep on re-reading the file all the time even if it didn't change\nat all.\n\nWe can do better and use the same stat(3P)-based mechanism that the\n\"packed\" backend uses. Instead of reading the file, we will only open\nthe file descriptor, fstat(3P) it, and then compare the info against the\ncached value from the last time we have updated the stack. This should\nalways work alright because \"tables.list\" is updated atomically via a\nrename, so even if the ctime or mtime wasn't granular enough to identify\na change, at least the inode number should have changed.\n\nThis change significantly speeds up operations where many refs are read,\nlike when using git-update-ref(1). The following benchmark creates N\nrefs in an otherwise-empty repository via `git update-ref --stdin`:\n\n Benchmark 1: update-ref: create many refs (refcount = 1, revision = HEAD~)\n Time (mean ± σ): 5.6 ms ± 0.2 ms [User: 2.5 ms, System: 3.0 ms]\n Range (min … max): 5.2 ms … 6.0 ms 89 runs\n\n Benchmark 2: update-ref: create many refs (refcount = 100, revision = HEAD~)\n Time (mean ± σ): 19.2 ms ± 0.3 ms [User: 8.6 ms, System: 10.4 ms]\n Range (min … max): 18.4 ms … 19.8 ms 63 runs\n\n Benchmark 3: update-ref: create many refs (refcount = 10000, revision = HEAD~)\n Time (mean ± σ): 1.310 s ± 0.014 s [User: 0.566 s, System: 0.724 s]\n Range (min … max): 1.291 s … 1.342 s 10 runs\n\n Benchmark 4: update-ref: create many refs (refcount = 1, revision = HEAD)\n Time (mean ± σ): 5.7 ms ± 0.4 ms [User: 2.6 ms, System: 3.1 ms]\n Range (min … max): 5.1 ms … 9.5 ms 91 runs\n\n Benchmark 5: update-ref: create many refs (refcount = 100, revision = HEAD)\n Time (mean ± σ): 15.2 ms ± 0.3 ms [User: 7.0 ms, System: 8.1 ms]\n Range (min … max): 14.3 ms … 17.1 ms 71 runs\n\n Benchmark 6: update-ref: create many refs (refcount = 10000, revision = HEAD)\n Time (mean ± σ): 916.1 ms ± 11.0 ms [User: 420.8 ms, System: 495.0 ms]\n Range (min … max): 906.9 ms … 943.8 ms 10 runs\n\n Summary\n update-ref: create many refs (refcount = 1, revision = HEAD~) ran\n 1.01 ± 0.09 times faster than update-ref: create many refs (refcount = 1, revision = HEAD)\n 2.72 ± 0.11 times faster than update-ref: create many refs (refcount = 100, revision = HEAD)\n 3.42 ± 0.13 times faster than update-ref: create many refs (refcount = 100, revision = HEAD~)\n 163.59 ± 5.62 times faster than update-ref: create many refs (refcount = 10000, revision = HEAD)\n 233.91 ± 7.92 times faster than update-ref: create many refs (refcount = 10000, revision = HEAD~)\n---\n reftable/stack.c | 10 +++++++++-\n reftable/stack.h | 1 +\n reftable/system.h | 1 +\n 3 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex b1ee247601..a51a2dbf1f 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -175,6 +175,7 @@ void reftable_stack_destroy(struct reftable_stack *st)\n \t\tst->readers_len = 0;\n \t\tFREE_AND_NULL(st->readers);\n \t}\n+\tstat_validity_clear(&st->list_validity);\n \tFREE_AND_NULL(st->list_file);\n \tFREE_AND_NULL(st->reftable_dir);\n \treftable_free(st);\n@@ -374,6 +375,8 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n \t\tsleep_millisec(delay);\n \t}\n \n+\tstat_validity_update(&st->list_validity, fd);\n+\n out:\n \tif (fd >= 0)\n \t\tclose(fd);\n@@ -388,8 +391,13 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n static int stack_uptodate(struct reftable_stack *st)\n {\n \tchar **names = NULL;\n-\tint err = read_lines(st->list_file, &names);\n+\tint err;\n \tint i = 0;\n+\n+\tif (stat_validity_check(&st->list_validity, st->list_file))\n+\t\treturn 0;\n+\n+\terr = read_lines(st->list_file, &names);\n \tif (err < 0)\n \t\treturn err;\n \ndiff --git a/reftable/stack.h b/reftable/stack.h\nindex f57005846e..3f80cc598a 100644\n--- a/reftable/stack.h\n+++ b/reftable/stack.h\n@@ -14,6 +14,7 @@ license that can be found in the LICENSE file or at\n #include \"reftable-stack.h\"\n \n struct reftable_stack {\n+\tstruct stat_validity list_validity;\n \tchar *list_file;\n \tchar *reftable_dir;\n \tint disable_auto_compact;\ndiff --git a/reftable/system.h b/reftable/system.h\nindex 6b74a81514..2cc7adf271 100644\n--- a/reftable/system.h\n+++ b/reftable/system.h\n@@ -12,6 +12,7 @@ license that can be found in the LICENSE file or at\n /* This header glues the reftable library to the rest of Git */\n \n #include \"git-compat-util.h\"\n+#include \"statinfo.h\"\n #include \"strbuf.h\"\n #include \"hash-ll.h\" /* hash ID, sizes.*/\n #include \"dir.h\" /* remove_dir_recursively, for tests.*/\n-- \n2.43.GIT\n\n"},{"id":"486396","messageId":"a23f38a80609a5c5a6931400ffd28a92b33838bb.1704714575.git.ps@pks.im","threadId":"60701","inReplyTo":"cover.1704714575.git.ps@pks.im","subject":"[PATCH 4/4] reftable/blocksource: use mmap to read tables","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-08T12:18:43Z","receivedAt":"2024-01-08T12:18:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The blocksource interface provides an interface to read blocks from a\nreftable table. This interface is implemented using read(3P) calls on\nthe underlying file descriptor. While this works alright, this pattern\nis very inefficient when repeatedly querying the reftable stack for one\nor more refs. This inefficiency can mostly be attributed to the fact\nthat we often need to re-read the same blocks over and over again, and\nevery single time we need to call read(3P) again.\n\nA natural fit in this context is to use mmap(3P) instead of read(3P),\nwhich has a bunch of benefits:\n\n - We do not need to come up with a caching strategy for some of the\n blocks as this will be handled by the kernel already.\n\n - We can avoid the overhead of having to call into the read(3P)\n syscall repeatedly.\n\n - We do not need to allocate returned blocks repeatedly, but can\n instead hand out pointers into the mmapped region directly.\n\nUsing mmap comes with a significant drawback on Windows though, because\nmmapped files cannot be deleted and neither is it possible to rename\nfiles onto an mmapped file. But for one, the reftable library gracefully\nhandles the case where auto-compaction cannot delete a still-open stack\nalready and ignores any such errors. Also, `reftable_stack_clean()` will\nprune stale tables which are not referenced by \"tables.list\" anymore so\nthat those files can eventually be pruned. And second, we never rewrite\nalready-rewritten stacks, so it does not matter that we cannot rename a\nfile over an mmaped file, either.\n\nAnother unfortunate property of mmap is that it is not supported by all\nsystems. But given that the size of reftables should typically be rather\nlimited (megabytes at most in the vast majority of repositories), we can\nprovide an easy fallback by just reading the complete table into memory\non such platforms. This is the same strategy that the \"packed\" backend\nuses in case the platform does not provide mmap.\n\nWhile this change doesn't significantly improve performance in the case\nwhere we're seeking through stacks once (like e.g. git-for-each-ref(1)\nwould). But it does speed up usecases where there is lots of random\naccess to refs, e.g. when writing. The following benchmark demonstrates\nthese savings with git-update-ref(1) creating N refs in an otherwise\nempty repository:\n\n Benchmark 1: update-ref: create many refs (refcount = 1, revision = HEAD~)\n Time (mean ± σ): 5.6 ms ± 0.2 ms [User: 2.7 ms, System: 2.8 ms]\n Range (min … max): 5.1 ms … 6.0 ms 90 runs\n\n Benchmark 2: update-ref: create many refs (refcount = 100, revision = HEAD~)\n Time (mean ± σ): 15.1 ms ± 0.4 ms [User: 7.1 ms, System: 8.0 ms]\n Range (min … max): 14.2 ms … 16.5 ms 71 runs\n\n Benchmark 3: update-ref: create many refs (refcount = 10000, revision = HEAD~)\n Time (mean ± σ): 919.4 ms ± 11.2 ms [User: 427.5 ms, System: 491.7 ms]\n Range (min … max): 908.1 ms … 936.6 ms 10 runs\n\n Benchmark 4: update-ref: create many refs (refcount = 1, revision = HEAD)\n Time (mean ± σ): 5.5 ms ± 0.3 ms [User: 2.4 ms, System: 3.1 ms]\n Range (min … max): 5.0 ms … 7.3 ms 89 runs\n\n Benchmark 5: update-ref: create many refs (refcount = 100, revision = HEAD)\n Time (mean ± σ): 10.4 ms ± 0.3 ms [User: 5.1 ms, System: 5.3 ms]\n Range (min … max): 9.7 ms … 11.0 ms 78 runs\n\n Benchmark 6: update-ref: create many refs (refcount = 10000, revision = HEAD)\n Time (mean ± σ): 483.5 ms ± 6.3 ms [User: 227.8 ms, System: 255.6 ms]\n Range (min … max): 477.5 ms … 498.8 ms 10 runs\n\n Summary\n update-ref: create many refs (refcount = 1, revision = HEAD) ran\n 1.01 ± 0.06 times faster than update-ref: create many refs (refcount = 1, revision = HEAD~)\n 1.89 ± 0.11 times faster than update-ref: create many refs (refcount = 100, revision = HEAD)\n 2.73 ± 0.16 times faster than update-ref: create many refs (refcount = 100, revision = HEAD~)\n 87.55 ± 4.65 times faster than update-ref: create many refs (refcount = 10000, revision = HEAD)\n 166.48 ± 8.80 times faster than update-ref: create many refs (refcount = 10000, revision = HEAD~)\n\nTheoretically, we could also replicate the strategy of the \"packed\"\nbackend where small tables are read into memory instead of using mmap.\nBenchmarks did not confirm that this has a performance benefit though.\n---\n reftable/blocksource.c | 48 ++++++++++++++++++++++++++++++------------\n 1 file changed, 35 insertions(+), 13 deletions(-)\n\ndiff --git a/reftable/blocksource.c b/reftable/blocksource.c\nindex a1ea304429..5d3f3d264c 100644\n--- a/reftable/blocksource.c\n+++ b/reftable/blocksource.c\n@@ -13,6 +13,12 @@ license that can be found in the LICENSE file or at\n #include \"reftable-blocksource.h\"\n #include \"reftable-error.h\"\n \n+#if defined(NO_MMAP)\n+static int use_mmap = 0;\n+#else\n+static int use_mmap = 1;\n+#endif\n+\n static void strbuf_return_block(void *b, struct reftable_block *dest)\n {\n \tif (dest->len)\n@@ -78,6 +84,7 @@ struct reftable_block_source malloc_block_source(void)\n struct file_block_source {\n \tint fd;\n \tuint64_t size;\n+\tunsigned char *data;\n };\n \n static uint64_t file_size(void *b)\n@@ -87,19 +94,23 @@ static uint64_t file_size(void *b)\n \n static void file_return_block(void *b, struct reftable_block *dest)\n {\n-\tif (dest->len)\n-\t\tmemset(dest->data, 0xff, dest->len);\n-\treftable_free(dest->data);\n }\n \n-static void file_close(void *b)\n+static void file_close(void *v)\n {\n-\tint fd = ((struct file_block_source *)b)->fd;\n-\tif (fd > 0) {\n-\t\tclose(fd);\n-\t\t((struct file_block_source *)b)->fd = 0;\n+\tstruct file_block_source *b = v;\n+\n+\tif (b->fd >= 0) {\n+\t\tclose(b->fd);\n+\t\tb->fd = -1;\n \t}\n \n+\tif (use_mmap)\n+\t\tmunmap(b->data, b->size);\n+\telse\n+\t\treftable_free(b->data);\n+\tb->data = NULL;\n+\n \treftable_free(b);\n }\n \n@@ -108,9 +119,7 @@ static int file_read_block(void *v, struct reftable_block *dest, uint64_t off,\n {\n \tstruct file_block_source *b = v;\n \tassert(off + size <= b->size);\n-\tdest->data = reftable_malloc(size);\n-\tif (pread_in_full(b->fd, dest->data, size, off) != size)\n-\t\treturn -1;\n+\tdest->data = b->data + off;\n \tdest->len = size;\n \treturn size;\n }\n@@ -127,8 +136,10 @@ int reftable_block_source_from_file(struct reftable_block_source *bs,\n {\n \tstruct stat st = { 0 };\n \tint err = 0;\n-\tint fd = open(name, O_RDONLY);\n+\tint fd;\n \tstruct file_block_source *p = NULL;\n+\n+\tfd = open(name, O_RDONLY);\n \tif (fd < 0) {\n \t\tif (errno == ENOENT) {\n \t\t\treturn REFTABLE_NOT_EXIST_ERROR;\n@@ -144,7 +155,18 @@ int reftable_block_source_from_file(struct reftable_block_source *bs,\n \n \tp = reftable_calloc(sizeof(struct file_block_source));\n \tp->size = st.st_size;\n-\tp->fd = fd;\n+\tif (use_mmap) {\n+\t\tp->data = xmmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\t\tp->fd = fd;\n+\t} else {\n+\t\tp->data = xmalloc(st.st_size);\n+\t\tif (read_in_full(fd, p->data, st.st_size) != st.st_size) {\n+\t\t\tclose(fd);\n+\t\t\treturn -1;\n+\t\t}\n+\t\tclose(fd);\n+\t\tp->fd = -1;\n+\t}\n \n \tassert(!bs->ops);\n \tbs->ops = &file_vtable;\n-- \n2.43.GIT\n\n"},{"id":"486542","messageId":"xmqqmstdccz8.fsf@gitster.g","threadId":"60701","inReplyTo":"01ece2626dd4cb494829e146d99c172fa8428478.1704714575.git.ps@pks.im","subject":"Re: [PATCH 1/4] reftable/stack: refactor stack reloading to have common exit path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-10T17:30:51Z","receivedAt":"2024-01-10T17:30:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> The `reftable_stack_reload_maybe_reuse()` function is responsible for\n> reloading the reftable list from disk. The function is quite hard to\n> follow though because it has a bunch of different exit paths, many of\n> which have to free the same set of resources.\n>\n> Refactor the function to have a common exit path. While at it, touch up\n> the style of this function a bit to match our usual coding style better.\n> ---\n> reftable/stack.c | 86 +++++++++++++++++++++++-------------------------\n> 1 file changed, 42 insertions(+), 44 deletions(-)\n\nMissing sign-off.\n\nOther than that, I did not find anything questionable in the\nconversion. By sticking to the two simple invariants:\n\n - we use \"err\" as our return value when we jump to \"out:\"\n\n - we always keep \"names\" and \"names_after\" freeable, and free them\n when we jump to \"out:\".\n\nthe exit status and leak prevention are both very clear (and the\nbehaviour is not changed---it is not like there are any existing\nleaks that are plugged by this restructuring of the loop).\n\n"},{"id":"486561","messageId":"xmqqsf359d22.fsf@gitster.g","threadId":"60701","inReplyTo":"726d302d7b21a5b948575492c717cfdb701de5cd.1704714575.git.ps@pks.im","subject":"Re: [PATCH 2/4] reftable/stack: refactor reloading to use file descriptor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-10T19:57:25Z","receivedAt":"2024-01-10T19:57:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> We're about to introduce a stat(3P)-based caching mechanism to reload\n> the list of stacks only when it has changed. In order to avoid race\n> conditions this requires us to have a file descriptor available that we\n> can use to call fstat(3P) on.\n>\n> Prepare for this by converting the code to use `fd_read_lines()` so that\n> we have the file descriptor readily available.\n> ---\n\nMissing sign-off.\n\nOther than that, the change is a refactoring that is very faithful\nto the original. Instead of doing the \"open - pretend we opened an\nempty file if it does not exist - read - close\" dance all inside the\nread_lines() call, this sort-of open codes the helper in this caller,\nso that later steps of this series can look at the file descriptor\nopen to it.\n\nLooking good.\n\n> reftable/stack.c | 21 ++++++++++++++++++---\n> 1 file changed, 18 insertions(+), 3 deletions(-)\n>\n> diff --git a/reftable/stack.c b/reftable/stack.c\n> index bf869a6772..b1ee247601 100644\n> --- a/reftable/stack.c\n> +++ b/reftable/stack.c\n> @@ -308,6 +308,7 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n> \tstruct timeval deadline;\n> \tint64_t delay = 0;\n> \tint tries = 0, err;\n> +\tint fd = -1;\n> \n> \terr = gettimeofday(&deadline, NULL);\n> \tif (err < 0)\n> @@ -329,9 +330,19 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n> \t\tif (tries > 3 && tv_cmp(&now, &deadline) >= 0)\n> \t\t\tgoto out;\n> \n> -\t\terr = read_lines(st->list_file, &names);\n> -\t\tif (err < 0)\n> -\t\t\tgoto out;\n> +\t\tfd = open(st->list_file, O_RDONLY);\n> +\t\tif (fd < 0) {\n> +\t\t\tif (errno != ENOENT) {\n> +\t\t\t\terr = REFTABLE_IO_ERROR;\n> +\t\t\t\tgoto out;\n> +\t\t\t}\n> +\n> +\t\t\tnames = reftable_calloc(sizeof(char *));\n> +\t\t} else {\n> +\t\t\terr = fd_read_lines(fd, &names);\n> +\t\t\tif (err < 0)\n> +\t\t\t\tgoto out;\n> +\t\t}\n> \n> \t\terr = reftable_stack_reload_once(st, names, reuse_open);\n> \t\tif (!err)\n> @@ -356,12 +367,16 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n> \t\tnames = NULL;\n> \t\tfree_names(names_after);\n> \t\tnames_after = NULL;\n> +\t\tclose(fd);\n> +\t\tfd = -1;\n> \n> \t\tdelay = delay + (delay * rand()) / RAND_MAX + 1;\n> \t\tsleep_millisec(delay);\n> \t}\n> \n> out:\n> +\tif (fd >= 0)\n> +\t\tclose(fd);\n> \tfree_names(names);\n> \tfree_names(names_after);\n> \treturn err;\n"},{"id":"486562","messageId":"xmqqo7dt9ckn.fsf@gitster.g","threadId":"60701","inReplyTo":"4fabdc3d8016dbc1e20fbe90058ee7320a5f770b.1704714575.git.ps@pks.im","subject":"Re: [PATCH 3/4] reftable/stack: use stat info to avoid re-reading stack list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-10T20:07:52Z","receivedAt":"2024-01-10T20:08:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> We can do better and use the same stat(3P)-based mechanism that the\n> \"packed\" backend uses. Instead of reading the file, we will only open\n> the file descriptor, fstat(3P) it, and then compare the info against the\n> cached value from the last time we have updated the stack. This should\n> always work alright because \"tables.list\" is updated atomically via a\n> rename, so even if the ctime or mtime wasn't granular enough to identify\n> a change, at least the inode number should have changed.\n\nOr the file size. Let's keep in mind that many users get useless\ninum from their filesystem X-<.\n\n> Summary\n> update-ref: create many refs (refcount = 1, revision = HEAD~) ran\n> 1.01 ± 0.09 times faster than update-ref: create many refs (refcount = 1, revision = HEAD)\n> 2.72 ± 0.11 times faster than update-ref: create many refs (refcount = 100, revision = HEAD)\n> 3.42 ± 0.13 times faster than update-ref: create many refs (refcount = 100, revision = HEAD~)\n> 163.59 ± 5.62 times faster than update-ref: create many refs (refcount = 10000, revision = HEAD)\n> 233.91 ± 7.92 times faster than update-ref: create many refs (refcount = 10000, revision = HEAD~)\n> ---\n\nNice.\n\n> @@ -374,6 +375,8 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n> \t\tsleep_millisec(delay);\n> \t}\n> \n> +\tstat_validity_update(&st->list_validity, fd);\n> +\n> out:\n> \tif (fd >= 0)\n> \t\tclose(fd);\n\nThe stat_validity_update() does not happen in the error codepath.\n\nShould we be clearing the validity of the list when somebody jumps\nto \"out:\" due to an error? Or by the time this function gets\ncalled, the caller would already have cleared the validity and an\nerror that jumps to \"out:\" keeps the list invalid?\n\nOther than the missing sign-off, the change looks very straight-forward.\n"},{"id":"486564","messageId":"xmqq5y00anlj.fsf@gitster.g","threadId":"60701","inReplyTo":"a23f38a80609a5c5a6931400ffd28a92b33838bb.1704714575.git.ps@pks.im","subject":"Re: [PATCH 4/4] reftable/blocksource: use mmap to read tables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-10T21:24:24Z","receivedAt":"2024-01-10T21:24:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Using mmap comes with a significant drawback on Windows though, because\n> mmapped files cannot be deleted and neither is it possible to rename\n> files onto an mmapped file. But for one, the reftable library gracefully\n> handles the case where auto-compaction cannot delete a still-open stack\n> already and ignores any such errors. Also, `reftable_stack_clean()` will\n> prune stale tables which are not referenced by \"tables.list\" anymore so\n> that those files can eventually be pruned. And second, we never rewrite\n> already-rewritten stacks, so it does not matter that we cannot rename a\n> file over an mmaped file, either.\n\nI somehow thought that we use \"read into an allocated memory and\npretend as if we mapped\" emulation on Windows, so all of that may be\nmoot.\n\n> diff --git a/reftable/blocksource.c b/reftable/blocksource.c\n> index a1ea304429..5d3f3d264c 100644\n> --- a/reftable/blocksource.c\n> +++ b/reftable/blocksource.c\n> @@ -13,6 +13,12 @@ license that can be found in the LICENSE file or at\n> #include \"reftable-blocksource.h\"\n> #include \"reftable-error.h\"\n> \n> +#if defined(NO_MMAP)\n> +static int use_mmap = 0;\n> +#else\n> +static int use_mmap = 1;\n> +#endif\n\nIs there (do you need) some externally controllable knob that the\nuser can use to turn this off on a system that is compiled without\nNO_MMAP? Otherwise it is misleading to have this as a variable.\n\n> -static void file_close(void *b)\n> +static void file_close(void *v)\n> {\n> -\tint fd = ((struct file_block_source *)b)->fd;\n> -\tif (fd > 0) {\n> -\t\tclose(fd);\n> -\t\t((struct file_block_source *)b)->fd = 0;\n> +\tstruct file_block_source *b = v;\n> +\n> +\tif (b->fd >= 0) {\n> +\t\tclose(b->fd);\n> +\t\tb->fd = -1;\n> \t}\n> \n> +\tif (use_mmap)\n> +\t\tmunmap(b->data, b->size);\n> +\telse\n> +\t\treftable_free(b->data);\n> +\tb->data = NULL;\n> +\n> \treftable_free(b);\n> }\n\nIt would have been nicer to do this kind of \"a void pointer is taken\nand we immediately cast it to a concrete and useful type upon entry\"\nclean-up as a separate step that is purely clean-up, if there were\nmany instances. It is the first such one in the series as far as I\nremember, so it is not a huge deal.\n\n> @@ -108,9 +119,7 @@ static int file_read_block(void *v, struct reftable_block *dest, uint64_t off,\n> {\n> \tstruct file_block_source *b = v;\n> \tassert(off + size <= b->size);\n> -\tdest->data = reftable_malloc(size);\n> -\tif (pread_in_full(b->fd, dest->data, size, off) != size)\n> -\t\treturn -1;\n> +\tdest->data = b->data + off;\n> \tdest->len = size;\n> \treturn size;\n> }\n\nSo, we used to delay the allocation and reading of a block until\nthis function gets called. Now, by the time the control reaches\nthe function, we are expected to have the data handy at b->data.\nThat is ensured by reftable_block_source_from_file(), I presume?\n\n> @@ -127,8 +136,10 @@ int reftable_block_source_from_file(struct reftable_block_source *bs,\n> {\n> \tstruct stat st = { 0 };\n> \tint err = 0;\n> -\tint fd = open(name, O_RDONLY);\n> +\tint fd;\n> \tstruct file_block_source *p = NULL;\n> +\n> +\tfd = open(name, O_RDONLY);\n> \tif (fd < 0) {\n> \t\tif (errno == ENOENT) {\n> \t\t\treturn REFTABLE_NOT_EXIST_ERROR;\n\nThis is a no-op clean-up that would have been better in a separate\nclean-up step. Again, not a huge deal.\n\n> @@ -144,7 +155,18 @@ int reftable_block_source_from_file(struct reftable_block_source *bs,\n> \n> \tp = reftable_calloc(sizeof(struct file_block_source));\n> \tp->size = st.st_size;\n> -\tp->fd = fd;\n> +\tif (use_mmap) {\n> +\t\tp->data = xmmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);\n> +\t\tp->fd = fd;\n\nThis is a bit unusual for our use of mmap() where the norm is to\nclose the file descriptor once we mapped (we only need the memory\nregion itself and not the originating file descriptor to unmap it).\n\nIs there a reason why we need to keep p->fd? Now the other side of\nthis if/else preallocates the whole thing and slurps everything in\ncore to allow the remainder of the code to mimic what happens on the\nmmaped block memory (e.g., we saw how file_read_block() assumes that\nb->data[0..b->size] are valid) and does not obviously need p->fd,\ncan we just remove .fd member from the file_block_source structure?\n\nThat way, file_close() can also lose the conditional close() call.\n\nFor that matter, do we need any codepath in this file that is\nenabled by !use_mmap? Can't we just use xmmap() and let its\n\"instead, we allocate, read into it and return\" emulation?\n\nThanks.\n\n> +\t} else {\n> +\t\tp->data = xmalloc(st.st_size);\n> +\t\tif (read_in_full(fd, p->data, st.st_size) != st.st_size) {\n> +\t\t\tclose(fd);\n> +\t\t\treturn -1;\n> +\t\t}\n> +\t\tclose(fd);\n> +\t\tp->fd = -1;\n> +\t}\n> \n> \tassert(!bs->ops);\n> \tbs->ops = &file_vtable;\n"},{"id":"486604","messageId":"ZZ-ZuCBevbNot97m@tanuki","threadId":"60701","inReplyTo":"xmqqmstdccz8.fsf@gitster.g","subject":"Re: [PATCH 1/4] reftable/stack: refactor stack reloading to have common exit path","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-11T07:33:12Z","receivedAt":"2024-01-11T07:33:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jan 10, 2024 at 09:30:51AM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > The `reftable_stack_reload_maybe_reuse()` function is responsible for\n> > reloading the reftable list from disk. The function is quite hard to\n> > follow though because it has a bunch of different exit paths, many of\n> > which have to free the same set of resources.\n> >\n> > Refactor the function to have a common exit path. While at it, touch up\n> > the style of this function a bit to match our usual coding style better.\n> > ---\n> > reftable/stack.c | 86 +++++++++++++++++++++++-------------------------\n> > 1 file changed, 42 insertions(+), 44 deletions(-)\n> \n> Missing sign-off.\n\nAh, sorry, I forgot my usual `git rebase --signoff`. Will fix in v2.\n\nPatrick\n"},{"id":"486605","messageId":"ZZ-bwC66hDpbm6qm@tanuki","threadId":"60701","inReplyTo":"xmqqo7dt9ckn.fsf@gitster.g","subject":"Re: [PATCH 3/4] reftable/stack: use stat info to avoid re-reading stack list","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-11T07:41:52Z","receivedAt":"2024-01-11T07:41:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jan 10, 2024 at 12:07:52PM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > We can do better and use the same stat(3P)-based mechanism that the\n> > \"packed\" backend uses. Instead of reading the file, we will only open\n> > the file descriptor, fstat(3P) it, and then compare the info against the\n> > cached value from the last time we have updated the stack. This should\n> > always work alright because \"tables.list\" is updated atomically via a\n> > rename, so even if the ctime or mtime wasn't granular enough to identify\n> > a change, at least the inode number should have changed.\n> \n> Or the file size. Let's keep in mind that many users get useless\n> inum from their filesystem X-<.\n> \n> > Summary\n> > update-ref: create many refs (refcount = 1, revision = HEAD~) ran\n> > 1.01 ± 0.09 times faster than update-ref: create many refs (refcount = 1, revision = HEAD)\n> > 2.72 ± 0.11 times faster than update-ref: create many refs (refcount = 100, revision = HEAD)\n> > 3.42 ± 0.13 times faster than update-ref: create many refs (refcount = 100, revision = HEAD~)\n> > 163.59 ± 5.62 times faster than update-ref: create many refs (refcount = 10000, revision = HEAD)\n> > 233.91 ± 7.92 times faster than update-ref: create many refs (refcount = 10000, revision = HEAD~)\n> > ---\n> \n> Nice.\n> \n> > @@ -374,6 +375,8 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n> > \t\tsleep_millisec(delay);\n> > \t}\n> > \n> > +\tstat_validity_update(&st->list_validity, fd);\n> > +\n> > out:\n> > \tif (fd >= 0)\n> > \t\tclose(fd);\n> \n> The stat_validity_update() does not happen in the error codepath.\n> \n> Should we be clearing the validity of the list when somebody jumps\n> to \"out:\" due to an error? Or by the time this function gets\n> called, the caller would already have cleared the validity and an\n> error that jumps to \"out:\" keeps the list invalid?\n\nIt likely does not matter much. This function only gets called when we\nhave previously determined the stack to be out of date already. So\nunless we update the validity cache, we know that the next validity\ncheck would continue to treat the stack as outdated.\n\nThat being said I think it's good hygiene to clear the validity cache\nregardless of that.\n\nPatrick\n"},{"id":"486610","messageId":"ZZ-zIHOQfU_llj4u@tanuki","threadId":"60701","inReplyTo":"xmqq5y00anlj.fsf@gitster.g","subject":"Re: [PATCH 4/4] reftable/blocksource: use mmap to read tables","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-11T09:21:36Z","receivedAt":"2024-01-11T09:21:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jan 10, 2024 at 01:24:24PM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > Using mmap comes with a significant drawback on Windows though, because\n> > mmapped files cannot be deleted and neither is it possible to rename\n> > files onto an mmapped file. But for one, the reftable library gracefully\n> > handles the case where auto-compaction cannot delete a still-open stack\n> > already and ignores any such errors. Also, `reftable_stack_clean()` will\n> > prune stale tables which are not referenced by \"tables.list\" anymore so\n> > that those files can eventually be pruned. And second, we never rewrite\n> > already-rewritten stacks, so it does not matter that we cannot rename a\n> > file over an mmaped file, either.\n> \n> I somehow thought that we use \"read into an allocated memory and\n> pretend as if we mapped\" emulation on Windows, so all of that may be\n> moot.\n\nAh, you're right in fact. I missed the definition of `git_mmap()` and\n`git_munmap()`.\n\n> > diff --git a/reftable/blocksource.c b/reftable/blocksource.c\n> > index a1ea304429..5d3f3d264c 100644\n> > --- a/reftable/blocksource.c\n> > +++ b/reftable/blocksource.c\n> > @@ -13,6 +13,12 @@ license that can be found in the LICENSE file or at\n> > #include \"reftable-blocksource.h\"\n> > #include \"reftable-error.h\"\n> > \n> > +#if defined(NO_MMAP)\n> > +static int use_mmap = 0;\n> > +#else\n> > +static int use_mmap = 1;\n> > +#endif\n> \n> Is there (do you need) some externally controllable knob that the\n> user can use to turn this off on a system that is compiled without\n> NO_MMAP? Otherwise it is misleading to have this as a variable.\n\nNo. The benefit of using a variable instead of using ifdefs is that we\nknow that both code paths continue to compile just fine. We do the same\nthing in \"refs/packed-backend.c\".\n\nBut this code is not needed at all anyway, as you pointed out, because\nwe can simply use the emulated mmap(3P) code.\n\n> > +static void file_close(void *v)\n> > {\n> > -\tint fd = ((struct file_block_source *)b)->fd;\n> > -\tif (fd > 0) {\n> > -\t\tclose(fd);\n> > -\t\t((struct file_block_source *)b)->fd = 0;\n> > +\tstruct file_block_source *b = v;\n> > +\n> > +\tif (b->fd >= 0) {\n> > +\t\tclose(b->fd);\n> > +\t\tb->fd = -1;\n> > \t}\n> > \n> > +\tif (use_mmap)\n> > +\t\tmunmap(b->data, b->size);\n> > +\telse\n> > +\t\treftable_free(b->data);\n> > +\tb->data = NULL;\n> > +\n> > \treftable_free(b);\n> > }\n> \n> It would have been nicer to do this kind of \"a void pointer is taken\n> and we immediately cast it to a concrete and useful type upon entry\"\n> clean-up as a separate step that is purely clean-up, if there were\n> many instances. It is the first such one in the series as far as I\n> remember, so it is not a huge deal.\n> \n> > @@ -108,9 +119,7 @@ static int file_read_block(void *v, struct reftable_block *dest, uint64_t off,\n> > {\n> > \tstruct file_block_source *b = v;\n> > \tassert(off + size <= b->size);\n> > -\tdest->data = reftable_malloc(size);\n> > -\tif (pread_in_full(b->fd, dest->data, size, off) != size)\n> > -\t\treturn -1;\n> > +\tdest->data = b->data + off;\n> > \tdest->len = size;\n> > \treturn size;\n> > }\n> \n> So, we used to delay the allocation and reading of a block until\n> this function gets called. Now, by the time the control reaches\n> the function, we are expected to have the data handy at b->data.\n> That is ensured by reftable_block_source_from_file(), I presume?\n> \n> > @@ -127,8 +136,10 @@ int reftable_block_source_from_file(struct reftable_block_source *bs,\n> > {\n> > \tstruct stat st = { 0 };\n> > \tint err = 0;\n> > -\tint fd = open(name, O_RDONLY);\n> > +\tint fd;\n> > \tstruct file_block_source *p = NULL;\n> > +\n> > +\tfd = open(name, O_RDONLY);\n> > \tif (fd < 0) {\n> > \t\tif (errno == ENOENT) {\n> > \t\t\treturn REFTABLE_NOT_EXIST_ERROR;\n> \n> This is a no-op clean-up that would have been better in a separate\n> clean-up step. Again, not a huge deal.\n\nYeah, fair enough. One problem I have with the reftable library is that\nits coding style doesn't quite blend in with the rest of the Git code\nbase, so I want to refactor code that I'm touching to match our coding\nstyle better. This means that there will be a lot of such smallish\nrefactorings, and I'm often not sure whether to evict them into their\nown commit or whether to do the refactoring in the same step.\n\nFor small changes like this one here I think it's reasonable to do so in\nthe same commit. But larger refactorings like e.g. the introduction of\nthe common exit path in the first patch of this series I of course put\ninto their own commit.\n\n> > @@ -144,7 +155,18 @@ int reftable_block_source_from_file(struct reftable_block_source *bs,\n> > \n> > \tp = reftable_calloc(sizeof(struct file_block_source));\n> > \tp->size = st.st_size;\n> > -\tp->fd = fd;\n> > +\tif (use_mmap) {\n> > +\t\tp->data = xmmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);\n> > +\t\tp->fd = fd;\n> \n> This is a bit unusual for our use of mmap() where the norm is to\n> close the file descriptor once we mapped (we only need the memory\n> region itself and not the originating file descriptor to unmap it).\n> \n> Is there a reason why we need to keep p->fd? Now the other side of\n> this if/else preallocates the whole thing and slurps everything in\n> core to allow the remainder of the code to mimic what happens on the\n> mmaped block memory (e.g., we saw how file_read_block() assumes that\n> b->data[0..b->size] are valid) and does not obviously need p->fd,\n> can we just remove .fd member from the file_block_source structure?\n> \n> That way, file_close() can also lose the conditional close() call.\n> \n> For that matter, do we need any codepath in this file that is\n> enabled by !use_mmap? Can't we just use xmmap() and let its\n> \"instead, we allocate, read into it and return\" emulation?\n\nNot really, I'll update the code accordingly.\n\nThanks!\n\nPatrick\n"},{"id":"486611","messageId":"cover.1704966670.git.ps@pks.im","threadId":"60701","inReplyTo":"cover.1704714575.git.ps@pks.im","subject":"[PATCH v2 0/5] reftable: optimize I/O patterns","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-11T10:06:35Z","receivedAt":"2024-01-11T10:06:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second version of my patch series that optimizes some I/O\npatterns that the reftable library uses. Changes compared to v1:\n\n - Added missing signoffs.\n\n - The validity cache is now cleared when reloading the reftable stack\n fails.\n\n - Style fixes for `reftable_block_source_from_file()` are now in a\n separate commit.\n\n - We now use `xmmap()` exclusively, relying on its fallback code on\n NO_MMAP platforms.\n\n - The file descriptor is closed immediately after mmapping now.\n\nThanks for your review, Junio!\n\nPatrick\n\nPatrick Steinhardt (5):\n reftable/stack: refactor stack reloading to have common exit path\n reftable/stack: refactor reloading to use file descriptor\n reftable/stack: use stat info to avoid re-reading stack list\n reftable/blocksource: refactor code to match our coding style\n reftable/blocksource: use mmap to read tables\n\n reftable/blocksource.c | 39 ++++++--------\n reftable/stack.c | 113 +++++++++++++++++++++++++----------------\n reftable/stack.h | 1 +\n reftable/system.h | 1 +\n 4 files changed, 85 insertions(+), 69 deletions(-)\n\nRange-diff against v1:\n1: 01ece2626d ! 1: 4b7f52c415 reftable/stack: refactor stack reloading to have common exit path\n @@ Commit message\n Refactor the function to have a common exit path. While at it, touch up\n the style of this function a bit to match our usual coding style better.\n \n + Signed-off-by: Patrick Steinhardt \n +\n ## reftable/stack.c ##\n @@ reftable/stack.c: static int tv_cmp(struct timeval *a, struct timeval *b)\n static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n2: 726d302d7b ! 2: 36b9f6b624 reftable/stack: refactor reloading to use file descriptor\n @@ Commit message\n Prepare for this by converting the code to use `fd_read_lines()` so that\n we have the file descriptor readily available.\n \n + Signed-off-by: Patrick Steinhardt \n +\n ## reftable/stack.c ##\n @@ reftable/stack.c: static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n \tstruct timeval deadline;\n3: 4fabdc3d80 ! 3: c0f7cec95b reftable/stack: use stat info to avoid re-reading stack list\n @@ Commit message\n cached value from the last time we have updated the stack. This should\n always work alright because \"tables.list\" is updated atomically via a\n rename, so even if the ctime or mtime wasn't granular enough to identify\n - a change, at least the inode number should have changed.\n + a change, at least the inode number or file size should have changed.\n \n This change significantly speeds up operations where many refs are read,\n like when using git-update-ref(1). The following benchmark creates N\n refs in an otherwise-empty repository via `git update-ref --stdin`:\n \n Benchmark 1: update-ref: create many refs (refcount = 1, revision = HEAD~)\n - Time (mean ± σ): 5.6 ms ± 0.2 ms [User: 2.5 ms, System: 3.0 ms]\n - Range (min … max): 5.2 ms … 6.0 ms 89 runs\n + Time (mean ± σ): 5.1 ms ± 0.2 ms [User: 2.4 ms, System: 2.6 ms]\n + Range (min … max): 4.8 ms … 7.2 ms 109 runs\n \n Benchmark 2: update-ref: create many refs (refcount = 100, revision = HEAD~)\n - Time (mean ± σ): 19.2 ms ± 0.3 ms [User: 8.6 ms, System: 10.4 ms]\n - Range (min … max): 18.4 ms … 19.8 ms 63 runs\n + Time (mean ± σ): 19.1 ms ± 0.9 ms [User: 8.9 ms, System: 9.9 ms]\n + Range (min … max): 18.4 ms … 26.7 ms 72 runs\n \n Benchmark 3: update-ref: create many refs (refcount = 10000, revision = HEAD~)\n - Time (mean ± σ): 1.310 s ± 0.014 s [User: 0.566 s, System: 0.724 s]\n - Range (min … max): 1.291 s … 1.342 s 10 runs\n + Time (mean ± σ): 1.336 s ± 0.018 s [User: 0.590 s, System: 0.724 s]\n + Range (min … max): 1.314 s … 1.373 s 10 runs\n \n Benchmark 4: update-ref: create many refs (refcount = 1, revision = HEAD)\n - Time (mean ± σ): 5.7 ms ± 0.4 ms [User: 2.6 ms, System: 3.1 ms]\n - Range (min … max): 5.1 ms … 9.5 ms 91 runs\n + Time (mean ± σ): 5.1 ms ± 0.2 ms [User: 2.4 ms, System: 2.6 ms]\n + Range (min … max): 4.8 ms … 7.2 ms 109 runs\n \n Benchmark 5: update-ref: create many refs (refcount = 100, revision = HEAD)\n - Time (mean ± σ): 15.2 ms ± 0.3 ms [User: 7.0 ms, System: 8.1 ms]\n - Range (min … max): 14.3 ms … 17.1 ms 71 runs\n + Time (mean ± σ): 14.8 ms ± 0.2 ms [User: 7.1 ms, System: 7.5 ms]\n + Range (min … max): 14.2 ms … 15.2 ms 82 runs\n \n Benchmark 6: update-ref: create many refs (refcount = 10000, revision = HEAD)\n - Time (mean ± σ): 916.1 ms ± 11.0 ms [User: 420.8 ms, System: 495.0 ms]\n - Range (min … max): 906.9 ms … 943.8 ms 10 runs\n + Time (mean ± σ): 927.6 ms ± 5.3 ms [User: 437.8 ms, System: 489.5 ms]\n + Range (min … max): 919.4 ms … 936.4 ms 10 runs\n \n Summary\n - update-ref: create many refs (refcount = 1, revision = HEAD~) ran\n - 1.01 ± 0.09 times faster than update-ref: create many refs (refcount = 1, revision = HEAD)\n - 2.72 ± 0.11 times faster than update-ref: create many refs (refcount = 100, revision = HEAD)\n - 3.42 ± 0.13 times faster than update-ref: create many refs (refcount = 100, revision = HEAD~)\n - 163.59 ± 5.62 times faster than update-ref: create many refs (refcount = 10000, revision = HEAD)\n - 233.91 ± 7.92 times faster than update-ref: create many refs (refcount = 10000, revision = HEAD~)\n + update-ref: create many refs (refcount = 1, revision = HEAD) ran\n + 1.00 ± 0.07 times faster than update-ref: create many refs (refcount = 1, revision = HEAD~)\n + 2.89 ± 0.14 times faster than update-ref: create many refs (refcount = 100, revision = HEAD)\n + 3.74 ± 0.25 times faster than update-ref: create many refs (refcount = 100, revision = HEAD~)\n + 181.26 ± 8.30 times faster than update-ref: create many refs (refcount = 10000, revision = HEAD)\n + 261.01 ± 12.35 times faster than update-ref: create many refs (refcount = 10000, revision = HEAD~)\n +\n + Signed-off-by: Patrick Steinhardt \n \n ## reftable/stack.c ##\n @@ reftable/stack.c: void reftable_stack_destroy(struct reftable_stack *st)\n @@ reftable/stack.c: static int reftable_stack_reload_maybe_reuse(struct reftable_s\n +\tstat_validity_update(&st->list_validity, fd);\n +\n out:\n ++\tif (err)\n ++\t\tstat_validity_clear(&st->list_validity);\n \tif (fd >= 0)\n \t\tclose(fd);\n + \tfree_names(names);\n @@ reftable/stack.c: static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n static int stack_uptodate(struct reftable_stack *st)\n {\n-: ---------- > 4: 96e79dc3ba reftable/blocksource: refactor code to match our coding style\n4: a23f38a806 ! 5: e53a20c8e1 reftable/blocksource: use mmap to read tables\n @@ Commit message\n already and ignores any such errors. Also, `reftable_stack_clean()` will\n prune stale tables which are not referenced by \"tables.list\" anymore so\n that those files can eventually be pruned. And second, we never rewrite\n - already-rewritten stacks, so it does not matter that we cannot rename a\n + already-written stacks, so it does not matter that we cannot rename a\n file over an mmaped file, either.\n \n Another unfortunate property of mmap is that it is not supported by all\n systems. But given that the size of reftables should typically be rather\n limited (megabytes at most in the vast majority of repositories), we can\n - provide an easy fallback by just reading the complete table into memory\n - on such platforms. This is the same strategy that the \"packed\" backend\n - uses in case the platform does not provide mmap.\n + use the fallback implementation provided by `git_mmap()` which reads the\n + whole file into memory instead. This is the same strategy that the\n + \"packed\" backend uses.\n \n While this change doesn't significantly improve performance in the case\n where we're seeking through stacks once (like e.g. git-for-each-ref(1)\n @@ Commit message\n empty repository:\n \n Benchmark 1: update-ref: create many refs (refcount = 1, revision = HEAD~)\n - Time (mean ± σ): 5.6 ms ± 0.2 ms [User: 2.7 ms, System: 2.8 ms]\n - Range (min … max): 5.1 ms … 6.0 ms 90 runs\n + Time (mean ± σ): 5.1 ms ± 0.2 ms [User: 2.5 ms, System: 2.5 ms]\n + Range (min … max): 4.8 ms … 7.1 ms 111 runs\n \n Benchmark 2: update-ref: create many refs (refcount = 100, revision = HEAD~)\n - Time (mean ± σ): 15.1 ms ± 0.4 ms [User: 7.1 ms, System: 8.0 ms]\n - Range (min … max): 14.2 ms … 16.5 ms 71 runs\n + Time (mean ± σ): 14.8 ms ± 0.5 ms [User: 7.1 ms, System: 7.5 ms]\n + Range (min … max): 14.1 ms … 18.7 ms 84 runs\n \n Benchmark 3: update-ref: create many refs (refcount = 10000, revision = HEAD~)\n - Time (mean ± σ): 919.4 ms ± 11.2 ms [User: 427.5 ms, System: 491.7 ms]\n - Range (min … max): 908.1 ms … 936.6 ms 10 runs\n + Time (mean ± σ): 926.4 ms ± 5.6 ms [User: 448.5 ms, System: 477.7 ms]\n + Range (min … max): 920.0 ms … 936.1 ms 10 runs\n \n Benchmark 4: update-ref: create many refs (refcount = 1, revision = HEAD)\n - Time (mean ± σ): 5.5 ms ± 0.3 ms [User: 2.4 ms, System: 3.1 ms]\n - Range (min … max): 5.0 ms … 7.3 ms 89 runs\n + Time (mean ± σ): 5.0 ms ± 0.2 ms [User: 2.4 ms, System: 2.5 ms]\n + Range (min … max): 4.7 ms … 5.4 ms 111 runs\n \n Benchmark 5: update-ref: create many refs (refcount = 100, revision = HEAD)\n - Time (mean ± σ): 10.4 ms ± 0.3 ms [User: 5.1 ms, System: 5.3 ms]\n - Range (min … max): 9.7 ms … 11.0 ms 78 runs\n + Time (mean ± σ): 10.5 ms ± 0.2 ms [User: 5.0 ms, System: 5.3 ms]\n + Range (min … max): 10.0 ms … 10.9 ms 93 runs\n \n Benchmark 6: update-ref: create many refs (refcount = 10000, revision = HEAD)\n - Time (mean ± σ): 483.5 ms ± 6.3 ms [User: 227.8 ms, System: 255.6 ms]\n - Range (min … max): 477.5 ms … 498.8 ms 10 runs\n + Time (mean ± σ): 529.6 ms ± 9.1 ms [User: 268.0 ms, System: 261.4 ms]\n + Range (min … max): 522.4 ms … 547.1 ms 10 runs\n \n Summary\n update-ref: create many refs (refcount = 1, revision = HEAD) ran\n 1.01 ± 0.06 times faster than update-ref: create many refs (refcount = 1, revision = HEAD~)\n - 1.89 ± 0.11 times faster than update-ref: create many refs (refcount = 100, revision = HEAD)\n - 2.73 ± 0.16 times faster than update-ref: create many refs (refcount = 100, revision = HEAD~)\n - 87.55 ± 4.65 times faster than update-ref: create many refs (refcount = 10000, revision = HEAD)\n - 166.48 ± 8.80 times faster than update-ref: create many refs (refcount = 10000, revision = HEAD~)\n + 2.08 ± 0.07 times faster than update-ref: create many refs (refcount = 100, revision = HEAD)\n + 2.95 ± 0.14 times faster than update-ref: create many refs (refcount = 100, revision = HEAD~)\n + 105.33 ± 3.76 times faster than update-ref: create many refs (refcount = 10000, revision = HEAD)\n + 184.24 ± 5.89 times faster than update-ref: create many refs (refcount = 10000, revision = HEAD~)\n \n Theoretically, we could also replicate the strategy of the \"packed\"\n backend where small tables are read into memory instead of using mmap.\n Benchmarks did not confirm that this has a performance benefit though.\n \n + Signed-off-by: Patrick Steinhardt \n +\n ## reftable/blocksource.c ##\n -@@ reftable/blocksource.c: license that can be found in the LICENSE file or at\n - #include \"reftable-blocksource.h\"\n - #include \"reftable-error.h\"\n - \n -+#if defined(NO_MMAP)\n -+static int use_mmap = 0;\n -+#else\n -+static int use_mmap = 1;\n -+#endif\n -+\n - static void strbuf_return_block(void *b, struct reftable_block *dest)\n - {\n - \tif (dest->len)\n @@ reftable/blocksource.c: struct reftable_block_source malloc_block_source(void)\n + }\n + \n struct file_block_source {\n - \tint fd;\n +-\tint fd;\n \tuint64_t size;\n +\tunsigned char *data;\n };\n @@ reftable/blocksource.c: static uint64_t file_size(void *b)\n -\tif (fd > 0) {\n -\t\tclose(fd);\n -\t\t((struct file_block_source *)b)->fd = 0;\n +-\t}\n +-\n +\tstruct file_block_source *b = v;\n -+\n -+\tif (b->fd >= 0) {\n -+\t\tclose(b->fd);\n -+\t\tb->fd = -1;\n - \t}\n - \n -+\tif (use_mmap)\n -+\t\tmunmap(b->data, b->size);\n -+\telse\n -+\t\treftable_free(b->data);\n -+\tb->data = NULL;\n -+\n ++\tmunmap(b->data, b->size);\n \treftable_free(b);\n }\n \n @@ reftable/blocksource.c: static int file_read_block(void *v, struct reftable_bloc\n \treturn size;\n }\n @@ reftable/blocksource.c: int reftable_block_source_from_file(struct reftable_block_source *bs,\n - {\n - \tstruct stat st = { 0 };\n - \tint err = 0;\n --\tint fd = open(name, O_RDONLY);\n -+\tint fd;\n - \tstruct file_block_source *p = NULL;\n -+\n -+\tfd = open(name, O_RDONLY);\n - \tif (fd < 0) {\n - \t\tif (errno == ENOENT) {\n - \t\t\treturn REFTABLE_NOT_EXIST_ERROR;\n -@@ reftable/blocksource.c: int reftable_block_source_from_file(struct reftable_block_source *bs,\n \n - \tp = reftable_calloc(sizeof(struct file_block_source));\n + \tp = reftable_calloc(sizeof(*p));\n \tp->size = st.st_size;\n -\tp->fd = fd;\n -+\tif (use_mmap) {\n -+\t\tp->data = xmmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);\n -+\t\tp->fd = fd;\n -+\t} else {\n -+\t\tp->data = xmalloc(st.st_size);\n -+\t\tif (read_in_full(fd, p->data, st.st_size) != st.st_size) {\n -+\t\t\tclose(fd);\n -+\t\t\treturn -1;\n -+\t\t}\n -+\t\tclose(fd);\n -+\t\tp->fd = -1;\n -+\t}\n ++\tp->data = xmmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);\n ++\tclose(fd);\n \n \tassert(!bs->ops);\n \tbs->ops = &file_vtable;\n\nbase-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b\n-- \n2.43.GIT\n\n"},{"id":"486612","messageId":"4b7f52c415591616d0af69e43c1f8597fcf24634.1704966670.git.ps@pks.im","threadId":"60701","inReplyTo":"cover.1704966670.git.ps@pks.im","subject":"[PATCH v2 1/5] reftable/stack: refactor stack reloading to have common exit path","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-11T10:06:39Z","receivedAt":"2024-01-11T10:06:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `reftable_stack_reload_maybe_reuse()` function is responsible for\nreloading the reftable list from disk. The function is quite hard to\nfollow though because it has a bunch of different exit paths, many of\nwhich have to free the same set of resources.\n\nRefactor the function to have a common exit path. While at it, touch up\nthe style of this function a bit to match our usual coding style better.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 86 +++++++++++++++++++++++-------------------------\n 1 file changed, 42 insertions(+), 44 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 16bab82063..bf869a6772 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -304,69 +304,67 @@ static int tv_cmp(struct timeval *a, struct timeval *b)\n static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n \t\t\t\t\t int reuse_open)\n {\n-\tstruct timeval deadline = { 0 };\n-\tint err = gettimeofday(&deadline, NULL);\n+\tchar **names = NULL, **names_after = NULL;\n+\tstruct timeval deadline;\n \tint64_t delay = 0;\n-\tint tries = 0;\n-\tif (err < 0)\n-\t\treturn err;\n+\tint tries = 0, err;\n \n+\terr = gettimeofday(&deadline, NULL);\n+\tif (err < 0)\n+\t\tgoto out;\n \tdeadline.tv_sec += 3;\n+\n \twhile (1) {\n-\t\tchar **names = NULL;\n-\t\tchar **names_after = NULL;\n-\t\tstruct timeval now = { 0 };\n-\t\tint err = gettimeofday(&now, NULL);\n-\t\tint err2 = 0;\n-\t\tif (err < 0) {\n-\t\t\treturn err;\n-\t\t}\n+\t\tstruct timeval now;\n \n-\t\t/* Only look at deadlines after the first few times. This\n-\t\t simplifies debugging in GDB */\n+\t\terr = gettimeofday(&now, NULL);\n+\t\tif (err < 0)\n+\t\t\tgoto out;\n+\n+\t\t/*\n+\t\t * Only look at deadlines after the first few times. This\n+\t\t * simplifies debugging in GDB.\n+\t\t */\n \t\ttries++;\n-\t\tif (tries > 3 && tv_cmp(&now, &deadline) >= 0) {\n-\t\t\tbreak;\n-\t\t}\n+\t\tif (tries > 3 && tv_cmp(&now, &deadline) >= 0)\n+\t\t\tgoto out;\n \n \t\terr = read_lines(st->list_file, &names);\n-\t\tif (err < 0) {\n-\t\t\tfree_names(names);\n-\t\t\treturn err;\n-\t\t}\n+\t\tif (err < 0)\n+\t\t\tgoto out;\n+\n \t\terr = reftable_stack_reload_once(st, names, reuse_open);\n-\t\tif (err == 0) {\n-\t\t\tfree_names(names);\n+\t\tif (!err)\n \t\t\tbreak;\n-\t\t}\n-\t\tif (err != REFTABLE_NOT_EXIST_ERROR) {\n-\t\t\tfree_names(names);\n-\t\t\treturn err;\n-\t\t}\n-\n-\t\t/* err == REFTABLE_NOT_EXIST_ERROR can be caused by a concurrent\n-\t\t writer. Check if there was one by checking if the name list\n-\t\t changed.\n-\t\t*/\n-\t\terr2 = read_lines(st->list_file, &names_after);\n-\t\tif (err2 < 0) {\n-\t\t\tfree_names(names);\n-\t\t\treturn err2;\n-\t\t}\n-\n+\t\tif (err != REFTABLE_NOT_EXIST_ERROR)\n+\t\t\tgoto out;\n+\n+\t\t/*\n+\t\t * REFTABLE_NOT_EXIST_ERROR can be caused by a concurrent\n+\t\t * writer. Check if there was one by checking if the name list\n+\t\t * changed.\n+\t\t */\n+\t\terr = read_lines(st->list_file, &names_after);\n+\t\tif (err < 0)\n+\t\t\tgoto out;\n \t\tif (names_equal(names_after, names)) {\n-\t\t\tfree_names(names);\n-\t\t\tfree_names(names_after);\n-\t\t\treturn err;\n+\t\t\terr = REFTABLE_NOT_EXIST_ERROR;\n+\t\t\tgoto out;\n \t\t}\n+\n \t\tfree_names(names);\n+\t\tnames = NULL;\n \t\tfree_names(names_after);\n+\t\tnames_after = NULL;\n \n \t\tdelay = delay + (delay * rand()) / RAND_MAX + 1;\n \t\tsleep_millisec(delay);\n \t}\n \n-\treturn 0;\n+out:\n+\tfree_names(names);\n+\tfree_names(names_after);\n+\treturn err;\n }\n \n /* -1 = error\n-- \n2.43.GIT\n\n"},{"id":"486613","messageId":"36b9f6b6240686cc5b0a761b889614fc31f01d34.1704966670.git.ps@pks.im","threadId":"60701","inReplyTo":"cover.1704966670.git.ps@pks.im","subject":"[PATCH v2 2/5] reftable/stack: refactor reloading to use file descriptor","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-11T10:06:43Z","receivedAt":"2024-01-11T10:06:48Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We're about to introduce a stat(3P)-based caching mechanism to reload\nthe list of stacks only when it has changed. In order to avoid race\nconditions this requires us to have a file descriptor available that we\ncan use to call fstat(3P) on.\n\nPrepare for this by converting the code to use `fd_read_lines()` so that\nwe have the file descriptor readily available.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 21 ++++++++++++++++++---\n 1 file changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex bf869a6772..b1ee247601 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -308,6 +308,7 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n \tstruct timeval deadline;\n \tint64_t delay = 0;\n \tint tries = 0, err;\n+\tint fd = -1;\n \n \terr = gettimeofday(&deadline, NULL);\n \tif (err < 0)\n@@ -329,9 +330,19 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n \t\tif (tries > 3 && tv_cmp(&now, &deadline) >= 0)\n \t\t\tgoto out;\n \n-\t\terr = read_lines(st->list_file, &names);\n-\t\tif (err < 0)\n-\t\t\tgoto out;\n+\t\tfd = open(st->list_file, O_RDONLY);\n+\t\tif (fd < 0) {\n+\t\t\tif (errno != ENOENT) {\n+\t\t\t\terr = REFTABLE_IO_ERROR;\n+\t\t\t\tgoto out;\n+\t\t\t}\n+\n+\t\t\tnames = reftable_calloc(sizeof(char *));\n+\t\t} else {\n+\t\t\terr = fd_read_lines(fd, &names);\n+\t\t\tif (err < 0)\n+\t\t\t\tgoto out;\n+\t\t}\n \n \t\terr = reftable_stack_reload_once(st, names, reuse_open);\n \t\tif (!err)\n@@ -356,12 +367,16 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n \t\tnames = NULL;\n \t\tfree_names(names_after);\n \t\tnames_after = NULL;\n+\t\tclose(fd);\n+\t\tfd = -1;\n \n \t\tdelay = delay + (delay * rand()) / RAND_MAX + 1;\n \t\tsleep_millisec(delay);\n \t}\n \n out:\n+\tif (fd >= 0)\n+\t\tclose(fd);\n \tfree_names(names);\n \tfree_names(names_after);\n \treturn err;\n-- \n2.43.GIT\n\n"},{"id":"486614","messageId":"c0f7cec95bd78231bbbce2a30662b71fe00b091d.1704966670.git.ps@pks.im","threadId":"60701","inReplyTo":"cover.1704966670.git.ps@pks.im","subject":"[PATCH v2 3/5] reftable/stack: use stat info to avoid re-reading stack list","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-11T10:06:48Z","receivedAt":"2024-01-11T10:06:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Whenever we call into the refs interfaces we potentially have to reload\nrefs in case they have been concurrently modified, either in-process or\nexternally. While this happens somewhat automatically for loose refs\nbecause we simply try to re-read the files, the \"packed\" backend will\nreload its snapshot of the packed-refs file in case its stat info has\nchanged since last reading it.\n\nIn the reftable backend we have a similar mechanism that is provided by\n`reftable_stack_reload()`. This function will read the list of stacks\nfrom \"tables.list\" and, if they have changed from the currently stored\nlist, reload the stacks. This is heavily inefficient though, as we have\nto check whether the stack is up-to-date on basically every read and\nthus keep on re-reading the file all the time even if it didn't change\nat all.\n\nWe can do better and use the same stat(3P)-based mechanism that the\n\"packed\" backend uses. Instead of reading the file, we will only open\nthe file descriptor, fstat(3P) it, and then compare the info against the\ncached value from the last time we have updated the stack. This should\nalways work alright because \"tables.list\" is updated atomically via a\nrename, so even if the ctime or mtime wasn't granular enough to identify\na change, at least the inode number or file size should have changed.\n\nThis change significantly speeds up operations where many refs are read,\nlike when using git-update-ref(1). The following benchmark creates N\nrefs in an otherwise-empty repository via `git update-ref --stdin`:\n\n Benchmark 1: update-ref: create many refs (refcount = 1, revision = HEAD~)\n Time (mean ± σ): 5.1 ms ± 0.2 ms [User: 2.4 ms, System: 2.6 ms]\n Range (min … max): 4.8 ms … 7.2 ms 109 runs\n\n Benchmark 2: update-ref: create many refs (refcount = 100, revision = HEAD~)\n Time (mean ± σ): 19.1 ms ± 0.9 ms [User: 8.9 ms, System: 9.9 ms]\n Range (min … max): 18.4 ms … 26.7 ms 72 runs\n\n Benchmark 3: update-ref: create many refs (refcount = 10000, revision = HEAD~)\n Time (mean ± σ): 1.336 s ± 0.018 s [User: 0.590 s, System: 0.724 s]\n Range (min … max): 1.314 s … 1.373 s 10 runs\n\n Benchmark 4: update-ref: create many refs (refcount = 1, revision = HEAD)\n Time (mean ± σ): 5.1 ms ± 0.2 ms [User: 2.4 ms, System: 2.6 ms]\n Range (min … max): 4.8 ms … 7.2 ms 109 runs\n\n Benchmark 5: update-ref: create many refs (refcount = 100, revision = HEAD)\n Time (mean ± σ): 14.8 ms ± 0.2 ms [User: 7.1 ms, System: 7.5 ms]\n Range (min … max): 14.2 ms … 15.2 ms 82 runs\n\n Benchmark 6: update-ref: create many refs (refcount = 10000, revision = HEAD)\n Time (mean ± σ): 927.6 ms ± 5.3 ms [User: 437.8 ms, System: 489.5 ms]\n Range (min … max): 919.4 ms … 936.4 ms 10 runs\n\n Summary\n update-ref: create many refs (refcount = 1, revision = HEAD) ran\n 1.00 ± 0.07 times faster than update-ref: create many refs (refcount = 1, revision = HEAD~)\n 2.89 ± 0.14 times faster than update-ref: create many refs (refcount = 100, revision = HEAD)\n 3.74 ± 0.25 times faster than update-ref: create many refs (refcount = 100, revision = HEAD~)\n 181.26 ± 8.30 times faster than update-ref: create many refs (refcount = 10000, revision = HEAD)\n 261.01 ± 12.35 times faster than update-ref: create many refs (refcount = 10000, revision = HEAD~)\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/stack.c | 12 +++++++++++-\n reftable/stack.h | 1 +\n reftable/system.h | 1 +\n 3 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex b1ee247601..c28d82299d 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -175,6 +175,7 @@ void reftable_stack_destroy(struct reftable_stack *st)\n \t\tst->readers_len = 0;\n \t\tFREE_AND_NULL(st->readers);\n \t}\n+\tstat_validity_clear(&st->list_validity);\n \tFREE_AND_NULL(st->list_file);\n \tFREE_AND_NULL(st->reftable_dir);\n \treftable_free(st);\n@@ -374,7 +375,11 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n \t\tsleep_millisec(delay);\n \t}\n \n+\tstat_validity_update(&st->list_validity, fd);\n+\n out:\n+\tif (err)\n+\t\tstat_validity_clear(&st->list_validity);\n \tif (fd >= 0)\n \t\tclose(fd);\n \tfree_names(names);\n@@ -388,8 +393,13 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n static int stack_uptodate(struct reftable_stack *st)\n {\n \tchar **names = NULL;\n-\tint err = read_lines(st->list_file, &names);\n+\tint err;\n \tint i = 0;\n+\n+\tif (stat_validity_check(&st->list_validity, st->list_file))\n+\t\treturn 0;\n+\n+\terr = read_lines(st->list_file, &names);\n \tif (err < 0)\n \t\treturn err;\n \ndiff --git a/reftable/stack.h b/reftable/stack.h\nindex f57005846e..3f80cc598a 100644\n--- a/reftable/stack.h\n+++ b/reftable/stack.h\n@@ -14,6 +14,7 @@ license that can be found in the LICENSE file or at\n #include \"reftable-stack.h\"\n \n struct reftable_stack {\n+\tstruct stat_validity list_validity;\n \tchar *list_file;\n \tchar *reftable_dir;\n \tint disable_auto_compact;\ndiff --git a/reftable/system.h b/reftable/system.h\nindex 6b74a81514..2cc7adf271 100644\n--- a/reftable/system.h\n+++ b/reftable/system.h\n@@ -12,6 +12,7 @@ license that can be found in the LICENSE file or at\n /* This header glues the reftable library to the rest of Git */\n \n #include \"git-compat-util.h\"\n+#include \"statinfo.h\"\n #include \"strbuf.h\"\n #include \"hash-ll.h\" /* hash ID, sizes.*/\n #include \"dir.h\" /* remove_dir_recursively, for tests.*/\n-- \n2.43.GIT\n\n"},{"id":"486615","messageId":"96e79dc3ba011915924a8a4766eba426ef83abb5.1704966670.git.ps@pks.im","threadId":"60701","inReplyTo":"cover.1704966670.git.ps@pks.im","subject":"[PATCH v2 4/5] reftable/blocksource: refactor code to match our coding style","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-11T10:06:52Z","receivedAt":"2024-01-11T10:06:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Refactor `reftable_block_source_from_file()` to match our coding style\nbetter.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/blocksource.c | 17 ++++++++---------\n 1 file changed, 8 insertions(+), 9 deletions(-)\n\ndiff --git a/reftable/blocksource.c b/reftable/blocksource.c\nindex a1ea304429..1e2fb5e9fd 100644\n--- a/reftable/blocksource.c\n+++ b/reftable/blocksource.c\n@@ -125,24 +125,23 @@ static struct reftable_block_source_vtable file_vtable = {\n int reftable_block_source_from_file(struct reftable_block_source *bs,\n \t\t\t\t const char *name)\n {\n-\tstruct stat st = { 0 };\n-\tint err = 0;\n-\tint fd = open(name, O_RDONLY);\n-\tstruct file_block_source *p = NULL;\n+\tstruct file_block_source *p;\n+\tstruct stat st;\n+\tint fd;\n+\n+\tfd = open(name, O_RDONLY);\n \tif (fd < 0) {\n-\t\tif (errno == ENOENT) {\n+\t\tif (errno == ENOENT)\n \t\t\treturn REFTABLE_NOT_EXIST_ERROR;\n-\t\t}\n \t\treturn -1;\n \t}\n \n-\terr = fstat(fd, &st);\n-\tif (err < 0) {\n+\tif (fstat(fd, &st) < 0) {\n \t\tclose(fd);\n \t\treturn REFTABLE_IO_ERROR;\n \t}\n \n-\tp = reftable_calloc(sizeof(struct file_block_source));\n+\tp = reftable_calloc(sizeof(*p));\n \tp->size = st.st_size;\n \tp->fd = fd;\n \n-- \n2.43.GIT\n\n"},{"id":"486616","messageId":"e53a20c8e1a5f2169da4fddf2d430d584a7731d1.1704966670.git.ps@pks.im","threadId":"60701","inReplyTo":"cover.1704966670.git.ps@pks.im","subject":"[PATCH v2 5/5] reftable/blocksource: use mmap to read tables","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-11T10:06:56Z","receivedAt":"2024-01-11T10:07:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The blocksource interface provides an interface to read blocks from a\nreftable table. This interface is implemented using read(3P) calls on\nthe underlying file descriptor. While this works alright, this pattern\nis very inefficient when repeatedly querying the reftable stack for one\nor more refs. This inefficiency can mostly be attributed to the fact\nthat we often need to re-read the same blocks over and over again, and\nevery single time we need to call read(3P) again.\n\nA natural fit in this context is to use mmap(3P) instead of read(3P),\nwhich has a bunch of benefits:\n\n - We do not need to come up with a caching strategy for some of the\n blocks as this will be handled by the kernel already.\n\n - We can avoid the overhead of having to call into the read(3P)\n syscall repeatedly.\n\n - We do not need to allocate returned blocks repeatedly, but can\n instead hand out pointers into the mmapped region directly.\n\nUsing mmap comes with a significant drawback on Windows though, because\nmmapped files cannot be deleted and neither is it possible to rename\nfiles onto an mmapped file. But for one, the reftable library gracefully\nhandles the case where auto-compaction cannot delete a still-open stack\nalready and ignores any such errors. Also, `reftable_stack_clean()` will\nprune stale tables which are not referenced by \"tables.list\" anymore so\nthat those files can eventually be pruned. And second, we never rewrite\nalready-written stacks, so it does not matter that we cannot rename a\nfile over an mmaped file, either.\n\nAnother unfortunate property of mmap is that it is not supported by all\nsystems. But given that the size of reftables should typically be rather\nlimited (megabytes at most in the vast majority of repositories), we can\nuse the fallback implementation provided by `git_mmap()` which reads the\nwhole file into memory instead. This is the same strategy that the\n\"packed\" backend uses.\n\nWhile this change doesn't significantly improve performance in the case\nwhere we're seeking through stacks once (like e.g. git-for-each-ref(1)\nwould). But it does speed up usecases where there is lots of random\naccess to refs, e.g. when writing. The following benchmark demonstrates\nthese savings with git-update-ref(1) creating N refs in an otherwise\nempty repository:\n\n Benchmark 1: update-ref: create many refs (refcount = 1, revision = HEAD~)\n Time (mean ± σ): 5.1 ms ± 0.2 ms [User: 2.5 ms, System: 2.5 ms]\n Range (min … max): 4.8 ms … 7.1 ms 111 runs\n\n Benchmark 2: update-ref: create many refs (refcount = 100, revision = HEAD~)\n Time (mean ± σ): 14.8 ms ± 0.5 ms [User: 7.1 ms, System: 7.5 ms]\n Range (min … max): 14.1 ms … 18.7 ms 84 runs\n\n Benchmark 3: update-ref: create many refs (refcount = 10000, revision = HEAD~)\n Time (mean ± σ): 926.4 ms ± 5.6 ms [User: 448.5 ms, System: 477.7 ms]\n Range (min … max): 920.0 ms … 936.1 ms 10 runs\n\n Benchmark 4: update-ref: create many refs (refcount = 1, revision = HEAD)\n Time (mean ± σ): 5.0 ms ± 0.2 ms [User: 2.4 ms, System: 2.5 ms]\n Range (min … max): 4.7 ms … 5.4 ms 111 runs\n\n Benchmark 5: update-ref: create many refs (refcount = 100, revision = HEAD)\n Time (mean ± σ): 10.5 ms ± 0.2 ms [User: 5.0 ms, System: 5.3 ms]\n Range (min … max): 10.0 ms … 10.9 ms 93 runs\n\n Benchmark 6: update-ref: create many refs (refcount = 10000, revision = HEAD)\n Time (mean ± σ): 529.6 ms ± 9.1 ms [User: 268.0 ms, System: 261.4 ms]\n Range (min … max): 522.4 ms … 547.1 ms 10 runs\n\n Summary\n update-ref: create many refs (refcount = 1, revision = HEAD) ran\n 1.01 ± 0.06 times faster than update-ref: create many refs (refcount = 1, revision = HEAD~)\n 2.08 ± 0.07 times faster than update-ref: create many refs (refcount = 100, revision = HEAD)\n 2.95 ± 0.14 times faster than update-ref: create many refs (refcount = 100, revision = HEAD~)\n 105.33 ± 3.76 times faster than update-ref: create many refs (refcount = 10000, revision = HEAD)\n 184.24 ± 5.89 times faster than update-ref: create many refs (refcount = 10000, revision = HEAD~)\n\nTheoretically, we could also replicate the strategy of the \"packed\"\nbackend where small tables are read into memory instead of using mmap.\nBenchmarks did not confirm that this has a performance benefit though.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/blocksource.c | 22 +++++++---------------\n 1 file changed, 7 insertions(+), 15 deletions(-)\n\ndiff --git a/reftable/blocksource.c b/reftable/blocksource.c\nindex 1e2fb5e9fd..8c41e3c70f 100644\n--- a/reftable/blocksource.c\n+++ b/reftable/blocksource.c\n@@ -76,8 +76,8 @@ struct reftable_block_source malloc_block_source(void)\n }\n \n struct file_block_source {\n-\tint fd;\n \tuint64_t size;\n+\tunsigned char *data;\n };\n \n static uint64_t file_size(void *b)\n@@ -87,19 +87,12 @@ static uint64_t file_size(void *b)\n \n static void file_return_block(void *b, struct reftable_block *dest)\n {\n-\tif (dest->len)\n-\t\tmemset(dest->data, 0xff, dest->len);\n-\treftable_free(dest->data);\n }\n \n-static void file_close(void *b)\n+static void file_close(void *v)\n {\n-\tint fd = ((struct file_block_source *)b)->fd;\n-\tif (fd > 0) {\n-\t\tclose(fd);\n-\t\t((struct file_block_source *)b)->fd = 0;\n-\t}\n-\n+\tstruct file_block_source *b = v;\n+\tmunmap(b->data, b->size);\n \treftable_free(b);\n }\n \n@@ -108,9 +101,7 @@ static int file_read_block(void *v, struct reftable_block *dest, uint64_t off,\n {\n \tstruct file_block_source *b = v;\n \tassert(off + size <= b->size);\n-\tdest->data = reftable_malloc(size);\n-\tif (pread_in_full(b->fd, dest->data, size, off) != size)\n-\t\treturn -1;\n+\tdest->data = b->data + off;\n \tdest->len = size;\n \treturn size;\n }\n@@ -143,7 +134,8 @@ int reftable_block_source_from_file(struct reftable_block_source *bs,\n \n \tp = reftable_calloc(sizeof(*p));\n \tp->size = st.st_size;\n-\tp->fd = fd;\n+\tp->data = xmmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\tclose(fd);\n \n \tassert(!bs->ops);\n \tbs->ops = &file_vtable;\n-- \n2.43.GIT\n\n"},{"id":"486774","messageId":"20240114101424.GA1196682@coredump.intra.peff.net","threadId":"60701","inReplyTo":"36b9f6b6240686cc5b0a761b889614fc31f01d34.1704966670.git.ps@pks.im","subject":"Re: [PATCH v2 2/5] reftable/stack: refactor reloading to use file descriptor","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-14T10:14:24Z","receivedAt":"2024-01-14T10:21:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 11, 2024 at 11:06:43AM +0100, Patrick Steinhardt wrote:\n\n> We're about to introduce a stat(3P)-based caching mechanism to reload\n> the list of stacks only when it has changed. In order to avoid race\n> conditions this requires us to have a file descriptor available that we\n> can use to call fstat(3P) on.\n> \n> Prepare for this by converting the code to use `fd_read_lines()` so that\n> we have the file descriptor readily available.\n\nCoverity noted a case with this series where we might feed a negative\nvalue to fstat(). I'm not sure if it's a bug or not.\n\nThe issue is that here:\n\n> @@ -329,9 +330,19 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n> \t\tif (tries > 3 && tv_cmp(&now, &deadline) >= 0)\n> \t\t\tgoto out;\n> \n> -\t\terr = read_lines(st->list_file, &names);\n> -\t\tif (err < 0)\n> -\t\t\tgoto out;\n> +\t\tfd = open(st->list_file, O_RDONLY);\n> +\t\tif (fd < 0) {\n> +\t\t\tif (errno != ENOENT) {\n> +\t\t\t\terr = REFTABLE_IO_ERROR;\n> +\t\t\t\tgoto out;\n> +\t\t\t}\n> +\n> +\t\t\tnames = reftable_calloc(sizeof(char *));\n> +\t\t} else {\n> +\t\t\terr = fd_read_lines(fd, &names);\n> +\t\t\tif (err < 0)\n> +\t\t\t\tgoto out;\n> +\t\t}\n\n...we might end up with fd as \"-1\" after calling open() on the list\nfile. For most errors we'll jump to \"out\", which makes sense. But if we\nget ENOENT, we keep going with an empty file-list, which makes sense.\n\nBut we then do other stuff with \"fd\". I think this case is OK:\n\n> @@ -356,12 +367,16 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n> \t\tnames = NULL;\n> \t\tfree_names(names_after);\n> \t\tnames_after = NULL;\n> +\t\tclose(fd);\n> +\t\tfd = -1;\n\nWe only get here if reftable_stack_reload_once() returned an error,\nwhich it won't do since we feed it a blank set of names (and anyway,\nclose(-1) is a harmless noop).\n\nBut if we actually get to the end of the function, it's more\nquestionable. As of this patch, it's OK:\n\n> \t\tdelay = delay + (delay * rand()) / RAND_MAX + 1;\n> \t\tsleep_millisec(delay);\n> \t}\n> \n> out:\n> +\tif (fd >= 0)\n> +\t\tclose(fd);\n> \tfree_names(names);\n> \tfree_names(names_after);\n> \treturn err;\n\nBut in the next patch we have this hunk:\n\n> @@ -374,7 +375,11 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n> sleep_millisec(delay);\n> }\n> \n> + stat_validity_update(&st->list_validity, fd);\n> +\n> out:\n> + if (err)\n> + stat_validity_clear(&st->list_validity);\n> if (fd >= 0)\n> close(fd);\n> free_names(names);\n\nwhich means we'll feed a negative value to stat_validity_update(). I\nthink this may be OK, because I'd imagine the only sensible thing to do\nis call stat_validity_clear() instead. And using a negative fd means\nfstat() will fail, which will cause stat_validity_update() to clear the\nvalidity struct anyway. But I thought it was worth double-checking.\n\n-Peff\n"},{"id":"486793","messageId":"ZaUC-WevQqOj31u9@tanuki","threadId":"60701","inReplyTo":"20240114101424.GA1196682@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/5] reftable/stack: refactor reloading to use file descriptor","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-15T10:03:37Z","receivedAt":"2024-01-15T10:03:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Jan 14, 2024 at 05:14:24AM -0500, Jeff King wrote:\n> On Thu, Jan 11, 2024 at 11:06:43AM +0100, Patrick Steinhardt wrote:\n> \n> > We're about to introduce a stat(3P)-based caching mechanism to reload\n> > the list of stacks only when it has changed. In order to avoid race\n> > conditions this requires us to have a file descriptor available that we\n> > can use to call fstat(3P) on.\n> > \n> > Prepare for this by converting the code to use `fd_read_lines()` so that\n> > we have the file descriptor readily available.\n> \n> Coverity noted a case with this series where we might feed a negative\n> value to fstat(). I'm not sure if it's a bug or not.\n> \n> The issue is that here:\n> \n> > @@ -329,9 +330,19 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n> > \t\tif (tries > 3 && tv_cmp(&now, &deadline) >= 0)\n> > \t\t\tgoto out;\n> > \n> > -\t\terr = read_lines(st->list_file, &names);\n> > -\t\tif (err < 0)\n> > -\t\t\tgoto out;\n> > +\t\tfd = open(st->list_file, O_RDONLY);\n> > +\t\tif (fd < 0) {\n> > +\t\t\tif (errno != ENOENT) {\n> > +\t\t\t\terr = REFTABLE_IO_ERROR;\n> > +\t\t\t\tgoto out;\n> > +\t\t\t}\n> > +\n> > +\t\t\tnames = reftable_calloc(sizeof(char *));\n> > +\t\t} else {\n> > +\t\t\terr = fd_read_lines(fd, &names);\n> > +\t\t\tif (err < 0)\n> > +\t\t\t\tgoto out;\n> > +\t\t}\n> \n> ...we might end up with fd as \"-1\" after calling open() on the list\n> file. For most errors we'll jump to \"out\", which makes sense. But if we\n> get ENOENT, we keep going with an empty file-list, which makes sense.\n> \n> But we then do other stuff with \"fd\". I think this case is OK:\n> \n> > @@ -356,12 +367,16 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n> > \t\tnames = NULL;\n> > \t\tfree_names(names_after);\n> > \t\tnames_after = NULL;\n> > +\t\tclose(fd);\n> > +\t\tfd = -1;\n> \n> We only get here if reftable_stack_reload_once() returned an error,\n> which it won't do since we feed it a blank set of names (and anyway,\n> close(-1) is a harmless noop).\n> \n> But if we actually get to the end of the function, it's more\n> questionable. As of this patch, it's OK:\n> \n> > \t\tdelay = delay + (delay * rand()) / RAND_MAX + 1;\n> > \t\tsleep_millisec(delay);\n> > \t}\n> > \n> > out:\n> > +\tif (fd >= 0)\n> > +\t\tclose(fd);\n> > \tfree_names(names);\n> > \tfree_names(names_after);\n> > \treturn err;\n> \n> But in the next patch we have this hunk:\n> \n> > @@ -374,7 +375,11 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n> > sleep_millisec(delay);\n> > }\n> > \n> > + stat_validity_update(&st->list_validity, fd);\n> > +\n> > out:\n> > + if (err)\n> > + stat_validity_clear(&st->list_validity);\n> > if (fd >= 0)\n> > close(fd);\n> > free_names(names);\n> \n> which means we'll feed a negative value to stat_validity_update(). I\n> think this may be OK, because I'd imagine the only sensible thing to do\n> is call stat_validity_clear() instead. And using a negative fd means\n> fstat() will fail, which will cause stat_validity_update() to clear the\n> validity struct anyway. But I thought it was worth double-checking.\n\nGood catch, and thanks a lot for double-checking. I was briefly\nwondering whether this behaviour is actually specified by POSIX. In any\ncase, fstat(3P) explicitly documents `EBADF` as:\n\n The fildes argument is not a valid file descriptor.\n\nThat makes me think that this code is indeed POSIX-compliant, as\nimplementations are expected to handle invalid file descriptors via this\nerror code.\n\nSo overall this works as intended, even though I would not consider it\nto be the cleanest way to handle this. Unless you or others think that\nthis should be refactored I'll leave it as-is for now though.\n\nPatrick\n"},{"id":"486843","messageId":"20240116151447.GD2119690@coredump.intra.peff.net","threadId":"60701","inReplyTo":"ZaUC-WevQqOj31u9@tanuki","subject":"Re: [PATCH v2 2/5] reftable/stack: refactor reloading to use file descriptor","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-16T15:14:47Z","receivedAt":"2024-01-16T15:14:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 15, 2024 at 11:03:37AM +0100, Patrick Steinhardt wrote:\n\n> > which means we'll feed a negative value to stat_validity_update(). I\n> > think this may be OK, because I'd imagine the only sensible thing to do\n> > is call stat_validity_clear() instead. And using a negative fd means\n> > fstat() will fail, which will cause stat_validity_update() to clear the\n> > validity struct anyway. But I thought it was worth double-checking.\n> \n> Good catch, and thanks a lot for double-checking. I was briefly\n> wondering whether this behaviour is actually specified by POSIX. In any\n> case, fstat(3P) explicitly documents `EBADF` as:\n> \n> The fildes argument is not a valid file descriptor.\n> \n> That makes me think that this code is indeed POSIX-compliant, as\n> implementations are expected to handle invalid file descriptors via this\n> error code.\n> \n> So overall this works as intended, even though I would not consider it\n> to be the cleanest way to handle this. Unless you or others think that\n> this should be refactored I'll leave it as-is for now though.\n\nThanks for confirming. I think we can leave your patch as-is. If\nanything, I would say that stat_validity_update() should check for \"fd <\n0\" itself. Not because I think fstat() is unlikely to behave differently\non some platform, but simply because it more clearly documents the\nexpectation.\n\n-Peff\n"},{"id":"486849","messageId":"xmqqv87t1b4u.fsf@gitster.g","threadId":"60701","inReplyTo":"20240116151447.GD2119690@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/5] reftable/stack: refactor reloading to use file descriptor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-16T16:44:17Z","receivedAt":"2024-01-16T16:44:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> So overall this works as intended, even though I would not consider it\n>> to be the cleanest way to handle this. Unless you or others think that\n>> this should be refactored I'll leave it as-is for now though.\n>\n> Thanks for confirming. I think we can leave your patch as-is. If\n> anything, I would say that stat_validity_update() should check for \"fd <\n> 0\" itself. Not because I think fstat() is unlikely to behave differently\n> on some platform, but simply because it more clearly documents the\n> expectation.\n\nThanks, I agree with your point that we should avoid calling system\nfunctions that take a file descriptor with a known-invalid (like\nnegative) one.\n"}]}