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

PATCH Superseded [email protected] 2 participants 4 messages Apr 5, 2017

lore

Share & tools

lore

[email protected]

author

2017-04-05 14:58 UTC #0 lore raw

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

From: Jeff Hostetler <[email protected]>

Version 5 of this patch series further simplifies the change to a single global variable for use by fsck. It eliminates the core config setting. Further discussion on the mailing list indicated that the config setting only added confusion and probably would not be used by anyone.

Jeff Hostetler (1):
read-cache: force_verify_index_checksum

 builtin/fsck.c  |  1 +
 cache.h         |  2 ++
 read-cache.c    |  7 +++++++
 t/t1450-fsck.sh | 11 +++++++++++
 4 files changed, 21 insertions(+)

Signature · 1 line

2.9.3

[email protected]

author

2017-04-05 14:58 UTC #1 lore raw

PATCH v5 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:

================
BEFORE:

$ ./p0002-read-cache.sh
0002.1: read_cache/discard_cache 1000 times   0.66(0.58+0.08)

AFTER:

$ ./p0002-read-cache.sh
0002.1: read_cache/discard_cache 1000 times   0.30(0.28+0.01)
================

Also, using the t/perf/p0004-read-tree.sh test that I created in a parallel patch series.

https://public-inbox.org/git/[email protected]/

Signed-off-by Jeff Hostetler <[email protected]>

---
 builtin/fsck.c  |  1 +
 cache.h         |  2 ++
 read-cache.c    |  7 +++++++
 t/t1450-fsck.sh | 11 +++++++++++
 4 files changed, 21 insertions(+)

builtin/fsck.c +1 -0

