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
