{"thread":{"id":"45686","subject":"[PATCH] repack: respect gc.pid lock","startedAt":"2017-04-13T20:28:58Z","lastAt":"2017-04-20T20:15:33Z","messageCount":13,"participants":["David Turner","Jacob Keller","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"316780","messageId":"20170413202712.22192-1-dturner@twosigma.com","threadId":"45686","inReplyTo":null,"subject":"[PATCH] repack: respect gc.pid lock","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2017-04-13T20:27:12Z","receivedAt":"2017-04-13T20:28:58Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"Git gc locks the repository (using a gc.pid file) so that other gcs\ndon't run concurrently. Make git repack respect this lock.\n\nNow repack, by default, will refuse to run at the same time as a gc.\nThis fixes a concurrency issue: a repack which deleted packs would\nmake a concurrent gc sad when its packs were deleted out from under\nit.  The gc would fail with: \"fatal: ./objects/pack/pack-$sha.pack\ncannot be accessed\".  Then it would die, probably leaving a large temp\npack hanging around.\n\nGit repack learns --no-lock, so that when run under git gc, it doesn't\nattempt to manage the lock itself.\n\nMartin Fick suggested just moving the lock into git repack, but this\nwould leave parts of git gc (e.g. git prune) protected by only local\nlocks.  I worried that a prune (part of git gc) concurrent with a\nrepack could confuse the repack, so I decided to go with this\nsolution.\n\nSigned-off-by: David Turner <dturner@twosigma.com>\n---\n Documentation/git-repack.txt |  5 +++\n Makefile                     |  1 +\n builtin/gc.c                 | 72 ++----------------------------------\n builtin/repack.c             | 13 +++++++\n repack.c                     | 88 ++++++++++++++++++++++++++++++++++++++++++++\n repack.h                     |  8 ++++\n t/t7700-repack.sh            |  8 ++++\n 7 files changed, 127 insertions(+), 68 deletions(-)\n create mode 100644 repack.c\n create mode 100644 repack.h\n\ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex 26afe6ed54..b347ff5c62 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -143,6 +143,11 @@ other objects in that pack they already have locally.\n \tbeing removed. In addition, any unreachable loose objects will\n \tbe packed (and their loose counterparts removed).\n \n+--no-lock::\n+\tDo not lock the repository, and do not respect any existing lock.\n+\tMostly useful for running repack within git gc.  Do not use this\n+\tunless you know what you are doing.\n+\n Configuration\n -------------\n \ndiff --git a/Makefile b/Makefile\nindex 9b36068ac5..7095f03959 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -816,6 +816,7 @@ LIB_OBJS += refs/files-backend.o\n LIB_OBJS += refs/iterator.o\n LIB_OBJS += ref-filter.o\n LIB_OBJS += remote.o\n+LIB_OBJS += repack.o\n LIB_OBJS += replace_object.o\n LIB_OBJS += rerere.o\n LIB_OBJS += resolve-undo.o\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex c2c61a57bb..9b9c27020b 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -18,6 +18,7 @@\n #include \"sigchain.h\"\n #include \"argv-array.h\"\n #include \"commit.h\"\n+#include \"repack.h\"\n \n #define FAILED_RUN \"failed to run %s\"\n \n@@ -45,7 +46,6 @@ static struct argv_array prune = ARGV_ARRAY_INIT;\n static struct argv_array prune_worktrees = ARGV_ARRAY_INIT;\n static struct argv_array rerere = ARGV_ARRAY_INIT;\n \n-static struct tempfile pidfile;\n static struct lock_file log_lock;\n \n static struct string_list pack_garbage = STRING_LIST_INIT_DUP;\n@@ -234,70 +234,6 @@ static int need_to_gc(void)\n \treturn 1;\n }\n \n-/* return NULL on success, else hostname running the gc */\n-static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n-{\n-\tstatic struct lock_file lock;\n-\tchar my_host[128];\n-\tstruct strbuf sb = STRBUF_INIT;\n-\tstruct stat st;\n-\tuintmax_t pid;\n-\tFILE *fp;\n-\tint fd;\n-\tchar *pidfile_path;\n-\n-\tif (is_tempfile_active(&pidfile))\n-\t\t/* already locked */\n-\t\treturn NULL;\n-\n-\tif (gethostname(my_host, sizeof(my_host)))\n-\t\txsnprintf(my_host, sizeof(my_host), \"unknown\");\n-\n-\tpidfile_path = git_pathdup(\"gc.pid\");\n-\tfd = hold_lock_file_for_update(&lock, pidfile_path,\n-\t\t\t\t       LOCK_DIE_ON_ERROR);\n-\tif (!force) {\n-\t\tstatic char locking_host[128];\n-\t\tint should_exit;\n-\t\tfp = fopen(pidfile_path, \"r\");\n-\t\tmemset(locking_host, 0, sizeof(locking_host));\n-\t\tshould_exit =\n-\t\t\tfp != NULL &&\n-\t\t\t!fstat(fileno(fp), &st) &&\n-\t\t\t/*\n-\t\t\t * 12 hour limit is very generous as gc should\n-\t\t\t * never take that long. On the other hand we\n-\t\t\t * don't really need a strict limit here,\n-\t\t\t * running gc --auto one day late is not a big\n-\t\t\t * problem. --force can be used in manual gc\n-\t\t\t * after the user verifies that no gc is\n-\t\t\t * running.\n-\t\t\t */\n-\t\t\ttime(NULL) - st.st_mtime <= 12 * 3600 &&\n-\t\t\tfscanf(fp, \"%\"SCNuMAX\" %127c\", &pid, locking_host) == 2 &&\n-\t\t\t/* be gentle to concurrent \"gc\" on remote hosts */\n-\t\t\t(strcmp(locking_host, my_host) || !kill(pid, 0) || errno == EPERM);\n-\t\tif (fp != NULL)\n-\t\t\tfclose(fp);\n-\t\tif (should_exit) {\n-\t\t\tif (fd >= 0)\n-\t\t\t\trollback_lock_file(&lock);\n-\t\t\t*ret_pid = pid;\n-\t\t\tfree(pidfile_path);\n-\t\t\treturn locking_host;\n-\t\t}\n-\t}\n-\n-\tstrbuf_addf(&sb, \"%\"PRIuMAX\" %s\",\n-\t\t    (uintmax_t) getpid(), my_host);\n-\twrite_in_full(fd, sb.buf, sb.len);\n-\tstrbuf_release(&sb);\n-\tcommit_lock_file(&lock);\n-\tregister_tempfile(&pidfile, pidfile_path);\n-\tfree(pidfile_path);\n-\treturn NULL;\n-}\n-\n static int report_last_gc_error(void)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n@@ -370,7 +306,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \n \targv_array_pushl(&pack_refs_cmd, \"pack-refs\", \"--all\", \"--prune\", NULL);\n \targv_array_pushl(&reflog, \"reflog\", \"expire\", \"--all\", NULL);\n-\targv_array_pushl(&repack, \"repack\", \"-d\", \"-l\", NULL);\n+\targv_array_pushl(&repack, \"repack\", \"-d\", \"-l\", \"--no-lock\", NULL);\n \targv_array_pushl(&prune, \"prune\", \"--expire\", NULL);\n \targv_array_pushl(&prune_worktrees, \"worktree\", \"prune\", \"--expire\", NULL);\n \targv_array_pushl(&rerere, \"rerere\", \"gc\", NULL);\n@@ -426,11 +362,11 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t} else\n \t\tadd_repack_all_option();\n \n-\tname = lock_repo_for_gc(force, &pid);\n+\tname = lock_repo_for_pack_manipulation(force, &pid);\n \tif (name) {\n \t\tif (auto_gc)\n \t\t\treturn 0; /* be quiet on --auto */\n-\t\tdie(_(\"gc is already running on machine '%s' pid %\"PRIuMAX\" (use --force if not)\"),\n+\t\tdie(_(\"pack operation (gc or repack) is already running on machine '%s' pid %\"PRIuMAX\" (use --force if not)\"),\n \t\t    name, (uintmax_t)pid);\n \t}\n \ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 677bc7c81a..619ac37a05 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -7,6 +7,7 @@\n #include \"strbuf.h\"\n #include \"string-list.h\"\n #include \"argv-array.h\"\n+#include \"repack.h\"\n \n static int delta_base_offset = 1;\n static int pack_kept_objects = -1;\n@@ -160,6 +161,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tint no_update_server_info = 0;\n \tint quiet = 0;\n \tint local = 0;\n+\tint no_lock = 0;\n \n \tstruct option builtin_repack_options[] = {\n \t\tOPT_BIT('a', NULL, &pack_everything,\n@@ -194,6 +196,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"maximum size of each packfile\")),\n \t\tOPT_BOOL(0, \"pack-kept-objects\", &pack_kept_objects,\n \t\t\t\tN_(\"repack objects in packs marked with .keep\")),\n+\t\tOPT_BOOL(0, \"no-lock\", &no_lock,\n+\t\t\t\tN_(\"Do not lock the repository, and do not respect any existing lock.  Mostly useful for operation within git gc.\")),\n \t\tOPT_END()\n \t};\n \n@@ -215,6 +219,15 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (write_bitmaps && !(pack_everything & ALL_INTO_ONE))\n \t\tdie(_(incremental_bitmap_conflict_error));\n \n+\tif (!no_lock) {\n+\t\tpid_t pid;\n+\t\tconst char *name = lock_repo_for_pack_manipulation(0, &pid);\n+\t\tif (name) {\n+\t\t\tdie(_(\"pack operation (gc or repack) is already running on machine '%s' pid %\"PRIuMAX\" (use --no-lock if not)\"),\n+\t\t\t    name, (uintmax_t)pid);\n+\t\t}\n+\t}\n+\n \tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n \tpacktmp = mkpathdup(\"%s/.tmp-%d-pack\", packdir, (int)getpid());\n \ndiff --git a/repack.c b/repack.c\nnew file mode 100644\nindex 0000000000..a6df28c7f2\n--- /dev/null\n+++ b/repack.c\n@@ -0,0 +1,88 @@\n+#include \"builtin.h\"\n+#include \"repack.h\"\n+#include \"strbuf.h\"\n+#include \"lockfile.h\"\n+#include \"tempfile.h\"\n+\n+static struct tempfile pidfile;\n+\n+/*\n+ * Commands should call this before doing any operation which might\n+ * delete a pack file (e.g. gc or repack).  We don't want to allow\n+ * multiple operations of this type to operate at the same time.\n+ *\n+ * For historical reasons, the pid file created is called \"gc.pid\",\n+ * even though it is also used for git-repack.\n+ *\n+ * The lock will persist until the process ends.\n+ *\n+ * If force is non-zero, any existing lock will be disregarded.\n+ *\n+ * return NULL on success, else hostname running the pack manipulation\n+ * operation.\n+ *\n+ * It is safe to call this function multiple times in the same process;\n+ * calls after the first successful call will always return NULL.\n+ */\n+const char *lock_repo_for_pack_manipulation(int force, pid_t* ret_pid)\n+{\n+\tstatic struct lock_file lock;\n+\tchar my_host[128];\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct stat st;\n+\tuintmax_t pid;\n+\tFILE *fp;\n+\tint fd;\n+\tchar *pidfile_path;\n+\n+\tif (is_tempfile_active(&pidfile))\n+\t\t/* already locked */\n+\t\treturn NULL;\n+\n+\tif (gethostname(my_host, sizeof(my_host)))\n+\t\txsnprintf(my_host, sizeof(my_host), \"unknown\");\n+\n+\tpidfile_path = git_pathdup(\"gc.pid\");\n+\tfd = hold_lock_file_for_update(&lock, pidfile_path,\n+\t\t\t\t       LOCK_DIE_ON_ERROR);\n+\tif (!force) {\n+\t\tstatic char locking_host[128];\n+\t\tint should_exit;\n+\t\tfp = fopen(pidfile_path, \"r\");\n+\t\tmemset(locking_host, 0, sizeof(locking_host));\n+\t\tshould_exit =\n+\t\t\tfp != NULL &&\n+\t\t\t!fstat(fileno(fp), &st) &&\n+\t\t\t/*\n+\t\t\t * 12 hour limit is very generous as gc should\n+\t\t\t * never take that long. On the other hand we\n+\t\t\t * don't really need a strict limit here,\n+\t\t\t * running gc --auto one day late is not a big\n+\t\t\t * problem. --force can be used in manual gc\n+\t\t\t * after the user verifies that no gc is\n+\t\t\t * running.\n+\t\t\t */\n+\t\t\ttime(NULL) - st.st_mtime <= 12 * 3600 &&\n+\t\t\tfscanf(fp, \"%\"SCNuMAX\" %127c\", &pid, locking_host) == 2 &&\n+\t\t\t/* be gentle to concurrent \"gc\" on remote hosts */\n+\t\t\t(strcmp(locking_host, my_host) || !kill(pid, 0) || errno == EPERM);\n+\t\tif (fp != NULL)\n+\t\t\tfclose(fp);\n+\t\tif (should_exit) {\n+\t\t\tif (fd >= 0)\n+\t\t\t\trollback_lock_file(&lock);\n+\t\t\t*ret_pid = pid;\n+\t\t\tfree(pidfile_path);\n+\t\t\treturn locking_host;\n+\t\t}\n+\t}\n+\n+\tstrbuf_addf(&sb, \"%\"PRIuMAX\" %s\",\n+\t\t    (uintmax_t) getpid(), my_host);\n+\twrite_in_full(fd, sb.buf, sb.len);\n+\tstrbuf_release(&sb);\n+\tcommit_lock_file(&lock);\n+\tregister_tempfile(&pidfile, pidfile_path);\n+\tfree(pidfile_path);\n+\treturn NULL;\n+}\ndiff --git a/repack.h b/repack.h\nnew file mode 100644\nindex 0000000000..bf9144ee37\n--- /dev/null\n+++ b/repack.h\n@@ -0,0 +1,8 @@\n+#ifndef REPACK_H\n+#define REPACK_H\n+\n+#include \"git-compat-util.h\"\n+\n+const char *lock_repo_for_pack_manipulation(int force, pid_t* ret_pid);\n+\n+#endif /* REPACK_H */\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 6061a04147..52f19c5871 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -196,5 +196,13 @@ test_expect_success 'objects made unreachable by grafts only are kept' '\n \tgit cat-file -t $H1\n \t'\n \n+test_expect_success 'repack respects gc.pid' '\n+\ttest_tick &&\n+\ttest_when_finished \"rm -f .git/gc.pid\" &&\n+\techo -n \"1234 hostname\" >.git/gc.pid &&\n+\ttest_must_fail git repack -a -d 2>err &&\n+\ttest_i18ngrep \"already running on machine .hostname. pid 1234\" err\n+\t'\n+\n test_done\n \n-- \n2.11.GIT\n\n"},{"id":"316803","messageId":"CA+P7+xomqvK=E0A_wPiyufzyF63yRLA=CQS3Sfec_Uub72DKrw@mail.gmail.com","threadId":"45686","inReplyTo":"20170413202712.22192-1-dturner@twosigma.com","subject":"Re: [PATCH] repack: respect gc.pid lock","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-04-14T00:33:55Z","receivedAt":"2017-04-14T00:34:22Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Apr 13, 2017 at 1:27 PM, David Turner <dturner@twosigma.com> wrote:\n> Git gc locks the repository (using a gc.pid file) so that other gcs\n> don't run concurrently. Make git repack respect this lock.\n>\n> Now repack, by default, will refuse to run at the same time as a gc.\n> This fixes a concurrency issue: a repack which deleted packs would\n> make a concurrent gc sad when its packs were deleted out from under\n> it.  The gc would fail with: \"fatal: ./objects/pack/pack-$sha.pack\n> cannot be accessed\".  Then it would die, probably leaving a large temp\n> pack hanging around.\n>\n> Git repack learns --no-lock, so that when run under git gc, it doesn't\n> attempt to manage the lock itself.\n>\n> Martin Fick suggested just moving the lock into git repack, but this\n> would leave parts of git gc (e.g. git prune) protected by only local\n> locks.  I worried that a prune (part of git gc) concurrent with a\n> repack could confuse the repack, so I decided to go with this\n> solution.\n>\n\nThe last paragraph could be reworded to be a bit less personal and\nmore as a direct statement of why moving the lock entirely to repack\nis a bad idea.\n\n> Signed-off-by: David Turner <dturner@twosigma.com>\n> ---\n>  Documentation/git-repack.txt |  5 +++\n>  Makefile                     |  1 +\n>  builtin/gc.c                 | 72 ++----------------------------------\n>  builtin/repack.c             | 13 +++++++\n>  repack.c                     | 88 ++++++++++++++++++++++++++++++++++++++++++++\n>  repack.h                     |  8 ++++\n>  t/t7700-repack.sh            |  8 ++++\n>  7 files changed, 127 insertions(+), 68 deletions(-)\n>  create mode 100644 repack.c\n>  create mode 100644 repack.h\n>\n> diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\n> index 26afe6ed54..b347ff5c62 100644\n> --- a/Documentation/git-repack.txt\n> +++ b/Documentation/git-repack.txt\n> @@ -143,6 +143,11 @@ other objects in that pack they already have locally.\n>         being removed. In addition, any unreachable loose objects will\n>         be packed (and their loose counterparts removed).\n>\n> +--no-lock::\n> +       Do not lock the repository, and do not respect any existing lock.\n> +       Mostly useful for running repack within git gc.  Do not use this\n> +       unless you know what you are doing.\n> +\n\nI would have phrased this more like:\n\n  Used internally by git gc to call git repack while already holding\nthe lock. Do not use unless you know what you're doing.\n"},{"id":"316844","messageId":"20170414193341.itr3ybiiu2brt63b@sigill.intra.peff.net","threadId":"45686","inReplyTo":"20170413202712.22192-1-dturner@twosigma.com","subject":"Re: [PATCH] repack: respect gc.pid lock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-14T19:33:41Z","receivedAt":"2017-04-14T19:33:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 13, 2017 at 04:27:12PM -0400, David Turner wrote:\n\n> Git gc locks the repository (using a gc.pid file) so that other gcs\n> don't run concurrently. Make git repack respect this lock.\n> \n> Now repack, by default, will refuse to run at the same time as a gc.\n> This fixes a concurrency issue: a repack which deleted packs would\n> make a concurrent gc sad when its packs were deleted out from under\n> it.  The gc would fail with: \"fatal: ./objects/pack/pack-$sha.pack\n> cannot be accessed\".  Then it would die, probably leaving a large temp\n> pack hanging around.\n> \n> Git repack learns --no-lock, so that when run under git gc, it doesn't\n> attempt to manage the lock itself.\n\nThis also means that two repack invocations cannot run simultaneously,\nbecause they want to take the same lock.  But depending on the options,\nthe two don't necessarily conflict. For example, two simultaneous\nincremental \"git repack -d\" invocations should be able to complete.\n\nDo we know where the error message is coming from? I couldn't find the\nerror message you've given above; grepping for \"cannot be accessed\"\nshows only error messages that would have \"packfile\" after the \"fatal:\".\nIs it a copy-paste error?\n\nIf that's the case, then it's the one in use_pack(). Do we know what\nprogram/operation is causing the error? Having a simultaneous gc delete\na packfile is _supposed_ to work, through a combination of:\n\n  1. Most sha1-access operations can re-scan the pack directory if they\n     find the packfile went away.\n\n  2. The pack-objects run by a simultaneous repack is somewhat special\n     in that once it finds and commits to a copy of an object in a pack,\n     we need to use exactly that pack, because we record its offset,\n     delta representation, etc. Usually this works because we open and\n     mmap the packfile before making that commitment, and open packfiles\n     are only closed if you run out of file descriptors (which should\n     only happen when you have a huge number of packs).\n\nSo I'm worried that this repack lock is going to regress some other\ncases that run fine together. But I'm also worried that it's a band-aid\nover a more subtle problem. If pack-objects is not able to run alongside\na gc, then you're also going to have problems serving fetches, and\nobviously you wouldn't want to take a lock there.\n\n-Peff\n"},{"id":"317049","messageId":"c6dd37238f154ccea56dda9b43f3277a@exmbdft7.ad.twosigma.com","threadId":"45686","inReplyTo":"20170414193341.itr3ybiiu2brt63b@sigill.intra.peff.net","subject":"RE: [PATCH] repack: respect gc.pid lock","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2017-04-17T23:29:18Z","receivedAt":"2017-04-17T23:29:28Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"\n> -----Original Message-----\n> From: Jeff King [mailto:peff@peff.net]\n> Sent: Friday, April 14, 2017 3:34 PM\n> To: David Turner <David.Turner@twosigma.com>\n> Cc: git@vger.kernel.org; christian.couder@gmail.com; mfick@codeaurora.org;\n> jacob.keller@gmail.com\n> Subject: Re: [PATCH] repack: respect gc.pid lock\n> \n> On Thu, Apr 13, 2017 at 04:27:12PM -0400, David Turner wrote:\n> \n> > Git gc locks the repository (using a gc.pid file) so that other gcs\n> > don't run concurrently. Make git repack respect this lock.\n> >\n> > Now repack, by default, will refuse to run at the same time as a gc.\n> > This fixes a concurrency issue: a repack which deleted packs would\n> > make a concurrent gc sad when its packs were deleted out from under\n> > it.  The gc would fail with: \"fatal: ./objects/pack/pack-$sha.pack\n> > cannot be accessed\".  Then it would die, probably leaving a large temp\n> > pack hanging around.\n> >\n> > Git repack learns --no-lock, so that when run under git gc, it doesn't\n> > attempt to manage the lock itself.\n> \n> This also means that two repack invocations cannot run simultaneously, because\n> they want to take the same lock.  But depending on the options, the two don't\n> necessarily conflict. For example, two simultaneous incremental \"git repack -d\"\n> invocations should be able to complete.\n> \n> Do we know where the error message is coming from? I couldn't find the error\n> message you've given above; grepping for \"cannot be accessed\"\n> shows only error messages that would have \"packfile\" after the \"fatal:\".\n> Is it a copy-paste error?\n\nYes, it is.  Sorry.\n\nWe saw this failure in the logs multiple  times (with three different\nshas, while a gc was running):\nApril 12, 2017 06:45 -> ERROR -> 'git -c repack.writeBitmaps=true repack -A -d --pack-kept-objects' in [repo] failed:\nfatal: packfile ./objects/pack/pack-[sha].pack cannot be accessed\nPossibly some other repack was also running at the time as well.\n\nMy colleague also saw it while manually doing gc (again while \nrepacks were likely to be running):\n$ git gc --aggressive\nCounting objects: 13800073, done.\nDelta compression using up to 8 threads.\nCompressing objects:  99% (11465846/11465971)   \nCompressing objects: 100% (11465971/11465971), done.\nfatal: packfile [repo]/objects/pack/pack-[sha].pack cannot be accessed\n\n(yes, I know that --aggressive is usually not desirable)\n\n> If that's the case, then it's the one in use_pack(). Do we know what\n> program/operation is causing the error? Having a simultaneous gc delete a\n> packfile is _supposed_ to work, through a combination of:\n> \n>   1. Most sha1-access operations can re-scan the pack directory if they\n>      find the packfile went away.\n> \n>   2. The pack-objects run by a simultaneous repack is somewhat special\n>      in that once it finds and commits to a copy of an object in a pack,\n>      we need to use exactly that pack, because we record its offset,\n>      delta representation, etc. Usually this works because we open and\n>      mmap the packfile before making that commitment, and open packfiles\n>      are only closed if you run out of file descriptors (which should\n>      only happen when you have a huge number of packs).\n\nWe have a reasonable rlimit (64k soft limit), so that failure mode is pretty \nunlikely.  I  think we should have had 20 or so packs -- not tens of thousands.\n\n> So I'm worried that this repack lock is going to regress some other cases that run\n> fine together. But I'm also worried that it's a band-aid over a more subtle\n> problem. If pack-objects is not able to run alongside a gc, then you're also going\n> to have problems serving fetches, and obviously you wouldn't want to take a\n> lock there.\n\nI see your point.  I don't know if it's pack-objects that's seeing this, although maybe \nthat's the only reasonable codepath.\n\nI did some tracing through the code, and couldn't figure out how to trigger that \nerror message.  It appears in two places in the code, but only when the pack is not \ninitialized.  But the packs always seem to be set up by that point in my test runs.  \nIt's worth noting that I'm not testing on the gitlab server; I'm testing on my laptop with\na completely different repo.  But I've tried various ways to repro this -- or even to \nget to a point where those errors would have been thrown given a missing pack -- \nand I have not been able to.\n\nDo you have any idea why this would be happening other than the rlimit thing?\n\n"},{"id":"317069","messageId":"20170418034157.oi6hkg5obnca5zsa@sigill.intra.peff.net","threadId":"45686","inReplyTo":"c6dd37238f154ccea56dda9b43f3277a@exmbdft7.ad.twosigma.com","subject":"Re: [PATCH] repack: respect gc.pid lock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-18T03:41:57Z","receivedAt":"2017-04-18T03:42:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 17, 2017 at 11:29:18PM +0000, David Turner wrote:\n\n> We saw this failure in the logs multiple  times (with three different\n> shas, while a gc was running):\n> April 12, 2017 06:45 -> ERROR -> 'git -c repack.writeBitmaps=true repack -A -d --pack-kept-objects' in [repo] failed:\n> fatal: packfile ./objects/pack/pack-[sha].pack cannot be accessed\n> Possibly some other repack was also running at the time as well.\n> \n> My colleague also saw it while manually doing gc (again while \n> repacks were likely to be running):\n\nThis is sort of a side question, but...why are you running other repacks\nalongside git-gc? It seems like you ought to be doing one or the other.\n\nI don't begrudge anybody with a complicated setup running their own set\nof gc commands, but I'd think you would want to do locking there, and\ndisable auto-gc entirely. Otherwise you're going to get different\nresults depending on who gc'd last.\n\n> $ git gc --aggressive\n> Counting objects: 13800073, done.\n> Delta compression using up to 8 threads.\n> Compressing objects:  99% (11465846/11465971)   \n> Compressing objects: 100% (11465971/11465971), done.\n> fatal: packfile [repo]/objects/pack/pack-[sha].pack cannot be accessed\n\nOK, so this presumably happened during the writing phase. Which seems\nlike the \"a pack was closed, and we couldn't re-open it\" problem we've\nseen before.\n\n> We have a reasonable rlimit (64k soft limit), so that failure mode is pretty \n> unlikely.  I  think we should have had 20 or so packs -- not tens of thousands.\n> [...]\n> Do you have any idea why this would be happening other than the rlimit thing?\n\nYeah, that should be enough (you could double check the return of\nget_max_fd_limit() on your system if you wanted to be paranoid).\n\nWe also keep only a limited number of bytes mmap'd at one time. Normally\nwe don't actually close packfiles when we release their mmap windows.\nBut I think there is one path that might. When use_pack() maps a pack,\nif the entire pack fits in a single window, then we close it; this is\ndue to d131b7afe (sha1_file.c: Don't retain open fds on small packs,\n2011-03-02).\n\nBut if we ever unmap that window, now we have no handle to the pack.\nNormally on a 64-bit system this wouldn't happen at all, since the\ndefault core.packedGitLimit is 8GB there.\n\nSo if you have a few small packs and one very large pack (over 8GB), I\nthink this could trigger. We may do the small-pack thing for some of\nthem, and then the large pack forces us to drop the mmaps for some of\nthe others. When we go back to access the small pack, we find it's gone.\n\nOne solution would be to bump core.packedGitLimit to something much\nhigher (it's an mmap, so we're really just chewing up address space;\nit's up to the OS to decide when to load pages from disk and when to\ndrop them).\n\nThe other alternative is to disable the small-pack closing from\nd131b7afe. It might need to be configurable, or perhaps auto-tuned based\non the fd limit. Linux systems tend to have generous descriptor limits,\nbut I'm not sure we can rely on that. OTOH, it seems like the code to\nclose descriptors when needed would take care of things. So maybe we\nshould just revert d131b7afe entirely.\n\nThe final thing I'd ask is whether you might be on a networked\nfilesystem that would foil our usual \"open descriptors mean packs don't\ngo away\" logic. But after having dug into the details above, I have a\nfeeling the answer is simply that you have repositories >8GB.\n\nAnd if that is the case, then yeah, your locking patch is definitely a\nband-aid. If you fetch and repack at the same time, you'll eventually\nsee a racy failed fetch.\n\n-Peff\n"},{"id":"317118","messageId":"d5c43adf0b074c6ebe43439bc3fc7539@exmbdft7.ad.twosigma.com","threadId":"45686","inReplyTo":"20170418034157.oi6hkg5obnca5zsa@sigill.intra.peff.net","subject":"RE: [PATCH] repack: respect gc.pid lock","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2017-04-18T17:08:14Z","receivedAt":"2017-04-18T17:08:20Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"> -----Original Message-----\n> From: Jeff King [mailto:peff@peff.net]\n> Sent: Monday, April 17, 2017 11:42 PM\n> To: David Turner <David.Turner@twosigma.com>\n> Cc: git@vger.kernel.org; christian.couder@gmail.com; mfick@codeaurora.org;\n> jacob.keller@gmail.com\n> Subject: Re: [PATCH] repack: respect gc.pid lock\n> \n> On Mon, Apr 17, 2017 at 11:29:18PM +0000, David Turner wrote:\n> \n> > We saw this failure in the logs multiple  times (with three different\n> > shas, while a gc was running):\n> > April 12, 2017 06:45 -> ERROR -> 'git -c repack.writeBitmaps=true repack -A -d\n> --pack-kept-objects' in [repo] failed:\n> > fatal: packfile ./objects/pack/pack-[sha].pack cannot be accessed\n> > Possibly some other repack was also running at the time as well.\n> >\n> > My colleague also saw it while manually doing gc (again while repacks\n> > were likely to be running):\n> \n> This is sort of a side question, but...why are you running other repacks alongside\n> git-gc? It seems like you ought to be doing one or the other.\n>\n> I don't begrudge anybody with a complicated setup running their own set of gc\n> commands, but I'd think you would want to do locking there, and disable auto-\n> gc entirely. Otherwise you're going to get different results depending on who\n> gc'd last.\n\nThat's what gitlab does, so you'll have to ask them why they do it that way.  \nFrom https://gitlab.com/gitlab-org/gitlab-ce/issues/30939#note_27487981\n it looks like they may have intended to have a lock but not quite succeeded.\n \n> > $ git gc --aggressive\n> > Counting objects: 13800073, done.\n> > Delta compression using up to 8 threads.\n> > Compressing objects:  99% (11465846/11465971)\n> > Compressing objects: 100% (11465971/11465971), done.\n> > fatal: packfile [repo]/objects/pack/pack-[sha].pack cannot be accessed\n> \n> OK, so this presumably happened during the writing phase. Which seems like the\n> \"a pack was closed, and we couldn't re-open it\" problem we've seen before.\n> \n> > We have a reasonable rlimit (64k soft limit), so that failure mode is\n> > pretty unlikely.  I  think we should have had 20 or so packs -- not tens of\n> thousands.\n> > [...]\n> > Do you have any idea why this would be happening other than the rlimit thing?\n> \n> Yeah, that should be enough (you could double check the return of\n> get_max_fd_limit() on your system if you wanted to be paranoid).\n> \n> We also keep only a limited number of bytes mmap'd at one time. Normally we\n> don't actually close packfiles when we release their mmap windows.\n> But I think there is one path that might. When use_pack() maps a pack, if the\n> entire pack fits in a single window, then we close it; this is due to d131b7afe\n> (sha1_file.c: Don't retain open fds on small packs, 2011-03-02).\n> \n> But if we ever unmap that window, now we have no handle to the pack.\n> Normally on a 64-bit system this wouldn't happen at all, since the default\n> core.packedGitLimit is 8GB there.\n\nAha, I missed that limit while messing around with the code.  That must be it.\n\n> So if you have a few small packs and one very large pack (over 8GB), I think this\n> could trigger. We may do the small-pack thing for some of them, and then the\n> large pack forces us to drop the mmaps for some of the others. When we go\n> back to access the small pack, we find it's gone.\n> \n> One solution would be to bump core.packedGitLimit to something much higher\n> (it's an mmap, so we're really just chewing up address space; it's up to the OS to\n> decide when to load pages from disk and when to drop them).\n>\n> The other alternative is to disable the small-pack closing from d131b7afe. It\n> might need to be configurable, or perhaps auto-tuned based on the fd limit.\n> Linux systems tend to have generous descriptor limits, but I'm not sure we can\n> rely on that. OTOH, it seems like the code to close descriptors when needed\n> would take care of things. So maybe we should just revert d131b7afe entirely.\n\nI definitely remember running into fd limits when processing very large numbers \nof packs at Twitter, but I don't recall the exact details.  Presumably, d131b7afe\nwas supposed to help with this, but in fact, it did not totally solve it. Perhaps \nwe were doing something funny.  Adjusting the fd limits was the easy fix.\n\nOn 64-bit systems, I think core.packedGitLimit doesn't make a \nlot of sense. There is plenty of address space.  Why not use it?\n\nFor 32-bit systems, of course, address space is more precious.\n\nI'll ask our git server administrator to adjust core.packedGitLimit\nand turn repacks back on to see if that fixes the issue.\n\n> The final thing I'd ask is whether you might be on a networked filesystem that\n> would foil our usual \"open descriptors mean packs don't go away\" logic. But\n> after having dug into the details above, I have a feeling the answer is simply that\n> you have repositories >8GB.\n\nYes, our repo is >8GB, and no, it's not on a networked filesystem.\n\n> And if that is the case, then yeah, your locking patch is definitely a band-aid. If\n> you fetch and repack at the same time, you'll eventually see a racy failed fetch.\n\nFair enough.\n"},{"id":"317120","messageId":"20170418171646.5a5mjhd4qjr6ot7d@sigill.intra.peff.net","threadId":"45686","inReplyTo":"d5c43adf0b074c6ebe43439bc3fc7539@exmbdft7.ad.twosigma.com","subject":"Re: [PATCH] repack: respect gc.pid lock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-18T17:16:46Z","receivedAt":"2017-04-18T17:16:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 18, 2017 at 05:08:14PM +0000, David Turner wrote:\n\n> On 64-bit systems, I think core.packedGitLimit doesn't make a \n> lot of sense. There is plenty of address space.  Why not use it?\n\nThat's my gut feeling, too. I'd have a slight worry that the OS's paging\nbehavior may respond differently if we have more memory mapped. But\nthat's not based on numbers, just a fear of the unknown. :)\n\nIf we have infinite windows anyway, I suspect we could just mmap entire\npackfiles and forget about all the window complexity in the first place.\nAlthough IIRC some operating systems take a long time to set up large\nmmaps, and we may only need a small part of a large pack.\n\n> I'll ask our git server administrator to adjust core.packedGitLimit\n> and turn repacks back on to see if that fixes the issue.\n\nThanks. Let us know if you get any results, either way.\n\n-Peff\n"},{"id":"317121","messageId":"2400e9cbfaff4838a8f3b23c4c2c5a22@exmbdft7.ad.twosigma.com","threadId":"45686","inReplyTo":"20170418034157.oi6hkg5obnca5zsa@sigill.intra.peff.net","subject":"RE: [PATCH] repack: respect gc.pid lock","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2017-04-18T17:16:52Z","receivedAt":"2017-04-18T17:16:58Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"> -----Original Message-----\n> From: Jeff King [mailto:peff@peff.net]\n> Sent: Monday, April 17, 2017 11:42 PM\n> To: David Turner <David.Turner@twosigma.com>\n> Cc: git@vger.kernel.org; christian.couder@gmail.com; mfick@codeaurora.org;\n> jacob.keller@gmail.com\n> Subject: Re: [PATCH] repack: respect gc.pid lock\n> \n> On Mon, Apr 17, 2017 at 11:29:18PM +0000, David Turner wrote:\n> \n> > We saw this failure in the logs multiple  times (with three different\n> > shas, while a gc was running):\n> > April 12, 2017 06:45 -> ERROR -> 'git -c repack.writeBitmaps=true repack -A -d\n> --pack-kept-objects' in [repo] failed:\n> > fatal: packfile ./objects/pack/pack-[sha].pack cannot be accessed\n> > Possibly some other repack was also running at the time as well.\n> >\n> > My colleague also saw it while manually doing gc (again while repacks\n> > were likely to be running):\n> \n> This is sort of a side question, but...why are you running other repacks alongside\n> git-gc? It seems like you ought to be doing one or the other.\n\nBut actually, it would be kind of nice if git would help protect us from doing this?\n"},{"id":"317122","messageId":"20170418171930.zad5wrbu5rvdsmg5@sigill.intra.peff.net","threadId":"45686","inReplyTo":"2400e9cbfaff4838a8f3b23c4c2c5a22@exmbdft7.ad.twosigma.com","subject":"Re: [PATCH] repack: respect gc.pid lock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-18T17:19:31Z","receivedAt":"2017-04-18T17:19:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 18, 2017 at 05:16:52PM +0000, David Turner wrote:\n\n> > -----Original Message-----\n> > From: Jeff King [mailto:peff@peff.net]\n> > Sent: Monday, April 17, 2017 11:42 PM\n> > To: David Turner <David.Turner@twosigma.com>\n> > Cc: git@vger.kernel.org; christian.couder@gmail.com; mfick@codeaurora.org;\n> > jacob.keller@gmail.com\n> > Subject: Re: [PATCH] repack: respect gc.pid lock\n> > \n> > On Mon, Apr 17, 2017 at 11:29:18PM +0000, David Turner wrote:\n> > \n> > > We saw this failure in the logs multiple  times (with three different\n> > > shas, while a gc was running):\n> > > April 12, 2017 06:45 -> ERROR -> 'git -c repack.writeBitmaps=true repack -A -d\n> > --pack-kept-objects' in [repo] failed:\n> > > fatal: packfile ./objects/pack/pack-[sha].pack cannot be accessed\n> > > Possibly some other repack was also running at the time as well.\n> > >\n> > > My colleague also saw it while manually doing gc (again while repacks\n> > > were likely to be running):\n> > \n> > This is sort of a side question, but...why are you running other repacks alongside\n> > git-gc? It seems like you ought to be doing one or the other.\n> \n> But actually, it would be kind of nice if git would help protect us from doing this?\n\nA lock can catch the racy cases where both run at the same time. But I\nthink that even:\n\n  git -c repack.writeBitmaps=true repack -Ad\n  [...wait...]\n  git gc\n\nis questionable, because that gc will erase your bitmaps. How does\ngit-gc know that it's doing a bad thing by repacking without bitmaps,\nand that you didn't simply change your configuration or want to get rid\nof them?\n\n-Peff\n"},{"id":"317124","messageId":"710ded65bb8843ab838d9c52cd796317@exmbdft7.ad.twosigma.com","threadId":"45686","inReplyTo":"20170418171930.zad5wrbu5rvdsmg5@sigill.intra.peff.net","subject":"RE: [PATCH] repack: respect gc.pid lock","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2017-04-18T17:43:29Z","receivedAt":"2017-04-18T17:43:36Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"\n\n> -----Original Message-----\n> From: Jeff King [mailto:peff@peff.net]\n> Sent: Tuesday, April 18, 2017 1:20 PM\n> To: David Turner <David.Turner@twosigma.com>\n> Cc: git@vger.kernel.org; christian.couder@gmail.com; mfick@codeaurora.org;\n> jacob.keller@gmail.com\n> Subject: Re: [PATCH] repack: respect gc.pid lock\n> \n> On Tue, Apr 18, 2017 at 05:16:52PM +0000, David Turner wrote:\n> \n> > > -----Original Message-----\n> > > From: Jeff King [mailto:peff@peff.net]\n> > > Sent: Monday, April 17, 2017 11:42 PM\n> > > To: David Turner <David.Turner@twosigma.com>\n> > > Cc: git@vger.kernel.org; christian.couder@gmail.com;\n> > > mfick@codeaurora.org; jacob.keller@gmail.com\n> > > Subject: Re: [PATCH] repack: respect gc.pid lock\n> > >\n> > > On Mon, Apr 17, 2017 at 11:29:18PM +0000, David Turner wrote:\n> > >\n> > > > We saw this failure in the logs multiple  times (with three\n> > > > different shas, while a gc was running):\n> > > > April 12, 2017 06:45 -> ERROR -> 'git -c repack.writeBitmaps=true\n> > > > repack -A -d\n> > > --pack-kept-objects' in [repo] failed:\n> > > > fatal: packfile ./objects/pack/pack-[sha].pack cannot be accessed\n> > > > Possibly some other repack was also running at the time as well.\n> > > >\n> > > > My colleague also saw it while manually doing gc (again while\n> > > > repacks were likely to be running):\n> > >\n> > > This is sort of a side question, but...why are you running other\n> > > repacks alongside git-gc? It seems like you ought to be doing one or the\n> other.\n> >\n> > But actually, it would be kind of nice if git would help protect us from doing\n> this?\n> \n> A lock can catch the racy cases where both run at the same time. But I think that\n> even:\n> \n>   git -c repack.writeBitmaps=true repack -Ad\n>   [...wait...]\n>   git gc\n> \n> is questionable, because that gc will erase your bitmaps. How does git-gc know\n> that it's doing a bad thing by repacking without bitmaps, and that you didn't\n> simply change your configuration or want to get rid of them?\n\nSorry, the gc in Gitlab does keep bitmaps.  The one I quoted in a previous \nmessage  doesn't, because the person typing the command was just doing some \nmanual  testing and I guess didn't realize that bitmaps were important.  Or \nperhaps he knew that repack.writeBitmaps was already set in the config.\n\nSo given that the lock will catch the races, might it be a good idea (if \nImplemented to avoid locking on repack -d)?\n"},{"id":"317125","messageId":"20170418175011.qx64luolrvqwwtpa@sigill.intra.peff.net","threadId":"45686","inReplyTo":"710ded65bb8843ab838d9c52cd796317@exmbdft7.ad.twosigma.com","subject":"Re: [PATCH] repack: respect gc.pid lock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-18T17:50:11Z","receivedAt":"2017-04-18T17:50:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 18, 2017 at 05:43:29PM +0000, David Turner wrote:\n\n> > A lock can catch the racy cases where both run at the same time. But I think that\n> > even:\n> > \n> >   git -c repack.writeBitmaps=true repack -Ad\n> >   [...wait...]\n> >   git gc\n> > \n> > is questionable, because that gc will erase your bitmaps. How does git-gc know\n> > that it's doing a bad thing by repacking without bitmaps, and that you didn't\n> > simply change your configuration or want to get rid of them?\n> \n> Sorry, the gc in Gitlab does keep bitmaps.  The one I quoted in a previous \n> message  doesn't, because the person typing the command was just doing some \n> manual  testing and I guess didn't realize that bitmaps were important.  Or \n> perhaps he knew that repack.writeBitmaps was already set in the config.\n\nSure, but I guess I'd just wonder what _else_ is different between the\ncommands (and if nothing, why are both running).\n\n> So given that the lock will catch the races, might it be a good idea (if \n> Implemented to avoid locking on repack -d)?\n\nI'm mildly negative just because it increases complexity, and I don't\nthink it's actually buying very much. It's not clear to me which\ninvocations of repack would want to lock and which ones wouldn't.\n\nIs \"-a\" or \"-A\" the key factor? Are there current callers who prefer the\ncurrent behavior of \"possibly duplicate some work, but never report\nfailure\" versus \"do not duplicate work, but sometimes fail due to lock\ncontention\"?\n\n-Peff\n"},{"id":"317369","messageId":"7e31f4ed5c0f4c31b2870fb58cf7110e@exmbdft7.ad.twosigma.com","threadId":"45686","inReplyTo":"20170418175011.qx64luolrvqwwtpa@sigill.intra.peff.net","subject":"RE: [PATCH] repack: respect gc.pid lock","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2017-04-20T20:10:24Z","receivedAt":"2017-04-20T20:11:40Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"> -----Original Message-----\n> From: Jeff King [mailto:peff@peff.net]\n> Sent: Tuesday, April 18, 2017 1:50 PM\n> To: David Turner <David.Turner@twosigma.com>\n> Cc: git@vger.kernel.org; christian.couder@gmail.com; mfick@codeaurora.org;\n> jacob.keller@gmail.com\n> Subject: Re: [PATCH] repack: respect gc.pid lock\n> \n> On Tue, Apr 18, 2017 at 05:43:29PM +0000, David Turner wrote:\n> \n> > > A lock can catch the racy cases where both run at the same time. But\n> > > I think that\n> > > even:\n> > >\n> > >   git -c repack.writeBitmaps=true repack -Ad\n> > >   [...wait...]\n> > >   git gc\n> > >\n> > > is questionable, because that gc will erase your bitmaps. How does\n> > > git-gc know that it's doing a bad thing by repacking without\n> > > bitmaps, and that you didn't simply change your configuration or want to get\n> rid of them?\n> >\n> > Sorry, the gc in Gitlab does keep bitmaps.  The one I quoted in a\n> > previous message  doesn't, because the person typing the command was\n> > just doing some manual  testing and I guess didn't realize that\n> > bitmaps were important.  Or perhaps he knew that repack.writeBitmaps was\n> already set in the config.\n> \n> Sure, but I guess I'd just wonder what _else_ is different between the commands\n> (and if nothing, why are both running).\n\nPresumably, repack is faster, and they're not intended to run concurrently (but \nthere's a Gitlab bug causing them to do so).  But you'll have to ask the Gitlab \nfolks for more details.\n\n> > So given that the lock will catch the races, might it be a good idea\n> > (if Implemented to avoid locking on repack -d)?\n> \n> I'm mildly negative just because it increases complexity, and I don't think it's\n> actually buying very much. It's not clear to me which invocations of repack\n> would want to lock and which ones wouldn't.\n> \n> Is \"-a\" or \"-A\" the key factor? Are there current callers who prefer the current\n> behavior of \"possibly duplicate some work, but never report failure\" versus \"do\n> not duplicate work, but sometimes fail due to lock contention\"?\n\nOne problem with failing is that it can leave a temp pack behind.\n\nI think the correct fix is to change the default code.packedGitLimit on 64-bit \nmachines to 32 terabytes (2**45 bytes).  That's because on modern Intel \nprocessors, there are 48 bits of address space actually available, but the kernel \nis going to probably reserve a few bits.  My machine claims to have 2**46 bytes \nof virtual address space available.  It's also several times bigger than any \nrepo that I know of or can easily imagine.\n\nDoes that seem reasonable to you?\n"},{"id":"317371","messageId":"20170420201443.ee4tgoymzpfvl4jq@sigill.intra.peff.net","threadId":"45686","inReplyTo":"7e31f4ed5c0f4c31b2870fb58cf7110e@exmbdft7.ad.twosigma.com","subject":"Re: [PATCH] repack: respect gc.pid lock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-20T20:14:43Z","receivedAt":"2017-04-20T20:15:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 20, 2017 at 08:10:24PM +0000, David Turner wrote:\n\n> > Is \"-a\" or \"-A\" the key factor? Are there current callers who prefer the current\n> > behavior of \"possibly duplicate some work, but never report failure\" versus \"do\n> > not duplicate work, but sometimes fail due to lock contention\"?\n> \n> One problem with failing is that it can leave a temp pack behind.\n\nYeah. IMHO we should probably treat failed object and pack writes as\nnormal tempfiles and remove them (but possibly respect a \"debug mode\"\nthat leaves them around). But that's another patch entirely.\n\n> I think the correct fix is to change the default code.packedGitLimit on 64-bit \n> machines to 32 terabytes (2**45 bytes).  That's because on modern Intel \n> processors, there are 48 bits of address space actually available, but the kernel \n> is going to probably reserve a few bits.  My machine claims to have 2**46 bytes \n> of virtual address space available.  It's also several times bigger than any \n> repo that I know of or can easily imagine.\n> \n> Does that seem reasonable to you?\n\nYes, it does.\n\n-Peff\n"}]}