Add fsync for directory #2132

icristescu · 2022-11-07T12:13:59Z

Calling fsync on newly created files is not enough - the corresponding entry in the containing directory might not make to disk. An explicit fsync() on a file descriptor for the directory is also needed (from https://man7.org/linux/man-pages/man2/fsync.2.html).

We need this for instance after creating the snapshot directory in #2125.

I am not very confident in this patch, so any feedback is very welcome.

icristescu · 2022-11-07T14:16:31Z

Rebased over #2125 to include the usage for this function.

Ngoguey42 · 2022-11-08T09:08:53Z

I now understand what you were trying to achieve. It covers the situation where the computer would power out just after the end of the snapshot export program.

I'm not against that diff.

Another solution would maybe be to put all these fsyncs in repo's close routine

metanivek

👍 I'm in favor of making our fsyncs more robust. Once this lands, lets consider modifying the gc worker to do the same.

src/irmin-pack/unix/s.ml

metanivek · 2022-11-08T14:35:31Z

src/irmin-pack/unix/ext.ml

+            (* Gc worker always fsyncs prefix and mapping files. File_manager
+               fsyncs dict, control, suffix and index. Fsync branch file here. Then
+               fsync directory. *)
+            let use_fsync = Irmin_pack.Conf.use_fsync t.config in
+            let () = if use_fsync then Branch.fsync branch_store in
+            let* () = Branch.close branch_store in
+            let () = Io.fsync_dir path |> Errs.raise_if_error in


For a future when file manager also manages the branch store (#2125 (comment)), all this code could live in File_manager.create_one_commit_store which would be cleaner and only require a comment about GC handling prefix/mapping files.

yes, agree, these sort of comment show that there is an issue in the structure of the code

metanivek · 2022-11-08T14:42:00Z

src/irmin-pack/unix/io.ml

+  let fsync_dir path =
+    try
+      let dirfd =
+        Unix.openfile path Unix.[ O_RDONLY; O_SYNC; O_RSYNC ] default_open_perm


It probably doesn't matter but I'm not sure that we need the two *SYNC flags when opening a directory.

I'm not sure either :) I read the doc on O_RSYNC (reads complete as writes (depending on O_SYNC/O_DSYNC)) as in both flags are necessary.

I read a little more on these flags (like https://www.man7.org/linux/man-pages/man2/open.2.html).

I think you are right that O_RSYNC depends on the presence of either O_SYNC or O_DSYNC, but it appears they only apply to regular files. So I think we can remove them when opening a file descriptor for a directory.

I also learned that O_RSYNC isn't implemented in Linux

Linux implements O_SYNC and O_DSYNC, but not O_RSYNC. Somewhat
incorrectly, glibc defines O_RSYNC to have the same value as
O_SYNC. (O_RSYNC is defined in the Linux header file
<asm/fcntl.h> on HP PA-RISC, but it is not used.)

but I found this for the IBM i operating system

O_RSYNC
Read operations to the file will be performed synchronously. Pending update requests affecting the data to be read are written to permanent storage. This flag is used in combination with O_SYNC or O_DSYNC. When O_RSYNC and O_SYNC are set, all file data and file attributes are written to permanent storage before the read operation returns. When O_RSYNC and O_DSYNC are set, all file data is written to permanent storage before the read operation returns.

src/irmin-pack/unix/io.ml

icristescu · 2022-11-08T17:02:34Z

I now understand what you were trying to achieve. I covers the situation where the computer would power out just after the end of the snapshot export program.

Hm, I though it was necessary, but reading your comment, I realise that it's maybe an overkill :) But the fsync_dir function is necessary in the case of fsync files created by split. I can keep only the fsync_dir function and remove its call from the export. Wdyt?

Ngoguey42 · 2022-11-09T13:15:26Z

My understanding of fsync is that calling it is only necessary to ensure persistence for situations where the system brutally turns off. In other words: if the computer never shuts down, fsync can be seen as a no-op.

The page cache being shared by all processes, it ensures that the files created by a process are immediately visible by other processes. The same goes for modifications of directories: A modification to a directory is first reflected in the page cache before being persisted to disk

icristescu · 2022-11-09T15:16:38Z

I removed the fsync_dir for the export snapshot, and added where I think it would make maybe more sense (in the gc_worker, where new files are created). But please feel free to close this PR if you think its unnecessary.

codecov-commenter · 2022-11-09T15:27:07Z

Codecov Report

Merging #2132 (5c7ddf0) into main (3dd2231) will decrease coverage by 0.09%.
The diff coverage is 50.56%.

@@            Coverage Diff             @@
##             main    #2132      +/-   ##
==========================================
- Coverage   68.21%   68.12%   -0.10%     
==========================================
  Files         134      134              
  Lines       16031    16048      +17     
==========================================
- Hits        10935    10932       -3     
- Misses       5096     5116      +20

Impacted Files	Coverage Δ
src/irmin-pack/unix/io.ml	`61.53% <0.00%> (-4.23%)`	⬇️
src/irmin-pack/unix/pack_index.ml	`63.88% <ø> (ø)`
src/irmin-pack/unix/gc_worker.ml	`6.01% <14.81%> (+2.25%)`	⬆️
src/irmin-pack/unix/chunked_suffix.ml	`85.84% <52.17%> (-10.63%)`	⬇️
src/irmin-pack/unix/file_manager.ml	`83.92% <75.00%> (-0.26%)`	⬇️
src/irmin-pack/unix/dispatcher.ml	`73.97% <86.66%> (-1.03%)`	⬇️
src/irmin-pack/unix/gc.ml	`79.16% <90.90%> (-2.98%)`	⬇️
src/irmin-pack/unix/ext.ml	`62.25% <100.00%> (-0.15%)`	⬇️
src/irmin-pack/unix/import.ml	`82.35% <100.00%> (+2.35%)`	⬆️
... and 8 more

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

metanivek · 2022-11-10T14:25:29Z

@Ngoguey42 you're correct in that fsync (and friends) is used to attempt durability guarantees and/or ordering of writes by forcing data to physical storage when it is called. I say attempt because it seems (based on the research I've done) to be dependent on file systems and actual storage device setups (some might not actually flush to disk, for example).

I have no strong opinion of whether we want to have this kind of durability guarantee in a situation like the snapshot export. My hunch is maybe it does not matter.

I think that adding fsync of a directory when new files have been added serves one main purpose. It is a more correct fsync strategy when a store has been configured to use fsync since we know that fsyncing just the file in this case is not sufficient (for some file systems / mount options 😅). Related, if we want to fsync files in the GC worker that have been created, we ought to also fsync 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.

Ngoguey42 · 2022-11-10T14:29:45Z

Random thought:

Would it be interesting to have all instances keep a file descriptor open on the directory at all time? We could then (somehow) use that file descriptor to read the content of the directory, instead of relying on paths.

Ngoguey42 · 2022-11-10T14:38:05Z

Also, I believe that the design i've written there #2082 can improve (ensure?) consistency without any fsync call.

icristescu force-pushed the fsync_dir branch from 56141bb to 280c826 Compare November 7, 2022 14:15

metanivek reviewed Nov 8, 2022

View reviewed changes

icristescu added the no-changelog-needed No changelog is needed here label Nov 8, 2022

fsync dir

5c7ddf0

icristescu force-pushed the fsync_dir branch from e208303 to 5c7ddf0 Compare November 9, 2022 15:12

metanivek approved these changes Nov 17, 2022

View reviewed changes

metanivek mentioned this pull request Mar 3, 2023

irmin-pack: Use temp control files upon writing #2206

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fsync for directory #2132

Add fsync for directory #2132

icristescu commented Nov 7, 2022

icristescu commented Nov 7, 2022

Ngoguey42 commented Nov 8, 2022 •

edited

metanivek left a comment

metanivek Nov 8, 2022

icristescu Nov 8, 2022

metanivek Nov 8, 2022

icristescu Nov 8, 2022

metanivek Nov 8, 2022

icristescu commented Nov 8, 2022 •

edited

Ngoguey42 commented Nov 9, 2022

icristescu commented Nov 9, 2022

codecov-commenter commented Nov 9, 2022

metanivek commented Nov 10, 2022

Ngoguey42 commented Nov 10, 2022 •

edited

Ngoguey42 commented Nov 10, 2022

Add fsync for directory #2132

Are you sure you want to change the base?

Add fsync for directory #2132

Conversation

icristescu commented Nov 7, 2022

icristescu commented Nov 7, 2022

Ngoguey42 commented Nov 8, 2022 • edited

metanivek left a comment

Choose a reason for hiding this comment

metanivek Nov 8, 2022

Choose a reason for hiding this comment

icristescu Nov 8, 2022

Choose a reason for hiding this comment

metanivek Nov 8, 2022

Choose a reason for hiding this comment

icristescu Nov 8, 2022

Choose a reason for hiding this comment

metanivek Nov 8, 2022

Choose a reason for hiding this comment

icristescu commented Nov 8, 2022 • edited

Ngoguey42 commented Nov 9, 2022

icristescu commented Nov 9, 2022

codecov-commenter commented Nov 9, 2022

Codecov Report

metanivek commented Nov 10, 2022

Ngoguey42 commented Nov 10, 2022 • edited

Ngoguey42 commented Nov 10, 2022

Ngoguey42 commented Nov 8, 2022 •

edited

icristescu commented Nov 8, 2022 •

edited

Ngoguey42 commented Nov 10, 2022 •

edited