[BUG] auto-repack exits prematurely, locking other processing out

[BUG] auto-repack exits prematurely, locking other processing out

[Adam Borowski]

Hi guys!

It looks like the periodic auto-repack backgrounds itself when it shouldn't
do so.  This causes the command it has triggered as a part of to fail:

==========================================================================
[~/linux](master)$ git pull --rebase
remote: Counting objects: 455, done.
remote: Compressing objects: 100% (64/64), done.
remote: Total 267 (delta 208), reused 262 (delta 203)
Receiving objects: 100% (267/267), 44.43 KiB | 0 bytes/s, done.
Resolving deltas: 100% (208/208), completed with 80 local objects.
From git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux
   4b660a7..f02f79d  master     -> linus/master
Auto packing the repository in background for optimum performance.
See "git help gc" for manual housekeeping.
First, rewinding head to replay your work on top of it...
Applying: perf: tools: fix missing casts for printf arguments.
Applying: vt: emulate 8- and 24-bit colour codes.
fatal: Unable to create '/home/kilobyte/linux/.git/refs/heads/master.lock': File exists.

If no other git process is currently running, this probably means a
git process crashed in this repository earlier. Make sure no other git
process is running and remove the file manually to continue.
Could not move back to refs/heads/master
[~/linux]((no branch, rebasing (null)))$
==========================================================================

-- 
Gnome 3, Windows 8, Slashdot Beta, now Firefox Ribbon^WAustralis.  WTF is going
on with replacing usable interfaces with tabletized ones?

Re: [BUG] auto-repack exits prematurely, locking other processing out

[Junio C Hamano]

Adam Borowski <kilobyte@angband.pl> writes:

> Hi guys!
>
> It looks like the periodic auto-repack backgrounds itself when it shouldn't
> do so.  This causes the command it has triggered as a part of to fail:

Yikes.  In the meantime, I think you can turn gc.autodetach off as a
workaround, e.g.

    $ git config --global --add gc.autodetach off

Duy, 9f673f94 (gc: config option for running --auto in background,
2014-02-08) turns to be not such a hot idea.  Sure, if we kick it
off background after doing something heavy, immediately before
giving control back to the end-user, and expect that the user will
stay thinking without making new changes (i.e. read-only stuff like
"git show" would be OK), then daemonize might be a great thing, but
we forgot, while doing that commit, that long-running operations
trigger the auto gc in the middle *and* they want it finish before
they continue, as the purpose of gc is to help the performance
during their further operation.



>
> ==========================================================================
> [~/linux](master)$ git pull --rebase
> remote: Counting objects: 455, done.
> remote: Compressing objects: 100% (64/64), done.
> remote: Total 267 (delta 208), reused 262 (delta 203)
> Receiving objects: 100% (267/267), 44.43 KiB | 0 bytes/s, done.
> Resolving deltas: 100% (208/208), completed with 80 local objects.
> From git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux
>    4b660a7..f02f79d  master     -> linus/master
> Auto packing the repository in background for optimum performance.
> See "git help gc" for manual housekeeping.
> First, rewinding head to replay your work on top of it...
> Applying: perf: tools: fix missing casts for printf arguments.
> Applying: vt: emulate 8- and 24-bit colour codes.
> fatal: Unable to create '/home/kilobyte/linux/.git/refs/heads/master.lock': File exists.
>
> If no other git process is currently running, this probably means a
> git process crashed in this repository earlier. Make sure no other git
> process is running and remove the file manually to continue.
> Could not move back to refs/heads/master
> [~/linux]((no branch, rebasing (null)))$
> ==========================================================================

Re: [BUG] auto-repack exits prematurely, locking other processing out

[Adam Borowski]

On Fri, May 23, 2014 at 02:40:41PM -0700, Junio C Hamano wrote:
> Adam Borowski <kilobyte@angband.pl> writes:
> > It looks like the periodic auto-repack backgrounds itself when it shouldn't
> > do so.  This causes the command it has triggered as a part of to fail:
> 
> Duy, 9f673f94 (gc: config option for running --auto in background,
> 2014-02-08) turns to be not such a hot idea.  Sure, if we kick it
> off background after doing something heavy, immediately before
> giving control back to the end-user, and expect that the user will
> stay thinking without making new changes (i.e. read-only stuff like
> "git show" would be OK), then daemonize might be a great thing, but
> we forgot, while doing that commit, that long-running operations
> trigger the auto gc in the middle *and* they want it finish before
> they continue, as the purpose of gc is to help the performance
> during their further operation.

Just add a lock that's triggered by daemonize, and have things block on this
lock.  This handles all cases:
* --auto in the middle of a command: the block will kick in immediately,
  effectively reverting to non-daemonized version
