structured cache: Added locking around large cache value updates to prevent races leading to file sharing violations

1 files changed, 37 insertions, 13 deletions

diff --git a/zenserver/cache/structuredcachestore.cpp b/zenserver/cache/structuredcachestore.cpp
index 83a21f87a..580446473 100644
--- a/zenserver/cache/structuredcachestore.cpp
+++ b/zenserver/cache/structuredcachestore.cpp
@@ -362,9 +362,19 @@ private:
 	uint64_t											 m_WriteCursor = 0;
 
 	void BuildPath(WideStringBuilderBase& Path, const IoHash& HashKey);
-	void PutLargeObject(const IoHash& HashKey, const ZenCacheValue& Value);
-	bool GetStandaloneCacheValue(const IoHash& HashKey, ZenCacheValue& OutValue, const DiskLocation& Loc);
+	void PutStandaloneCacheValue(const IoHash& HashKey, const ZenCacheValue& Value);
+	bool GetStandaloneCacheValue(const DiskLocation& Loc, const IoHash& HashKey, ZenCacheValue& OutValue);
 	bool GetInlineCacheValue(const DiskLocation& Loc, ZenCacheValue& OutValue);
+
+	// These locks are here to avoid contention on file creation, therefore it's sufficient
+	// that we take the same lock for the same hash
+	//
+	// These locks are small and should really be spaced out so they don't share cache lines,
+	// but we don't currently access them at particularly high frequency so it should not be
+	// an issue in practice
+
+	RwLock		   m_ShardedLocks[256];
+	inline RwLock& LockForHash(const IoHash& Hash) { return m_ShardedLocks[Hash.Hash[19]]; }
 };
 
 ZenCacheDiskLayer::CacheBucket::CacheBucket(CasStore& Cas) : m_CasStore(Cas)
@@ -491,23 +501,25 @@ ZenCacheDiskLayer::CacheBucket::BuildPath(WideStringBuilderBase& Path, const IoH
 bool
 ZenCacheDiskLayer::CacheBucket::GetInlineCacheValue(const DiskLocation& Loc, ZenCacheValue& OutValue)
 {
-	if (!Loc.IsFlagSet(DiskLocation::kStandaloneFile))
+	if (Loc.IsFlagSet(DiskLocation::kStandaloneFile))
 	{
-		OutValue.Value = IoBufferBuilder::MakeFromFileHandle(m_SobsFile.Handle(), Loc.Offset(), Loc.Size());
-		OutValue.Value.SetContentType(Loc.GetContentType());
-
-		return true;
+		return false;
 	}
 
-	return false;
+	OutValue.Value = IoBufferBuilder::MakeFromFileHandle(m_SobsFile.Handle(), Loc.Offset(), Loc.Size());
+	OutValue.Value.SetContentType(Loc.GetContentType());
+
+	return true;
 }
 
 bool
-ZenCacheDiskLayer::CacheBucket::GetStandaloneCacheValue(const IoHash& HashKey, ZenCacheValue& OutValue, const DiskLocation& Loc)
+ZenCacheDiskLayer::CacheBucket::GetStandaloneCacheValue(const DiskLocation& Loc, const IoHash& HashKey, ZenCacheValue& OutValue)
 {
 	WideStringBuilder<128> DataFilePath;
 	BuildPath(DataFilePath, HashKey);
 
+	RwLock::SharedLockScope ValueLock(LockForHash(HashKey));
+
 	if (IoBuffer Data = IoBufferBuilder::MakeFromFile(DataFilePath.c_str()))
 	{
 		OutValue.Value = Data;
@@ -540,7 +552,7 @@ ZenCacheDiskLayer::CacheBucket::Get(const IoHash& HashKey, ZenCacheValue& OutVal
 
 		_.ReleaseNow();
 
-		return GetStandaloneCacheValue(HashKey, OutValue, Loc);
+		return GetStandaloneCacheValue(Loc, HashKey, OutValue);
 	}
 
 	return false;
@@ -556,7 +568,7 @@ ZenCacheDiskLayer::CacheBucket::Put(const IoHash& HashKey, const ZenCacheValue&
 
 	if (Value.Value.Size() >= m_LargeObjectThreshold)
 	{
-		return PutLargeObject(HashKey, Value);
+		return PutStandaloneCacheValue(HashKey, Value);
 	}
 	else
 	{
@@ -615,6 +627,8 @@ ZenCacheDiskLayer::CacheBucket::Scrub(ScrubContext& Ctx)
 {
 	std::atomic<uint64_t> ScrubbedChunks{0}, ScrubbedBytes{0};
 
+	std::vector<IoHash> BadChunks;
+
 	{
 		RwLock::SharedLockScope _(m_IndexLock);
 
@@ -633,7 +647,7 @@ ZenCacheDiskLayer::CacheBucket::Scrub(ScrubContext& Ctx)
 			{
 				if (Loc.IsFlagSet(DiskLocation::kStandaloneFile))
 				{
-					if (GetStandaloneCacheValue(HashKey, Value, Loc))
+					if (GetStandaloneCacheValue(Loc, HashKey, Value))
 					{
 						// Note: we cannot currently validate contents since we don't
 						// have a content hash!
@@ -641,6 +655,7 @@ ZenCacheDiskLayer::CacheBucket::Scrub(ScrubContext& Ctx)
 					else
 					{
 						// Value not found
+						BadChunks.push_back(HashKey);
 					}
 				}
 			}
@@ -649,6 +664,13 @@ ZenCacheDiskLayer::CacheBucket::Scrub(ScrubContext& Ctx)
 
 	Ctx.ReportScrubbed(ScrubbedChunks, ScrubbedBytes);
 
+	if (BadChunks.empty())
+	{
+		return;
+	}
+
+	Ctx.ReportBadCasChunks(BadChunks);
+
 	if (Ctx.RunRecovery())
 	{
 		// Clean out bad data
@@ -662,8 +684,10 @@ ZenCacheDiskLayer::CacheBucket::GarbageCollect(GcContext& GcCtx)
 }
 
 void
-ZenCacheDiskLayer::CacheBucket::PutLargeObject(const IoHash& HashKey, const ZenCacheValue& Value)
+ZenCacheDiskLayer::CacheBucket::PutStandaloneCacheValue(const IoHash& HashKey, const ZenCacheValue& Value)
 {
+	RwLock::ExclusiveLockScope ValueLock(LockForHash(HashKey));
+
 	WideStringBuilder<128> DataFilePath;
 	BuildPath(DataFilePath, HashKey);