Merge pull request #90 from EpicGames/de/simplify-cache-bucket-put-standalonev1.0.1.4

Fix standalone file lock in CacheBucket

1 files changed, 72 insertions, 64 deletions

diff --git a/zenserver/cache/structuredcachestore.cpp b/zenserver/cache/structuredcachestore.cpp
index a929284b9..3e2692859 100644
--- a/zenserver/cache/structuredcachestore.cpp
+++ b/zenserver/cache/structuredcachestore.cpp
@@ -1511,6 +1511,7 @@ ZenCacheDiskLayer::CacheBucket::CollectGarbage(GcContext& GcCtx)
 
 			Path.Reset();
 			BuildPath(Path, Key);
+			fs::path FilePath = Path.ToPath();
 
 			{
 				RwLock::SharedLockScope __(m_IndexLock);
@@ -1526,8 +1527,14 @@ ZenCacheDiskLayer::CacheBucket::CollectGarbage(GcContext& GcCtx)
 					ZEN_DEBUG("skipping z$ delete standalone of file '{}' FAILED, it has been added back", Path.ToUtf8());
 					continue;
 				}
-				ZEN_DEBUG("deleting standalone cache file '{}'", Path.ToUtf8());
-				fs::remove(Path.c_str(), Ec);
+				__.ReleaseNow();
+
+				RwLock::ExclusiveLockScope ValueLock(LockForHash(Key));
+				if (fs::is_regular_file(FilePath))
+				{
+					ZEN_DEBUG("deleting standalone cache file '{}'", Path.ToUtf8());
+					fs::remove(FilePath, Ec);
+				}
 			}
 
 			if (Ec)
