Defrag changes

[dranikpg]

1. Lower max current defrags limit
2. Use DashTable instead of absl::flat_hash_map in small bins
3. Add background defrag iteration
4. Make it adaptive to raise the max cycles quota if it occured

[dranikpg]

augment review

[augmentcode]

🤖 Augment PR Summary

Summary: Refines tiered-storage defragmentation to reduce memory pressure and make cleanup self-healing.

Changes:

• Reduced the cap on concurrent small-bin defrag operations (100 → 50) to bound in-flight memory.
• Removed the delayed-defrag queue and its exported metric, shifting backlog discovery to scanning.
• Added RunDefragScan() to periodically traverse fragmented small bins and enqueue defrag reads.
• Made the scan adaptive by scaling its CPU time-slice based on how many bins were enqueued in the previous pass.
• Extended OpManager::NotifyDelete with a was_read signal and added HasReadPending() to avoid duplicate defrag reads.
• Switched SmallBins stashed-bin tracking from absl::flat_hash_map to DashTable (with a custom hash) and added fragmented-bin traversal support.
• Added a tiered-storage test covering background defrag scan behavior when upload budget transitions from negative to positive.

Technical Notes: The defrag scan runs as part of the offloading heartbeat and aims to keep overhead low when the small-bin table is already compacted.

_{🤖 Was this summary useful? React with 👍 or 👎}

[augmentcode]

Review completed. 4 suggestions posted.

Comment augment review to trigger a new review at any time.

[augmentcode]

src/server/tiered_storage.cc:735: RunDefragScan() only breaks when UploadBudget() < 0, so it still runs when the budget is exactly 0. That differs from NotifyDelete()'s UploadBudget() > 0 gate and may enqueue defrags when there's no upload headroom.

Severity: medium

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

[dranikpg]

+4kb but fixed

[augmentcode]

src/server/tiered_storage.cc:738: The scan stops only when pending_defrags > kMaxPendingDefrags, so at exactly the cap it can still enqueue more and exceed the intended concurrency bound.

Severity: medium

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

[dranikpg]

+1 but fixed

[augmentcode]

src/server/tiered_storage.cc:724: The scan callback enqueues via EnqueueForDefrag() without checking pending_defrags/UploadBudget(), so a single TraverseFragmented() step can schedule many defrags before the outer loop re-checks limits.

Severity: medium

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

[dranikpg]

is ok

[augmentcode]

src/server/tiering/small_bins.cc:106: DashTable::Insert() returns false on duplicate keys; ignoring the return can silently keep an old StashInfo while still incrementing stashed_entries_cnt. If the same segment.offset is ever reported twice (e.g. reuse/race), stats and fragmentation tracking can drift.

Severity: medium

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Adaptive background defrag scan and DashTable tracking for small bins

✨ Enhancement 🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

---

AI Description

• Add an adaptive background scan to discover fragmented small bins and enqueue defrags.
• Replace small-bin stashed tracking with DashTable to improve iteration and reduce overhead.
• Tighten pending defrag limits and remove delayed-queue metrics; add regression coverage.

Diagram

graph TD
  HB["Periodic heartbeat"] --> TS["TieredStorage"] --> OFF["RunOffloading"] --> SCAN["RunDefragScan"] --> SB["SmallBins"] --> OM["OpManager"] --> IO[("Disk pages")]
  OM --> ND["NotifyDelete(was_read)"] --> SCAN

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep a bounded delayed-defrag queue

• ➕ Deterministic: bins discovered at delete time without scanning
• ➕ Simple mental model (producer/consumer queue)
• ➖ Still needs careful bounding/backpressure to avoid unbounded growth
• ➖ Queue can get stale (bins may become non-fragmented before processing)
• ➖ Harder to tune for low CPU when system is healthy

2. Continue using absl::flat_hash_map + custom hash/traversal

