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

The function `add_packed_git()` adds a packfile to the known list of packs in case it exists. In case the packfile doesn't look like a pack though, or in case it doesn't exist, the function will simply return a `NULL` pointer.

The ordering in which those checks are done is somewhat curious though: we first perform a couple of access(3p) syscalls for auxiliary files like ".keep" before we determine whether the ".pack" file itself exists. And if we see that the ".pack" file does not exist, we bail out and thus effectively discard all the information. This means that the access(3p) calls were a complete waste of time.

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.

All of this likely doesn't matter in the general case: Git shouldn't end up looking too many nonexistent packfiles anyway. But there are edge cases where it _does_ matter. One such edge case that we have hit in our production systems was a stale "multi-pack-index" file that contained references to nonexistent packfiles. As there is no negative lookup cache this causes us to repeatedly look up the same packfiles via the multi-pack index, only to realize that they don't exist. This translated into hundreds of thousands of syscalls to access(3p) and stat(3p).

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. This requires us to restore the final packfile name after the access(3p) calls. But given that this is a mere memory copy it is very unlikely to matter in comparison to the four syscalls we performed beforehand.

Another mitigation would be to introduce a negative lookup cache so that we don't try to add missing packfiles repeatedly. But that refactoring would be a bit more involved for dubious gains, so for now the author has decided against it.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
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.

Thanks!

Patrick
---
 packfile.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)