On Mon, May 19, 2025 at 02:52:21AM -0400, Jeff King wrote:
> On Fri, May 16, 2025 at 11:34:10AM -0700, Junio C Hamano wrote:
>
> > > The reason why we do this is likely because we reuse `p->pack_name` to
> > > derive the other files' names, as well, so that we only have to allocate
> > > this buffer once. As such, we have to compute the packfile name last so
> > > that it doesn't get overwritten by the other names.
> >
> > I vaguely recall that the order of checks between .idx and .pack
> > were deliberately chosen, as we do not want to add and attempt to
> > use .pack file before its associated .idx file is ready since
> > without the latter the former is unusable at runtime.
>
> It is definitely intentional and important that we discover packs by
> their .idx file first. The writing side uses the creation of the .idx
> file (via atomic rename) as the final step in making a pack accessible.
> Before that, a reader could see a .pack without its various accessory
> files (and so not realize it has a .keep file, for example).
Yup. But that order doesn't change anyway in this patch. What changes is
only the order for auxiliary and non-mandatory ".keep", ".promisor" and
".mtimes" files.
> > Side note: It may have been more important back when the name of
> > the packfile was a hash of names of the objects in the pack,
> > which meant that when you repack a quiescent and fully repacked
> > repository twice with different parameters, you could have ended
> > up with a packfile whose name is the same but the contents as a
> > bytestream (hence the object offset) are different, making a
> > stale .idx not pointing into the correct position in the new
> > .pack file. These days the name of the packfile is based on the
> > contents of the pack as a bytestream, so we no longer suffer
> > in such a scenario.
>
> I don't think reading the .idx file first was sufficient to protect us
> there, since the .pack could be replaced racily after the reader opened
> the .idx. I don't recall if we were subject to that race back then (and
> it just didn't come up enough to worry about), or if repack would throw
> away an identically-named file. Anyway, as you note, it's no longer an
> issue.
>
> But I think that is mostly orthogonal to how the auxiliary files are
> handled. I think Patrick's guess is correct that the order we have is
> simply because it was most convenient to end up with ".pack" in the
> name.
Taking a step back though: do we always ensure the order in which we
those auxiliary files are created and deleted? If we know that those are
always:
- Moved into place before their ".idx" file.
- Deleted after deleting their ".pack" file.
Then reordering may cause us to race indeed so that we see the packfile,
but miss the auxiliary data structures. `unlink_pack_path()` does seem
to ensure the latter property, as both ".idx" and ".pack" are deleted
first. I'm not quite sure about the former, but it seems like we also do
this.
So I think that the proposed patch is wrong. There should definitely be
a comment in that function though to say that this order is intentional
and not merely an optimization.
[snip]
> > > while the issue was entirely self-made because the multi-pack index
> > > should have been regenerated, we can still reduce the number of syscalls
> > > by 75% in the case of nonexistent packfiles by reordering these calls.
> >
> > That sounds more like a band-aid, if we still do the remaining 25%
> > that we somehow know would be unnecessary.
>
> Yeah, that was my gut reaction, too. add_packed_git() is not
> optimized[1] at all. But it shouldn't have to be, because it's meant to
> be called once per process. Even with the re-ordering, we'd still make a
> bunch of pointless stat() calls for the .pack file we know is not there.
It certainly is, as mentioned in the cover letter. It was a supposedly
cheap win as I didn't want to introduce a negative lookup cache for an
edge case like this. But the patch below looks simple enough -- I feared
it would be a lot more involved.
> The code in prepare_midx_pack() converts a numeric pack id (referenced
> inside the midx) into a "struct packed_git" pointer, caching the results
> in multi_pack_index->packs. That field holds NULL for "we have not
> looked it up yet" or a valid pointer to a packed_git. It probably needs
> to hold a third state: "we tried and failed".
>
> Something like this (large untested) patch:
>
> diff --git a/midx.c b/midx.c
> index 3d0015f782..354b1f886c 100644
> --- a/midx.c
> +++ b/midx.c
> @@ -405,7 +405,7 @@ void close_midx(struct multi_pack_index *m)
> munmap((unsigned char *)m->data, m->data_len);
>
> for (i = 0; i < m->num_packs; i++) {
> - if (m->packs[i])
> + if (m->packs[i] && m->packs[i] != (void *)(intptr_t)-1)
> m->packs[i]->multi_pack_index = 0;
> }
> FREE_AND_NULL(m->packs);
> @@ -458,6 +458,8 @@ int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,
>
> pack_int_id = midx_for_pack(&m, pack_int_id);
>
> + if (m->packs[pack_int_id] == (void *)(intptr_t)-1)
> + return 1;
> if (m->packs[pack_int_id])
> return 0;
>
> @@ -482,8 +484,10 @@ int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,
> strbuf_release(&pack_name);
> strbuf_release(&key);
>
> - if (!p)
> + if (!p) {
> + m->packs[pack_int_id] = (void *)(intptr_t)-1;
> return 1;
> + }
>
> p->multi_pack_index = 1;
> m->packs[pack_int_id] = p;
>
> -Peff
>
> [1] There probably are optimization opportunities in add_packed_git(). I
> don't think re-ordering will help much in the common case that we
> actually do have the pack. But really, most callers do not care
> about these auxiliary files at all! We could simply skip them during
> the initial setup and lazy-load them via accessor functions.
>
> I _think_ that should be OK with respect to races. For a newly added
> pack, we know they will always be in place before the matching .idx
> file (per the logic I outlined above). For a pack that goes away, we
> might racily fail to see its auxiliary file. But that is mostly true
> now (we might see its .idx, and then the .promisor file is deleted
> before we call access()). It does increase the size of that window,
> though (and in particular lets it happen even if the pack has
> actually been opened).
I'm not sure it would be okay, as mentioned above. The current ordering
ensures that we always see auxiliary data structures in case the ".pack"
file exists. If we started to cache then that wouldn't be the case
anymore.
> I'm not sure how much that would matter in practice. OTOH, I'm not
> sure that saving a few access() calls would be all that meaningful,
> unless you have a zillion packs. And if you have a zillion packs,
> you're likely to get much bigger speedups in other areas by
> repacking them anyway. So it hasn't really been an area I've
> pursued before.
It probably doesn't in the general case at all.
Patrick
