Add split function to create a new chunk #2118

icristescu · 2022-10-14T13:28:43Z

Rebased on #2115.

I want to add more tests, but I will not change too much the code, apart from adapting to #2115 and #2117.

metanivek

Exciting! I know this is WIP but left some comments for consideration.

src/irmin-pack/unix/chunked_suffix.ml

src/irmin-pack/unix/chunked_suffix_intf.ml

src/irmin-pack/unix/file_manager.ml

metanivek · 2022-10-14T19:44:37Z

test/irmin-pack/test_gc.ml

@@ -833,3 +833,24 @@ module Concurrent_gc = struct
      tc "Test kill gc and close" test_kill_gc_and_close;
    ]
 end
+
+module Split = struct
+  let two_splits () =


🎉 it works!

codecov-commenter · 2022-10-17T11:55:51Z

Codecov Report

Merging #2118 (a9a37ae) into main (22bbfde) will increase coverage by 3.28%.
The diff coverage is 83.52%.

❗ Current head a9a37ae differs from pull request most recent head 2be7958. Consider uploading reports for the commit 2be7958 to get more accurate results

@@            Coverage Diff             @@
##             main    #2118      +/-   ##
==========================================
+ Coverage   64.91%   68.20%   +3.28%     
==========================================
  Files         134      134              
  Lines       15960    16031      +71     
==========================================
+ Hits        10361    10934     +573     
+ Misses       5599     5097     -502

Impacted Files	Coverage Δ
src/irmin-pack/unix/errors.ml	`28.88% <ø> (ø)`
src/irmin-pack/unix/gc_worker.ml	`3.75% <0.00%> (-0.28%)`	⬇️
src/irmin-pack/unix/io_errors.ml	`58.33% <ø> (ø)`
src/irmin-pack/unix/ext.ml	`62.40% <71.42%> (+0.25%)`	⬆️
src/irmin-pack/unix/file_manager.ml	`84.17% <88.88%> (+0.10%)`	⬆️
src/irmin-pack/unix/chunked_suffix.ml	`96.47% <100.00%> (+10.19%)`	⬆️
src/irmin-pack/unix/dispatcher.ml	`75.00% <100.00%> (ø)`
src/irmin-pack/unix/gc.ml	`82.14% <100.00%> (ø)`
src/irmin-test/store.ml	`94.89% <0.00%> (-0.06%)`	⬇️
src/irmin-fs/unix/irmin_fs_unix.ml	`68.38% <0.00%> (+0.64%)`	⬆️
... and 5 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

metanivek

More comments for consideration. I think this is really close to ready!

src/irmin-pack/unix/chunked_suffix.ml

src/irmin-pack/unix/chunked_suffix_intf.ml

src/irmin-pack/unix/file_manager.ml

metanivek · 2022-10-18T19:17:30Z

src/irmin-pack/unix/file_manager.ml

+      Control.set_payload t.control pl
+    in
+    (* Step 4. Fsync the control file *)
+    Control.fsync t.control


I'm still wondering about this fsync. We update the control file in other places without fsyncing (for instance, in swap) -- should we break that here?

In the event that the control file data does not make it to disk, the following things would happen:

There would be a dangling chunk that is removed on next open by cleanup

A future GC would be non-optimal and a chunk would live an extra GC cycle before being deleted.

Point 1 is okay (I think) and point 2 is less great since it means more disk space usage. I don't have a strong opinion here but maybe we leave it and see what impact it has on benchmarks (?).

Yeah, I'm not sure about the fsync calls here neither

