Skip to contentSkip to conversation

read-cache: call verify_hdr() in a background thread

PATCH Superseded [email protected] 5 participants 13 messages Apr 14, 2017 – May 9, 2017

v7 of 8 Superseded Latest is v8 →

lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH v7 1/1 read-cache: call verify_hdr() in a background thread

From: Jeff Hostetler <[email protected]>

Version 7 of this patch cleans up the fsck test using test_when_finished and eliminates unnecessary mv/rm's.

Jeff Hostetler (1):
read-cache: force_verify_index_checksum

 builtin/fsck.c  |  1 +
 cache.h         |  2 ++
 read-cache.c    |  7 +++++++
 t/t1450-fsck.sh | 13 +++++++++++++
 4 files changed, 23 insertions(+)
Signature · 1 line
2.9.3
author
#1 lore raw

PATCH v7 1/1 read-cache: force_verify_index_checksum

From: Jeff Hostetler <[email protected]>

Teach git to skip verification of the SHA1-1 checksum at the end of the index file in verify_hdr() which is called from read_index() unless the "force_verify_index_checksum" global variable is set.

Teach fsck to force this verification.

The checksum verification is for detecting disk corruption, and for small projects, the time it takes to compute SHA-1 is not that significant, but for gigantic repositories this calculation adds significant time to every command.

These effect can be seen using t/perf/p0002-read-cache.sh:

Test HEAD~1 HEAD
--------------------------------------------------------------------------------------
0002.1: read_cache/discard_cache 1000 times 0.66(0.44+0.20) 0.30(0.27+0.02) -54.5%

Signed-off-by Jeff Hostetler <[email protected]>
---
 builtin/fsck.c  |  1 +
 cache.h         |  2 ++
 read-cache.c    |  7 +++++++
 t/t1450-fsck.sh | 13 +++++++++++++
 4 files changed, 23 insertions(+)
