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@xxxxxx
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@xxxxxx
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@xxxxxx>
## 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
