Commit 44f9fd6496 (pack-bitmap.c: check preferred pack validity when
opening MIDX bitmap, 2022-05-24) prevents a race condition whereby the
preferred pack disappears between opening the MIDX bitmap and attempting
verbatim reuse out of its packs.
That commit forces open_midx_bitmap_1() to ensure the validity of the
MIDX's preferred pack, meaning that we have an open file handle on the
*.pack, ensuring that we can reuse bytes out of verbatim later on in the
process[^1].
But 44f9fd6496 was not extended to cover multi-pack reuse, meaning that
this same race condition exists for non-preferred packs during verbatim
reuse. Work around that race in the same way by only marking valid packs
as reuse-able. For packs that aren't reusable, skip over them but
include the number of objects they have to ensure we allocate a large
enough 'reuse' bitmap (e.g. if a pack in the middle of the MIDX
disappeared but we still want to reuse later packs).
Since we're ensuring the validity of these packs within the verbatim
reuse code, we no longer have to special-case the preferred pack and
open it within the open_midx_bitmap_1() function.
An alternative approach to the one taken here would be to open all
MIDX'd packs from within open_midx_bitmap_1(). But that would be both
slower and make the bitmaps less useful, since we can still perform some
pack reuse among the packs that still exist when the *.bitmap is opened.
After applying this patch, we can simulate the new behavior after
instrumenting Git like so:
diff --git a/packfile.c b/packfile.c
index 9560f0a33c..aedce72524 100644
--- a/packfile.c
+++ b/packfile.c
@@ -557,6 +557,11 @@ static int open_packed_git_1(struct packed_git *p)
; /* nothing */
p->pack_fd = git_open(p->pack_name);
+ {
+ const char *delete = getenv("GIT_RACILY_DELETE");
+ if (delete && !strcmp(delete, pack_basename(p)))
+ return -1;
+ }
if (p->pack_fd < 0 || fstat(p->pack_fd, &st))
return -1;
pack_open_fds++;
and adding the following test:
test_expect_success 'disappearing packs' '
git init disappearing-packs &&
(
cd disappearing-packs &&
git config pack.allowPackReuse multi &&
test_commit A &&
test_commit B &&
test_commit C &&
A="$(echo "A" | git pack-objects --revs $packdir/pack-A)" &&
B="$(echo "A..B" | git pack-objects --revs $packdir/pack-B)" &&
C="$(echo "B..C" | git pack-objects --revs $packdir/pack-C)" &&
git multi-pack-index write --bitmap --preferred-pack=pack-A-$A.idx &&
test_pack_objects_reused_all 9 3 &&
test_env GIT_RACILY_DELETE=pack-A-$A.pack \
test_pack_objects_reused_all 6 2 &&
test_env GIT_RACILY_DELETE=pack-B-$B.pack \
test_pack_objects_reused_all 6 2 &&
test_env GIT_RACILY_DELETE=pack-C-$C.pack \
test_pack_objects_reused_all 6 2
)
'
Note that we could relax the single-pack version of this which was most
recently addressed in dc1daacdcc (pack-bitmap: check pack validity when
opening bitmap, 2021-07-23), but only partially. Because we still need
to know the object count in the pack, we'd still have to open the pack's
*.idx, so the savings there are marginal.
Note likewise that we add a new "if (!packs_nr)" early return in the
pack reuse code to avoid a potentially expensive allocation on the
'reuse' bitmap in the case that no packs are available for reuse.
[^1]: Unless we run out of open file handles. If that happens and we are
forced to close the only open file handle of a file that has been
removed from underneath us, there is nothing we can do.
Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx>
---
pack-bitmap.c | 41 ++++++++++++++++++-----------------------
1 file changed, 18 insertions(+), 23 deletions(-)
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 683f467051..ff09b15eb7 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -389,8 +389,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
struct stat st;
char *bitmap_name = midx_bitmap_filename(midx);
int fd = git_open(bitmap_name);
- uint32_t i, preferred_pack;
- struct packed_git *preferred;
+ uint32_t i;
if (fd < 0) {
if (errno != ENOENT)
@@ -445,18 +444,6 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
}
}
- if (midx_preferred_pack(bitmap_git->midx, &preferred_pack) < 0) {
- warning(_("could not determine MIDX preferred pack"));
- goto cleanup;
- }
-
- preferred = bitmap_git->midx->packs[preferred_pack];
- if (!is_pack_valid(preferred)) {
- warning(_("preferred pack (%s) is invalid"),
- preferred->pack_name);
- goto cleanup;
- }
-
return 0;
cleanup:
@@ -2285,8 +2272,10 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,
if (!pack.bitmap_nr)
continue;
- ALLOC_GROW(packs, packs_nr + 1, packs_alloc);
- memcpy(&packs[packs_nr++], &pack, sizeof(pack));
+ if (is_pack_valid(pack.p)) {
+ ALLOC_GROW(packs, packs_nr + 1, packs_alloc);
+ memcpy(&packs[packs_nr++], &pack, sizeof(pack));
+ }
objects_nr += pack.p->num_objects;
}
@@ -2320,16 +2309,22 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,
pack_int_id = -1;
}
- ALLOC_GROW(packs, packs_nr + 1, packs_alloc);
- packs[packs_nr].p = pack;
- packs[packs_nr].pack_int_id = pack_int_id;
- packs[packs_nr].bitmap_nr = pack->num_objects;
- packs[packs_nr].bitmap_pos = 0;
- packs[packs_nr].from_midx = bitmap_git->midx;
+ if (is_pack_valid(pack)) {
+ ALLOC_GROW(packs, packs_nr + 1, packs_alloc);
+ packs[packs_nr].p = pack;
+ packs[packs_nr].pack_int_id = pack_int_id;
+ packs[packs_nr].bitmap_nr = pack->num_objects;
+ packs[packs_nr].bitmap_pos = 0;
+ packs[packs_nr].from_midx = bitmap_git->midx;
+ packs_nr++;
+ }
- objects_nr = packs[packs_nr++].bitmap_nr;
+ objects_nr = pack->num_objects;
}
+ if (!packs_nr)
+ return;
+
word_alloc = objects_nr / BITS_IN_EWORD;
if (objects_nr % BITS_IN_EWORD)
word_alloc++;
base-commit: caacdb5dfd60540ecec30ec479f147f3c8167e11
--
2.47.1.441.g7fdbfadc049
