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

From: Taylor Blau <me@ttaylorr.com>
Date: May 23, 2025, 01:02 UTC
Message-ID: <aC/JDrF5uJh9XxXj@nand.local>
In-Reply-To: <20250519065221.GC102701@coredump.intra.peff.net>

On Mon, May 19, 2025 at 02:52:21AM -0400, Jeff King wrote:

Show 17 quoted lines

> 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. Or seeing a pack before its .bitmap is written, in which case any clones or fetches unfortunate enough to take place after the pack was read but before the .bitmap was written would be significantly slower than they otherwise would have been with bitmaps.

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

This all sounds right to me. And we do end up loading a new MIDX via reprepare_packed_git(): that function calls prepare_packed_git() (which doesn't immediately return, since we just zero'd out the r->objects->packed_git_initialized flag). We then enumerate the ODBs, calling prepare_multi_pack_index_one() and prepare_packed_git_one() for each.

From there we end up in load_multi_pack_index(), which gets a freshened view of the MIDX file. If all goes well there, then prepare_multi_pack_index_one() assigns it to r->objects->multi_pack_index as expected.

Show 5 quoted lines

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

Yup, that seems like a good direction to me.

Show 5 quoted lines

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

Nice idea. I initially though, "that's not right, we definitely need to populate the flag bits, e.g., p->is_cruft". But that was before I read "and lazy-load them via accessor functions". That turns those fields into a tri-state, which is a nice way to avoid this altogether.

Thanks, Taylor