From 1ae9671cd9ff1e88d9ca8452ff2e64ea2b6f250a Mon Sep 17 00:00:00 2001 From: Alex Date: Tue, 21 Jul 2026 10:43:44 +0200 Subject: [PATCH 01/17] remove forced libc++ for clang builds --- CMakeLists.txt | 9 --------- 1 file changed, 9 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index d54e15b9..850e7d29 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -265,15 +265,6 @@ function(common_setup_for_metall_executable name) # ----- Compile Options ----- # add_common_compile_options(${name}) - # Memo: - # On macOS and FreeBSD libc++ is the default standard library and the -stdlib=libc++ is not required. - # https://libcxx.llvm.org/docs/UsingLibcxx.html - if (CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND - NOT (CMAKE_CXX_COMPILER_ID MATCHES "Darwin" OR CMAKE_CXX_COMPILER_ID MATCHES "FreeBSD")) - target_compile_options(${name} PRIVATE -stdlib=libc++) - endif () - # -------------------- - # ----- Compile Definitions ----- # foreach (X ${COMPILER_DEFS}) target_compile_definitions(${name} PRIVATE ${X}) From d3aa7fa07e43c6e692be087a8bf97daf240f0d05 Mon Sep 17 00:00:00 2001 From: Alex Date: Tue, 21 Jul 2026 10:44:08 +0200 Subject: [PATCH 02/17] fix munmap not unmapping when call_msync is true --- include/metall/detail/mmap.hpp | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/include/metall/detail/mmap.hpp b/include/metall/detail/mmap.hpp index 3089f72f..eb50cf35 100644 --- a/include/metall/detail/mmap.hpp +++ b/include/metall/detail/mmap.hpp @@ -235,10 +235,19 @@ inline bool os_munmap(void *const addr, const size_t length) { return true; } +/// \brief Unmaps a region, optionally syncing it to the backing file first. +/// \param addr The start address of the region. +/// \param length The length of the region. +/// \param call_msync If true, calls msync(2) with MS_SYNC before unmapping. +/// \return On success, returns true. On error, returns false. inline bool munmap(void *const addr, const size_t length, const bool call_msync) { - if (call_msync) return os_msync(addr, length, true); - return os_munmap(addr, length); + bool ret = true; + if (call_msync) { + ret &= os_msync(addr, length, true); + } + ret &= os_munmap(addr, length); + return ret; } inline bool munmap(const int fd, void *const addr, const size_t length, From ba1dd6cbdb97540a0caf6c68cb3242b134c4fc0a Mon Sep 17 00:00:00 2001 From: Alex Date: Tue, 21 Jul 2026 10:44:25 +0200 Subject: [PATCH 03/17] add durable file write primitives --- include/metall/detail/file.hpp | 166 ++++++++++++++++++++++++++++++++- 1 file changed, 163 insertions(+), 3 deletions(-) diff --git a/include/metall/detail/file.hpp b/include/metall/detail/file.hpp index d7ad914c..95a62025 100644 --- a/include/metall/detail/file.hpp +++ b/include/metall/detail/file.hpp @@ -47,14 +47,44 @@ inline bool os_close(const int fd) { return true; } +/// \brief Calls fsync(2) on fd, retrying on EINTR. +/// fsync is the durability primitive of this code base. +/// On Linux it writes the data to the device and flushes the device write +/// cache, so it is durable against power loss. +/// On macOS it writes the data to the device but does not flush the device +/// write cache. Only fcntl(F_FULLFSYNC) (very slow) flushes it duarable at power +/// loss. whereas fsync is only process and OS crashes safe. Since MacOS is +/// typically no production target, it uses just fsync. +/// \param fd An open file descriptor. +/// \return On success, returns true. On error, returns false. inline bool os_fsync(const int fd) { - if (::fsync(fd) != 0) { - logger::perror(logger::level::error, __FILE__, __LINE__, "fsync"); - return false; + while (::fsync(fd) != 0) { + if (errno != EINTR) { + logger::perror(logger::level::error, __FILE__, __LINE__, "fsync"); + return false; + } } return true; } +/// \brief Calls fdatasync(2) on fd, retrying on EINTR. +/// Falls back to fsync(2) on platforms without fdatasync. +/// \param fd An open file descriptor. +/// \return On success, returns true. On error, returns false. +inline bool os_fdatasync(const int fd) { +#ifdef __linux__ + while (::fdatasync(fd) != 0) { + if (errno != EINTR) { + logger::perror(logger::level::error, __FILE__, __LINE__, "fdatasync"); + return false; + } + } + return true; +#else + return os_fsync(fd); +#endif +} + inline bool fsync(const fs::path &path) { const int fd = ::open(path.c_str(), O_RDONLY); if (fd == -1) { @@ -69,6 +99,136 @@ inline bool fsync(const fs::path &path) { return ret; } +/// \brief Makes the entries of a directory durable with fsync(2). +/// A file rename or unlink is durable only after the directory that holds the +/// entry is fsynced. +/// \param dir_path A path to a directory. +/// \return On success, returns true. On error, returns false. +inline bool fsync_directory(const fs::path &dir_path) { +#ifdef O_DIRECTORY + const int fd = ::open(dir_path.c_str(), O_RDONLY | O_DIRECTORY); +#else + const int fd = ::open(dir_path.c_str(), O_RDONLY); +#endif + if (fd == -1) { + const std::string s("open directory for fsync: " + dir_path.string()); + logger::perror(logger::level::error, __FILE__, __LINE__, s.c_str()); + return false; + } + + bool ret = true; + ret &= os_fsync(fd); + ret &= os_close(fd); + + return ret; +} + +/// \brief Fsyncs dir_path and every directory below it. +/// Used after a directory tree was populated with already-synced files to make +/// all directory entries durable. +/// \param dir_path A path to the root of a directory tree. +/// \return On success, returns true. On error, returns false. +inline bool fsync_directory_tree(const fs::path &dir_path) { + bool ret = fsync_directory(dir_path); + try { + for (const auto &p : fs::recursive_directory_iterator(dir_path)) { + if (p.is_directory()) { + ret &= fsync_directory(p.path()); + } + } + } catch (...) { + logger::out(logger::level::error, __FILE__, __LINE__, + "Exception was thrown while walking a directory tree"); + return false; + } + return ret; +} + +/// \brief Replaces to_replace with replacement using rename(2) and makes the +/// replacement durable. +/// rename(2) is atomic but not durable. Fsyncing the parent directory makes +/// the new directory entry durable. +/// \pre The content of replacement is already synced to disk. +/// \pre Both paths are on the same file system. +/// \param to_replace The path that receives the new file. +/// \param replacement The path of the file that replaces to_replace. +/// \return On success, returns true. On error, returns false. +inline bool atomic_durable_replace_file(const fs::path &to_replace, + const fs::path &replacement) { + if (::rename(replacement.c_str(), to_replace.c_str()) != 0) { + const std::string s("rename: " + replacement.string() + " -> " + + to_replace.string()); + logger::perror(logger::level::error, __FILE__, __LINE__, s.c_str()); + return false; + } + + return fsync_directory(to_replace.parent_path()); +} + +/// \brief Creates a named temporary file in parent_path with mkstemp(3). +/// The file is not deleted automatically. The caller removes it, or renames it +/// into place. +/// \param parent_path Directory the temporary file is created in. +/// \param tmp_file_path Receives the path of the created file. +/// \return An open file descriptor on success, -1 on error. +inline int make_named_tempfile(const fs::path &parent_path, + fs::path *tmp_file_path) { + std::string filename_template = (parent_path / ".metall.XXXXXX").string(); + + // mkstemp mutates the template in place. std::string::data() is mutable and + // null-terminated. + const int fd = ::mkstemp(filename_template.data()); + if (fd == -1) { + std::string s("mkstemp in: " + parent_path.string()); + logger::perror(logger::level::error, __FILE__, __LINE__, s.c_str()); + return -1; + } + + *tmp_file_path = filename_template; + return fd; +} + +/// \brief Writes a file atomically and durably. +/// Calls write_func with a temporary path in the same directory as final_path. +/// When write_func succeeds, the temporary file is synced to disk and renamed +/// to final_path, and the rename is made durable. On any failure the previous +/// content of final_path stays intact and the temporary file is removed. +/// \param final_path The path of the file to write. +/// \param write_func A function bool(const fs::path &) that writes the file +/// content to the given path. +/// \return On success, returns true. On error, returns false. +inline bool write_file_atomically( + const fs::path &final_path, + const std::function &write_func) { + fs::path tmp_path; + const int fd = make_named_tempfile(final_path.parent_path(), &tmp_path); + if (fd == -1) { + return false; + } + // write_func opens the file itself. Only the path of the temporary file is + // needed here. + if (!os_close(fd)) { + std::error_code ec; + fs::remove(tmp_path, ec); + return false; + } + + if (!write_func(tmp_path)) { + std::error_code ec; + fs::remove(tmp_path, ec); + return false; + } + + if (!fsync(tmp_path) || + !atomic_durable_replace_file(final_path, tmp_path)) { + std::error_code ec; + fs::remove(tmp_path, ec); + return false; + } + + return true; +} + inline bool extend_file_size_manually(const int fd, const off_t offset, const ssize_t file_size) { auto buffer = new unsigned char[4096]; From 5a639e72b78d538202fd89eaf72dd3db24998b40 Mon Sep 17 00:00:00 2001 From: Alex Date: Tue, 21 Jul 2026 10:44:41 +0200 Subject: [PATCH 04/17] fix uninitialized buffer in extend_file_size_manually --- include/metall/detail/file.hpp | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/include/metall/detail/file.hpp b/include/metall/detail/file.hpp index 95a62025..74f67620 100644 --- a/include/metall/detail/file.hpp +++ b/include/metall/detail/file.hpp @@ -229,21 +229,33 @@ inline bool write_file_atomically( return true; } +/// \brief Extends a file by writing zero bytes in [offset, offset + +/// file_size). +/// \param fd An open file descriptor. +/// \param offset The position the zero bytes start at. +/// \param file_size The number of zero bytes to write. +/// \return On success, returns true. On error, returns false. inline bool extend_file_size_manually(const int fd, const off_t offset, const ssize_t file_size) { - auto buffer = new unsigned char[4096]; - for (off_t i = offset; i < file_size / 4096 + offset; ++i) { - ::pwrite(fd, buffer, 4096, i * 4096); + constexpr std::size_t block_size = 4096; + const std::vector buffer(block_size, 0); + + ssize_t remaining = file_size; + off_t position = offset; + while (remaining > 0) { + const std::size_t write_size = + std::min(block_size, remaining); + const ssize_t written = ::pwrite(fd, buffer.data(), write_size, position); + if (written == -1) { + if (errno == EINTR) continue; + logger::perror(logger::level::error, __FILE__, __LINE__, "pwrite"); + return false; + } + remaining -= written; + position += written; } - const std::size_t remained_size = file_size % 4096; - if (remained_size > 0) - ::pwrite(fd, buffer, remained_size, file_size - remained_size); - - delete[] buffer; - const bool ret = os_fsync(fd); - - return ret; + return os_fsync(fd); } inline bool extend_file_size(const int fd, const std::size_t file_size, From 0b21ebf5db0e5642bcc3133b3c28ae8a8afc026c Mon Sep 17 00:00:00 2001 From: Alex Date: Tue, 21 Jul 2026 10:44:41 +0200 Subject: [PATCH 05/17] fsync after msync where msync does not flush the device cache --- include/metall/detail/mmap.hpp | 35 ++++++++++++++++++++++- include/metall/kernel/segment_storage.hpp | 11 +++++-- 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/include/metall/detail/mmap.hpp b/include/metall/detail/mmap.hpp index eb50cf35..27957439 100644 --- a/include/metall/detail/mmap.hpp +++ b/include/metall/detail/mmap.hpp @@ -217,6 +217,17 @@ inline std::pair map_file_write_private_mode( return std::make_pair(fd, mapped_addr); } +/// \brief Calls msync(2) on the given range. +/// On Linux, msync with MS_SYNC takes the same kernel path as fdatasync for +/// the mapped range, including the device cache flush. On other platforms it +/// writes the pages to the device without flushing the device cache; callers +/// that need fsync-level durability there must also fsync the backing file +/// descriptor (see os_fsync for the macOS design choice). +/// \param addr The start address of the range. +/// \param length The length of the range. +/// \param sync If true, uses MS_SYNC. Otherwise, uses MS_ASYNC. +/// \param additional_flags Flags that are added to the msync flags. +/// \return On success, returns true. On error, returns false. inline bool os_msync(void *const addr, const size_t length, const bool sync, const int additional_flags = 0) { if (::msync(addr, length, (sync ? MS_SYNC : MS_ASYNC) | additional_flags) != @@ -250,11 +261,33 @@ inline bool munmap(void *const addr, const size_t length, return ret; } +/// \brief Unmaps a region and closes the backing file descriptor, optionally +/// syncing the region durably first. +/// \param fd The file descriptor of the backing file. +/// \param addr The start address of the region. +/// \param length The length of the region. +/// \param call_msync If true, syncs the region to the backing file before +/// unmapping. The sync is durable also on platforms where msync(2) does not +/// flush the device cache, because the open file descriptor allows an +/// additional fsync there. +/// \return On success, returns true. On error, returns false. inline bool munmap(const int fd, void *const addr, const size_t length, const bool call_msync) { + if (call_msync) { + bool ret = os_msync(addr, length, true); +#ifndef __linux__ + // os_msync alone does not flush the device cache on this platform. The + // file descriptor is still open here, so the flush can be added. + ret &= os_fsync(fd); +#endif + ret &= os_close(fd); + ret &= os_munmap(addr, length); + return ret; + } + bool ret = true; ret &= os_close(fd); - ret &= munmap(addr, length, call_msync); + ret &= munmap(addr, length, false); return ret; } diff --git a/include/metall/kernel/segment_storage.hpp b/include/metall/kernel/segment_storage.hpp index e5ca3c36..99c74e68 100644 --- a/include/metall/kernel/segment_storage.hpp +++ b/include/metall/kernel/segment_storage.hpp @@ -770,8 +770,15 @@ class segment_storage { #endif const auto map = static_cast(m_segment) + block_no * k_block_size; - num_successes.fetch_add(mdtl::os_msync(map, k_block_size, sync) ? 1 - : 0); + bool ok = mdtl::os_msync(map, k_block_size, sync); +#ifndef __linux__ + // On Linux, msync(MS_SYNC) syncs the files. + // Other platforms do not guarantee this. + if (ok && sync) { + ok = mdtl::os_fsync(m_block_fd_list[block_no]); + } +#endif + num_successes.fetch_add(ok ? 1 : 0); } else { break; } From cf04c7b59ca7ee2dd96991a922a629147f5321fe Mon Sep 17 00:00:00 2001 From: Alex Date: Tue, 21 Jul 2026 10:45:03 +0200 Subject: [PATCH 06/17] write management metadata files atomically --- .../kernel/attributed_object_directory.hpp | 6 ++- include/metall/kernel/bin_directory.hpp | 11 ++++- include/metall/kernel/chunk_directory.hpp | 11 ++++- include/metall/kernel/manager_kernel_impl.ipp | 41 +++++++++++-------- 4 files changed, 48 insertions(+), 21 deletions(-) diff --git a/include/metall/kernel/attributed_object_directory.hpp b/include/metall/kernel/attributed_object_directory.hpp index 83075b75..ddbfc5aa 100644 --- a/include/metall/kernel/attributed_object_directory.hpp +++ b/include/metall/kernel/attributed_object_directory.hpp @@ -22,6 +22,7 @@ #include #include +#include #include #include @@ -541,7 +542,10 @@ class attributed_object_directory { return false; } - if (!json::write_json(json_root, path)) { + if (!metall::mtlldetail::write_file_atomically( + path, [&json_root](const fs::path &tmp_path) { + return json::write_json(json_root, tmp_path); + })) { return false; } diff --git a/include/metall/kernel/bin_directory.hpp b/include/metall/kernel/bin_directory.hpp index f96f2d58..8cfd8e13 100644 --- a/include/metall/kernel/bin_directory.hpp +++ b/include/metall/kernel/bin_directory.hpp @@ -26,6 +26,7 @@ #endif #include +#include #include namespace metall { @@ -195,9 +196,16 @@ class bin_directory { return m_table[bin_no].end(); } - /// \brief + /// \brief Serializes the directory to a file, atomically and durably. /// \param path bool serialize(const fs::path &path) const { + return mdtl::write_file_atomically( + path, + [this](const fs::path &tmp_path) { return priv_serialize(tmp_path); }); + } + + private: + bool priv_serialize(const fs::path &path) const { std::ofstream ofs(path); if (!ofs.is_open()) { std::stringstream ss; @@ -224,6 +232,7 @@ class bin_directory { return true; } + public: /// \brief /// \param path bool deserialize(const fs::path &path) { diff --git a/include/metall/kernel/chunk_directory.hpp b/include/metall/kernel/chunk_directory.hpp index 0cc70e8e..19653776 100644 --- a/include/metall/kernel/chunk_directory.hpp +++ b/include/metall/kernel/chunk_directory.hpp @@ -14,6 +14,7 @@ #include #include +#include #include #include #include @@ -294,9 +295,16 @@ class chunk_directory { return m_table[chunk_no].num_occupied_slots; } - /// \brief + /// \brief Serializes the directory to a file, atomically and durably. /// \param path bool serialize(const fs::path &path) const { + return mdtl::write_file_atomically( + path, + [this](const fs::path &tmp_path) { return priv_serialize(tmp_path); }); + } + + private: + bool priv_serialize(const fs::path &path) const { std::ofstream ofs(path); if (!ofs.is_open()) { std::stringstream ss; @@ -357,6 +365,7 @@ class chunk_directory { return true; } + public: /// \brief /// \param path /// \return diff --git a/include/metall/kernel/manager_kernel_impl.ipp b/include/metall/kernel/manager_kernel_impl.ipp index 291e16de..edef5f0d 100644 --- a/include/metall/kernel/manager_kernel_impl.ipp +++ b/include/metall/kernel/manager_kernel_impl.ipp @@ -1123,10 +1123,13 @@ bool manager_kernel::priv_remove_data_store( template bool manager_kernel::priv_write_management_metadata( const path_type &base_path, const json_store &json_root) { - if (!mdtl::ptree::write_json( - json_root, - storage::get_path(base_path, {k_management_dir_name, - k_manager_metadata_file_name}))) { + const auto file_path = storage::get_path( + base_path, {k_management_dir_name, k_manager_metadata_file_name}); + if (!mdtl::write_file_atomically(file_path, + [&json_root](const path_type &tmp_path) { + return mdtl::ptree::write_json(json_root, + tmp_path); + })) { logger::out(logger::level::error, __FILE__, __LINE__, "Failed to write management metadata"); return false; @@ -1246,22 +1249,24 @@ bool manager_kernel::priv_write_description( const auto &file_name = storage::get_path( base_path, {k_management_dir_name, k_description_file_name}); - std::ofstream ofs(file_name); - if (!ofs.is_open()) { - std::string s("Failed to open: " + file_name.string()); - logger::out(logger::level::error, __FILE__, __LINE__, s.c_str()); - return false; - } - - if (!(ofs << description)) { - std::string s("Failed to write data:" + file_name.string()); - logger::out(logger::level::error, __FILE__, __LINE__, s.c_str()); - return false; - } + return mdtl::write_file_atomically( + file_name, [&description, &file_name](const path_type &tmp_path) { + std::ofstream ofs(tmp_path); + if (!ofs.is_open()) { + std::string s("Failed to open: " + tmp_path.string()); + logger::out(logger::level::error, __FILE__, __LINE__, s.c_str()); + return false; + } - ofs.close(); + if (!(ofs << description)) { + std::string s("Failed to write data:" + file_name.string()); + logger::out(logger::level::error, __FILE__, __LINE__, s.c_str()); + return false; + } - return true; + ofs.close(); + return true; + }); } } // namespace metall::kernel From d7131132db65d68543957b2a39c7a2d35eb2b27e Mon Sep 17 00:00:00 2001 From: Alex Date: Tue, 21 Jul 2026 10:54:41 +0200 Subject: [PATCH 07/17] split datastore lock from properly-closed mark --- .../metall/detail/properly_closed_mark.hpp | 218 +++++++++++------- include/metall/kernel/manager_kernel.hpp | 21 +- include/metall/kernel/manager_kernel_impl.ipp | 124 ++++++++-- include/metall/kernel/storage.hpp | 5 + 4 files changed, 263 insertions(+), 105 deletions(-) diff --git a/include/metall/detail/properly_closed_mark.hpp b/include/metall/detail/properly_closed_mark.hpp index 520716c5..1a3438cc 100644 --- a/include/metall/detail/properly_closed_mark.hpp +++ b/include/metall/detail/properly_closed_mark.hpp @@ -1,8 +1,8 @@ #ifndef METALL_DETAIL_PROPERLYCLOSED_MARK_HPP #define METALL_DETAIL_PROPERLYCLOSED_MARK_HPP -#include #include +#include #include #include @@ -10,136 +10,186 @@ #include #include +#include namespace metall::mtlldetail { /** - * A marker file on the filesystem that determines if and how the datastore - * is currently opened and if it was properly closed the last time it was opened. + * Open-state tracking and crash detection for a datastore. * - * It allows multiple readers xor one writer. - * It uses the existence of the file in combination with flock(2) to achieve this. + * Two files with separate roles: + * + * - A lockfile. It always exists while a datastore exists. It is never + * removed. flock(2) on it allows multiple readers or one writer. Because + * the file is persistent, creating a datastore can take the lock before it + * destroys existing data, and a failed open cannot be confused with a + * crashed datastore. + * + * - A mark file (the properly-closed mark). A read-write open removes it and + * makes the removal durable. A successful close recreates it durably. Its + * absence at open time means the datastore was not properly closed. + * + * The mark is only created by an explicit call to mark_properly_closed(). + * The destructor only releases the lock. The caller creates the mark after + * it has verified that all datastore state reached disk. */ struct properly_closed_mark { -private: - // if mark_path is empty, this mark is in an invalid state - // and should not create the mark on close + private: + int m_lock_fd = -1; + // if m_mark_path is empty, this instance holds no open datastore std::filesystem::path m_mark_path{}; - int m_mark_fd = -1; bool m_read_only = false; - void unlock() { - if (m_mark_fd < 0) { - return; + static int open_or_create_lockfile( + const std::filesystem::path &lockfile_path) { + const int fd = ::open(lockfile_path.c_str(), O_RDONLY); + if (fd >= 0) { + return fd; + } + if (errno != ENOENT) { + std::string s("open lockfile: " + lockfile_path.string()); + logger::perror(logger::level::error, __FILE__, __LINE__, s.c_str()); + return -1; } - ::flock(m_mark_fd, LOCK_UN); - ::close(m_mark_fd); - m_mark_fd = -1; + // The lockfile does not exist yet (datastore created by an older version). + const int new_fd = + ::open(lockfile_path.c_str(), O_CREAT | O_RDONLY, S_IRUSR | S_IWUSR); + if (new_fd < 0) { + std::string s("create lockfile: " + lockfile_path.string()); + logger::perror(logger::level::error, __FILE__, __LINE__, s.c_str()); + return -1; + } + // The lockfile must exist before the lock has any meaning for other + // processes. + fsync_directory(lockfile_path.parent_path()); + return new_fd; } - bool open_mark() { - const int fd = ::open(m_mark_path.c_str(), O_RDONLY); + bool lock(const std::filesystem::path &lockfile_path, + const bool shared) { + assert(m_lock_fd == -1); + + const int fd = open_or_create_lockfile(lockfile_path); if (fd < 0) { return false; } - m_mark_fd = fd; - return true; - } - - bool lock_exclusive() { - if (!open_mark()) { + if (::flock(fd, (shared ? LOCK_SH : LOCK_EX) | LOCK_NB) != 0) { + logger::out(logger::level::error, __FILE__, __LINE__, + "The datastore is already open (lockfile is locked)"); + ::close(fd); return false; } - const int ret = ::flock(m_mark_fd, LOCK_EX | LOCK_NB); - return ret == 0; + m_lock_fd = fd; + return true; } - bool lock_shared() { - if (!open_mark()) { - return false; + void unlock() { + if (m_lock_fd < 0) { + return; } - - const int ret = ::flock(m_mark_fd, LOCK_SH | LOCK_NB); - return ret == 0; + ::flock(m_lock_fd, LOCK_UN); + ::close(m_lock_fd); + m_lock_fd = -1; } - bool open_impl() { - if (m_read_only) { - return lock_shared(); - } else { - if (!lock_exclusive()) { - return false; - } + public: + properly_closed_mark() noexcept = default; - return remove_file(m_mark_path); - } - } + properly_closed_mark(const properly_closed_mark &) = delete; + properly_closed_mark &operator=(const properly_closed_mark &) = delete; -public: - properly_closed_mark() noexcept = default; + properly_closed_mark(properly_closed_mark &&other) noexcept + : m_lock_fd{std::exchange(other.m_lock_fd, -1)}, + m_mark_path{std::move(other.m_mark_path)}, + m_read_only{other.m_read_only} { + other.m_mark_path.clear(); + } - bool create(const std::filesystem::path &path) { - m_mark_path = path; - m_read_only = false; + properly_closed_mark &operator=(properly_closed_mark &&other) noexcept { + assert(this != &other); + std::swap(m_lock_fd, other.m_lock_fd); + std::swap(m_mark_path, other.m_mark_path); + std::swap(m_read_only, other.m_read_only); + return *this; + } - if (!remove_file(m_mark_path)) { - // could not create, prevent mark creation on close - m_mark_path.clear(); + /// The destructor only releases the lock. It does not create the mark, + /// because the state of the datastore is unknown here. + ~properly_closed_mark() noexcept { release(); } + + /// \brief Takes the writer lock for a datastore that is being created. + /// The mark is not touched. The caller destroys and recreates the datastore + /// directory (which removes any old mark) while the lock is held. + /// \return On success, returns true. On error (including a concurrently + /// open datastore), returns false. + bool create(const std::filesystem::path &lockfile_path, + const std::filesystem::path &mark_path) { + if (!lock(lockfile_path, false)) { return false; } - + m_mark_path = mark_path; + m_read_only = false; return true; } - bool open(const std::filesystem::path &path, const bool read_only) { - m_mark_path = path; - m_read_only = read_only; - - if (!open_impl()) { - // could not open, prevent mark creation on close - m_mark_path.clear(); + /// \brief Takes the lock and consumes the mark for an existing datastore. + /// Writer: takes the exclusive lock, requires the mark, removes it and makes + /// the removal durable before the caller modifies any data. + /// Reader: takes the shared lock and requires the mark. + /// \return On success, returns true. On error (already open, or not + /// properly closed), returns false. + bool open(const std::filesystem::path &lockfile_path, + const std::filesystem::path &mark_path, const bool read_only) { + if (!lock(lockfile_path, read_only)) { return false; } - return true; - } - - properly_closed_mark(const properly_closed_mark &other) = delete; - properly_closed_mark &operator=(const properly_closed_mark &other) = delete; + if (!file_exist(mark_path)) { + logger::out(logger::level::error, __FILE__, __LINE__, + "The datastore was not closed properly and may be broken"); + unlock(); + return false; + } - properly_closed_mark(properly_closed_mark &&other) noexcept - : m_mark_path{std::move(other.m_mark_path)}, - m_mark_fd{std::exchange(other.m_mark_fd, -1)}, - m_read_only{other.m_read_only} { - } + if (!read_only) { + if (!remove_file(mark_path) || + !fsync_directory(mark_path.parent_path())) { + logger::out(logger::level::error, __FILE__, __LINE__, + "Failed to durably remove the properly-closed mark"); + unlock(); + return false; + } + } - properly_closed_mark &operator=(properly_closed_mark &&other) noexcept { - assert(this != &other); - std::swap(m_mark_path, other.m_mark_path); - std::swap(m_mark_fd, other.m_mark_fd); - std::swap(m_read_only, other.m_read_only); - return *this; + m_mark_path = mark_path; + m_read_only = read_only; + return true; } - ~properly_closed_mark() noexcept { - close(); + /// \brief Creates the mark durably. + /// The caller invokes this only after all datastore state was synced to + /// disk. The lock stays held until release(). + /// \return On success, returns true. On error, returns false. + bool mark_properly_closed() { + if (m_mark_path.empty() || m_read_only) { + return false; + } + // create_file syncs the file and its parent directory. + return create_file(m_mark_path); } - void close() { - if (!m_read_only && !m_mark_path.empty()) { - create_file(m_mark_path); - } + /// \brief Releases the lock without touching the mark. + void release() { unlock(); + m_mark_path.clear(); } - [[nodiscard]] bool is_read_only() const noexcept { - return m_read_only; - } + [[nodiscard]] bool is_read_only() const noexcept { return m_read_only; } }; -} // namespace metall::mtlldetail +} // namespace metall::mtlldetail -#endif // METALL_DETAIL_PROPERLYCLOSED_MARK_HPP +#endif // METALL_DETAIL_PROPERLYCLOSED_MARK_HPP diff --git a/include/metall/kernel/manager_kernel.hpp b/include/metall/kernel/manager_kernel.hpp index ee5eaffb..1785a6ef 100644 --- a/include/metall/kernel/manager_kernel.hpp +++ b/include/metall/kernel/manager_kernel.hpp @@ -119,6 +119,12 @@ class manager_kernel { static constexpr const char *k_properly_closed_mark_file_name = "properly_closed_mark"; + // The lockfile lives next to the datastore root directory, not inside it, + // so it survives datastore re-creation and the lock can be taken before + // existing data is destroyed. + static constexpr const char *k_lock_file_name = "mds_lock"; + + // For manager metadata data static constexpr const char *k_manager_metadata_file_name = "manager_metadata"; @@ -192,8 +198,14 @@ class manager_kernel { /// \return Returns true if success; otherwise, returns false bool open_read_only(const path_type &base_path); - /// \brief Expect to be called by a single thread - void close(); + /// \brief Closes the datastore. + /// Serializes the management data, syncs the segment, and creates the + /// properly-closed mark only if everything succeeded. + /// Expect to be called by a single thread + /// \return Returns true if all data was persisted; otherwise, returns + /// false. On false, no properly-closed mark is created and the datastore is + /// reported as inconsistent by consistent(). + bool close(); /// \brief Flush data to persistent memory /// \param synchronous If true, performs synchronous operation; @@ -492,6 +504,11 @@ class manager_kernel { static bool priv_create_datastore_directory(const path_type &base_path); + static path_type priv_lock_file_path(const path_type &base_path); + + void priv_undo_consumed_mark(); + + // ---------- For consistence support ---------- // static bool priv_consistent(const path_type &base_path); static bool priv_check_version(const json_store &metadata_json); diff --git a/include/metall/kernel/manager_kernel_impl.ipp b/include/metall/kernel/manager_kernel_impl.ipp index edef5f0d..8cec0963 100644 --- a/include/metall/kernel/manager_kernel_impl.ipp +++ b/include/metall/kernel/manager_kernel_impl.ipp @@ -56,19 +56,37 @@ bool manager_kernel::open( } template -void manager_kernel::close() { - if (m_segment_storage.is_open()) { - priv_check_sanity(); - if (!m_segment_storage.read_only()) { - priv_serialize_management_data(); - m_segment_storage.sync(true); - } +bool manager_kernel::close() { + if (!m_segment_storage.is_open()) { + return true; + } - m_good = false; - m_segment_storage.release(); + priv_check_sanity(); + const bool read_only = m_segment_storage.read_only(); + + bool success = true; + if (!read_only) { + success &= priv_serialize_management_data(); + success &= m_segment_storage.sync(true); + } + + m_good = false; + success &= m_segment_storage.release(); - m_properly_closed_mark.close(); + if (!read_only) { + if (success) { + success &= m_properly_closed_mark.mark_properly_closed(); + } else { + // Some data may not have reached disk. Without the mark the next open + // reports the datastore as inconsistent instead of silently using it. + logger::out(logger::level::error, __FILE__, __LINE__, + "Failed to persist the datastore on close; " + "the properly-closed mark is not created"); + } } + m_properly_closed_mark.release(); + + return success; } template @@ -609,9 +627,26 @@ bool manager_kernel::priv_create_datastore_directory( return false; } + // Make the new directory entries durable. + const auto root_dir = + storage::get_path(base_path, k_management_dir_name).parent_path(); + if (!mdtl::fsync_directory(base_path) || + !mdtl::fsync_directory_tree(root_dir)) { + logger::out(logger::level::error, __FILE__, __LINE__, + "Failed to fsync the datastore directories"); + return false; + } + return true; } +template +typename manager_kernel::path_type +manager_kernel::priv_lock_file_path( + const path_type &base_path) { + return base_path / k_lock_file_name; +} + template void manager_kernel::priv_check_sanity() const { assert(m_good); @@ -844,11 +879,13 @@ bool manager_kernel::priv_open( return false; } - if (!m_properly_closed_mark.open(storage::get_path(base_path, k_properly_closed_mark_file_name), - read_only)) { + if (!m_properly_closed_mark.open( + priv_lock_file_path(base_path), + storage::get_path(base_path, k_properly_closed_mark_file_name), + read_only)) { logger::out(logger::level::error, __FILE__, __LINE__, - "Unable to open data store — either it is already open, or it was not " - "closed properly and might have been collapsed."); + "Unable to open data store: either it is already open, or it " + "was not closed properly and might have been collapsed."); return false; } @@ -858,18 +895,30 @@ bool manager_kernel::priv_open( read_only)) { logger::out(logger::level::error, __FILE__, __LINE__, "Failed to open the application data segment"); + priv_undo_consumed_mark(); return false; } m_segment_storage.get_segment_header().manager_kernel_address = this; if (!priv_deserialize_management_data()) { m_segment_storage.release(); + priv_undo_consumed_mark(); return false; } return true; } +template +void manager_kernel::priv_undo_consumed_mark() { + // A failed open did not modify the datastore. Recreating the mark keeps the + // datastore openable. + if (!m_properly_closed_mark.is_read_only()) { + m_properly_closed_mark.mark_properly_closed(); + } + m_properly_closed_mark.release(); +} + template bool manager_kernel::priv_create( const path_type &base_path, const size_type vm_reserve_size) { @@ -885,17 +934,31 @@ bool manager_kernel::priv_create( return false; } - if (!priv_create_datastore_directory(base_path)) { + // The base directory must exist before the lockfile can be created in it. + if (!mdtl::create_directory(base_path)) { std::stringstream ss; - ss << "Failed to initialize datastore under " << base_path; + ss << "Failed to create directory " << base_path; + logger::out(logger::level::error, __FILE__, __LINE__, ss.str().c_str()); + return false; + } + + // The lock is taken before any existing data is destroyed. This refuses to + // re-create a datastore that another process has open. + if (!m_properly_closed_mark.create( + priv_lock_file_path(base_path), + storage::get_path(base_path, k_properly_closed_mark_file_name))) { + std::stringstream ss; + ss << "Failed to lock the datastore under " << base_path + << " (is it open elsewhere?)"; logger::out(logger::level::error, __FILE__, __LINE__, ss.str().c_str()); return false; } - if (!m_properly_closed_mark.create(storage::get_path(base_path, k_properly_closed_mark_file_name))) { + if (!priv_create_datastore_directory(base_path)) { std::stringstream ss; - ss << "Failed to remove a closed mark under " << base_path; + ss << "Failed to initialize datastore under " << base_path; logger::out(logger::level::error, __FILE__, __LINE__, ss.str().c_str()); + m_properly_closed_mark.release(); return false; } @@ -904,6 +967,7 @@ bool manager_kernel::priv_create( if (!m_segment_storage.create(m_base_path, vm_reserve_size)) { logger::out(logger::level::error, __FILE__, __LINE__, "Cannot create an application data segment"); + m_properly_closed_mark.release(); return false; } m_segment_storage.get_segment_header().manager_kernel_address = this; @@ -912,6 +976,7 @@ bool manager_kernel::priv_create( !priv_set_version(m_manager_metadata.get()) || !priv_write_management_metadata(m_base_path, *m_manager_metadata)) { m_segment_storage.release(); + m_properly_closed_mark.release(); return false; } @@ -1116,7 +1181,28 @@ bool manager_kernel::priv_copy_data_store( template bool manager_kernel::priv_remove_data_store( const path_type &base_path) { - return storage::remove(base_path); + if (!mdtl::directory_exist(storage::root_path(base_path)) && + !mdtl::file_exist(priv_lock_file_path(base_path))) { + return true; // nothing to remove + } + + // The lock refuses to remove a datastore that is open elsewhere. The + // lockfile itself is never removed: removing it would allow two processes + // to hold locks on different inodes of the same path. + mdtl::properly_closed_mark lock; + if (!lock.create( + priv_lock_file_path(base_path), + storage::get_path(base_path, k_properly_closed_mark_file_name))) { + std::string s("Cannot remove a datastore that is open elsewhere: " + + base_path.string()); + logger::out(logger::level::error, __FILE__, __LINE__, s.c_str()); + return false; + } + + if (!storage::remove(base_path)) { + return false; + } + return mdtl::fsync_directory(base_path); } // ---------- Management metadata ---------- // diff --git a/include/metall/kernel/storage.hpp b/include/metall/kernel/storage.hpp index b74656c6..85892156 100644 --- a/include/metall/kernel/storage.hpp +++ b/include/metall/kernel/storage.hpp @@ -37,6 +37,11 @@ class storage { return path; } + /// \brief Returns the root directory of a datastore. + static path_type root_path(const path_type &base_path) { + return priv_get_root_path(base_path); + } + /// \brief Create a new datastore. If a datastore already exists, remove it /// and create a new one. /// \param base_path A path to a directory where a datastore is created. From eeff3a950ff80a3173e99ac5c2530edc7cef6bd6 Mon Sep 17 00:00:00 2001 From: Alex Date: Tue, 21 Jul 2026 10:54:41 +0200 Subject: [PATCH 08/17] publish snapshots and copies atomically --- include/metall/kernel/manager_kernel.hpp | 9 + include/metall/kernel/manager_kernel_impl.ipp | 163 +++++++++++++++--- 2 files changed, 144 insertions(+), 28 deletions(-) diff --git a/include/metall/kernel/manager_kernel.hpp b/include/metall/kernel/manager_kernel.hpp index 1785a6ef..bbd68ff8 100644 --- a/include/metall/kernel/manager_kernel.hpp +++ b/include/metall/kernel/manager_kernel.hpp @@ -124,6 +124,9 @@ class manager_kernel { // existing data is destroyed. static constexpr const char *k_lock_file_name = "mds_lock"; + // Temporary directory a snapshot or copy is built in before it is renamed + // to the final datastore root. + static constexpr const char *k_tmp_datastore_dir_name = ".tmp_datastore"; // For manager metadata data static constexpr const char *k_manager_metadata_file_name = @@ -508,6 +511,12 @@ class manager_kernel { void priv_undo_consumed_mark(); + /// Publishes a fully written datastore copy: fsyncs the directory tree, + /// creates the destination lockfile, replaces the datastore root of + /// dst_base_path with the one under tmp_base_path, and makes the change + /// durable. + static bool priv_publish_datastore_copy(const path_type &tmp_base_path, + const path_type &dst_base_path); // ---------- For consistence support ---------- // static bool priv_consistent(const path_type &base_path); diff --git a/include/metall/kernel/manager_kernel_impl.ipp b/include/metall/kernel/manager_kernel_impl.ipp index 8cec0963..0fd25f12 100644 --- a/include/metall/kernel/manager_kernel_impl.ipp +++ b/include/metall/kernel/manager_kernel_impl.ipp @@ -1070,20 +1070,56 @@ bool manager_kernel::priv_snapshot( const path_type &destination_base_path, const bool clone, const int num_max_copy_threads) { priv_check_sanity(); - priv_serialize_management_data(); - if (!priv_create_datastore_directory(destination_base_path)) { + if (!priv_serialize_management_data()) { + logger::out(logger::level::error, __FILE__, __LINE__, + "Failed to serialize management data for the snapshot"); + return false; + } + + // Lock the destination while it is replaced. This also creates the + // destination lockfile. + if (!mdtl::create_directory(destination_base_path)) { + std::stringstream ss; + ss << "Failed to create directory " << destination_base_path; + logger::out(logger::level::error, __FILE__, __LINE__, ss.str().c_str()); + return false; + } + mdtl::properly_closed_mark destination_lock; + if (!destination_lock.create( + priv_lock_file_path(destination_base_path), + storage::get_path(destination_base_path, + k_properly_closed_mark_file_name))) { + std::stringstream ss; + ss << "The snapshot destination is open elsewhere: " + << destination_base_path; + logger::out(logger::level::error, __FILE__, __LINE__, ss.str().c_str()); + return false; + } + + // The snapshot is built under a temporary base path and published with a + // rename, so a half-written snapshot is never visible under the final path. + const path_type tmp_base_path = + destination_base_path / k_tmp_datastore_dir_name; + if (!mdtl::remove_file(tmp_base_path)) { // leftover of a crashed snapshot + std::stringstream ss; + ss << "Failed to remove leftover " << tmp_base_path; + logger::out(logger::level::error, __FILE__, __LINE__, ss.str().c_str()); + return false; + } + + if (!priv_create_datastore_directory(tmp_base_path)) { std::stringstream ss; - ss << "Failed to init the destination: " << destination_base_path; + ss << "Failed to init the snapshot destination: " << tmp_base_path; logger::out(logger::level::error, __FILE__, __LINE__, ss.str().c_str()); return false; } // Copy segment directory - if (!m_segment_storage.snapshot(destination_base_path, clone, + if (!m_segment_storage.snapshot(tmp_base_path, clone, num_max_copy_threads)) { std::stringstream ss; - ss << "Failed to copy " << m_base_path << " to " << destination_base_path; + ss << "Failed to copy " << m_base_path << " to " << tmp_base_path; logger::out(logger::level::error, __FILE__, __LINE__, ss.str().c_str()); return false; } @@ -1092,13 +1128,7 @@ bool manager_kernel::priv_snapshot( const auto src_mng_dir = storage::get_path(m_base_path, k_management_dir_name); const auto dst_mng_dir = - storage::get_path(destination_base_path, k_management_dir_name); - if (!mdtl::create_directory(dst_mng_dir)) { - std::stringstream ss; - ss << "Failed to create directory: " << dst_mng_dir; - logger::out(logger::level::error, __FILE__, __LINE__, ss.str().c_str()); - return false; - } + storage::get_path(tmp_base_path, k_management_dir_name); // Use a normal copy instead of reflink. // reflink might slow down if there are many reflink copied files. if (!mtlldetail::copy_files_in_directory_in_parallel(src_mng_dir, dst_mng_dir, @@ -1113,17 +1143,18 @@ bool manager_kernel::priv_snapshot( json_store meta_data; if (!priv_set_uuid(&meta_data)) return false; if (!priv_set_version(&meta_data)) return false; - if (!priv_write_management_metadata(destination_base_path, meta_data)) - return false; + if (!priv_write_management_metadata(tmp_base_path, meta_data)) return false; - // Finally, mark it as properly-closed - if (!priv_mark_properly_closed(destination_base_path)) { + // The mark is created inside the temporary base. It becomes visible under + // the final path together with all other files when the snapshot is + // published. + if (!priv_mark_properly_closed(tmp_base_path)) { logger::out(logger::level::error, __FILE__, __LINE__, "Failed to create a properly closed mark"); return false; } - return true; + return priv_publish_datastore_copy(tmp_base_path, destination_base_path); } // ---------- File operations ---------- // @@ -1138,26 +1169,65 @@ bool manager_kernel::priv_copy_data_store( return false; } - if (!storage::create(dst_base_path)) { + // The source is opened read-only (shared lock) so no writer can modify or + // re-create it while it is copied. + mdtl::properly_closed_mark source_lock; + if (!source_lock.open( + priv_lock_file_path(src_base_path), + storage::get_path(src_base_path, k_properly_closed_mark_file_name), + true)) { + std::string s("Failed to lock the copy source: " + src_base_path.string()); + logger::out(logger::level::error, __FILE__, __LINE__, s.c_str()); + return false; + } + + // Lock the destination while it is replaced. This also creates the + // destination lockfile. + if (!mdtl::create_directory(dst_base_path)) { + std::string s("Failed to create directory " + dst_base_path.string()); + logger::out(logger::level::error, __FILE__, __LINE__, s.c_str()); + return false; + } + mdtl::properly_closed_mark destination_lock; + if (!destination_lock.create( + priv_lock_file_path(dst_base_path), + storage::get_path(dst_base_path, + k_properly_closed_mark_file_name))) { + std::string s("The copy destination is open elsewhere: " + + dst_base_path.string()); + logger::out(logger::level::error, __FILE__, __LINE__, s.c_str()); + return false; + } + + // The copy is built under a temporary base path and published with a + // rename, so a half-written copy is never visible under the final path. + const path_type tmp_base_path = dst_base_path / k_tmp_datastore_dir_name; + if (!mdtl::remove_file(tmp_base_path)) { // leftover of a crashed copy + std::string s("Failed to remove leftover " + tmp_base_path.string()); + logger::out(logger::level::error, __FILE__, __LINE__, s.c_str()); + return false; + } + + if (!priv_create_datastore_directory(tmp_base_path)) { logger::out(logger::level::error, __FILE__, __LINE__, "Failed to initialize the datastore directory"); return false; } // Copy segment directory - segment_storage::copy(src_base_path, dst_base_path, use_clone, - num_max_copy_threads); + if (!segment_storage::copy(src_base_path, tmp_base_path, use_clone, + num_max_copy_threads)) { + std::string s("Failed to copy the segment directory from " + + src_base_path.string()); + logger::out(logger::level::error, __FILE__, __LINE__, s.c_str()); + return false; + } // Copy management directory const auto src_mng_dir = storage::get_path(src_base_path, k_management_dir_name); const auto dst_mng_dir = - storage::get_path(dst_base_path, k_management_dir_name); - if (!mdtl::create_directory(dst_mng_dir)) { - std::string s("Failed to create directory: " + dst_mng_dir.string()); - logger::out(logger::level::error, __FILE__, __LINE__, s.c_str()); - return false; - } + storage::get_path(tmp_base_path, k_management_dir_name); // Use a normal copy instead of reflink. // reflink might slow down if there are many reflink copied files. if (!mtlldetail::copy_files_in_directory_in_parallel(src_mng_dir, dst_mng_dir, @@ -1168,13 +1238,50 @@ bool manager_kernel::priv_copy_data_store( return false; } - // Finally, mark it as properly-closed - if (!priv_mark_properly_closed(dst_base_path)) { + // The mark is created inside the temporary base and becomes visible with + // the rename. + if (!priv_mark_properly_closed(tmp_base_path)) { logger::out(logger::level::error, __FILE__, __LINE__, "Failed to create a properly closed mark"); return false; } + return priv_publish_datastore_copy(tmp_base_path, dst_base_path); +} + +template +bool manager_kernel::priv_publish_datastore_copy( + const path_type &tmp_base_path, const path_type &dst_base_path) { + const auto tmp_root = storage::root_path(tmp_base_path); + const auto dst_root = storage::root_path(dst_base_path); + + // File contents were synced when they were written or copied. This makes + // the directory entries durable before the rename publishes them. + if (!mdtl::fsync_directory_tree(tmp_root)) { + logger::out(logger::level::error, __FILE__, __LINE__, + "Failed to fsync the datastore copy"); + return false; + } + + // Replace an existing datastore root. The datastore is briefly absent, but + // never half-written. + if (!mdtl::remove_file(dst_root)) { + std::string s("Failed to remove " + dst_root.string()); + logger::out(logger::level::error, __FILE__, __LINE__, s.c_str()); + return false; + } + + if (!mdtl::atomic_durable_replace_file(dst_root, tmp_root)) { + return false; + } + + // Remove the now empty temporary base directory. + if (!mdtl::remove_file(tmp_base_path) || + !mdtl::fsync_directory(dst_base_path)) { + logger::out(logger::level::warning, __FILE__, __LINE__, + "Failed to clean up the temporary snapshot directory"); + } + return true; } From bd85b18bfd9b24f3f510b19afcfc6150407ffd23 Mon Sep 17 00:00:00 2001 From: Alex Date: Tue, 21 Jul 2026 10:54:41 +0200 Subject: [PATCH 09/17] add crash and durability regression tests --- test/kernel/CMakeLists.txt | 2 + test/kernel/durability_test.cpp | 307 ++++++++++++++++++++++++++ test/sandbox.hpp | 175 +++++++++++++++ test/test_utility.hpp | 18 +- test/utility/CMakeLists.txt | 4 +- test/utility/file_durability_test.cpp | 141 ++++++++++++ 6 files changed, 645 insertions(+), 2 deletions(-) create mode 100644 test/kernel/durability_test.cpp create mode 100644 test/sandbox.hpp create mode 100644 test/utility/file_durability_test.cpp diff --git a/test/kernel/CMakeLists.txt b/test/kernel/CMakeLists.txt index c28d067a..07b440fc 100644 --- a/test/kernel/CMakeLists.txt +++ b/test/kernel/CMakeLists.txt @@ -17,6 +17,8 @@ target_compile_definitions(manager_test_single_thread PRIVATE "METALL_DISABLE_CO add_metall_test_executable(snapshot_test snapshot_test.cpp) +add_metall_test_executable(durability_test durability_test.cpp) + add_metall_test_executable(copy_datastore_test copy_datastore_test.cpp) include(setup_omp) diff --git a/test/kernel/durability_test.cpp b/test/kernel/durability_test.cpp new file mode 100644 index 00000000..272a4a9b --- /dev/null +++ b/test/kernel/durability_test.cpp @@ -0,0 +1,307 @@ +// Copyright 2026 Lawrence Livermore National Security, LLC and other Metall +// Project Developers. See the top-level COPYRIGHT file for details. +// +// SPDX-License-Identifier: (Apache-2.0 OR MIT) + +#include "gtest/gtest.h" + +#include + +#include + +#include + +#include "../sandbox.hpp" +#include "../test_utility.hpp" + +namespace { + +namespace fs = std::filesystem; +using test_utility::subprocess_result; + +// Internal datastore layout, used to verify on-disk state. +constexpr const char *k_root_dir_name = "mds"; +constexpr const char *k_lock_file_name = "mds_lock"; +constexpr const char *k_tmp_datastore_dir_name = ".tmp_datastore"; +constexpr const char *k_mark_file_name = "properly_closed_mark"; + +fs::path ds_path(const std::string &name) { + test_utility::create_test_dir(); + return test_utility::make_test_path(name); +} + +// A destructed manager closes the datastore properly. The next process sees +// a consistent datastore with the data. +TEST(DurabilityTest, ProperCloseAcrossProcesses) { + const auto path = ds_path("proper_close"); + metall::manager::remove(path); + + const auto res = METALL_SANDBOX { + metall::manager manager(metall::create_only, path); + auto *v = manager.construct("v")(42); + if (!v) return 1; + return 0; + // The manager destructor runs before the subprocess exits. + }; + ASSERT_EQ(res, subprocess_result::exit_success); + + ASSERT_TRUE(metall::manager::consistent(path)); + ASSERT_TRUE(fs::exists(path / k_root_dir_name / k_mark_file_name)); + + metall::manager manager(metall::open_read_only, path); + ASSERT_TRUE(manager.check_sanity()); + const auto v = manager.find("v"); + ASSERT_NE(v.first, nullptr); + EXPECT_EQ(*v.first, 42); +} + +// A process that dies while the datastore is open leaves no properly-closed +// mark. The datastore is reported as inconsistent and cannot be opened. +TEST(DurabilityTest, CrashWhileOpenIsDetected) { + const auto path = ds_path("crash_open"); + metall::manager::remove(path); + + const auto res = METALL_SANDBOX { + metall::manager manager(metall::create_only, path); + manager.construct("v")(42); + ::_exit(0); // dies without closing: destructors do not run + }; + ASSERT_EQ(res, subprocess_result::exit_success); + + EXPECT_FALSE(metall::manager::consistent(path)); + EXPECT_FALSE(fs::exists(path / k_root_dir_name / k_mark_file_name)); + + { + metall::manager manager(metall::open_only, path); + EXPECT_FALSE(manager.check_sanity()); + } + { + metall::manager manager(metall::open_read_only, path); + EXPECT_FALSE(manager.check_sanity()); + } +} + +// A close that cannot persist the management data does not create the +// properly-closed mark. +TEST(DurabilityTest, FailedCloseLeavesNoMark) { + if (::geteuid() == 0) { + GTEST_SKIP() << "Runs as root; permissions do not restrict writes"; + } + + const auto path = ds_path("failed_close"); + metall::manager::remove(path); + + const auto management_dir = path / k_root_dir_name / "management"; + { + metall::manager manager(metall::create_only, path); + ASSERT_TRUE(manager.check_sanity()); + manager.construct("v")(42); + + // Writing the management data fails at close. + fs::permissions(management_dir, + fs::perms::owner_read | fs::perms::owner_exec); + } + fs::permissions(management_dir, fs::perms::owner_all); + + EXPECT_FALSE(metall::manager::consistent(path)); + EXPECT_FALSE(fs::exists(path / k_root_dir_name / k_mark_file_name)); +} + +// One writer excludes all other opens. Multiple readers share. +TEST(DurabilityTest, OpenLockMatrix) { + const auto path = ds_path("lock_matrix"); + metall::manager::remove(path); + + { + metall::manager manager(metall::create_only, path); + ASSERT_TRUE(manager.check_sanity()); + manager.construct("v")(1); + + metall::manager second_writer(metall::open_only, path); + EXPECT_FALSE(second_writer.check_sanity()); + metall::manager reader(metall::open_read_only, path); + EXPECT_FALSE(reader.check_sanity()); + } + + { + metall::manager reader1(metall::open_read_only, path); + ASSERT_TRUE(reader1.check_sanity()); + metall::manager reader2(metall::open_read_only, path); + EXPECT_TRUE(reader2.check_sanity()); + + // The mark still exists while readers are open, so this failure comes + // from the lock, not from a missing mark. + ASSERT_TRUE(fs::exists(path / k_root_dir_name / k_mark_file_name)); + metall::manager writer(metall::open_only, path); + EXPECT_FALSE(writer.check_sanity()); + } + + // After all managers are closed the datastore opens normally. + metall::manager writer(metall::open_only, path); + ASSERT_TRUE(writer.check_sanity()); + const auto v = writer.find("v"); + ASSERT_NE(v.first, nullptr); + EXPECT_EQ(*v.first, 1); +} + +// Re-creating a datastore that is open elsewhere is refused and does not +// destroy the data. +TEST(DurabilityTest, CreateRefusedWhileOpen) { + const auto path = ds_path("create_while_open"); + metall::manager::remove(path); + + { + metall::manager manager(metall::create_only, path); + ASSERT_TRUE(manager.check_sanity()); + manager.construct("v")(7); + + metall::manager creator(metall::create_only, path); + EXPECT_FALSE(creator.check_sanity()); + + // The open manager is still usable. + const auto v = manager.find("v"); + ASSERT_NE(v.first, nullptr); + EXPECT_EQ(*v.first, 7); + } + + // The data survived the refused re-creation. + ASSERT_TRUE(metall::manager::consistent(path)); + metall::manager manager(metall::open_read_only, path); + const auto v = manager.find("v"); + ASSERT_NE(v.first, nullptr); + EXPECT_EQ(*v.first, 7); +} + +// Removing a datastore that is open elsewhere is refused. +TEST(DurabilityTest, RemoveRefusedWhileOpen) { + const auto path = ds_path("remove_while_open"); + metall::manager::remove(path); + + { + metall::manager manager(metall::create_only, path); + ASSERT_TRUE(manager.check_sanity()); + manager.construct("v")(7); + + EXPECT_FALSE(metall::manager::remove(path)); + + const auto v = manager.find("v"); + ASSERT_NE(v.first, nullptr); + } + + EXPECT_TRUE(metall::manager::remove(path)); + EXPECT_FALSE(metall::manager::consistent(path)); +} + +// A datastore created by a version without a lockfile opens normally. +TEST(DurabilityTest, OpenWithoutLockfile) { + const auto path = ds_path("no_lockfile"); + metall::manager::remove(path); + + { + metall::manager manager(metall::create_only, path); + ASSERT_TRUE(manager.check_sanity()); + manager.construct("v")(9); + } + + ASSERT_TRUE(fs::remove(path / k_lock_file_name)); + + { + metall::manager manager(metall::open_only, path); + ASSERT_TRUE(manager.check_sanity()); + const auto v = manager.find("v"); + ASSERT_NE(v.first, nullptr); + EXPECT_EQ(*v.first, 9); + } + EXPECT_TRUE(metall::manager::consistent(path)); + EXPECT_TRUE(fs::exists(path / k_lock_file_name)); +} + +// A snapshot is complete under its final path and leaves no temporary +// directory behind. +TEST(DurabilityTest, SnapshotIsPublishedAtomically) { + const auto src = ds_path("snapshot_src"); + const auto dst = ds_path("snapshot_dst"); + metall::manager::remove(src); + metall::manager::remove(dst); + + { + metall::manager manager(metall::create_only, src); + manager.construct("v")(11); + ASSERT_TRUE(manager.snapshot(dst)); + + // A second snapshot replaces the destination. + *(manager.find("v").first) = 12; + ASSERT_TRUE(manager.snapshot(dst)); + } + + EXPECT_TRUE(metall::manager::consistent(dst)); + EXPECT_FALSE(fs::exists(dst / k_tmp_datastore_dir_name)); + + metall::manager manager(metall::open_read_only, dst); + const auto v = manager.find("v"); + ASSERT_NE(v.first, nullptr); + EXPECT_EQ(*v.first, 12); +} + +// A snapshot onto a datastore that is open elsewhere is refused. +TEST(DurabilityTest, SnapshotRefusedOntoOpenDatastore) { + const auto src = ds_path("snapshot_busy_src"); + const auto dst = ds_path("snapshot_busy_dst"); + metall::manager::remove(src); + metall::manager::remove(dst); + + metall::manager source_manager(metall::create_only, src); + source_manager.construct("v")(1); + + metall::manager destination_manager(metall::create_only, dst); + ASSERT_TRUE(destination_manager.check_sanity()); + + EXPECT_FALSE(source_manager.snapshot(dst)); + + // The open destination is not damaged. + EXPECT_TRUE(destination_manager.check_sanity()); +} + +// A copy is complete under its final path and leaves no temporary directory +// behind. +TEST(DurabilityTest, CopyIsPublishedAtomically) { + const auto src = ds_path("copy_src"); + const auto dst = ds_path("copy_dst"); + metall::manager::remove(src); + metall::manager::remove(dst); + + { + metall::manager manager(metall::create_only, src); + manager.construct("v")(21); + } + + ASSERT_TRUE(metall::manager::copy(src, dst)); + EXPECT_TRUE(metall::manager::consistent(dst)); + EXPECT_FALSE(fs::exists(dst / k_tmp_datastore_dir_name)); + + metall::manager manager(metall::open_read_only, dst); + const auto v = manager.find("v"); + ASSERT_NE(v.first, nullptr); + EXPECT_EQ(*v.first, 21); +} + +// Copying onto a datastore that is open elsewhere is refused. +TEST(DurabilityTest, CopyRefusedOntoOpenDatastore) { + const auto src = ds_path("copy_busy_src"); + const auto dst = ds_path("copy_busy_dst"); + metall::manager::remove(src); + metall::manager::remove(dst); + + { + metall::manager manager(metall::create_only, src); + manager.construct("v")(1); + } + + metall::manager destination_manager(metall::create_only, dst); + ASSERT_TRUE(destination_manager.check_sanity()); + + EXPECT_FALSE(metall::manager::copy(src, dst)); + EXPECT_TRUE(destination_manager.check_sanity()); +} + +} // namespace diff --git a/test/sandbox.hpp b/test/sandbox.hpp new file mode 100644 index 00000000..dc541b39 --- /dev/null +++ b/test/sandbox.hpp @@ -0,0 +1,175 @@ +// This file is ported from dice-template-library +// (include/dice/template-library/sandbox.hpp), backported to C++17 and +// adapted to this code base. + +#ifndef METALL_TEST_SANDBOX_HPP +#define METALL_TEST_SANDBOX_HPP + +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include + +namespace test_utility { + +/// Execution result of a subprocess +enum struct subprocess_result : int { + exit_success = -1, ///< normal exit, code == 0 + exit_failure = -2, ///< normal exit, code != 0 + interrupted = SIGINT, + illegal_instruction = SIGILL, + aborted = SIGABRT, + floating_point_exception = SIGFPE, + segmentation_fault = SIGSEGV, + terminated = SIGTERM, +}; + +namespace detail_sandbox { + +inline void clear_all_signal_handlers() { +#if defined(NSIG) + constexpr int max_sig = NSIG; +#elif defined(_NSIG) + constexpr int max_sig = _NSIG; +#else + constexpr int max_sig = 64; // Fallback +#endif + + sigset_t empty_sigs; + sigemptyset(&empty_sigs); + + // unblock global, inhibited signals + ::sigprocmask(SIG_SETMASK, &empty_sigs, nullptr); + + struct sigaction sa {}; + sa.sa_handler = SIG_DFL; + sa.sa_mask = empty_sigs; // unblock local, inhibited signals + sa.sa_flags = 0; + + for (int i = 1; i < max_sig; ++i) { + // ignore errors (like EINVAL for unknown signals) + ::sigaction(i, &sa, nullptr); + } +} + +inline void flush_all_streams() { + // normally the fflush below also handles + // these but if std::ios_base::sync_with_stdio(false); + // was ever called they have their own buffers + std::cout.flush(); + std::cerr.flush(); + + fflush(nullptr); + + // OS file descriptors do not need to be flushed +} + +// the noexcept is important: if func throws, this turns it into +// std::terminate signalling SIGABRT +template +[[nodiscard]] int invoke_like_main(F &&func) noexcept { + if constexpr (std::is_same_v, void>) { + std::invoke(std::forward(func)); + return 0; + } else { + return std::invoke(std::forward(func)); + } +} + +struct sandbox {}; + +template +[[nodiscard]] subprocess_result operator+(sandbox, F func) { + static_assert( + std::is_invocable_r_v || std::is_invocable_r_v, + "Function must be invocable like a main() function"); + + // ensure no stale data is in output streams + flush_all_streams(); + + int const pid = fork(); + if (pid < 0) { + throw std::system_error{errno, std::system_category(), "Unable to fork"}; + } + + if (pid == 0) { + // child process + + // remove signal handles that may have been installed (e.g. by gtest) + clear_all_signal_handlers(); + + // invoke user provided function + int const exit_code = invoke_like_main(std::move(func)); + + // flush again, to avoid loosing output if the function does not abort + flush_all_streams(); + + // ensure no destructors can run even if func does not abort + _exit(exit_code); + } + + // parent process + + int wstatus; + int res; + do { + res = ::waitpid(pid, &wstatus, 0); + } while (res < 0 && errno == EINTR); + + if (res < 0) { + throw std::system_error{errno, std::system_category(), "waitpid failed"}; + } + + if (WIFEXITED(wstatus)) { + if (WEXITSTATUS(wstatus) == 0) { + return subprocess_result::exit_success; + } else { + return subprocess_result::exit_failure; + } + } else if (WIFSIGNALED(wstatus)) { + return static_cast(WTERMSIG(wstatus)); + } else { + // this should be unreachable with the flags passed to waitpid + throw std::runtime_error{"Process exited in a weird way"}; + } +} + +} // namespace detail_sandbox +} // namespace test_utility + +/** + * Run the provided code in a separate process. + * A process crash inside the code (abort, _exit without cleanup) leaves the + * parent process intact, so the parent can inspect the state the crashed + * process left on disk. + * + * @return enum describing how the subprocess exited + * @note exceptions are turned into subprocess_result::aborted + * + * @pre The application is single threaded when this is called, + * or at least the other threads are not holding any global locks. + * + * @pre The provided function must not use std::exit to exit the subprocess, + * use return instead. Ignoring this precondition causes destructors to + * run and potentially close resources the parent still needs. + * + * @example + * @code + * auto const res = METALL_SANDBOX { + * assert(false); + * }; + * + * assert(res == test_utility::subprocess_result::aborted); + * @endcode + */ +#define METALL_SANDBOX ::test_utility::detail_sandbox::sandbox{} + [&]() + +#endif // METALL_TEST_SANDBOX_HPP diff --git a/test/test_utility.hpp b/test/test_utility.hpp index f8bae3b4..da47fca3 100644 --- a/test/test_utility.hpp +++ b/test/test_utility.hpp @@ -9,6 +9,7 @@ #include "gtest/gtest.h" #include +#include #include #include_next #include @@ -39,9 +40,24 @@ inline bool create_test_dir() { return true; } +namespace detail { +// Test executables can contain identically named test cases (for example, +// manager_test and manager_test_single_thread). The program name keeps their +// datastore paths distinct when ctest runs them concurrently. +inline const char *get_program_name() { +#if defined(__GLIBC__) + return ::program_invocation_short_name; +#elif defined(__APPLE__) || defined(__FreeBSD__) + return ::getprogname(); +#else + return "unknown"; +#endif +} +} // namespace detail + inline fs::path make_test_path(const fs::path &name = fs::path()) { std::stringstream file_name; - file_name << "metalltest-" + file_name << "metalltest-" << detail::get_program_name() << "-" << ::testing::UnitTest::GetInstance()->current_test_case()->name() << "-" << ::testing::UnitTest::GetInstance()->current_test_info()->name() diff --git a/test/utility/CMakeLists.txt b/test/utility/CMakeLists.txt index 4542864d..1a36ed3c 100644 --- a/test/utility/CMakeLists.txt +++ b/test/utility/CMakeLists.txt @@ -1 +1,3 @@ -add_metall_test_executable(bitset_test bitset_test.cpp) \ No newline at end of file +add_metall_test_executable(bitset_test bitset_test.cpp) + +add_metall_test_executable(file_durability_test file_durability_test.cpp) \ No newline at end of file diff --git a/test/utility/file_durability_test.cpp b/test/utility/file_durability_test.cpp new file mode 100644 index 00000000..c6b83647 --- /dev/null +++ b/test/utility/file_durability_test.cpp @@ -0,0 +1,141 @@ +// Copyright 2026 Lawrence Livermore National Security, LLC and other Metall +// Project Developers. See the top-level COPYRIGHT file for details. +// +// SPDX-License-Identifier: (Apache-2.0 OR MIT) + +#include "gtest/gtest.h" + +#include +#include + +#include +#include + +#include + +#include "../test_utility.hpp" + +namespace { + +namespace fs = std::filesystem; +namespace mdtl = metall::mtlldetail; + +std::string read_file(const fs::path &path) { + std::ifstream ifs(path); + return std::string((std::istreambuf_iterator(ifs)), + std::istreambuf_iterator()); +} + +bool write_string(const fs::path &path, const std::string &content) { + std::ofstream ofs(path); + ofs << content; + ofs.close(); + return !!ofs; +} + +fs::path prepare_test_dir(const std::string &name) { + test_utility::create_test_dir(); + const auto dir = test_utility::make_test_path(name); + mdtl::remove_file(dir); + mdtl::create_directory(dir); + return dir; +} + +TEST(FileDurabilityTest, MakeNamedTempfile) { + const auto dir = prepare_test_dir("tempfile"); + + fs::path tmp_path; + const int fd = mdtl::make_named_tempfile(dir, &tmp_path); + ASSERT_NE(fd, -1); + EXPECT_TRUE(mdtl::file_exist(tmp_path)); + EXPECT_EQ(tmp_path.parent_path(), dir); + + EXPECT_EQ(::write(fd, "x", 1), 1); + EXPECT_TRUE(mdtl::os_close(fd)); + EXPECT_TRUE(mdtl::remove_file(tmp_path)); +} + +TEST(FileDurabilityTest, FsyncDirectory) { + const auto dir = prepare_test_dir("fsync_dir"); + EXPECT_TRUE(mdtl::fsync_directory(dir)); + EXPECT_FALSE(mdtl::fsync_directory(dir / "does_not_exist")); +} + +TEST(FileDurabilityTest, FsyncDirectoryTree) { + const auto dir = prepare_test_dir("fsync_tree"); + ASSERT_TRUE(mdtl::create_directory(dir / "a" / "b")); + ASSERT_TRUE(mdtl::create_file(dir / "a" / "file")); + EXPECT_TRUE(mdtl::fsync_directory_tree(dir)); + EXPECT_FALSE(mdtl::fsync_directory_tree(dir / "does_not_exist")); +} + +TEST(FileDurabilityTest, AtomicDurableReplaceFile) { + const auto dir = prepare_test_dir("replace"); + + ASSERT_TRUE(write_string(dir / "target", "old")); + ASSERT_TRUE(write_string(dir / "replacement", "new")); + + EXPECT_TRUE( + mdtl::atomic_durable_replace_file(dir / "target", dir / "replacement")); + EXPECT_EQ(read_file(dir / "target"), "new"); + EXPECT_FALSE(mdtl::file_exist(dir / "replacement")); + + // The replacement must exist. + EXPECT_FALSE( + mdtl::atomic_durable_replace_file(dir / "target", dir / "missing")); + EXPECT_EQ(read_file(dir / "target"), "new"); +} + +TEST(FileDurabilityTest, WriteFileAtomically) { + const auto dir = prepare_test_dir("atomic_write"); + const auto target = dir / "file"; + + // Initial write + EXPECT_TRUE(mdtl::write_file_atomically( + target, + [](const fs::path &tmp_path) { return write_string(tmp_path, "v1"); })); + EXPECT_EQ(read_file(target), "v1"); + + // Replace + EXPECT_TRUE(mdtl::write_file_atomically( + target, + [](const fs::path &tmp_path) { return write_string(tmp_path, "v2"); })); + EXPECT_EQ(read_file(target), "v2"); + + // A failed write keeps the previous content and leaves no temporary file. + EXPECT_FALSE(mdtl::write_file_atomically( + target, [](const fs::path &) { return false; })); + EXPECT_EQ(read_file(target), "v2"); + + std::size_t num_entries = 0; + for ([[maybe_unused]] const auto &entry : fs::directory_iterator(dir)) { + ++num_entries; + } + EXPECT_EQ(num_entries, 1); // only the target file + + // A target in a directory that does not exist fails. + EXPECT_FALSE(mdtl::write_file_atomically( + dir / "no_dir" / "file", + [](const fs::path &tmp_path) { return write_string(tmp_path, "x"); })); +} + +TEST(FileDurabilityTest, ExtendFileSizeManuallyWritesZeros) { + const auto dir = prepare_test_dir("extend"); + const auto file = dir / "file"; + + const int fd = ::open(file.c_str(), O_RDWR | O_CREAT, S_IRUSR | S_IWUSR); + ASSERT_NE(fd, -1); + + constexpr ssize_t size = 4096 * 2 + 123; // not a multiple of the block size + ASSERT_TRUE(mdtl::extend_file_size_manually(fd, 0, size)); + ASSERT_TRUE(mdtl::os_close(fd)); + + ASSERT_EQ(mdtl::get_file_size(file), size); + const auto content = read_file(file); + ASSERT_EQ(content.size(), std::size_t(size)); + for (const char c : content) { + ASSERT_EQ(c, '\0'); + } +} + +} // namespace From 9660c98e7f7f819d7bd5625e8f5654d0a06750e3 Mon Sep 17 00:00:00 2001 From: Alex Date: Tue, 21 Jul 2026 11:14:08 +0200 Subject: [PATCH 10/17] work around gcc failing on _Static_assert in macOS 26 SDK headers --- CMakeLists.txt | 5 +++++ cmake/check_gcc_macos_sdk.cmake | 30 ++++++++++++++++++++++++++++++ cmake/gcc_macos_sdk_fix.hpp | 10 ++++++++++ 3 files changed, 45 insertions(+) create mode 100644 cmake/check_gcc_macos_sdk.cmake create mode 100644 cmake/gcc_macos_sdk_fix.hpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 850e7d29..fe22759f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -176,6 +176,11 @@ if (NOT RUN_BUILD_AND_TEST_WITH_CI) endif () endif () +# gcc with the macOS 26 SDK needs a workaround to compile the mach headers. +# Must run before Boost is added so the flag reaches the Boost targets. +include(check_gcc_macos_sdk) +apply_gcc_macos_sdk_workaround_if_needed() + # ---------- Metall Macros ---------- # foreach (X ${COMPILER_DEFS}) message(STATUS "Metall compile definition: ${X}") diff --git a/cmake/check_gcc_macos_sdk.cmake b/cmake/check_gcc_macos_sdk.cmake new file mode 100644 index 00000000..d426135e --- /dev/null +++ b/cmake/check_gcc_macos_sdk.cmake @@ -0,0 +1,30 @@ +include(CheckCXXSourceCompiles) + +# gcc cannot compile the mach headers of the macOS 26 SDK: they use the C11 +# keyword _Static_assert, which gcc does not accept in C++. This affects +# every C++ file that includes the mach headers, for example the mac backend +# of Boost.Chrono built for the tests and benchmarks. +# The workaround force-includes cmake/gcc_macos_sdk_fix.hpp, which maps +# _Static_assert to static_assert for gcc. It is applied only when a compile +# check shows that the mach headers do not compile without it. +macro(apply_gcc_macos_sdk_workaround_if_needed) + if (CMAKE_SYSTEM_NAME STREQUAL "Darwin" AND CMAKE_CXX_COMPILER_ID STREQUAL "GNU") + check_cxx_source_compiles("#include \nint main() { return 0; }\n" + METALL_MACH_HEADERS_COMPILE) + if (NOT METALL_MACH_HEADERS_COMPILE) + set(_metall_gcc_sdk_fix_header "${PROJECT_SOURCE_DIR}/cmake/gcc_macos_sdk_fix.hpp") + set(CMAKE_REQUIRED_FLAGS "-include ${_metall_gcc_sdk_fix_header}") + check_cxx_source_compiles("#include \nint main() { return 0; }\n" + METALL_MACH_HEADERS_COMPILE_WITH_FIX) + unset(CMAKE_REQUIRED_FLAGS) + if (METALL_MACH_HEADERS_COMPILE_WITH_FIX) + message(STATUS "Applying the _Static_assert workaround for gcc with this macOS SDK") + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -include ${_metall_gcc_sdk_fix_header}") + else () + message(WARNING "The mach headers of this macOS SDK do not compile with this gcc, " + "and the _Static_assert workaround does not help. The build will likely fail.") + endif () + unset(_metall_gcc_sdk_fix_header) + endif () + endif () +endmacro() diff --git a/cmake/gcc_macos_sdk_fix.hpp b/cmake/gcc_macos_sdk_fix.hpp new file mode 100644 index 00000000..47d2dae9 --- /dev/null +++ b/cmake/gcc_macos_sdk_fix.hpp @@ -0,0 +1,10 @@ +// The macOS 26 SDK uses the C11 keyword _Static_assert at file scope in +// headers that are also compiled as C++ (mach/port.h, mach/message.h). +// clang accepts _Static_assert in C++ as an extension, gcc does not, so gcc +// fails on any C++ file that includes the mach headers. +// This mapping makes the SDK headers compile with gcc. The build system +// force-includes this file (-include) when it detects the problem, see +// cmake/check_gcc_macos_sdk.cmake. +#if defined(__GNUC__) && !defined(__clang__) && defined(__cplusplus) +#define _Static_assert static_assert +#endif From 62a9126905901a880ba626577e687aa3e943673b Mon Sep 17 00:00:00 2001 From: Alex Date: Tue, 21 Jul 2026 11:48:43 +0200 Subject: [PATCH 11/17] fix leaks and zero-size VLAs in tests found by ASan and UBSan --- test/container/fallback_allocator_test.cpp | 1 + test/kernel/manager_test.cpp | 2 ++ test/kernel/multilayer_bitset_test.cpp | 9 +++++++-- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/test/container/fallback_allocator_test.cpp b/test/container/fallback_allocator_test.cpp index e207d457..1c9f7cf2 100644 --- a/test/container/fallback_allocator_test.cpp +++ b/test/container/fallback_allocator_test.cpp @@ -97,6 +97,7 @@ TEST(FallbackAllocatorAdaptorTest, Types) { GTEST_ASSERT_EQ(p->a, 10); GTEST_ASSERT_EQ(p->b, 20.0); std::allocator_traits::destroy(alloc, metall::to_raw_pointer(p)); + std::allocator_traits::deallocate(alloc, p, 1); } GTEST_ASSERT_EQ(std::allocator_traits::max_size(alloc), diff --git a/test/kernel/manager_test.cpp b/test/kernel/manager_test.cpp index 26ae3d2e..16272cd4 100644 --- a/test/kernel/manager_test.cpp +++ b/test/kernel/manager_test.cpp @@ -1442,6 +1442,7 @@ TEST(ManagerTest, CheckSanity) { auto *manager = new manager_type(metall::create_only, dir_path()); ASSERT_TRUE(manager->check_sanity()); ASSERT_FALSE(manager->read_only()); + delete manager; } { @@ -1449,6 +1450,7 @@ TEST(ManagerTest, CheckSanity) { new manager_type(metall::open_only, dir_path().string() + "-invalid"); ASSERT_FALSE(bad_manager->check_sanity()); ASSERT_TRUE(bad_manager->read_only()); + delete bad_manager; } } } // namespace diff --git a/test/kernel/multilayer_bitset_test.cpp b/test/kernel/multilayer_bitset_test.cpp index 4fbc6cff..7edcb6b4 100644 --- a/test/kernel/multilayer_bitset_test.cpp +++ b/test/kernel/multilayer_bitset_test.cpp @@ -129,6 +129,9 @@ void RandomSetAndResetHelper2(const std::size_t num_bits) { } } else { const auto n = std::min((size_t)dist(rnd_gen), num_bits - cnt_trues); + if (n == 0) { + continue; + } metall::kernel::multilayer_bitset::bit_position_type buf[n]; bitset.find_and_set_many(num_bits, n, buf); cnt_trues += n; @@ -156,8 +159,10 @@ void RandomSetAndResetHelper2(const std::size_t num_bits) { // Set the remaining bits { const auto num_rem = num_bits - cnt_trues; - metall::kernel::multilayer_bitset::bit_position_type buf[num_rem]; - bitset.find_and_set_many(num_bits, num_rem, buf); + if (num_rem > 0) { + metall::kernel::multilayer_bitset::bit_position_type buf[num_rem]; + bitset.find_and_set_many(num_bits, num_rem, buf); + } } for (std::size_t pos = 0; pos < num_bits; ++pos) { ASSERT_TRUE(bitset.get(num_bits, pos)) << "pos = " << pos; From b51b88129a19b2535fc4d38f49329ba909daf5a8 Mon Sep 17 00:00:00 2001 From: Alex Date: Tue, 21 Jul 2026 13:54:02 +0200 Subject: [PATCH 12/17] Revert "remove forced libc++ for clang builds" This reverts commit 7a4a21e7858093feca559230b441d1dad30bacf9. --- CMakeLists.txt | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/CMakeLists.txt b/CMakeLists.txt index fe22759f..be38cc90 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -270,6 +270,15 @@ function(common_setup_for_metall_executable name) # ----- Compile Options ----- # add_common_compile_options(${name}) + # Memo: + # On macOS and FreeBSD libc++ is the default standard library and the -stdlib=libc++ is not required. + # https://libcxx.llvm.org/docs/UsingLibcxx.html + if (CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND + NOT (CMAKE_CXX_COMPILER_ID MATCHES "Darwin" OR CMAKE_CXX_COMPILER_ID MATCHES "FreeBSD")) + target_compile_options(${name} PRIVATE -stdlib=libc++) + endif () + # -------------------- + # ----- Compile Definitions ----- # foreach (X ${COMPILER_DEFS}) target_compile_definitions(${name} PRIVATE ${X}) From f1e222b2ef293b8869fb060133565be6e6f8c07d Mon Sep 17 00:00:00 2001 From: Alex Date: Tue, 21 Jul 2026 14:39:23 +0200 Subject: [PATCH 13/17] address review --- CMakeLists.txt | 6 --- cmake/check_gcc_macos_sdk.cmake | 30 -------------- cmake/gcc_macos_sdk_fix.hpp | 10 ----- include/metall/detail/file.hpp | 12 ++---- .../metall/detail/properly_closed_mark.hpp | 2 +- include/metall/kernel/manager_kernel.hpp | 2 +- include/metall/kernel/manager_kernel_impl.ipp | 10 +++-- test/kernel/manager_test.cpp | 39 +++++++++++++++++++ test/test_utility.hpp | 23 +++-------- 9 files changed, 57 insertions(+), 77 deletions(-) delete mode 100644 cmake/check_gcc_macos_sdk.cmake delete mode 100644 cmake/gcc_macos_sdk_fix.hpp diff --git a/CMakeLists.txt b/CMakeLists.txt index be38cc90..dc7dd005 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -175,12 +175,6 @@ if (NOT RUN_BUILD_AND_TEST_WITH_CI) endif () endif () endif () - -# gcc with the macOS 26 SDK needs a workaround to compile the mach headers. -# Must run before Boost is added so the flag reaches the Boost targets. -include(check_gcc_macos_sdk) -apply_gcc_macos_sdk_workaround_if_needed() - # ---------- Metall Macros ---------- # foreach (X ${COMPILER_DEFS}) message(STATUS "Metall compile definition: ${X}") diff --git a/cmake/check_gcc_macos_sdk.cmake b/cmake/check_gcc_macos_sdk.cmake deleted file mode 100644 index d426135e..00000000 --- a/cmake/check_gcc_macos_sdk.cmake +++ /dev/null @@ -1,30 +0,0 @@ -include(CheckCXXSourceCompiles) - -# gcc cannot compile the mach headers of the macOS 26 SDK: they use the C11 -# keyword _Static_assert, which gcc does not accept in C++. This affects -# every C++ file that includes the mach headers, for example the mac backend -# of Boost.Chrono built for the tests and benchmarks. -# The workaround force-includes cmake/gcc_macos_sdk_fix.hpp, which maps -# _Static_assert to static_assert for gcc. It is applied only when a compile -# check shows that the mach headers do not compile without it. -macro(apply_gcc_macos_sdk_workaround_if_needed) - if (CMAKE_SYSTEM_NAME STREQUAL "Darwin" AND CMAKE_CXX_COMPILER_ID STREQUAL "GNU") - check_cxx_source_compiles("#include \nint main() { return 0; }\n" - METALL_MACH_HEADERS_COMPILE) - if (NOT METALL_MACH_HEADERS_COMPILE) - set(_metall_gcc_sdk_fix_header "${PROJECT_SOURCE_DIR}/cmake/gcc_macos_sdk_fix.hpp") - set(CMAKE_REQUIRED_FLAGS "-include ${_metall_gcc_sdk_fix_header}") - check_cxx_source_compiles("#include \nint main() { return 0; }\n" - METALL_MACH_HEADERS_COMPILE_WITH_FIX) - unset(CMAKE_REQUIRED_FLAGS) - if (METALL_MACH_HEADERS_COMPILE_WITH_FIX) - message(STATUS "Applying the _Static_assert workaround for gcc with this macOS SDK") - set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -include ${_metall_gcc_sdk_fix_header}") - else () - message(WARNING "The mach headers of this macOS SDK do not compile with this gcc, " - "and the _Static_assert workaround does not help. The build will likely fail.") - endif () - unset(_metall_gcc_sdk_fix_header) - endif () - endif () -endmacro() diff --git a/cmake/gcc_macos_sdk_fix.hpp b/cmake/gcc_macos_sdk_fix.hpp deleted file mode 100644 index 47d2dae9..00000000 --- a/cmake/gcc_macos_sdk_fix.hpp +++ /dev/null @@ -1,10 +0,0 @@ -// The macOS 26 SDK uses the C11 keyword _Static_assert at file scope in -// headers that are also compiled as C++ (mach/port.h, mach/message.h). -// clang accepts _Static_assert in C++ as an extension, gcc does not, so gcc -// fails on any C++ file that includes the mach headers. -// This mapping makes the SDK headers compile with gcc. The build system -// force-includes this file (-include) when it detects the problem, see -// cmake/check_gcc_macos_sdk.cmake. -#if defined(__GNUC__) && !defined(__clang__) && defined(__cplusplus) -#define _Static_assert static_assert -#endif diff --git a/include/metall/detail/file.hpp b/include/metall/detail/file.hpp index 74f67620..bf481cb9 100644 --- a/include/metall/detail/file.hpp +++ b/include/metall/detail/file.hpp @@ -105,11 +105,7 @@ inline bool fsync(const fs::path &path) { /// \param dir_path A path to a directory. /// \return On success, returns true. On error, returns false. inline bool fsync_directory(const fs::path &dir_path) { -#ifdef O_DIRECTORY const int fd = ::open(dir_path.c_str(), O_RDONLY | O_DIRECTORY); -#else - const int fd = ::open(dir_path.c_str(), O_RDONLY); -#endif if (fd == -1) { const std::string s("open directory for fsync: " + dir_path.string()); logger::perror(logger::level::error, __FILE__, __LINE__, s.c_str()); @@ -194,12 +190,12 @@ inline int make_named_tempfile(const fs::path &parent_path, /// to final_path, and the rename is made durable. On any failure the previous /// content of final_path stays intact and the temporary file is removed. /// \param final_path The path of the file to write. -/// \param write_func A function bool(const fs::path &) that writes the file +/// \param write_func A callable bool(const fs::path &) that writes the file /// content to the given path. /// \return On success, returns true. On error, returns false. -inline bool write_file_atomically( - const fs::path &final_path, - const std::function &write_func) { +template +inline bool write_file_atomically(const fs::path &final_path, + write_function_type &&write_func) { fs::path tmp_path; const int fd = make_named_tempfile(final_path.parent_path(), &tmp_path); if (fd == -1) { diff --git a/include/metall/detail/properly_closed_mark.hpp b/include/metall/detail/properly_closed_mark.hpp index 1a3438cc..83918cb7 100644 --- a/include/metall/detail/properly_closed_mark.hpp +++ b/include/metall/detail/properly_closed_mark.hpp @@ -135,7 +135,7 @@ struct properly_closed_mark { return true; } - /// \brief Takes the lock and consumes the mark for an existing datastore. + /// \brief Takes the lock and checks the mark for an existing datastore. /// Writer: takes the exclusive lock, requires the mark, removes it and makes /// the removal durable before the caller modifies any data. /// Reader: takes the shared lock and requires the mark. diff --git a/include/metall/kernel/manager_kernel.hpp b/include/metall/kernel/manager_kernel.hpp index bbd68ff8..b9f180ab 100644 --- a/include/metall/kernel/manager_kernel.hpp +++ b/include/metall/kernel/manager_kernel.hpp @@ -509,7 +509,7 @@ class manager_kernel { static path_type priv_lock_file_path(const path_type &base_path); - void priv_undo_consumed_mark(); + void priv_restore_properly_closed_mark(); /// Publishes a fully written datastore copy: fsyncs the directory tree, /// creates the destination lockfile, replaces the datastore root of diff --git a/include/metall/kernel/manager_kernel_impl.ipp b/include/metall/kernel/manager_kernel_impl.ipp index 0fd25f12..99d4a374 100644 --- a/include/metall/kernel/manager_kernel_impl.ipp +++ b/include/metall/kernel/manager_kernel_impl.ipp @@ -627,7 +627,9 @@ bool manager_kernel::priv_create_datastore_directory( return false; } - // Make the new directory entries durable. + // Make the new directory entries durable. The fsync on base_path covers + // the entry of the new root directory. The tree fsync covers the root + // directory and the directories below it. const auto root_dir = storage::get_path(base_path, k_management_dir_name).parent_path(); if (!mdtl::fsync_directory(base_path) || @@ -895,14 +897,14 @@ bool manager_kernel::priv_open( read_only)) { logger::out(logger::level::error, __FILE__, __LINE__, "Failed to open the application data segment"); - priv_undo_consumed_mark(); + priv_restore_properly_closed_mark(); return false; } m_segment_storage.get_segment_header().manager_kernel_address = this; if (!priv_deserialize_management_data()) { m_segment_storage.release(); - priv_undo_consumed_mark(); + priv_restore_properly_closed_mark(); return false; } @@ -910,7 +912,7 @@ bool manager_kernel::priv_open( } template -void manager_kernel::priv_undo_consumed_mark() { +void manager_kernel::priv_restore_properly_closed_mark() { // A failed open did not modify the datastore. Recreating the mark keeps the // datastore openable. if (!m_properly_closed_mark.is_read_only()) { diff --git a/test/kernel/manager_test.cpp b/test/kernel/manager_test.cpp index 16272cd4..e6e77d02 100644 --- a/test/kernel/manager_test.cpp +++ b/test/kernel/manager_test.cpp @@ -94,6 +94,16 @@ TEST(ManagerTest, CreateAndOpenModes) { { manager_type::remove(dir_path()); + // on a datastore that does not exist + { + manager_type manager{metall::open_only, dir_path()}; + ASSERT_FALSE(manager.check_sanity()); + } + { + manager_type manager{metall::open_read_only, dir_path()}; + ASSERT_FALSE(manager.check_sanity()); + } + // after create { manager_type manager{metall::create_only, dir_path()}; @@ -113,6 +123,11 @@ TEST(ManagerTest, CreateAndOpenModes) { manager_type manager2{metall::open_only, dir_path()}; ASSERT_FALSE(manager2.check_sanity()); } + + { + manager_type manager2{metall::create_only, dir_path()}; + ASSERT_FALSE(manager2.check_sanity()); + } } // after open @@ -129,6 +144,11 @@ TEST(ManagerTest, CreateAndOpenModes) { manager_type manager2{metall::open_only, dir_path()}; ASSERT_FALSE(manager2.check_sanity()); } + + { + manager_type manager2{metall::create_only, dir_path()}; + ASSERT_FALSE(manager2.check_sanity()); + } } // after read-only open @@ -144,9 +164,28 @@ TEST(ManagerTest, CreateAndOpenModes) { ASSERT_FALSE(manager3.check_sanity()); } + { + manager_type manager2{metall::create_only, dir_path()}; + ASSERT_FALSE(manager2.check_sanity()); + } + manager_type manager2{metall::open_read_only, dir_path()}; ASSERT_TRUE(manager2.check_sanity()); } + + // after close, every mode works again and the datastore is intact + { + manager_type manager{metall::open_only, dir_path()}; + ASSERT_TRUE(manager.check_sanity()); + } + { + manager_type manager{metall::open_read_only, dir_path()}; + ASSERT_TRUE(manager.check_sanity()); + } + { + manager_type manager{metall::create_only, dir_path()}; + ASSERT_TRUE(manager.check_sanity()); + } } } diff --git a/test/test_utility.hpp b/test/test_utility.hpp index da47fca3..d29ecf19 100644 --- a/test/test_utility.hpp +++ b/test/test_utility.hpp @@ -9,8 +9,8 @@ #include "gtest/gtest.h" #include -#include #include +#include #include_next #include @@ -40,24 +40,13 @@ inline bool create_test_dir() { return true; } -namespace detail { -// Test executables can contain identically named test cases (for example, -// manager_test and manager_test_single_thread). The program name keeps their -// datastore paths distinct when ctest runs them concurrently. -inline const char *get_program_name() { -#if defined(__GLIBC__) - return ::program_invocation_short_name; -#elif defined(__APPLE__) || defined(__FreeBSD__) - return ::getprogname(); -#else - return "unknown"; -#endif -} -} // namespace detail - inline fs::path make_test_path(const fs::path &name = fs::path()) { + // Test executables can contain identically named test cases (for example, + // manager_test and manager_test_single_thread). A per-process random value + // keeps their datastore paths distinct when ctest runs them concurrently. + static const unsigned int process_random_value = std::random_device{}(); std::stringstream file_name; - file_name << "metalltest-" << detail::get_program_name() << "-" + file_name << "metalltest-" << process_random_value << "-" << ::testing::UnitTest::GetInstance()->current_test_case()->name() << "-" << ::testing::UnitTest::GetInstance()->current_test_info()->name() From 2c84f8d75a31c1d1f6d3e9f7b439626e2f3719a6 Mon Sep 17 00:00:00 2001 From: Alex Date: Tue, 21 Jul 2026 16:04:18 +0200 Subject: [PATCH 14/17] address review (2) --- .../metall/detail/properly_closed_mark.hpp | 58 +++++++++++-------- test/kernel/durability_test.cpp | 22 ++++--- 2 files changed, 49 insertions(+), 31 deletions(-) diff --git a/include/metall/detail/properly_closed_mark.hpp b/include/metall/detail/properly_closed_mark.hpp index 83918cb7..cca36820 100644 --- a/include/metall/detail/properly_closed_mark.hpp +++ b/include/metall/detail/properly_closed_mark.hpp @@ -40,37 +40,47 @@ struct properly_closed_mark { std::filesystem::path m_mark_path{}; bool m_read_only = false; - static int open_or_create_lockfile( - const std::filesystem::path &lockfile_path) { + // The lockfile is created only when a datastore is created. A missing + // lockfile on open means the datastore is broken or incomplete. + static int open_lockfile(const std::filesystem::path &lockfile_path) { const int fd = ::open(lockfile_path.c_str(), O_RDONLY); + if (fd < 0) { + std::string s("open lockfile (a missing lockfile means a broken " + "datastore): " + + lockfile_path.string()); + logger::perror(logger::level::error, __FILE__, __LINE__, s.c_str()); + } + return fd; + } + + // Creates the lockfile for a new datastore. A datastore create may also + // target an existing datastore; then the existing lockfile is opened + // instead. It is never replaced with a new inode: all processes must lock + // one inode, or two of them could hold "exclusive" locks on different + // inodes of the same path. + static int create_lockfile(const std::filesystem::path &lockfile_path) { + const int fd = ::open(lockfile_path.c_str(), + O_CREAT | O_EXCL | O_RDONLY, S_IRUSR | S_IWUSR); if (fd >= 0) { + // The new lockfile must exist durably before the lock has any meaning + // for other processes. + fsync_directory(lockfile_path.parent_path()); return fd; } - if (errno != ENOENT) { - std::string s("open lockfile: " + lockfile_path.string()); - logger::perror(logger::level::error, __FILE__, __LINE__, s.c_str()); - return -1; - } - // The lockfile does not exist yet (datastore created by an older version). - const int new_fd = - ::open(lockfile_path.c_str(), O_CREAT | O_RDONLY, S_IRUSR | S_IWUSR); - if (new_fd < 0) { - std::string s("create lockfile: " + lockfile_path.string()); - logger::perror(logger::level::error, __FILE__, __LINE__, s.c_str()); - return -1; + if (errno == EEXIST) { + // Re-creating an existing datastore. + return open_lockfile(lockfile_path); } - // The lockfile must exist before the lock has any meaning for other - // processes. - fsync_directory(lockfile_path.parent_path()); - return new_fd; + + std::string s("create lockfile: " + lockfile_path.string()); + logger::perror(logger::level::error, __FILE__, __LINE__, s.c_str()); + return -1; } - bool lock(const std::filesystem::path &lockfile_path, - const bool shared) { + // Takes the flock on an open lockfile descriptor. Owns fd on success. + bool lock(const int fd, const bool shared) { assert(m_lock_fd == -1); - - const int fd = open_or_create_lockfile(lockfile_path); if (fd < 0) { return false; } @@ -127,7 +137,7 @@ struct properly_closed_mark { /// open datastore), returns false. bool create(const std::filesystem::path &lockfile_path, const std::filesystem::path &mark_path) { - if (!lock(lockfile_path, false)) { + if (!lock(create_lockfile(lockfile_path), false)) { return false; } m_mark_path = mark_path; @@ -143,7 +153,7 @@ struct properly_closed_mark { /// properly closed), returns false. bool open(const std::filesystem::path &lockfile_path, const std::filesystem::path &mark_path, const bool read_only) { - if (!lock(lockfile_path, read_only)) { + if (!lock(open_lockfile(lockfile_path), read_only)) { return false; } diff --git a/test/kernel/durability_test.cpp b/test/kernel/durability_test.cpp index 272a4a9b..bdde41db 100644 --- a/test/kernel/durability_test.cpp +++ b/test/kernel/durability_test.cpp @@ -192,8 +192,9 @@ TEST(DurabilityTest, RemoveRefusedWhileOpen) { EXPECT_FALSE(metall::manager::consistent(path)); } -// A datastore created by a version without a lockfile opens normally. -TEST(DurabilityTest, OpenWithoutLockfile) { +// The lockfile is created once at datastore creation. A datastore without a +// lockfile is broken and refuses to open. Re-creation makes it usable again. +TEST(DurabilityTest, MissingLockfileRefusesOpen) { const auto path = ds_path("no_lockfile"); metall::manager::remove(path); @@ -203,16 +204,23 @@ TEST(DurabilityTest, OpenWithoutLockfile) { manager.construct("v")(9); } + ASSERT_TRUE(fs::exists(path / k_lock_file_name)); ASSERT_TRUE(fs::remove(path / k_lock_file_name)); { metall::manager manager(metall::open_only, path); - ASSERT_TRUE(manager.check_sanity()); - const auto v = manager.find("v"); - ASSERT_NE(v.first, nullptr); - EXPECT_EQ(*v.first, 9); + EXPECT_FALSE(manager.check_sanity()); + } + { + metall::manager manager(metall::open_read_only, path); + EXPECT_FALSE(manager.check_sanity()); + } + + // create() recreates the lockfile and the datastore. + { + metall::manager manager(metall::create_only, path); + EXPECT_TRUE(manager.check_sanity()); } - EXPECT_TRUE(metall::manager::consistent(path)); EXPECT_TRUE(fs::exists(path / k_lock_file_name)); } From e808846bcce23a389e489728ba9a8b81c8410eb0 Mon Sep 17 00:00:00 2001 From: Alex Date: Tue, 21 Jul 2026 16:10:39 +0200 Subject: [PATCH 15/17] address review (3) --- include/metall/kernel/manager_kernel_impl.ipp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/include/metall/kernel/manager_kernel_impl.ipp b/include/metall/kernel/manager_kernel_impl.ipp index 99d4a374..824fbda0 100644 --- a/include/metall/kernel/manager_kernel_impl.ipp +++ b/include/metall/kernel/manager_kernel_impl.ipp @@ -630,10 +630,8 @@ bool manager_kernel::priv_create_datastore_directory( // Make the new directory entries durable. The fsync on base_path covers // the entry of the new root directory. The tree fsync covers the root // directory and the directories below it. - const auto root_dir = - storage::get_path(base_path, k_management_dir_name).parent_path(); if (!mdtl::fsync_directory(base_path) || - !mdtl::fsync_directory_tree(root_dir)) { + !mdtl::fsync_directory_tree(storage::root_path(base_path))) { logger::out(logger::level::error, __FILE__, __LINE__, "Failed to fsync the datastore directories"); return false; From 0d351e3d554a508042948112fc977eb8142bc55b Mon Sep 17 00:00:00 2001 From: Alex Date: Fri, 24 Jul 2026 10:11:58 +0200 Subject: [PATCH 16/17] address review (4) --- .../metall/detail/properly_closed_mark.hpp | 27 +++++++++---------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/include/metall/detail/properly_closed_mark.hpp b/include/metall/detail/properly_closed_mark.hpp index cca36820..5f3509ef 100644 --- a/include/metall/detail/properly_closed_mark.hpp +++ b/include/metall/detail/properly_closed_mark.hpp @@ -59,23 +59,20 @@ struct properly_closed_mark { // one inode, or two of them could hold "exclusive" locks on different // inodes of the same path. static int create_lockfile(const std::filesystem::path &lockfile_path) { - const int fd = ::open(lockfile_path.c_str(), - O_CREAT | O_EXCL | O_RDONLY, S_IRUSR | S_IWUSR); - if (fd >= 0) { - // The new lockfile must exist durably before the lock has any meaning - // for other processes. - fsync_directory(lockfile_path.parent_path()); - return fd; - } - - if (errno == EEXIST) { - // Re-creating an existing datastore. - return open_lockfile(lockfile_path); + // Creates the file if absent or opens the existing + // inode otherwise. + const int fd = ::open(lockfile_path.c_str(), O_CREAT | O_RDONLY, + S_IRUSR | S_IWUSR); + if (fd < 0) { + std::string s("create lockfile: " + lockfile_path.string()); + logger::perror(logger::level::error, __FILE__, __LINE__, s.c_str()); + return -1; } - std::string s("create lockfile: " + lockfile_path.string()); - logger::perror(logger::level::error, __FILE__, __LINE__, s.c_str()); - return -1; + // The new lockfile must exist durably before the lock has any meaning + // for other processes. Fsyncing when the file already existed is idempotent. + fsync_directory(lockfile_path.parent_path()); + return fd; } // Takes the flock on an open lockfile descriptor. Owns fd on success. From b5df68f5372400993feaa48884d0151f2d09b7f7 Mon Sep 17 00:00:00 2001 From: Alex Date: Fri, 24 Jul 2026 10:19:47 +0200 Subject: [PATCH 17/17] address review (5) --- include/metall/kernel/manager_kernel_impl.ipp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/include/metall/kernel/manager_kernel_impl.ipp b/include/metall/kernel/manager_kernel_impl.ipp index 824fbda0..514ef10a 100644 --- a/include/metall/kernel/manager_kernel_impl.ipp +++ b/include/metall/kernel/manager_kernel_impl.ipp @@ -627,11 +627,9 @@ bool manager_kernel::priv_create_datastore_directory( return false; } - // Make the new directory entries durable. The fsync on base_path covers - // the entry of the new root directory. The tree fsync covers the root - // directory and the directories below it. - if (!mdtl::fsync_directory(base_path) || - !mdtl::fsync_directory_tree(storage::root_path(base_path))) { + // recursive directory fsync on the base path also covers + // the root path and the directories it contains + if (!mdtl::fsync_directory_tree(base_path)) { logger::out(logger::level::error, __FILE__, __LINE__, "Failed to fsync the datastore directories"); return false;