* --auto at the end, the user does nothing: gc will finish its work in the
  background, just as you wanted
* --auto at the end, the user starts a new write two seconds later: gc works
  in the foreground with those 2 seconds headstart

The only loss is the lack of a progress indicator, and even that can be
done.

-- 
Gnome 3, Windows 8, Slashdot Beta, now Firefox Ribbon^WAustralis.  WTF is going
on with replacing usable interfaces with tabletized ones?

Re: [BUG] auto-repack exits prematurely, locking other processing out

[Junio C Hamano]

Adam Borowski <kilobyte@angband.pl> writes:

> On Fri, May 23, 2014 at 02:40:41PM -0700, Junio C Hamano wrote:
>> Adam Borowski <kilobyte@angband.pl> writes:
>> > It looks like the periodic auto-repack backgrounds itself when it shouldn't
>> > do so.  This causes the command it has triggered as a part of to fail:
>> 
>> Duy, 9f673f94 (gc: config option for running --auto in background,
>> 2014-02-08) turns to be not such a hot idea.  Sure, if we kick it
>> off background after doing something heavy, immediately before
>> giving control back to the end-user, and expect that the user will
>> stay thinking without making new changes (i.e. read-only stuff like
>> "git show" would be OK), then daemonize might be a great thing, but
>> we forgot, while doing that commit, that long-running operations
>> trigger the auto gc in the middle *and* they want it finish before
>> they continue, as the purpose of gc is to help the performance
>> during their further operation.
>
> Just add a lock that's triggered by daemonize, and have things block on this
> lock.

Hmph, it defeats the whole point of running it in the background,
doesn't it?  How would "blocking on the lock" be different from
launching "gc --auto" and waiting for it to come back?

And it would also require addition of the big-repository-lock and
code to take the lock sprinkled all over the place.  I am not sure
if we want to go there...

Re: [BUG] auto-repack exits prematurely, locking other processing out

[Duy Nguyen]

On Sat, May 24, 2014 at 4:40 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy, 9f673f94 (gc: config option for running --auto in background,
> 2014-02-08) turns to be not such a hot idea.  Sure, if we kick it
> off background after doing something heavy, immediately before
> giving control back to the end-user, and expect that the user will
> stay thinking without making new changes (i.e. read-only stuff like
> "git show" would be OK), then daemonize might be a great thing, but
> we forgot, while doing that commit, that long-running operations
> trigger the auto gc in the middle *and* they want it finish before
> they continue, as the purpose of gc is to help the performance
> during their further operation.

If by "long-running operations" you mean in a single process, it's my
first thought too but it looks like autogc is always called when the
process is all done and about to exit. The "git pull" case is
different because there's rebase after fetch. I see no easy way to
detect this kind of "middle of operation".

So we have two options: scripts should disable autogc before doing
things, a env variable would be more convenient than temporarily
updating gc.auto. Or we move "pack-refs" and "reflog expire" up,
before turning gc into a background task. Any locking will be
serialized this way. We could even go further to keep all but "repack"
in the background because it's "repack" that takes the longest time
(maybe "prune" coming close to second).
-- 
Duy

[PATCH] gc --auto: do not lock refs in the background

[Nguyễn Thái Ngọc Duy]

9f673f9 (gc: config option for running --auto in background -
2014-02-08) puts "gc --auto" in background to reduce user's wait
time. Part of the garbage collecting is pack-refs and pruning
reflogs. These require locking some refs and may abort other processes
trying to lock the same ref. If gc --auto is fired in the middle of a
script, gc's holding locks in the background could fail the script,
which could never happen before 9f673f9.

Keep running pack-refs and "reflog --prune" in foreground to stop
parallel ref updates. The remaining background operations (repack,
prune and rerere) should impact running git processes.

Reported-by: Adam Borowski <kilobyte@angband.pl>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/gc.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 85f5c2b..8d219d8 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -26,6 +26,7 @@ static const char * const builtin_gc_usage[] = {
 };
 
 static int pack_refs = 1;
+static int prune_reflogs = 1;
 static int aggressive_depth = 250;
 static int aggressive_window = 250;
 static int gc_auto_threshold = 6700;
@@ -258,6 +259,19 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
 	return NULL;
 }
 
