Fix possible data race when committing block files #14501

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: member
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: member

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

    Conflicts

    No conflicts as of last run.

21. DrahtBot added the label Needs rebase on Aug 5, 2020
22. 
    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.

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

    Rebased and added a fix for #19614 (which was completely broken)
25. laanwj added this to the "Blockers" column in a project

    ---

26. DrahtBot removed the label Needs rebase on Aug 20, 2020
27. Bugfix: Define and use HAVE_FDATASYNC correctly outside LevelDB c4b85ba704
28. util: Refactor FileCommit from an #if sequence nested in #else, to a sequence of #elif

    This should not change the actual code generation.

    f6cec0bcaf
29. util: Prefer Mac-specific F_FULLSYNC over fdatasync in FileCommit 844d650eea
30. util.h: Document FileCommit function ce5cbaea63
31. util: Introduce DirectoryCommit commit function to sync a directory 220bb16cbe
32. 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
33. util: Check for file being NULL in DirectoryCommit ef712298c3
34. luke-jr force-pushed on Aug 25, 2020
35. fanquake referenced this in commit 0adb80fe63 on Aug 31, 2020
36. sidhujag referenced this in commit b58c80dbee on Aug 31, 2020
37. 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).
38. 
    laanwj commented at 9:07 pm on January 7, 2021: member

    Code review ACK ef712298c3f8bc2afdad783f05080443b72b3f77
39. laanwj merged this on Jan 7, 2021
40. laanwj closed this on Jan 7, 2021

    ---

41. laanwj removed this from the "Blockers" column in a project

    ---

42. sidhujag referenced this in commit c01fae845f on Jan 7, 2021
43. DrahtBot locked this on Aug 16, 2022

Contributors
luke-jr ken2812221 MarcoFalke DrahtBot adaminsky Empact laanwj

Labels
Block storage Data corruption