• ➕ Avoids introducing/depending on DashTable behaviors and policies
• ➕ Well-known performance characteristics
• ➖ May be less memory/CPU efficient for the intended access patterns
• ➖ Does not address the cursor-based iteration integration as directly

3. Random sampling (probabilistic) instead of full traversal cursor

• ➕ Even lower CPU cost in steady state
• ➕ Avoids scanning overhead on large tables
• ➖ Non-deterministic convergence; fragmented bins may linger longer
• ➖ More difficult to reason about progress guarantees

Recommendation: The PR’s approach (cursor-based traversal + adaptive time budget) is the best fit for avoiding queue growth while keeping steady-state CPU low and ensuring eventual compaction. The added was_read signal and HasReadPending guard reduce redundant IO, and the DashTable hashing fix prevents pathological table growth under page-aligned keys.

Files changed (11) +216 / -46

Enhancement (4) +67 / -31

tiered_storage.ccAdd adaptive background defrag scan and tighten defrag IO limits +44/-26

Add adaptive background defrag scan and tighten defrag IO limits

• Lowers the maximum concurrent pending defrags and removes the delayed defrag queue. Introduces 'RunDefragScan()' with a cursor-based traversal and an adaptive CPU time budget that scales based on previous scan hits, using 'HasReadPending' to avoid duplicating reads.

src/server/tiered_storage.cc

tiered_storage.hAdd defrag scan state and switch cursors to DashCursor +8/-2

Add defrag scan state and switch cursors to DashCursor

• Adds 'RunDefragScan()' and tracking state ('defrag_cursor_', 'last_defrag_scan_hits_'). Updates the offloading cursor type to 'detail::DashCursor' to match DashTable-backed iteration.

src/server/tiered_storage.h

op_manager.ccTrack pending reads by segment and pass read-context to deletes +11/-2

Track pending reads by segment and pass read-context to deletes

• Adds 'HasReadPending()' to detect whether a specific pending read exists for a segment. Updates delete paths to call 'NotifyDelete(segment, was_read)' so defrag decisions can distinguish delete-after-read from delete-without-read.

src/server/tiering/op_manager.cc

op_manager.hExtend OpManager API with HasReadPending and NotifyDelete(was_read) +4/-1

Extend OpManager API with HasReadPending and NotifyDelete(was_read)

• Introduces 'HasReadPending(PendingId, DiskSegment)' and updates the 'NotifyDelete' virtual interface to include a 'was_read' boolean for more informed scheduling decisions.

src/server/tiering/op_manager.h

Bug fix (2) +36 / -2

server_family.ccRemove delayed defrag queue metric from INFO output +0/-1

Remove delayed defrag queue metric from INFO output

• Stops exporting 'tiered_delayed_defrag_queue_size' in formatted tiered metrics, aligning with removal of the delayed queue mechanism.

src/server/server_family.cc

small_bins.hIntroduce DashTable policy for stashed bins with robust hashing +36/-1

Introduce DashTable policy for stashed bins with robust hashing

• Defines a DashTable policy tuned for page-aligned offset keys and adds a Murmur3-style bit-mixing hash to prevent pathological bucket distribution. Replaces 'absl::flat_hash_map' with 'DashTable<size_t, StashInfo>' and exposes cursor-based traversal for fragmented bins.

src/server/tiering/small_bins.h

Refactor (3) +19 / -12

stats.ccUpdate TieredStats aggregation after removing delayed-queue field +1/-2

Update TieredStats aggregation after removing delayed-queue field

• Adjusts the TieredStats size assertion and removes aggregation of the deleted 'delayed_defrag_queue_size' member.

src/server/stats.cc

stats.hDrop delayed defrag queue size from TieredStats +0/-3

Drop delayed defrag queue size from TieredStats

• Removes the 'delayed_defrag_queue_size' stat field since defrag backlog is no longer tracked via a queue.

src/server/stats.h

small_bins.ccMigrate stashed bin tracking to DashTable and add fragmented traversal +18/-7

Migrate stashed bin tracking to DashTable and add fragmented traversal

