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

[Taylor Blau <me@ttaylorr.com>]

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

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

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

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