From a331880c88668c8c1e793c12acd88bc1d60f9ee0 Mon Sep 17 00:00:00 2001 From: Dan Engelbrecht Date: Mon, 11 Dec 2023 03:44:00 -0500 Subject: fix deadlock at bucket creation (#598) - Make sure we don't hold the namespace bucket lock when we create buckets to avoid deadlock - Pass lock scope to helper functions to clarify locking rules - Block flush and gc operations for a bucket that is not yet initialized - Add ZenCacheDiskLayer::GetOrCreateBucket to avoid code duplication --- src/zenserver/cache/cachedisklayer.cpp | 338 +++++++++++++++++---------------- 1 file changed, 175 insertions(+), 163 deletions(-) (limited to 'src/zenserver/cache/cachedisklayer.cpp') diff --git a/src/zenserver/cache/cachedisklayer.cpp b/src/zenserver/cache/cachedisklayer.cpp index 700529443..13f3c9e58 100644 --- a/src/zenserver/cache/cachedisklayer.cpp +++ b/src/zenserver/cache/cachedisklayer.cpp @@ -246,7 +246,8 @@ public: return OutVersion == CurrentDiskBucketVersion; } - void ParseManifest(ZenCacheDiskLayer::CacheBucket& Bucket, + void ParseManifest(RwLock::ExclusiveLockScope& BucketLock, + ZenCacheDiskLayer::CacheBucket& Bucket, std::filesystem::path ManifestPath, ZenCacheDiskLayer::CacheBucket::IndexMap& Index, std::vector& AccessTimes, @@ -256,13 +257,15 @@ public: IoBuffer MakeSidecarManifest(const Oid& BucketId, uint64_t EntryCount); uint64_t GetSidecarSize() const { return m_ManifestEntryCount * sizeof(ManifestData); } - void WriteSidecarFile(const std::filesystem::path& SidecarPath, + void WriteSidecarFile(RwLock::SharedLockScope& BucketLock, + const std::filesystem::path& SidecarPath, uint64_t SnapshotLogPosition, const ZenCacheDiskLayer::CacheBucket::IndexMap& Index, const std::vector& AccessTimes, const std::vector& Payloads, const std::vector& MetaDatas); - bool ReadSidecarFile(ZenCacheDiskLayer::CacheBucket& Bucket, + bool ReadSidecarFile(RwLock::ExclusiveLockScope& BucketLock, + ZenCacheDiskLayer::CacheBucket& Bucket, std::filesystem::path SidecarPath, ZenCacheDiskLayer::CacheBucket::IndexMap& Index, std::vector& AccessTimes, @@ -309,7 +312,8 @@ private: }; void -BucketManifestSerializer::ParseManifest(ZenCacheDiskLayer::CacheBucket& Bucket, +BucketManifestSerializer::ParseManifest(RwLock::ExclusiveLockScope& BucketLock, + ZenCacheDiskLayer::CacheBucket& Bucket, std::filesystem::path ManifestPath, ZenCacheDiskLayer::CacheBucket::IndexMap& Index, std::vector& AccessTimes, @@ -317,7 +321,7 @@ BucketManifestSerializer::ParseManifest(ZenCacheDiskLayer::CacheBucket& B { if (Manifest["UsingMetaFile"sv].AsBool()) { - ReadSidecarFile(Bucket, GetMetaPath(Bucket.m_BucketDir, Bucket.m_BucketName), Index, AccessTimes, Payloads); + ReadSidecarFile(BucketLock, Bucket, GetMetaPath(Bucket.m_BucketDir, Bucket.m_BucketName), Index, AccessTimes, Payloads); return; } @@ -373,7 +377,7 @@ BucketManifestSerializer::ParseManifest(ZenCacheDiskLayer::CacheBucket& B if (RawSize != 0 || RawHash != IoHash::Zero) { BucketPayload& Payload = Payloads[KeyIndex]; - Bucket.SetMetaData(Payload, BucketMetaData{.RawSize = RawSize, .RawHash = RawHash}); + Bucket.SetMetaData(BucketLock, Payload, BucketMetaData{.RawSize = RawSize, .RawHash = RawHash}); } } @@ -496,7 +500,8 @@ BucketManifestSerializer::MakeSidecarManifest(const Oid& BucketId, uint64_t Entr } bool -BucketManifestSerializer::ReadSidecarFile(ZenCacheDiskLayer::CacheBucket& Bucket, +BucketManifestSerializer::ReadSidecarFile(RwLock::ExclusiveLockScope& BucketLock, + ZenCacheDiskLayer::CacheBucket& Bucket, std::filesystem::path SidecarPath, ZenCacheDiskLayer::CacheBucket::IndexMap& Index, std::vector& AccessTimes, @@ -567,7 +572,7 @@ BucketManifestSerializer::ReadSidecarFile(ZenCacheDiskLayer::CacheBucket& if (Entry->RawSize && Entry->RawHash != IoHash::Zero) { - Bucket.SetMetaData(PayloadEntry, BucketMetaData{.RawSize = Entry->RawSize, .RawHash = Entry->RawHash}); + Bucket.SetMetaData(BucketLock, PayloadEntry, BucketMetaData{.RawSize = Entry->RawSize, .RawHash = Entry->RawHash}); } } @@ -580,7 +585,8 @@ BucketManifestSerializer::ReadSidecarFile(ZenCacheDiskLayer::CacheBucket& } void -BucketManifestSerializer::WriteSidecarFile(const std::filesystem::path& SidecarPath, +BucketManifestSerializer::WriteSidecarFile(RwLock::SharedLockScope&, + const std::filesystem::path& SidecarPath, uint64_t SnapshotLogPosition, const ZenCacheDiskLayer::CacheBucket::IndexMap& Index, const std::vector& AccessTimes, @@ -696,6 +702,10 @@ ZenCacheDiskLayer::CacheBucket::OpenOrCreate(std::filesystem::path BucketDir, bo using namespace std::literals; ZEN_TRACE_CPU("Z$::Disk::Bucket::OpenOrCreate"); + ZEN_ASSERT(m_IsFlushing.load()); + + // We want to take the lock here since we register as a GC referencer a construction + RwLock::ExclusiveLockScope IndexLock(m_IndexLock); ZEN_LOG_SCOPE("opening cache bucket '{}'", BucketDir); @@ -738,20 +748,25 @@ ZenCacheDiskLayer::CacheBucket::OpenOrCreate(std::filesystem::path BucketDir, bo return false; } - InitializeIndexFromDisk(IsNew); + InitializeIndexFromDisk(IndexLock, IsNew); + + auto _ = MakeGuard([&]() { + // We are now initialized, allow flushing when we exit + m_IsFlushing.store(false); + }); if (IsNew) { return true; } - ManifestReader.ParseManifest(*this, ManifestPath, m_Index, m_AccessTimes, m_Payloads); + ManifestReader.ParseManifest(IndexLock, *this, ManifestPath, m_Index, m_AccessTimes, m_Payloads); return true; } void -ZenCacheDiskLayer::CacheBucket::WriteIndexSnapshot(const std::function& ClaimDiskReserveFunc) +ZenCacheDiskLayer::CacheBucket::WriteIndexSnapshotLocked(const std::function& ClaimDiskReserveFunc) { ZEN_TRACE_CPU("Z$::Disk::Bucket::WriteIndexSnapshot"); @@ -861,7 +876,7 @@ ZenCacheDiskLayer::CacheBucket::WriteIndexSnapshot(const std::function::IsValid(LogPath)) { - LogEntryCount = ReadLog(LogPath, m_LogFlushPosition); + LogEntryCount = ReadLog(IndexLock, LogPath, m_LogFlushPosition); } else if (fs::is_regular_file(LogPath)) { @@ -1100,7 +1115,7 @@ ZenCacheDiskLayer::CacheBucket::InitializeIndexFromDisk(const bool IsNew) if (IsNew || LogEntryCount > 0) { - WriteIndexSnapshot(); + WriteIndexSnapshot(IndexLock); } } @@ -1218,14 +1233,14 @@ ZenCacheDiskLayer::CacheBucket::Get(const IoHash& HashKey, ZenCacheValue& OutVal { ZEN_TRACE_CPU("Z$::Disk::Bucket::Get::MemCache"); OutValue.Value = IoBufferBuilder::ReadFromFileMaybe(OutValue.Value); - RwLock::ExclusiveLockScope _(m_IndexLock); + RwLock::ExclusiveLockScope UpdateIndexLock(m_IndexLock); if (auto UpdateIt = m_Index.find(HashKey); UpdateIt != m_Index.end()) { - BucketPayload& WritePayload = m_Payloads[EntryIndex]; + BucketPayload& WritePayload = m_Payloads[UpdateIt->second]; // Only update if it has not already been updated by other thread if (!WritePayload.MemCached) { - SetMemCachedData(WritePayload, OutValue.Value); + SetMemCachedData(UpdateIndexLock, WritePayload, OutValue.Value); } } } @@ -1250,7 +1265,7 @@ ZenCacheDiskLayer::CacheBucket::Get(const IoHash& HashKey, ZenCacheValue& OutVal OutValue.RawHash = IoHash::HashBuffer(OutValue.Value); OutValue.RawSize = OutValue.Value.GetSize(); } - RwLock::ExclusiveLockScope __(m_IndexLock); + RwLock::ExclusiveLockScope UpdateIndexLock(m_IndexLock); if (auto WriteIt = m_Index.find(HashKey); WriteIt != m_Index.end()) { BucketPayload& WritePayload = m_Payloads[WriteIt.value()]; @@ -1258,7 +1273,7 @@ ZenCacheDiskLayer::CacheBucket::Get(const IoHash& HashKey, ZenCacheValue& OutVal // Only set if no other path has already updated the meta data if (!WritePayload.MetaData) { - SetMetaData(WritePayload, {.RawSize = OutValue.RawSize, .RawHash = OutValue.RawHash}); + SetMetaData(UpdateIndexLock, WritePayload, {.RawSize = OutValue.RawSize, .RawHash = OutValue.RawHash}); } } } @@ -1297,7 +1312,7 @@ ZenCacheDiskLayer::CacheBucket::MemCacheTrim(GcClock::TimePoint ExpireTime) { GcClock::Tick ExpireTicks = ExpireTime.time_since_epoch().count(); - RwLock::ExclusiveLockScope _(m_IndexLock); + RwLock::ExclusiveLockScope IndexLock(m_IndexLock); if (m_MemCachedPayloads.empty()) { return; @@ -1312,7 +1327,7 @@ ZenCacheDiskLayer::CacheBucket::MemCacheTrim(GcClock::TimePoint ExpireTime) } if (m_AccessTimes[Index] < ExpireTicks) { - RemoveMemCachedData(Payload); + RemoveMemCachedData(IndexLock, Payload); } } m_MemCachedPayloads.shrink_to_fit(); @@ -1434,7 +1449,7 @@ ZenCacheDiskLayer::CacheBucket::SaveSnapshot(const std::function& Cl { RwLock::SharedLockScope IndexLock(m_IndexLock); - WriteIndexSnapshot(); + WriteIndexSnapshot(IndexLock); // Note: this copy could be eliminated on shutdown to // reduce memory usage and execution time Index = m_Index; @@ -1474,7 +1489,7 @@ ZenCacheDiskLayer::CacheBucket::SaveSnapshot(const std::function& Cl else { RwLock::SharedLockScope IndexLock(m_IndexLock); - WriteIndexSnapshot(); + WriteIndexSnapshot(IndexLock); const uint64_t EntryCount = m_Index.size(); Buffer = ManifestWriter.MakeSidecarManifest(m_BucketId, EntryCount); uint64_t SidecarSize = ManifestWriter.GetSidecarSize(); @@ -1502,7 +1517,8 @@ ZenCacheDiskLayer::CacheBucket::SaveSnapshot(const std::function& Cl return; } - ManifestWriter.WriteSidecarFile(GetMetaPath(m_BucketDir, m_BucketName), + ManifestWriter.WriteSidecarFile(IndexLock, + GetMetaPath(m_BucketDir, m_BucketName), m_LogFlushPosition, m_Index, m_AccessTimes, @@ -1776,8 +1792,8 @@ ZenCacheDiskLayer::CacheBucket::ScrubStorage(ScrubContext& Ctx) m_StandaloneSize.fetch_sub(Location.Size(), std::memory_order::relaxed); } - RemoveMemCachedData(Payload); - RemoveMetaData(Payload); + RemoveMemCachedData(IndexLock, Payload); + RemoveMetaData(IndexLock, Payload); Location.Flags |= DiskLocation::kTombStone; LogEntries.push_back(DiskIndexEntry{.Key = BadKey, .Location = Location}); @@ -1813,7 +1829,7 @@ ZenCacheDiskLayer::CacheBucket::ScrubStorage(ScrubContext& Ctx) { RwLock::ExclusiveLockScope IndexLock(m_IndexLock); - CompactState(Payloads, AccessTimes, MetaDatas, MemCachedPayloads, FirstReferenceIndex, Index, IndexLock); + CompactState(IndexLock, Payloads, AccessTimes, MetaDatas, MemCachedPayloads, FirstReferenceIndex, Index, IndexLock); } } } @@ -1875,6 +1891,10 @@ ZenCacheDiskLayer::CacheBucket::GatherReferences(GcContext& GcCtx) WriteBlockLongestTimeUs = std::max(ElapsedUs, WriteBlockLongestTimeUs); }); #endif // CALCULATE_BLOCKING_TIME + if (m_Index.empty()) + { + return; + } Index = m_Index; AccessTimes = m_AccessTimes; Payloads = m_Payloads; @@ -2097,8 +2117,6 @@ ZenCacheDiskLayer::CacheBucket::CollectGarbage(GcContext& GcCtx) } }); - m_SlogFile.Flush(); - auto __ = MakeGuard([&]() { if (!DeletedChunks.empty()) { @@ -2117,13 +2135,20 @@ ZenCacheDiskLayer::CacheBucket::CollectGarbage(GcContext& GcCtx) WriteBlockTimeUs += ElapsedUs; WriteBlockLongestTimeUs = std::max(ElapsedUs, WriteBlockLongestTimeUs); }); - CompactState(Payloads, AccessTimes, MetaDatas, MemCachedPayloads, FirstReferenceIndex, Index, IndexLock); + CompactState(IndexLock, Payloads, AccessTimes, MetaDatas, MemCachedPayloads, FirstReferenceIndex, Index, IndexLock); } GcCtx.AddDeletedCids(std::vector(DeletedChunks.begin(), DeletedChunks.end())); } }); - std::span ExpiredCacheKeySpan = GcCtx.ExpiredCacheKeys(m_BucketDir.string()); + std::span ExpiredCacheKeySpan = GcCtx.ExpiredCacheKeys(m_BucketDir.string()); + if (ExpiredCacheKeySpan.empty()) + { + return; + } + + m_SlogFile.Flush(); + std::unordered_set ExpiredCacheKeys(ExpiredCacheKeySpan.begin(), ExpiredCacheKeySpan.end()); std::vector ExpiredStandaloneEntries; @@ -2291,7 +2316,7 @@ ZenCacheDiskLayer::CacheBucket::CollectGarbage(GcContext& GcCtx) std::vector LogEntries; LogEntries.reserve(MovedChunks.size() + RemovedChunks.size()); { - RwLock::ExclusiveLockScope __(m_IndexLock); + RwLock::ExclusiveLockScope IndexLock(m_IndexLock); Stopwatch Timer; const auto ____ = MakeGuard([&] { uint64_t ElapsedUs = Timer.GetElapsedTimeUs(); @@ -2329,8 +2354,8 @@ ZenCacheDiskLayer::CacheBucket::CollectGarbage(GcContext& GcCtx) m_Configuration.PayloadAlignment, OldDiskLocation.GetFlags() | DiskLocation::kTombStone)}); - RemoveMemCachedData(Payload); - RemoveMetaData(Payload); + RemoveMemCachedData(IndexLock, Payload); + RemoveMetaData(IndexLock, Payload); m_Index.erase(ChunkHash); DeletedChunks.insert(ChunkHash); @@ -2367,7 +2392,7 @@ ZenCacheDiskLayer::CacheBucket::EntryCount() const } CacheValueDetails::ValueDetails -ZenCacheDiskLayer::CacheBucket::GetValueDetails(const IoHash& Key, PayloadIndex Index) const +ZenCacheDiskLayer::CacheBucket::GetValueDetails(RwLock::SharedLockScope& IndexLock, const IoHash& Key, PayloadIndex Index) const { std::vector Attachments; const BucketPayload& Payload = m_Payloads[Index]; @@ -2379,7 +2404,7 @@ ZenCacheDiskLayer::CacheBucket::GetValueDetails(const IoHash& Key, PayloadIndex CbObjectView Obj(Value.GetData()); Obj.IterateAttachments([&Attachments](CbFieldView Field) { Attachments.emplace_back(Field.AsAttachment()); }); } - BucketMetaData MetaData = GetMetaData(Payload); + BucketMetaData MetaData = GetMetaData(IndexLock, Payload); return CacheValueDetails::ValueDetails{.Size = Payload.Location.Size(), .RawSize = MetaData.RawSize, .RawHash = MetaData.RawHash, @@ -2389,7 +2414,7 @@ ZenCacheDiskLayer::CacheBucket::GetValueDetails(const IoHash& Key, PayloadIndex } CacheValueDetails::BucketDetails -ZenCacheDiskLayer::CacheBucket::GetValueDetails(const std::string_view ValueFilter) const +ZenCacheDiskLayer::CacheBucket::GetValueDetails(RwLock::SharedLockScope& IndexLock, const std::string_view ValueFilter) const { CacheValueDetails::BucketDetails Details; RwLock::SharedLockScope _(m_IndexLock); @@ -2398,7 +2423,7 @@ ZenCacheDiskLayer::CacheBucket::GetValueDetails(const std::string_view ValueFilt Details.Values.reserve(m_Index.size()); for (const auto& It : m_Index) { - Details.Values.insert_or_assign(It.first, GetValueDetails(It.first, It.second)); + Details.Values.insert_or_assign(It.first, GetValueDetails(IndexLock, It.first, It.second)); } } else @@ -2406,7 +2431,7 @@ ZenCacheDiskLayer::CacheBucket::GetValueDetails(const std::string_view ValueFilt IoHash Key = IoHash::FromHexString(ValueFilter); if (auto It = m_Index.find(Key); It != m_Index.end()) { - Details.Values.insert_or_assign(It->first, GetValueDetails(It->first, It->second)); + Details.Values.insert_or_assign(It->first, GetValueDetails(IndexLock, It->first, It->second)); } } return Details; @@ -2416,10 +2441,10 @@ void ZenCacheDiskLayer::CacheBucket::EnumerateBucketContents( std::function& Fn) const { - RwLock::SharedLockScope _(m_IndexLock); + RwLock::SharedLockScope IndexLock(m_IndexLock); for (const auto& It : m_Index) { - CacheValueDetails::ValueDetails Vd = GetValueDetails(It.first, It.second); + CacheValueDetails::ValueDetails Vd = GetValueDetails(IndexLock, It.first, It.second); Fn(It.first, Vd); } @@ -2594,16 +2619,16 @@ ZenCacheDiskLayer::CacheBucket::PutStandaloneCacheValue(const IoHash& HashKey, c SetReferences(IndexLock, m_FirstReferenceIndex[EntryIndex], References); } m_AccessTimes[EntryIndex] = GcClock::TickCount(); - RemoveMemCachedData(Payload); + RemoveMemCachedData(IndexLock, Payload); m_StandaloneSize.fetch_sub(OldSize, std::memory_order::relaxed); } if (Value.RawSize != 0 || Value.RawHash != IoHash::Zero) { - SetMetaData(m_Payloads[EntryIndex], {.RawSize = Value.RawSize, .RawHash = Value.RawHash}); + SetMetaData(IndexLock, m_Payloads[EntryIndex], {.RawSize = Value.RawSize, .RawHash = Value.RawHash}); } else { - RemoveMetaData(m_Payloads[EntryIndex]); + RemoveMetaData(IndexLock, m_Payloads[EntryIndex]); } m_SlogFile.Append({.Key = HashKey, .Location = Loc}); @@ -2611,7 +2636,9 @@ ZenCacheDiskLayer::CacheBucket::PutStandaloneCacheValue(const IoHash& HashKey, c } void -ZenCacheDiskLayer::CacheBucket::SetMetaData(BucketPayload& Payload, const ZenCacheDiskLayer::CacheBucket::BucketMetaData& MetaData) +ZenCacheDiskLayer::CacheBucket::SetMetaData(RwLock::ExclusiveLockScope&, + BucketPayload& Payload, + const ZenCacheDiskLayer::CacheBucket::BucketMetaData& MetaData) { if (Payload.MetaData) { @@ -2634,7 +2661,7 @@ ZenCacheDiskLayer::CacheBucket::SetMetaData(BucketPayload& Payload, const ZenCac } void -ZenCacheDiskLayer::CacheBucket::RemoveMetaData(BucketPayload& Payload) +ZenCacheDiskLayer::CacheBucket::RemoveMetaData(RwLock::ExclusiveLockScope&, BucketPayload& Payload) { if (Payload.MetaData) { @@ -2644,7 +2671,7 @@ ZenCacheDiskLayer::CacheBucket::RemoveMetaData(BucketPayload& Payload) } void -ZenCacheDiskLayer::CacheBucket::SetMemCachedData(BucketPayload& Payload, IoBuffer& MemCachedData) +ZenCacheDiskLayer::CacheBucket::SetMemCachedData(RwLock::ExclusiveLockScope&, BucketPayload& Payload, IoBuffer& MemCachedData) { uint64_t PayloadSize = MemCachedData.GetSize(); ZEN_ASSERT(PayloadSize != 0); @@ -2669,7 +2696,7 @@ ZenCacheDiskLayer::CacheBucket::SetMemCachedData(BucketPayload& Payload, IoBuffe } size_t -ZenCacheDiskLayer::CacheBucket::RemoveMemCachedData(BucketPayload& Payload) +ZenCacheDiskLayer::CacheBucket::RemoveMemCachedData(RwLock::ExclusiveLockScope&, BucketPayload& Payload) { if (Payload.MemCached) { @@ -2684,7 +2711,7 @@ ZenCacheDiskLayer::CacheBucket::RemoveMemCachedData(BucketPayload& Payload) } ZenCacheDiskLayer::CacheBucket::BucketMetaData -ZenCacheDiskLayer::CacheBucket::GetMetaData(const BucketPayload& Payload) const +ZenCacheDiskLayer::CacheBucket::GetMetaData(RwLock::SharedLockScope&, const BucketPayload& Payload) const { if (Payload.MetaData) { @@ -2727,8 +2754,8 @@ ZenCacheDiskLayer::CacheBucket::PutInlineCacheValue(const IoHash& HashKey, const ZEN_ASSERT_SLOW(EntryIndex < PayloadIndex(m_AccessTimes.size())); BucketPayload& Payload = m_Payloads[EntryIndex]; - RemoveMemCachedData(Payload); - RemoveMetaData(Payload); + RemoveMemCachedData(IndexLock, Payload); + RemoveMetaData(IndexLock, Payload); Payload = (BucketPayload{.Location = Location}); m_AccessTimes[EntryIndex] = GcClock::TickCount(); @@ -3026,6 +3053,10 @@ ZenCacheDiskLayer::CacheBucket::RemoveExpiredData(GcCtx& Ctx, GcStats& Stats) { return nullptr; } + if (m_Index.empty()) + { + return nullptr; + } TotalEntries = m_Index.size(); @@ -3071,8 +3102,8 @@ ZenCacheDiskLayer::CacheBucket::RemoveExpiredData(GcCtx& Ctx, GcStats& Stats) auto It = m_Index.find(Entry.Key); ZEN_ASSERT(It != m_Index.end()); BucketPayload& Payload = m_Payloads[It->second]; - RemoveMetaData(Payload); - Stats.FreedMemory += RemoveMemCachedData(Payload); + RemoveMetaData(IndexLock, Payload); + Stats.FreedMemory += RemoveMemCachedData(IndexLock, Payload); m_Index.erase(It); Stats.DeletedCount++; } @@ -3091,7 +3122,7 @@ ZenCacheDiskLayer::CacheBucket::RemoveExpiredData(GcCtx& Ctx, GcStats& Stats) IndexMap Index; { RwLock::ExclusiveLockScope IndexLock(m_IndexLock); - CompactState(Payloads, AccessTimes, MetaDatas, MemCachedPayloads, FirstReferenceIndex, Index, IndexLock); + CompactState(IndexLock, Payloads, AccessTimes, MetaDatas, MemCachedPayloads, FirstReferenceIndex, Index, IndexLock); } } @@ -3485,6 +3516,14 @@ ZenCacheDiskLayer::CacheBucket::CreateReferenceCheckers(GcCtx& Ctx) ZEN_INFO("GCV2: cachebucket [CREATE CHECKERS] '{}': completed in {}", m_BucketDir, NiceTimeSpanMs(Timer.GetElapsedTimeMs())); }); + { + RwLock::SharedLockScope __(m_IndexLock); + if (m_Index.empty()) + { + return {}; + } + } + return {new DiskBucketReferenceChecker(*this)}; } @@ -3665,7 +3704,8 @@ ZenCacheDiskLayer::CacheBucket::ClearReferenceCache() } void -ZenCacheDiskLayer::CacheBucket::CompactState(std::vector& Payloads, +ZenCacheDiskLayer::CacheBucket::CompactState(RwLock::ExclusiveLockScope&, + std::vector& Payloads, std::vector& AccessTimes, std::vector& MetaDatas, std::vector& MemCachedPayloads, @@ -3748,124 +3788,99 @@ ZenCacheDiskLayer::ZenCacheDiskLayer(GcManager& Gc, JobQueue& JobQueue, const st ZenCacheDiskLayer::~ZenCacheDiskLayer() { -} - -bool -ZenCacheDiskLayer::Get(std::string_view InBucket, const IoHash& HashKey, ZenCacheValue& OutValue) -{ - ZEN_TRACE_CPU("Z$::Disk::Get"); - - const auto BucketName = std::string(InBucket); - CacheBucket* Bucket = nullptr; - + try { - RwLock::SharedLockScope _(m_Lock); - - auto It = m_Buckets.find(BucketName); - - if (It != m_Buckets.end()) { - Bucket = It->second.get(); + RwLock::ExclusiveLockScope _(m_Lock); + for (auto& It : m_Buckets) + { + m_DroppedBuckets.emplace_back(std::move(It.second)); + } + m_Buckets.clear(); } + // We destroy the buckets without holding a lock since destructor calls GcManager::RemoveGcReferencer which takes an exclusive lock. + // This can cause a deadlock, if GC is running we would block while holding ZenCacheDiskLayer::m_Lock + m_DroppedBuckets.clear(); } - - if (Bucket == nullptr) + catch (std::exception& Ex) { - // Bucket needs to be opened/created + ZEN_ERROR("~ZenCacheDiskLayer() failed. Reason: '{}'", Ex.what()); + } +} - RwLock::ExclusiveLockScope _(m_Lock); +ZenCacheDiskLayer::CacheBucket* +ZenCacheDiskLayer::GetOrCreateBucket(std::string_view InBucket) +{ + ZEN_TRACE_CPU("Z$::Disk::GetOrCreateBucket"); + const auto BucketName = std::string(InBucket); + { + RwLock::SharedLockScope SharedLock(m_Lock); if (auto It = m_Buckets.find(BucketName); It != m_Buckets.end()) { - Bucket = It->second.get(); - } - else - { - auto InsertResult = - m_Buckets.emplace(BucketName, - std::make_unique(m_Gc, m_TotalMemCachedSize, BucketName, m_Configuration.BucketConfig)); - Bucket = InsertResult.first->second.get(); - - std::filesystem::path BucketPath = m_RootDir; - BucketPath /= BucketName; - - if (!Bucket->OpenOrCreate(BucketPath)) - { - m_Buckets.erase(InsertResult.first); - return false; - } + return It->second.get(); } } - ZEN_ASSERT(Bucket != nullptr); - if (Bucket->Get(HashKey, OutValue)) + // We create the bucket without holding a lock since contructor calls GcManager::AddGcReferencer which takes an exclusive lock. + // This can cause a deadlock, if GC is running we would block while holding ZenCacheDiskLayer::m_Lock + std::unique_ptr Bucket( + std::make_unique(m_Gc, m_TotalMemCachedSize, BucketName, m_Configuration.BucketConfig)); + + RwLock::ExclusiveLockScope Lock(m_Lock); + if (auto It = m_Buckets.find(BucketName); It != m_Buckets.end()) { - TryMemCacheTrim(); - return true; + return It->second.get(); } - return false; -} - -void -ZenCacheDiskLayer::Put(std::string_view InBucket, const IoHash& HashKey, const ZenCacheValue& Value, std::span References) -{ - ZEN_TRACE_CPU("Z$::Disk::Put"); - - const auto BucketName = std::string(InBucket); - CacheBucket* Bucket = nullptr; + std::filesystem::path BucketPath = m_RootDir; + BucketPath /= BucketName; + try { - RwLock::SharedLockScope _(m_Lock); - - auto It = m_Buckets.find(BucketName); - - if (It != m_Buckets.end()) + if (!Bucket->OpenOrCreate(BucketPath)) { - Bucket = It->second.get(); + ZEN_WARN("Found directory '{}' in our base directory '{}' but it is not a valid bucket", BucketName, m_RootDir); + return nullptr; } } - - if (Bucket == nullptr) + catch (const std::exception& Err) { - // New bucket needs to be created + ZEN_WARN("Creating bucket '{}' in '{}' FAILED, reason: '{}'", BucketName, BucketPath, Err.what()); + throw; + } - RwLock::ExclusiveLockScope _(m_Lock); + CacheBucket* Result = Bucket.get(); + m_Buckets.emplace(BucketName, std::move(Bucket)); - if (auto It = m_Buckets.find(BucketName); It != m_Buckets.end()) - { - Bucket = It->second.get(); - } - else - { - auto InsertResult = - m_Buckets.emplace(BucketName, - std::make_unique(m_Gc, m_TotalMemCachedSize, BucketName, m_Configuration.BucketConfig)); - Bucket = InsertResult.first->second.get(); + return Result; +} - std::filesystem::path BucketPath = m_RootDir; - BucketPath /= BucketName; +bool +ZenCacheDiskLayer::Get(std::string_view InBucket, const IoHash& HashKey, ZenCacheValue& OutValue) +{ + ZEN_TRACE_CPU("Z$::Disk::Get"); - try - { - if (!Bucket->OpenOrCreate(BucketPath)) - { - ZEN_WARN("Found directory '{}' in our base directory '{}' but it is not a valid bucket", BucketName, m_RootDir); - m_Buckets.erase(InsertResult.first); - return; - } - } - catch (const std::exception& Err) - { - ZEN_WARN("creating bucket '{}' in '{}' FAILED, reason: '{}'", BucketName, BucketPath, Err.what()); - throw; - } + if (CacheBucket* Bucket = GetOrCreateBucket(InBucket); Bucket != nullptr) + { + if (Bucket->Get(HashKey, OutValue)) + { + TryMemCacheTrim(); + return true; } } + return false; +} - ZEN_ASSERT(Bucket != nullptr); +void +ZenCacheDiskLayer::Put(std::string_view InBucket, const IoHash& HashKey, const ZenCacheValue& Value, std::span References) +{ + ZEN_TRACE_CPU("Z$::Disk::Put"); - Bucket->Put(HashKey, Value, References); - TryMemCacheTrim(); + if (CacheBucket* Bucket = GetOrCreateBucket(InBucket); Bucket != nullptr) + { + Bucket->Put(HashKey, Value, References); + TryMemCacheTrim(); + } } void @@ -4026,10 +4041,6 @@ ZenCacheDiskLayer::Flush() { RwLock::SharedLockScope __(m_Lock); - if (m_Buckets.empty()) - { - return; - } Buckets.reserve(m_Buckets.size()); for (auto& Kv : m_Buckets) { @@ -4060,7 +4071,6 @@ void ZenCacheDiskLayer::ScrubStorage(ScrubContext& Ctx) { RwLock::SharedLockScope _(m_Lock); - { std::vector> Results; Results.reserve(m_Buckets.size()); @@ -4181,20 +4191,22 @@ ZenCacheDiskLayer::EnumerateBucketContents(std::string_view CacheValueDetails::NamespaceDetails ZenCacheDiskLayer::GetValueDetails(const std::string_view BucketFilter, const std::string_view ValueFilter) const { - RwLock::SharedLockScope _(m_Lock); CacheValueDetails::NamespaceDetails Details; - if (BucketFilter.empty()) { - Details.Buckets.reserve(BucketFilter.empty() ? m_Buckets.size() : 1); - for (auto& Kv : m_Buckets) + RwLock::SharedLockScope IndexLock(m_Lock); + if (BucketFilter.empty()) + { + Details.Buckets.reserve(BucketFilter.empty() ? m_Buckets.size() : 1); + for (auto& Kv : m_Buckets) + { + Details.Buckets[Kv.first] = Kv.second->GetValueDetails(IndexLock, ValueFilter); + } + } + else if (auto It = m_Buckets.find(std::string(BucketFilter)); It != m_Buckets.end()) { - Details.Buckets[Kv.first] = Kv.second->GetValueDetails(ValueFilter); + Details.Buckets[It->first] = It->second->GetValueDetails(IndexLock, ValueFilter); } } - else if (auto It = m_Buckets.find(std::string(BucketFilter)); It != m_Buckets.end()) - { - Details.Buckets[It->first] = It->second->GetValueDetails(ValueFilter); - } return Details; } -- cgit v1.2.3