fix deadlock at bucket creation (#598)

- Make sure we don't hold the namespace bucket lock when we create buckets to avoid deadlock
- Pass lock scope to helper functions to clarify locking rules
- Block flush and gc operations for a bucket that is not yet initialized
- Add ZenCacheDiskLayer::GetOrCreateBucket to avoid code duplication

1 files changed, 30 insertions, 14 deletions

diff --git a/src/zenserver/cache/cachedisklayer.h b/src/zenserver/cache/cachedisklayer.h
index 11f474d5a..277371f2c 100644
--- a/src/zenserver/cache/cachedisklayer.h
+++ b/src/zenserver/cache/cachedisklayer.h
@@ -215,7 +215,7 @@ public:
 		uint64_t	EntryCount() const;
 		BucketStats Stats();
 
-		CacheValueDetails::BucketDetails GetValueDetails(const std::string_view ValueFilter) const;
+		CacheValueDetails::BucketDetails GetValueDetails(RwLock::SharedLockScope& IndexLock, const std::string_view ValueFilter) const;
 		void EnumerateBucketContents(std::function<void(const IoHash& Key, const CacheValueDetails::ValueDetails& Details)>& Fn) const;
 
 		void GetUsageByAccess(GcClock::TimePoint TickStart, GcClock::Duration SectionLength, std::vector<uint64_t>& InOutUsageSlots);
@@ -301,7 +301,7 @@ public:
 		BucketConfiguration	  m_Configuration;
 		BlockStore			  m_BlockStore;
 		Oid					  m_BucketId;
-		std::atomic_bool	  m_IsFlushing{};
+		std::atomic_bool	  m_IsFlushing{true};  // Don't allow flush until we are properly initialized
 
 		// These files are used to manage storage of small objects for this bucket
 
@@ -342,7 +342,7 @@ public:
 		IoBuffer GetStandaloneCacheValue(ZenContentType ContentType, const IoHash& HashKey) const;
 		void	 PutInlineCacheValue(const IoHash& HashKey, const ZenCacheValue& Value, std::span<IoHash> References);
 		IoBuffer GetInlineCacheValue(const DiskLocation& Loc) const;
-		CacheValueDetails::ValueDetails GetValueDetails(const IoHash& Key, PayloadIndex Index) const;
+		CacheValueDetails::ValueDetails GetValueDetails(RwLock::SharedLockScope&, const IoHash& Key, PayloadIndex Index) const;
 
 		void		CompactReferences(RwLock::ExclusiveLockScope&);
 		void		SetReferences(RwLock::ExclusiveLockScope&, ReferenceIndex& FirstReferenceIndex, std::span<IoHash> References);
@@ -359,20 +359,35 @@ public:
 		bool		   LockedGetReferences(ReferenceIndex FirstReferenceIndex, std::vector<IoHash>& OutReferences) const;
 		void		   ClearReferenceCache();
 
-		void		   SetMetaData(BucketPayload& Payload, const ZenCacheDiskLayer::CacheBucket::BucketMetaData& MetaData);
-		void		   RemoveMetaData(BucketPayload& Payload);
-		BucketMetaData GetMetaData(const BucketPayload& Payload) const;
-		void		   SetMemCachedData(BucketPayload& Payload, IoBuffer& MemCachedData);
-		size_t		   RemoveMemCachedData(BucketPayload& Payload);
+		void		   SetMetaData(RwLock::ExclusiveLockScope&,
+								   BucketPayload&										 Payload,
+								   const ZenCacheDiskLayer::CacheBucket::BucketMetaData& MetaData);
+		void		   RemoveMetaData(RwLock::ExclusiveLockScope&, BucketPayload& Payload);
+		BucketMetaData GetMetaData(RwLock::SharedLockScope&, const BucketPayload& Payload) const;
+		void		   SetMemCachedData(RwLock::ExclusiveLockScope&, BucketPayload& Payload, IoBuffer& MemCachedData);
+		size_t		   RemoveMemCachedData(RwLock::ExclusiveLockScope&, BucketPayload& Payload);
 
-		void	 InitializeIndexFromDisk(bool IsNew);
-		uint64_t ReadIndexFile(const std::filesystem::path& IndexPath, uint32_t& OutVersion);
-		uint64_t ReadLog(const std::filesystem::path& LogPath, uint64_t LogPosition);
+		void	 InitializeIndexFromDisk(RwLock::ExclusiveLockScope&, bool IsNew);
+		uint64_t ReadIndexFile(RwLock::ExclusiveLockScope&, const std::filesystem::path& IndexPath, uint32_t& OutVersion);
+		uint64_t ReadLog(RwLock::ExclusiveLockScope&, const std::filesystem::path& LogPath, uint64_t LogPosition);
 
 		void SaveSnapshot(const std::function<uint64_t()>& ClaimDiskReserveFunc = []() { return 0; });
-		void WriteIndexSnapshot(const std::function<uint64_t()>& ClaimDiskReserveFunc = []() { return 0; });
+		void WriteIndexSnapshot(
+			RwLock::ExclusiveLockScope&,
+			const std::function<uint64_t()>& ClaimDiskReserveFunc = []() { return 0; })
+		{
+			WriteIndexSnapshotLocked(ClaimDiskReserveFunc);
+		}
+		void WriteIndexSnapshot(
+			RwLock::SharedLockScope&,
+			const std::function<uint64_t()>& ClaimDiskReserveFunc = []() { return 0; })
+		{
+			WriteIndexSnapshotLocked(ClaimDiskReserveFunc);
+		}
+		void WriteIndexSnapshotLocked(const std::function<uint64_t()>& ClaimDiskReserveFunc = []() { return 0; });
 
-		void CompactState(std::vector<BucketPayload>&  Payloads,
+		void CompactState(RwLock::ExclusiveLockScope&,
+						  std::vector<BucketPayload>&  Payloads,
 						  std::vector<AccessTime>&	   AccessTimes,
 						  std::vector<BucketMetaData>& MetaDatas,
 						  std::vector<IoBuffer>&	   MemCachedPayloads,
@@ -406,7 +421,8 @@ public:
 	};
 
 private:
-	inline void TryMemCacheTrim()
+	CacheBucket* GetOrCreateBucket(std::string_view InBucket);
+	inline void	 TryMemCacheTrim()
 	{
 		if (m_Configuration.MemCacheTargetFootprintBytes == 0)
 		{