Re: [PATCH] packfile: avoid access(3p) calls for missing packs

From: Jeff King <peff@peff.net>
Date: May 19, 2025, 06:52 UTC
Message-ID: <20250519065221.GC102701@coredump.intra.peff.net>
In-Reply-To: <xmqq7c2gv1zx.fsf@gitster.g>

On Fri, May 16, 2025 at 11:34:10AM -0700, Junio C Hamano wrote:

Show 9 quoted lines

> > 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).

Show 10 quoted lines

>     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.

Show 6 quoted lines

> > All of this likely doesn't matter in the general case: Git shouldn't end
> > up looking too many nonexistent packfiles anyway.
> 
> In any case, we shouldn't attempt to use .pack when its associated
> files are missing.  It is not about nonexistent but about incomplete
> (including "in the process of being written").

This shouldn't be a race for files being written. The .idx is written last, so the .pack should always be there. And the midx is written after that, so again, the pack should always be there. At least for newly written packs.

We might see the opposite: packs going away because they were repacked (and perhaps even a new midx generated). In that case you might see a race like:

  - process R opens the existing midx, which mentions pack P (but does
    not yet open P)

  - process W repacks, deleting P and generating a new midx with P'

  - process R wants object X; the midx claims it is in pack P, so it
    tries to open that but fails

What should happen here is that R will fail to find the object at all, should hit reprepare_packed_git(), and will then pick up the new pack.

I don't recall offhand whether reprepare_packed_git() will open the new midx. If it doesn't, we'd still find the object via the actual pack .idx, but we may end up consulting the now-stale midx over and over (so we'll get the right answer, but there may be some room for optimization).

Show 6 quoted lines

> > 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.

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 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.