misc fixes for file syncing
f16a9739fdd4
Actions

Description

misc fixes for file syncing

Summary:
This fixes a possible (unlikely) data race when commiting block files.

rationale from core#12696

It was recently pointed out to me that calling fsync() or fdatasync() on a new file is not sufficient to ensure it's persisted to disk, as the existence of the file itself is stored in the directory inode. This means that ensuring that a new file is actually committed also requires an fsync() on the parent directory. This only applies to new files, calling fsync() on an old file is always fine.

In theory this means that the block index can race block persistence, as a poorly timed power outage could cause us to commit data to the block index that gets lost in the filesystem. The change here ensures that we call fsync() on the blocks directory after committing new block files. I checked the LevelDB source code and they already do this when updating their writeahead log. In theory this could happen at the same time as a chain split and that could cause you to come back online and then miss the block you had committed to the index, which would put you permanently out of sync between the two. This seems pretty far fetched, but we should handle this case correctly anyway.

Prefer Mac-specific F_FULLSYNC over fdatasync in FileCommit, as MacOS can erroneously report that it has fdatasync. See discussion here:
https://github.com/haskell/unix/issues/37
This was not an issue before this diff, because HAVE_FDATASYNC was never properly defined (see next point below).

Define and use HAVE_FDATASYNC correctly. Previously it was never defined and FileCommit always used fsync. This was not a big problem, it just caused a small performance loss, but if we want to maintain this code path it is better if it actually works.

This is a backport of core#14501
Depends on D16151

Test Plan:
ninja all check-all

Before and after this commit, apply this patch, run bitcoind and check that HAVE_FDATASYNC was previously not defined but now it is:

--- a/src/bitcoind.cpp
+++ b/src/bitcoind.cpp
@@ -314,9 +314,13 @@ static bool AppInit(int argc, char *argv[]) {
 }

 int main(int argc, char *argv[]) {
-#ifdef WIN32
-    util::WinCmdLineArgs winArgs;
-    std::tie(argc, argv) = winArgs.get();
+#if defined(HAVE_FDATASYNC)
+    LogPrintf("\n\n#### HAVE_FDATASYNC defined\n\n");
+#endif
+#if HAVE_FDATASYNC
+    LogPrintf("\n\n#### HAVE_FDATASYNC\n\n");
+#else
+    LogPrintf("\n\n##### not HAVE_FDATASYNC\n\n");
 #endif
     SetupEnvironment();