From 504131ac26895d1aaa814785d455c2c7f61c4e16 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Fri, 8 Oct 2021 17:46:29 -0400 Subject: [PATCH 1/4] midx.c: extract MIDX lookup by object_dir The first thing that write_midx_internal() does is load the MIDX corresponding to the given object directory, if one is present. Prepare for other functions in midx.c to do the same thing by extracting that operation out to a small helper function. Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano --- midx.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/midx.c b/midx.c index 7e06e85975..cc2ae7e96b 100644 --- a/midx.c +++ b/midx.c @@ -1107,6 +1107,22 @@ cleanup: return ret; } +static struct multi_pack_index *lookup_multi_pack_index(struct repository *r, + const char *object_dir) +{ + struct multi_pack_index *cur; + + /* Ensure the given object_dir is local, or a known alternate. */ + find_odb(r, object_dir); + + for (cur = get_multi_pack_index(r); cur; cur = cur->next) { + if (!strcmp(object_dir, cur->object_dir)) + return cur; + } + + return NULL; +} + static int write_midx_internal(const char *object_dir, struct string_list *packs_to_include, struct string_list *packs_to_drop, @@ -1120,15 +1136,11 @@ static int write_midx_internal(const char *object_dir, struct hashfile *f = NULL; struct lock_file lk; struct write_midx_context ctx = { 0 }; - struct multi_pack_index *cur; int pack_name_concat_len = 0; int dropped_packs = 0; int result = 0; struct chunkfile *cf; - /* Ensure the given object_dir is local, or a known alternate. */ - find_odb(the_repository, object_dir); - midx_name = get_midx_filename(object_dir); if (safe_create_leading_directories(midx_name)) die_errno(_("unable to create leading directories of %s"), @@ -1140,12 +1152,7 @@ static int write_midx_internal(const char *object_dir, * packs to include, since all packs and objects are copied * blindly from an existing MIDX if one is present. */ - for (cur = get_multi_pack_index(the_repository); cur; cur = cur->next) { - if (!strcmp(object_dir, cur->object_dir)) { - ctx.m = cur; - break; - } - } + ctx.m = lookup_multi_pack_index(the_repository, object_dir); } if (ctx.m && !midx_checksum_valid(ctx.m)) { From 98926e0d015e03a3b922ba6d8ca652f1da7b4429 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Fri, 8 Oct 2021 17:46:32 -0400 Subject: [PATCH 2/4] midx.c: lookup MIDX by object directory during expire Before a new MIDX can be written, expire_midx_packs() first loads the existing MIDX, figures out which packs can be expired, and then writes a new MIDX based on that information. In order to load the existing MIDX, it uses load_multi_pack_index(), which mmaps the multi-pack-index file, but does not store the resulting `struct multi_pack_index *` in the object store. write_midx_internal() also needs to open the existing MIDX, and it does so by iterating the results of get_multi_pack_index(), so that it reuses the same pointer held by the object store. But before it can move the new MIDX into place, it close_object_store() to munmap() the multi-pack-index file to accommodate platforms like Windows which don't allow overwriting files which are memory mapped. That's where things get weird. Since expire_midx_packs has its own *separate* memory mapped copy of the MIDX, the MIDX file is still memory mapped! Interestingly, this doesn't seem to cause a problem in our tests. (I believe that this has much more to do with my own lack of familiarity with Windows than it does a lack of coverage in our tests). In any case, we can side-step the whole issue by teaching expire_midx_packs() to use the `struct multi_pack_index` pointer it found via the object store instead of maintain its own copy. That way, when write_midx_internal() calls `close_object_store()`, we know that there are no memory mapped copies of the MIDX laying around. A couple of other small notes about this patch: - As far as I can tell, passing `local == 1` to the call to load_multi_pack_index() was an error, since object_dir could be an alternate. But it doesn't matter, since even though we write `m->local = 1`, we never read that field back later on. - Setting `m = NULL` after write_midx_internal() was likely to prevent a double-free back from when that function took a `struct multi_pack_index *` that it called close_midx() on itself. We can rely on write_midx_internal() to call that for us now. Finally, this enforces the same "the value of --object-dir must be the local object store, or an alternate" rule from f57a739691 (midx: avoid opening multiple MIDXs when writing, 2021-09-01) to the `expire` sub-command, too. Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano --- midx.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/midx.c b/midx.c index cc2ae7e96b..053279bef3 100644 --- a/midx.c +++ b/midx.c @@ -1696,7 +1696,7 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned fla { uint32_t i, *count, result = 0; struct string_list packs_to_drop = STRING_LIST_INIT_DUP; - struct multi_pack_index *m = load_multi_pack_index(object_dir, 1); + struct multi_pack_index *m = lookup_multi_pack_index(r, object_dir); struct progress *progress = NULL; if (!m) @@ -1741,12 +1741,11 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned fla free(count); - if (packs_to_drop.nr) { + if (packs_to_drop.nr) result = write_midx_internal(object_dir, NULL, &packs_to_drop, NULL, NULL, flags); - m = NULL; - } string_list_clear(&packs_to_drop, 0); + return result; } From c0f1f9dec4457ffecf9e087665e6eb28da19c18f Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Fri, 8 Oct 2021 17:46:35 -0400 Subject: [PATCH 3/4] midx.c: lookup MIDX by object directory during repack Apply similar treatment as in the last commit to the MIDX `repack` operation. Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano --- midx.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/midx.c b/midx.c index 053279bef3..f2c976051d 100644 --- a/midx.c +++ b/midx.c @@ -1861,7 +1861,7 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size, struct child_process cmd = CHILD_PROCESS_INIT; FILE *cmd_in; struct strbuf base_name = STRBUF_INIT; - struct multi_pack_index *m = load_multi_pack_index(object_dir, 1); + struct multi_pack_index *m = lookup_multi_pack_index(r, object_dir); /* * When updating the default for these configuration @@ -1933,11 +1933,8 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size, } result = write_midx_internal(object_dir, NULL, NULL, NULL, NULL, flags); - m = NULL; cleanup: - if (m) - close_midx(m); free(include_pack); return result; } From ae22e8415d5ec2b52a4c83e181297ac15aeaced0 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Fri, 8 Oct 2021 17:46:38 -0400 Subject: [PATCH 4/4] midx.c: guard against commit_lock_file() failures When writing a MIDX, we atomically move the new MIDX into place via commit_lock_file(), but do not check to see if that call was successful. Make sure that we do check in order to prevent us from incorrectly reporting that we wrote a new MIDX if we actually encountered an error. Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano --- midx.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/midx.c b/midx.c index f2c976051d..8433086ac1 100644 --- a/midx.c +++ b/midx.c @@ -1423,7 +1423,8 @@ static int write_midx_internal(const char *object_dir, if (ctx.m) close_object_store(the_repository->objects); - commit_lock_file(&lk); + if (commit_lock_file(&lk) < 0) + die_errno(_("could not write multi-pack-index")); clear_midx_files_ext(object_dir, ".bitmap", midx_hash); clear_midx_files_ext(object_dir, ".rev", midx_hash);