refactor: v2 linked clone by shuo-wu · Pull Request #500 · longhorn/longhorn-spdk-engine

shuo-wu · 2026-04-30T03:31:14Z

Which issue(s) this PR fixes:

What this PR does / why we need it:

Special notes for your reviewer:

We introduced a special kind of empty snapshot lvol called entrypoint lvol for src replicas. Then, all clone replicas use an entrypoint lvol as their ActiveChain[0], which is like a backing image. After that, clone replicas should be both readable and writable like normal replicas.

Additional documentation or context

Notice Claude Opus 4.6 generates the codes based on the following prompt/plan. And the plan is mainly for simple reference only. Because there are several rounds of improvement after that.

First, refactor the existing linked-clone feature for repo `longhorn-spdk-engine` based on the descriptions below:
- The source replica:
  1. will prepare an entrypoint lvol (which is actually an empty snapshot lvol) from the requested snapshot
  2. record all entrypoints and all clone replicas pointing to it. Even for a src replica detected from an existing disk after restart, it should recover these info as well.
  3. avoid or ignore all entrypoints when checking or constructing its snapshot map 
  4. automatically clean up the entrypoint when there is no clone replica pointing to it
  5. forbid the replica deletion when cleanup is true, while there are still clone replicas pointing to it
  6. forbid the snapshot deletion when there is still a valid entrypoint pointing to it
  7. sync func should verify (and probably repair) all entrypoint lvols for the src replica, and linked-clone source information for clone replicas. 
- The clone replica itself:
  1. grows a new head lvol from the entrypoint lvol.
  2. record its entrypoint and the source replica. Even for the replica detected from an existing disk after restart, it should recover these info as well.
  3. sync func should verify linked-clone source information for clone replicas. And for case that the clone replica's root parent somehow becomes `the source snapshot directly`, the sync func should repair it.

mergify · 2026-05-14T22:49:30Z

This pull request is now in conflict. Could you fix it @shuo-wu? 🙏

derekbit · 2026-05-15T06:19:06Z

cc @davidcheng0922 @COLDTURNIP for review

davidcheng0922 · 2026-05-15T09:33:22Z

