Remove any unreferenced blocks in block store on open (#492)

* Remove any unreferenced blocks in block store on open

5 files changed, 28 insertions, 17 deletions

diff --git a/src/zenserver/cache/cachedisklayer.cpp b/src/zenserver/cache/cachedisklayer.cpp
index aeea779fb..73c02bcc2 100644
--- a/src/zenserver/cache/cachedisklayer.cpp
+++ b/src/zenserver/cache/cachedisklayer.cpp
@@ -650,7 +650,7 @@ ZenCacheDiskLayer::CacheBucket::OpenLog(const bool IsNew)
 		KnownLocations.push_back(BlockLocation);
 	}
 
-	m_BlockStore.CreateMissingBlocks(KnownLocations);
+	m_BlockStore.SyncExistingBlocksOnDisk(KnownLocations);
 
 	if (IsNew || LogEntryCount > 0)
 	{
diff --git a/src/zenserver/cache/structuredcachestore.cpp b/src/zenserver/cache/structuredcachestore.cpp
index 207323451..77316a5dc 100644
--- a/src/zenserver/cache/structuredcachestore.cpp
+++ b/src/zenserver/cache/structuredcachestore.cpp
@@ -1120,18 +1120,13 @@ TEST_CASE("z$.gc")
 					const bool	  Exists = Zcs.Get(Bucket, Key, CacheValue);
 					CHECK(!Exists);
 				}
+				// GC could not remove the currently written block so size will not be zero
+				CHECK_NE(0, Zcs.StorageSize().DiskSize);
 			}
 		}
 		{
-			// GC could not remove the currently written block so size will not be zero
+			// Unreferenced blocks will be pruned so size should now be zero
 			ZenCacheNamespace Zcs(Gc, *JobQueue, TempDir.Path() / "cache", true);
-			CHECK_NE(0, Zcs.StorageSize().DiskSize);
-
-			// GC again and now the stray write block should be removed
-			GcContext GcCtx(GcClock::Now() + std::chrono::minutes(2), GcClock::Now() + std::chrono::minutes(2));
-			GcCtx.CollectSmallObjects(true);
-			Gc.CollectGarbage(GcCtx);
-
 			CHECK_EQ(0, Zcs.StorageSize().DiskSize);
 		}
 	}
diff --git a/src/zenstore/blockstore.cpp b/src/zenstore/blockstore.cpp
index c4453a39f..c5f978d18 100644
--- a/src/zenstore/blockstore.cpp
+++ b/src/zenstore/blockstore.cpp
@@ -224,15 +224,22 @@ BlockStore::Initialize(const std::filesystem::path& BlocksBasePath, uint64_t Max
 }
 
 void
-BlockStore::CreateMissingBlocks(const std::vector<BlockStoreLocation>& KnownLocations)
+BlockStore::SyncExistingBlocksOnDisk(const std::vector<BlockStoreLocation>& KnownLocations)
 {
-	ZEN_TRACE_CPU("BlockStore::CreateMissingBlocks");
+	ZEN_TRACE_CPU("BlockStore::SyncExistingBlocksOnDisk");
 
 	RwLock::ExclusiveLockScope InsertLock(m_InsertLock);
 
 	tsl::robin_set<uint32_t> MissingBlocks;
+	tsl::robin_set<uint32_t> DeleteBlocks;
+	DeleteBlocks.reserve(m_ChunkBlocks.size());
+	for (auto It : m_ChunkBlocks)
+	{
+		DeleteBlocks.insert(It.first);
+	}
 	for (const auto& Entry : KnownLocations)
 	{
+		DeleteBlocks.erase(Entry.BlockIndex);
 		if (auto It = m_ChunkBlocks.find(Entry.BlockIndex); It != m_ChunkBlocks.end() && !It->second.IsNull())
 		{
 			continue;
@@ -246,6 +253,16 @@ BlockStore::CreateMissingBlocks(const std::vector<BlockStoreLocation>& KnownLoca
 		NewBlockFile->Create(0);
 		m_ChunkBlocks[BlockIndex] = NewBlockFile;
 	}
+	for (std::uint32_t BlockIndex : DeleteBlocks)
+	{
+		std::filesystem::path BlockPath = GetBlockPath(m_BlocksBasePath, BlockIndex);
+		if (m_ChunkBlocks[BlockIndex])
+		{
+			m_TotalSize.fetch_sub(m_ChunkBlocks[BlockIndex]->FileSize(), std::memory_order::relaxed);
+			m_ChunkBlocks[BlockIndex]->MarkAsDeleteOnClose();
+		}
+		m_ChunkBlocks.erase(BlockIndex);
+	}
 }
 
 void
@@ -1193,8 +1210,8 @@ TEST_CASE("blockstore.clean.stray.blocks")
 	ThirdChunk = Store.TryGetChunk(ThirdChunkLocation);
 	CHECK(!ThirdChunk);
 
-	// Recreate a fake block for an missing chunk location
-	Store.CreateMissingBlocks({FirstChunkLocation, SecondChunkLocation, ThirdChunkLocation});
+	// Recreate a fake block for a missing chunk location
+	Store.SyncExistingBlocksOnDisk({FirstChunkLocation, SecondChunkLocation, ThirdChunkLocation});
 	// We create a fake block for the location - we should still not be able to get the chunk
 	CHECK(GetDirectoryContent(RootDirectory / "store", true, false).size() == 2);
 	ThirdChunk = Store.TryGetChunk(ThirdChunkLocation);
diff --git a/src/zenstore/compactcas.cpp b/src/zenstore/compactcas.cpp
index 0468e95de..115bdcf03 100644
--- a/src/zenstore/compactcas.cpp
+++ b/src/zenstore/compactcas.cpp
@@ -845,7 +845,7 @@ CasContainerStrategy::OpenContainer(bool IsNewStore)
 		KnownLocations.emplace_back(std::move(BlockLocation));
 	}
 
-	m_BlockStore.CreateMissingBlocks(KnownLocations);
+	m_BlockStore.SyncExistingBlocksOnDisk(KnownLocations);
 
 	if (IsNewStore || (LogEntryCount > 0))
 	{
diff --git a/src/zenstore/include/zenstore/blockstore.h b/src/zenstore/include/zenstore/blockstore.h
index 48d33edbc..56906f570 100644
--- a/src/zenstore/include/zenstore/blockstore.h
+++ b/src/zenstore/include/zenstore/blockstore.h
@@ -132,9 +132,8 @@ public:
 	void Initialize(const std::filesystem::path& BlocksBasePath, uint64_t MaxBlockSize, uint64_t MaxBlockCount);
 
 	// Ask the store to create empty blocks for all locations that does not have a block
-	// This is to make sure we don't have locations that points to a block that will be written
-	// in the future which would result in us getting garbage data
-	void CreateMissingBlocks(const std::vector<BlockStoreLocation>& KnownLocations);
+	// Remove any block that is not referenced
+	void SyncExistingBlocksOnDisk(const std::vector<BlockStoreLocation>& KnownLocations);
 
 	void Close();