mirror of
https://github.com/kvcache-ai/Mooncake.git
synced 2026-11-05 01:17:55 +00:00
[GH-ISSUE #4700] [RFC]: Rethinking the Master Global State Lock: Lifecycle Isolation and Finer-Grained Synchronization #1197
Labels
No labels
ascend
auto-closed
bug
bug
CI/Build
Common
documentation
enhancement
feature request
good first issue
help wanted
high priority
MetaX
nightly-failure
pull-request
PyTorch Backend
question
RFC
Roadmap
stale
Store
Tests
Transfer Engine
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
kvcache-ai/Mooncake#1197
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @Icedcoco on GitHub (Oct 15, 2026).
Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/4700
Changes proposed
1. Proposal and review goals
PR #4406 removed the legacy Master snapshot path, retained the OpLog-backed snapshot, and renamed
snapshot_mutex_tomaster_state_mutex_. The old snapshot requirement for a global Master freeze is gone, but many guard sites remain. Some of those sites now protect unrelated lifecycle or metadata interactions that accumulated around the original barrier.The direction proposed here is to give recovery, memory-segment remount, and LOCAL_DISK registration lifecycle explicit protocols; then gradually remove the global shared lock from ordinary RPCs and background scans; and finally decide whether the
master_state_mutex_member can be removed entirely.Here, “finer grained” means reducing the conflict domain. Unmounting one client's disk or remounting one segment should not automatically block every object operation in the Master. It does not primarily mean reducing the memory occupied by a mutex object or shortening one critical section in isolation.
The current preliminary assessment is deliberately open to correction:
std::shared_mutexmust remain permanently.ReMountSegment()retaining metadata pointers after releasing shard locks. The next hardest issues are internal concurrency during restore and ordering between LOCAL_DISK deregistration and replica admission.The first questions for reviewers are:
Answers to these questions determine whether the eventual design is a small protocol split or a substantially broader state-publication change. Please also point out if this RFC overstates the protection provided by the current global lock, or if it misses callers that bypass it.
2. Background
2.1 From a snapshot freeze lock to a general state barrier
The old
snapshot_mutex_was introduced with Master metadata persistence and recovery. The basic arrangement was shared locking for ordinary state access and exclusive locking for snapshot-like operations that needed to pause protocol participants. The historical entry point is described in [the Master persistence and recovery commit][history-persistence].Over time, other workflows began relying on the same barrier:
Therefore, keeping the lock after #4406 does not by itself mean that legacy snapshots were incompletely removed. It means that the original snapshot barrier accumulated responsibilities beyond snapshots. The next cleanup should separate those responsibilities instead of assuming they have one common lifetime.
2.2 Relationship to the retained OpLog snapshot
The current batch OpLog snapshot captures state from the
HotStandbyServicemetadata store. It checks cursor consistency at an applied durable-prefix boundary, pauses the standby replication loop, copies metadata in chunks, and releases the capture. That protocol has its own synchronization and lifetime rules and does not acquire the Master's global state lock; see [the standby capture path][source-oplog-capture]. It answers how an applied log state becomes a coherent snapshot. This RFC addresses how the recovered state is accessed at runtime after it has been installed in Master.This RFC does not propose changing the OpLog snapshot format, replay order, durable prefix, or leadership fencing. Those persistence guarantees remain owned by the OpLog protocol.
The separation under discussion is runtime synchronization for Master in-memory state. The existence of an OpLog snapshot does not imply that the Master-wide lock is required. Conversely, removing the legacy snapshot path does not prove that every historical guard site is now unnecessary.
3. Current state: what the lock actually protects
3.1 Guard sites and existing local synchronization
At the audit baseline,
master_service.cppdirectly constructs a guard formaster_state_mutex_at 69 sites: 65 shared-lock sites and 4 exclusive-lock sites. This is a static guard-site count, not a function count, and excludes tests and benchmarks that access the lock through test peers. The complete classification appears in Section 8.Existing local synchronization includes:
client_mutex_ok_client_ClientLivenessRecordserving/retaining guardsSegmentAllocatorRegistrationLocalSsdManagerregistry, client-lifecycle, stats, and mailbox locksobject_operation_locks_entriesRelevant definitions are linked from [metadata and lock ordering][source-header], [client liveness][source-liveness], [allocator registration][source-registration], and [LocalSsdManager][source-ssd-header]. Reviewers should correct this inventory if any of these mechanisms has a broader or narrower contract than stated here.
3.2 Shared locking does not serialize ordinary requests
Put, Get, Remove, eviction, and many other paths take the shared lock, but they still run concurrently with one another. Their ordinary data races are mainly handled by shard locks and other local mechanisms.
The global lock's main extra role is to make these shared accesses mutually exclusive with four exclusive workflows. It does not give
RemoveAll(), regex scans, or cross-shard queries a globally consistent snapshot of all ordinary requests.This distinction matters:
3.3 Four exclusive entry points
ReMountSegment(), L841RestoreFromStandbyState(), L3748GracefulUnmountSegment(), L2930UnmountLocalDiskSegment(), L8855Source entry points: [remount][source-remount], [restore][source-restore], [graceful unmount][source-graceful], and [LOCAL_DISK unmount][source-local-disk].
4. Risks exposed by direct lock removal
4.1 Remount retains raw pointers after releasing shard locks
This is the clearest current memory-safety constraint.
ReMountSegment()walks shards and collectsReplica*andObjectMetadata*. It then releases the shard locks, imports allocator state, and later uses those pointers to callreplace_memory_buffer()and grant leases. The current exclusive global lock blocks other protocol-compliant readers and writers during that interval; see [the remount pointer lifetime][source-remount-pointers].If the shared guard is removed from Remove, eviction, or durable finalize without changing remount, one legal interleaving is:
Removing the shared guard from Get is not automatically safe either. Remount later mutates the buffer without reacquiring the metadata shard lock; a reader holding a shard read lock can still race with that replacement.
Working conclusion for discussion: every path that can read or mutate these objects must not be declared removable merely because it has an accessor. First either eliminate remount's unlocked raw-pointer interval or retain an equivalent exclusion protocol during migration.
The global lock is not a formal proof that every metadata access in the repository is covered. Bypassing helpers and callbacks still need auditing. The remount observation should not be broadened into a claim that the current implementation is otherwise race-free.
4.2 Read paths also access mutable state outside the shard
TryGetReadableReplicaDescriptor()readsinvalid_replica_endpoints_; restore replaces this set and remount erases entries from it. The set has no independent mutex at present; see [the readability check][source-readability].This affects more than
GetReplicaList*.ExistKeyImpl(),BatchExistKeyImpl(), andGetAllKeys()reach the same check throughHasReadableReplica(). Eviction, Copy/Move, and drain scans have similar visibility decisions.Removing their shared guards without changing ownership or synchronization would permit concurrent access to an
unordered_set, which is a C++ data race. A small lock for the set would solve container safety but would not solve the replica-buffer publication problem above.4.3 RPC gating is not internal-thread gating during restore
The production supervisor restores a candidate Master before registering or exposing its RPC service; see [the serving gate][source-supervisor]. This is a useful structure for eliminating a runtime global lock.
However,
MasterServicestarts eviction, client monitoring, task cleanup, offboarding, replica cleanup, job dispatch, and dynamic-replication workers during construction; see [the worker startup path][source-workers]. Restore itself may install metadata in chunks, rebuild quota state, replace auxiliary state, and perform durable repair.Removing only the exclusive restore guard could allow:
The fact that a worker is usually idle is not a synchronization contract. Tests, embedded callers, and future entry points may invoke restore directly.
The first design question should therefore be whether restore is supported only for an unpublished, not-yet-serving instance. If yes, startup ordering or a one-time activation barrier can replace per-request participation in a global RW lock.
4.4 LOCAL_DISK has a check-to-write admission window
NotifyOffloadSuccess()andAddReplicaForRetainedClient()currently perform the following while holding the shared state lock:UnmountLocalDiskSegment()removes the registry entry under exclusive locking, releases the global lock, and then performs an owner-targeted sweep. The exclusive removal waits for prior shared admissions to finish; subsequent admission checks fail. The sweep does not need to hold the global exclusive lock for its entire duration if that admission ordering is preserved.Without an equivalent protocol, this interleaving is possible:
LocalSsdManageralready has a per-record lifecycle lock, and deregistration waits for manager operations that have acquired the record. ButHasMountedLocalDiskSegment()callsGetUsage(), and that internal guard ends whenGetUsage()returns; it does not cover a later metadata write. See [the manager implementation][source-ssd-impl] and [replica admission][source-add-replica].ClientLivenessRecordguards protect client liveness. Explicit disk unmount does not currently acquire the same guard, and disk unmount is not equivalent to taking the entire client offline. Those lifetimes should not be conflated without an explicit contract.4.5 Same-UUID remount needs an explicit contract
The owner-targeted sweep avoids deleting objects registered by a different client while the sweep is in progress; an existing [interleaving test][source-ssd-test] covers that shape.
The source still permits a different question:
The existing test uses different UUIDs for the leaving and late clients, so it does not establish same-UUID safety. Normal FileStorage drain uses its own latch and lock to constrain heartbeat remounts; see [the client drain path][source-file-drain]. This should therefore be described as a Master API contract and validation question, not as a reproduced production failure.
Two possible contracts are:
4.6 Does graceful unmount really need global exclusivity?
We should not assume that removing this exclusive guard requires adding another global or segment lifecycle mutex.
The current function already holds
ScopedSegmentAccessacross ownership validation,PrepareGracefulUnmountSegment(), and schedulerSchedule(). The segment path removes the allocator from the allocatable set and callsSetAllocatable(false).SegmentAllocatorRegistration::Allocate()checks availability and liveness before and after allocation, and the allocator reference is accessed through atomic shared-pointer operations. See [segment state transition][source-segment] and [allocator registration][source-registration].That makes this a useful independent review item: can “close admission for new allocations” be the linearization point, allowing already-admitted requests to finish under the existing segment and registration state, without blocking unrelated metadata?
The following cases still need proof:
The scheduler already releases its internal mutex before invoking the callback; see [DeadlineScheduler][source-scheduler]. A redesign must preserve that property and avoid a new scheduler/segment/client lock cycle. The historical ABBA fix is a design constraint, not proof that the current code always deadlocks.
4.7 Scans, accounting, and OpLog ordering cannot be delegated to one lock
Cross-shard scans already permit ordinary requests to interleave. A finer-grained design should preserve or add “reacquire the shard, then revalidate object, replica ID, version, and state” checks instead of replacing every scan with a new global lock.
The following points need explicit review:
ClearInvalidHandles(), Put/Upsert, and BatchRemove stale? Mount takes the same shared lock today, so the old global lock does not make a one-time UUID snapshot complete for clients mounted later. A replica-bound liveness record may be more accurate than one copied UUID set, but compatibility with restored state must be checked.4.8 Removing this lock may still leave global waits
ReMountSegment()currently holdsclient_mutex_exclusively for the main recovery interval in addition tomaster_state_mutex_.FindClientRecord(), Put/Upsert, and other paths acquire the same client mutex. Removing onlymaster_state_mutex_can therefore leave unrelated clients waiting behind remount.RestoreFromStandbyState()also holds both locks, although startup-only restore can remove its steady-state cost through lifecycle isolation.This does not mean the entire client registry must be redesigned in this RFC. It means two benefits must be measured separately: removing per-request participation in the global state mutex, and reducing lifecycle operations' blocking domain. To achieve the second benefit, remount may need to acquire stable liveness/resource identities, release the registry lock, and revalidate at commit; see [record lookup][source-client-record]. That would open remount/offboarding interleavings that are currently serialized, so
ok_client_,client_host_id_,standby_allocator_keepalive_, andstandby_accounted_memory_bytes_need explicit ownership rules.5. Goals and boundaries
5.1 Goals
5.2 Explicitly out of scope for this RFC
6. Alternatives and trade-offs
The suggested discussion direction is B, with A as a conservative intermediate. This is not a commitment to introduce every proposed generation or state type. The community should first decide which invariants require them.
Renaming
master_state_mutex_torecovery_mutex_while continuing to make ordinary requests acquire a shared guard would preserve the global hot-path coupling under a new name.7. Preliminary design for discussion
7.1 Restore: turn concurrency into startup ordering where possible
The first proposal is an explicit Master lifecycle:
The absence of a
Serving -> Recoveringedge is a proposed constraint, not an established API guarantee.Possible rules:
This lets steady-state RPCs avoid a shared lock whose only purpose is protecting a phase that occurs before publication.
If online restore is required, use a separate protocol: close admission, drain requests and callbacks that have entered, then restore and publish. A single atomic
restoringflag is insufficient because a request can pass the check and remain in its critical interval. Waiting for drain while holding a callback-needed lock is also unsafe. Online restore should be a separately approved scope because it substantially expands the design.7.2 Remount: eliminate address escape before choosing segment granularity
Consider remount as three logical phases: prepare, rebind, and publish. Names and exact interfaces are intentionally left open for review.
Prepare: identify the segment registration being recovered; prevent new allocation and premature reads; under shard locks collect stable identities such as tenant/key, replica ID, descriptor, and versions instead of retaining
Replica*after unlock.Rebind: import allocations into the prepared allocator. Before changing metadata, reacquire the owning shard write lock, look up the object, and validate replica ID, old descriptor/registration, and version. If the object was deleted or changed, release the unused imported allocation or perform a bounded retry/failure; do not commit through an old pointer.
Publish: publish segment allocatable/readable state only after buffers, allocator binding, accounting, and readiness satisfy the selected invariant. Keep the old allocator alive until dependent buffers have moved or been safely released.
Questions that must be answered before implementation:
Adding a per-segment mutex alone does not answer these questions. Remove and eviction may not know all segments involved before entering their shard, and may not acquire the same new lock. If that mutex is expected to preserve object lifetime, every relevant path must participate.
A conservative transition can be evaluated: hold all relevant shard write locks in a fixed order across the rare remount pointer-use interval. If the shard set cannot be proven stable, temporarily holding all shards may remove the hot-path global mutex while still causing a broad pause. That is not segment-level isolation, and the order between multiple shards, client guards, and segment locks still requires proof.
If a per-segment transition state or generation is selected, a candidate state machine is
Recovering -> Ready -> Retiring. This RFC does not require those exact enum names. Reusing existing registration/segment state is preferable to maintaining two overlapping state machines.7.3 Endpoint readability: put synchronization with the owner of the state
For
invalid_replica_endpoints_, compare:The current preference is to make readiness part of segment/registration state eventually, while allowing the first step to use one of the smaller mechanisms. Publishing readiness must follow correct buffer and allocator binding. Protecting the set itself is not a complete remount protocol.
7.4 LOCAL_DISK: organize admission, deregistration, and sweep by registration
The invariant to preserve is:
Two routes are worth comparing.
D1: serialize disk lifecycle per client. Mount, admission, and unmount share a stable per-client disk gate. Unmount closes admission, waits for already-admitted operations, sweeps, and only then allows a new mount for that UUID.
It may be possible to reuse
ClientLivenessRecordguards, since Mount and Notify already use related liveness checks. That would bind disk cleanup to heartbeat, memory operations, and liveness transitions, however. The impact on liveness latency, record cleanup, and long sweeps must be measured; reusing an existing guard is not automatically free.A dedicated per-client gate needs stable ownership. Removing a registry record must not allow a same-UUID remount to create a different gate while the old sweep still runs. A tombstone/retiring entry or independent gate owner may be required.
D2: explicit registration handle or generation. Extend the existing
LocalSsdManager::ClientAccessidea so a business operation receives a handle that spans admission, metadata write, and required accounting. Unmount marks that registration unavailable, then waits for in-flight handles. If a new generation may be created concurrently, the old sweep matches registration identity/generation rather than UUID alone.Implementation questions include:
ClientAccessis currently private, and its lifecycle guard covers one manager method. Exposing a business handle requires a complete API rather than returningGetUsage()and assuming the guard survives.The first policy question is whether same-UUID parallel remount is needed at all. If it is forbidden, D1 is simpler. If it is allowed, D2 gives a more complete ownership model. Neither route should put unrelated clients' ordinary Get/Put operations behind a disk lifecycle gate.
7.5 Graceful unmount: start from existing segment synchronization
Treat this as an independent review item. First determine whether the current segment mutex, registration availability, and scheduler already express the required semantics.
A possible contract is: after allocation admission closes, new allocation fails or chooses another segment; operations admitted before the transition may finish under existing lifetime rules; physical unmount still follows the grace period and buffer validity. If the product requires all in-flight allocations to drain before the RPC returns, add an explicit in-flight protocol instead of relying on an incidental global lock.
Even if this entry point's exclusive global guard is removed, it must not change the same segment midway through remount allocator import and publication. A local transition check or protocol may be sufficient; unrelated metadata should not continue to pay for a global barrier.
7.6 Business and background paths must obey the same invariant
Ordinary metadata operations continue to use shard accessors. Object-local tasks, quota ledgers, and replica state changes remain under their owning shard or subsystem synchronization.
Eviction, offboarding, promotion, dynamic replication, drain jobs, and durable finalize must not become loopholes around the new protocol. Scans should retain stable identities and revalidate at commit; registration-bearing operations must also validate lifecycle identity.
Avoid hiding a new global gate inside
MetadataAccessorto “cover all callers automatically”. That would recouple all shards and may break nested lock ordering.8. Guard-site classification: what might be removed first
The table below covers all 69 direct guard sites in the audit baseline. The line number is the start of the guard declaration. The categories mean:
8.1 Four high-priority independent candidates
PollRemoveAll, L8933PromotionObjectHeartbeat, L10394FetchTasks, L13317MarkTaskToComplete, L13337These are D candidates, not approved deletions. If online restore later replaces these components, their service-admission contract must be revisited.
8.2 Other shared sites: remove by dependency
MountSegment: L712ClearInvalidHandles: L2501ProcessClientOffboardingJob: L2662TaskCleanupThreadFunc: L2851UnmountSegment,UnmountNoFSegment: L2869, 2975ExistKeyImpl,BatchExistKeyImpl,GetAllKeys: L3027, 3084, 3127BatchReplicaClear: L4382GetReplicaListByRegex: L4676GetReplicaList,GetReplicaListForAdmin: L4709, 4812BatchGetReplicaListand admin version: L4870, 5026PutStart,UpsertStart: L5475, 6163PutEnd,PutRevoke: L5588, 5892AddReplicaForRetainedClient: L5787CopyStart/End/Revoke: L6833, 7045, 7226MoveStart/End/Revoke: L7305, 7451, 7645Remove,RemoveByRegex: L7704, 7780RemoveAllpaths: L7878, 8023BatchRemove: L8164MountLocalDiskSegment: L8802OffloadObjectHeartbeat: L8896ReportSsdCapacity: L8949NotifyOffloadSuccessNACK: L9004NotifyOffloadSuccesssuccess branch: L9029RunPromotionCandidateRetry: L9493, 9569DynamicReplicationAdmissionThreadFunc,SubmitReplicaActionProposal: L9787, 10035PromotionAllocStart,NotifyPromotionSuccess/Failure: L10417, 10552, 10680EvictionThreadFunc: L10847BatchEvict,NoFBatchEvict: L11242, 11888, 12427ClientMonitorFunc,TryUnmountNoFSegmentByHeartbeat: L12651, 12762CreateCopyTask: L13075, 13134;CreateMoveTask: L13194, 13263ScheduleDrainJobTasks,MaybeCompleteDrainJob: L13630, 13743The table contains 61 shared sites here plus the four D sites above, for 65 shared sites. The four exclusive sites are listed in Section 3.3; they are protocol entry points rather than general deletion candidates.
The previous broad claim “this only touches metadata, so its shared guard can be deleted” is not sufficient. In particular, existence queries,
GetAllKeys, durable finalize,BatchReplicaClear, and the entireTaskCleanupThreadFuncneed prerequisites and must not be placed on an unconditional deletion list.9. Lock ordering and publication constraints
The current header documents ordering among the client lock, tenant-policy mutex, master-state lock, metadata shard, quota recompute, segment/quota table, and soft-pin index. A new design also involves liveness transitions, LOCAL_DISK lifecycle gates, mailbox state, and scheduler callbacks. Renaming one line in the old table is not enough.
At minimum, an implementation review should require:
These are review constraints, not a proven deadlock-free lock graph. A complete dependency graph should be added after the community selects a design and its call paths.
10. Validation plan: correctness before performance claims
10.1 Targeted interleavings
Use barriers and test hooks to force specific interleavings instead of relying only on random stress:
TSAN can supplement race detection, and ASAN plus targeted stress can expose pointer-lifetime bugs. Neither replaces assertions for stale registrations, duplicate allocations, or accounting order. Tests should cover HA and non-HA, different memory allocators, and the affected NoF/CXL branches.
10.2 Performance hypotheses and measurements
The expected benefits are lower cache-line contention from shared lock state and less tail latency when a lifecycle operation runs. Both are hypotheses. Contention may move to client, shard, allocation, or quota locks.
Compare the current baseline, an intermediate design, and the final design on:
The repository already has a [batch-eviction benchmark][source-benchmark] that observes global exclusive-lock waits through a test peer. After lock removal, translate such implementation-specific metrics into user-visible latency and isolation metrics rather than deleting regression coverage simply because the old mutex no longer exists.
11. Suggested discussion and delivery boundaries
The following decomposition is offered for community selection, not as an approved implementation plan:
Steps 3 and 4 can be designed independently. Step 6 depends on Steps 4 and 5; L-category paths also depend on Step 3. Shared guards should not be removed in bulk before those prerequisites are resolved. Every surviving exclusive workflow must remain correct during the mixed old/new transition.
Each implementation PR should include its own correctness argument and validation results. Avoid one large PR that simultaneously changes startup APIs, registration identity, remount, wire protocol, and all guard sites. If the community decides to retain a transitional barrier only for necessary cases, document its remaining participants and its exit criteria.
12. Questions for reviewers and maintainers
The central request of this RFC is a critical review. Please comment with concrete counterexamples, source references, or simpler alternatives, especially where the proposed design is over-engineered or where the current analysis is too optimistic.
The desired outcome of this discussion is not immediate approval. It is a shared list of invariants, rejected assumptions, and an implementation boundary that developers can defend before coding starts.
This draft is based on the specified source commit, repository history, and existing tests. It does not modify runtime code and does not claim that a lock-removal experiment or performance test has been run. The interleavings are code-derived hypotheses and are labelled as validation targets. This RFC is not a proof that the current implementation or any proposal is free of all concurrency bugs.
This RFC draft was prepared with AI assistance. Any feedback, error corrections, or architectural suggestions are warmly welcome, and I will incorporate them with an open mind so we can work together toward clean, efficient, and robust code.
Before submitting a new issue...
@github-actions[bot] commented on GitHub (Oct 15, 2026):
Thanks for opening this issue, @Icedcoco!
102317026A maintainer will triage this when possible. To help us respond faster, please include:
Useful links: Documentation · Contributing guide
@kunkunblueberry commented on GitHub (Oct 17, 2026):
Thanks for this RFC — the per-site D/M/L/C classification and the explicit interleaving sketches make this far easier to review than a plain "remove the global lock" proposal would be. Especially useful: stating the no-cycles-out-of-Serving constraint as a proposal rather than an assumed invariant, and isolating the 4 D-class candidates from the harder remount/restore work.
I would like to participate on parts of this rather than take over the whole decomposition. Concretely I can help with:
One observation from my initial read: Question 5 (same-UUID remount semantics) seems to block the most downstream design choices, including the remount publish phase — if a later remount is linearized on the bucket lock over group_id, the publish phase can stay much simpler than the epoch-merge version. Happy to help write that decision down once it is settled.
@he-yufeng commented on GitHub (Oct 18, 2026):
Baseline checks out against main: #4406 did land (10-13) and the renamed
master_state_mutex_is now confined to a single file, master_service.cpp, with 71 acquisition sites. Two measurements from that file that should sharpen the decomposition:The lock is overwhelmingly read-side: 55 of the 71 sites take
shared_lock(including thesnap_lock/snapshot_locknaming leftovers from the old path), and only 4 takeunique_lock. So the contention story is writer-waits-on-readers, and the decomposition risk lives in which of those 55 read sites actually depend on the same invariant as one of the 4 writers. A guard-site map (each acquisition, the fields it reads, and the writer it pairs with) would make the RFC's "unrelated lifecycle or metadata interactions" claim auditable instead of asserted; without it, splitting the lock is unfalsifiable either way.Since the surface is one file, a cheaper first step might be mechanical: annotate each site with the invariant it participates in (a short comment convention is enough), then let the protocol decomposition fall out of the clusters. That produces the migration plan as a byproduct, and it is reviewable in one PR without any behavior change.
Happy to review the guard-site map when it exists.
@kunkunblueberry commented on GitHub (Oct 18, 2026):
Two things: a correction to the baseline count, and the guard-site map you asked for.
Count
The file has 59 acquisition sites — 55
shared_lock, 4unique_lock. 71 is the number of lines that mention the symbol: 59 sites + 10 wrapped declarations (themaster_state_mutex_);continuation line) + 2 comments. Reproduce with:So the writer:reader ratio is the 4:55 you measured, but the denominator is 59, not 71.
The four exclusive sites are all segment-registry lifecycle
ReMountSegmentsegment_manager_, standby accounting, invalid replica endpointsGracefulUnmountSegmentsegment_manager_(drain scheduling)RestoreFromStandbyStateUnmountLocalDiskSegmentlocal_ssd_manager_registrationGuard-site map
Static analysis of each acquisition plus the member fields its enclosing function touches.
xs= iterates allkNumShardsunder the lock (needs a consistent cross-shard view, not exclusion).dynamic_replication_callbackReMountSegmentFinalizeRemovedReplicasAfterDurableFinalizeMetadataEraseAfterDurableFinalizeExpiredProcessingReplicasAfterDurablediscarded_replicas_FinalizeExpiredReplicationTaskAfterDurableClearInvalidHandlesProcessClientOffboardingJobTaskCleanupThreadFuncUnmountSegmentGracefulUnmountSegmentUnmountNoFSegmentExistKeyImplBatchExistKeyImplGetAllKeysxsRestoreFromStandbyStateBatchReplicaClearGetReplicaListByRegexxsGetReplicaListGetReplicaListForAdminBatchGetReplicaListBatchGetReplicaListForAdminPutEndAddReplicaForRetainedClientPutRevokeCopyStartCopyEndCopyRevokeMoveStartMoveEndMoveRevokeRemoveRemoveByRegexxsRemoveAllRemoveAll(overload)BatchRemoveUnmountLocalDiskSegmentOffloadObjectHeartbeatPollRemoveAllReportSsdCapacityRunPromotionCandidateRetryRunPromotionCandidateRetrySubmitReplicaActionProposalPromotionObjectHeartbeatPromotionAllocStartNotifyPromotionSuccessNotifyPromotionFailureEvictTenantMemoryForQuotaBatchEvictNoFBatchEvictTryUnmountNoFSegmentByHeartbeatCreateCopyTaskCreateCopyTask(2nd)CreateMoveTaskCreateMoveTask(2nd)FetchTasksMarkTaskToCompleteScheduleDrainJobTasksxsMaybeCompleteDrainJobxsWhat the map says about the decomposition
Every object-metadata mutation holds the global lock in shared mode.
PutEnd,PutRevoke,CopyEnd,MoveEnd,Remove*,BatchEvict,NotifyPromotionSuccessall writemetadata_shards_while sharingmaster_state_mutex_; exclusivity there comes fromMetadataAccessorRW/MetadataShardAccessorRW, i.e. lock-order level 4, not level 3. So for those 18 sites the global lock cannot be protecting writer-writer exclusion. What it does protect is (a) ordering against the four exclusive sites above, and (b) for thexssites, a consistent view across shards.That splits the 55 shared sites into two different dependencies, and they need different replacements:
PutEnd,Copy*,Move*, eviction, offload, promotion, drain). These need the barrier to survive in some form; the natural target is "registration/unmount publishes a new registry generation, readers pin the generation they resolved against".GetAllKeys,GetReplicaListByRegex,RemoveByRegex,ScheduleDrainJobTasks,MaybeCompleteDrainJob) and 4 more live in helpers invoked under the lock (TenantHasObjects,RebuildTenantQuotaUsageFromMetadata,ReRouteRestoredObjectsByKey,ClearCandidatesForReload). These need a snapshot, not exclusion; a shard-set epoch would let them go lock-free, and that is independent of the protocol split.Clusters that look removable first because they touch only their own subsystem: task registry (9 sites,
task_manager_only), promotion state (9 sites), local-SSD capacity (ReportSsdCapacity,PollRemoveAll,OffloadObjectHeartbeat). None of these resolve segment names, so they do not need the segment-lifecycle barrier.The comment above
UnmountLocalDiskSegment(lines ~8938-8945) already states the exact hazard: a registration admitted against the old segment must not land in a shard the sweep has already passed. That is precisely the invariant to encode once, rather than rediscover per site — and it is the one place where the exclusive lock is load-bearing for correctness, not just convenience.If the annotation convention is the preferred format, I can turn the table into one short tag per site (e.g.
// [inv:segment-registry],// [inv:object-metadata],// [inv:cross-shard]) as a separate no-behavior-change PR, which also makes the cluster counts greppable.@he-yufeng commented on GitHub (Oct 18, 2026):
The map is exactly what the decomposition question needed, and the count correction checks out: 59 acquisition sites (55 shared / 4 unique), so the ratio I measured stands with the right denominator.
The load-bearing finding in your table is not the ratio but point 1: every object-metadata mutation holds the global lock in shared mode, so writer-writer exclusion for those sites has never come from
master_state_mutex_— it comes from the level-4 accessors. That means sharding or removing the global lock does not touch write correctness on the hot path at all; the only real dependencies are the two you isolated, ordering against the four segment-lifecycle sites and consistent views for the cross-shard scans. Two different replacements for two different dependencies is the right shape, and the registry-generation pin for segment lifecycle matches how theUnmountLocalDiskSegmentcomment already states the hazard.On sequencing, the removable-first clusters (task registry, promotion state, local-SSD capacity) are the right first extractions precisely because they do not resolve segment names. One thing the annotation pass should double-check before each extraction: the "also touches" column. Sites like
PutEnd(oplog, offload),BatchRemove(client registry), or the drain pair sit in a primary cluster but reach into a second subsystem, and any ordering those secondary touches get for free today has to be made explicit or consciously dropped. The map already records the touches, so this is a verification step, not new analysis.On the annotation PR: yes, worth doing as a separate no-behavior-change PR, and I would tag two axes rather than one — the invariant domain (
[inv:task-registry]etc.) and what the site actually needs from the global lock ([needs:lifecycle-order],[needs:cross-shard-view],[needs:none]). The first axis tells you which cluster a site belongs to; the second tells you whether extraction is safe, and they do not correlate perfectly (the promotion and task sites are[needs:none], but a metadata writer likeCopyStartthat resolves segment handles is[needs:lifecycle-order]). With both tags greppable, each extraction PR becomes a mechanical review of one tag set.@Icedcoco commented on GitHub (Oct 20, 2026):
Thanks for offering to help with this. My current direction is to reduce the scope of
master_state_mutex_incrementally rather than assume that the global lock needs to remain around every path. Each path should use synchronization that matches the state and invariants it actually protects, while preserving the existing externally visible behavior.Since you offered to prototype restore lifecycle and admission, please feel free to start with that as a focused PR. Please include the lock sites changed and targeted tests for concurrent restore admission and failure or cleanup paths.
We can review the same-UUID remount behavior and the
group_idlocking idea against the concrete implementation and tests, then decide whether the approach should be adjusted or extended. I’ll be happy to work through that review with you. @kunkunblueberry@kunkunblueberry commented on GitHub (Oct 20, 2026):
Two replies, one per thread.
@Icedcoco — the restore lifecycle prototype already exists as a PR: #4736. It is the focused slice you described, and it is rebased onto current
main(95750b2c). On your three asks:master_state_mutex_stays exactly where it is. I made that split on purpose so the state machine can be reviewed without arguing about lock removal at the same time. The lock-site work starts with the annotation PR below, then one extraction.RestoreLifecycleOverlappingRestoreIsRefused. It holds the restore core's exclusive lock through the test peer, soRecoveringis published while the core cannot progress; the overlapping call must then be refused at admission. This is a deterministic window, not a timing race (an earlier version of the test was timing-dependent and a reviewer correctly rejected it).RestoreLifecycleFailedRestoreReachesFailed,RestoreLifecycleRetryAfterFailedRestoreReachesReady(replacement-restore retry fromFailed),RestoreLifecycleRestoreAfterServingIsRefused, andRestoreLifecycleWorkersPausedOnlyInRestoreWindowsfor the worker gate.Locally on the rebased tree: lifecycle 9/9,
master_service_test71/71,master_service_ha_test127/130 with the same three environment-onlyBatchEvict*failures documented in the PR.@he-yufeng — the two-axis tagging is what I built, and you are right about
[needs:none]; I had it wrong. Correction first, because it changes the annotation: I originally tagged the task and promotion sites[needs:lifecycle-order]on the premise that the restore clears the task table viaTaskManagerSerializer::Reset(). I checked before writing this:TaskManagerSerializerhas no callers outsidetask_manager.cpp, and the restore core touches neithertask_manager_norpromotion_*/local_ssd_manager_(it only readssegment_manager_.HasSegmentByEndpoint). So those sites share no state with the restore and belong at[needs:none]. I will switch them in the annotation PR.One behaviour question I do want on record before extracting them, since @Icedcoco asked to preserve externally visible behaviour: today those calls block for the whole restore window because the core holds
master_state_mutex_exclusively. After extraction they would run during recovery — data-safe, but a client could fetch or complete a task while the master is rebuilding metadata. If that is acceptable,[needs:none]is exactly right and the extraction is a pure removal; if not, the extraction needs the lifecycle gate. I would rather state the choice in the extraction PR than encode it as a third tag — unless you would prefer the tag.On the "also touches" verification: for the task pair I already checked it — the secondary touch is the client liveness record (
FindClientRecord+TryAcquireServing/RetainingGuard), and that is pinned by the guard, not by the global lock. I will do the same check per cluster before each extraction, starting withPutEnd's oplog append and offload hand-off, since those are the ones where a secondary ordering dependency is most plausible.On remount and the
group_ididea: happy to bring that as the next design discussion, but I would sequence it after the first extraction so it lands against working code. One input that constrains it: RFC #2979 makes SegmentView the authority for segment information and states "No SegmentId reuse. New mounts receive a new client-generated UUID", so the remount publish phase should reuse SegmentView rather than introduce a parallel generation — and thegroup_idlinearization idea should be checked against that before we commit to it.@kunkunblueberry commented on GitHub (Oct 27, 2026):
The annotation PR is up: #4994 (comments only, 94 added lines, no synchronization change). Two things in it correct what I said earlier in this thread, so they belong here rather than only in the PR body.
1. The task cluster does not come out as one clean extraction. I had put the task and promotion sites at
[needs:none]on the grounds that the restore core touches neithertask_manager_norpromotion_*. That is true, but it answers a different question:[needs:lifecycle-order]is about ordering against the segment lifecycle. Checking the code,CreateCopyTaskandCreateMoveTaskdo resolve segment names —ExistsSegmentName(target)for every target, andGetClientIdBySegmentName(selected_source_segment, ...)on submission. So all fourCreate*Tasksites are[needs:lifecycle-order], and onlyFetchTasks,MarkTaskToCompleteandTaskCleanupThreadFuncare[needs:none]. The first extraction from the task cluster is therefore 3 sites, not 9; the other 6 need the registry-generation pin first.2. The same check moved two more off
[needs:none].PromotionAllocStartallocates from the segment registry, and the drain pair splits:MaybeCompleteDrainJobresolves segments,ScheduleDrainJobTasksscans object metadata across keys.Current distribution over 63 acquisition sites (4 of them
unique_lock):On the first extraction, the
[needs:none]set is now explicit, so the remaining question is which cluster to take. I am leaning toward[inv:eviction](3 sites:EvictNamespaceMemory,BatchEvict,EvictManagedWeightMembersToCold) because after #4887 that path runs throughmemory_eviction_->evictor, which carries its own per-key synchronization — if that holds, the global lock there is a pure removal rather than a removal plus a new mechanism. I will verify that before opening anything, and I am happy to take a different cluster if you would rather start with the task or local-SSD sites.One sequencing note: #4801 and #4802 are already working on the remount path, so I am leaving that domain to them.