• Replaces flat_hash_map operations with DashTable APIs ('InsertNew', 'Find', 'Erase') and adds 'TraverseFragmented' to iterate fragmented bins via a cursor. Simplifies DeleteBin handling by reading and erasing through DashTable iterators.

src/server/tiering/small_bins.cc

Tests (2) +94 / -1

tiered_storage_test.ccAdd regression test for background defrag scan behavior +93/-0

Add regression test for background defrag scan behavior

• Adds 'RunDefragScan' test to validate that fragmentation created under negative upload budget is later discovered and compacted when budget becomes positive, and that scanning becomes a no-op once compacted.

src/server/tiered_storage_test.cc

op_manager_test.ccUpdate OpManager test stub for NotifyDelete signature change +1/-1

Update OpManager test stub for NotifyDelete signature change

• Adjusts the test implementation to match the updated 'NotifyDelete(DiskSegment, bool)' interface.

src/server/tiering/op_manager_test.cc

[augmentcode]

Review completed. 1 suggestion posted.

Comment augment review to trigger a new review at any time.

[augmentcode]

src/server/tiered_storage_test.cc:512: The fixed SleepFor(100ms) assumes ~10 heartbeats at hz=100, which can be timing-sensitive in CI (different FLAGS_hz or slower scheduling) and make this test intermittently fail. Consider making the final “no further defrags” assertion purely condition-based rather than time-based.

Severity: medium

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

---

🟡 Remediation Recommended

1. InsertNew duplicates offsets ✓ Resolved 🐞 Bug ≡ Correctness

Description

SmallBins::ReportStashed now uses DashTable::InsertNew for stashed_bins_, but InsertNew is a
force-insert path that does not enforce unique keys. If an offset is ever inserted twice, the table
can contain multiple StashInfo entries for the same page offset while the rest of SmallBins uses
Find/Erase assuming uniqueness, leading to incorrect fragmentation tracking and stale entries.

Code

src/server/tiering/small_bins.cc[105]

+  stashed_bins_.InsertNew(segment.offset, StashInfo{uint8_t(list.size()), bytes});

Evidence

SmallBins switched to InsertNew, which in DashTable is explicitly implemented via a force-insert
mode and ultimately calls Segment::InsertUniq that performs an insertion attempt without checking
for an existing equal key. Since SmallBins later uses Find(offset)/Erase(it) as if there is a
single entry per offset, duplicate keys would break those assumptions.

src/server/tiering/small_bins.cc[87-107]
src/core/dash.h[132-142]
src/core/dash.h[952-995]
src/core/dash_internal.h[1379-1442]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`SmallBins::ReportStashed()` inserts into `stashed_bins_` using `DashTable::InsertNew`, which force-inserts and can create duplicate keys. The rest of `SmallBins` treats `stashed_bins_` as a unique map keyed by page offset.
### Issue Context
With `absl::flat_hash_map`, `operator[]=` overwrote existing entries; with `DashTable`, `InsertNew` routes to a force-insert implementation that does not check for existing keys.
### Fix Focus Areas
- src/server/tiering/small_bins.cc[87-107]
- src/core/dash.h[132-142]
### Suggested fix
Replace `InsertNew(...)` with a unique insert that returns an `inserted` boolean and either:
1) `DCHECK(inserted)` (if duplicates are forbidden), or
2) update in-place when `inserted == false` (to preserve old overwrite semantics).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Defrag cap not enforced 🐞 Bug ☼ Reliability

Description

TieredStorage::RunDefragScan checks pending_defrags against kMaxPendingDefrags only before calling
TraverseFragmented, but the traversal callback can enqueue multiple defrags in a single call. This
allows pending_defrags to temporarily exceed kMaxPendingDefrags and weakens the intended bound on
concurrent defrag read buffers.

Code

src/server/tiered_storage.cc[R720-748]