+func (r *Replica) recoverCloneReplicaInfo() {
+	r.isCloneReplica = false
+	r.cloneSourceReplicaName = ""
+	r.cloneEntrypointLvolName = ""
+
+	if len(r.ActiveChain) == 0 || r.ActiveChain[0] == nil || !IsCloneEntrypointLvol(r.ActiveChain[0].Name) {
+		return
+	}


I'm not sure if my understanding the restart-repair flow correctly:

syncCloneReplicaInfo() wants to repair the case where the clone root parent points to the source snapshot directly instead of the entrypoint.

After restart, constructActiveChainFromSnapshotLvolMap() leaves the chain base nil if the entrypoint lvol is already missing.

recoverCloneReplicaInfo() only recognizes a linked-clone replica when ActiveChain[0] is still the entrypoint.

That means isCloneReplica is no longer recovered in this case, and
syncCloneReplicaInfo() returns early before the repair logic runs.

If the entrypoint is already gone at restart time, will this state still be recoverable, or will the clone metadata be lost permanently?

I see. Do you mean the case below?

A clone replica somehow lost its ep lvol, and its root snap lvol was directly pointing to the src replica snap lvol

A restart happened, and the spdk server tried to reconstruct the replica cache

The new replica cache executed construct first. And in construct(), recoverCloneReplicaInfo considered this replica as non-clone replica, because the parant of the root snap is src replica snap rather than ep lvol then r.ActiveChain[0] is nil

The new replica cache then executed syncCloneReplicaInfo, which returned in the beginning due to the check if !r.isCloneReplica {, no chance to reach the repair logic at all!

This case is a little bit complicated, and your concern is fair.
I am not sure whether it's really worth repairing the clone replica for these cases. Do you think it's better to blindly mark all kinds of unexpected dst replicas as ERR?

cc @derekbit

Fair, maybe it is too little chance to happen, maybe not worth to fix now

We can create an improvement ticket for future improvement.

derekbit

One question: should we rename linked-clone to fast-clone?

derekbit

LGTM.

The PR longhorn/types is not submitted.

Introduce naming conventions for clone entrypoint lvols. Entrypoints sit between a source replica's snapshot and clone replica heads. Add constants ReplicaCloneEntrypointLvolInfix and ReplicaCloneEntrypointTmpHeadSuffix, plus helper functions for constructing, parsing, and identifying entrypoint lvol names. Longhorn 12552 Signed-off-by: Shuo Wu <shuo.wu@suse.com>

Add CloneEntrypointInfo struct to track entrypoint lvol metadata and its associated clone replicas. Extend the Replica struct with fields for both source replicas (cloneEntrypointMap) and clone replicas (isCloneReplica, cloneSourceReplicaName, cloneEntrypointLvolName). Longhorn 12552 Signed-off-by: Shuo Wu <shuo.wu@suse.com>

Filter out clone entrypoint lvols and their tmp heads from the snapshot lvol map construction so they do not interfere with the replica's snapshot chain. Update constructActiveChainFromSnapshotLvolMap comments to reflect that clone entrypoints are handled externally. Longhorn 12552 Signed-off-by: Shuo Wu <shuo.wu@suse.com>

…struct Add recoverCloneEntrypointInfo to scan the bdev lvol map for entrypoint lvols belonging to this replica, populating cloneEntrypointMap from SPDK state. Resolve clone replica names via GetCloneReplicaNameFromEntrypointChildLvol. Add recoverCloneReplicaInfo to detect linked clones by checking whether the chain root has a clone entrypoint as parent. Both are called during construct() for recovery after restart. Longhorn 12552 Signed-off-by: Shuo Wu <shuo.wu@suse.com>

Rewrite snapshotLinkedCloneSrcStart to create clone entrypoint lvols between the source snapshot and clone heads, enabling multiple linked clones per snapshot with clean lifecycle management. Add createCloneEntrypoint (3-step: clone, snapshot, delete leftover). Record clone source info in SnapshotCloneDstStart for linked clones. Longhorn 12552 Signed-off-by: Shuo Wu <shuo.wu@suse.com>

Forbid replica deletion (with cleanup) while any clone entrypoint still has active clone replicas pointing to it. The guard runs early in Delete() before any SPDK cleanup to avoid side effects on a rejected delete. Forbid snapshot deletion while a valid clone entrypoint with active clone replicas is attached to that snapshot. Longhorn 12552 Signed-off-by: Shuo Wu <shuo.wu@suse.com>

Add syncCloneEntrypoints called from Sync() to refresh entrypoint state from SPDK, auto-cleanup entrypoints with no children, discover new entrypoints, and remove orphaned tmp-head lvols. Longhorn 12552 Signed-off-by: Shuo Wu <shuo.wu@suse.com>

Add syncCloneReplicaInfo to verify the clone replica's chain root parent matches the expected entrypoint during Sync. Handles three cases: parent matches (no-op), entrypoint removed and parent points to the source snapshot (recreate), or lineage corrupted (mark error). repairCloneEntrypoint delegates to the source replica via gRPC (CloneSrcStart/SrcFinish) so the source registers the entrypoint in its cloneEntrypointMap. Longhorn 12552 Signed-off-by: Shuo Wu <shuo.wu@suse.com>

…stry handling The clone entrypoint serves the same role as a backing image: a read-only chain base. Place it at ActiveChain[0] to unify chain semantics. - Include entrypoint lvols in replicaLvolFilter - Extend constructActiveChainFromSnapshotLvolMap to load entrypoints - Guard BackingImage assignment for actual backing images only - Lock ActiveChain[0] directly in linkHeadWithParent - Update repairCloneEntrypoint to set ActiveChain[0] after reparenting Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Shuo Wu <shuo.wu@suse.com>

ActiveChain[0] (backing image or clone entrypoint) shared its Children map across replicas, causing cross-replica mutation when linkHeadWithParent or removeLvolFromSnapshotLvolMapWithoutLock modified the map. - Replace additive Children insertion with full map replacement in constructActiveChainFromSnapshotLvolMap and repairCloneEntrypoint, keeping only the current replica's root lvol - Copy BackingImage.Snapshot per-replica in prepareHead with a fresh Children map instead of sharing the pointer Longhorn 12552 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Shuo Wu <shuo.wu@suse.com>

derekbit · 2026-05-19T00:04:53Z

Hello @shuo-wu
longhorn/types PR is merged.

Add clone metadata fields to the Replica protobuf message and populate them in ServiceReplicaToProtoReplica. New proto fields: - is_clone_replica (bool) - clone_source_replica_name (string) - clone_entrypoint_lvol_name (string) - clone_entrypoint_map (map<string, int32>) Longhorn 12552 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Shuo Wu <shuo.wu@suse.com>

Updates generated gRPC stubs (spdkrpc, imrpc) to include the new DstReplicaSrcReplicaPairMap field in EngineSnapshotCloneRequest. Updates go.mod/go.sum to reference the new types pseudo-version. Longhorn 12552 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Shuo Wu <shuo.wu@suse.com>

…rcReplicaPairMap Extend SnapshotClone to clone all N dst replicas simultaneously for linked-clone volumes, instead of requiring exactly 1 replica and relying on N-1 DST rebuilds afterward. Key design points: - Add isCloning flag: set for the duration of SnapshotClone (linked-clone mode only). Guards ReplicaAdd, snapshot operations, backup, and expansion from running concurrently with the clone. ValidateAndUpdate is also skipped while cloning to avoid false divergence errors. - snapshotCloneLinkedN: dispatches one goroutine per dst replica under the engine lock. Each goroutine calls SnapshotCloneDst (BdevLvolSetParent — a near-instantaneous metadata op) on its paired src replica. Results are aggregated; the engine lock is held throughout via wg.Wait(). - DstReplicaSrcReplicaPairMap: the manager (which already ran the scheduler) passes an explicit dst→src replica name map so the engine does not need to auto-detect co-location. Dst replicas absent from the map are marked ModeERR so the manager can schedule a rebuild for them later; the clone continues for all paired replicas. The entire operation fails only if no dst replica can be paired at all. - Backward compatibility: an empty DstReplicaSrcReplicaPairMap falls back to the original 1-replica deep-copy/linked-clone path so old managers continue to work. Longhorn 12552 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Shuo Wu <shuo.wu@suse.com>

…cycle Integration tests against a real SPDK target: - Linked-clone volume read/write and independent snapshots - Source replica/snapshot deletion blocked by active clone entrypoints - Recovery of entrypoint and clone info after restart - syncCloneEntrypoints auto-cleanup of childless entrypoints and orphaned tmp-head lvols (including correct SPDK state simulation via BdevLvolSnapshot to create a read-only ep + undeleted tmp-head) - Repair of broken clone lineage via syncCloneReplicaInfo - N-replica simultaneous linked-clone via DstReplicaSrcReplicaPairMap Unit tests (no SPDK required): - syncWithBdevLvolMap dispatch to correct sync path - syncCloneReplicaInfo cases 1/2/3 covering entrypoint lookup logic - Full Sync() execution paths (clone replica detection, error paths) Also refactor Sync() to delegate to syncWithBdevLvolMap for testability; spdkClient=nil skips validateAndUpdate so unit tests can exercise sync logic without a live SPDK connection. Longhorn 12552 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Shuo Wu <shuo.wu@suse.com>

shuo-wu · 2026-05-19T03:31:58Z

I just added a summary in the longhorn-manager PR longhorn/longhorn-manager#4732. I think that is pretty helpful for the whole feature's review. If you are interested, PTAL!

derekbit · 2026-05-19T07:35:59Z

@davidcheng0922 Do you have any concern? If no, it looks good to merge.

davidcheng0922 · 2026-05-19T08:07:29Z

@davidcheng0922 Do you have any concern? If no, it looks good to merge.

No more, thank you. It looks pretty solid

derekbit · 2026-05-19T09:24:12Z

@mergify backport v1.12.x

mergify · 2026-05-19T09:24:37Z

backport v1.12.x

✅ Backports have been created

Details

#536 refactor: v2 linked clone (backport #500) has been created for branch v1.12.x

shuo-wu requested a review from a team April 30, 2026 03:31

shuo-wu self-assigned this Apr 30, 2026

shuo-wu force-pushed the refactor/linked-clone-entrypoint branch 3 times, most recently from ca3e8b0 to 4376b2c Compare May 1, 2026 01:06

shuo-wu force-pushed the refactor/linked-clone-entrypoint branch 2 times, most recently from 952c40d to 2a245b6 Compare May 14, 2026 22:48

shuo-wu force-pushed the refactor/linked-clone-entrypoint branch 8 times, most recently from 9a934db to e40549c Compare May 14, 2026 23:27

derekbit requested review from COLDTURNIP, c3y1huang and davidcheng0922 May 15, 2026 06:19

davidcheng0922 reviewed May 15, 2026

View reviewed changes

derekbit reviewed May 17, 2026

View reviewed changes

Comment thread pkg/spdk/engine.go

Comment thread pkg/spdk/engine.go Outdated

Comment thread pkg/spdk/engine.go

Comment thread pkg/spdk/engine.go

Comment thread pkg/spdk/engine.go Outdated

derekbit reviewed May 17, 2026

View reviewed changes

Comment thread pkg/spdk/replica.go

shuo-wu force-pushed the refactor/linked-clone-entrypoint branch from e40549c to 903ee43 Compare May 18, 2026 03:53

shuo-wu requested review from davidcheng0922 and derekbit May 18, 2026 04:15

derekbit reviewed May 18, 2026

View reviewed changes

shuo-wu added 3 commits May 18, 2026 14:33

shuo-wu and others added 7 commits May 18, 2026 14:33

shuo-wu force-pushed the refactor/linked-clone-entrypoint branch 4 times, most recently from 58ea611 to 2041fd2 Compare May 18, 2026 22:15

shuo-wu and others added 4 commits May 18, 2026 17:47

shuo-wu force-pushed the refactor/linked-clone-entrypoint branch from 2041fd2 to 4c87559 Compare May 19, 2026 00:48

shuo-wu marked this pull request as ready for review May 19, 2026 01:19

derekbit approved these changes May 19, 2026

View reviewed changes

derekbit merged commit 0325a68 into longhorn:master May 19, 2026
8 of 9 checks passed

derekbit mentioned this pull request May 22, 2026

Revert "test: add unit and integration tests for linked-clone entrypoint lifecycle" #538

Merged

Conversation

shuo-wu commented Apr 30, 2026

Which issue(s) this PR fixes:

What this PR does / why we need it:

Special notes for your reviewer:

Additional documentation or context

Uh oh!

mergify Bot commented May 14, 2026

Uh oh!

derekbit commented May 15, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

davidcheng0922 May 15, 2026

Choose a reason for hiding this comment

Uh oh!

shuo-wu May 18, 2026

Choose a reason for hiding this comment

Uh oh!

davidcheng0922 May 18, 2026

Choose a reason for hiding this comment

Uh oh!

derekbit May 18, 2026

Choose a reason for hiding this comment

Uh oh!

derekbit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

derekbit left a comment

Choose a reason for hiding this comment

Uh oh!

derekbit commented May 19, 2026

Uh oh!

shuo-wu commented May 19, 2026

Uh oh!

derekbit commented May 19, 2026

Uh oh!

davidcheng0922 commented May 19, 2026

Uh oh!

Uh oh!

derekbit commented May 19, 2026

Uh oh!

mergify Bot commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Backports have been created

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mergify Bot commented May 19, 2026 •

edited

Loading