fix(fs_native): give lookups and loads their own workers - #107
Open
voipmonitor wants to merge 3 commits into
Open
voipmonitor wants to merge 3 commits into
voipmonitor wants to merge 3 commits into
Conversation
The native filesystem connector ran every operation in one FIFO served by
num_workers threads. With checkpoint_on_evict, the first L1 eviction
pass above the watermark asks to write every current checkpoint page it
scans (most of RAM, tens of GiB on a large RAM tier) as one store task
split over all workers. A restore that needed disk pages queued its
lookup (EXISTS) and load (GET) behind that whole burst, for as long as
the disk needed to write it. Cancelling the restore could not shorten
the wait, and the engine gave up on it after 30 s.
ConnectorBase already supports per-operation worker lanes; FSConnector
now accepts them (LMCacheFSClient per_op_workers). fs_native defaults to
2 lookup workers, num_workers load workers and max(2, num_workers // 4)
store workers, so reads never queue behind writes, and fewer concurrent
writes leave a saturated disk some room for them. Deletes keep the
shared pool. per_op_workers in the adapter config overrides the default;
{} restores the single shared pool.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2 tasks done
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.
Problem
Root cause of the storage stall behind ktsaou's crash (GLM-5.3-Flash TP2, 140 GiB RAM tier, disk tier with
on-evictwrites): a checkpoint restore that needed disk pages stayed pending for over 90 s. #106 makes that survivable; this PR removes the stall.fs_nativeran every operation (lookupEXISTS, loadGET, storeSET, delete) in one FIFO served bynum_workersthreads:FSConnectorpassed no lane config toConnectorBase. Withcheckpoint_on_evict, the first L1 eviction pass above the watermark asks to write every current checkpoint page it scans, and nothing is on disk yet, so that is all of them:select_chunk_coherent_victimskeeps scanning past pages that need a write until it finds enough evictable ones. They go to the adapter as one store task split intonum_workerstiles, which occupies every worker. A restore that needs disk pages queues itsEXISTSandGETtiles behind the whole burst. Cancelling the restore cannot shorten the wait: neither the prefetch controller nor the native queue can cancel a queued request.Fix
ConnectorBasealready supports per-operation worker lanes (Mooncake uses them).FSConnectorandLMCacheFSClientnow acceptper_op_workers, andfs_nativedefaults to:lookup: 2 workers (a lookup only stats files);retrieve:num_workers;store:max(2, num_workers // 4): fewer concurrent writes leave a saturated disk some room for loads, without writing more slowly in the measurement below;delete: the sharednum_workerspool.per_op_workersin the adapter config overrides the default;{}restores the single shared pool. Docs:docs/source/mp/l2_storage/fs_native.rst.Measurement (real LMCache components, CPU only)
StorageManagerwith a 16 GiB RAM tier,fs_native(8 workers, O_DIRECT) on the test server's md RAID5 of 4 NVMe partitions,checkpoint_on_evict, andCheckpointPayloadStore(the server side of a restore). Checkpoint A (64 × 4 MiB) is on disk only. The server then publishes checkpoints to 85 % of RAM: one eviction pass requests all 3,520 page writes (13.8 GiB) at once, and A is restored while they drain (its lease is cancelled after 30 s, like the worker does). Each row is one run.Before, a restore behind a write burst waits as long as the whole burst takes, which grows with the RAM tier (a 140 GiB tier can queue over 100 GiB). With lanes it no longer waits in the queue. On this RAID5, O_DIRECT reads still compete with parity writes on the device, which fewer concurrent stores reduce; other runs gave 2.1 s and 44.6 s with the default lanes and 1.25 s with 1 store worker (86 s burst). A normal NVMe disk should be far less affected by device contention.
Tests
tests/v1/storage_backend/test_fs_native_connector.py::test_reads_in_their_own_lanes_do_not_wait_behind_queued_writes: with one shared worker a lookup and a load submitted after a 512 MiB write complete after it; with read lanes they complete before it.tests/v1/distributed/test_l2_adapter_factory.py::TestFSNativeLanes: default lanes, explicit and emptyper_op_workers, invalid lanes rejected, and the factory forwards the lanes to the native client.lmcache_fsfrom this branch in the beta image:tests/v1/distributed,tests/v1/storage_backend/test_fs_native_connector.py,tests/v1/multiprocess: 3 failures, the same 3 as on the base (test_engine_passthroughs,test_event_ipc_handle_path,test_qstore: CUDA-only paths in a CPU container). Seventest_async_engine_driven_transfer_context.pyfailures in one run under concurrent disk load passed on rerun (22/22 alone; the multiprocess suite again showed only the 3 base failures).Notes for reviewers
.lil/changes/lmcache-107.json.If applicable:
🤖 Generated with Claude Code