From 898a48b5121ed09879e34dc3315387d927a91ebf Mon Sep 17 00:00:00 2001 From: Dan Engelbrecht Date: Tue, 24 May 2022 13:24:51 +0200 Subject: Make sure to hold exclusive lock over index and all shard locks. Clear index on drop. --- zenserver/cache/structuredcachestore.cpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) (limited to 'zenserver/cache/structuredcachestore.cpp') diff --git a/zenserver/cache/structuredcachestore.cpp b/zenserver/cache/structuredcachestore.cpp index 3189a14cc..1db99280e 100644 --- a/zenserver/cache/structuredcachestore.cpp +++ b/zenserver/cache/structuredcachestore.cpp @@ -1199,8 +1199,16 @@ ZenCacheDiskLayer::CacheBucket::Put(const IoHash& HashKey, const ZenCacheValue& void ZenCacheDiskLayer::CacheBucket::Drop() { + RwLock::ExclusiveLockScope _(m_IndexLock); + std::vector> ShardLocks; + ShardLocks.reserve(256); + for (RwLock& Lock : m_ShardedLocks) + { + ShardLocks.push_back(std::make_unique(Lock)); + } m_BlockStore.Close(); m_SlogFile.Close(); + m_Index.clear(); DeleteDirectories(m_BucketDir); } @@ -2274,7 +2282,7 @@ ZenCacheStore::GetNamespace(std::string_view Namespace) void ZenCacheStore::IterateNamespaces(const std::function& Callback) const { - std::vector > Namespaces; + std::vector> Namespaces; { RwLock::SharedLockScope _(m_NamespacesLock); Namespaces.reserve(m_Namespaces.size()); -- cgit v1.2.3 From ba135d4a90746ed905a825f48fc3ccfe0641301e Mon Sep 17 00:00:00 2001 From: Dan Engelbrecht Date: Tue, 24 May 2022 13:52:45 +0200 Subject: Use rename/delete and keep pointer for dropped buckets --- zenserver/cache/structuredcachestore.cpp | 157 ++++++++++++++++++++----------- 1 file changed, 104 insertions(+), 53 deletions(-) (limited to 'zenserver/cache/structuredcachestore.cpp') diff --git a/zenserver/cache/structuredcachestore.cpp b/zenserver/cache/structuredcachestore.cpp index 1db99280e..be9e408b4 100644 --- a/zenserver/cache/structuredcachestore.cpp +++ b/zenserver/cache/structuredcachestore.cpp @@ -580,19 +580,6 @@ ZenCacheDiskLayer::CacheBucket::~CacheBucket() { } -bool -ZenCacheDiskLayer::CacheBucket::Delete(std::filesystem::path BucketDir) -{ - if (std::filesystem::exists(BucketDir)) - { - DeleteDirectories(BucketDir); - - return true; - } - - return false; -} - void ZenCacheDiskLayer::CacheBucket::OpenOrCreate(std::filesystem::path BucketDir, bool AllowCreate) { @@ -1158,11 +1145,6 @@ ZenCacheDiskLayer::CacheBucket::GetStandaloneCacheValue(const DiskLocation& Loc, bool ZenCacheDiskLayer::CacheBucket::Get(const IoHash& HashKey, ZenCacheValue& OutValue) { - if (!m_IsOk) - { - return false; - } - RwLock::SharedLockScope _(m_IndexLock); auto It = m_Index.find(HashKey); if (It == m_Index.end()) @@ -1184,11 +1166,6 @@ ZenCacheDiskLayer::CacheBucket::Get(const IoHash& HashKey, ZenCacheValue& OutVal void ZenCacheDiskLayer::CacheBucket::Put(const IoHash& HashKey, const ZenCacheValue& Value) { - if (!m_IsOk) - { - return; - } - if (Value.Value.Size() >= m_LargeObjectThreshold) { return PutStandaloneCacheValue(HashKey, Value); @@ -1196,10 +1173,43 @@ ZenCacheDiskLayer::CacheBucket::Put(const IoHash& HashKey, const ZenCacheValue& PutInlineCacheValue(HashKey, Value); } +static bool +DeleteBucketFromDisk(const std::filesystem::path& BucketDir, std::string_view BucketName) +{ + int dropIndex = 0; + do + { + if (!std::filesystem::exists(BucketDir)) + { + return false; + } + + std::string DroppedBucketName = fmt::format("[dropped]{}({})", BucketName, dropIndex); + std::filesystem::path DroppedBucketPath = BucketDir.parent_path() / DroppedBucketName; + if (std::filesystem::exists(DroppedBucketPath)) + { + dropIndex++; + continue; + } + + std::error_code Ec; + std::filesystem::rename(BucketDir, DroppedBucketPath, Ec); + if (!Ec) + { + DeleteDirectories(DroppedBucketPath); + return true; + } + // TODO: Do we need to bail at some point? + zen::Sleep(100); + } while (true); +} + void ZenCacheDiskLayer::CacheBucket::Drop() { - RwLock::ExclusiveLockScope _(m_IndexLock); + RwLock::ExclusiveLockScope _(m_IndexLock); + m_IsOk = false; + std::vector> ShardLocks; ShardLocks.reserve(256); for (RwLock& Lock : m_ShardedLocks) @@ -1208,8 +1218,10 @@ ZenCacheDiskLayer::CacheBucket::Drop() } m_BlockStore.Close(); m_SlogFile.Close(); + + DeleteBucketFromDisk(m_BucketDir, m_BucketName); + m_Index.clear(); - DeleteDirectories(m_BucketDir); } void @@ -1711,7 +1723,12 @@ ZenCacheDiskLayer::CollectGarbage(GcContext& GcCtx) for (auto& Kv : m_Buckets) { - Kv.second.CollectGarbage(GcCtx); + CacheBucket& Bucket = *Kv.second; + if (!Bucket.IsOk()) + { + continue; + } + Bucket.CollectGarbage(GcCtx); } } @@ -1724,7 +1741,11 @@ ZenCacheDiskLayer::UpdateAccessTimes(const zen::access_tracking::AccessTimes& Ac { if (auto It = m_Buckets.find(Kv.first); It != m_Buckets.end()) { - CacheBucket& Bucket = It->second; + CacheBucket& Bucket = *It->second; + if (!Bucket.IsOk()) + { + continue; + } Bucket.UpdateAccessTimes(Kv.second); } } @@ -1937,7 +1958,11 @@ ZenCacheDiskLayer::Get(std::string_view InBucket, const IoHash& HashKey, ZenCach if (it != m_Buckets.end()) { - Bucket = &it->second; + Bucket = it->second.get(); + if (!Bucket->IsOk()) + { + return false; + } } } @@ -1949,22 +1974,25 @@ ZenCacheDiskLayer::Get(std::string_view InBucket, const IoHash& HashKey, ZenCach if (auto it = m_Buckets.find(BucketName); it != m_Buckets.end()) { - Bucket = &it->second; + Bucket = it->second.get(); } else { - auto It = m_Buckets.try_emplace(BucketName, BucketName); - Bucket = &It.first->second; + auto InsertResult = m_Buckets.emplace(BucketName, std::make_unique(BucketName)); + Bucket = InsertResult.first->second.get(); std::filesystem::path BucketPath = m_RootDir; BucketPath /= BucketName; Bucket->OpenOrCreate(BucketPath); + if (!Bucket->IsOk()) + { + return false; + } } } ZEN_ASSERT(Bucket != nullptr); - return Bucket->Get(HashKey, OutValue); } @@ -1981,7 +2009,11 @@ ZenCacheDiskLayer::Put(std::string_view InBucket, const IoHash& HashKey, const Z if (it != m_Buckets.end()) { - Bucket = &it->second; + Bucket = it->second.get(); + if (!Bucket->IsOk()) + { + return; + } } } @@ -1993,26 +2025,27 @@ ZenCacheDiskLayer::Put(std::string_view InBucket, const IoHash& HashKey, const Z if (auto it = m_Buckets.find(BucketName); it != m_Buckets.end()) { - Bucket = &it->second; + Bucket = it->second.get(); } else { - auto It = m_Buckets.try_emplace(BucketName, BucketName); - Bucket = &It.first->second; + auto InsertResult = m_Buckets.emplace(BucketName, std::make_unique(BucketName)); + Bucket = InsertResult.first->second.get(); std::filesystem::path BucketPath = m_RootDir; BucketPath /= BucketName; Bucket->OpenOrCreate(BucketPath); + if (!Bucket->IsOk()) + { + return; + } } } ZEN_ASSERT(Bucket != nullptr); - if (Bucket->IsOk()) - { - Bucket->Put(HashKey, Value); - } + Bucket->Put(HashKey, Value); } void @@ -2034,9 +2067,8 @@ ZenCacheDiskLayer::DiscoverBuckets() } else { - auto InsertResult = m_Buckets.try_emplace(BucketName, BucketName); - - CacheBucket& Bucket = InsertResult.first->second; + auto InsertResult = m_Buckets.emplace(BucketName, std::make_unique(BucketName)); + CacheBucket& Bucket = *InsertResult.first->second; Bucket.OpenOrCreate(BucketPath, /* AllowCreate */ false); @@ -2063,19 +2095,23 @@ ZenCacheDiskLayer::DropBucket(std::string_view InBucket) if (it != m_Buckets.end()) { - CacheBucket* Bucket = &it->second; - - Bucket->Drop(); - + CacheBucket& Bucket = *it->second; + m_DroppedBuckets.push_back(std::move(it->second)); m_Buckets.erase(it); + if (!Bucket.IsOk()) + { + return false; + } + + Bucket.Drop(); return true; } + // Make sure we remove the folder even if we don't know about the bucket std::filesystem::path BucketPath = m_RootDir; BucketPath /= std::string(InBucket); - - return CacheBucket::Delete(BucketPath); + return DeleteBucketFromDisk(BucketPath, InBucket); } void @@ -2088,7 +2124,12 @@ ZenCacheDiskLayer::Flush() Buckets.reserve(m_Buckets.size()); for (auto& Kv : m_Buckets) { - Buckets.push_back(&Kv.second); + CacheBucket* Bucket = Kv.second.get(); + if (!Bucket->IsOk()) + { + continue; + } + Buckets.push_back(Bucket); } } @@ -2105,7 +2146,12 @@ ZenCacheDiskLayer::Scrub(ScrubContext& Ctx) for (auto& Kv : m_Buckets) { - Kv.second.Scrub(Ctx); + CacheBucket& Bucket = *Kv.second; + if (!Bucket.IsOk()) + { + continue; + } + Bucket.Scrub(Ctx); } } @@ -2116,7 +2162,12 @@ ZenCacheDiskLayer::GatherReferences(GcContext& GcCtx) for (auto& Kv : m_Buckets) { - Kv.second.GatherReferences(GcCtx); + CacheBucket& Bucket = *Kv.second; + if (!Bucket.IsOk()) + { + continue; + } + Bucket.GatherReferences(GcCtx); } } @@ -2128,7 +2179,7 @@ ZenCacheDiskLayer::TotalSize() const for (auto& Kv : m_Buckets) { - TotalSize += Kv.second.TotalSize(); + TotalSize += Kv.second->TotalSize(); } return TotalSize; -- cgit v1.2.3 From 4d1faaa2076bff77734d152b1403c3c90884bc83 Mon Sep 17 00:00:00 2001 From: Dan Engelbrecht Date: Tue, 24 May 2022 23:22:52 +0200 Subject: drop bucket test --- zenserver/cache/structuredcachestore.cpp | 73 ++++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) (limited to 'zenserver/cache/structuredcachestore.cpp') diff --git a/zenserver/cache/structuredcachestore.cpp b/zenserver/cache/structuredcachestore.cpp index be9e408b4..f3f6503f3 100644 --- a/zenserver/cache/structuredcachestore.cpp +++ b/zenserver/cache/structuredcachestore.cpp @@ -3177,6 +3177,79 @@ TEST_CASE("z$.namespaces") } } +TEST_CASE("z$.drop.bucket") +{ + using namespace testutils; + + const auto CreateCacheValue = [](size_t Size) -> CbObject { + std::vector Buf; + Buf.resize(Size); + + CbObjectWriter Writer; + Writer.AddBinary("Binary"sv, Buf.data(), Buf.size()); + return Writer.Save(); + }; + + ScopedTemporaryDirectory TempDir; + CreateDirectories(TempDir.Path()); + + IoHash Key1; + IoHash Key2; + + auto PutValue = + [&CreateCacheValue](ZenCacheStore& Zcs, std::string_view Namespace, std::string_view Bucket, size_t KeyIndex, size_t Size) { + // Create a cache record + IoHash Key = CreateKey(KeyIndex); + CbObject CacheValue = CreateCacheValue(Size); + + IoBuffer Buffer = CacheValue.GetBuffer().AsIoBuffer(); + Buffer.SetContentType(ZenContentType::kCbObject); + + ZenCacheValue PutValue = {.Value = Buffer}; + Zcs.Put(Namespace, Bucket, Key, PutValue); + return Key; + }; + auto GetValue = [](ZenCacheStore& Zcs, std::string_view Namespace, std::string_view Bucket, const IoHash& Key) { + ZenCacheValue GetValue; + Zcs.Get(Namespace, Bucket, Key, GetValue); + return GetValue; + }; + WorkerThreadPool Workers(1); + { + CasGc Gc; + ZenCacheStore Zcs(Gc, {.BasePath = TempDir.Path() / "cache", .AllowAutomaticCreationOfNamespaces = true}); + const auto Bucket = "teardrinker"sv; + const auto Namespace = "mynamespace"sv; + + Key1 = PutValue(Zcs, Namespace, Bucket, 42, 4096); + Key2 = PutValue(Zcs, Namespace, Bucket, 43, 2048); + + ZenCacheValue Value1 = GetValue(Zcs, Namespace, Bucket, Key1); + CHECK(Value1.Value); + + std::atomic_bool WorkComplete = false; + Workers.ScheduleWork([&]() { + zen::Sleep(100); + Value1.Value = IoBuffer{}; + Value1 = GetValue(Zcs, Namespace, Bucket, Key1); + CHECK(!Value1.Value); + WorkComplete = true; + }); + // On Windows, DropBucket() will be blocked as long as we hold a reference to a buffer in the bucket + // Our DropBucket execution blocks any incoming request from completing until we are done with the drop + Zcs.DropBucket(Namespace, Bucket); + while (!WorkComplete) + { + zen::Sleep(1); + } + CHECK(!Value1.Value); + + // Entire bucket should be dropped, but doing a request should will re-create the namespace but it must still be empty + ZenCacheValue Value2 = GetValue(Zcs, Namespace, Bucket, Key2); + CHECK(!Value2.Value); + } +} + TEST_CASE("z$.blocked.disklayer.put") { ScopedTemporaryDirectory TempDir; -- cgit v1.2.3 From 49ad6da1f77daa8c91a087046d82ab78d2e41314 Mon Sep 17 00:00:00 2001 From: Dan Engelbrecht Date: Wed, 25 May 2022 12:34:51 +0200 Subject: If a bucket is in m_BucketMap it is OK, no need for separate flag --- zenserver/cache/structuredcachestore.cpp | 77 +++++++++----------------------- 1 file changed, 21 insertions(+), 56 deletions(-) (limited to 'zenserver/cache/structuredcachestore.cpp') diff --git a/zenserver/cache/structuredcachestore.cpp b/zenserver/cache/structuredcachestore.cpp index f3f6503f3..bb85f9824 100644 --- a/zenserver/cache/structuredcachestore.cpp +++ b/zenserver/cache/structuredcachestore.cpp @@ -572,7 +572,7 @@ ZenCacheMemoryLayer::CacheBucket::Put(const IoHash& HashKey, const ZenCacheValue ////////////////////////////////////////////////////////////////////////// -ZenCacheDiskLayer::CacheBucket::CacheBucket(std::string BucketName) : m_BucketName(std::move(BucketName)) +ZenCacheDiskLayer::CacheBucket::CacheBucket(std::string BucketName) : m_BucketName(std::move(BucketName)), m_BucketId(Oid::Zero) { } @@ -580,7 +580,7 @@ ZenCacheDiskLayer::CacheBucket::~CacheBucket() { } -void +bool ZenCacheDiskLayer::CacheBucket::OpenOrCreate(std::filesystem::path BucketDir, bool AllowCreate) { using namespace std::literals; @@ -598,7 +598,10 @@ ZenCacheDiskLayer::CacheBucket::OpenOrCreate(std::filesystem::path BucketDir, bo if (Manifest) { m_BucketId = Manifest["BucketId"].AsObjectId(); - m_IsOk = m_BucketId != Oid::Zero; + if (m_BucketId == Oid::Zero) + { + return false; + } } else if (AllowCreate) { @@ -612,7 +615,7 @@ ZenCacheDiskLayer::CacheBucket::OpenOrCreate(std::filesystem::path BucketDir, bo } else { - return; + return false; } OpenLog(BucketDir, IsNew); @@ -628,7 +631,7 @@ ZenCacheDiskLayer::CacheBucket::OpenOrCreate(std::filesystem::path BucketDir, bo } } - m_IsOk = true; + return true; } void @@ -1208,7 +1211,6 @@ void ZenCacheDiskLayer::CacheBucket::Drop() { RwLock::ExclusiveLockScope _(m_IndexLock); - m_IsOk = false; std::vector> ShardLocks; ShardLocks.reserve(256); @@ -1724,10 +1726,6 @@ ZenCacheDiskLayer::CollectGarbage(GcContext& GcCtx) for (auto& Kv : m_Buckets) { CacheBucket& Bucket = *Kv.second; - if (!Bucket.IsOk()) - { - continue; - } Bucket.CollectGarbage(GcCtx); } } @@ -1742,10 +1740,6 @@ ZenCacheDiskLayer::UpdateAccessTimes(const zen::access_tracking::AccessTimes& Ac if (auto It = m_Buckets.find(Kv.first); It != m_Buckets.end()) { CacheBucket& Bucket = *It->second; - if (!Bucket.IsOk()) - { - continue; - } Bucket.UpdateAccessTimes(Kv.second); } } @@ -1959,10 +1953,6 @@ ZenCacheDiskLayer::Get(std::string_view InBucket, const IoHash& HashKey, ZenCach if (it != m_Buckets.end()) { Bucket = it->second.get(); - if (!Bucket->IsOk()) - { - return false; - } } } @@ -1984,9 +1974,9 @@ ZenCacheDiskLayer::Get(std::string_view InBucket, const IoHash& HashKey, ZenCach std::filesystem::path BucketPath = m_RootDir; BucketPath /= BucketName; - Bucket->OpenOrCreate(BucketPath); - if (!Bucket->IsOk()) + if (!Bucket->OpenOrCreate(BucketPath)) { + m_Buckets.erase(BucketName); return false; } } @@ -2010,10 +2000,6 @@ ZenCacheDiskLayer::Put(std::string_view InBucket, const IoHash& HashKey, const Z if (it != m_Buckets.end()) { Bucket = it->second.get(); - if (!Bucket->IsOk()) - { - return; - } } } @@ -2035,9 +2021,9 @@ ZenCacheDiskLayer::Put(std::string_view InBucket, const IoHash& HashKey, const Z std::filesystem::path BucketPath = m_RootDir; BucketPath /= BucketName; - Bucket->OpenOrCreate(BucketPath); - if (!Bucket->IsOk()) + if (!Bucket->OpenOrCreate(BucketPath)) { + m_Buckets.erase(BucketName); return; } } @@ -2064,25 +2050,20 @@ ZenCacheDiskLayer::DiscoverBuckets() // New bucket needs to be created if (auto It = m_Buckets.find(BucketName); It != m_Buckets.end()) { + continue; } - else - { - auto InsertResult = m_Buckets.emplace(BucketName, std::make_unique(BucketName)); - CacheBucket& Bucket = *InsertResult.first->second; - Bucket.OpenOrCreate(BucketPath, /* AllowCreate */ false); + auto InsertResult = m_Buckets.emplace(BucketName, std::make_unique(BucketName)); + CacheBucket& Bucket = *InsertResult.first->second; - if (Bucket.IsOk()) - { - ZEN_INFO("Discovered bucket '{}'", BucketName); - } - else - { - ZEN_WARN("Found directory '{}' in our base directory '{}' but it is not a valid bucket", BucketName, m_RootDir); + if (!Bucket.OpenOrCreate(BucketPath, /* AllowCreate */ false)) + { + ZEN_WARN("Found directory '{}' in our base directory '{}' but it is not a valid bucket", BucketName, m_RootDir); - m_Buckets.erase(InsertResult.first); - } + m_Buckets.erase(InsertResult.first); + continue; } + ZEN_INFO("Discovered bucket '{}'", BucketName); } } @@ -2098,10 +2079,6 @@ ZenCacheDiskLayer::DropBucket(std::string_view InBucket) CacheBucket& Bucket = *it->second; m_DroppedBuckets.push_back(std::move(it->second)); m_Buckets.erase(it); - if (!Bucket.IsOk()) - { - return false; - } Bucket.Drop(); @@ -2125,10 +2102,6 @@ ZenCacheDiskLayer::Flush() for (auto& Kv : m_Buckets) { CacheBucket* Bucket = Kv.second.get(); - if (!Bucket->IsOk()) - { - continue; - } Buckets.push_back(Bucket); } } @@ -2147,10 +2120,6 @@ ZenCacheDiskLayer::Scrub(ScrubContext& Ctx) for (auto& Kv : m_Buckets) { CacheBucket& Bucket = *Kv.second; - if (!Bucket.IsOk()) - { - continue; - } Bucket.Scrub(Ctx); } } @@ -2163,10 +2132,6 @@ ZenCacheDiskLayer::GatherReferences(GcContext& GcCtx) for (auto& Kv : m_Buckets) { CacheBucket& Bucket = *Kv.second; - if (!Bucket.IsOk()) - { - continue; - } Bucket.GatherReferences(GcCtx); } } -- cgit v1.2.3 From 0e325ea21a56a154bdf4b92745095c588f69c817 Mon Sep 17 00:00:00 2001 From: Dan Engelbrecht Date: Wed, 25 May 2022 12:39:59 +0200 Subject: dropIndex -> DropIndex --- zenserver/cache/structuredcachestore.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'zenserver/cache/structuredcachestore.cpp') diff --git a/zenserver/cache/structuredcachestore.cpp b/zenserver/cache/structuredcachestore.cpp index bb85f9824..9ebec1e76 100644 --- a/zenserver/cache/structuredcachestore.cpp +++ b/zenserver/cache/structuredcachestore.cpp @@ -1179,7 +1179,7 @@ ZenCacheDiskLayer::CacheBucket::Put(const IoHash& HashKey, const ZenCacheValue& static bool DeleteBucketFromDisk(const std::filesystem::path& BucketDir, std::string_view BucketName) { - int dropIndex = 0; + int DropIndex = 0; do { if (!std::filesystem::exists(BucketDir)) @@ -1187,11 +1187,11 @@ DeleteBucketFromDisk(const std::filesystem::path& BucketDir, std::string_view Bu return false; } - std::string DroppedBucketName = fmt::format("[dropped]{}({})", BucketName, dropIndex); + std::string DroppedBucketName = fmt::format("[dropped]{}({})", BucketName, DropIndex); std::filesystem::path DroppedBucketPath = BucketDir.parent_path() / DroppedBucketName; if (std::filesystem::exists(DroppedBucketPath)) { - dropIndex++; + DropIndex++; continue; } -- cgit v1.2.3