@@ -1718,100 +1725,101 @@ ZenCacheDiskLayer::UpdateAccessTimes(const zen::access_tracking::AccessTimes& Ac
 void
 ZenCacheDiskLayer::CacheBucket::PutStandaloneCacheValue(const IoHash& HashKey, const ZenCacheValue& Value)
 {
-	RwLock::ExclusiveLockScope ValueLock(LockForHash(HashKey));
-
-	ExtendablePathBuilder<256> DataFilePath;
-	BuildPath(DataFilePath, HashKey);
-
 	TemporaryFile DataFile;
 
 	std::error_code Ec;
 	DataFile.CreateTemporary(m_BucketDir.c_str(), Ec);
-
 	if (Ec)
 	{
-		throw std::system_error(Ec, fmt::format("Failed to open temporary file for put at '{}'", m_BucketDir));
+		throw std::system_error(Ec, fmt::format("Failed to open temporary file for put in '{}'", m_BucketDir));
 	}
 
 	DataFile.WriteAll(Value.Value, Ec);
-
 	if (Ec)
 	{
-		throw std::system_error(Ec, fmt::format("Failed to write payload ({} bytes) to file", NiceBytes(Value.Value.Size())));
+		throw std::system_error(Ec,
+								fmt::format("Failed to write payload ({} bytes) to temporary file '{}' for put in '{}'",
+											NiceBytes(Value.Value.Size()),
+											DataFile.GetPath().string(),
+											m_BucketDir));
 	}
 
-	// Move file into place (atomically)
-
+	ExtendablePathBuilder<256> DataFilePath;
+	BuildPath(DataFilePath, HashKey);
 	std::filesystem::path FsPath{DataFilePath.ToPath()};
 
-	DataFile.MoveTemporaryIntoPlace(FsPath, Ec);
-
-	if (Ec)
+	// We retry to open the file since it can be held open for read.
+	// This happens if the server processes a Get request for the file or
+	// if we are busy sending the file upstream
+	int RetryCount = 3;
+	do
 	{
-		int RetryCount = 3;
-
-		do
+		Ec.clear();
 		{
-			std::filesystem::path ParentPath = FsPath.parent_path();
-			CreateDirectories(ParentPath);
-
+			RwLock::ExclusiveLockScope ValueLock(LockForHash(HashKey));
 			DataFile.MoveTemporaryIntoPlace(FsPath, Ec);
+		}
 
-			if (!Ec)
+		if (!Ec)
+		{
+			uint8_t EntryFlags = DiskLocation::kStandaloneFile;
+
+			if (Value.Value.GetContentType() == ZenContentType::kCbObject)
 			{
-				break;
+				EntryFlags |= DiskLocation::kStructured;
+			}
+			else if (Value.Value.GetContentType() == ZenContentType::kCompressedBinary)
+			{
+				EntryFlags |= DiskLocation::kCompressed;
 			}
 
-			std::error_code InnerEc;
-			const uint64_t	ExistingFileSize = std::filesystem::file_size(FsPath, InnerEc);
+			DiskLocation Loc(Value.Value.Size(), EntryFlags);
+			IndexEntry	 Entry = IndexEntry(Loc, GcClock::TickCount());
 
-			if (!InnerEc && ExistingFileSize == Value.Value.Size())
+			RwLock::ExclusiveLockScope _(m_IndexLock);
+			if (auto It = m_Index.find(HashKey); It == m_Index.end())
+			{
+				// Previously unknown object
+				m_Index.insert({HashKey, Entry});
+			}
+			else
 			{
-				// Concurrent write of same value?
-				return;
+				// TODO: should check if write is idempotent and bail out if it is?
+				It.value() = Entry;
 			}
 
-			// Semi arbitrary back-off
-			zen::Sleep(1000 * RetryCount);
-		} while (RetryCount--);
+			m_SlogFile.Append({.Key = HashKey, .Location = Loc});
+			m_TotalSize.fetch_add(Loc.Size(), std::memory_order::relaxed);
+			return;
+		}
 
-		if (Ec)
+		std::filesystem::path ParentPath = FsPath.parent_path();
+		if (!std::filesystem::is_directory(ParentPath))
 		{
-			throw std::system_error(Ec, fmt::format("Failed to finalize file '{}'", DataFilePath.ToUtf8()));
+			Ec.clear();
+			std::filesystem::create_directories(ParentPath, Ec);
+			if (!Ec)
+			{
+				// Retry without sleep
+				continue;
+			}
+			throw std::system_error(
+				Ec,
+				fmt::format("Failed to create parent directory '{}' for file '{}' for put in '{}'", ParentPath, FsPath, m_BucketDir));
 		}
-	}
-
-	// Update index
 
-	uint8_t EntryFlags = DiskLocation::kStandaloneFile;
+		ZEN_INFO("Failed renaming temporary file '{}' to '{}' for put in '{}', pausing and retrying, reason '{}'",
+				 DataFile.GetPath().string(),
+				 FsPath.string(),
+				 m_BucketDir,
+				 Ec.message());
 
-	if (Value.Value.GetContentType() == ZenContentType::kCbObject)
-	{
-		EntryFlags |= DiskLocation::kStructured;
-	}
-	else if (Value.Value.GetContentType() == ZenContentType::kCompressedBinary)
-	{
-		EntryFlags |= DiskLocation::kCompressed;
-	}
-
-	RwLock::ExclusiveLockScope _(m_IndexLock);
-
-	DiskLocation Loc(Value.Value.Size(), EntryFlags);
-	IndexEntry	 Entry = IndexEntry(Loc, GcClock::TickCount());
-
-	if (auto It = m_Index.find(HashKey); It == m_Index.end())
-	{
-		// Previously unknown object
-		m_Index.insert({HashKey, Entry});
-	}
-	else
-	{
-		// TODO: should check if write is idempotent and bail out if it is?
-		It.value() = Entry;
-	}
+		// Semi arbitrary back-off
+		zen::Sleep(200 * (4 - RetryCount));	 // Sleep at most for a total of 2 seconds
+		RetryCount--;
+	} while (RetryCount > 0);
 
-	m_SlogFile.Append({.Key = HashKey, .Location = Loc});
-	m_TotalSize.fetch_add(Loc.Size(), std::memory_order::relaxed);
+	throw std::system_error(Ec, fmt::format("Failed to finalize file '{}' for put in '{}'", DataFilePath.ToUtf8(), m_BucketDir));
 }
 
 void