[GH-ISSUE #389] [Performance] commit_chunks(): Strategy to reduce lock contention during RocksDB I/O #226

New issue

Closed

opened 2026-10-26 09:47:25 +00:00 by bsakaguchi · 2 comments

bsakaguchi commented

2026-10-26 09:47:25 +00:00

Owner

Originally created by @red1239109-cmd on GitHub (Jan 21, 2026).
Original GitHub issue: https://github.com/deepseek-ai/3FS/issues/389

Hello team,

While reviewing the critical path in the engine, I noticed a potential performance optimization opportunity in commit_chunks(), specifically regarding lock granularity.

Current Behavior
In commit_chunks():

chunk_ids are sorted (👍 excellent for deadlock avoidance).

All corresponding meta_cache entries are locked.

A RocksDB write_batch is constructed.

meta_store.write() is executed (Disk I/O).

Cache entries are updated.

This design ensures strong consistency and is easy to reason about.

Observation (The Bottleneck)
The locks are held throughout the entire duration of write_batch construction and, more critically, the meta_store.write() call.

If the underlying storage experiences latency spikes (e.g., compaction, fsync stalls, or high load), this prolongs the critical section. This could cause unrelated get() or update() calls on these chunks to block, increasing tail latency for the entire system.

Discussion: Potential Optimization
To minimize the critical section, we might consider a structure that separates the "preparation" from the "commit":

1.Snapshot (Lock -> Copy -> Unlock): Capture minimal state needed for validation.

2.Prepare (No Lock): Construct the RocksDB write_batch (CPU bound work).

3.Commit (Lock -> Validate -> Write -> Update): Re-acquire locks, verify state hasn't changed (CAS-like logic), and then perform the write.

Note: I understand this introduces complexity regarding atomicity between RocksDB and the in-memory cache. The current implementation prioritizes correctness, which is the right default.

I'm raising this primarily as a discussion point for future scalability, especially if we observe lock contention under heavy write workloads.

Thanks for the solid engineering!

Originally created by @red1239109-cmd on GitHub (Jan 21, 2026). Original GitHub issue: https://github.com/deepseek-ai/3FS/issues/389 Hello team, While reviewing the critical path in the engine, I noticed a potential performance optimization opportunity in commit_chunks(), specifically regarding lock granularity. Current Behavior In commit_chunks(): chunk_ids are sorted (👍 excellent for deadlock avoidance). All corresponding meta_cache entries are locked. A RocksDB write_batch is constructed. meta_store.write() is executed (Disk I/O). Cache entries are updated. This design ensures strong consistency and is easy to reason about. Observation (The Bottleneck) The locks are held throughout the entire duration of write_batch construction and, more critically, the meta_store.write() call. If the underlying storage experiences latency spikes (e.g., compaction, fsync stalls, or high load), this prolongs the critical section. This could cause unrelated get() or update() calls on these chunks to block, increasing tail latency for the entire system. Discussion: Potential Optimization To minimize the critical section, we might consider a structure that separates the "preparation" from the "commit": 1.Snapshot (Lock -> Copy -> Unlock): Capture minimal state needed for validation. 2.Prepare (No Lock): Construct the RocksDB write_batch (CPU bound work). 3.Commit (Lock -> Validate -> Write -> Update): Re-acquire locks, verify state hasn't changed (CAS-like logic), and then perform the write. Note: I understand this introduces complexity regarding atomicity between RocksDB and the in-memory cache. The current implementation prioritizes correctness, which is the right default. I'm raising this primarily as a discussion point for future scalability, especially if we observe lock contention under heavy write workloads. Thanks for the solid engineering!

bsakaguchi closed this issue

2026-10-26 09:47:25 +00:00

bsakaguchi commented

2026-10-26 09:47:27 +00:00

Author

Owner

@SF-Zhou commented on GitHub (Jan 21, 2026):

Thank you for your attention. We are aware of this situation—the current design is a deliberate trade-off. The most time-consuming part after acquiring the lock is the I/O operation in self.meta_store.write(write_batch, sync), while the CPU-intensive write batch construction is already fast enough. To maintain consistency between RocksDB and the in-memory cache, the lock during RocksDB updates cannot be removed, and the approach you proposed does not resolve this issue.

@SF-Zhou commented on GitHub (Jan 21, 2026): Thank you for your attention. We are aware of this situation—the current design is a deliberate trade-off. The most time-consuming part after acquiring the lock is the I/O operation in `self.meta_store.write(write_batch, sync)`, while the CPU-intensive write batch construction is already fast enough. To maintain consistency between RocksDB and the in-memory cache, the lock during RocksDB updates cannot be removed, and the approach you proposed does not resolve this issue.

bsakaguchi commented

2026-10-26 09:47:27 +00:00

Author

Owner

@red1239109-cmd commented on GitHub (Jan 21, 2026):

Thanks for the clarification — that makes sense. Appreciate the explanation.

@red1239109-cmd commented on GitHub (Jan 21, 2026): Thanks for the clarification — that makes sense. Appreciate the explanation.

bsakaguchi referenced this issue

2026-10-26 09:48:47 +00:00

[PR #226] [MERGED] Add ubuntu20.04 compile dockerfile #335