SDSTOR-22200#883
Open
shosseinimotlagh wants to merge 25 commits into
Open
Conversation
The class was named BlkAllocMetrics but is exclusively used by VarsizeBlkAllocator and its slab subsystem (SlabMetrics registers to it as parent). The name is now accurate and leaves room for a separate FixedBlkAllocMetrics class.
FixedBlkAllocMetrics: new metrics class with num_alloc, num_alloc_failure counters and a blk_alloc_memory_size gauge. Wired into FixedBlkAllocator constructor; gauge is set once at construction since MPMC queue capacity is fixed (total_blks x 12B per slot: 6B BlkId + 6B folly sequence field, confirmed via GDB on 461GB index device = 10.06GB). VarsizeBlkAllocMetrics: adds the same blk_alloc_memory_size gauge, updated once after FreeBlkCacheQueue construction via the new total_slab_capacity() helper (sum of all slab entry_capacity() x 12B per slot = 1.60GB for the production data blkstore configuration).
The cache evictor's get_size() callback was returning only m_cache_size (the
raw node data buffer), but each cached entry allocates additional heap memory:
actual_cost = m_cache_size + sizeof(CacheBufferType)
+ sizeof(homeds::MemVector)
+ sizeof(homeds::MemPiece)
This missing overhead caused two interconnected bugs:
1. cache_size metric underreported actual RSS by factor of:
(m_cache_size + overhead) / m_cache_size
2. Evictor's m_cur_size tracked only m_cache_size, so when m_cur_size
approached m_max_size, actual RSS was already at:
m_max_size × (m_cache_size + overhead) / m_cache_size
This prevented eviction from triggering before OOM.
The fix adds the overhead using compile-time sizeof() for each component,
ensuring both the cache_size gauge and eviction threshold (m_cur_size vs
m_max_size) accurately reflect true memory consumption.
For typical 512B btree nodes: overhead ≈ 320B → multiplier ≈ 1.625×
For larger 4KB nodes: same absolute overhead → multiplier ≈ 1.078×
The formula scales correctly across different node sizes without hardcoding.
Add verify_node_fowarding() using std::queue for iterative tree traversal. Benefits over recursive verify_node_recursive(): - Reduces lock contention: ancestor nodes unlocked while processing descendants - Enables cache eviction during traversal: ref_count drops to 1 immediately per node - Lower peak stack depth for deep trees This does NOT solve cross-volume cache accumulation during recovery. The actual OOM fix is force_evict() after each volume completes (separate commit). Add recursive parameter (default false) to verify_tree() propagated through: - btree.hpp: verify_tree(update_debug_bm, recursive) - mapping.hpp/cpp: verify_tree delegation - volume.hpp/cpp: verify_tree delegation Dispatcher verify_node() routes to verify_node_recursive() when recursive=true, verify_node_fowarding() when recursive=false (default).
Add release_cached_tree() to evict cached btree nodes when safe: - Buffer not locked (try_lock succeeds) - Buffer not dirty (no pending writeback) - ref_count permits eviction (only cache/wb_cache hold refs) Implementation: - btree.hpp: EvictionStats struct + release_cached_tree() + release_cached_nodes_recursive() - ssd_btree.hpp: free_node_from_cache() with isSyncCall=true to avoid null deref on wb_req->bcp - mapping.hpp: delegation wrapper - volume.hpp: delegation wrapper Safety: Cache::erase (cache_only=true) correctly handles wb_cache refs - buffer removed from hash_set/eviction list while wb_cache retains ref for checkpoint flush. Use case: Call after verify_tree() during recovery to release per-volume cache before processing next volume, preventing cross-volume memory accumulation.
…lation In vol_recovery_start_phase2(), call release_cached_tree() after recovery_start_phase2() finishes for each volume. This proactively evicts clean cached btree nodes, providing: 1. Limits peak memory: max(single volume) instead of sum(all volumes) Without this fix, cache accumulates across volumes during sequential recovery. 2. Frees cache space for dirty buffers: Clean nodes from verify_tree() consume cache quota but provide zero value after recovery. Evicting them makes room for dirty buffers that need cache space for writeback/checkpoint. 3. Defensive against future bugs: Reduces OOM risk from any dirty buffer accumulation by not wasting cache on stale clean buffers. Critical ordering per volume: 1. verify_tree() - loads nodes into cache for verification 2. recovery_start_phase2() - io_replay needs nodes in cache 3. release_cached_tree() - safe to evict after io_replay completes
Add release_cache_after_recovery boolean flag to GeneralConfig in homeblks_config.fbs with default value true. This allows runtime control of cache release behavior during volume recovery phase 2. When enabled (default), cached btree nodes are safely evicted after each volume's recovery to limit peak memory usage and free cache space for dirty buffers. The flag is marked as hotswap, allowing dynamic configuration updates without restart.
Add recovery_cache_release_latency gauge to HomeBlksMetrics and m_cache_release_ms field to HomeBlksRecoveryStats to track time spent releasing cached btree nodes during volume recovery phase 2. The metric accumulates time across all volumes and logs per-volume cache release duration. This enables monitoring the performance overhead of cache eviction and helps identify if the cache release operation becomes a bottleneck during recovery.
Run ./apply-clang-format.sh to format all C++ source files according to src/.clang-format style configuration. This ensures consistent code style across the codebase.
szmyd
reviewed
May 8, 2026
Replace compile-time sizeof(CacheBufferType) with runtime MallocExtension_GetAllocatedSize
to accurately measure the full derived class size (BtreeNode), not just the base class.
Root cause: sizeof(CacheBufferType) returns base CacheBuffer<BlkId> size (176B), missing
the WriteBackCacheBuffer intermediate class and BtreeNode derived class members. The
missing 32 bytes come from BtreeNode::m_common_header (transient_hdr_t), which contains
folly::SharedMutexReadPriority and other transient state.
Inheritance chain:
CacheBuffer<BlkId> (176B base)
→ WriteBackCacheBuffer (adds writeback state)
→ BtreeNode (adds transient_hdr_t with lock, +32B total = 208B)
GDB-validated overhead breakdown:
- BtreeNode: 208 bytes (MallocExtension_GetAllocatedSize confirmed)
- MemVector: 80 bytes
- MemPiece[1]: 32 bytes (18 actual, tcmalloc rounds to 32)
- Total overhead: 208 + 80 + 32 = 320 bytes
- Per-node total: 512B data + 320B overhead = 832 bytes
This fix ensures cache size tracking matches actual RSS consumption measured in production.
- Add align_up helper and compile-time offset calculations for transient_hdr_t - Add static_asserts to validate struct layout matches expected offsets - Add transient_hdr_t::size() static method - Add transient_hdr_expected_size constexpr for cache overhead calculation - Update cache.h k_derived_class_overhead to handle DEBUG (40B) vs RELEASE (32B) - Add k_mempiece_tcmalloc_size constant (32B) for tcmalloc size class rounding The transient_hdr_t struct size differs between builds: - RELEASE: 32 bytes (upgraders + lock + is_leaf) - DEBUG: 40 bytes (adds int is_lock field) This ensures cache overhead calculation (320B total) remains accurate across builds.
…alculation - Add include of btree_node.h to cache.h to access transient_hdr_expected_size constant - Replace hardcoded 32 with homeds::btree::transient_hdr_expected_size in k_derived_class_overhead - Ensures cache.h uses the validated compile-time constant instead of duplicating the value - Maintains single source of truth for transient_hdr_t size calculation
Static assertions were failing because hardcoded offset values didn't match actual struct layout. Use compile-time expressions with align_up() to compute correct offsets based on actual type sizes/alignments. This ensures validation succeeds regardless of platform-specific padding differences.
The align_up calculation was producing incorrect values (12/16 instead of 32). Using sizeof(transient_hdr_t) directly ensures we get the actual compiler-computed size which matches GDB validation (32 bytes for both debug and release builds).
The cache overhead calculation was missing the WriteBackCacheBuffer-specific members (bcp + req[2] array), causing it to undercount actual memory usage by 36 bytes per cached btree node. Before this fix: k_derived_class_overhead = 12 bytes (only transient_hdr, missing WriteBackCacheBuffer) Total overhead per node = 284 bytes After this fix: k_derived_class_overhead = k_writeback_buffer_overhead (36B) + transient_hdr (12B) = 48 bytes Total overhead per node = 320 bytes The 36-byte undercount per node caused: 1. cache_size metric to under-report actual RSS 2. Eviction threshold to trigger too late (after OOM risk) For recovery loading 10M nodes, this resulted in ~360MB of unaccounted memory, contributing to OOM during verify_tree operations. The fix properly accounts for all derived class members: - WriteBackCacheBuffer members (bcp + req[2]): 36 bytes * btree_cp_ptr bcp (shared_ptr): 16B * writeback_req_ptr req[2] (intrusive_ptr array): 16B * padding/alignment: 4B - BtreeNode::m_common_header (transient_hdr_t): 12 bytes (platform-specific) Validated via GDB: sizeof(CacheBuffer<BlkId>) = 160 sizeof(WriteBackCacheBuffer) = 196 WriteBackCacheBuffer overhead = 196 - 160 = 36 bytes Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
The transient_hdr size varies by platform based on folly::SharedMutexReadPriority implementation. Updated comments to reflect observed 12-byte size on x86_64 rather than hardcoding 32 bytes. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Replace hardcoded 36-byte constant with actual sizeof() calculation to ensure
compile-time verification and self-documentation.
Before:
static constexpr uint32_t k_writeback_buffer_overhead{36}; // magic number
After:
using MappingBtreeBufferType = homeds::btree::WriteBackCacheBuffer<...>;
using BaseBufferType = CacheBuffer<homestore::BlkId>;
static constexpr uint32_t k_writeback_buffer_overhead =
sizeof(MappingBtreeBufferType) - sizeof(BaseBufferType);
Benefits:
1. Compile-time proof: sizeof() is evaluated by the compiler, not manually calculated
2. Self-documenting: code shows exactly what types are being measured
3. Maintainable: if WriteBackCacheBuffer members change, the calculation auto-updates
4. Verifiable: compiler will error if types are not fully defined
The calculation captures:
- btree_cp_ptr bcp (shared_ptr): ~16 bytes
- writeback_req_ptr req[2] (intrusive_ptr array): ~16 bytes
- padding/alignment: ~4 bytes
Total: sizeof(WriteBackCacheBuffer) - sizeof(CacheBuffer<BlkId>) = 196 - 160 = 36 bytes
This works because in ssd_btree.hpp, writeBack_cache.hpp is included before cache.h,
ensuring WriteBackCacheBuffer is fully defined when this calculation occurs.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
The previous approach of using sizeof() directly in cache.h failed because WriteBackCacheBuffer is not defined when cache.h is parsed in many translation units (e.g., blkbuffer.cpp, log_dev.cpp, indx_mgr.cpp). Solution: Use a hardcoded constant (36 bytes) in cache.h with compile-time verification via static_assert in ssd_btree.hpp where all types are available. Benefits: 1. Compilation succeeds: cache.h doesn't require WriteBackCacheBuffer definition 2. Compile-time proof: static_assert verifies 36 == sizeof(WB) - sizeof(CB) 3. Self-documenting: Assertion message explains what to update if it fails 4. Type-safe: Cast to uint32_t prevents narrowing conversion warnings The static_assert in ssd_btree.hpp will fail at compile-time if: - WriteBackCacheBuffer members change - Platform ABI affects struct layout - The constant becomes stale Verified GDB measurements: sizeof(CacheBuffer<BlkId>) = 160 sizeof(WriteBackCacheBuffer<MappingKey, ...>) = 196 Overhead = 196 - 160 = 36 bytes Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
The static_assert in ssd_btree.hpp failed because MappingKey and MappingValue are not available when ssd_btree.hpp is included from many translation units (home_blks.cpp, volume.cpp, indx_mgr.cpp, etc.). Solution: Move the static_assert to mapping.cpp where: 1. All headers are included (mapping.hpp includes ssd_btree.hpp) 2. MappingKey and MappingValue are fully defined 3. WriteBackCacheBuffer template is instantiated 4. The assertion will fire during compilation of mapping.cpp This provides compile-time verification without breaking the build in other translation units that include ssd_btree.hpp before MappingKey is defined. The assertion validates: sizeof(WriteBackCacheBuffer<MappingKey, ...>) - sizeof(CacheBuffer<BlkId>) == 36 If WriteBackCacheBuffer members change, the build will fail with a clear message telling the developer to update cache.h. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
The static_assert failed, indicating the actual overhead is not 36 bytes on this build platform. Adding ShowSize diagnostic template to display the actual sizeof values. The compiler will show: - sizeof(WriteBackCacheBuffer<MappingKey, ...>) - sizeof(CacheBuffer<BlkId>) - The difference (actual overhead) Once we see the actual values, we'll: 1. Update k_writeback_buffer_overhead in cache.h 2. Re-enable the static_assert with the correct value Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Changed k_writeback_buffer_overhead from 36 to 24 bytes - Added static_assert in mapping.cpp for compile-time verification - Verified via sizeof(): WriteBackCacheBuffer(184) - CacheBuffer<BlkId>(160) = 24 - Confirmed in production: 308B total overhead (release), 328B (debug) - Removed verbose comments, kept minimal documentation - GDB verified: overhead constant across debug/release builds
Fix typo in function name for clarity
Function name was misleading - implementation uses iterative BFS with std::queue, not recursion. Simplified name to release_cached_nodes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prevent OOM during volume recovery by releasing cached btree nodes
Problem Statement
During multi-volume recovery, HomeStore experiences OOM (Out of Memory) crashes when the total cached btree nodes across all volumes exceed available memory. This occurs because:
Cross-volume cache accumulation: During
vol_recovery_start_phase2(), each volume'sverify_tree()loads all btree nodes into cache sequentially. Without eviction between volumes, total cache size = sum(all volumes).Clean node waste: After recovery completes, cached nodes from
verify_tree()provide zero value but continue consuming cache quota, leaving no room for dirty buffers that actually need writeback.Peak memory formula: With N volumes and per-node overhead of 832 bytes (512B-4096B buffer + MemVector + MemPiece + BtreeNode), peak memory during recovery reaches:
Total = N × (nodes_per_volume × (buffer_size + overhead))
Example: 13 volumes × 6.86M nodes × 832 bytes ≈ 77.3GB, exceeding typical 60GB limits.
Root Cause Analysis (GDB Session Evidence)
Extensive GDB investigation on production pod revealed:
dirty_buf_cnt=0, ruling out dirty buffer accumulationSolution
This PR implements a two-pronged approach to prevent OOM:
1. Iterative Btree Verification (Performance Improvement)
verify_node_fowarding()with iterative BFS traversal usingstd::queueBtreeNodePtrimmediately after processing each node2. Safe Cache Release After Recovery (OOM Prevention)
release_cached_tree()to safely evict btree node buffers after each volume's recoveryfree_node_from_cache()inssd_btree.hppto evict buffers with proper safety checks:try_locksucceeds)Cache::erase(cache_only=true)removes buffer from hash_set/eviction list while wb_cache retains reference for checkpoint flush3. Integration & Configuration
release_cached_tree()invol_recovery_start_phase2()AFTERrecovery_start_phase2()completes for each volumerelease_cache_after_recoveryconfig flag (default: true) with hotswap supportrecovery_cache_release_latencymetric to track performance overheadHomeBlksRecoveryStatsto log cache release timeTechnical Details
Cache Eviction Safety Checks: