[2/4] midx.c: lookup MIDX by object directory during expire

Commit Message

Taylor Blau Oct. 8, 2021, 9:46 p.m. UTC

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 <[e-mail]>
---
This does leak the MIDX write_midx_internal returns before calling
close_object_store(). We can't just blindly call close_object_store()
here, either, since it's susceptible to double-frees. I'll think about
improving this in a separate series.

 midx.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

--
2.33.0.96.g73915697e6

Comments

Derrick Stolee Oct. 13, 2021, 1:28 p.m. UTC | #1

On 10/8/2021 5:46 PM, Taylor Blau wrote:
> 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).

You are fixing a bug in two ways:

1. This change ensures we use the 'struct multi_pack_index' that exists
   in the object store, ensuring it is closed before committing the lockfile.

2. Without this change, the change in patch 4 would cause the 'expire' tests
   to start failing on Windows, because the commands would die().

If our tests also verified that the .git/objects/pack/multi-pack-index.lock
file was missing at the end of the command, then we would have caught this
bug on Windows. I don't think that's a reasonable test, but instead we(I)
should have used the API correctly from the start.

The tests _do_ verify that the expired packs are deleted, but the new MIDX
probably refers to the old packs still. Since those packs are not actually
used (the necessary condition for expiring them), later Git commands do not
attempt to load them and hence do not fail. That is, until we try to expire
again and likely get warnings about missing pack-files.

Thanks,
-Stolee

Patch

diff --git a/midx.c b/midx.c
index b66b75a3cd..7f1addf4b6 100644
--- a/midx.c
+++ b/midx.c
@@ -1707,7 +1707,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)
@@ -1752,12 +1752,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;
 }