reduce work when there are no blocks to compact (#558)

* reduce work when there are no blocks to compact
* fix lock scopes

2 files changed, 115 insertions, 105 deletions

diff --git a/src/zenserver/cache/cachedisklayer.cpp b/src/zenserver/cache/cachedisklayer.cpp
index 2be32e372..af8b6227b 100644
--- a/src/zenserver/cache/cachedisklayer.cpp
+++ b/src/zenserver/cache/cachedisklayer.cpp
@@ -2448,6 +2448,7 @@ public:
 		{
 			std::unordered_map<uint32_t, uint64_t> BlockUsage;
 			{
+				RwLock::SharedLockScope __(m_Bucket.m_IndexLock);
 				for (const auto& Entry : m_Bucket.m_Index)
 				{
 					ZenCacheDiskLayer::CacheBucket::PayloadIndex		 Index	 = Entry.second;
@@ -2480,73 +2481,79 @@ public:
 					m_Bucket.m_BlockStore.GetBlocksToCompact(BlockUsage, Ctx.Settings.CompactBlockUsageThresholdPercent);
 				BlockCompactState.IncludeBlocks(BlocksToCompact);
 
+				if (BlocksToCompact.size() > 0)
 				{
-					RwLock::SharedLockScope __(m_Bucket.m_IndexLock);
-					for (const auto& Entry : m_Bucket.m_Index)
 					{
-						ZenCacheDiskLayer::CacheBucket::PayloadIndex		 Index	 = Entry.second;
-						const ZenCacheDiskLayer::CacheBucket::BucketPayload& Payload = m_Bucket.m_Payloads[Index];
-						const DiskLocation&									 Loc	 = Payload.Location;
-
-						if (Loc.IsFlagSet(DiskLocation::kStandaloneFile))
-						{
-							continue;
-						}
-						if (!BlockCompactState.AddKeepLocation(Loc.GetBlockLocation(m_Bucket.m_Configuration.PayloadAlignment)))
+						RwLock::SharedLockScope __(m_Bucket.m_IndexLock);
+						for (const auto& Entry : m_Bucket.m_Index)
 						{
-							continue;
+							ZenCacheDiskLayer::CacheBucket::PayloadIndex		 Index	 = Entry.second;
+							const ZenCacheDiskLayer::CacheBucket::BucketPayload& Payload = m_Bucket.m_Payloads[Index];
+							const DiskLocation&									 Loc	 = Payload.Location;
+
+							if (Loc.IsFlagSet(DiskLocation::kStandaloneFile))
+							{
+								continue;
+							}
+							if (!BlockCompactState.AddKeepLocation(Loc.GetBlockLocation(m_Bucket.m_Configuration.PayloadAlignment)))
+							{
+								continue;
+							}
+							BlockCompactStateKeys.push_back(Entry.first);
 						}
-						BlockCompactStateKeys.push_back(Entry.first);
 					}
-				}
 
-				if (Ctx.Settings.IsDeleteMode)
-				{
-					if (Ctx.Settings.Verbose)
+					if (Ctx.Settings.IsDeleteMode)
 					{
-						ZEN_INFO("GCV2: cachebucket [COMPACT] '{}': compacting {} blocks", m_Bucket.m_BucketDir, BlocksToCompact.size());
-					}
-
-					m_Bucket.m_BlockStore.CompactBlocks(
-						BlockCompactState,
-						m_Bucket.m_Configuration.PayloadAlignment,
-						[&](const BlockStore::MovedChunksArray& MovedArray, uint64_t FreedDiskSpace) {
-							std::vector<DiskIndexEntry> MovedEntries;
-							RwLock::ExclusiveLockScope	_(m_Bucket.m_IndexLock);
-							for (const std::pair<size_t, BlockStoreLocation>& Moved : MovedArray)
-							{
-								size_t		  ChunkIndex = Moved.first;
-								const IoHash& Key		 = BlockCompactStateKeys[ChunkIndex];
+						if (Ctx.Settings.Verbose)
+						{
+							ZEN_INFO("GCV2: cachebucket [COMPACT] '{}': compacting {} blocks",
+									 m_Bucket.m_BucketDir,
+									 BlocksToCompact.size());
+						}
 
-								if (auto It = m_Bucket.m_Index.find(Key); It != m_Bucket.m_Index.end())
+						m_Bucket.m_BlockStore.CompactBlocks(
+							BlockCompactState,
+							m_Bucket.m_Configuration.PayloadAlignment,
+							[&](const BlockStore::MovedChunksArray& MovedArray, uint64_t FreedDiskSpace) {
+								std::vector<DiskIndexEntry> MovedEntries;
+								RwLock::ExclusiveLockScope	_(m_Bucket.m_IndexLock);
+								for (const std::pair<size_t, BlockStoreLocation>& Moved : MovedArray)
 								{
-									ZenCacheDiskLayer::CacheBucket::BucketPayload& Payload	   = m_Bucket.m_Payloads[It->second];
-									const BlockStoreLocation&					   OldLocation = BlockCompactState.GetLocation(ChunkIndex);
-									if (Payload.Location.GetBlockLocation(m_Bucket.m_Configuration.PayloadAlignment) != OldLocation)
+									size_t		  ChunkIndex = Moved.first;
+									const IoHash& Key		 = BlockCompactStateKeys[ChunkIndex];
+
+									if (auto It = m_Bucket.m_Index.find(Key); It != m_Bucket.m_Index.end())
 									{
-										// Someone has moved our chunk so lets just skip the new location we were provided, it will be GC:d
-										// at a later time
-										continue;
+										ZenCacheDiskLayer::CacheBucket::BucketPayload& Payload = m_Bucket.m_Payloads[It->second];
+										const BlockStoreLocation& OldLocation				   = BlockCompactState.GetLocation(ChunkIndex);
+										if (Payload.Location.GetBlockLocation(m_Bucket.m_Configuration.PayloadAlignment) != OldLocation)
+										{
+											// Someone has moved our chunk so lets just skip the new location we were provided, it will be
+											// GC:d at a later time
+											continue;
+										}
+										const BlockStoreLocation& NewLocation = Moved.second;
+
+										Payload.Location = DiskLocation(NewLocation,
+																		m_Bucket.m_Configuration.PayloadAlignment,
+																		Payload.Location.GetFlags());
+										MovedEntries.push_back({.Key = Key, .Location = Payload.Location});
 									}
-									const BlockStoreLocation& NewLocation = Moved.second;
-
-									Payload.Location =
-										DiskLocation(NewLocation, m_Bucket.m_Configuration.PayloadAlignment, Payload.Location.GetFlags());
-									MovedEntries.push_back({.Key = Key, .Location = Payload.Location});
 								}
-							}
-							m_Bucket.m_SlogFile.Append(MovedEntries);
-							Stats.RemovedDisk += FreedDiskSpace;
-						},
-						ClaimDiskReserveCallback);
-				}
-				else
-				{
-					if (Ctx.Settings.Verbose)
+								m_Bucket.m_SlogFile.Append(MovedEntries);
+								Stats.RemovedDisk += FreedDiskSpace;
+							},
+							ClaimDiskReserveCallback);
+					}
+					else
 					{
-						ZEN_INFO("GCV2: cachebucket [COMPACT] '{}': skipped compacting of {} eligible blocks",
-								 m_Bucket.m_BucketDir,
-								 BlocksToCompact.size());
+						if (Ctx.Settings.Verbose)
+						{
+							ZEN_INFO("GCV2: cachebucket [COMPACT] '{}': skipped compacting of {} eligible blocks",
+									 m_Bucket.m_BucketDir,
+									 BlocksToCompact.size());
+						}
 					}
 				}
 			}
diff --git a/src/zenstore/compactcas.cpp b/src/zenstore/compactcas.cpp
index 1d608be8d..02db5f848 100644
--- a/src/zenstore/compactcas.cpp
+++ b/src/zenstore/compactcas.cpp
@@ -604,70 +604,73 @@ public:
 					m_CasContainerStrategy.m_BlockStore.GetBlocksToCompact(BlockUsage, Ctx.Settings.CompactBlockUsageThresholdPercent);
 				BlockCompactState.IncludeBlocks(BlocksToCompact);
 
+				if (BlocksToCompact.size() > 0)
 				{
-					RwLock::SharedLockScope __(m_CasContainerStrategy.m_LocationMapLock);
-					for (const auto& Entry : m_CasContainerStrategy.m_LocationMap)
 					{
-						size_t						  Index = Entry.second;
-						const BlockStoreDiskLocation& Loc	= m_CasContainerStrategy.m_Locations[Index];
-
-						if (!BlockCompactState.AddKeepLocation(Loc.Get(m_CasContainerStrategy.m_PayloadAlignment)))
+						RwLock::SharedLockScope __(m_CasContainerStrategy.m_LocationMapLock);
+						for (const auto& Entry : m_CasContainerStrategy.m_LocationMap)
 						{
-							continue;
+							size_t						  Index = Entry.second;
+							const BlockStoreDiskLocation& Loc	= m_CasContainerStrategy.m_Locations[Index];
+
+							if (!BlockCompactState.AddKeepLocation(Loc.Get(m_CasContainerStrategy.m_PayloadAlignment)))
+							{
+								continue;
+							}
+							BlockCompactStateKeys.push_back(Entry.first);
 						}
-						BlockCompactStateKeys.push_back(Entry.first);
 					}
-				}
 
-				if (Ctx.Settings.IsDeleteMode)
-				{
-					if (Ctx.Settings.Verbose)
+					if (Ctx.Settings.IsDeleteMode)
 					{
-						ZEN_INFO("GCV2: compactcas [COMPACT] '{}': compacting {} blocks",
-								 m_CasContainerStrategy.m_RootDirectory / m_CasContainerStrategy.m_ContainerBaseName,
-								 BlocksToCompact.size());
-					}
-
-					m_CasContainerStrategy.m_BlockStore.CompactBlocks(
-						BlockCompactState,
-						m_CasContainerStrategy.m_PayloadAlignment,
-						[&](const BlockStore::MovedChunksArray& MovedArray, uint64_t FreedDiskSpace) {
-							std::vector<CasDiskIndexEntry> MovedEntries;
-							RwLock::ExclusiveLockScope	   _(m_CasContainerStrategy.m_LocationMapLock);
-							for (const std::pair<size_t, BlockStoreLocation>& Moved : MovedArray)
-							{
-								size_t		  ChunkIndex = Moved.first;
-								const IoHash& Key		 = BlockCompactStateKeys[ChunkIndex];
+						if (Ctx.Settings.Verbose)
+						{
+							ZEN_INFO("GCV2: compactcas [COMPACT] '{}': compacting {} blocks",
+									 m_CasContainerStrategy.m_RootDirectory / m_CasContainerStrategy.m_ContainerBaseName,
+									 BlocksToCompact.size());
+						}
 
-								if (auto It = m_CasContainerStrategy.m_LocationMap.find(Key);
-									It != m_CasContainerStrategy.m_LocationMap.end())
+						m_CasContainerStrategy.m_BlockStore.CompactBlocks(
+							BlockCompactState,
+							m_CasContainerStrategy.m_PayloadAlignment,
+							[&](const BlockStore::MovedChunksArray& MovedArray, uint64_t FreedDiskSpace) {
+								std::vector<CasDiskIndexEntry> MovedEntries;
+								RwLock::ExclusiveLockScope	   _(m_CasContainerStrategy.m_LocationMapLock);
+								for (const std::pair<size_t, BlockStoreLocation>& Moved : MovedArray)
 								{
-									BlockStoreDiskLocation&	  Location	  = m_CasContainerStrategy.m_Locations[It->second];
-									const BlockStoreLocation& OldLocation = BlockCompactState.GetLocation(ChunkIndex);
-									if (Location.Get(m_CasContainerStrategy.m_PayloadAlignment) != OldLocation)
+									size_t		  ChunkIndex = Moved.first;
+									const IoHash& Key		 = BlockCompactStateKeys[ChunkIndex];
+
+									if (auto It = m_CasContainerStrategy.m_LocationMap.find(Key);
+										It != m_CasContainerStrategy.m_LocationMap.end())
 									{
-										// Someone has moved our chunk so lets just skip the new location we were provided, it will be GC:d
-										// at a later time
-										continue;
+										BlockStoreDiskLocation&	  Location	  = m_CasContainerStrategy.m_Locations[It->second];
+										const BlockStoreLocation& OldLocation = BlockCompactState.GetLocation(ChunkIndex);
+										if (Location.Get(m_CasContainerStrategy.m_PayloadAlignment) != OldLocation)
+										{
+											// Someone has moved our chunk so lets just skip the new location we were provided, it will be
+											// GC:d at a later time
+											continue;
+										}
+										const BlockStoreLocation& NewLocation = Moved.second;
+
+										Location = BlockStoreDiskLocation(NewLocation, m_CasContainerStrategy.m_PayloadAlignment);
+										MovedEntries.push_back(CasDiskIndexEntry{.Key = Key, .Location = Location});
 									}
-									const BlockStoreLocation& NewLocation = Moved.second;
-
-									Location = BlockStoreDiskLocation(NewLocation, m_CasContainerStrategy.m_PayloadAlignment);
-									MovedEntries.push_back(CasDiskIndexEntry{.Key = Key, .Location = Location});
 								}
-							}
-							m_CasContainerStrategy.m_CasLog.Append(MovedEntries);
-							Stats.RemovedDisk += FreedDiskSpace;
-						},
-						ClaimDiskReserveCallback);
-				}
-				else
-				{
-					if (Ctx.Settings.Verbose)
+								m_CasContainerStrategy.m_CasLog.Append(MovedEntries);
+								Stats.RemovedDisk += FreedDiskSpace;
+							},
+							ClaimDiskReserveCallback);
+					}
+					else
 					{
-						ZEN_INFO("GCV2: compactcas [COMPACT] '{}': skipped compacting of {} eligible blocks",
-								 m_CasContainerStrategy.m_RootDirectory / m_CasContainerStrategy.m_ContainerBaseName,
-								 BlocksToCompact.size());
+						if (Ctx.Settings.Verbose)
+						{
+							ZEN_INFO("GCV2: compactcas [COMPACT] '{}': skipped compacting of {} eligible blocks",
+									 m_CasContainerStrategy.m_RootDirectory / m_CasContainerStrategy.m_ContainerBaseName,
+									 BlocksToCompact.size());
+						}
 					}
 				}
 			}