read-cache: call verify_hdr() in a background thread
v4 cover read-cache: call verify_hdr() in a background thread
From: Jeff Hostetler <[email protected]>
Version 4 of this patch series incorporates the cleanup made by Junio to my 3rd veresion, updates fsck to always verify the checksum, and a new simplified perf test using p0002 as discussed on the mailing list.
================
Version 3 of this patch series simplifies this effort to just turn on/off the hash verification using a "core.checksumindex" config variable.
I've preserved the original checksum validation code so that we can force it on in fsck if desired.
It eliminates the original threading model completely.
Jeff Hostetler (4):
read-cache: core.checksumindex
fsck: force core.checksumindex=1
t1450-fsck: test core.checksumindex
p0002-read-cache: test core.checksumindex
Documentation/config.txt | 8 ++++++++ builtin/fsck.c | 1 + cache.h | 7 +++++++ read-cache.c | 18 ++++++++++++++++++ t/perf/p0002-read-cache.sh | 8 +++++++- t/t1450-fsck.sh | 27 +++++++++++++++++++++++++++ 6 files changed, 68 insertions(+), 1 deletion(-)
Signature · 1 line
2.9.3
PATCH v4 1/4 read-cache: core.checksumindex
From: Jeff Hostetler <[email protected]>
Teach git to skip verification of the SHA-1 checksum at the end of the index file in verify_hdr() called from read_index() when the core.checksumIndex configuration variable is set to false.
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.
On the Linux kernel repository, the effect is rather trivial. The time to reading its index with 58k entries drops from 0.0284 sec down to 0.0155 sec.
On my Windows source tree (450MB index), I'm seeing a savings of 0.6 seconds -- read_index() went from 1.2 to 0.6 seconds.
--- Documentation/config.txt | 8 ++++++++ read-cache.c | 12 ++++++++++++ 2 files changed, 20 insertions(+)
Documentation/config.txt +8 -0
@@ -267,6 +267,14 @@ advice.*:: show directions on how to proceed from the current state. -- +core.checksumIndex:: + Tell Git to validate the checksum at the end of the index + file to detect corruption. Defaults to `true`. Those who + work on a project with too many files may want to set this + variable to `false` to make it faster to load the index (in + exchange for reliability, but in general modern disks are + reliable enough for most people). + core.fileMode:: Tells Git if the executable bit of files in the working tree is to be honored.
read-cache.c +12 -0
@@ -1376,12 +1376,24 @@ static int verify_hdr(struct cache_header *hdr, unsigned long size)git_SHA_CTX c;unsigned char sha1[20 ];int hdr_version; +int do_checksum =1 ;/* default to true for now */ if (hdr->hdr_signature !=htonl (CACHE_SIGNATURE ))return error ("bad signature" ); 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); + +/* +* Since we run very early in command startup, git_config() +* may not have been called yet and the various "core_*" +* global variables haven't been set. So look it up +* explicitly. +*/ +git_config_get_bool ("core.checksumindex" , &do_checksum); +if (!do_checksum) +return 0 ; +git_SHA1_Init (&c);git_SHA1_Update (&c, hdr, size -20 );git_SHA1_Final (sha1, &c);
Signature · 1 line
2.9.3
PATCH v4 2/4 fsck: force core.checksumindex=1
In reply to [email protected]From: Jeff Hostetler <[email protected]>
Teach fsck to override core.checksumindex and always verify the index checksum when reading the index.
--- builtin/fsck.c | 1 + cache.h | 7 +++++++ read-cache.c | 20 +++++++++++++------- 3 files changed, 21 insertions(+), 7 deletions(-)
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_core_checksum_index =1 ;read_cache ();for (i =0 ; i < active_nr; i++) {unsigned int mode;
cache.h +7 -0
@@ -685,6 +685,13 @@ 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 *); +/* +* Override "core.checksumindex" config settings. Allows commands +* like "fsck" to force it without altering on-disk settings in case +* routines call die() before it can be reset. +*/ +extern int force_core_checksum_index; +/* Environment bits from configuration mechanism */ extern int trust_executable_bit;extern int trust_ctime;
read-cache.c +13 -7
@@ -1371,6 +1371,8 @@ struct ondisk_cache_entry_extended { ondisk_cache_entry_extended_size(ce_namelen(ce)) : \ ondisk_cache_entry_size(ce_namelen(ce))) +int force_core_checksum_index; +static int verify_hdr (struct cache_header *hdr,unsigned long size) {git_SHA_CTX c;
@@ -1384,13 +1386,17 @@ 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); -/* -* Since we run very early in command startup, git_config() -* may not have been called yet and the various "core_*" -* global variables haven't been set. So look it up -* explicitly. -*/ -git_config_get_bool ("core.checksumindex" , &do_checksum); +if (force_core_checksum_index) + do_checksum =1 ; +else { +/* +* Since we run very early in command startup, git_config() +* may not have been called yet and the various "core_*" +* global variables haven't been set. So look it up +* explicitly. +*/ +git_config_get_bool ("core.checksumindex" , &do_checksum); + }if (!do_checksum)return 0 ;
Signature · 1 line
2.9.3
Re: [PATCH v4 2/4] fsck: force core.checksumindex=1
Quoted 2 lines · On Mon, Apr 3, 2017 at 8:53 PM, <[email protected]> wrote:
Teach fsck to override core.checksumindex and always verify the index checksum when reading the index.
Sorry to only chime in about this at v4.
I think this patch & the documentation you added for core.checksumindex in 1/4 would be much less confusing if you made this a on-by-default command-line option like e.g. "full".
With this patch nothing amends the documentation to indicate that the core.checksumindex is magically overridden when fsck runs, I think something like this (needs amending to integrate) on top would make this clearer:
Documentation/git-fsck.txt +1 -1
@@ -12,7 +12,7 @@ SYNOPSIS 'git fsck' [--tags] [--root] [--unreachable] [--cache] [--no-reflogs] [--[no-]full] [--strict] [--verbose] [--lost-found] [--[no-]dangling] [--[no-]progress] [--connectivity-only] - [--[no-]name-objects] [<object>*] + [--[no-]name-objects] [--[no-]checksum-index] [<object>*] DESCRIPTION -----------
@@ -61,6 +61,11 @@ index file, all SHA-1 references in `refs`
namespace, and all reflogs
object pools. This is now default; you can turn it off with --no-full.
+--[no-]checksum-index: + Validate the checksum at the end of the index file, on by + default, locally overrides any "core.checksumIndex" setting + unless negated. See linkgit:git-config[1].
+
--connectivity-only::
Check only the connectivity of tags, commits and tree objects. By avoiding to unpack blobs, this speeds up the operation, at the
builtin/fsck.c +1 -0
@@ -24,6 +24,7 @@ static int show_tags;static int show_unreachable;static int include_reflogs =1 ;static int check_full =1 ; +static int fsck_opt_checksum_index =1 ;static int connectivity_only;static int check_strict;static int keep_cache_objects;
@@ -656,6 +657,8 @@ static struct option fsck_opts[] = { OPT_BOOL(0, "cache", &keep_cache_objects, N_("make index
objects head nodes")),
OPT_BOOL(0, "reflogs", &include_reflogs, N_("make reflogs head
nodes (default)")),
OPT_BOOL(0, "full", &check_full, N_("also consider packs and
alternate objects")),
+ OPT_BOOL(0, "checksum-index", &fsck_opt_checksum_index,
+ N_("validate the checksum at the end of the index file")),
OPT_BOOL(0, "connectivity-only", &connectivity_only, N_("check
only connectivity")),
OPT_BOOL(0, "strict", &check_strict, N_("enable more strict checking")), OPT_BOOL(0, "lost-found", &write_lost_and_found,
@@ -681,6 +684,9 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
if (check_strict)
fsck_obj_options.strict = 1;+ if (fsck_opt_checksum_index)
+ force_core_checksum_index = 1;
+
if (show_progress == -1)
show_progress = isatty(2);
if (verbose)Re: [PATCH v4 2/4] fsck: force core.checksumindex=1
Quoted 6 lines · Ævar Arnfjörð Bjarmason
Quoted 2 lines · On Mon, Apr 3, 2017 at 8:53 PM, <[email protected]> wrote:
Teach fsck to override core.checksumindex and always verify the index checksum when reading the index.
Sorry to only chime in about this at v4.
Quoted 8 lines
I think this patch & the documentation you added for core.checksumindex in 1/4 would be much less confusing if you made this a on-by-default command-line option like e.g. "full".
With this patch nothing amends the documentation to indicate that the core.checksumindex is magically overridden when fsck runs, I think something like this (needs amending to integrate) on top would make this clearer:
I think that is the wrong direction to reduce confusion. We don't need more options to twiddle this flag, we need fewer. For instance, in your proposed documentation:
Review
Quoted 9 lines
(quoted diff) +0
-0@@ -61,6 +61,11 @@ index file, all SHA-1 references in `refs`namespace, and all reflogs
object pools. This is now default; you can turn it off with --no-full.+--[no-]checksum-index:
(quoted diff) +3
-0+ Validate the checksum at the end of the index file, on by + default, locally overrides any "core.checksumIndex" setting + unless negated. See linkgit:git-config[1].
That tells us _what_ it does, but I'm left scratching my head with "why".
I don't think there is any reason you would want fsck not to compute that checksum (just like there is no option to ask fsck not to check pack sha1 trailers).
I would go so far as to say that the config option itself is unnecessary in this iteration of the series. I only asked for it so that we could test the verification code paths (both for correctness and performance). But if fsck can exercise the code path, we can check correctness that way. And for performance, it's probably enough to test two separate builds (which Jeff has already done).
Junio also asked for the usual "add a config, and then later we'll flip the default" procedure. But IMHO that isn't necessary here. Nobody should ever care about this flag. It was largely useless to check it on every read in the first place. And if you suspect there's corruption in your repository, you should run "git fsck".
-Peff
Re: [PATCH v4 2/4] fsck: force core.checksumindex=1
Quoted 45 lines · Jeff King
Quoted 6 lines · Ævar Arnfjörð Bjarmason
Quoted 2 lines · On Mon, Apr 3, 2017 at 8:53 PM, <[email protected]> wrote:
Teach fsck to override core.checksumindex and always verify the index checksum when reading the index.
Sorry to only chime in about this at v4.
Quoted 8 lines
I think this patch & the documentation you added for core.checksumindex in 1/4 would be much less confusing if you made this a on-by-default command-line option like e.g. "full".
With this patch nothing amends the documentation to indicate that the core.checksumindex is magically overridden when fsck runs, I think something like this (needs amending to integrate) on top would make this clearer:
I think that is the wrong direction to reduce confusion. We don't need more options to twiddle this flag, we need fewer. For instance, in your proposed documentation:
Review
Quoted 9 lines
(quoted diff) +0
-0@@ -61,6 +61,11 @@ index file, all SHA-1 references in `refs`namespace, and all reflogs
object pools. This is now default; you can turn it off with --no-full.+--[no-]checksum-index:
(quoted diff) +3
-0+ Validate the checksum at the end of the index file, on by + default, locally overrides any "core.checksumIndex" setting + unless negated. See linkgit:git-config[1].That tells us _what_ it does, but I'm left scratching my head with "why".
I don't think there is any reason you would want fsck not to compute that checksum (just like there is no option to ask fsck not to check pack sha1 trailers).
I would go so far as to say that the config option itself is unnecessary in this iteration of the series. I only asked for it so that we could test the verification code paths (both for correctness and performance). But if fsck can exercise the code path, we can check correctness that way. And for performance, it's probably enough to test two separate builds (which Jeff has already done).
Quoted 5 lines
Junio also asked for the usual "add a config, and then later we'll flip the default" procedure. But IMHO that isn't necessary here. Nobody should ever care about this flag. It was largely useless to check it on every read in the first place. And if you suspect there's corruption in your repository, you should run "git fsck".
The part that confused my & I found unintuitive is that there's a new core.WHATEVER config that'll get silently overridden by a specific command, git-fsck.
Nothing else I can think of in core.* works like this, i.e. it's a namespace for "applies to all of git", core.editor, core.ignoreCase etc.
Having git-fsck have a command-line option that's on by default as I suggested is one way to get out of that confusion. It makes it a special case of a CLI option overriding some config.
But yeah, another way to resolve this is to get rid of the config option altogether, or document in git-config.txt that core.checksumIndex is obeyed by everything except git-fsck.
Re: [PATCH v4 2/4] fsck: force core.checksumindex=1
Quoted 11 lines · Ævar Arnfjörð Bjarmason
The part that confused my & I found unintuitive is that there's a new core.WHATEVER config that'll get silently overridden by a specific command, git-fsck.
Nothing else I can think of in core.* works like this, i.e. it's a namespace for "applies to all of git", core.editor, core.ignoreCase etc.
Having git-fsck have a command-line option that's on by default as I suggested is one way to get out of that confusion. It makes it a special case of a CLI option overriding some config.
Yeah, I do agree your suggestion makes it slightly less confusing.
I mostly just think we can avoid the situation altogether, which IMHO is preferable.
-Peff
Re: [PATCH v4 2/4] fsck: force core.checksumindex=1
In reply to Ævar Arnfjörð BjarmasonQuoted 52 lines · Ævar Arnfjörð Bjarmason
Quoted 45 lines · Jeff King
Quoted 6 lines · Ævar Arnfjörð Bjarmason
Quoted 2 lines · On Mon, Apr 3, 2017 at 8:53 PM, <[email protected]> wrote:
Teach fsck to override core.checksumindex and always verify the index checksum when reading the index.
Sorry to only chime in about this at v4.
Quoted 8 lines
I think this patch & the documentation you added for core.checksumindex in 1/4 would be much less confusing if you made this a on-by-default command-line option like e.g. "full".
With this patch nothing amends the documentation to indicate that the core.checksumindex is magically overridden when fsck runs, I think something like this (needs amending to integrate) on top would make this clearer:
I think that is the wrong direction to reduce confusion. We don't need more options to twiddle this flag, we need fewer. For instance, in your proposed documentation:
Review
Quoted 9 lines
(quoted diff) +0
-0@@ -61,6 +61,11 @@ index file, all SHA-1 references in `refs`namespace, and all reflogs
object pools. This is now default; you can turn it off with --no-full.+--[no-]checksum-index:
(quoted diff) +3
-0+ Validate the checksum at the end of the index file, on by + default, locally overrides any "core.checksumIndex" setting + unless negated. See linkgit:git-config[1].That tells us _what_ it does, but I'm left scratching my head with "why".
I don't think there is any reason you would want fsck not to compute that checksum (just like there is no option to ask fsck not to check pack sha1 trailers).
I would go so far as to say that the config option itself is unnecessary in this iteration of the series. I only asked for it so that we could test the verification code paths (both for correctness and performance). But if fsck can exercise the code path, we can check correctness that way. And for performance, it's probably enough to test two separate builds (which Jeff has already done).
Quoted 5 lines
Junio also asked for the usual "add a config, and then later we'll flip the default" procedure. But IMHO that isn't necessary here. Nobody should ever care about this flag. It was largely useless to check it on every read in the first place. And if you suspect there's corruption in your repository, you should run "git fsck".
Quoted 7 lines
The part that confused my & I found unintuitive is that there's a new core.WHATEVER config that'll get silently overridden by a specific command, git-fsck.
Nothing else I can think of in core.* works like this, i.e. it's a namespace for "applies to all of git", core.editor, core.ignoreCase etc.
My "force_core_checksum_index" global override was a bit of a hack. I looked at having fsck explicitly set the "core.checksumindex" config value (which would write to the disk in the code paths I followed) before it loaded the index, but if we ever find an invalid checksum, read_cache() would call die() and fsck would not have a chance to set it back in the user's config. So I introduced the global override.
Quoted 4 lines
Having git-fsck have a command-line option that's on by default as I suggested is one way to get out of that confusion. It makes it a special case of a CLI option overriding some config.
I looked at doing that, but thought it would be overkill since no one is likely to care about turning it off -- or rather, fsck should always do it whenever it reads the index.
Quoted 5 lines
But yeah, another way to resolve this is to get rid of the config option altogether, or document in git-config.txt that core.checksumIndex is obeyed by everything except git-fsck.
If there's no objections then, I'll just remove the config setting and keep the force_ global for fsck.
Jeff
Re: [PATCH v4 2/4] fsck: force core.checksumindex=1
Quoted 6 lines · Jeff Hostetler
Quoted 3 lines
But yeah, another way to resolve this is to get rid of the config option altogether, or document in git-config.txt that core.checksumIndex is obeyed by everything except git-fsck.
If there's no objections then, I'll just remove the config setting and keep the
force_global for fsck.
That makes the most sense to me. It doesn't fill the "remain careful by default, and we can flip the config later" criterion that Junio set out. But I really just don't think "careful" here is buying us anything.
-Peff
PATCH v4 3/4 t1450-fsck: test core.checksumindex
In reply to [email protected]From: Jeff Hostetler <[email protected]>
Add test for fsck to force verify the index checksum. Add test to demonstrate that status does ignore the checksum when reading the index.
--- t/t1450-fsck.sh | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
t/t1450-fsck.sh +27 -0
@@ -689,4 +689,31 @@ test_expect_success 'bogus head does not fallback to all heads' ' ! grep $blob out ' +test_expect_success 'validate force_core_checksum_index=1' ' + git fsck --cache +' + +test_expect_success PERL 'detect index file corrupt 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_expect_success PERL 'verify status ignores corrupt checksum' ' + cp .git/index .git/index.backup && + echo zzzzzzzz >zzzzzzzz && + git add zzzzzzzz && + perl -pi -e "s/zzzzzzzz/yyyyyyyy/" .git/index && + git -c core.checksumindex=0 status && + # Status may fix the checksum + rm .git/index && + mv .git/index.backup .git/index && + rm zzzzzzzz +' + test_done
Signature · 1 line
2.9.3
PATCH v4 4/4 p0002-read-cache: test core.checksumindex
In reply to [email protected]From: Jeff Hostetler <[email protected]>
Teach t/perf/p0002-read-cache to time read_cache() with and without the index checksum calculation.
--- t/perf/p0002-read-cache.sh | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
t/perf/p0002-read-cache.sh +7 -1
@@ -7,7 +7,13 @@ test_description="Tests performance of reading the index" test_perf_default_repo count=1000 -test_perf "read_cache/discard_cache $count times" " +test_perf "read_cache/discard_cache checksum=1 $count times" " + git config --local core.checksumindex 1 && + test-read-cache $count +" + +test_perf "read_cache/discard_cache checksum=0 $count times" " + git config --local core.checksumindex 0 && test-read-cache $count "
Signature · 1 line
2.9.3