Fix possible data race when committing block files #14501

luke-jr · 2018-10-17T09:08:05Z

Reviving #12696

src/util.cpp

luke-jr · 2018-11-22T01:46:56Z

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

ken2812221 · 2018-11-22T01:51:28Z

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

luke-jr · 2018-11-22T01:53:18Z

git follows the move just fine.

maflcko · 2018-11-22T18:20:06Z

@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.

DrahtBot · 2019-09-30T12:54:44Z

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.

luke-jr · 2019-10-13T22:24:52Z

Rebased

DrahtBot · 2019-10-30T14:53:51Z

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

Conflicts

No conflicts as of last run.

adaminsky · 2020-08-07T05:50:36Z

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.

luke-jr · 2020-08-20T17:46:37Z

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

…sequence of #elif This should not change the actual code generation.

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.

… LevelDB c4b85ba Bugfix: Define and use HAVE_FDATASYNC correctly outside LevelDB (Luke Dashjr) Pull request description: Fixes a bug introduced in #19614 The LevelDB-specific fdatasync check was only using `AC_SUBST`, which works for Makefiles, but doesn't define anything for C++. Furthermore, the #define is typically 0 or 1, never undefined. This fixes both issues by defining it and checking its value instead of whether it is merely defined. Pulled out of #14501 by fanquake's request ACKs for top commit: fanquake: ACK c4b85ba - thanks for catching and fixing my mistake. laanwj: Code review ACK c4b85ba Tree-SHA512: 91d5d426ba000b4f3ee7e2315635e24bbb23ceff16269ddf4f65a63d25fc9e9cf94a3b236eed2f8031cc36ddcf78aeb5916efcb244f415943a8a12f907ede8f9

…outside LevelDB c4b85ba Bugfix: Define and use HAVE_FDATASYNC correctly outside LevelDB (Luke Dashjr) Pull request description: Fixes a bug introduced in bitcoin#19614 The LevelDB-specific fdatasync check was only using `AC_SUBST`, which works for Makefiles, but doesn't define anything for C++. Furthermore, the #define is typically 0 or 1, never undefined. This fixes both issues by defining it and checking its value instead of whether it is merely defined. Pulled out of bitcoin#14501 by fanquake's request ACKs for top commit: fanquake: ACK c4b85ba - thanks for catching and fixing my mistake. laanwj: Code review ACK c4b85ba Tree-SHA512: 91d5d426ba000b4f3ee7e2315635e24bbb23ceff16269ddf4f65a63d25fc9e9cf94a3b236eed2f8031cc36ddcf78aeb5916efcb244f415943a8a12f907ede8f9

Empact · 2020-09-02T17:28:02Z

src/util/system.cpp

