Re: [PATCH v1 0/3] trace2 output for bitmap decision path

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Teng Long <dyroneteng@gmail.com>
Cc: git@vger.kernel.org, me@ttaylorr.com, derrickstolee@github.com,
	tenglong.tl@alibaba-inc.com
Subject: Re: [PATCH v1 0/3] trace2 output for bitmap decision path
Date: Thu, 24 Mar 2022 14:22:17 +0100	[thread overview]
Message-ID: <220324.867d8jo45p.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <cover.1648119652.git.dyroneteng@gmail.com>

On Thu, Mar 24 2022, Teng Long wrote:

> A Git repo can be chosen to use the normal bitmap (before MIDX) and MIDX bitmap.
>
> I recently tried to understand this part of the MIDX implementation because I found
> a bug which has been discovered and repaired in community [1].
>
> I am grateful to Taylor Blau for his help and for introducing me to the testing
> method according to the `git rev-list --test-bitmap <rev>`.
>
> In the process of understanding and troubleshooting by using this command, I found
> when the execution is failed it will output a single line of
> "fatal: failed to load bitmap indexes", sometimes will be more informations like
> if the bitmap file is broken, the outputs maybe contain
> "error: Corrupted bitmap index file (wrong header)".), but most of time are single
> line output I mentioned above. So, this brings a little obstacle for debugging and
> troubleshooting I think, because "failed to load bitmap indexes" can represent
> to much informations (many causes like: midx config turn off, bitmap inexistence, etc.)
>
> Therefore, as a git repo can be chosen to use the normal bitmap (before MIDX) or
> MIDX bitmap, or they can both exist and let git make the decision. I think why not add
> some extra informations based on TRACE2 to help for showing the bitmap decision path
> clearer and more plentiful, so when the failure occurs the user can use it to debug
> around bitmap in a quicker way.
>
> Thanks.
>
> Links:
> 	1. https://public-inbox.org/git/cover.1638991570.git.me@ttaylorr.com/)
>
> Teng Long (3):
>   pack-bitmap.c: use "ret" in "open_midx_bitmap()"
>   pack-bitmap.c: add "break" statement in "open_pack_bitmap()"
>   bitmap: add trace outputs during open "bitmap" file
>
>  midx.c        |  2 ++
>  pack-bitmap.c | 17 +++++++++++++----
>  2 files changed, 15 insertions(+), 4 deletions(-)
>
> Range-diff against v0:
> -:  ---------- > 1:  3048b4dd29 pack-bitmap.c: use "ret" in "open_midx_bitmap()"
> -:  ---------- > 2:  70500b6343 pack-bitmap.c: add "break" statement in "open_pack_bitmap()"
> -:  ---------- > 3:  9912450fc1 bitmap: add trace outputs during open "bitmap" file

Was there an on-list v0 (RFC?) or is this a range-diff against nothing?
Best not to include it until a v2 then.

Comments:

Sometimes it's better to split up patches, but I think these 3x should
really be squashed together. We make incremental progress to nowhere in
1/3 and 2/3, and it all comes together in 3/3. The 1-2/3 are trivial
enough that we can squash them in.

We then end up with this, with my comments added:

	 midx.c        |  2 ++
	 pack-bitmap.c | 17 +++++++++++++----
	 2 files changed, 15 insertions(+), 4 deletions(-)

	diff --git a/midx.c b/midx.c
	index 865170bad05..fda96440287 100644
	--- a/midx.c
	+++ b/midx.c
	@@ -392,6 +392,8 @@ int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, i
	 	struct multi_pack_index *m_search;

	 	prepare_repo_settings(r);
	+	trace2_data_string("midx", r, "core.multipackIndex",
	+					   r->settings.core_multi_pack_index ? "true" : "false");

Weird indentation here.

