Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add fsync for directory #2132

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

icristescu
Copy link
Contributor

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
Copy link
Contributor Author

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

@Ngoguey42
Copy link
Contributor

Ngoguey42 commented Nov 8, 2022

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

Copy link
Member

@metanivek metanivek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 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 Outdated Show resolved Hide resolved
Comment on lines 379 to 385
(* 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
@icristescu
Copy link
Contributor Author

icristescu commented Nov 8, 2022

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?

@icristescu icristescu added the no-changelog-needed No changelog is needed here label Nov 8, 2022
@Ngoguey42
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link

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
Copy link
Member

@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
Copy link
Contributor

Ngoguey42 commented Nov 10, 2022

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog-needed No changelog is needed here
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants