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/)
EINVAL is handled specially to avoid crashing out on (network, fuse) filesystems that don't handle f[data]sync.
I checked that the syncing inside leveldb is already generating an I/O error as appropriate.
Strong concept ACK! Nice!
796 | + LogPrintf("%s: fflush failed: %d\n", __func__, errno); 797 | + return false; 798 | + } 799 | #ifdef WIN32 800 | HANDLE hFile = (HANDLE)_get_osfhandle(_fileno(file)); 801 | FlushFileBuffers(hFile);
This value should probably be checked as well: https://msdn.microsoft.com/en-us/library/windows/desktop/aa364439(v=vs.85).aspx
Ok
801 | FlushFileBuffers(hFile); 802 | #else 803 | #if defined(__linux__) || defined(__NetBSD__) 804 | - fdatasync(fileno(file)); 805 | + if (fdatasync(fileno(file)) != 0 && errno != EINVAL) { // Ignore EINVAL for filesystems that don't support sync 806 | + LogPrintf("%s: fdatasync failed: %d\n", __func__, errno);
I can't come up with a specific reason other than "gut feeling", but potentially logging to the same disk that just failed to sync feels icky.
That's not necessarily the case, e.g. they might be logging to stdout, or piping the log to something else over the network. Logging the error is extremely important for troubleshooting in any case, even if it might fail.
1629 | fileOld = OpenUndoFile(posOld);
1630 | if (fileOld) {
1631 | if (fFinalize)
1632 | TruncateFile(fileOld, vinfoBlockFile[nLastBlockFile].nUndoSize);
1633 | - FileCommit(fileOld);
1634 | + flush_failed = FileCommit(fileOld);
|=
Whoops, and the logic for these is backwards. They should be:
flush_failed |= !FileCommit(fileOld);
Ah yes this should be flush_ok
1627 | }
1628 |
1629 | fileOld = OpenUndoFile(posOld);
1630 | if (fileOld) {
1631 | if (fFinalize)
1632 | TruncateFile(fileOld, vinfoBlockFile[nLastBlockFile].nUndoSize);
Wouldn't hurt to check the return value here too, but I guess it's not crucial since any error would just propagate to the sync.
Will do
48 | @@ -49,7 +49,8 @@ bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data 49 | 50 | // Serialize 51 | if (!SerializeDB(fileout, data)) return false; 52 | - FileCommit(fileout.Get()); 53 | + if (!FileCommit(fileout.Get())) 54 | + return error("%s: Failed to flush file %s", __func__, pathTmp.string()); 55 | fileout.fclose();
Should this attempt to close before returning? I assume we're in bad shape already if we get here, but I'm afraid that some one-off sync failure could lead to multiple copies open and weird fd behavior.
Doesn't CAutofile destructor take care of this?
yep, nevermind missed that this was a CAutofile.
1620 | FILE *fileOld = OpenBlockFile(posOld);
1621 | if (fileOld) {
1622 | if (fFinalize)
1623 | TruncateFile(fileOld, vinfoBlockFile[nLastBlockFile].nSize);
1624 | - FileCommit(fileOld);
1625 | + flush_failed = FileCommit(fileOld);
See fclose comment above.
4764 | @@ -4760,7 +4765,8 @@ bool DumpMempool(void) 4765 | } 4766 | 4767 | file << mapDeltas; 4768 | - FileCommit(file.Get()); 4769 | + if (!FileCommit(file.Get())) 4770 | + throw std::runtime_error("FileCommit failed"); 4771 | file.fclose();
aaand here :)
concept ACK
800 | HANDLE hFile = (HANDLE)_get_osfhandle(_fileno(file)); 801 | FlushFileBuffers(hFile); 802 | #else 803 | #if defined(__linux__) || defined(__NetBSD__) 804 | - fdatasync(fileno(file)); 805 | + if (fdatasync(fileno(file)) != 0 && errno != EINVAL) { // Ignore EINVAL for filesystems that don't support sync
Interestingly, leveldb does not compare the result to EINVAL. Which makes sense for a database, as if writes can't be synchronized at any point, the db can't really ever be trusted.
I think we might want to operate the same way. If a FlushBlockFile() fails due to a disk/filesystem that can't sync, do we really want to continue with updating the index db?
leveldb does compare against EINVAL, we had this issue a long time ago where leveldb didn't work with certain filesystems (e.g. NTFS mounted from Linux). This was eventually fixed by adding the compare against EINVAL. We certainly don't want to repeat this issue with the block files!
$ git grep fsync
util/env_posix.cc: if (fsync(fd) < 0 && errno != EINVAL) {
``
Hmm. leveldb checks the fsync() in SyncDirIfManifest() against EINVAL, but not the fdatasync() in Sync().
I have no knowledge of the past issue though, so I'll defer to you on that.
as if writes can't be synchronized at any point, the db can't really ever be trusted.
I don't agree. When shutting down bitcoind and the OS cleanly, f(data)sync is a no-op. It's only an issue if there are sudden crashes or power outages. In which case the user will just have to accept potential corruption on crashes when choosing to use a filesystem that doesn't support these things. Certainly for the block files, which are huge and more commonly hosted externally.
My intent here is not to break any current usecases.
utACK after squash.
utACK 36424a4f09de38e1ab54381aa7844781688f5c75 (squash recommended).
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/)
squashed 36424a4 → cf0277928fa8d955d75f661021845789194dfff7
