From f6cec0bcaf560fa310853ad3fe17022602b63d5f Mon Sep 17 00:00:00 2001 From: Evan Klitzke Date: Thu, 15 Mar 2018 06:54:11 -0700 Subject: [PATCH 1/6] util: Refactor FileCommit from an #if sequence nested in #else, to a sequence of #elif This should not change the actual code generation. --- src/util/system.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/util/system.cpp b/src/util/system.cpp index f5931b5ff1..2dbf13f6b5 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -1024,23 +1024,21 @@ bool FileCommit(FILE *file) LogPrintf("%s: FlushFileBuffers failed: %d\n", __func__, GetLastError()); return false; } -#else - #if HAVE_FDATASYNC +#elif HAVE_FDATASYNC if (fdatasync(fileno(file)) != 0 && errno != EINVAL) { // Ignore EINVAL for filesystems that don't support sync LogPrintf("%s: fdatasync failed: %d\n", __func__, errno); return false; } - #elif defined(MAC_OSX) && defined(F_FULLFSYNC) +#elif defined(MAC_OSX) && defined(F_FULLFSYNC) if (fcntl(fileno(file), F_FULLFSYNC, 0) == -1) { // Manpage says "value other than -1" is returned on success LogPrintf("%s: fcntl F_FULLFSYNC failed: %d\n", __func__, errno); return false; } - #else +#else if (fsync(fileno(file)) != 0 && errno != EINVAL) { LogPrintf("%s: fsync failed: %d\n", __func__, errno); return false; } - #endif #endif return true; } From 844d650eea3bd809884cc5dd996a388bdc58314e Mon Sep 17 00:00:00 2001 From: Evan Klitzke Date: Thu, 15 Mar 2018 06:54:11 -0700 Subject: [PATCH 2/6] util: Prefer Mac-specific F_FULLSYNC over fdatasync in FileCommit --- src/util/system.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/util/system.cpp b/src/util/system.cpp index 2dbf13f6b5..01c588b0d3 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -1024,16 +1024,16 @@ bool FileCommit(FILE *file) LogPrintf("%s: FlushFileBuffers failed: %d\n", __func__, GetLastError()); return false; } -#elif HAVE_FDATASYNC - if (fdatasync(fileno(file)) != 0 && errno != EINVAL) { // Ignore EINVAL for filesystems that don't support sync - LogPrintf("%s: fdatasync failed: %d\n", __func__, errno); - return false; - } #elif defined(MAC_OSX) && defined(F_FULLFSYNC) if (fcntl(fileno(file), F_FULLFSYNC, 0) == -1) { // Manpage says "value other than -1" is returned on success LogPrintf("%s: fcntl F_FULLFSYNC failed: %d\n", __func__, errno); return false; } +#elif HAVE_FDATASYNC + if (fdatasync(fileno(file)) != 0 && errno != EINVAL) { // Ignore EINVAL for filesystems that don't support sync + LogPrintf("%s: fdatasync failed: %d\n", __func__, errno); + return false; + } #else if (fsync(fileno(file)) != 0 && errno != EINVAL) { LogPrintf("%s: fsync failed: %d\n", __func__, errno); From ce5cbaea63ad4ea78e533bdb14f47f414061ae7f Mon Sep 17 00:00:00 2001 From: Evan Klitzke Date: Thu, 15 Mar 2018 06:54:11 -0700 Subject: [PATCH 3/6] util.h: Document FileCommit function --- src/util/system.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/util/system.h b/src/util/system.h index 1df194ca84..d2373998df 100644 --- a/src/util/system.h +++ b/src/util/system.h @@ -56,6 +56,11 @@ bool error(const char* fmt, const Args&... args) } void PrintExceptionContinue(const std::exception *pex, const char* pszThread); + +/** + * Ensure file contents are fully committed to disk, using a platform-specific + * feature analogous to fsync(). + */ bool FileCommit(FILE *file); bool TruncateFile(FILE *file, unsigned int length); int RaiseFileDescriptorLimit(int nMinFD); From 220bb16cbee5b91d0bc0fcc6c71560d631295fa5 Mon Sep 17 00:00:00 2001 From: Evan Klitzke Date: Thu, 15 Mar 2018 06:54:11 -0700 Subject: [PATCH 4/6] util: Introduce DirectoryCommit commit function to sync a directory --- src/util/system.cpp | 9 +++++++++ src/util/system.h | 7 +++++++ 2 files changed, 16 insertions(+) diff --git a/src/util/system.cpp b/src/util/system.cpp index 01c588b0d3..43c6ae54ab 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -1043,6 +1043,15 @@ bool FileCommit(FILE *file) return true; } +void DirectoryCommit(const fs::path &dirname) +{ +#ifndef WIN32 + FILE* file = fsbridge::fopen(dirname, "r"); + fsync(fileno(file)); + fclose(file); +#endif +} + bool TruncateFile(FILE *file, unsigned int length) { #if defined(WIN32) return _chsize(_fileno(file), length) == 0; diff --git a/src/util/system.h b/src/util/system.h index d2373998df..029204f01e 100644 --- a/src/util/system.h +++ b/src/util/system.h @@ -62,6 +62,13 @@ void PrintExceptionContinue(const std::exception *pex, const char* pszThread); * feature analogous to fsync(). */ bool FileCommit(FILE *file); + +/** + * Sync directory contents. This is required on some environments to ensure that + * newly created files are committed to disk. + */ +void DirectoryCommit(const fs::path &dirname); + bool TruncateFile(FILE *file, unsigned int length); int RaiseFileDescriptorLimit(int nMinFD); void AllocateFileRange(FILE *file, unsigned int offset, unsigned int length); From 457490403853321d308c6ca6aaa90d6f8f29b4cf Mon Sep 17 00:00:00 2001 From: Evan Klitzke Date: Thu, 15 Mar 2018 06:54:11 -0700 Subject: [PATCH 5/6] 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. --- src/flatfile.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/flatfile.cpp b/src/flatfile.cpp index 8a8f7b681c..11cf357f3d 100644 --- a/src/flatfile.cpp +++ b/src/flatfile.cpp @@ -92,6 +92,7 @@ bool FlatFileSeq::Flush(const FlatFilePos& pos, bool finalize) fclose(file); return error("%s: failed to commit file %d", __func__, pos.nFile); } + DirectoryCommit(m_dir); fclose(file); return true; From ef712298c3f8bc2afdad783f05080443b72b3f77 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Wed, 17 Oct 2018 08:34:53 +0000 Subject: [PATCH 6/6] util: Check for file being NULL in DirectoryCommit --- src/util/system.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/util/system.cpp b/src/util/system.cpp index 43c6ae54ab..2ed9cf7048 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -1047,8 +1047,10 @@ void DirectoryCommit(const fs::path &dirname) { #ifndef WIN32 FILE* file = fsbridge::fopen(dirname, "r"); - fsync(fileno(file)); - fclose(file); + if (file) { + fsync(fileno(file)); + fclose(file); + } #endif }