From 95e6375813f7970ff1dd80126d18a1bd3b2c3bd3 Mon Sep 17 00:00:00 2001 From: Oleg Sidorkin Date: Wed, 4 Jan 2023 16:53:21 +0300 Subject: [PATCH] fix: Use spinlocks for inode cache memory synchronization (#1229) Changed the inode cache implementation to use spinlocks instead of pthread mutexes. This makes the inode cache work on FreeBSD and other systems where the pthread mutexes are destroyed when the last memory mapping containing the mutexes is unmapped. Also added tmpfs, ufs and zfs to the list of supported filesystems on macOS and BSDs. See also ccache discussion #1228. --- src/InodeCache.cpp | 99 +++++++++++++++++++++++------------- src/InodeCache.hpp | 1 + test/suites/inode_cache.bash | 2 +- unittest/test_InodeCache.cpp | 2 +- 4 files changed, 67 insertions(+), 37 deletions(-) diff --git a/src/InodeCache.cpp b/src/InodeCache.cpp index 934227916..b88ac23b2 100644 --- a/src/InodeCache.cpp +++ b/src/InodeCache.cpp @@ -33,6 +33,7 @@ #include #include +#include #include #include @@ -67,7 +68,7 @@ namespace { // Note: The key is hashed using the main hash algorithm, so the version number // does not need to be incremented if said algorithm is changed (except if the // digest size changes since that affects the entry format). -const uint32_t k_version = 1; +const uint32_t k_version = 2; // Note: Increment the version number if constants affecting storage size are // changed. @@ -88,6 +89,8 @@ static_assert( const void* MMAP_FAILED = reinterpret_cast(-1); // NOLINT: Must cast here +const auto MAX_LOCK_DURATION = util::Duration(5); + bool fd_is_on_known_to_work_file_system(int fd) { @@ -119,7 +122,10 @@ fd_is_on_known_to_work_file_system(int fd) // Is a filesystem you know works with the inode cache missing in this // list? Please submit an issue or pull request to the ccache project. "apfs", + "tmpfs", + "ufs", "xfs", + "zfs", }; if (std::find(known_to_work_filesystems.begin(), known_to_work_filesystems.end(), @@ -140,6 +146,48 @@ fd_is_on_known_to_work_file_system(int fd) return known_to_work; } +bool +spin_lock(std::atomic& owner_pid, const pid_t self_pid) +{ + pid_t prev_pid = 0; + pid_t lock_pid = 0; + bool reset_timer = false; + util::TimePoint lock_time; + for (;;) { + for (int i = 0; i < 10000; ++i) { + lock_pid = owner_pid.load(std::memory_order_relaxed); + if (lock_pid == 0 + && owner_pid.compare_exchange_weak( + lock_pid, self_pid, std::memory_order_acquire)) { + return false; + } + + if (prev_pid != lock_pid) { + // Check for changing pid here so ABA locking is detected with better + // probability + prev_pid = lock_pid; + reset_timer = true; + } + sched_yield(); + } + // If everything is ok, we should never hit this + if (reset_timer) { + lock_time = util::TimePoint::now(); + reset_timer = false; + } else { + if (util::TimePoint::now() - lock_time > MAX_LOCK_DURATION) { + return true; + } + } + } +} + +void +spin_unlock(std::atomic& owner_pid) +{ + owner_pid.store(0, std::memory_order_release); +} + } // namespace struct InodeCache::Key @@ -162,7 +210,7 @@ struct InodeCache::Entry struct InodeCache::Bucket { - pthread_mutex_t mt; + std::atomic owner_pid; Entry entries[k_num_entries]; }; @@ -258,40 +306,25 @@ InodeCache::with_bucket(const Digest& key_digest, Util::big_endian_to_int(key_digest.bytes(), hash); const uint32_t index = hash % k_num_buckets; Bucket* bucket = &m_sr->buckets[index]; - int err = pthread_mutex_lock(&bucket->mt); -#ifdef HAVE_PTHREAD_MUTEX_ROBUST - if (err == EOWNERDEAD) { - if (m_config.debug()) { - ++m_sr->errors; - } - err = pthread_mutex_consistent(&bucket->mt); - if (err) { - LOG( - "Can't consolidate stale mutex at index {}: {}", index, strerror(err)); - LOG_RAW("Consider removing the inode cache file if the problem persists"); + bool broken_lock = spin_lock(bucket->owner_pid, m_self_pid); + while (broken_lock) { + LOG("Wiping inodes cache because of stale mutex at index {}", index); + if (!drop() || !initialize()) { return false; } - LOG("Wiping bucket at index {} because of stale mutex", index); - memset(bucket->entries, 0, sizeof(Bucket::entries)); - } else { -#endif - if (err != 0) { - LOG("Failed to lock mutex at index {}: {}", index, strerror(err)); - LOG_RAW("Consider removing the inode cache file if problem persists"); + if (m_config.debug()) { ++m_sr->errors; - return false; } -#ifdef HAVE_PTHREAD_MUTEX_ROBUST + bucket = &m_sr->buckets[index]; + broken_lock = spin_lock(bucket->owner_pid, m_self_pid); } -#endif - try { bucket_handler(bucket); } catch (...) { - pthread_mutex_unlock(&bucket->mt); + spin_unlock(bucket->owner_pid); throw; } - pthread_mutex_unlock(&bucket->mt); + spin_unlock(bucket->owner_pid); return true; } @@ -326,14 +359,9 @@ InodeCache::create_new_file(const std::string& filename) // Initialize new shared region. sr->version = k_version; - pthread_mutexattr_t mattr; - pthread_mutexattr_init(&mattr); - pthread_mutexattr_setpshared(&mattr, PTHREAD_PROCESS_SHARED); -#ifdef HAVE_PTHREAD_MUTEX_ROBUST - pthread_mutexattr_setrobust(&mattr, PTHREAD_MUTEX_ROBUST); -#endif for (auto& bucket : sr->buckets) { - pthread_mutex_init(&bucket.mt, &mattr); + bucket.owner_pid = 0; + memset(bucket.entries, 0, sizeof(Bucket::entries)); } munmap(sr, sizeof(SharedRegion)); @@ -389,7 +417,8 @@ InodeCache::InodeCache(const Config& config, util::Duration min_age) // CCACHE_DISABLE_INODE_CACHE_MIN_AGE is only for testing purposes; see // test/suites/inode_cache.bash. m_min_age(getenv("CCACHE_DISABLE_INODE_CACHE_MIN_AGE") ? util::Duration(0) - : min_age) + : min_age), + m_self_pid(getpid()) { } @@ -498,7 +527,7 @@ bool InodeCache::drop() { std::string file = get_file(); - if (unlink(file.c_str()) != 0) { + if (unlink(file.c_str()) != 0 && errno != ENOENT) { return false; } LOG("Dropped inode cache {}", file); diff --git a/src/InodeCache.hpp b/src/InodeCache.hpp index e270ef15c..c4e8c4eab 100644 --- a/src/InodeCache.hpp +++ b/src/InodeCache.hpp @@ -132,4 +132,5 @@ private: util::Duration m_min_age; struct SharedRegion* m_sr = nullptr; bool m_failed = false; + const pid_t m_self_pid; }; diff --git a/test/suites/inode_cache.bash b/test/suites/inode_cache.bash index 830ef8c75..51a809f86 100644 --- a/test/suites/inode_cache.bash +++ b/test/suites/inode_cache.bash @@ -9,7 +9,7 @@ SUITE_inode_cache_PROBE() { touch test.c $CCACHE $COMPILER -c test.c - if [[ ! -f "${CCACHE_TEMPDIR}/inode-cache-32.v1" && ! -f "${CCACHE_TEMPDIR}/inode-cache-64.v1" ]]; then + if [[ ! -f "${CCACHE_TEMPDIR}/inode-cache-32.v2" && ! -f "${CCACHE_TEMPDIR}/inode-cache-64.v2" ]]; then local fs_type=$(stat -fLc %T "${CCACHE_DIR}") echo "inode cache not supported on ${fs_type}" fi diff --git a/unittest/test_InodeCache.cpp b/unittest/test_InodeCache.cpp index b02adab08..5f08f36fb 100644 --- a/unittest/test_InodeCache.cpp +++ b/unittest/test_InodeCache.cpp @@ -177,7 +177,7 @@ TEST_CASE("Drop file") CHECK(Stat::stat(inode_cache.get_file())); CHECK(inode_cache.drop()); CHECK(!Stat::stat(inode_cache.get_file())); - CHECK(!inode_cache.drop()); + CHECK(inode_cache.drop()); } TEST_CASE("Test content type") -- 2.47.2