Page MenuHomePhabricator

misc fixes for file syncing
ClosedPublic

Authored by PiRK on May 14 2024, 07:51.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABCf16a9739fdd4: 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();

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK requested review of this revision.May 14 2024, 07:51
PiRK edited the test plan for this revision. (Show Details)

@bot build-win64 build-linux64 build-osx

This revision is now accepted and ready to land.May 14 2024, 09:22
This revision was automatically updated to reflect the committed changes.