Also, if we think it's a good idea to log these shouldn't it be in
repo_cfg_bool() in repo-settings.c, why is core.multipackIndex out of
all in r->settings special?

	 	if (!r->settings.core_multi_pack_index)
	 		return 0;

	diff --git a/pack-bitmap.c b/pack-bitmap.c
	index 97909d48da3..cac8d4a978f 100644
	--- a/pack-bitmap.c
	+++ b/pack-bitmap.c
	@@ -484,25 +484,34 @@ static int open_pack_bitmap(struct repository *r,
	 	assert(!bitmap_git->map);

	 	for (p = get_all_packs(r); p; p = p->next) {
	-		if (open_pack_bitmap_1(bitmap_git, p) == 0)
	+		if (open_pack_bitmap_1(bitmap_git, p) == 0) {

Aside: If we end up changing this line anyway, it's OK to just change it
to "if (!open...".

	 			ret = 0;
	+			break;
	+		}
	 	}

	+	trace2_data_string("bitmap", the_repository, "open bitmap (non-midx)",
	+					   ret ? "failed" : "ok");
	 	return ret;
	 }

	 static int open_midx_bitmap(struct repository *r,
	 			    struct bitmap_index *bitmap_git)
	 {
	+	int ret = -1;
	 	struct multi_pack_index *midx;

	 	assert(!bitmap_git->map);

	 	for (midx = get_multi_pack_index(r); midx; midx = midx->next) {
	-		if (!open_midx_bitmap_1(bitmap_git, midx))
	-			return 0;
	+		if (!open_midx_bitmap_1(bitmap_git, midx)) {
	+			ret = 0;
	+			break;
	+		}
	 	}
	-	return -1;
	+	trace2_data_string("midx", the_repository, "open bitmap (midx)",
	+					   ret ? "failed" : "ok");
	+	return ret;
	 }

	 static int open_bitmap(struct repository *r,

It seems odd not to use trace2 regions for this, and to not add add this
data logging open_bitmap(). I came up with this on top of this when
testing this:

	diff --git a/pack-bitmap.c b/pack-bitmap.c
	index cac8d4a978f..ba71a7ea5cd 100644
	--- a/pack-bitmap.c
	+++ b/pack-bitmap.c
	@@ -318,11 +318,14 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,

	 	free(idx_name);

	-	if (fd < 0)
	+	if (fd < 0) {
	+		/* TODO: Log trace2_data_string() here, do we care? */
	 		return -1;
	+	}

	 	if (fstat(fd, &st)) {
	 		close(fd);
	+		/* TODO: Log trace2_data_string() here, do we care? */
	 		return -1;
	 	}

	@@ -330,6 +333,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
	 		struct strbuf buf = STRBUF_INIT;
	 		get_midx_filename(&buf, midx->object_dir);
	 		/* ignore extra bitmap file; we can only handle one */
	+		/* NOTE: You'll already get a warning (well, "error") event due to this, and it'll be in your region */
	 		warning("ignoring extra bitmap file: %s", buf.buf);
	 		close(fd);
	 		strbuf_release(&buf);
	@@ -344,9 +348,11 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
	 	close(fd);

	 	if (load_bitmap_header(bitmap_git) < 0)
	+		/* TODO: Add trace2_data_string() or warning/error here? */
	 		goto cleanup;

	 	if (!hasheq(get_midx_checksum(bitmap_git->midx), bitmap_git->checksum))
	+		/* TODO: Add trace2_data_string() or warning/error here? */
	 		goto cleanup;

	 	if (load_midx_revindex(bitmap_git->midx) < 0) {
	@@ -479,49 +485,44 @@ static int open_pack_bitmap(struct repository *r,
	 			    struct bitmap_index *bitmap_git)
	 {
	 	struct packed_git *p;
	-	int ret = -1;
	-
	-	assert(!bitmap_git->map);

	 	for (p = get_all_packs(r); p; p = p->next) {
	 		if (open_pack_bitmap_1(bitmap_git, p) == 0) {
	-			ret = 0;
	-			break;
	+			return 0;
	 		}
	 	}
	-
	-	trace2_data_string("bitmap", the_repository, "open bitmap (non-midx)",
	-					   ret ? "failed" : "ok");
	-	return ret;
	+	return -1;
	 }

	 static int open_midx_bitmap(struct repository *r,
	 			    struct bitmap_index *bitmap_git)
	 {
	-	int ret = -1;
	 	struct multi_pack_index *midx;

	-	assert(!bitmap_git->map);
	-
	 	for (midx = get_multi_pack_index(r); midx; midx = midx->next) {
	 		if (!open_midx_bitmap_1(bitmap_git, midx)) {
	-			ret = 0;
	-			break;
	+			return 0;
	 		}
	 	}
	-	trace2_data_string("midx", the_repository, "open bitmap (midx)",
	-					   ret ? "failed" : "ok");
	-	return ret;
	+	return -1;
	 }

	 static int open_bitmap(struct repository *r,
	 		       struct bitmap_index *bitmap_git)
	 {
	+	int ret;
	+
	 	assert(!bitmap_git->map);

	-	if (!open_midx_bitmap(r, bitmap_git))
	-		return 0;
	-	return open_pack_bitmap(r, bitmap_git);
	+	trace2_region_enter("pack-bitmap", "open_bitmap", r);
	+	if (!open_midx_bitmap(r, bitmap_git)) {
	+		ret = 0;
	+		goto done;
	+	}
	+	ret = open_pack_bitmap(r, bitmap_git);
	+done:
	+	trace2_region_leave("pack-bitmap", "open_bitmap", r);
	+	return ret;
	 }

	 struct bitmap_index *prepare_bitmap_git(struct repository *r)

I.e. surely you just want to create a region, and if you care enough to
log failure shouldn't we log that in open_midx_bitmap_1() if we care,
and perhaps error() there instead of silently returning -1?