[05/11] maintenance: add commit-graph task

Commit Message

Philippe Blain via GitGitGadget Aug. 6, 2020, 3:48 p.m. UTC

From: Derrick Stolee <[e-mail]>

The first new task in the 'git maintenance' builtin is the
'commit-graph' job. It is based on the sequence of events in the
'commit-graph' job in Scalar [1]. This sequence is as follows:

1. git commit-graph write --reachable --split
2. git commit-graph verify --shallow
3. If the verify succeeds, stop.
4. Delete the commit-graph-chain file.
5. git commit-graph write --reachable --split

By writing an incremental commit-graph file using the "--split"
option we minimize the disruption from this operation. The default
behavior is to merge layers until the new "top" layer is less than
half the size of the layer below. This provides quick writes most
of the time, with the longer writes following a power law
distribution.

Most importantly, concurrent Git processes only look at the
commit-graph-chain file for a very short amount of time, so they
will verly likely not be holding a handle to the file when we try
to replace it. (This only matters on Windows.)

If a concurrent process reads the old commit-graph-chain file, but
our job expires some of the .graph files before they can be read,
then those processes will see a warning message (but not fail).
This could be avoided by a future update to use the --expire-time
argument when writing the commit-graph.

By using 'git commit-graph verify --shallow' we can ensure that
the file we just wrote is valid. This is an extra safety precaution
that is faster than our 'write' subcommand. In the rare situation
that the newest layer of the commit-graph is corrupt, we can "fix"
the corruption by deleting the commit-graph-chain file and rewrite
the full commit-graph as a new one-layer commit graph. This does
not completely prevent _that_ file from being corrupt, but it does
recompute the commit-graph by parsing commits from the object
database. In our use of this step in Scalar and VFS for Git, we
have only seen this issue arise because our microsoft/git fork
reverted 43d3561 ("commit-graph write: don't die if the existing
graph is corrupt" 2019-03-25) for a while to keep commit-graph
writes very fast. We dropped the revert when updating to v2.23.0.
The verify still has potential for catching corrupt data across
the layer boundary: if the new file has commit X with parent Y
in an old file but the commit ID for Y in the old file had a
bitswap, then we will notice that in the 'verify' command.

[1] https://github.com/microsoft/scalar/blob/master/Scalar.Common/Maintenance/CommitGraphStep.cs

Signed-off-by: Derrick Stolee <[e-mail]>
---
 Documentation/git-maintenance.txt | 18 +++++++++
 builtin/gc.c                      | 63 +++++++++++++++++++++++++++++++
 commit-graph.c                    |  8 ++--
 commit-graph.h                    |  1 +
 t/t7900-maintenance.sh            |  2 +
 5 files changed, 88 insertions(+), 4 deletions(-)

Comments

Martin Ågren Aug. 7, 2020, 10:29 p.m. UTC | #1

On Thu, 6 Aug 2020 at 18:50, Derrick Stolee via GitGitGadget
<[e-mail]> wrote:
> diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
> index 089fa4cedc..35b0be7d40 100644
> --- a/Documentation/git-maintenance.txt
> +++ b/Documentation/git-maintenance.txt
> @@ -35,6 +35,24 @@ run::
>  TASKS
>  -----
>
> +commit-graph::
> +       The `commit-graph` job updates the `commit-graph` files incrementally,
> +       then verifies that the written data is correct. If the new layer has an
> +       issue, then the chain file is removed and the `commit-graph` is
> +       rewritten from scratch.
> ++
> +The verification only checks the top layer of the `commit-graph` chain.
> +If the incremental write merged the new commits with at least one
> +existing layer, then there is potential for on-disk corruption being
> +carried forward into the new file. This will be noticed and the new
> +commit-graph file will be clean as Git reparses the commit data from
> +the object database.

This reads somewhat awkwardly. I think what you mean is something like
"is there a risk for on-disk corruption? yes, but no: we're clever
enough to detect it and avoid it". So from a user's point of view, I
think this is too detailed.

How about

 The verification checks the top layer of the resulting `commit-graph` chain.
 This ensures that the maintenance task leaves the top layer in a shape
 that matches the object database, even if it was ostensibly constructed
 by "merging in" existing, incorrect layers.

? I don't quite like it -- it's a bit too technical -- but it describes
the end result which the user should care about -- on-disk data is
consistent and correct -- rather than how we got there.

Perhaps:

 It is ensured that the resulting "top layer" is correct. This should
 help avoid most on-disk corruption of the commit-graph and ensure
 that the commit-graph matches the object database.

Don't know whether that's entirely true though...

Food for thought, perhaps.

There's something probabilistic about this whole thing: If a low layer
is corrupt, you might "eventually" get to replace it. I suppose it could
make sense to go "verify the whole thing, drop however many top layers
we need to drop to get only correct layers, then generate a new layer
(and merge and whatnot) on top of that". But the proposed commit message
makes it fairly clear that this would have other drawbacks and that we
don't really expect corrupt layers in the first place.

Martin

Derrick Stolee Aug. 12, 2020, 1:30 p.m. UTC | #2

On 8/7/2020 6:29 PM, Martin Ågren wrote:
> On Thu, 6 Aug 2020 at 18:50, Derrick Stolee via GitGitGadget
> <[e-mail]> wrote:
>> diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
>> index 089fa4cedc..35b0be7d40 100644
>> --- a/Documentation/git-maintenance.txt
>> +++ b/Documentation/git-maintenance.txt
>> @@ -35,6 +35,24 @@ run::
>>  TASKS
>>  -----
>>
>> +commit-graph::
>> +       The `commit-graph` job updates the `commit-graph` files incrementally,
>> +       then verifies that the written data is correct. If the new layer has an
>> +       issue, then the chain file is removed and the `commit-graph` is
>> +       rewritten from scratch.
>> ++
>> +The verification only checks the top layer of the `commit-graph` chain.
>> +If the incremental write merged the new commits with at least one
>> +existing layer, then there is potential for on-disk corruption being
>> +carried forward into the new file. This will be noticed and the new
>> +commit-graph file will be clean as Git reparses the commit data from
>> +the object database.
> 
> This reads somewhat awkwardly. I think what you mean is something like
> "is there a risk for on-disk corruption? yes, but no: we're clever
> enough to detect it and avoid it". So from a user's point of view, I
> think this is too detailed.
> 
> How about
> 
>  The verification checks the top layer of the resulting `commit-graph` chain.
>  This ensures that the maintenance task leaves the top layer in a shape
>  that matches the object database, even if it was ostensibly constructed
>  by "merging in" existing, incorrect layers.
> 
> ? I don't quite like it -- it's a bit too technical -- but it describes
> the end result which the user should care about -- on-disk data is
> consistent and correct -- rather than how we got there.
> 
> Perhaps:
> 
>  It is ensured that the resulting "top layer" is correct. This should
>  help avoid most on-disk corruption of the commit-graph and ensure
>  that the commit-graph matches the object database.
> 
> Don't know whether that's entirely true though...

This is my understanding. We focus on the data we just wrote to see
if anything went wrong at a lower level (i.e. filesystem or RAM
corruption during the write process) instead of the data "at rest".

> Food for thought, perhaps.
> 
> There's something probabilistic about this whole thing: If a low layer
> is corrupt, you might "eventually" get to replace it. I suppose it could
> make sense to go "verify the whole thing, drop however many top layers
> we need to drop to get only correct layers, then generate a new layer
> (and merge and whatnot) on top of that". But the proposed commit message
> makes it fairly clear that this would have other drawbacks and that we
> don't really expect corrupt layers in the first place.

Right. And we want to keep the amount of work to be very small in most
cases, amortizing the cost of the big merge operations across many much
smaller runs. Perhaps a later change could introduce an option to drop
the '--shallow' option and check the entire chain, for users willing to
pay that price.

Back to the point of your comments: I'm not sure this second paragraph
is required at all in the documentation. The first paragraph already
says:

	...then verifies that the written data is correct.

This "written data" _is_ the top layer of the chain. There is probably
no reason to dig deeper into _why_ we do this in this user-facing
documentation.

So, I propose just deleting this paragraph. What do you think?

Thanks for keeping a close eye on documentation changes! I updated
my local branch to include the '+' problem from your other reply.

Thanks,
-Stolee

Martin Ågren Aug. 14, 2020, 12:23 p.m. UTC | #3

On Wed, 12 Aug 2020 at 15:30, Derrick Stolee <[e-mail]> wrote:
>
> On 8/7/2020 6:29 PM, Martin Ågren wrote:
> > On Thu, 6 Aug 2020 at 18:50, Derrick Stolee via GitGitGadget
> > <[e-mail]> wrote:
> >> diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
> >> index 089fa4cedc..35b0be7d40 100644
> >> --- a/Documentation/git-maintenance.txt
> >> +++ b/Documentation/git-maintenance.txt
> >> @@ -35,6 +35,24 @@ run::
> >>  TASKS
> >>  -----
> >>
> >> +commit-graph::
> >> +       The `commit-graph` job updates the `commit-graph` files incrementally,
> >> +       then verifies that the written data is correct. If the new layer has an
> >> +       issue, then the chain file is removed and the `commit-graph` is
> >> +       rewritten from scratch.
> >> ++
> >> +The verification only checks the top layer of the `commit-graph` chain.
> >> +If the incremental write merged the new commits with at least one
> >> +existing layer, then there is potential for on-disk corruption being
> >> +carried forward into the new file. This will be noticed and the new
> >> +commit-graph file will be clean as Git reparses the commit data from
> >> +the object database.
> >
> > This reads somewhat awkwardly. I think what you mean is something like
> > "is there a risk for on-disk corruption? yes, but no: we're clever
> > enough to detect it and avoid it". So from a user's point of view, I
> > think this is too detailed.

[snip quite a bit]

> Back to the point of your comments: I'm not sure this second paragraph
> is required at all in the documentation. The first paragraph already
> says:
>
>         ...then verifies that the written data is correct.
>
> This "written data" _is_ the top layer of the chain. There is probably
> no reason to dig deeper into _why_ we do this in this user-facing
> documentation.
>
> So, I propose just deleting this paragraph. What do you think?

Yeah, that makes lots of sense. Thanks!


Martin

Patch

diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
index 089fa4cedc..35b0be7d40 100644
--- a/Documentation/git-maintenance.txt
+++ b/Documentation/git-maintenance.txt
@@ -35,6 +35,24 @@  run::
 TASKS
 -----
 
+commit-graph::
+	The `commit-graph` job updates the `commit-graph` files incrementally,
+	then verifies that the written data is correct. If the new layer has an
+	issue, then the chain file is removed and the `commit-graph` is
+	rewritten from scratch.
++
+The verification only checks the top layer of the `commit-graph` chain.
+If the incremental write merged the new commits with at least one
+existing layer, then there is potential for on-disk corruption being
+carried forward into the new file. This will be noticed and the new
+commit-graph file will be clean as Git reparses the commit data from
+the object database.
++
+The incremental write is safe to run alongside concurrent Git processes
+since it will not expire `.graph` files that were in the previous
+`commit-graph-chain` file. They will be deleted by a later run based on
+the expiration delay.
+
 gc::
 	Cleanup unnecessary files and optimize the local repository. "GC"
 	stands for "garbage collection," but this task performs many
diff --git a/builtin/gc.c b/builtin/gc.c
index 150dce4301..3b7b914d60 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -710,6 +710,64 @@  struct maintenance_opts {
 	int quiet;
 };
 
+static int run_write_commit_graph(struct maintenance_opts *opts)
+{
+	struct child_process child = CHILD_PROCESS_INIT;
+
+	child.git_cmd = 1;
+	strvec_pushl(&child.args, "commit-graph", "write",
+		     "--split", "--reachable", NULL);
+
+	if (opts->quiet)
+		strvec_push(&child.args, "--no-progress");
+
+	return !!run_command(&child);
+}
+
+static int run_verify_commit_graph(struct maintenance_opts *opts)
+{
+	struct child_process child = CHILD_PROCESS_INIT;
+
+	child.git_cmd = 1;
+	strvec_pushl(&child.args, "commit-graph", "verify",
+		     "--shallow", NULL);
+
+	if (opts->quiet)
+		strvec_push(&child.args, "--no-progress");
+
+	return !!run_command(&child);
+}
+
+static int maintenance_task_commit_graph(struct maintenance_opts *opts)
+{
+	struct repository *r = the_repository;
+	char *chain_path;
+
+	close_object_store(r->objects);
+	if (run_write_commit_graph(opts)) {
+		error(_("failed to write commit-graph"));
+		return 1;
+	}
+
+	if (!run_verify_commit_graph(opts))
+		return 0;
+
+	warning(_("commit-graph verify caught error, rewriting"));
+
+	chain_path = get_commit_graph_chain_filename(r->objects->odb);
+	if (unlink(chain_path)) {
+		UNLEAK(chain_path);
+		die(_("failed to remove commit-graph at %s"), chain_path);
+	}
+	free(chain_path);
+
+	if (!run_write_commit_graph(opts))
+		return 0;
+
+	error(_("failed to rewrite commit-graph"));
+	return 1;
+}
+
 static int maintenance_task_gc(struct maintenance_opts *opts)
 {
 	struct child_process child = CHILD_PROCESS_INIT;
@@ -738,6 +796,7 @@  struct maintenance_task {
 
 enum maintenance_task_label {
 	TASK_GC,
+	TASK_COMMIT_GRAPH,
 
 	/* Leave as final value */
 	TASK__COUNT
@@ -749,6 +808,10 @@  static struct maintenance_task tasks[] = {
 		maintenance_task_gc,
 		1,
 	},
+	[TASK_COMMIT_GRAPH] = {
+		"commit-graph",
+		maintenance_task_commit_graph,
+	},
 };
 
 static int maintenance_run(struct maintenance_opts *opts)
diff --git a/commit-graph.c b/commit-graph.c
index 1af68c297d..9705d237e4 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -172,7 +172,7 @@  static char *get_split_graph_filename(struct object_directory *odb,
 		       oid_hex);
 }
 
-static char *get_chain_filename(struct object_directory *odb)
+char *get_commit_graph_chain_filename(struct object_directory *odb)
 {
 	return xstrfmt("%s/info/commit-graphs/commit-graph-chain", odb->path);
 }
@@ -521,7 +521,7 @@  static struct commit_graph *load_commit_graph_chain(struct repository *r,
 	struct stat st;
 	struct object_id *oids;
 	int i = 0, valid = 1, count;
-	char *chain_name = get_chain_filename(odb);
+	char *chain_name = get_commit_graph_chain_filename(odb);
 	FILE *fp;
 	int stat_res;
 
@@ -1619,7 +1619,7 @@  static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 	}
 
 	if (ctx->split) {
-		char *lock_name = get_chain_filename(ctx->odb);
+		char *lock_name = get_commit_graph_chain_filename(ctx->odb);
 
 		hold_lock_file_for_update_mode(&lk, lock_name,
 					       LOCK_DIE_ON_ERROR, 0444);
@@ -1996,7 +1996,7 @@  static void expire_commit_graphs(struct write_commit_graph_context *ctx)
 	if (ctx->split_opts && ctx->split_opts->expire_time)
 		expire_time = ctx->split_opts->expire_time;
 	if (!ctx->split) {
-		char *chain_file_name = get_chain_filename(ctx->odb);
+		char *chain_file_name = get_commit_graph_chain_filename(ctx->odb);
 		unlink(chain_file_name);
 		free(chain_file_name);
 		ctx->num_commit_graphs_after = 0;
diff --git a/commit-graph.h b/commit-graph.h
index 28f89cdf3e..3c202748c3 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -25,6 +25,7 @@  struct commit;
 struct bloom_filter_settings;
 
 char *get_commit_graph_filename(struct object_directory *odb);
+char *get_commit_graph_chain_filename(struct object_directory *odb);
 int open_commit_graph(const char *graph_file, int *fd, struct stat *st);
 
 /*
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 7b63b4ec0c..384294d111 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -4,6 +4,8 @@  test_description='git maintenance builtin'
 
 . ./test-lib.sh
 
+GIT_TEST_COMMIT_GRAPH=0
+
 test_expect_success 'help text' '
 	test_expect_code 129 git maintenance -h 2>err &&
 	test_i18ngrep "usage: git maintenance run" err