Fix possible data race when committing block files #14501

pull luke-jr wants to merge 7 commits into bitcoin:master from luke-jr:fsync_dir changing 4 files +32 −10
  1. luke-jr commented at 9:08 AM on October 17, 2018: member

    Reviving #12696

  2. luke-jr force-pushed on Oct 17, 2018
  3. in src/util.cpp:1022 in 90cdaef77b outdated 
    1018 | @@ -1019,16 +1019,16 @@ bool FileCommit(FILE *file)
1019 |          LogPrintf("%s: FlushFileBuffers failed: %d\n", __func__, GetLastError());
1020 |          return false;
1021 |      }
1022 | +#if defined(MAC_OSX) && defined(F_FULLFSYNC)
    ken2812221 commented at 9:52 AM on October 17, 2018:

    It's impossible to both define WIN32 and MAC_OSX at the same time. Should be elif

    luke-jr commented at 10:41 AM on October 17, 2018:

    This was meant to be an #elif, fixed.

  4. fanquake added the label Block storage on Oct 17, 2018
  5. fanquake added the label Data corruption on Oct 17, 2018
  6. luke-jr force-pushed on Oct 17, 2018
  7. DrahtBot added the label Needs rebase on Nov 5, 2018
  8. luke-jr commented at 1:46 AM on November 22, 2018: member

    (GitHub and DrahtBot are misdetecting a rebase needed here; it is a clean merge still... can't at least DrahtBot get fixed??)

  9. ken2812221 commented at 1:51 AM on November 22, 2018: contributor

    You have to move src/util.cpp to src/util/system.cpp?

  10. luke-jr commented at 1:53 AM on November 22, 2018: member

    git follows the move just fine.

  11. MarcoFalke commented at 6:20 PM on November 22, 2018: member

    @luke-jr There are different merge strategies and flags (https://git-scm.com/docs/git-merge#git-merge-mergetool) and GitHub uses a rather strict one, which they were unable to disclose to me.

    It appear that this pull request hasn't had any review yet, so doing a rebase comes at no cost at all.

  12. DrahtBot added the label Up for grabs on Sep 30, 2019
  13. DrahtBot commented at 12:54 PM on September 30, 2019: contributor

    <!--5fd3d806e98f4a0ca80977bb178665a0-->There hasn't been much activity lately and the patch still needs rebase, so I am closing this for now. Please let me know when you want to continue working on this, so the pull request can be re-opened.

  14. DrahtBot closed this on Sep 30, 2019
  15. meshcollider reopened this on Oct 13, 2019
  16. luke-jr force-pushed on Oct 13, 2019
  17. DrahtBot removed the label Needs rebase on Oct 13, 2019
  18. luke-jr commented at 10:24 PM on October 13, 2019: member

    Rebased

  19. MarcoFalke removed the label Up for grabs on Oct 16, 2019
  20. DrahtBot commented at 2:53 PM on October 30, 2019: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  21. DrahtBot cross-referenced this on Jul 28, 2020 from issue util: use HAVE_FDATASYNC to determine fdatasync() use by fanquake
  22. DrahtBot added the label Needs rebase on Aug 5, 2020
  23. adaminsky commented at 5:50 AM on August 7, 2020: contributor

    Is this still being worked on? With #19614 recently merged, all that needs to be done is add the DirectoryCommit function. I'm happy to help.

    I saw the previous discussion about if calling fsync on an unchanged directory has overhead, and my understanding is that the directory's dirty bit will not be set, so no disk writes will occur. Therefore, the current method looks good to me.

  24. luke-jr force-pushed on Aug 20, 2020
  25. luke-jr commented at 5:46 PM on August 20, 2020: member

    Rebased and added a fix for #19614 (which was completely broken)

  26. laanwj added this to the "Blockers" column in a project
  27. DrahtBot removed the label Needs rebase on Aug 20, 2020
  28. DrahtBot cross-referenced this on Aug 20, 2020 from issue build: configure.ac and netbsd-build.md for NetBSD by midnightmagic
  29. Bugfix: Define and use HAVE_FDATASYNC correctly outside LevelDB c4b85ba704
  30. util: Refactor FileCommit from an #if sequence nested in #else, to a sequence of #elif
    This should not change the actual code generation.
    f6cec0bcaf
  31. util: Prefer Mac-specific F_FULLSYNC over fdatasync in FileCommit 844d650eea
  32. util.h: Document FileCommit function ce5cbaea63
  33. util: Introduce DirectoryCommit commit function to sync a directory 220bb16cbe
  34. Fix possible data race when committing block files
    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, a 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 change ensures that we call fsync() on the blocks directory after
committing new block files.
    4574904038
  35. util: Check for file being NULL in DirectoryCommit ef712298c3
  36. luke-jr force-pushed on Aug 25, 2020
  37. luke-jr cross-referenced this on Aug 25, 2020 from issue Bugfix: Define and use HAVE_FDATASYNC correctly outside LevelDB by luke-jr
  38. fanquake referenced this in commit 0adb80fe63 on Aug 31, 2020
  39. sidhujag referenced this in commit b58c80dbee on Aug 31, 2020
  40. in src/util/system.cpp:1052 in ef712298c3 
    1056 | +{
1057 | +#ifndef WIN32
1058 | +    FILE* file = fsbridge::fopen(dirname, "r");
1059 | +    if (file) {
1060 | +        fsync(fileno(file));
1061 | +        fclose(file);
    Empact commented at 5:28 PM on September 2, 2020:

    How about log the possible errors here? E.g. https://github.com/bitcoin/bitcoin/blob/0adb80fe630ccaf3ab4577ad1070d35c2dd807d8/src/util/system.cpp#L1033-L1034

    laanwj commented at 11:48 AM on December 21, 2020:

    If you do that, please restrict it to logging once. Logging fsync errors every time can get very verbose on some network file systems otherwise, which have lots of corner cases with regard to synchronization (more so on directories, we had to patch leveldb for this once).

  41. laanwj commented at 9:07 PM on January 7, 2021: member

    Code review ACK ef712298c3f8bc2afdad783f05080443b72b3f77

  42. laanwj merged this on Jan 7, 2021
  43. laanwj closed this on Jan 7, 2021
  44. laanwj removed this from the "Blockers" column in a project
  45. sidhujag referenced this in commit c01fae845f on Jan 7, 2021
  46. luke-jr cross-referenced this on Feb 28, 2021 from issue More robust file/directory syncing and error handling by luke-jr
  47. bitcoin locked this on Aug 16, 2022
Contributors
luke-jrken2812221MarcoFalkeDrahtBotadaminskyEmpactlaanwj
Labels
Block storageData corruption
Linked (view graph)
#12696 Fix possible data race when committing block files#19803 Bugfix: Define and use HAVE_FDATASYNC correctly outside LevelDB#21313 More robust file/directory syncing and error handling
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:54 UTC