From c45af94dbc960cbb46ac482ec9cb7f73f55ea270 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 11 Jul 2017 05:06:35 -0400 Subject: [PATCH] gc: run pre-detach operations under lock We normally try to avoid having two auto-gc operations run at the same time, because it wastes resources. This was done long ago in 64a99eb47 (gc: reject if another gc is running, unless --force is given, 2013-08-08). When we do a detached auto-gc, we run the ref-related commands _before_ detaching, to avoid confusing lock contention. This was done by 62aad1849 (gc --auto: do not lock refs in the background, 2014-05-25). These two features do not interact well. The pre-detach operations are run before we check the gc.pid lock, meaning that on a busy repository we may run many of them concurrently. Ideally we'd take the lock before spawning any operations, and hold it for the duration of the program. This is tricky, though, with the way the pid-file interacts with the daemonize() process. Other processes will check that the pid recorded in the pid-file still exists. But detaching causes us to fork and continue running under a new pid. So if we take the lock before detaching, the pid-file will have a bogus pid in it. We'd have to go back and update it with the new pid after detaching. We'd also have to play some tricks with the tempfile subsystem to tweak the "owner" field, so that the parent process does not clean it up on exit, but the child process does. Instead, we can do something a bit simpler: take the lock only for the duration of the pre-detach work, then detach, then take it again for the post-detach work. Technically, this means that the post-detach lock could lose to another process doing pre-detach work. But in the long run this works out. That second process would then follow-up by doing post-detach work. Unless it was in turn blocked by a third process doing pre-detach work, and so on. This could in theory go on indefinitely, as the pre-detach work does not repack, and so need_to_gc() will continue to trigger. But in each round we are racing between the pre- and post-detach locks. Eventually, one of the post-detach locks will win the race and complete the full gc. So in the worst case, we may racily repeat the pre-detach work, but we would never do so simultaneously (it would happen via a sequence of serialized race-wins). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/gc.c | 4 ++++ t/t6500-gc.sh | 21 +++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/builtin/gc.c b/builtin/gc.c index 91f7696a85..2d2027d8b0 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -413,8 +413,12 @@ int cmd_gc(int argc, const char **argv, const char *prefix) if (report_last_gc_error()) return -1; + if (lock_repo_for_gc(force, &pid)) + return 0; if (gc_before_repack()) return -1; + delete_tempfile(&pidfile); + /* * failure to daemonize is ok, we'll continue * in foreground diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh index cc7acd101d..41b0be575d 100755 --- a/t/t6500-gc.sh +++ b/t/t6500-gc.sh @@ -95,6 +95,27 @@ test_expect_success 'background auto gc does not run if gc.log is present and re test_line_count = 1 packs ' +test_expect_success 'background auto gc respects lock for all operations' ' + # make sure we run a background auto-gc + test_commit make-pack && + git repack && + test_config gc.autopacklimit 1 && + test_config gc.autodetach true && + + # create a ref whose loose presence we can use to detect a pack-refs run + git update-ref refs/heads/should-be-loose HEAD && + test_path_is_file .git/refs/heads/should-be-loose && + + # now fake a concurrent gc that holds the lock; we can use our + # shell pid so that it looks valid. + hostname=$(hostname || echo unknown) && + printf "$$ %s" "$hostname" >.git/gc.pid && + + # our gc should exit zero without doing anything + run_and_wait_for_auto_gc && + test_path_is_file .git/refs/heads/should-be-loose +' + # DO NOT leave a detached auto gc process running near the end of the # test script: it can run long enough in the background to racily # interfere with the cleanup in 'test_done'. -- 2.11.4.GIT