| Message ID | 7fdbfadc04926efc094633b238a55168c92e3d58.1734117577.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | pack-bitmap.c: ensure pack validity for all reuse packs | expand |
On Fri, Dec 13, 2024 at 02:20:02PM -0500, Taylor Blau wrote: > 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). Nicely explained. > 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. Right, makes sense. > 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. Yeah, I agree that what you wrote is much nicer. It lets us use the optimization as much as possible. I think unsaid here is what happens to the objects in the pack that disappeared. And they are fine: their bits are still set in the reachability bitmap, but not in the verbatim reuse bitmap. So we skip them during that optimized step, but still handle them in the usual way in the rest of pack-objects (adding them to the to_pack struct, considering them for deltas, and so on). Good. > 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++; Cute. I wondered if we could convince pack-objects to pause at the right spot (and actually repack/delete the pack in question) without such a patch, but I'm not sure. I guess the right spot is after we load the midx, but before we finish making the bitmap. Possibly you could start the pack-objects process, feed a few lines that require object access (maybe a tag that needs to be peeled?), and then pause before sending the rest of the input. But it feels quite complex and error-prone. > 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. Yeah, since the reuse there is all-or-nothing anyway, I think there's not much to be gained. If the bitmap result from the traversal turns out not to use _any_ objects from that big pack, I guess we save opening the pack. But I don't think opening is really that expensive (we do not even mmap until asked for a specific object). > 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. OK. It's not like we aren't also allocating a bitmap result of the same size, so it's purely a constant-time speedup. But every little bit helps, and the change is quite simple. > @@ -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; > } OK, so this is the multi-pack reuse code path. So we know we have a midx here. > @@ -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; And this one is the single-reuse case, where we either have a single pack or are reusing the first pack out of a midx. We need to cover it because you dropped the earlier check from 44f9fd6496. Makes sense. In the long run I'm not sure there is a reason to keep this "only do single pack reuse even if we have a midx" code path. I see it as mostly there for testing and comparison with the new, better path. But I guess it's possible that some repacking/midx strategies could prefer spending more resources to possibly make better packs. If we did get rid of that feature, then this is_pack_valid() would be pointless (since the single path case is already covered by dc1daacdcc (pack-bitmap: check pack validity when opening bitmap, 2021-07-23). But it does not really hurt to leave it; if the pack has already been opened then it is a cheap noop. So this all looks good to me. If I nerd-sniped you into the fifo madness that is required to trigger the race in our test suite without a code change, I'll review what you write. But I also think it is OK to leave it. -Peff
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++;
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@ttaylorr.com> --- pack-bitmap.c | 41 ++++++++++++++++++----------------------- 1 file changed, 18 insertions(+), 23 deletions(-) base-commit: caacdb5dfd60540ecec30ec479f147f3c8167e11