[PATCH v3 0/2] packfile: avoid access(3p) calls for missing packs

Hi,

this patch addresses an issue we have recently seen in our production systems due to a stale MIDX. The MIDX contained entries for packfiles that didn't exist anymore, which caused Git to repeatedly look up those packfiles. Each missing packfile resulted in four repeated syscalls: three access(3p) calls to check for supporting data structures, and one call to stat(3p) to check for the packfile itself. The first three calls are essentially wasted though when the stat(3p) call itself fails, which is being fixed by this patch.

I doubt that the patch matters in almost any repository, but given that the refactoring is trivial I thought to submit the patch regardless of that. Another step would be to introduce a negative lookup cache -- but that would be a bit more involved, so I decided against it for now as I don't want to introduce complexity for dubious gains.

Changes in v2:
  - Drop the patch that reorders syscalls and add a comment explaining
    why the order is important.
  - Add a negative lookup cache for indexed packfiles.
  - Link to v1: https://lore.kernel.org/r/20250516-pks-pack-avoid-stats-on-missing-v1-1-e2ef4d8798a3@pks.im

Changes in v3:
  - Use a macro to hide away the `(void *)(intptr_t)-1` magic.
  - Link to v2: https://lore.kernel.org/r/20250520-pks-pack-avoid-stats-on-missing-v2-0-333c5217fb05@pks.im

Thanks!

Patrick

---
Patrick Steinhardt (2):
      packfile: explain ordering of how we look up auxiliary pack files
      midx: stop repeatedly looking up nonexistent packfiles

 midx.c     | 12 ++++++++++--
 packfile.c | 11 +++++++++++
 2 files changed, 21 insertions(+), 2 deletions(-)

Range-diff versus v2:

1:  cf601a63e98 = 1:  c4087674967 packfile: explain ordering of how we look up auxiliary pack files
2:  e3108f7ce48 ! 2:  2fa98231464 midx: stop repeatedly looking up nonexistent packfiles
    @@ Commit message
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## midx.c ##
    +@@
    + #include "pack-bitmap.h"
    + #include "pack-revindex.h"
    + 
    ++#define MIDX_PACK_ERROR ((void *)(intptr_t)-1)
    ++
    + int midx_checksum_valid(struct multi_pack_index *m);
    + void clear_midx_files_ext(const char *object_dir, const char *ext,
    + 			  const char *keep_hash);
     @@ midx.c: 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)
    ++		if (m->packs[i] && m->packs[i] != MIDX_PACK_ERROR)
      			m->packs[i]->multi_pack_index = 0;
      	}
      	FREE_AND_NULL(m->packs);
    @@ midx.c: 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)
    ++	if (m->packs[pack_int_id] == MIDX_PACK_ERROR)
     +		return 1;
      	if (m->packs[pack_int_id])
      		return 0;
    @@ midx.c: int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,
      
     -	if (!p)
     +	if (!p) {
    -+		m->packs[pack_int_id] = (void *)(intptr_t)-1;
    ++		m->packs[pack_int_id] = MIDX_PACK_ERROR;
      		return 1;
     +	}
      
    @@ midx.c: struct packed_git *nth_midxed_pack(struct multi_pack_index *m,
      				   uint32_t pack_int_id)
      {
      	uint32_t local_pack_int_id = midx_for_pack(&m, pack_int_id);
    -+	if (m->packs[local_pack_int_id] == (void *)(intptr_t)-1)
    ++	if (m->packs[local_pack_int_id] == MIDX_PACK_ERROR)
     +		return NULL;
      	return m->packs[local_pack_int_id];
      }

--- base-commit: 1a8a4971cc6c179c4dd711f4a7f5d7178f4b3ab7 change-id: 20250516-pks-pack-avoid-stats-on-missing-8e3b75755cf0