Re: [PATCH v3 04/10] packfile: refactor misleading code when unusing pack windows
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Jan 12, 2026, 14:52 UTC
- Message-ID
- <CAOLa=ZR0sM21dpS5cfhhPQZLHOq5L1KXM5h0jXaK1+m17CV5xg@mail.gmail.com>
- In-Reply-To
- <20260109-b4-pks-pack-store-via-source-v3-4-877fd7b7bf81@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> The function `unuse_one_window()` is responsible for unmapping one of > the packfile windows, which is done when we have exceeded the allowed > number of window. >
For my understanding, when dealing with packfiles, we mmap windows of the packfile onto memory. If this is above a threshold, we need to evict one of these mappings, this is done on LRU basis via the `unuse_one_window()`.
Show 7 quoted lines
> The function receives a `struct packed_git` as input, which serves as an > additional packfile that should be considered to be closed. If not > given, we seemingly skip that and instead go through all of the > repository's packfiles. The conditional that checks whether we have a > packfile though does not make much sense anymore, as we dereference the > packfile regardless of whether or not it is a `NULL` pointer to derive > the repository's packfile store.
Yup reading the function for the first time, that struck me immediately too.
Show 9 quoted lines
> The function was originally introduced via f0e17e86e1 (pack: move > release_pack_memory(), 2017-08-18), and here we indeed had a caller that > passed a `NULL` pointer. That caller was later removed via 9827d4c185 > (packfile: drop release_pack_memory(), 2019-08-12), so starting with > that commit we always pass a `struct packed_git`. In 9c5ce06d74 > (packfile: use `repository` from `packed_git` directly, 2024-12-03) we > then inadvertently started to rely on the fact that the pointer is never > `NULL` because we use it now to identify the repository. >
Nice investigation, that explains.
Show 38 quoted lines
> Arguably, it didn't really make sense in the first place that the caller
> provides a packfile, as the selected window would have been overridden
> anyway by the subsequent loop over all packfiles if there was an older
> window. So the overall logic is quite misleading overall. The only case
> where it _could_ make a difference is when there were two packfiles with
> the same `last_used` value, but that case doesn't ever happen because
> the `pack_used_ctr` is strictly increasing.
>
> Refactor the code so that we instead pass in the object database to
> help make the code less misleading.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> packfile.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/packfile.c b/packfile.c
> index 191344eb1c..3700612465 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -355,16 +355,15 @@ static void scan_windows(struct packed_git *p,
> }
> }
>
> -static int unuse_one_window(struct packed_git *current)
> +static int unuse_one_window(struct object_database *odb)
> {
> struct packfile_list_entry *e;
> struct packed_git *lru_p = NULL;
> struct pack_window *lru_w = NULL, *lru_l = NULL;
>
> - if (current)
> - scan_windows(current, &lru_p, &lru_w, &lru_l);
> - for (e = current->repo->objects->packfiles->packs.head; e; e = e->next)
> + for (e = odb->packfiles->packs.head; e; e = e->next)
> scan_windows(e->pack, &lru_p, &lru_w, &lru_l);
> +
>This is much nicer indeed.
Show 17 quoted lines
> if (lru_p) {
> munmap(lru_w->base, lru_w->len);
> pack_mapped -= lru_w->len;
> @@ -740,8 +739,8 @@ unsigned char *use_pack(struct packed_git *p,
> win->len = (size_t)len;
> pack_mapped += win->len;
>
> - while (settings->packed_git_limit < pack_mapped
> - && unuse_one_window(p))
> + while (settings->packed_git_limit < pack_mapped &&
> + unuse_one_window(p->repo->objects))
> ; /* nothing */
> win->base = xmmap_gently(NULL, win->len,
> PROT_READ, MAP_PRIVATE,
>
> --
> 2.52.0.542.g9473a8513b.dirty