+  unsigned hits = 0;
+  auto cb = [this, &hits](size_t offset) {
+    tiering::DiskSegment segment{offset, tiering::kPageSize};
+    if (!op_manager_->HasReadPending(kFragmentedBin, segment)) {
+      op_manager_->EnqueueForDefrag({offset, tiering::kPageSize});
+      ++hits;
+    }
+  };
+
+  // Scale the cpu time-slice by how much work the previous scan found: sleepy on a stale table,
+  // more aggressive while there's a backlog to clear.
+  const uint64_t time_budget_us = std::min(kBaseUs + last_defrag_scan_hits_ * kUsPerHit, kMaxUs);
+
+  uint64_t cycles = 0;
+  do {
+    if (UploadBudget() <= 0)
+      break;
+
+    if (op_manager_->stats_.pending_defrags >= kMaxPendingDefrags)
+      break;
+
+    defrag_cursor_ = bins_->TraverseFragmented(defrag_cursor_, cb);
+
+    // TODO: yield as background fiber to perform more work on idle
+    // Limit allowed cpu-timeslice
+    cycles = base::CycleClock::Now() - start_cycles;
+    if (base::CycleClock::ToUsec(cycles) >= time_budget_us)
+      break;
+  } while (defrag_cursor_);

Evidence

RunDefragScan only checks the cap before traversal, but the traversal mechanism explicitly allows
invoking the callback multiple times per call; each enqueue increments pending_defrags. Therefore a
single traversal step can push pending_defrags above the cap until completion callbacks run.

src/server/tiered_storage.cc[713-751]
src/server/tiered_storage.cc[440-447]
src/core/dash.h[335-343]
src/server/tiering/small_bins.cc[168-174]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`RunDefragScan()` can enqueue multiple defrags per `TraverseFragmented()` call, but only checks `pending_defrags >= kMaxPendingDefrags` once per outer loop iteration. Because `DashTable::Traverse` may invoke the callback multiple times per traversal step, the callback can enqueue past the limit.
### Issue Context
`ShardOpManager::EnqueueForDefrag()` increments `stats_.pending_defrags` per enqueue, so exceeding the cap defeats the purpose of the concurrency guard.
### Fix Focus Areas
- src/server/tiered_storage.cc[713-751]
- src/server/tiered_storage.cc[440-447]
- src/core/dash.h[335-343]
### Suggested fix
Add a cap check inside the traversal callback before enqueueing:
- If `op_manager_->stats_.pending_defrags >= kMaxPendingDefrags`, skip enqueueing (and optionally skip counting a hit).
- Optionally track a local `bool reached_cap` and avoid further work in the callback when set (even if traversal continues), so `hits` accurately reflects enqueues performed.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

---

Powered by Qodo

[romange]

I do not understand the comment. What does it mean?

[dranikpg]

It means NotiftyDelete can either be called after a read or just so arbitrarily when a value is deleted - in the first case we have access to the value that was deleted

[romange]

I saw this style several times so I want to comment here about naming and contracts.
was_read and the comment above explains how you got here (the caller context) and not what it does for the function. The contract is not clear. While the logic is sound I think a better style, imho - to describe the variables (and their meaning) in terms of a contract without bringing the caller context into it if possible. For example,

// Notify delete. Return true if the filled segment needs to be marked as free.
// page_in_memory is true when the delete follows a completed read, so the page's data is
// still in the pending-reads cache and defrag can reuse it without an extra disk read.
bool NotifyDelete(DiskSegment segment, bool page_in_memory) = 0;

if you are changing the name, please sync it with every NotifyDelete declaration/implementation

[dranikpg]

Done

[romange]

how are you sure the key is unique?
where in the flow before we check for it?

[romange]

nit: fixed -> bounded, but also revisit the comment if it still applies as is.

[romange]

nice! 👏🏼

[dranikpg]

actually that was an overkill

[dranikpg]

no, no overkill - dividing by kPageSize breaks the dashtable as it requires high bits to be set as well

[romange]

LGTM

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →