threads / discuss / 17652

gc considered dangerous

Subject: gc considered dangerous

## tl;dr

13 messages between Feb 8, 2009 and Feb 9, 2009.

replies: 12people: 5as markdown or json

Robin Rosenberg· Feb 8, 2009, 02:47 UTC · lore

I've seen this. Running git gc on Windows, while having Elipse open can kill your object database.

Eclipse keeps the pack files open most of the time. Then you launch git gui which recommends the user to do a git gc. I never do (it *always* wants to do this), so I haven't encountered the issue, but if gc doesn't find a new optimal pack it tries to rewrite a new pack with the same id. So it rm's the idx file (fine) and the the pack file (not ok) and gives up, which means it has a .pack file with no index, so it cannot use it. Trying git gc again after eclipse exits will execute the final stab on your objects.

The underlying bug is ofcource that Windows locks files when they are open. A *nix* user does not suffer from this problem.

I'll investigate more at some other time. This is a preliminary analysis.

-- robin
Robin Rosenberg· Feb 8, 2009, 14:34 UTC · re: Robin Rosenberg · lore

Re: gc considered dangerous

söndag 08 februari 2009 03:47:25 skrev Robin Rosenberg:
Show 18 quoted lines
> 
> I've seen this. Running git gc on Windows, while having Elipse open can kill your
> object database. 
> 
> Eclipse keeps the pack files open most of the time. Then you 
> launch git gui which recommends the user to do a git gc. I never
> do (it *always* wants to do this), so I haven't encountered the 
> issue, but if gc doesn't find a new optimal pack it tries to rewrite a 
> new pack with the same id. So it rm's the idx file (fine) and the the
> pack file (not ok) and gives up, which means it has a .pack file with 
> no index, so it cannot use it. Trying git gc again after eclipse exits 
> will execute the final stab on your objects. 
> 
> The underlying bug is ofcource that Windows locks files when
> they are open. A *nix* user does not suffer from this problem.
> 
> I'll investigate more at some other time. This is a preliminary
> analysis.
A receipe for triggering is available at http://code.google.com/p/msysgit/issues/detail?id=189
-- robin
Johannes Schindelin· Feb 8, 2009, 14:56 UTC · re: Robin Rosenberg · lore

Re: gc considered dangerous

Hi,
On Sun, 8 Feb 2009, Robin Rosenberg wrote:
> I've seen this. Running git gc on Windows, while having Elipse open can 
> kill your object database.
You had me really scared with the mail subject!
> if gc doesn't find a new optimal pack it tries to rewrite a new pack 
> with the same id. So it rm's the idx file (fine) and the the pack file 
> (not ok) and gives up,

I disagree with your notion that it is fine to kill the existing idx file until the new one has been written successfully.

