Merge pull request #91 from EpicGames/de/block-store-gc-bugv1.0.1.3

Make sure block store owner and block store state does not get out of sync when fetching a chunk

1 files changed, 25 insertions, 50 deletions

diff --git a/zenstore/blockstore.cpp b/zenstore/blockstore.cpp
index 54a8eb9df..1946169c4 100644
--- a/zenstore/blockstore.cpp
+++ b/zenstore/blockstore.cpp
@@ -71,9 +71,9 @@ BlockStoreFile::FileSize()
 }
 
 void
-BlockStoreFile::MarkAsDeleteOnClose(std::error_code& Ec)
+BlockStoreFile::MarkAsDeleteOnClose()
 {
-	m_File.MarkAsDeleteOnClose(Ec);
+	m_IoBuffer.MarkAsDeleteOnClose();
 }
 
 IoBuffer
@@ -262,26 +262,28 @@ BlockStore::WriteChunk(const void* Data, uint64_t Size, uint64_t Alignment, Writ
 BlockStore::ReclaimSnapshotState
 BlockStore::GetReclaimSnapshotState()
 {
-	ReclaimSnapshotState	   State;
-	RwLock::ExclusiveLockScope _(m_InsertLock);
+	ReclaimSnapshotState	State;
+	RwLock::SharedLockScope _(m_InsertLock);
 	for (uint32_t BlockIndex : m_ActiveWriteBlocks)
 	{
 		State.m_ActiveWriteBlocks.insert(BlockIndex);
 	}
 	State.BlockCount = m_ChunkBlocks.size();
-	_.ReleaseNow();
 	return State;
 }
 
-Ref<BlockStoreFile>
-BlockStore::GetChunkBlock(const BlockStoreLocation& Location)
+IoBuffer
+BlockStore::TryGetChunk(const BlockStoreLocation& Location)
 {
 	RwLock::SharedLockScope InsertLock(m_InsertLock);
 	if (auto BlockIt = m_ChunkBlocks.find(Location.BlockIndex); BlockIt != m_ChunkBlocks.end())
 	{
-		return BlockIt->second;
+		if (const Ref<BlockStoreFile>& Block = BlockIt->second; Block)
+		{
+			return Block->GetChunk(Location.Offset, Location.Size);
+		}
 	}
-	return {};
+	return IoBuffer();
 }
 
 void
@@ -465,13 +467,8 @@ BlockStore::ReclaimSpace(const ReclaimSnapshotState&			Snapshot,
 						ReadBlockLongestTimeUs = std::max(ElapsedUs, ReadBlockLongestTimeUs);
 					});
 					m_ChunkBlocks[BlockIndex]	  = nullptr;
-				}
-				ZEN_DEBUG("marking cas block store file '{}' for delete, block #{}", OldBlockFile->GetPath(), BlockIndex);
-				std::error_code Ec;
-				OldBlockFile->MarkAsDeleteOnClose(Ec);
-				if (Ec)
-				{
-					ZEN_WARN("Failed to flag file '{}' for deletion: '{}'", OldBlockFile->GetPath(), Ec.message());
+					ZEN_DEBUG("marking cas block store file '{}' for delete, block #{}", OldBlockFile->GetPath(), BlockIndex);
+					OldBlockFile->MarkAsDeleteOnClose();
 				}
 				continue;
 			}
@@ -589,15 +586,9 @@ BlockStore::ReclaimSpace(const ReclaimSnapshotState&			Snapshot,
 					ReadBlockLongestTimeUs = std::max(ElapsedUs, ReadBlockLongestTimeUs);
 				});
 				m_ChunkBlocks[BlockIndex]	   = nullptr;
+				ZEN_DEBUG("marking cas block store file '{}' for delete, block #{}", OldBlockFile->GetPath(), BlockIndex);
+				OldBlockFile->MarkAsDeleteOnClose();
 			}
-			ZEN_DEBUG("marking cas block store file '{}' for delete, block #{}", OldBlockFile->GetPath(), BlockIndex);
-			std::error_code Ec;
-			OldBlockFile->MarkAsDeleteOnClose(Ec);
-			if (Ec)
-			{
-				ZEN_WARN("Failed to flag file '{}' for deletion: '{}'", OldBlockFile->GetPath(), Ec.message());
-			}
-			OldBlockFile = nullptr;
 		}
 	}
 	catch (std::exception& ex)
@@ -606,12 +597,7 @@ BlockStore::ReclaimSpace(const ReclaimSnapshotState&			Snapshot,
 		if (NewBlockFile)
 		{
 			ZEN_DEBUG("dropping incomplete cas block store file '{}'", NewBlockFile->GetPath());
-			std::error_code Ec;
-			NewBlockFile->MarkAsDeleteOnClose(Ec);
-			if (Ec)
-			{
-				ZEN_WARN("Failed to flag file '{}' for deletion: '{}'", NewBlockFile->GetPath(), Ec.message());
-			}
+			NewBlockFile->MarkAsDeleteOnClose();
 		}
 	}
 }
@@ -1032,9 +1018,7 @@ TEST_CASE("blockstore.blockfile")
 		{
 			BlockStoreFile File1(RootDirectory / "1");
 			File1.Open();
-			std::error_code Ec;
-			File1.MarkAsDeleteOnClose(Ec);
-			CHECK(!Ec);
+			File1.MarkAsDeleteOnClose();
 			DataChunk = File1.GetChunk(0, 5);
 			BoopChunk = File1.GetChunk(5, 5);
 		}