+static int gc_before_repack(void)
+{
+	if (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))
+		return error(FAILED_RUN, pack_refs_cmd.argv[0]);
+
+	if (prune_reflogs && run_command_v_opt(reflog.argv, RUN_GIT_CMD))
+		return error(FAILED_RUN, reflog.argv[0]);
+
+	pack_refs = 0;
+	prune_reflogs = 0;
+	return 0;
+}
+
 int cmd_gc(int argc, const char **argv, const char *prefix)
 {
 	int aggressive = 0;
@@ -320,12 +334,15 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 				fprintf(stderr, _("Auto packing the repository for optimum performance.\n"));
 			fprintf(stderr, _("See \"git help gc\" for manual housekeeping.\n"));
 		}
-		if (detach_auto)
+		if (detach_auto) {
+			if (gc_before_repack())
+				return -1;
 			/*
 			 * failure to daemonize is ok, we'll continue
 			 * in foreground
 			 */
 			daemonize();
+		}
 	} else
 		add_repack_all_option();
 
@@ -337,11 +354,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 		    name, (uintmax_t)pid);
 	}
 
-	if (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))
-		return error(FAILED_RUN, pack_refs_cmd.argv[0]);
-
-	if (run_command_v_opt(reflog.argv, RUN_GIT_CMD))
-		return error(FAILED_RUN, reflog.argv[0]);
+	if (gc_before_repack())
+		return -1;
 
 	if (run_command_v_opt(repack.argv, RUN_GIT_CMD))
 		return error(FAILED_RUN, repack.argv[0]);
-- 
1.9.1.346.ga2b5940

Re: [PATCH] gc --auto: do not lock refs in the background

[Duy Nguyen]

On Sun, May 25, 2014 at 7:38 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> Keep running pack-refs and "reflog --prune" in foreground to stop
> parallel ref updates. The remaining background operations (repack,
> prune and rerere) should impact running git processes.

Eck.. s/should impact/should not impact/
-- 
Duy

Re: [PATCH] gc --auto: do not lock refs in the background

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> 9f673f9 (gc: config option for running --auto in background -
> 2014-02-08) puts "gc --auto" in background to reduce user's wait
> time. Part of the garbage collecting is pack-refs and pruning
> reflogs. These require locking some refs and may abort other processes
> trying to lock the same ref. If gc --auto is fired in the middle of a
> script, gc's holding locks in the background could fail the script,
> which could never happen before 9f673f9.
>
> Keep running pack-refs and "reflog --prune" in foreground to stop
> parallel ref updates. The remaining background operations (repack,
> prune and rerere) should impact running git processes.
>
> Reported-by: Adam Borowski <kilobyte@angband.pl>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/gc.c | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)

OK, as it happens the order of various gc phases we have is already
to run pack-refs and reflog expire before everything else, so this
change does not affect semantics, which is good ;-)



> diff --git a/builtin/gc.c b/builtin/gc.c
> index 85f5c2b..8d219d8 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -26,6 +26,7 @@ static const char * const builtin_gc_usage[] = {
>  };
>  
>  static int pack_refs = 1;
> +static int prune_reflogs = 1;
>  static int aggressive_depth = 250;
>  static int aggressive_window = 250;
>  static int gc_auto_threshold = 6700;
> @@ -258,6 +259,19 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
>  	return NULL;
>  }
>  
> +static int gc_before_repack(void)
> +{
> +	if (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))
> +		return error(FAILED_RUN, pack_refs_cmd.argv[0]);
> +
> +	if (prune_reflogs && run_command_v_opt(reflog.argv, RUN_GIT_CMD))
> +		return error(FAILED_RUN, reflog.argv[0]);
> +
> +	pack_refs = 0;
> +	prune_reflogs = 0;
> +	return 0;
> +}
> +
>  int cmd_gc(int argc, const char **argv, const char *prefix)
>  {
>  	int aggressive = 0;
> @@ -320,12 +334,15 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
>  				fprintf(stderr, _("Auto packing the repository for optimum performance.\n"));
>  			fprintf(stderr, _("See \"git help gc\" for manual housekeeping.\n"));
>  		}
> -		if (detach_auto)
> +		if (detach_auto) {
> +			if (gc_before_repack())
> +				return -1;
>  			/*
>  			 * failure to daemonize is ok, we'll continue
>  			 * in foreground
>  			 */
>  			daemonize();
> +		}
>  	} else
>  		add_repack_all_option();
>  
> @@ -337,11 +354,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
>  		    name, (uintmax_t)pid);
>  	}
>  
> -	if (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))
> -		return error(FAILED_RUN, pack_refs_cmd.argv[0]);
> -
> -	if (run_command_v_opt(reflog.argv, RUN_GIT_CMD))
> -		return error(FAILED_RUN, reflog.argv[0]);
> +	if (gc_before_repack())
> +		return -1;
>  
>  	if (run_command_v_opt(repack.argv, RUN_GIT_CMD))
>  		return error(FAILED_RUN, repack.argv[0]);