index 1a5cacc..04c5e50 100644
@@ -771,6 +771,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
 	}
 
 	if (keep_cache_objects) {
+		force_verify_index_checksum = 1;
 		read_cache();
 		for (i = 0; i < active_nr; i++) {
 			unsigned int mode;

cache.h +2 -0

index 80b6372..1cea517 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 force_verify_index_checksum;
+
 /* Environment bits from configuration mechanism */
 extern int trust_executable_bit;
 extern int trust_ctime;

read-cache.c +7 -0

index 9054369..5637fa3 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 force_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 (!force_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 +11 -0

index 33a51c9..86757ff 100755
@@ -689,4 +689,15 @@ test_expect_success 'bogus head does not fallback to all heads' '
 	! grep $blob out
 '
 
+test_expect_success PERL 'detect corrupt index file in fsck' '
+	cp .git/index .git/index.backup &&
+	echo zzzzzzzz >zzzzzzzz &&
+	git add zzzzzzzz &&
+	perl -pi -e "s/zzzzzzzz/yyyyyyyy/" .git/index &&
+	test_must_fail git fsck --cache &&
+	rm .git/index &&
+	mv .git/index.backup .git/index &&
+	rm zzzzzzzz
+'
+
 test_done

Signature · 1 line

2.9.3

Jonathan Nieder

Reviewed-by on v5 1/1

2017-04-05 17:03 UTC #2 lore raw

Re: [PATCH v5] read-cache: force_verify_index_checksum

Hi,

Quoted 7 lines · [email protected] wrote:

Signed-off-by Jeff Hostetler <[email protected]>

---
 builtin/fsck.c  |  1 +
 cache.h         |  2 ++
 read-cache.c    |  7 +++++++
 t/t1450-fsck.sh | 11 +++++++++++
 4 files changed, 21 insertions(+)

Yay! I love this version.

builtin/fsck.c int cmd_fsck(int argc, const char **argv, const char *prefix) · line 774

Quoted 7 lines

builtin/fsck.c +1 -0

@@ -771,6 +771,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
 	}
 
 	if (keep_cache_objects) {
+		force_verify_index_checksum = 1;

nit: now that there isn't a config this is overriding, this isn't "force_verify" so much as "verify".

[...]

Quoted 5 lines

builtin/fsck.c +1 -0

+++ b/t/t1450-fsck.sh

@@ -689,4 +689,15 @@ test_expect_success 'bogus head does not fallback to all heads' '
 	! grep $blob out
 '

builtin/fsck.c test_expect_success 'bogus head does not fallback to all heads' ' · line 701

Quoted 10 lines

builtin/fsck.c +10 -0

+test_expect_success PERL 'detect corrupt index file in fsck' '
+	cp .git/index .git/index.backup &&
+	echo zzzzzzzz >zzzzzzzz &&
+	git add zzzzzzzz &&
+	perl -pi -e "s/zzzzzzzz/yyyyyyyy/" .git/index &&
+	test_must_fail git fsck --cache &&
+	rm .git/index &&
+	mv .git/index.backup .git/index &&
+	rm zzzzzzzz
+'

This is great.

optional: you can do the cleanup commands in test_when_finished to make sure they happen even if the test fails.

Tests don't seem to use "perl -pi" anywhere else. This instance could be simplified by using sed.

With whatever subset of the changes below look good,

Reviewed-by Jonathan Nieder <[email protected]>

builtin/fsck.c +1 -1

index 92324e130c..b5e13a4556 100644
@@ -771,7 +771,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
 	}
 
 	if (keep_cache_objects) {
-		force_verify_index_checksum = 1;
+		verify_index_checksum = 1;
 		read_cache();
 		for (i = 0; i < active_nr; i++) {
 			unsigned int mode;

cache.h +1 -1

index 48b47083d3..9834d71f28 100644
@@ -706,7 +706,7 @@ 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 force_verify_index_checksum;
+extern int verify_index_checksum;
 
 /* Environment bits from configuration mechanism */
 extern int trust_executable_bit;

read-cache.c +2 -2

index 13326464d4..008b335844 100644
@@ -1372,7 +1372,7 @@ struct ondisk_cache_entry_extended {
 			    ondisk_cache_entry_size(ce_namelen(ce)))
 
 /* Allow fsck to force verification of the index checksum. */
-int force_verify_index_checksum;
+int verify_index_checksum;
 
 static int verify_hdr(struct cache_header *hdr, unsigned long size)
 {

@@ -1386,7 +1386,7 @@ static int verify_hdr(struct cache_header *hdr, unsigned long size)
 	if (hdr_version < INDEX_FORMAT_LB || INDEX_FORMAT_UB < hdr_version)
 		return error("bad index version %d", hdr_version);
 
-	if (!force_verify_index_checksum)
+	if (!verify_index_checksum)
 		return 0;
 
 	git_SHA1_Init(&c);

t/t1450-fsck.sh +6 -6

index 86757ffa52..e69b32f219 100755
@@ -689,15 +689,15 @@ test_expect_success 'bogus head does not fallback to all heads' '
 	! grep $blob out
 '
 
-test_expect_success PERL 'detect corrupt index file in fsck' '
+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 &&
+	test_when_finished "rm zzzzzzzz" &&
 	git add zzzzzzzz &&
-	perl -pi -e "s/zzzzzzzz/yyyyyyyy/" .git/index &&
-	test_must_fail git fsck --cache &&
-	rm .git/index &&
-	mv .git/index.backup .git/index &&
-	rm zzzzzzzz
+	sed -e "s/zzzzzzzz/yyyyyyyy/" .git/index >.git/index+ &&
+	mv .git/index+ .git/index &&
+	test_must_fail git fsck --cache
 '
 
 test_done

Jeff Hostetler

author

2017-04-05 18:10 UTC #3 lore raw

Re: [PATCH v5] read-cache: force_verify_index_checksum

Quoted 19 lines · Jonathan Nieder

Hi,
Quoted 7 lines · [email protected] wrote:
Signed-off-by Jeff Hostetler <[email protected]>
---
 builtin/fsck.c  |  1 +
 cache.h         |  2 ++
 read-cache.c    |  7 +++++++
 t/t1450-fsck.sh | 11 +++++++++++
 4 files changed, 21 insertions(+)
Yay! I love this version.
Quoted 5 lines
builtin/fsck.c +0 -0
@@ -771,6 +771,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
 	}
 

Quoted 5 lines

builtin/fsck.c int cmd_fsck(int argc, const char **argv, const char *prefix) · line 774
Quoted 2 lines
builtin/fsck.c +1 -0
 	if (keep_cache_objects) {
+		force_verify_index_checksum = 1;
nit: now that there isn't a config this is overriding, this isn't "force_verify" so much as "verify".

good point.

Quoted 23 lines

[...]
Quoted 5 lines
+++ b/t/t1450-fsck.sh
builtin/fsck.c +0 -0
@@ -689,4 +689,15 @@ test_expect_success 'bogus head does not fallback to all heads' '
 	! grep $blob out
 '
 
builtin/fsck.c test_expect_success 'bogus head does not fallback to all heads' ' · line 701
Quoted 10 lines
builtin/fsck.c +10 -0
+test_expect_success PERL 'detect corrupt index file in fsck' '
+	cp .git/index .git/index.backup &&
+	echo zzzzzzzz >zzzzzzzz &&
+	git add zzzzzzzz &&
+	perl -pi -e "s/zzzzzzzz/yyyyyyyy/" .git/index &&
+	test_must_fail git fsck --cache &&
+	rm .git/index &&
+	mv .git/index.backup .git/index &&
+	rm zzzzzzzz
+'
This is great.
optional: you can do the cleanup commands in test_when_finished to make sure they happen even if the test fails.

Quoted 6 lines

Tests don't seem to use "perl -pi" anywhere else. This instance could be simplified by using sed.
With whatever subset of the changes below look good,
Reviewed-by Jonathan Nieder <[email protected]>
[...]

good point.

I just pushed another version.
Thanks,
Jeff