fsync on one file might lead to fsync for all files on some filesystems (see irmin-pack: inverstigate performance regression with GC #1959 (comment));

we might have to call fsyncon the parent directory because, according to https://man7.org/linux/man-pages/man2/fdatasync.2.html:

Calling fsync() does not necessarily ensure that the entry in the directory containing the file has also reached disk. For that an explicit fsync() on a file descriptor for the directory is also needed.

So I think we should remove the fsyncs from the main process, and do an fsync on the root at the end of each Gc (similar to what we have now). We only guarantee that data is not lost if it was added before the last gc.

Are you also saying that you want to remove the fsync on the suffix before creating the new chunk? I still think that is a good idea (I haven't thought through all the consequences otherwise), but either way, I think we have identified a place that needs more careful consideration.

Also, you might be saying this for fsyncing the directory but just to be on the same page, I think that only applies when you are adding new files since the language of the manual is ensure that the entry in the directory. From Ensuring data reaches disk:

The more subtle usages deal with newly created files, or overwriting existing files. A newly created file may require an fsync() of not just the file itself, but also of the directory in which it was created (since this is where the file system looks to find your file). This behavior is actually file system (and mount option) dependent. You can either code specifically for each file system and mount option combination, or just perform fsync() calls on the directories to ensure that your code is portable.

This would apply for the new chunk but not the control file.

Indeed, I think we need to think a bit more about fsync here. For now I removed it.

Are you also saying that you want to remove the fsync on the suffix before creating the new chunk? I still think that is a good idea (I haven't thought through all the consequences otherwise), but either way, I think we have identified a place that needs more careful consideration.

Yes, I think it's not a good idea to call fsync at all in the main process. If we leave all fsync to the gc process, we can guarantee durability for data up to the last gc. I think this is what we have now too?

Also, you might be saying this for fsyncing the directory but just to be on the same page, I think that only applies when you are adding new files since the language of the manual is ensure that the entry in the directory.
This would apply for the new chunk but not the control file.

Indeed, it only applies to new chunk files. But our API lets the user call split several times before a call to gc. If we do add fsync to the control file here, we have to call fsync for all the directory.

👍 I definitely understand the hesitancy to introduce an fsync in the main process. My main concern is that if we do not fsync here, we have no control file field to help us after a crash. If the N-1 chunk didn't flush all the way to disk, it will open fine (will pass the consistency check bc we will just pass the length of the chunk as end_poff) but actually not be correct. This has me thinking more about suffix_end_poff and whether we should consider moving to storing the length of the suffix that has been persisted in order to check that for consistency on open. Let's keep this as an open discussion (particularly if we need to track more information in the control file).

icristescu · 2022-10-24T13:00:58Z

Rebased, this is ready for another round of reviews.

src/irmin-pack/unix/chunked_suffix.ml

src/irmin-pack/unix/file_manager.ml

Ngoguey42 · 2022-10-24T14:58:22Z

src/irmin-pack/unix/chunked_suffix.ml

+    let length_from_chunk suffix_off ao =
+      let chunk_len = Ao.end_poff ao in
+      Int63.Syntax.(suffix_off + chunk_len)


The length_from_chunk name seem to suggest that this function returns the length of the chunk. What about
end_offset_of_chunk? The first parameter could be named start_offset

Ngoguey42 · 2022-10-24T15:17:27Z

src/irmin-pack/unix/file_manager.ml

+               (* TODO: remove this last check, kept for passing the tests. *)
+            || gen0 <> gen1


Why should this be removed? The generation may change independently from the chunks

Ah never mind. this branch doesn't modify the suffix/mappin

Ngoguey42 · 2022-10-24T15:25:16Z

src/irmin-pack/unix/gc.ml

@@ -235,8 +235,7 @@ module Make (Args : Gc_args.S) = struct
    | Error (`Msg error) -> Error (`Corrupted_gc_result_file error)
    | Ok ok -> ok |> Result.map_error gc_error

-  let clean_after_abort t =
-    Fm.cleanup ~root:t.root ~generation:(t.generation - 1)
+  let clean_after_abort t = Fm.cleanup t.fm ~generation:(t.generation - 1)


This is the only place where Fn.cleanup is called. IINM, the the generation inside fm is t.generation - 1, which means that passing generation is not necessary.

indeed, thanks

metanivek

A couple of small documentation comments but this LGTM!

metanivek · 2022-10-25T16:09:12Z

CHANGES.md

+  - Add `split` to create a new append-only file on disk, for the following
+    writes. (#2118, @icristescu)


Suggested change

- Add `split` to create a new append-only file on disk, for the following

writes. (#2118, @icristescu)

- Add `split` to create a new suffix chunk. Subsequent writes will append to

this chunk until `split` is called again. (#2118, @icristescu)

I'm not sure what the exact wording should be, but trying to coordinate with the previous entry I added to describe the on-disk format change. We can always revisit the change log in a future PR too.

metanivek · 2022-10-25T16:12:09Z

src/irmin-pack/unix/s.ml

+      stores that allow gc. Raises [RO_Not_Allowed] if called by a readonly
+      instance. *)


Should also mention Split_forbidden_during_batch and Multiple_empty_chunks.

@Ngoguey42

…ils, irmin-test, irmin-pack, irmin-mirage, irmin-mirage-graphql, irmin-mirage-git, irmin-http, irmin-graphql, irmin-git, irmin-fs, irmin-containers, irmin-cli, irmin-chunk and irmin-bench (3.5.0) CHANGES: ### Added - **irmin-pack** - Add `Irmin_pack_unix.Stats.Latest_gc` which is now the parameter of GC's `finished` callback (mirage/irmin#2089, @Ngoguey42) - Add `Gc.oldest_live_commit` which returns the key of the commit on which the latest gc was called on. (mirage/irmin#2110, @icristescu) - Add `split` to create a new suffix chunk. Subsequent writes will append to this chunk until `split` is called again. (mirage/irmin#2118, @icristescu) - Add `create_one_commit_store` to create a new store from the existing one, containing only one commit. (mirage/irmin#2125, @icristescu) ### Changed - **irmin-pack** - Upgraded on-disk format to version 4. (mirage/irmin#2110, @icristescu) - Detecting control file corruption with a checksum (mirage/irmin#2119, @art-w) - Change on-disk layout of the suffix from a single file to a multiple, chunked file design (mirage/irmin#2115, @metanivek) - Modify GC to work with new chunked suffix. See `examples/gc.ml` for a demonstration of how it works with the new `split` function. (mirage/irmin#2126, @metanivek) ### Fixed

@Ngoguey42

…ils, irmin-test, irmin-pack, irmin-mirage, irmin-mirage-graphql, irmin-mirage-git, irmin-http, irmin-graphql, irmin-git, irmin-fs, irmin-containers, irmin-cli, irmin-chunk and irmin-bench (3.5.0) CHANGES: ### Added - **irmin-pack** - Add `Irmin_pack_unix.Stats.Latest_gc` which is now the parameter of GC's `finished` callback (mirage/irmin#2089, @Ngoguey42) - Add `Gc.oldest_live_commit` which returns the key of the commit on which the latest gc was called on. (mirage/irmin#2110, @icristescu) - Add `split` to create a new suffix chunk. Subsequent writes will append to this chunk until `split` is called again. (mirage/irmin#2118, @icristescu) - Add `create_one_commit_store` to create a new store from the existing one, containing only one commit. (mirage/irmin#2125, @icristescu) ### Changed - **irmin-pack** - Upgraded on-disk format to version 4. (mirage/irmin#2110, @icristescu) - Detecting control file corruption with a checksum (mirage/irmin#2119, @art-w) - Change on-disk layout of the suffix from a single file to a multiple, chunked file design (mirage/irmin#2115, @metanivek) - Modify GC to work with new chunked suffix. See `examples/gc.ml` for a demonstration of how it works with the new `split` function. (mirage/irmin#2126, @metanivek) ### Fixed

icristescu mentioned this pull request Oct 14, 2022

Release 3.5 #2096

Closed

34 tasks

metanivek reviewed Oct 14, 2022

View reviewed changes

icristescu force-pushed the split branch 2 times, most recently from 2763598 to 8250ba3 Compare October 17, 2022 14:21

icristescu mentioned this pull request Oct 17, 2022

irmin-pack: introduce chunked suffix abstraction #2115

Merged

icristescu force-pushed the split branch 2 times, most recently from c950da8 to 73df70b Compare October 18, 2022 14:01

metanivek reviewed Oct 18, 2022

View reviewed changes

icristescu force-pushed the split branch 2 times, most recently from 3f6c87f to b91cc50 Compare October 24, 2022 12:57

icristescu changed the title ~~(WIP) Add split function to create a new chunk~~ Add split function to create a new chunk Oct 24, 2022

metanivek reviewed Oct 24, 2022

View reviewed changes

src/irmin-pack/unix/chunked_suffix.ml Outdated Show resolved Hide resolved

src/irmin-pack/unix/file_manager.ml Show resolved Hide resolved

Ngoguey42 approved these changes Oct 24, 2022

View reviewed changes

icristescu force-pushed the split branch from 8de5e4e to 2be7958 Compare October 24, 2022 16:22

Ngoguey42 approved these changes Oct 25, 2022

View reviewed changes

metanivek approved these changes Oct 25, 2022

View reviewed changes

metanivek mentioned this pull request Oct 25, 2022

irmin-pack: update GC to work with chunked suffix #2126

Merged

Add a split function to create new chunk file

16c4572

icristescu force-pushed the split branch from 2be7958 to 16c4572 Compare October 26, 2022 12:44

icristescu merged commit 3dd2231 into mirage:main Oct 26, 2022

icristescu deleted the split branch October 26, 2022 12:53

metanivek added this to the Irmin 3.5 milestone Oct 26, 2022

metanivek mentioned this pull request Nov 10, 2022

irmin-pack: fix file manager cleanup #2136

Merged

metanivek mentioned this pull request Dec 14, 2022

[new release] irmin project (3.5.0) ocaml/opam-repository#22670

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add split function to create a new chunk #2118

Add split function to create a new chunk #2118

icristescu commented Oct 14, 2022

metanivek left a comment

metanivek Oct 14, 2022

codecov-commenter commented Oct 17, 2022 •

edited

Loading

metanivek left a comment

metanivek Oct 18, 2022

icristescu Oct 19, 2022

metanivek Oct 19, 2022 •

edited

Loading

icristescu Oct 24, 2022

metanivek Oct 24, 2022

icristescu commented Oct 24, 2022

Ngoguey42 Oct 24, 2022

Ngoguey42 Oct 24, 2022

Ngoguey42 Oct 24, 2022

Ngoguey42 Oct 24, 2022

icristescu Oct 24, 2022

metanivek left a comment

metanivek Oct 25, 2022

metanivek Oct 25, 2022

		(* TODO: remove this last check, kept for passing the tests. *)
		\|\| gen0 <> gen1

		- Add `split` to create a new append-only file on disk, for the following
		writes. (#2118, @icristescu)

		stores that allow gc. Raises [RO_Not_Allowed] if called by a readonly
		instance. *)

Add split function to create a new chunk #2118

Add split function to create a new chunk #2118

Conversation

icristescu commented Oct 14, 2022

metanivek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 17, 2022 • edited Loading

Codecov Report

metanivek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metanivek Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icristescu commented Oct 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metanivek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 17, 2022 •

edited

Loading

metanivek Oct 19, 2022 •

edited

Loading