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

5 files changed, 38 insertions, 94 deletions

diff --git a/zenstore/basicfile.cpp b/zenstore/basicfile.cpp
index 8eb172a1c..e5a2adc41 100644
--- a/zenstore/basicfile.cpp
+++ b/zenstore/basicfile.cpp
@@ -373,31 +373,6 @@ BasicFile::SetFileSize(uint64_t FileSize)
 #endif
 }
 
-void
-BasicFile::MarkAsDeleteOnClose(std::error_code& Ec)
-{
-	Ec.clear();
-#if ZEN_PLATFORM_WINDOWS
-	FILE_DISPOSITION_INFO Fdi{};
-	Fdi.DeleteFile = TRUE;
-	BOOL Success   = SetFileInformationByHandle(m_FileHandle, FileDispositionInfo, &Fdi, sizeof Fdi);
-	if (!Success)
-	{
-		Ec = MakeErrorCodeFromLastError();
-	}
-#elif ZEN_PLATFORM_LINUX || ZEN_PLATFORM_MAC
-	std::filesystem::path SourcePath = PathFromHandle(m_FileHandle);
-	if (unlink(SourcePath.c_str()) < 0)
-	{
-		int UnlinkError = zen::GetLastError();
-		if (UnlinkError != ENOENT)
-		{
-			Ec = MakeErrorCode(UnlinkError);
-		}
-	}
-#endif
-}
-
 void*
 BasicFile::Detach()
 {
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]);
diff --git a/zenstore/compactcas.cpp b/zenstore/compactcas.cpp
index cc0e2241c..2d48265f7 100644
--- a/zenstore/compactcas.cpp
+++ b/zenstore/compactcas.cpp
@@ -258,8 +258,8 @@ CasContainerStrategy::InsertChunk(const void* ChunkData, size_t ChunkSize, const
 			RwLock::ExclusiveLockScope _(m_LocationMapLock);
 			m_LocationMap.emplace(ChunkHash, DiskLocation);
 		}
-		m_TotalSize.fetch_add(static_cast<uint64_t>(ChunkSize), std::memory_order::relaxed);
 	});
+	m_TotalSize.fetch_add(static_cast<uint64_t>(ChunkSize), std::memory_order::relaxed);
 
 	return CasStore::InsertResult{.New = true};
 }
@@ -279,16 +279,10 @@ CasContainerStrategy::FindChunk(const IoHash& ChunkHash)
 	{
 		return IoBuffer();
 	}
-	BlockStoreLocation Location = KeyIt->second.Get(m_PayloadAlignment);
-	_.ReleaseNow();
-
-	Ref<BlockStoreFile> ChunkBlock = m_BlockStore.GetChunkBlock(Location);
-	if (!ChunkBlock)
-	{
-		return IoBuffer();
-	}
+	const BlockStoreLocation& Location = KeyIt->second.Get(m_PayloadAlignment);
 
-	return ChunkBlock->GetChunk(Location.Offset, Location.Size);
+	IoBuffer Chunk = m_BlockStore.TryGetChunk(Location);
+	return Chunk;
 }
 
 bool
diff --git a/zenstore/include/zenstore/basicfile.h b/zenstore/include/zenstore/basicfile.h
index 5a500c65f..ce9988776 100644
--- a/zenstore/include/zenstore/basicfile.h
+++ b/zenstore/include/zenstore/basicfile.h
@@ -33,11 +33,12 @@ public:
 
 	enum class Mode : uint32_t
 	{
-		kRead			= 0,  // Opens a existing file for read only
-		kWrite			= 1,  // Opens (or creates) a file for read and write
-		kTruncate		= 2,  // Opens (or creates) a file for read and write and sets the size to zero
-		kDelete			= 3,  // Opens (or creates) a file for read and write enabling MarkAsDeleteOnClose()
-		kTruncateDelete = 4	  // Opens (or creates) a file for read and write and sets the size to zero enabling MarkAsDeleteOnClose()
+		kRead	  = 0,	// Opens a existing file for read only
+		kWrite	  = 1,	// Opens (or creates) a file for read and write
+		kTruncate = 2,	// Opens (or creates) a file for read and write and sets the size to zero
+		kDelete	  = 3,	// Opens (or creates) a file for read and write allowing .DeleteFile file disposition to be set
+		kTruncateDelete =
+			4  // Opens (or creates) a file for read and write and sets the size to zero allowing .DeleteFile file disposition to be set
 	};
 
 	void	 Open(const std::filesystem::path& FileName, Mode Mode);
@@ -55,7 +56,6 @@ public:
 	void	 SetFileSize(uint64_t FileSize);
 	IoBuffer ReadAll();
 	void	 WriteAll(IoBuffer Data, std::error_code& Ec);
-	void	 MarkAsDeleteOnClose(std::error_code& Ec);
 	void*	 Detach();
 
 	inline void* Handle() { return m_FileHandle; }
diff --git a/zenstore/include/zenstore/blockstore.h b/zenstore/include/zenstore/blockstore.h
index 9edfc36e8..34c475fb6 100644
--- a/zenstore/include/zenstore/blockstore.h
+++ b/zenstore/include/zenstore/blockstore.h
@@ -89,7 +89,7 @@ struct BlockStoreFile : public RefCounted
 	const std::filesystem::path& GetPath() const;
 	void						 Open();
 	void						 Create(uint64_t InitialSize);
-	void						 MarkAsDeleteOnClose(std::error_code& Ec);
+	void						 MarkAsDeleteOnClose();
 	uint64_t					 FileSize();
 	IoBuffer					 GetChunk(uint64_t Offset, uint64_t Size);
 	void						 Read(void* Data, uint64_t Size, uint64_t FileOffset);
@@ -133,8 +133,8 @@ public:
 
 	void WriteChunk(const void* Data, uint64_t Size, uint64_t Alignment, WriteChunkCallback Callback);
 
-	Ref<BlockStoreFile> GetChunkBlock(const BlockStoreLocation& Location);
-	void				Flush();
+	IoBuffer TryGetChunk(const BlockStoreLocation& Location);
+	void	 Flush();
 
 	ReclaimSnapshotState GetReclaimSnapshotState();
 	void				 ReclaimSpace(