patchbuiltin/repack.c: remove redundant pack-based bitmaps

When we write a MIDX bitmap after repacking, it is possible that the repository would be left in a state with both pack- and multi-pack reachability bitmaps.

This can occur, for instance, if a pack that was kept (either by having a .keep file, or during a geometric repack in which it is not rolled up) has a bitmap file, and the repack wrote a multi-pack index and bitmap.

When loading a reachability bitmap for the repository, the multi-pack one is always preferred, so the pack-based one is redundant. Let's remove it unconditionally, even if '-d' isn't passed, since there is no practical reason to keep both around. The patch below does just that.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/repack.c  | 40 +++++++++++++++++++++++++++++++++++++---
 t/t7700-repack.sh | 21 +++++++++++++++++++++
 2 files changed, 58 insertions(+), 3 deletions(-)

On Wed, Oct 12, 2022 at 03:05:33PM -0400, Taylor Blau wrote:

Yeah, this is certainly a reasonable thing to be doing. I wonder if you want to share the story of why the original midx-bitmap series did not include this patch. It is (IMHO, at least) an interesting debugging tale.

The patch looks good. Two very minor comments, though I'd be happy enough to see it merged as-is:

We could downgrade this to a warning, since there is no downside to retaining those files (aside from wasted space). In remove_redundant_pack(), we call into unlink_pack_path(), which just ignores unlink errors (though arguably it should at least warn).

You could avoid having to repeat the string-list cleanup here by structuring it like:

  if (!ret && write_bitmaps)
	remove_redundant_bitmaps(&include, packdir);

  /* as before, clear string list and possibly return ret */

Since it's only one line, it's not that big a deal, but it simplifies the flow.

It's correct either way, of course. One thing I did have to do while reviewing this was look at this hunk in place. The context omits that this is in the "if (write_midx)" conditional, which is of course very important. ;)

-Peff

On Mon, Oct 17 2022, Jeff King wrote:

An ENOENT would indicate a race with a concurrent process.

That in itself seems like a wart of our lock management. I.e. we invoked "multi-pack-index" earlier and took a lock to create the midx.

I don't know how exactly that interact with the pack bitmaps, but should we ideally have taken locks on the relevant associated files instead during the larger operation?

But anyway, that's a much larger topic, so we could sweep it under the rug here.

But at that point should we be ignoring other unlink() errors? Maybe we should additionally be ignoring EBUSY.

But the rest all seem from a quick scan of unlink(2) to be things we'd like to just die on, e.g. EIO.

On Mon, Oct 17, 2022 at 08:50:07PM +0200, Ævar Arnfjörð Bjarmason wrote:

In the code above, I don't think it is. We included some packs in the midx, and we know that their bitmaps (if any) are now redundant. So we remove them by calling unlink() speculatively, rather than asking "do we have this file? If so, unlink".

Likewise in remove_redundant_pack(). We are not just removing the packfiles, but speculatively removing their .rev, .bitmap, .promisor, etc. If they don't exist, that's perfectly normal. Arguably we could notice and complain if the .pack or .idx went away in the interim, since those would indicate a race.

So I think the code above is right to ignore ENOENT. The one in remove_redundant_pack() does so, too, but it also ignores everything else.

For most errors besides ENOENT, I think a warning is the right thing. It is OK that we failed to remove the file (it does not make the overall operation a failure). But it may be helpful for the user to let them know that the cruft was left (e.g., for EPERM or EIO). And that applies here and in remove_redundant_pack().

Really, both here and remove_redundant_pack() should probably be using unlink_or_warn(). Though if you did want to notice the race on deleting the actual .pack file, it would need to be hand-coded.

-Peff

Jeff King <peff@peff.net> writes:

Yeah, unlink_or_warn() would probably be the right way to go.

Other than that, the patch looks good to you folks?

Thanks.

On Mon, Oct 17, 2022 at 02:04:13PM -0700, Junio C Hamano wrote:

Yeah. I suggested a minor reordering of the one change, but the substance of the patch looks good to me (and I am OK either way on the little suggestions).

-Peff

On Mon, Oct 17, 2022 at 02:02:53PM -0400, Jeff King wrote:

Sure, though note that this is a mystery that plague me on-and-off for a year or so before finally figuring it out ;-).

When we initially deployed MIDX bitmaps at GitHub, I held back this patch as a recovery mechanism (ie., if something was horribly broken with MIDX bitmaps, we could quickly deploy a patch to stop reading them, knowing that the existing pack bitmaps were still around).

After a while building up confidence in the MIDX bitmaps implementation, I wrote something that resembles the patch here and deployed it. No big deal, right?

Wrong. The "total time spent running `git repack`" metrics started ticking up. But the metrics ticked up even in hosts and sites that had not yet received the patch. Huh?

This led to many of the trace2 regions in midx.c that I recently[1, 2] sent. But after deploying that patch and reverting it over and over again, nothing in the trace2 metrics ended up yielding a satisfying answer.

