Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix possible data race when committing block files #12696

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.
+27 −6
Diff settings

Always

Just for now

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.
  • Loading branch information...
eklitzke committed Mar 15, 2018
commit 4894e368fa61cbae1370a8b83581533b0b223f45
@@ -623,6 +623,7 @@ if test x$TARGET_OS = xdarwin; then
fi

AC_CHECK_HEADERS([endian.h sys/endian.h byteswap.h stdio.h stdlib.h unistd.h strings.h sys/types.h sys/stat.h sys/select.h sys/prctl.h])
AC_CHECK_FUNCS([fdatasync])

AC_CHECK_DECLS([strnlen])

@@ -732,14 +732,21 @@ void FileCommit(FILE *file)
#ifdef WIN32
HANDLE hFile = (HANDLE)_get_osfhandle(_fileno(file));
FlushFileBuffers(hFile);
#else
#if defined(__linux__) || defined(__NetBSD__)
fdatasync(fileno(file));
#elif defined(__APPLE__) && defined(F_FULLFSYNC)
#elif defined(__APPLE__) && defined(F_FULLFSYNC)
fcntl(fileno(file), F_FULLFSYNC, 0);
#else
#elif HAVE_FDATASYNC
fdatasync(fileno(file));
#else
fsync(fileno(file));

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 22, 2018

Contributor

You aren't changing this, but it would be nice if these functions returned errors so failures could be logged.

#endif
}

void DirectoryCommit(const fs::path &dirname)
{
#ifndef WIN32
FILE* file = fsbridge::fopen(dirname, "r");
fsync(fileno(file));

This comment has been minimized.

Copy link
@luke-jr

luke-jr Jun 12, 2018

Member

Is fsync really enough here, or should we be calling FileCommit on it?

#endif
fclose(file);
#endif
}

@@ -168,7 +168,19 @@ 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().
*/
void 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);

This comment has been minimized.

Copy link
@promag

promag Mar 15, 2018

Member

nit, fs:path& dirname. The same in the implementation.


bool TruncateFile(FILE *file, unsigned int length);
int RaiseFileDescriptorLimit(int nMinFD);
void AllocateFileRange(FILE *file, unsigned int offset, unsigned int length);
@@ -1614,6 +1614,7 @@ void static FlushBlockFile(bool fFinalize = false)
if (fFinalize)
TruncateFile(fileOld, vinfoBlockFile[nLastBlockFile].nSize);
FileCommit(fileOld);
DirectoryCommit(GetDataDir() / "blocks");

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 22, 2018

Contributor

Realize you are currently working on this, but if you decide to go with this code as is, you should definitely comment here that call this isn't needed in the case where new files aren't being added, in case somebody wants to try to optimize this later.

This comment has been minimized.

Copy link
@luke-jr

luke-jr Jun 12, 2018

Member

Won't it be a reasonably fast noop if the directory hasn't changed?

fclose(fileOld);
}

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.