| Message ID | 7fdbfadc04926efc094633b238a55168c92e3d58.1734117577.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 62b3ec8a3f160cc9f1949b28644fc3947252db73 |
| 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
Taylor Blau <me@ttaylorr.com> writes: > @@ -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; This is a pure removal, not the block of code moved elsewhere. I am having a hard time resolving the conflict between this one and what f34db3f6 (pack-bitmap.c: open and store incremental bitmap layers, 2024-11-19) did to the same part of the file, which is: @@ -459,13 +478,20 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git, goto cleanup; } - preferred = bitmap_git->midx->packs[preferred_pack]; + preferred = nth_midxed_pack(bitmap_git->midx, preferred_pack); if (!is_pack_valid(preferred)) { warning(_("preferred pack (%s) is invalid"), preferred->pack_name); goto cleanup; } + if (midx->base_midx) { + bitmap_git->base = prepare_midx_bitmap_git(midx->base_midx); + bitmap_git->base_nr = bitmap_git->base->base_nr + 1; + } else { + bitmap_git->base_nr = 1; + } + return 0; cleanup: Both are from you, and I am guessing that you have tried all of your topics in flight together, if not the other topics. I wonder what we can do better to make sure the work a contributor has already done (in this case, resolve interaction between two topics) is not wasted and recreated (possibly incorrectly) by the maintainer. Thanks.
On Wed, Dec 18, 2024 at 11:41:50AM -0800, Junio C Hamano wrote: > Both are from you, and I am guessing that you have tried all of your > topics in flight together, if not the other topics. Oops, sorry for the trouble. I believe the correct resolution is the following: --- 8< --- diff --cc pack-bitmap.c index ff09b15eb7,83696d834f..0000000000 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@@ -388,9 -427,17 +427,16 @@@ static int open_midx_bitmap_1(struct bi { struct stat st; char *bitmap_name = midx_bitmap_filename(midx); - int fd = git_open(bitmap_name); + int fd; - uint32_t i, preferred_pack; - struct packed_git *preferred; + uint32_t i; + fd = git_open(bitmap_name); + if (fd < 0 && errno == ENOENT) { + FREE_AND_NULL(bitmap_name); + bitmap_name = midx_bitmap_filename(midx); + fd = git_open(bitmap_name); + } + if (fd < 0) { if (errno != ENOENT) warning_errno("cannot open '%s'", bitmap_name); @@@ -444,6 -491,25 +490,13 @@@ } } - if (midx_preferred_pack(bitmap_git->midx, &preferred_pack) < 0) { - warning(_("could not determine MIDX preferred pack")); - goto cleanup; - } - - preferred = nth_midxed_pack(bitmap_git->midx, preferred_pack); - if (!is_pack_valid(preferred)) { - warning(_("preferred pack (%s) is invalid"), - preferred->pack_name); - goto cleanup; - } - + if (midx->base_midx) { + bitmap_git->base = prepare_midx_bitmap_git(midx->base_midx); + bitmap_git->base_nr = bitmap_git->base->base_nr + 1; + } else { + bitmap_git->base_nr = 1; + } + return 0; cleanup: --- >8 --- IOW, we no longer need to check the validity of the preferred pack in either case. But in an incremental MIDX bitmaps world, we do need to keep calling prepare_midx_bitmap_git() along the MIDX's ->base pointer, if non-NULL. > I wonder what we can do better to make sure the work a contributor has > already done (in this case, resolve interaction between two topics) is > not wasted and recreated (possibly incorrectly) by the maintainer. I am not sure. During the interim maintainer period, Patrick sent a couple of rounds of ps/build with a final patch to the effect of "unbreak everything in seen", which could be dropped. But I think an easier thing to do would have been for myself to indicate that you'd run into a non-trivial conflict here and provide the resolution proactively. Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: > IOW, we no longer need to check the validity of the preferred pack in > either case. But in an incremental MIDX bitmaps world, we do need to > keep calling prepare_midx_bitmap_git() along the MIDX's ->base pointer, > if non-NULL. Thanks. >> I wonder what we can do better to make sure the work a contributor has >> already done (in this case, resolve interaction between two topics) is >> not wasted and recreated (possibly incorrectly) by the maintainer. > > I am not sure. During the interim maintainer period, Patrick sent a > couple of rounds of ps/build with a final patch to the effect of > "unbreak everything in seen", which could be dropped. > > But I think an easier thing to do would have been for myself to indicate > that you'd run into a non-trivial conflict here and provide the > resolution proactively. A trick used by recent series from Patrick say things like "this is based on X, with Y and Z merged". This patch could have done the same way. It of course makes two topics entangled and one takes the other hostage, so we need to carefully judge if such a dependency is worth it. So far, I found Patrick's judgement on the choice of dependencies quite solid (essentially, Y and Z must be cooked enough at least for 'next', and can reasonably be expected to graduate while we iterate on the new topic that is being built). Thanks.
On Wed, Dec 18, 2024 at 05:16:35PM -0800, Junio C Hamano wrote: > >> I wonder what we can do better to make sure the work a contributor has > >> already done (in this case, resolve interaction between two topics) is > >> not wasted and recreated (possibly incorrectly) by the maintainer. > > > > I am not sure. During the interim maintainer period, Patrick sent a > > couple of rounds of ps/build with a final patch to the effect of > > "unbreak everything in seen", which could be dropped. > > > > But I think an easier thing to do would have been for myself to indicate > > that you'd run into a non-trivial conflict here and provide the > > resolution proactively. > > A trick used by recent series from Patrick say things like "this is > based on X, with Y and Z merged". This patch could have done the > same way. It of course makes two topics entangled and one takes the > other hostage, so we need to carefully judge if such a dependency is > worth it. So far, I found Patrick's judgement on the choice of > dependencies quite solid (essentially, Y and Z must be cooked enough > at least for 'next', and can reasonably be expected to graduate while > we iterate on the new topic that is being built). I typically try and do the same, but I didn't want to base this topic on top of tb/incremental-midx-part-2, since that topic isn't mature enough and I didn't want to hold this one hostage as a result. I forgot that the other topic was still being held in 'seen'; I should have remembered and mentioned the conflict in the patch earlier. Sorry again for the trouble :-<. Thanks, Taylor
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