My preliminary guess is that this code in pack-write.c needs to use the lock file paradigm:

        if (!index_name) {
		[...]
        } else {
                unlink(index_name);
                fd = open(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
        }

Ciao, Dscho

Robin Rosenberg· Feb 8, 2009, 16:04 UTC · re: Johannes Schindelin · lore

Re: gc considered dangerous

söndag 08 februari 2009 15:56:38 skrev Johannes Schindelin:
Show 8 quoted lines
> Hi,
> 
> On Sun, 8 Feb 2009, Robin Rosenberg wrote:
> 
> > I've seen this. Running git gc on Windows, while having Elipse open can 
> > kill your object database.
> 
> You had me really scared with the mail subject!

Losing a repo *is* scary., especially if your boss is the one losing it. That should simply not happen unless there is a disk failure. Fortunately I believe the two lost branches did not contain anything useful.

Show 6 quoted lines
> > if gc doesn't find a new optimal pack it tries to rewrite a new pack 
> > with the same id. So it rm's the idx file (fine) and the the pack file 
> > (not ok) and gives up,
> 
> I disagree with your notion that it is fine to kill the existing idx file 
> until the new one has been written successfully.

Then you misunderstood me. I meant that the repack script thought it was fine.

> My preliminary guess is that this code in pack-write.c needs to use the 
> lock file paradigm:
Show 7 quoted lines
> 
>         if (!index_name) {
> 		[...]
>         } else {
>                 unlink(index_name);
>                 fd = open(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
>         }

Yeah, it needs to check that it can actually delete both files before actually doing it. Then again if it wants to replace a file with a new one just like it, why shouldn't it just skip it. Isn't that the only case I see where we can lose data?

GC should also notice that something is not right when there are old-files present.

-- robin
Johannes Schindelin· Feb 8, 2009, 17:18 UTC · re: Robin Rosenberg · lore

Re: gc considered dangerous

Hi,
On Sun, 8 Feb 2009, Robin Rosenberg wrote:
Show 13 quoted lines
> söndag 08 februari 2009 15:56:38 skrev Johannes Schindelin:
> 
> > On Sun, 8 Feb 2009, Robin Rosenberg wrote:
> > 
> > > I've seen this. Running git gc on Windows, while having Elipse open 
> > > can kill your object database.
> > 
> > You had me really scared with the mail subject!
> 
> Losing a repo *is* scary., especially if your boss is the one losing it. 
> That should simply not happen unless there is a disk failure. 
> Fortunately I believe the two lost branches did not contain anything 
> useful.
If you care about your data, you don't use Windows, IMHO.
Show 23 quoted lines
> > > if gc doesn't find a new optimal pack it tries to rewrite a new pack 
> > > with the same id. So it rm's the idx file (fine) and the the pack 
> > > file (not ok) and gives up,
> > 
> > I disagree with your notion that it is fine to kill the existing idx 
> > file until the new one has been written successfully.
>
> Then you misunderstood me. I meant that the repack script thought it was 
> fine.
> 
> > My preliminary guess is that this code in pack-write.c needs to use the 
> > lock file paradigm:
> 
> > 
> >         if (!index_name) {
> > 		[...]
> >         } else {
> >                 unlink(index_name);
> >                 fd = open(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
> >         }
> 
> Yeah, it needs to check that it can actually delete both files before 
> actually doing it.
Nope.  It needs to avoid unlinking until it knows what it did was fine.
> Then again if it wants to replace a file with a new one just like it,

It might have the same name, but that does not mean that it is bytewise identical. The name depends on the uncompressed objects.

> Isn't that the only case I see where we can lose data?

No, you said so yourself: Eclipse prevents the pack from being deleted. There are a gazillion idiotic programs on Windows who think they should lock random files.

So you could hit the very same issue even if your repository has twenty packs, and therefore could do with some gc'ing.

Ciao, Dscho

Steffen Prohaska· Feb 8, 2009, 18:24 UTC · re: Robin Rosenberg · lore

Re: gc considered dangerous

On Feb 8, 2009, at 5:04 PM, Robin Rosenberg wrote:
Show 16 quoted lines
> söndag 08 februari 2009 15:56:38 skrev Johannes Schindelin:
>> Hi,
>>
>> On Sun, 8 Feb 2009, Robin Rosenberg wrote:
>>
>>> I've seen this. Running git gc on Windows, while having Elipse  
>>> open can
>>> kill your object database.
>>
>> You had me really scared with the mail subject!
>
> Losing a repo *is* scary., especially if your boss is the one losing  
> it.
> That should simply not happen unless there is a disk failure.  
> Fortunately
> I believe the two lost branches did not contain anything useful.

If you have a backup of the corrupted repository right after it got corrupted, you can create an index by running "git index-pack" on the pack that is lacking an index. This might rescue the lost branches.

	Steffen
Robin Rosenberg· Feb 8, 2009, 19:37 UTC · re: Steffen Prohaska · lore

Re: gc considered dangerous

söndag 08 februari 2009 19:24:33 skrev Steffen Prohaska:
> If you have a backup of the corrupted repository right after it got
> corrupted, you can create an index by running "git index-pack" on
> the pack that is lacking an index.  This might rescue the lost
> branches.

Yep, but the problem was that git gc was run again on the corrupted repo so there was only the idx left. (L)Users...

Couldn't we make repack refuse to repack if it discovered left-overs from a failed repack, or at least make it use the old pack files as reference instead of ignoring them? Leaving too many objects is, after all, much better than destroying the repo.

-- robin
Junio C Hamano· Feb 8, 2009, 20:00 UTC · re: Johannes Schindelin · lore

Re: gc considered dangerous

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 9 quoted lines
> My preliminary guess is that this code in pack-write.c needs to use the 
> lock file paradigm:
>
>         if (!index_name) {
> 		[...]
>         } else {
>                 unlink(index_name);
>                 fd = open(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
>         }
Whoa.  That particular code has been (and is still) correct.

When repacking we should pack into a temporary pack and idx file and then replace the real ones after both new pack and its idx are successfully written, and I thought that is how we've been doing this all the time. Maybe the caller has been broken at some point? Sigh...

Robin Rosenberg· Feb 8, 2009, 20:08 UTC · re: Junio C Hamano · lore

Re: gc considered dangerous

söndag 08 februari 2009 21:00:57 skrev Junio C Hamano:
Show 18 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > My preliminary guess is that this code in pack-write.c needs to use the 
> > lock file paradigm:
> >
> >         if (!index_name) {
> > 		[...]
> >         } else {
> >                 unlink(index_name);
> >                 fd = open(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
> >         }
> 
> Whoa.  That particular code has been (and is still) correct.
> 
> When repacking we should pack into a temporary pack and idx file and then
> replace the real ones after both new pack and its idx are successfully
> written, and I thought that is how we've been doing this all the time.
> Maybe the caller has been broken at some point?  Sigh...
I intend to test something like this:
Robin Rosenberg· Feb 8, 2009, 20:10 UTC · re: Junio C Hamano · lore

Re: gc considered dangerous

söndag 08 februari 2009 21:00:57 skrev Junio C Hamano:
Show 19 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > My preliminary guess is that this code in pack-write.c needs to use the 
> > lock file paradigm:
> >
> >         if (!index_name) {
> > 		[...]
> >         } else {
> >                 unlink(index_name);
> >                 fd = open(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
> >         }
> 
> Whoa.  That particular code has been (and is still) correct.
> 
> When repacking we should pack into a temporary pack and idx file and then
> replace the real ones after both new pack and its idx are successfully
> written, and I thought that is how we've been doing this all the time.
> Maybe the caller has been broken at some point?  Sigh...
> 
I intend to test something like this (as of yet completely untested)
-- robin
commit 25a77c80efeb221d165db0bef66b4498aacfac96
Author: Robin Rosenberg <robin.rosenberg@dewire.com>
Date:   Sun Feb 8 20:59:30 2009 +0100
    Make repack more fail-safe
    
    If renaming an old pack fails try to restore halfway renames
    befor failing. The basis is the assumption that this occurs
    because a files is locked for reading on Windows.
diff --git a/git-repack.sh b/git-repack.sh
index 458a497..e816997 100755
--- a/git-repack.sh
+++ b/git-repack.sh
@@ -94,14 +94,25 @@ for name in $names ; do
 	chmod a-w "$PACKTMP-$name.idx"
 	mkdir -p "$PACKDIR" || exit
 
-	for sfx in pack idx
-	do
-		if test -f "$PACKDIR/pack-$name.$sfx"
-		then
-			mv -f "$PACKDIR/pack-$name.$sfx" \
-				"$PACKDIR/old-pack-$name.$sfx"
-		fi
-	done &&
+	if test -f "$PACKDIR/pack-$name.pack"
+	then
+		mv -f "$PACKDIR/pack-$name.pack" \
+			"$PACKDIR/old-pack-$name.pack"
+	fi &&
+	if test -f "$PACKDIR/pack-$name.idx"
+	then
+		mv -f "$PACKDIR/pack-$name.idx" \
+			"$PACKDIR/old-pack-$name.idx" ||
+		(
+			mv -f $PACKDIR/old-pack-$name.pack" \
+			"$PACKDIR/pack-$name.pack" || (
+			echo >&2 "Failed to restore after a failure to rename pack-$name to old-$pack"
+			echo >&2 "Please aquire advice on how to recover from this"
+			echo >&2 "situation before you proceed."
+			false
+			)
+		)
+	fi &&
 	mv -f "$PACKTMP-$name.pack" "$PACKDIR/pack-$name.pack" &&
 	mv -f "$PACKTMP-$name.idx"  "$PACKDIR/pack-$name.idx" &&
 	test -f "$PACKDIR/pack-$name.pack" &&
@@ -109,6 +120,8 @@ for name in $names ; do
 		echo >&2 "Couldn't replace the existing pack with updated one."
 		echo >&2 "The original set of packs have been saved as"
 		echo >&2 "old-pack-$name.{pack,idx} in $PACKDIR."
+		echo >&2 "Please aquire advice on how to recover from this situation"
+		echo >&2 "before you proceed."
 		exit 1
 	}
 	rm -f "$PACKDIR/old-pack-$name.pack" "$PACKDIR/old-pack-$name.idx"
Mike Hommey· Feb 9, 2009, 06:59 UTC · re: Junio C Hamano · lore

Re: gc considered dangerous

On Sun, Feb 08, 2009 at 12:00:57PM -0800, Junio C Hamano wrote:
Show 13 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > My preliminary guess is that this code in pack-write.c needs to use the 
> > lock file paradigm:
> >
> >         if (!index_name) {
> > 		[...]
> >         } else {
> >                 unlink(index_name);
> >                 fd = open(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
> >         }
> 
> Whoa.  That particular code has been (and is still) correct.

Don't both unlink and open fail in the case the index file is locked in Windows ?

Mike
Junio C Hamano· Feb 9, 2009, 07:43 UTC · re: Mike Hommey · lore

Re: gc considered dangerous

Mike Hommey <mh@glandium.org> writes:
Show 17 quoted lines
> On Sun, Feb 08, 2009 at 12:00:57PM -0800, Junio C Hamano wrote:
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 
>> > My preliminary guess is that this code in pack-write.c needs to use the 
>> > lock file paradigm:
>> >
>> >         if (!index_name) {
>> > 		[...]
>> >         } else {
>> >                 unlink(index_name);
>> >                 fd = open(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
>> >         }
>> 
>> Whoa.  That particular code has been (and is still) correct.
>
> Don't both unlink and open fail in the case the index file is locked in
> Windows ?
Read the rest of the thread and read the code.

The index_file in question is not the file git-repack is telling pack-write.c to unlink and open.

Junio C Hamano· Feb 8, 2009, 20:09 UTC · re: Robin Rosenberg · lore

Re: gc considered dangerous

Robin Rosenberg <robin.rosenberg.lists@dewire.com> writes:
Show 17 quoted lines
> I've seen this. Running git gc on Windows, while having Elipse open can kill your
> object database. 
>
> Eclipse keeps the pack files open most of the time. Then you 
> launch git gui which recommends the user to do a git gc. I never
> do (it *always* wants to do this), so I haven't encountered the 
> issue, but if gc doesn't find a new optimal pack it tries to rewrite a 
> new pack with the same id. So it rm's the idx file (fine) and the the
> pack file (not ok) and gives up, which means it has a .pack file with 
> no index, so it cannot use it. Trying git gc again after eclipse exits 
> will execute the final stab on your objects. 
>
> The underlying bug is ofcource that Windows locks files when
> they are open. A *nix* user does not suffer from this problem.
>
> I'll investigate more at some other time. This is a preliminary
> analysis.
That sounds like you are hitting this codepath in git-repack.sh:
	fullbases="$fullbases pack-$name"
	chmod a-w "$PACKTMP-$name.pack"
	chmod a-w "$PACKTMP-$name.idx"
	mkdir -p "$PACKDIR" || exit
	for sfx in pack idx
	do
		if test -f "$PACKDIR/pack-$name.$sfx"
		then
			mv -f "$PACKDIR/pack-$name.$sfx" \
				"$PACKDIR/old-pack-$name.$sfx"
		fi
	done &&
	mv -f "$PACKTMP-$name.pack" "$PACKDIR/pack-$name.pack" &&
	mv -f "$PACKTMP-$name.idx"  "$PACKDIR/pack-$name.idx" &&
	test -f "$PACKDIR/pack-$name.pack" &&
	test -f "$PACKDIR/pack-$name.idx" || {
		echo >&2 "Couldn't replace the existing pack with updated one."
		echo >&2 "The original set of packs have been saved as"
		echo >&2 "old-pack-$name.{pack,idx} in $PACKDIR."
		exit 1
	}
	rm -f "$PACKDIR/old-pack-$name.pack" "$PACKDIR/old-pack-$name.idx"

We created a pack and an idx successfully in a pair of temporary files, we notice that pack-$name.idx and/or pack-$name.pack exists and try to move them out of the way, then we install the new ones in their final destination, and we try to see if that move succeeded. If any one of these steps fails, the entire process fails, but along the way we shouldn't have lost anything.

I see if .pack can be renamed but .idx can't, then it is possible to get into a state where you have to mix and match pack-$name.pack and old-pack-$name.idx.

← back to recent threads