+    FILE* file = fsbridge::fopen(dirname, "r");
+    if (file) {
+        fsync(fileno(file));
+        fclose(file);


How about log the possible errors here? E.g.

bitcoin/src/util/system.cpp

Lines 1033 to 1034 in 0adb80f

if (fsync(fileno(file)) != 0 && errno != EINVAL) {

LogPrintf("%s: fsync failed: %d\n", __func__, errno);

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).

laanwj · 2021-01-07T21:07:23Z

Code review ACK ef71229

ef71229 util: Check for file being NULL in DirectoryCommit (Luke Dashjr) 4574904 Fix possible data race when committing block files (Evan Klitzke) 220bb16 util: Introduce DirectoryCommit commit function to sync a directory (Evan Klitzke) ce5cbae util.h: Document FileCommit function (Evan Klitzke) 844d650 util: Prefer Mac-specific F_FULLSYNC over fdatasync in FileCommit (Evan Klitzke) f6cec0b util: Refactor FileCommit from an #if sequence nested in #else, to a sequence of #elif (Evan Klitzke) Pull request description: Reviving bitcoin#12696 ACKs for top commit: laanwj: Code review ACK ef71229 Tree-SHA512: 07d650990ef4c18d645dee3f9a199a940683ad17557d79d93979a76c4e710d8d70e6eae01d1a5991494a24a7654eb7db868be0c34a31e70b2509945d95bc9cce

luke-jr force-pushed the fsync_dir branch from 246c7b0 to 56483d1 Compare October 17, 2018 09:11

ken2812221 reviewed Oct 17, 2018

View reviewed changes

src/util.cpp Outdated Show resolved Hide resolved

fanquake added Block storage Data corruption labels Oct 17, 2018

luke-jr force-pushed the fsync_dir branch from 56483d1 to 62fb7bb Compare October 17, 2018 10:41

DrahtBot added the Needs rebase label Nov 5, 2018

DrahtBot added the Up for grabs label Sep 30, 2019

DrahtBot closed this Sep 30, 2019

meshcollider reopened this Oct 13, 2019

luke-jr force-pushed the fsync_dir branch from 62fb7bb to 425b654 Compare October 13, 2019 17:53

DrahtBot removed the Needs rebase label Oct 13, 2019

maflcko removed the Up for grabs label Oct 16, 2019

DrahtBot mentioned this pull request Jul 28, 2020

util: use HAVE_FDATASYNC to determine fdatasync() use #19614

Merged

DrahtBot added the Needs rebase label Aug 5, 2020

luke-jr force-pushed the fsync_dir branch from 425b654 to 3a0c9b5 Compare August 20, 2020 17:46

DrahtBot removed the Needs rebase label Aug 20, 2020

DrahtBot mentioned this pull request Aug 20, 2020

build: configure.ac and netbsd-build.md for NetBSD #19430

Closed

luke-jr and others added 4 commits August 25, 2020 16:46

Bugfix: Define and use HAVE_FDATASYNC correctly outside LevelDB

c4b85ba

util: Refactor FileCommit from an #if sequence nested in #else, to a …

f6cec0b

…sequence of #elif This should not change the actual code generation.

util: Prefer Mac-specific F_FULLSYNC over fdatasync in FileCommit

844d650

util.h: Document FileCommit function

ce5cbae

eklitzke and others added 3 commits August 25, 2020 16:46

util: Introduce DirectoryCommit commit function to sync a directory

220bb16

util: Check for file being NULL in DirectoryCommit

ef71229

luke-jr force-pushed the fsync_dir branch from 3a0c9b5 to ef71229 Compare August 25, 2020 16:48

luke-jr mentioned this pull request Aug 25, 2020

Bugfix: Define and use HAVE_FDATASYNC correctly outside LevelDB #19803

Merged

Empact reviewed Sep 2, 2020

View reviewed changes

laanwj merged commit 86a8b35 into bitcoin:master Jan 7, 2021

luke-jr mentioned this pull request Feb 28, 2021

More robust file/directory syncing and error handling #21313

Closed

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix possible data race when committing block files #14501

Fix possible data race when committing block files #14501

luke-jr commented Oct 17, 2018

Uh oh!

Uh oh!

luke-jr commented Nov 22, 2018

Uh oh!

ken2812221 commented Nov 22, 2018

Uh oh!

luke-jr commented Nov 22, 2018

Uh oh!

maflcko commented Nov 22, 2018

Uh oh!

DrahtBot commented Sep 30, 2019

Uh oh!

luke-jr commented Oct 13, 2019

Uh oh!

DrahtBot commented Oct 30, 2019 •

edited

Loading

Uh oh!

adaminsky commented Aug 7, 2020

Uh oh!

luke-jr commented Aug 20, 2020

Uh oh!

Empact Sep 2, 2020

Uh oh!

laanwj Dec 21, 2020 •

edited

Loading

Uh oh!

laanwj commented Jan 7, 2021

Uh oh!

Uh oh!

	if (fsync(fileno(file)) != 0 && errno != EINVAL) {
	LogPrintf("%s: fsync failed: %d\n", __func__, errno);

Fix possible data race when committing block files #14501

Fix possible data race when committing block files #14501

Conversation

luke-jr commented Oct 17, 2018

Uh oh!

Uh oh!

luke-jr commented Nov 22, 2018

Uh oh!

ken2812221 commented Nov 22, 2018

Uh oh!

luke-jr commented Nov 22, 2018

Uh oh!

maflcko commented Nov 22, 2018

Uh oh!

DrahtBot commented Sep 30, 2019

Uh oh!

luke-jr commented Oct 13, 2019

Uh oh!

DrahtBot commented Oct 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

adaminsky commented Aug 7, 2020

Uh oh!

luke-jr commented Aug 20, 2020

Uh oh!

Empact Sep 2, 2020

Choose a reason for hiding this comment

Uh oh!

laanwj Dec 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jan 7, 2021

Uh oh!

Uh oh!

DrahtBot commented Oct 30, 2019 •

edited

Loading

laanwj Dec 21, 2020 •

edited

Loading