This is an automated email from the ASF dual-hosted git repository. chhsiao pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 5a10007b86e4c7039a5260f6aacb75376270d57f Author: Chun-Hung Hsiao <chhs...@mesosphere.io> AuthorDate: Wed Oct 10 15:37:33 2018 -0700 Stout: Added a sync option for `write` and `rename`. This patch adds an option for the caller to sync the file and directory contents to the disk after a write to prevent filesystem inconsistency against reboots. Review: https://reviews.apache.org/r/69009 --- 3rdparty/stout/include/stout/os/posix/fsync.hpp | 27 +++++++++++++++- 3rdparty/stout/include/stout/os/posix/rename.hpp | 37 +++++++++++++++++++++- 3rdparty/stout/include/stout/os/windows/rename.hpp | 10 ++++-- 3rdparty/stout/include/stout/os/write.hpp | 31 ++++++++++++------ 3rdparty/stout/include/stout/protobuf.hpp | 29 ++++++++++++++--- 5 files changed, 115 insertions(+), 19 deletions(-) diff --git a/3rdparty/stout/include/stout/os/posix/fsync.hpp b/3rdparty/stout/include/stout/os/posix/fsync.hpp index 9a6bbf6..2cc5f76 100644 --- a/3rdparty/stout/include/stout/os/posix/fsync.hpp +++ b/3rdparty/stout/include/stout/os/posix/fsync.hpp @@ -15,9 +15,14 @@ #include <unistd.h> +#include <string> + #include <stout/nothing.hpp> #include <stout/try.hpp> +#include <stout/os/close.hpp> +#include <stout/os/int_fd.hpp> +#include <stout/os/open.hpp> namespace os { @@ -30,7 +35,27 @@ inline Try<Nothing> fsync(int fd) return Nothing(); } -} // namespace os { +// A wrapper function for the above `fsync()` with opening and closing the file. +// NOTE: This function is POSIX only and can be used to commit changes to a +// directory (e.g., renaming files) to the filesystem. +inline Try<Nothing> fsync(const std::string& path) +{ + Try<int_fd> fd = os::open(path, O_RDONLY | O_CLOEXEC); + + if (fd.isError()) { + return Error(fd.error()); + } + + Try<Nothing> result = fsync(fd.get()); + + // We ignore the return value of `close()` since any I/O-related failure + // scenarios would already have been triggered by `open()` or `fsync()`. + os::close(fd.get()); + + return result; +} + +} // namespace os { #endif // __STOUT_OS_POSIX_FSYNC_HPP__ diff --git a/3rdparty/stout/include/stout/os/posix/rename.hpp b/3rdparty/stout/include/stout/os/posix/rename.hpp index 9cff6db..9bc6ee1 100644 --- a/3rdparty/stout/include/stout/os/posix/rename.hpp +++ b/3rdparty/stout/include/stout/os/posix/rename.hpp @@ -16,20 +16,55 @@ #include <stdio.h> #include <string> +#include <vector> #include <stout/error.hpp> +#include <stout/foreach.hpp> #include <stout/nothing.hpp> +#include <stout/path.hpp> #include <stout/try.hpp> +#include <stout/os/fsync.hpp> namespace os { -inline Try<Nothing> rename(const std::string& from, const std::string& to) +// Rename a given path to another one. If `sync` is set to true, `fsync()` will +// be called on both the source directory and the destination directory to +// ensure that the result is committed to their filesystems. +// +// NOTE: This function can fail with `sync` set to true if either the source +// directory or the destination directory gets removed before it returns. If +// multiple processes or threads access to the filesystems concurrently, the +// caller should either enforce a proper synchronization, or set `sync` to false +// and call `fsync()` explicitly on POSIX systems to handle such failures. +inline Try<Nothing> rename( + const std::string& from, + const std::string& to, + bool sync = false) { if (::rename(from.c_str(), to.c_str()) != 0) { return ErrnoError(); } + if (sync) { + const std::string to_dir = Path(to).dirname(); + const std::string from_dir = Path(from).dirname(); + + std::vector<std::string> dirs = {to_dir}; + if (from_dir != to_dir) { + dirs.emplace_back(from_dir); + } + + foreach (const std::string& dir, dirs) { + Try<Nothing> fsync = os::fsync(dir); + + if (fsync.isError()) { + return Error( + "Failed to fsync directory '" + dir + "': " + fsync.error()); + } + } + } + return Nothing(); } diff --git a/3rdparty/stout/include/stout/os/windows/rename.hpp b/3rdparty/stout/include/stout/os/windows/rename.hpp index 523912a..6747f29 100644 --- a/3rdparty/stout/include/stout/os/windows/rename.hpp +++ b/3rdparty/stout/include/stout/os/windows/rename.hpp @@ -22,10 +22,12 @@ #include <stout/internal/windows/longpath.hpp> - namespace os { -inline Try<Nothing> rename(const std::string& from, const std::string& to) +inline Try<Nothing> rename( + const std::string& from, + const std::string& to, + bool sync = false) { // Use `MoveFile` to perform the file move. The MSVCRT implementation of // `::rename` fails if the `to` file already exists[1], while some UNIX @@ -41,7 +43,9 @@ inline Try<Nothing> rename(const std::string& from, const std::string& to) const BOOL result = ::MoveFileExW( ::internal::windows::longpath(from).data(), ::internal::windows::longpath(to).data(), - MOVEFILE_COPY_ALLOWED | MOVEFILE_REPLACE_EXISTING); + MOVEFILE_COPY_ALLOWED | + MOVEFILE_REPLACE_EXISTING | + (sync ? MOVEFILE_WRITE_THROUGH : 0)); if (!result) { return WindowsError( diff --git a/3rdparty/stout/include/stout/os/write.hpp b/3rdparty/stout/include/stout/os/write.hpp index cd35285..f7538f9 100644 --- a/3rdparty/stout/include/stout/os/write.hpp +++ b/3rdparty/stout/include/stout/os/write.hpp @@ -20,6 +20,7 @@ #include <stout/try.hpp> #include <stout/os/close.hpp> +#include <stout/os/fsync.hpp> #include <stout/os/int_fd.hpp> #include <stout/os/open.hpp> #include <stout/os/socket.hpp> @@ -30,7 +31,6 @@ #include <stout/os/posix/write.hpp> #endif // __WINDOWS__ - namespace os { namespace signal_safe { @@ -110,9 +110,12 @@ inline Try<Nothing> write(int_fd fd, const std::string& message) } -// A wrapper function that wraps the above write() with -// open and closing the file. -inline Try<Nothing> write(const std::string& path, const std::string& message) +// A wrapper function for the above `write()` with opening and closing the file. +// If `sync` is set to true, an `fsync()` will be called before `close()`. +inline Try<Nothing> write( + const std::string& path, + const std::string& message, + bool sync = false) { Try<int_fd> fd = os::open( path, @@ -125,9 +128,16 @@ inline Try<Nothing> write(const std::string& path, const std::string& message) Try<Nothing> result = write(fd.get(), message); - // We ignore the return value of close(). This is because users - // calling this function are interested in the return value of - // write(). Also an unsuccessful close() doesn't affect the write. + if (sync && result.isSome()) { + // We call `fsync()` before closing the file instead of opening it with the + // `O_SYNC` flag for better performance. See: + // http://lkml.iu.edu/hypermail/linux/kernel/0105.3/0353.html + result = os::fsync(fd.get()); + } + + // We ignore the return value of `close()` because users calling this function + // are interested in the return value of `write()`, or `fsync()` if `sync` is + // set to true. Also an unsuccessful `close()` doesn't affect the write. os::close(fd.get()); return result; @@ -136,9 +146,12 @@ inline Try<Nothing> write(const std::string& path, const std::string& message) // NOTE: This overload is necessary to disambiguate between arguments // of type `HANDLE` (`typedef void*`) and `char*` on Windows. -inline Try<Nothing> write(const char* path, const std::string& message) +inline Try<Nothing> write( + const char* path, + const std::string& message, + bool sync = false) { - return write(std::string(path), message); + return write(std::string(path), message, sync); } } // namespace os { diff --git a/3rdparty/stout/include/stout/protobuf.hpp b/3rdparty/stout/include/stout/protobuf.hpp index 1d03e1e..eb4adef 100644 --- a/3rdparty/stout/include/stout/protobuf.hpp +++ b/3rdparty/stout/include/stout/protobuf.hpp @@ -46,6 +46,7 @@ #include <stout/try.hpp> #include <stout/os/close.hpp> +#include <stout/os/fsync.hpp> #include <stout/os/int_fd.hpp> #include <stout/os/lseek.hpp> #include <stout/os/open.hpp> @@ -132,8 +133,10 @@ Try<Nothing> write( } +// A wrapper function for the above `write()` with opening and closing the file. +// If `sync` is set to true, an `fsync()` will be called before `close()`. template <typename T> -Try<Nothing> write(const std::string& path, const T& t) +Try<Nothing> write(const std::string& path, const T& t, bool sync = false) { Try<int_fd> fd = os::open( path, @@ -146,18 +149,28 @@ Try<Nothing> write(const std::string& path, const T& t) Try<Nothing> result = write(fd.get(), t); - // NOTE: We ignore the return value of close(). This is because - // users calling this function are interested in the return value of - // write(). Also an unsuccessful close() doesn't affect the write. + if (sync && result.isSome()) { + // We call `fsync()` before closing the file instead of opening it with the + // `O_SYNC` flag for better performance. See: + // http://lkml.iu.edu/hypermail/linux/kernel/0105.3/0353.html + result = os::fsync(fd.get()); + } + + // We ignore the return value of `close()` because users calling this function + // are interested in the return value of `write()`, or `fsync()` if `sync` is + // set to true. Also an unsuccessful `close()` doesn't affect the write. os::close(fd.get()); return result; } +// A wrapper function to append a protobuf message with opening and closing the +// file. If `sync` is set to true, an `fsync()` will be called before `close()`. inline Try<Nothing> append( const std::string& path, - const google::protobuf::Message& message) + const google::protobuf::Message& message, + bool sync = false) { Try<int_fd> fd = os::open( path, @@ -170,6 +183,12 @@ inline Try<Nothing> append( Try<Nothing> result = write(fd.get(), message); + if (sync && result.isSome()) { + // We call `fsync()` before closing the file instead of opening it with the + // `O_SYNC` flag for better performance. + result = os::fsync(fd.get()); + } + // NOTE: We ignore the return value of close(). This is because // users calling this function are interested in the return value of // write(). Also an unsuccessful close() doesn't affect the write.