Add logging and error handling for file syncing

laanwj · furszy · commit 4be426fb9bb0 · 2021-04-09T01:03:05.000-03:00
Add logging and error handling inside, and outside of FileCommit. Functions such as fsync, fdatasync will return error in case of hardware I/O errors, and ignoring this means it can silently continue through data corruption. (c.f. https://lwn.net/SubscriberLink/752063/12b232ab5039efbe/)
diff --git a/src/addrdb.cpp b/src/addrdb.cpp
@@ -51,7 +51,8 @@ bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data
 
     // Serialize
     if (!SerializeDB(fileout, data)) return false;
-    FileCommit(fileout.Get());
+    if (!FileCommit(fileout.Get()))
+        return error("%s: Failed to flush file %s", __func__, pathTmp.string());
     fileout.fclose();
 
     // replace existing file, if any, with new file
diff --git a/src/util.cpp b/src/util.cpp
@@ -638,21 +638,37 @@ bool TryCreateDirectory(const fs::path& p)
     return false;
 }
 
-void FileCommit(FILE* fileout)
+bool FileCommit(FILE* file)
 {
-    fflush(fileout); // harmless if redundantly called
+    if (fflush(file) != 0) { // harmless if redundantly called
+        LogPrintf("%s: fflush failed: %d\n", __func__, errno);
+        return false;
+    }
 #ifdef WIN32
-    HANDLE hFile = (HANDLE)_get_osfhandle(_fileno(fileout));
-    FlushFileBuffers(hFile);
-#else
-#if defined(__linux__) || defined(__NetBSD__)
-    fdatasync(fileno(fileout));
-#elif defined(__APPLE__) && defined(F_FULLFSYNC)
-    fcntl(fileno(fileout), F_FULLFSYNC, 0);
+    HANDLE hFile = (HANDLE)_get_osfhandle(_fileno(file));
+    if (FlushFileBuffers(hFile) == 0) {
+        LogPrintf("%s: FlushFileBuffers failed: %d\n", __func__, GetLastError());
+        return false;
+    }
 #else
-    fsync(fileno(fileout));
-#endif
+    #if defined(__linux__) || defined(__NetBSD__)
+    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(__APPLE__) && 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
+    if (fsync(fileno(file)) != 0 && errno != EINVAL) {
+        LogPrintf("%s: fsync failed: %d\n", __func__, errno);
+        return false;
+    }
+    #endif
 #endif
+    return true;
 }
 
 bool TruncateFile(FILE* file, unsigned int length)
diff --git a/src/util.h b/src/util.h
@@ -80,7 +80,7 @@ bool error(const char* fmt, const Args&... args)
 }
 
 void PrintExceptionContinue(const std::exception* pex, const char* pszThread);
-void FileCommit(FILE* fileout);
+bool FileCommit(FILE* file);
 bool TruncateFile(FILE* file, unsigned int length);
 int RaiseFileDescriptorLimit(int nMinFD);
 void AllocateFileRange(FILE* file, unsigned int offset, unsigned int length);
diff --git a/src/validation.cpp b/src/validation.cpp
@@ -1388,22 +1388,27 @@ void static FlushBlockFile(bool fFinalize = false)
     LOCK(cs_LastBlockFile);
 
     CDiskBlockPos posOld(nLastBlockFile, 0);
+    bool status = true;
 
     FILE* fileOld = OpenBlockFile(posOld);
     if (fileOld) {
         if (fFinalize)
-            TruncateFile(fileOld, vinfoBlockFile[nLastBlockFile].nSize);
-        FileCommit(fileOld);
+            status &= TruncateFile(fileOld, vinfoBlockFile[nLastBlockFile].nSize);
+        status &= FileCommit(fileOld);
         fclose(fileOld);
     }
 
     fileOld = OpenUndoFile(posOld);
     if (fileOld) {
         if (fFinalize)
-            TruncateFile(fileOld, vinfoBlockFile[nLastBlockFile].nUndoSize);
-        FileCommit(fileOld);
+            status &= TruncateFile(fileOld, vinfoBlockFile[nLastBlockFile].nUndoSize);
+        status &= FileCommit(fileOld);
         fclose(fileOld);
     }
+
+    if (!status) {
+        AbortNode("Flushing block file to disk failed. This is likely the result of an I/O error.");
+    }
 }
 
 bool FindUndoPos(CValidationState& state, int nFile, CDiskBlockPos& pos, unsigned int nAddSize);
@@ -4269,7 +4274,8 @@ bool DumpMempool(const CTxMemPool& pool)
         }
 
         file << mapDeltas;
-        FileCommit(file.Get());
+        if (!FileCommit(file.Get()))
+            throw std::runtime_error("FileCommit failed");
         file.fclose();
         if (!RenameOver(GetDataDir() / "mempool.dat.new", GetDataDir() / "mempool.dat")) {
             throw std::runtime_error("Rename failed");

Original file line number	Diff line number	Diff line change
`@@ -80,7 +80,7 @@ bool error(const char* fmt, const Args&... args)`
`80`	`80`	`}`
`81`	`81`
`82`	`82`	`void PrintExceptionContinue(const std::exception* pex, const char* pszThread);`
`83`		`-void FileCommit(FILE* fileout);`
	`83`	`+bool FileCommit(FILE* file);`
`84`	`84`	`bool TruncateFile(FILE* file, unsigned int length);`
`85`	`85`	`int RaiseFileDescriptorLimit(int nMinFD);`
`86`	`86`	`void AllocateFileRange(FILE* file, unsigned int offset, unsigned int length);`