From e1c9ad42a81ac2d5fa0aae0ceb2a1c8fc5be1504 Mon Sep 17 00:00:00 2001 From: metanivek Date: Fri, 28 Apr 2023 14:21:24 -0400 Subject: [PATCH 1/3] irmin-pack: fix v2 lower migration Previously we did not take the dead header of the dict into account when performing a lower layer migration for older stores. This commit fixes this issue by removing the dead header from the dict during the migration. --- src/irmin-pack/unix/file_manager.ml | 28 ++++++++++++++++++++- test/irmin-pack/test_lower.ml | 38 +++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/src/irmin-pack/unix/file_manager.ml b/src/irmin-pack/unix/file_manager.ml index 6c4fdc786f..fa296a80a7 100644 --- a/src/irmin-pack/unix/file_manager.ml +++ b/src/irmin-pack/unix/file_manager.ml @@ -568,10 +568,35 @@ struct let data = Layout.prefix ~root ~generation in Sparse.Ao.create ~mapping ~data >>= Sparse.Ao.close in - (* Step 4. Update the upper control file. *) + (* Step 4. Remove dead header from dict (if needed) *) + let* dict_end_poff, after_payload_write = + if dead_header_size > 0 then ( + let dict_path = Layout.dict ~root in + let tmp_dict_path = Filename.temp_file ~temp_dir:root "store" "dict" in + let* dict_file = Io.open_ ~path:dict_path ~readonly:false in + let* len = Io.read_size dict_file in + let* tmp_dict_file = Io.open_ ~path:tmp_dict_path ~readonly:false in + let contents_len = Int63.to_int len - dead_header_size in + let* contents = + Io.read_to_string dict_file + ~off:(Int63.of_int dead_header_size) + ~len:contents_len + in + Io.write_exn tmp_dict_file ~off:Int63.zero ~len:contents_len contents; + let* _ = Io.close dict_file in + let* _ = Io.close tmp_dict_file in + (* Delay moving the temp file until after the payload is written so + that we do not try to remove the dead header twice after a failure. *) + Ok + ( Int63.of_int contents_len, + fun () -> Io.move_file ~src:tmp_dict_path ~dst:dict_path )) + else Ok (payload.dict_end_poff, Fun.const (Ok ())) + in + (* Step 5. Update the upper control file. *) let payload = { payload with + dict_end_poff; chunk_start_idx; appendable_chunk_poff = Int63.zero; volume_num = 1; @@ -587,6 +612,7 @@ struct } in let* () = Control.set_payload control payload in + let* () = after_payload_write () in Ok payload let load_payload ~config ~root ~lower_root ~control = diff --git a/test/irmin-pack/test_lower.ml b/test/irmin-pack/test_lower.ml index 398ea21ca5..b5d99276ea 100644 --- a/test/irmin-pack/test_lower.ml +++ b/test/irmin-pack/test_lower.ml @@ -210,6 +210,28 @@ module Store_tc = struct | Gced { generation; _ } -> generation | _ -> Alcotest.fail "expected gced status" + (* Reads all objects from the repo by iterating its index and folding its commit trees. *) + let read_everything repo = + let fm = Store.Internal.file_manager repo in + let index = Store.Internal.File_manager.index fm in + let commits = ref [] in + let () = + Store.Internal.Index.iter + (fun hash (_offset, _len, kind) -> + match kind with + | Irmin_pack.Pack_value.Kind.Commit_v2 -> commits := hash :: !commits + | _ -> ()) + index + in + Lwt_list.map_s + (fun hash -> + [%log.debug "read %a" Irmin.Type.(pp Store.Hash.t) hash]; + let* commit = Store.Commit.of_hash repo hash in + match commit with + | None -> Alcotest.fail "failed to read commit" + | Some commit -> Store.Tree.fold (Store.Commit.tree commit) ()) + !commits + let test_create () = let* repo = init () in (* A newly created store with a lower should have an empty volume. *) @@ -307,6 +329,21 @@ module Store_tc = struct let previous_tree = Store.Commit.tree @@ Option.get parent in let* a_opt = Store.Tree.find previous_tree [ "a" ] in Alcotest.(check (option string)) "upper to lower" (Some "a") a_opt; + + (* Tests that dead header is handled appropriately *) + let test_migrate_v2 () = + let ( / ) = Filename.concat in + let root_archive = + "test" / "irmin-pack" / "data" / "version_2_to_3_always" + in + let root = "_build" / "test_lower_migrate_v2" in + setup_test_env ~root_archive ~root_local_build:root; + let lower_root = root / "lower" in + (* Open store and trigger migration. This should succeed. *) + let* repo = Store.Repo.v (config ~fresh:false ~lower_root root) in + let* _ = read_everything repo in + Store.Repo.close repo + Store.Repo.close repo let test_migrate_then_gc () = @@ -444,6 +481,7 @@ module Store = struct quick_tc "control file updated after add" test_add_volume_reopen; quick_tc "add volume and reopen" test_add_volume_reopen; quick_tc "create without lower then migrate" test_migrate; + quick_tc "migrate v2" test_migrate_v2; quick_tc "migrate then gc" test_migrate_then_gc; quick_tc "test data locality" test_volume_data_locality; quick_tc "test cleanup" test_cleanup; From 66af058ea763d60a8212f4665f2bd46e03ca0cc3 Mon Sep 17 00:00:00 2001 From: metanivek Date: Fri, 28 Apr 2023 14:23:04 -0400 Subject: [PATCH 2/3] irmin-pack: add tests for migrating v3 stores These stores migrate fine (these tests did not fail) but adding them for completeness as we add future stores. Also added a line that reads all contents in the store to ensure data moved correctly during the migration. --- test/irmin-pack/test_lower.ml | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/test/irmin-pack/test_lower.ml b/test/irmin-pack/test_lower.ml index b5d99276ea..13d0f17a6f 100644 --- a/test/irmin-pack/test_lower.ml +++ b/test/irmin-pack/test_lower.ml @@ -329,6 +329,8 @@ module Store_tc = struct let previous_tree = Store.Commit.tree @@ Option.get parent in let* a_opt = Store.Tree.find previous_tree [ "a" ] in Alcotest.(check (option string)) "upper to lower" (Some "a") a_opt; + let* _ = read_everything repo in + Store.Repo.close repo (* Tests that dead header is handled appropriately *) let test_migrate_v2 () = @@ -344,6 +346,27 @@ module Store_tc = struct let* _ = read_everything repo in Store.Repo.close repo + let test_migrate_v3 () = + (* minimal indexing *) + let ( / ) = Filename.concat in + let root_archive = "test" / "irmin-pack" / "data" / "version_3_minimal" in + let root = "_build" / "test_lower_migrate_v3_minimal" in + setup_test_env ~root_archive ~root_local_build:root; + let lower_root = root / "lower" in + (* Open store and trigger migration. This should succeed. *) + let* repo = Store.Repo.v (config ~fresh:false ~lower_root root) in + let* _ = read_everything repo in + let* _ = Store.Repo.close repo in + + (* always indexing *) + let ( / ) = Filename.concat in + let root_archive = "test" / "irmin-pack" / "data" / "version_3_always" in + let root = "_build" / "test_lower_migrate_v3_always" in + setup_test_env ~root_archive ~root_local_build:root; + let lower_root = root / "lower" in + (* Open store and trigger migration. This should succeed. *) + let* repo = Store.Repo.v (config ~fresh:false ~lower_root root) in + let* _ = read_everything repo in Store.Repo.close repo let test_migrate_then_gc () = @@ -367,6 +390,7 @@ module Store_tc = struct (* GC at [b] requires reading [a] data from the lower volume *) let* _ = Store.Gc.start_exn repo (Store.Commit.key b_commit) in let* _ = Store.Gc.finalise_exn ~wait:true repo in + let* _ = read_everything repo in Store.Repo.close repo let test_volume_data_locality () = @@ -482,6 +506,7 @@ module Store = struct quick_tc "add volume and reopen" test_add_volume_reopen; quick_tc "create without lower then migrate" test_migrate; quick_tc "migrate v2" test_migrate_v2; + quick_tc "migrate v3" test_migrate_v3; quick_tc "migrate then gc" test_migrate_then_gc; quick_tc "test data locality" test_volume_data_locality; quick_tc "test cleanup" test_cleanup; From fffa054dc2de6acb750bde44d085dac1448564d3 Mon Sep 17 00:00:00 2001 From: metanivek Date: Fri, 28 Apr 2023 14:30:35 -0400 Subject: [PATCH 3/3] Add changelog entry for fixed migration --- CHANGES.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index 5f846dccef..67d1054569 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,3 +1,10 @@ +## Unreleased + +### Fixed + +- **irmin-pack** + - Fix issue when migrating v2 stores to use lower layer (@metanivek, #2241) + ## 3.7.0 (2023-04-26) ### Added