@@ -1058,12 +1042,7 @@ namespace {
 
 	std::string ReadChunkAsString(BlockStore& Store, const BlockStoreLocation& Location)
 	{
-		Ref<BlockStoreFile> ChunkBlock(Store.GetChunkBlock(Location));
-		if (!ChunkBlock)
-		{
-			return "";
-		}
-		IoBuffer ChunkData = ChunkBlock->GetChunk(Location.Offset, Location.Size);
+		IoBuffer ChunkData = Store.TryGetChunk(Location);
 		if (!ChunkData)
 		{
 			return "";
@@ -1129,7 +1108,7 @@ TEST_CASE("blockstore.chunks")
 
 	BlockStore Store;
 	Store.Initialize(RootDirectory, 128, 1024, {});
-	Ref<BlockStoreFile> BadChunk = Store.GetChunkBlock({.BlockIndex = 0, .Offset = 0, .Size = 512});
+	IoBuffer BadChunk = Store.TryGetChunk({.BlockIndex = 0, .Offset = 0, .Size = 512});
 	CHECK(!BadChunk);
 
 	std::string		   FirstChunkData	   = "This is the data of the first chunk that we will write";
@@ -1202,7 +1181,7 @@ TEST_CASE("blockstore.iterate.chunks")
 
 	BlockStore Store;
 	Store.Initialize(RootDirectory / "store", ScrubSmallChunkWindowSize * 2, 1024, {});
-	Ref<BlockStoreFile> BadChunk = Store.GetChunkBlock({.BlockIndex = 0, .Offset = 0, .Size = 512});
+	IoBuffer BadChunk = Store.TryGetChunk({.BlockIndex = 0, .Offset = 0, .Size = 512});
 	CHECK(!BadChunk);
 
 	std::string		   FirstChunkData	  = "This is the data of the first chunk that we will write";
@@ -1264,6 +1243,7 @@ TEST_CASE("blockstore.reclaim.space")
 	for (size_t ChunkIndex = 0; ChunkIndex < ChunkCount; ++ChunkIndex)
 	{
 		IoBuffer Chunk = CreateChunk(57 + ChunkIndex);
+
 		Store.WriteChunk(Chunk.Data(), Chunk.Size(), Alignment, [&](const BlockStoreLocation& L) { ChunkLocations.push_back(L); });
 		ChunkHashes.push_back(IoHash::HashBuffer(Chunk.Data(), Chunk.Size()));
 	}
@@ -1331,11 +1311,10 @@ TEST_CASE("blockstore.reclaim.space")
 
 	for (size_t ChunkIndex = 0; ChunkIndex < ChunkCount; ++ChunkIndex)
 	{
-		Ref<BlockStoreFile> ChunkBlock = Store.GetChunkBlock(NewChunkLocations[ChunkIndex]);
+		IoBuffer ChunkBlock = Store.TryGetChunk(NewChunkLocations[ChunkIndex]);
 		if (ChunkIndex >= DeleteChunkCount)
 		{
-			CHECK(ChunkBlock);
-			IoBuffer VerifyChunk = ChunkBlock->GetChunk(NewChunkLocations[ChunkIndex].Offset, NewChunkLocations[ChunkIndex].Size);
+			IoBuffer VerifyChunk = Store.TryGetChunk(NewChunkLocations[ChunkIndex]);
 			CHECK(VerifyChunk);
 			IoHash VerifyHash = IoHash::HashBuffer(VerifyChunk.Data(), VerifyChunk.Size());
 			CHECK(VerifyHash == ChunkHashes[ChunkIndex]);
@@ -1405,9 +1384,7 @@ TEST_CASE("blockstore.thread.read.write")
 	for (size_t ChunkIndex = 0; ChunkIndex < ChunkCount; ++ChunkIndex)
 	{
 		WorkerPool.ScheduleWork([&Store, ChunkIndex, &ChunkLocations, &ChunkHashes, &WorkCompleted]() {
-			Ref<BlockStoreFile> ChunkBlock = Store.GetChunkBlock(ChunkLocations[ChunkIndex]);
-			CHECK(ChunkBlock);
-			IoBuffer VerifyChunk = ChunkBlock->GetChunk(ChunkLocations[ChunkIndex].Offset, ChunkLocations[ChunkIndex].Size);
+			IoBuffer VerifyChunk = Store.TryGetChunk(ChunkLocations[ChunkIndex]);
 			CHECK(VerifyChunk);
 			IoHash VerifyHash = IoHash::HashBuffer(VerifyChunk.Data(), VerifyChunk.Size());
 			CHECK(VerifyHash == ChunkHashes[ChunkIndex]);
@@ -1432,9 +1409,7 @@ TEST_CASE("blockstore.thread.read.write")
 			WorkCompleted.fetch_add(1);
 		});
 		WorkerPool.ScheduleWork([&Store, ChunkIndex, &ChunkLocations, &ChunkHashes, &WorkCompleted]() {
-			Ref<BlockStoreFile> ChunkBlock = Store.GetChunkBlock(ChunkLocations[ChunkIndex]);
-			CHECK(ChunkBlock);
-			IoBuffer VerifyChunk = ChunkBlock->GetChunk(ChunkLocations[ChunkIndex].Offset, ChunkLocations[ChunkIndex].Size);
+			IoBuffer VerifyChunk = Store.TryGetChunk(ChunkLocations[ChunkIndex]);
 			CHECK(VerifyChunk);
 			IoHash VerifyHash = IoHash::HashBuffer(VerifyChunk.Data(), VerifyChunk.Size());
 			CHECK(VerifyHash == ChunkHashes[ChunkIndex]);