builtin/fsck.c +1 -0
index 1a5cacc..5512d06 100644
@@ -771,6 +771,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
 	}
 
 	if (keep_cache_objects) {
+		verify_index_checksum = 1;
 		read_cache();
 		for (i = 0; i < active_nr; i++) {
 			unsigned int mode;
cache.h +2 -0
index 80b6372..87f13bf 100644
@@ -685,6 +685,8 @@ extern void update_index_if_able(struct index_state *, struct lock_file *);
 extern int hold_locked_index(struct lock_file *, int);
 extern void set_alternate_index_output(const char *);
 
+extern int verify_index_checksum;
+
 /* Environment bits from configuration mechanism */
 extern int trust_executable_bit;
 extern int trust_ctime;
read-cache.c +7 -0
index 9054369..c4205aa 100644
@@ -1371,6 +1371,9 @@ struct ondisk_cache_entry_extended {
 			    ondisk_cache_entry_extended_size(ce_namelen(ce)) : \
 			    ondisk_cache_entry_size(ce_namelen(ce)))
 
+/* Allow fsck to force verification of the index checksum. */
+int verify_index_checksum;
+
 static int verify_hdr(struct cache_header *hdr, unsigned long size)
 {
 	git_SHA_CTX c;

@@ -1382,6 +1385,10 @@ static int verify_hdr(struct cache_header *hdr, unsigned long size)
 	hdr_version = ntohl(hdr->hdr_version);
 	if (hdr_version < INDEX_FORMAT_LB || INDEX_FORMAT_UB < hdr_version)
 		return error("bad index version %d", hdr_version);
+
+	if (!verify_index_checksum)
+		return 0;
+
 	git_SHA1_Init(&c);
 	git_SHA1_Update(&c, hdr, size - 20);
 	git_SHA1_Final(sha1, &c);
t/t1450-fsck.sh +13 -0
index 33a51c9..677e15a 100755
@@ -689,4 +689,17 @@ test_expect_success 'bogus head does not fallback to all heads' '
 	! grep $blob out
 '
 
+test_expect_success 'detect corrupt index file in fsck' '
+	cp .git/index .git/index.backup &&
+	test_when_finished "mv .git/index.backup .git/index" &&
+	echo zzzzzzzz >zzzzzzzz &&
+	git add zzzzzzzz &&
+	sed -e "s/zzzzzzzz/yyyyyyyy/" .git/index >.git/index.yyy &&
+	mv .git/index.yyy .git/index &&
+	# Confirm that fsck detects invalid checksum
+	test_must_fail git fsck --cache &&
+	# Confirm that status no longer complains about invalid checksum
+	git status
+'
+
 test_done
Signature · 1 line
2.9.3
#2 lore raw

Re: [PATCH v7] read-cache: force_verify_index_checksum

Am 14.04.2017 um 22:32 schrieb [email protected]:

Quoted 5 lines
t/t1450-fsck.sh +0 -0
index 33a51c9..677e15a 100755

t/t1450-fsck.sh test_expect_success 'bogus head does not fallback to all heads' ' · line 697

Quoted 9 lines
t/t1450-fsck.sh +6 -0
@@ -689,4 +689,17 @@ test_expect_success 'bogus head does not fallback to all heads' '
 	! grep $blob out
 '
 
+test_expect_success 'detect corrupt index file in fsck' '
+	cp .git/index .git/index.backup &&
+	test_when_finished "mv .git/index.backup .git/index" &&
+	echo zzzzzzzz >zzzzzzzz &&
+	git add zzzzzzzz &&
+	sed -e "s/zzzzzzzz/yyyyyyyy/" .git/index >.git/index.yyy &&

sed on a binary file? Sooner or later we are going to run into portability issues.

t/t1450-fsck.sh test_expect_success 'bogus head does not fallback to all heads' ' · line 700

Quoted 3 lines
t/t1450-fsck.sh +3 -0
+	mv .git/index.yyy .git/index &&
+	# Confirm that fsck detects invalid checksum
+	test_must_fail git fsck --cache &&

You should ensure that this failure is really because of an invalid checksum. The failure could also be due to an extra LF at the end that sed inserted, no?

t/t1450-fsck.sh test_expect_success 'bogus head does not fallback to all heads' ' · line 705

Quoted 5 lines
t/t1450-fsck.sh +4 -0
+	# Confirm that status no longer complains about invalid checksum
+	git status
+'
+
 test_done

-- Hannes

author
#3 lore raw

Re: [PATCH v7] read-cache: force_verify_index_checksum

Quoted 10 lines · Johannes Sixt

Am 14.04.2017 um 22:32 schrieb [email protected]:

Quoted 9 lines
t/t1450-fsck.sh +0 -0
index 33a51c9..677e15a 100755
@@ -689,4 +689,17 @@ test_expect_success 'bogus head does not

fallback to all heads' '
! grep $blob out
'

Quoted 8 lines

t/t1450-fsck.sh test_expect_success 'bogus head does not fallback to all heads' ' · line 697

Quoted 6 lines
t/t1450-fsck.sh +6 -0
+test_expect_success 'detect corrupt index file in fsck' '
+    cp .git/index .git/index.backup &&
+    test_when_finished "mv .git/index.backup .git/index" &&
+    echo zzzzzzzz >zzzzzzzz &&
+    git add zzzzzzzz &&
+    sed -e "s/zzzzzzzz/yyyyyyyy/" .git/index >.git/index.yyy &&

sed on a binary file? Sooner or later we are going to run into portability issues.

In v5 of this patch series I used "perl" and it was suggested that I use "sed" instead. It doesn't matter to me which we use. My testing showed that it was safe, but that was only Linux.

Does the mailing list have a preference for this ?

Quoted 7 lines

t/t1450-fsck.sh test_expect_success 'bogus head does not fallback to all heads' ' · line 700

Quoted 5 lines
t/t1450-fsck.sh +3 -0
+    mv .git/index.yyy .git/index &&
+    # Confirm that fsck detects invalid checksum
+    test_must_fail git fsck --cache &&

You should ensure that this failure is really because of an invalid checksum. The failure could also be due to an extra LF at the end that sed inserted, no?

I suppose we could, but I'm tempted to wait on that for now.

Jeff

#4 lore raw

Re: [PATCH v7] read-cache: force_verify_index_checksum

Quoted 10 lines · Jeff Hostetler
Quoted 9 lines

t/t1450-fsck.sh test_expect_success 'bogus head does not fallback to all heads' ' · line 697

Quoted 6 lines
t/t1450-fsck.sh +6 -0
+test_expect_success 'detect corrupt index file in fsck' '
+    cp .git/index .git/index.backup &&
+    test_when_finished "mv .git/index.backup .git/index" &&
+    echo zzzzzzzz >zzzzzzzz &&
+    git add zzzzzzzz &&
+    sed -e "s/zzzzzzzz/yyyyyyyy/" .git/index >.git/index.yyy &&

sed on a binary file? Sooner or later we are going to run into portability issues.

Quoted 7 lines

In v5 of this patch series I used "perl" and it was suggested that I use "sed" instead.
It doesn't matter to me which we use. My testing showed that it was safe, but that
was only Linux.

Does the mailing list have a preference for this ?

Instead of munging pathnames z* to y*, I'd prefer to see the actual checksum bytes at the end replaced in the index file. After all that is what this test really cares about, and it ensures that the failure detected is due to checksum mismatch.

Quoted 11 lines
Quoted 7 lines

t/t1450-fsck.sh test_expect_success 'bogus head does not fallback to all heads' ' · line 700

Quoted 3 lines
t/t1450-fsck.sh +3 -0
+    mv .git/index.yyy .git/index &&
+    # Confirm that fsck detects invalid checksum
+    test_must_fail git fsck --cache &&

You should ensure that this failure is really because of an invalid checksum. The failure could also be due to an extra LF at the end that sed inserted, no?

I suppose we could, but I'm tempted to wait on that for now.

Jeff

#5 lore raw

Re: [PATCH v7] read-cache: force_verify_index_checksum

Hi,

Quoted 9 lines · Junio C Hamano
Quoted 7 lines · Jeff Hostetler
Quoted 7 lines
Quoted 6 lines
t/t1450-fsck.sh +6 -0
+test_expect_success 'detect corrupt index file in fsck' '
+    cp .git/index .git/index.backup &&
+    test_when_finished "mv .git/index.backup .git/index" &&
+    echo zzzzzzzz >zzzzzzzz &&
+    git add zzzzzzzz &&
+    sed -e "s/zzzzzzzz/yyyyyyyy/" .git/index >.git/index.yyy &&
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines

sed on a binary file? Sooner or later we are going to run into portability issues.

In v5 of this patch series I used "perl" and it was suggested that I use "sed" instead. It doesn't matter to me which we use. My testing showed that it was safe, but that was only Linux.

I am sorry to hear that the Git mailing list's review gives you whiplash.

The problem with sed is that BSD sed behaves a bit differently than GNU sed, and we quietly expect every contributor to be an expert in the portability aspects of sed.

TBH I am quite surprised that anybody would have suggested to use sed rather than Perl to edit binary files in the first place. In my opinion, that was bad advice.

Quoted 6 lines
Quoted 1 line

Does the mailing list have a preference for this ?

Instead of munging pathnames z* to y*, I'd prefer to see the actual checksum bytes at the end replaced in the index file. After all that is what this test really cares about, and it ensures that the failure detected is due to checksum mismatch.

I see that v8 uses a Perl script again, and it is well written and obvious.

Just in case that certain reviewers favor length over readability, let me offer this snippet:

	size=$(perl -e "print -s \".git/index\"") &&
	dd if=/dev/zero of=.git/index bs=1 seek=$(($size-20) count=20

Since whatever hash will be used in the future is most likely larger than 20 bytes, this should still work fine (and even if somebody sane replaces the SHA-1 of the index with a CRC-32 for the same benefit we have now, the test will fail quickly and it is easy to replace the 20 by 4).

Ciao,
Dscho

#6 lore raw

Re: [PATCH v7] read-cache: force_verify_index_checksum

Quoted 5 lines · Johannes Schindelin

Just in case that certain reviewers favor length over readability, let me offer this snippet:

	size=$(perl -e "print -s \".git/index\"") &&
	dd if=/dev/zero of=.git/index bs=1 seek=$(($size-20) count=20

Yup, this does sound like a good direction to go (I think you have some unbalanced parens but it is sufficient to convey the idea).

Quoted 4 lines

Since whatever hash will be used in the future is most likely larger than 20 bytes, this should still work fine (and even if somebody sane replaces the SHA-1 of the index with a CRC-32 for the same benefit we have now, the test will fail quickly and it is easy to replace the 20 by 4).

True that, too.

#7 lore raw

Re: [PATCH v7] read-cache: force_verify_index_checksum

In reply to [email protected]
Quoted 10 lines · On Fri, Apr 14, 2017 at 10:32 PM, <[email protected]> wrote:
t/t1450-fsck.sh +2 -0
index 33a51c9..677e15a 100755
@@ -689,4 +689,17 @@ test_expect_success 'bogus head does not fallback to all heads' '
        ! grep $blob out
 '
 
+test_expect_success 'detect corrupt index file in fsck' '
+       cp .git/index .git/index.backup &&

t/t1450-fsck.sh test_expect_success 'bogus head does not fallback to all heads' ' · line 703

Quoted 10 lines
t/t1450-fsck.sh +10 -0
+       test_when_finished "mv .git/index.backup .git/index" &&
+       echo zzzzzzzz >zzzzzzzz &&
+       git add zzzzzzzz &&
+       sed -e "s/zzzzzzzz/yyyyyyyy/" .git/index >.git/index.yyy &&
+       mv .git/index.yyy .git/index &&
+       # Confirm that fsck detects invalid checksum
+       test_must_fail git fsck --cache &&
+       # Confirm that status no longer complains about invalid checksum
+       git status
+'

This test does not pass when the GIT_TEST_SPLIT_INDEX env variable is set on my Linux machine.

Also it looks like you sent a v8 of this patch series with a different test, but what is in master looks like the above test instead of the test in your v8.

author
#8 lore raw

Re: [PATCH v7] read-cache: force_verify_index_checksum

Quoted 25 lines · Christian Couder
Quoted 10 lines · On Fri, Apr 14, 2017 at 10:32 PM, <[email protected]> wrote:
t/t1450-fsck.sh +2 -0
index 33a51c9..677e15a 100755
@@ -689,4 +689,17 @@ test_expect_success 'bogus head does not fallback to all heads' '
        ! grep $blob out
 '
 
+test_expect_success 'detect corrupt index file in fsck' '
+       cp .git/index .git/index.backup &&

t/t1450-fsck.sh test_expect_success 'bogus head does not fallback to all heads' ' · line 703

Quoted 10 lines
t/t1450-fsck.sh +10 -0
+       test_when_finished "mv .git/index.backup .git/index" &&
+       echo zzzzzzzz >zzzzzzzz &&
+       git add zzzzzzzz &&
+       sed -e "s/zzzzzzzz/yyyyyyyy/" .git/index >.git/index.yyy &&
+       mv .git/index.yyy .git/index &&
+       # Confirm that fsck detects invalid checksum
+       test_must_fail git fsck --cache &&
+       # Confirm that status no longer complains about invalid checksum
+       git status
+'

This test does not pass when the GIT_TEST_SPLIT_INDEX env variable is set on my Linux machine.

Quoted 4 lines

Also it looks like you sent a v8 of this patch series with a different test, but what is in master looks like the above test instead of the test in your v8.

There was concern about using sed on a binary file not being portable and a request to change the test to just corrupt the checksum rather than an index-entry, so I changed it in v8.

Does the v8 version of the test also fail on your machine ?

Thanks
Jeff

#9 lore raw

Re: [PATCH v7] read-cache: force_verify_index_checksum

Quoted 11 lines · Jeff Hostetler
Quoted 7 lines · Christian Couder

This test does not pass when the GIT_TEST_SPLIT_INDEX env variable is set on my Linux machine.

Also it looks like you sent a v8 of this patch series with a different test, but what is in master looks like the above test instead of the test in your v8.

Quoted 5 lines

There was concern about using sed on a binary file not being portable and a request to change the test to just corrupt the checksum rather than an index-entry, so I changed it in v8.

Does the v8 version of the test also fail on your machine ?

The v8 version of the test succeeds on my machine.

author
#10 lore raw

Re: [PATCH v7] read-cache: force_verify_index_checksum

Quoted 12 lines · Christian Couder
Quoted 11 lines · Jeff Hostetler
Quoted 7 lines · Christian Couder

This test does not pass when the GIT_TEST_SPLIT_INDEX env variable is set on my Linux machine.

Also it looks like you sent a v8 of this patch series with a different test, but what is in master looks like the above test instead of the test in your v8.

Quoted 7 lines
Quoted 5 lines

There was concern about using sed on a binary file not being portable and a request to change the test to just corrupt the checksum rather than an index-entry, so I changed it in v8.

Does the v8 version of the test also fail on your machine ?

The v8 version of the test succeeds on my machine.

OK, thanks. It worked on mine too. Since the v8 version is a better test, I'm content to stick with it and not try to address the problem with the previous version.

Jeff

#11 lore raw

Re: [PATCH v7] read-cache: force_verify_index_checksum

Quoted 17 lines · Jeff Hostetler
Quoted 16 lines · Christian Couder
Quoted 15 lines · Jeff Hostetler
Quoted 7 lines · Christian Couder

This test does not pass when the GIT_TEST_SPLIT_INDEX env variable is set on my Linux machine.

Also it looks like you sent a v8 of this patch series with a different test, but what is in master looks like the above test instead of the test in your v8.

There was concern about using sed on a binary file not being portable and a request to change the test to just corrupt the checksum rather than an index-entry, so I changed it in v8.

Quoted 7 lines
Quoted 3 lines
Quoted 1 line

Does the v8 version of the test also fail on your machine ?

The v8 version of the test succeeds on my machine.

OK, thanks. It worked on mine too. Since the v8 version is a better test, I'm content to stick with it and not try to address the problem with the previous version.

Thanks. Let's merge that in.

#12 lore raw

Re: [PATCH v7] read-cache: force_verify_index_checksum

In reply to Christian Couder
Quoted 25 lines · Christian Couder
Quoted 10 lines · On Fri, Apr 14, 2017 at 10:32 PM, <[email protected]> wrote:
t/t1450-fsck.sh +2 -0
index 33a51c9..677e15a 100755
@@ -689,4 +689,17 @@ test_expect_success 'bogus head does not fallback to all heads' '
        ! grep $blob out
 '
 
+test_expect_success 'detect corrupt index file in fsck' '
+       cp .git/index .git/index.backup &&

t/t1450-fsck.sh test_expect_success 'bogus head does not fallback to all heads' ' · line 703

Quoted 10 lines
t/t1450-fsck.sh +10 -0
+       test_when_finished "mv .git/index.backup .git/index" &&
+       echo zzzzzzzz >zzzzzzzz &&
+       git add zzzzzzzz &&
+       sed -e "s/zzzzzzzz/yyyyyyyy/" .git/index >.git/index.yyy &&
+       mv .git/index.yyy .git/index &&
+       # Confirm that fsck detects invalid checksum
+       test_must_fail git fsck --cache &&
+       # Confirm that status no longer complains about invalid checksum
+       git status
+'

This test does not pass when the GIT_TEST_SPLIT_INDEX env variable is set on my Linux machine.

Quoted 3 lines

Also it looks like you sent a v8 of this patch series with a different test, but what is in master looks like the above test instead of the test in your v8.

Thanks for raising this. I almost forgot that the final finishing touch to update the test was needed in 'master', and this exchange caught it before it is too late.