You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by ch...@apache.org on 2018/10/31 18:28:48 UTC
[mesos] 01/04: Stout: Added a sync option for `write` and `rename`.
This is an automated email from the ASF dual-hosted git repository.
chhsiao pushed a commit to branch 1.7.x
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 49fa4c79877ee4ad9c10d9718e607798a1b5bffc
Author: Chun-Hung Hsiao <ch...@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.