I spent a few days trying to debug the issue but didn't make any progress. I confirmed that, yes, we were indeed reading MIDX bitmaps and not relying on the pack-based bitmaps for some weird reason. And after I confirmed that, I set the bug aside to work on other things, and mostly forgot about it.

But in an unrelated incident, a colleague of mine noted a bug in a GitHub-internal program called `git-get-unpacked-size`. The output of this program is the number of bytes in non-bitmapped packs, and it's used as a heuristic to help us pick repository networks to repack roughly in order of "most non-repacked bytes".

The bug in `get-unpacked-size` was that we were including the size of the cruft pack, meaning that certain networks would get scheduled much more often. But in fixing that bug, I noticed that packs in the MIDX would also get counted against our unpacked size.

So I wrote the minimal fix to fix the cruft pack bug, and noted the MIDX issue for another day. When we did come back to it (I say "we", since Victoria Dye implemented the actual fix), I suggested that we ignore any packs that appear in a MIDX from the unpacked size, since we only write a MIDX after repacking a repository.

When we did so, I suspected that it might inadvertently solve the original mystery. It did, and here's why.

Individual maintenance runs did not get slower when removing redundant pack bitmaps. But because removing the .bitmap file causes that pack to be counted towards a repository's total unpacked size, it biases scheduling towards larger repositories (which have bigger packs, and therefore a more inflated overall size). So we weren't slowing anything down, just doing a slightly more expensive maintenance (on bigger repositories) slightly more often. This explained not only the uptick, but why the effect was so resistant to measuring.

It also explained why we saw the same effect when removing pack bitmaps with MIDX bitmaps deployed to a single site only: whenever `git get-unpacked-size` ran on a replica that *did* have the new code, the unpacked-size count would get inflated. That explains why we saw maintenance times tick up outside of our testing site.

So, that's the story :-).

[1]: https://lore.kernel.org/git/dc50527d99680fb0ff1f3240531105badaa221dc.1665612094.git.me@ttaylorr.com/ [2]: https://lore.kernel.org/git/dd5ceb1ec3c01e8a2af55130718fff0b5eaf2de0.1665612094.git.me@ttaylorr.com/

Thanks, Taylor

On Mon, Oct 17, 2022 at 02:02:53PM -0400, Jeff King wrote:

I think that downgrading this to `warning_errno()` is appropriate. I'll make that change locally and send a new version.

I think a good separate topic is teaching `remove_redundant_pack()` to emit warnings for non-ENOENT errors, too. But I'll leave that for another day :-).

Great suggestion, thanks. I'll apply it locally and send a reroll now.

Thanks, Taylor

When we write a MIDX bitmap after repacking, it is possible that the repository would be left in a state with both pack- and multi-pack reachability bitmaps.

This can occur, for instance, if a pack that was kept (either by having a .keep file, or during a geometric repack in which it is not rolled up) has a bitmap file, and the repack wrote a multi-pack index and bitmap.

When loading a reachability bitmap for the repository, the multi-pack one is always preferred, so the pack-based one is redundant. Let's remove it unconditionally, even if '-d' isn't passed, since there is no practical reason to keep both around. The patch below does just that.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
A small reroll to address a pair of comments from Peff.

Range-diff against v1:
1:  393fd4c6db ! 1:  1e0ef7ee7f builtin/repack.c: remove redundant pack-based bitmaps
    @@ builtin/repack.c: static int write_midx_included_packs(struct string_list *inclu
     +		strbuf_addstr(&path, ".bitmap");
     +
     +		if (unlink(path.buf) && errno != ENOENT)
    -+			die_errno(_("could not remove stale bitmap: %s"),
    -+				  path.buf);
    ++			warning_errno(_("could not remove stale bitmap: %s"),
    ++				      path.buf);
     +
     +		strbuf_setlen(&path, packdir_len);
     +	}
    @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
      						refs_snapshot ? get_tempfile_path(refs_snapshot) : NULL,
      						show_progress, write_bitmaps > 0);

    -+		if (ret) {
    -+			string_list_clear(&include, 0);
    -+			return ret;
    -+		}
    -+
    -+		if (write_bitmaps)
    ++		if (!ret && write_bitmaps)
     +			remove_redundant_bitmaps(&include, packdir);
     +
      		string_list_clear(&include, 0);
    --
    --		if (ret)
    --			return ret;
    - 	}

    - 	reprepare_packed_git(the_repository);
    + 		if (ret)

      ## t/t7700-repack.sh ##
     @@ t/t7700-repack.sh: test_expect_success '--write-midx -b packs non-kept objects' '

 builtin/repack.c  | 32 ++++++++++++++++++++++++++++++++
 t/t7700-repack.sh | 21 +++++++++++++++++++++
 2 files changed, 53 insertions(+)

On Mon, Oct 17, 2022 at 10:45:12PM -0400, Taylor Blau wrote:

Thanks, this version looks great to me.

As noted elsewhere in the thread, this could be unlink_or_warn(), but doing it manually lets us produce a more specific message, which is nice.

-Peff