read-cache: call verify_hdr() in a background thread
PATCH v6 1/1 read-cache: call verify_hdr() in a background thread
From: Jeff Hostetler <[email protected]>
Version 6 of this patch series cleans up a little further and uses test_when_finished in the unit test.
================
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 | 13 +++++++++++++ 4 files changed, 23 insertions(+)
Signature · 1 line
2.9.3
PATCH v6 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]/
--- 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
@@ -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
@@ -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
@@ -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
@@ -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 && + test_when_finished "rm zzzzzzzz" && + git add zzzzzzzz && + sed -e "s/zzzzzzzz/yyyyyyyy/" .git/index >.git/index.yyy && + rm .git/index && + mv .git/index.yyy .git/index && + test_when_finished "rm .git/index" && + test_must_fail git fsck --cache +' + test_done
Signature · 1 line
2.9.3
Re: [PATCH v6] read-cache: force_verify_index_checksum
Quoted 6 lines · On Wed, Apr 05, 2017 at 06:06:25PM +0000, [email protected] wrote:
--- builtin/fsck.c | 1 + cache.h | 2 ++ read-cache.c | 7 +++++++ t/t1450-fsck.sh | 13 +++++++++++++ 4 files changed, 23 insertions(+)
This version is delightfully simple. Thanks for sticking with it.
t/t1450-fsck.sh test_expect_success 'bogus head does not fallback to all heads' ' · line 703
Quoted 12 lines
t/t1450-fsck.sh +12
-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 && + test_when_finished "rm zzzzzzzz" && + git add zzzzzzzz && + sed -e "s/zzzzzzzz/yyyyyyyy/" .git/index >.git/index.yyy && + rm .git/index && + mv .git/index.yyy .git/index && + test_when_finished "rm .git/index" && + test_must_fail git fsck --cache +'
I was surprised by the second when-finished "rm". The earlier "mv" should be enough to restore everything. I guess you're trying to overcome the tendency of "mv" to complain about overwriting on some platforms.
But wouldn't that make the first when-finished "mv" potentially a problem, if it happens before the "rm .git/index" line?
Perhaps "mv -f" would make it all simpler (ordinarily I'd suggest just "git reset --hard", but since we're corrupting the index, it may be better not to rely on git for the restore). So maybe just:
cp .git/index .git/index.backup && test_when_finished "mv -f .git/index.backup .git/index" &&
...
I think the "rm zzzzzzzz" can go, too. We don't typically worry too much about accumulating random files in the working tree. The important thing is that the index is restored to a sane state.
-Peff
Re: [PATCH v6] read-cache: force_verify_index_checksum
Quoted 20 lines · Jeff King
Quoted 6 lines · On Wed, Apr 05, 2017 at 06:06:25PM +0000, [email protected] wrote:
--- builtin/fsck.c | 1 + cache.h | 2 ++ read-cache.c | 7 +++++++ t/t1450-fsck.sh | 13 +++++++++++++ 4 files changed, 23 insertions(+)This version is delightfully simple. Thanks for sticking with it.
Quoted 9 lines
t/t1450-fsck.sh +9
-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 && + test_when_finished "rm zzzzzzzz" && + git add zzzzzzzz && + sed -e "s/zzzzzzzz/yyyyyyyy/" .git/index >.git/index.yyy && + rm .git/index && + mv .git/index.yyy .git/index &&
Quoted 8 lines
t/t1450-fsck.sh test_expect_success 'bogus head does not fallback to all heads' ' · line 703
Quoted 3 lines
t/t1450-fsck.sh +3
-0+ test_when_finished "rm .git/index" && + test_must_fail git fsck --cache +'I was surprised by the second when-finished "rm". The earlier "mv" should be enough to restore everything. I guess you're trying to overcome the tendency of "mv" to complain about overwriting on some platforms.
Yeah, just being cautious. (I spend a lot of time on Windows.)
Quoted 12 lines
But wouldn't that make the first when-finished "mv" potentially a problem, if it happens before the "rm .git/index" line?
Perhaps "mv -f" would make it all simpler (ordinarily I'd suggest just "git reset --hard", but since we're corrupting the index, it may be better not to rely on git for the restore). So maybe just:
cp .git/index .git/index.backup &&
test_when_finished"mv -f .git/index.backup .git/index" &&
...
Quoted 6 lines
I think the "rm zzzzzzzz" can go, too. We don't typically worry too much about accumulating random files in the working tree. The important thing is that the index is restored to a sane state.
-Peff
Good point. Let me take another stab at it.
Thanks
Jeff
Re: [PATCH v6] read-cache: force_verify_index_checksum
In reply to [email protected]Hi,
Quoted 5 lines · [email protected] wrote:
t/t1450-fsck.sh +0
-0@@ -689,4 +689,17 @@ test_expect_success 'bogus head does not fallback to all heads' ' ! grep $blob out '
t/t1450-fsck.sh test_expect_success 'bogus head does not fallback to all heads' ' · line 700
Quoted 10 lines
t/t1450-fsck.sh +9
-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 && + test_when_finished "rm zzzzzzzz" && + git add zzzzzzzz && + sed -e "s/zzzzzzzz/yyyyyyyy/" .git/index >.git/index.yyy && + rm .git/index && + mv .git/index.yyy .git/index &&
Why is the "rm" before "mv" needed? I would have expected "mv" to replace the file.
If that doesn't work, I expect there are a large number of other tests that would need fixing...
t/t1450-fsck.sh test_expect_success 'bogus head does not fallback to all heads' ' · line 701
Quoted 1 line
t/t1450-fsck.sh +1
-0+ test_when_finished "rm .git/index" &&
Likewise --- this seems redundant next to the "mv" enqueued previously.
t/t1450-fsck.sh test_expect_success 'bogus head does not fallback to all heads' ' · line 702
Quoted 1 line
t/t1450-fsck.sh +1
-0+ test_must_fail git fsck --cache
That said,