CP-312874 SMAPIv3 storage-layer engine for SXM v3 snapshot migration

[LunfanZhang]

SMAPIv1 destinations reproduce a VM's snapshot chains by: each copy lands on whichever destination VDI has the most similar content, which happens to be the copy of the snapshot
before it. SMAPIv3 backends report no similar content, so every VDI arrives as a full, unrelated copy and the structure is lost.

The fix is to let the caller name the base, and have the VM migration path work out what it should be.

• DATA.copy2 — DATA.copy with an explicit dest_base. SMAPIv1 keeps its own similar_content search and ignores it; SMAPIv3 clones the base, mirrors the source VDI into the clone over the NBD proxy, and freezes the result into a snapshot.

• snapshot_parent — each vdi_mirror records the VDI it directly follows, derived by lineage_parent_of from the VM snapshot tree. A branch the VM has reverted away from starts a chain of its own.

• Copy order — same-sized VDIs are now ordered by a depth-first walk of those edges rather than by snapshot time, so a VDI is always copied after its base. with_many already hands each VDI the results of the ones before it, so that accumulator is the source→destination mapping; miss parent just means a full copy.

Two layouts a SMAPIv3 destination cannot represent:

1. a VDI.snapshot that belongs to no VM snapshot,
2. a snapshot VDI whose active disk was deleted

both are refused with operation_not_allowed before anything is copied.

[changlei-li]

send_start now blocks synchronously for the whole tree. Have you estimated the time it takes is acceptable when the tree is deep and wide?

[LunfanZhang]

QEMU mirror has better performance, so when the tree is deep and wide, the new SMAPIv3 will much faster then SMAPIv1.

[psafont]

Were confounding removed from the comparison? Like using different hardware disks for both tests

[LunfanZhang]

Good question - Both sources are now local SSD on hardware-identical hosts. The two hosts are the same model throughout: AMD EPYC 9124 16-Core (32 CPU, 3.0 GHz), 137 GB RAM, XenServer 9.0.0 same build, and — directly on your point — the local SR on both hosts is the same disk, sda = PERC H755N Front, 1.5 TB, non-rotational, behind an identical Broadcom MegaRAID SAS39xx controller. The xfs (V3) and ext4 (V1) SRs are filesystems on the same disk model and controller, not different hardware tiers.

[cplaursen]

Can we create a separate mutex for the snapshot_mappings table, rather than reuse the access_table mutex? It seems to me that they are independent, and this creates unnecessary contention.

[LunfanZhang]

Good catch. update to use a new snapshot_mappings_m rather than reuse the mutex .

[minglumlu]

Generally, I prefer changing the way how to iterate individual VDIs to introducing the whole VDI tree when handling an active VDI.

[LunfanZhang]

Generally, I prefer changing the way how to iterate individual VDIs to introducing the whole VDI tree when handling an active VDI.

@minglumlu that is what the latest push does. There is no VDI tree any more: send_start handles a single
VDI, and the chain is reproduced by ordering the VDIs plus one parent pointer on each.

1. One parent pointer per VDI, not a tree

vdi_mirror gains

  ; snapshot_parent: [`VDI] API.Ref.t option
        (** The VDI this one directly follows in its disk's snapshot chain *)

filled by lineage_parent_of ~__context ~vm ~vdi, which walks VM.parent upwards from the VDI's owning VM to the
nearest ancestor that still has a disk in the same snapshot_of lineage. A branch the VM has reverted away from
therefore starts a chain of its own.

2. Ordering: depth-first, as the tiebreak of the existing size sort

position numbers all_vdis in DFS pre-order over those edges (roots = no parent, or a parent that is not in the
migration set); siblings go oldest-first, so the live disk comes last. Size stays the primary key:

  let compare_fun v1 v2 =
    match Int64.compare v1.size v2.size with
    | 0 -> compare (position v1) (position v2)
    | r -> r

For a VM with disks A and B, four snapshots each, A smaller than B:

  [S_A1  S_A2  S_A3  S_A4  A_leaf   S_B1  S_B2  S_B3  S_B4  B_leaf]

The invariant this buys: a VDI is always copied after the VDI it follows.

3. with_many hands each VDI what came before it — a one-line change

with_many already accumulated results and passed them to the final continuation; it just never gave them to the
individual elements:

  -        withfn x (fun y -> inner xs (y :: acc))
  +        withfn acc x (fun y -> inner xs (y :: acc))

That accumulator is done_map, a mirror_record list carrying mr_local_vdi_reference / mr_remote_vdi / mr_remote_sr.
vdi_copy_fun still handles exactly one VDI, so no with_vdi_tree / with_vdi_forest was needed.

4. Where the two meet

  (* The destination copy of the VDI this one follows, if it is already on
     the same destination SR. Copying onto it reproduces the chain. *)
  let dest_base =
    Option.bind vconf.snapshot_parent (fun parent ->
        List.find_opt
          (fun mr ->
            mr.mr_local_vdi_reference = parent && mr.mr_remote_sr = dest_sr
          )
          done_map
        |> Option.map (fun mr -> mr.mr_remote_vdi)
    )

Because of (2), the parent is already in done_map by the time a node is reached, so the copy lands on it and the
chain is rebuilt one node at a time. None just means a full copy — never wrong data.

5. Storage layer

The base is passed down via the new DATA.copy2: SMAPIv1 finds its own base through similar_content/nearest and
ignores dest_base; SMAPIv3 reports no similar content and relies on it.

Storage_smapiv3_migrate.get_snapshot_tree and the State.snapshot_mappings side table are both gone — no
tree is discovered, built or persisted anywhere.

[minglumlu]

Now should it be is_active_leaf for being more accurate?

[LunfanZhang]

I think snapshot_parent is accurate expression here.

[minglumlu]

It should be a bug in sorting VDIs when can't finding mirrored/copied parent on destination side. It should fail.

[LunfanZhang]

Agreed — Option.bind silently swallows the case where the parent cannot be found, which should abort the migration with an explicit error instead.
Changed Option.bind to Option.map, so None can now only mean "this VDI is the root of its chain"; a parent that has not been copied raises instead.

[minglumlu]

How about dest_sr_p and dest_vdi_p?

[LunfanZhang]

dest_p is pre-existing: it is declared for DATA.copy and shared by copy2. I would rather not rename.
For dest_base_p I would prefer to keep the name. dest_vdi_p is already taken by another place (e.g., https://github.com/xapi-project/xen-api/blob/master/ocaml/xapi-idl/storage/storage_interface.ml#L733) and means the opposite. dest_base_p means a base parent here which sounds reasonable.

[minglumlu]

This is not related with this change.
But I think it's worth adding a comment for the local parent so that the semantic of the copy would be to copy all delta changes between (parent_vdi, vdi]. It is not used so far because for SMAPIv1, the parent is retrieved (layer-violation) again in vhd-tool, and current SMAPIv3 mirror doesn't support it. When the parent is None, it means the underline storage implementation needs to retrieve all changes up to the hidden root VDI. Lacking of this actually introduces some limitations - we have to assume there are no hidden VDIs between the visible VDIs in storage implementation.
So adding it now can make it easier for future improvements.

[LunfanZhang]

Yes, copy2 pins the destination base but leaves the source-side delta base to the backend, which derives it from
the source chain itself and therefore assumes no hidden VDI in between.

I have written that down on copy2.

but I would rather not add the parameter in this PR. One that no backend impl is a trap — Some p reads as a promise and would be silently ignored. I would prefer to add it together with the backend change that can actually use it.

[minglumlu]

Hi @LunfanZhang
The overview looks good to me now. Thank you! One still standing major comment is on how to prepare the VDI list. I would suggest to present the VDI trees explicitly. It's general data structures for both SMAPIv1 and SMAPIv3. They are helpful for understanding and hence ensuring the correctness. The sorting algorithm can keep the same for v1 trees to ensure no regression. May I please have your thoughts?

[gthvn1]

Within the limits of my skills, as this code goes beyond them, I don't spot any errors. Congrats for the great work!

[LunfanZhang]

Hi @LunfanZhang The overview looks good to me now. Thank you! One still standing major comment is on how to prepare the VDI list. I would suggest to present the VDI trees explicitly. It's general data structures for both SMAPIv1 and SMAPIv3. They are helpful for understanding and hence ensuring the correctness. The sorting algorithm can keep the same for v1 trees to ensure no regression. May I please have your thoughts?

Thanks @minglumlu for your time to review this. I believe for the both SMAPIv1 and SMAPIv3 should apply the algorithm to DFS within the tree from top to down, and sort each trees with leaf size. I can keep the same for v1 trees to ensure no regression, but as I mentioned at : #7223 (comment), there is bug in current SMAPIv1 algorithm, so although it takes some risk, I think it`d better to change the SMAPIv1.

[minglumlu]

This means there is a parent which is not in all_vdis. SMAPIv1 will support it?

[LunfanZhang]

lineage_parent_of walks the VM.parent chain, which only contains this VM's own snapshots, and their disks are all in all_vdis.
A invalid VM.parent is already turned into None by the is_valid_ref check at the top of that function. So neither backend (v3 or v1) can reach this branch — it is defensive only.

Even there is hidden VDI, it does matter, I think SMAPIv1 do not care too much about the order as it check the parent directly with similar_content with each Copy

[minglumlu]

This would go through the whole all_vdis again.

[LunfanZhang]

see: #7223 (comment)

[minglumlu]

This is of O(N^2) complexity. It can be avoided.

[LunfanZhang]

see: #7223 (comment)

[minglumlu]

This is the original code. But I can't understand it. I would think the key for comparison is something like:

1. vconf.snapshot_of - for different trees
2. depth in the same tree
3. is_snapshot
4. age

[LunfanZhang]

The original logic is to transfer the small size VDI first, which then mirror the leaf first, thats why the tree is separate in SMAPIv1. From my perspective, dividing different tree with vconf.snapshot_of ` and compare the age for snapshot , and put the leaf at the end of snapshot set is enough. which is similar to a round of DFS. but DFS is more strict.

[minglumlu]

Then it can be changed to smaller tree (the size of the active leaf) first?
The approach of comparison should be either splitting between SMAPIv1 and SMAPIv3, or common for both. Such comparison by size does no meaning at all for SMAPIv3. It would make the whole sorting awkward.

[LunfanZhang]

Hi @minglumlu old order had an unstated precondition, that for all the snapshot VDI belong to a tree, it's virtual size is usually the same, we can assume the walk was consulted only between VDIs of identical virtual_size.

and I proposal new order, sort tree first but not VDIs, Which is your suggestion for more strict.
in general we get the root from all_vdis first, and walk each root then sort the tree list base on the tree_size:

List.filter is_root all_vdis
 |> List.map (fun root ->
        let tree = walk root in
        (tree_size tree, tree)
    )
 |> List.stable_sort (fun (s1, _) (s2, _) -> Int64.compare s1 s2)
 |> List.concat_map snd

is_root is "no snapshot_parent, or a parent is in all_vdi". and tree_size impl like tree_size is List.fold_left (fun acc v -> Int64.max acc v.size) 0L tree which filter the max chain node for comparing.

and for tree, first create a table to reflect the parent-children:

let children =
  List.fold_left
    (fun acc v ->
      match v.snapshot_parent with
      | Some parent when VdiSet.mem parent present ->
          VdiMap.update parent
            (fun siblings -> Some (v :: Option.value ~default:[] siblings))
            acc
      | _ ->
          acc
    )
    VdiMap.empty eldest_first
  |> VdiMap.map List.rev

and walk the tree:

let rec walk v =
  v
  :: (VdiMap.find_opt v.vdi children
     |> Option.value ~default:[]
     |> List.concat_map walk
     )
in

children is a persistent Map built by one fold and never touched again; walk returns its subtree rather than writing into an index, so no Hashtbl and the int ref.
Sibling order is settled once by eldest_first sorting on a key (not is_snapshot, snapshot_time)

• snapshots before the active disk,
• oldest first

For a example as we mentioned before, 4 snapshots 2 disk, now the final order is:

[S_A1  S_A2  S_A3  S_A4  A_leaf]  [S_B1  S_B2  S_B3  S_B4  B_leaf]
 \______ tree A, 10G __________/    \______ tree B, 20G __________/

and current time complexity is reduce from O(N²) to O(N log N).

[minglumlu]

When the active is on a SMAPIv3 SR, all its snapshots will be in SMAPIv3 SR?
I think it is true. If so, is_v3_vdi can be avoided on each snapshot.

[LunfanZhang]

I make some change, so the is_v3_vdi is only asked at the roots only.

is_v3_vdi now appears exactly twice in the function, both times as a List.filter at the top of a pipeline, and never inside hidden_below — one query per migrated disk instead of one per VDI in the tree. A tree rooted on a SMAPIv1 SR is dropped at the root and never walked, so the assert stays SMAPIv3-only.

[minglumlu]

Only active disks can have the hidden VDI snapshots? I think the snapshots VDI can have as well.

[LunfanZhang]

snapshot VDI can have hidden VDI as well. I confirmed it on a pool for both ext and gfs2. and now I turn the walk to the whole subtree:

let rec hidden_below origin =
      Db.VDI.get_snapshots ~__context ~self:origin
      |> List.concat_map (fun s ->
          if List.mem s snapshot_vdis then [] else (s, origin) :: hidden_below s
      )
    in
    let hidden =
      active_vdis @ snapshot_vdis
      |> List.sort_uniq compare
      |> List.filter is_v3_vdi
      |> List.concat_map hidden_below
    in

[minglumlu]

Same comment for the is_v3_vdi.

[LunfanZhang]

for the orphans snapshots case, is_v3_vdi only sees what survives it — on a healthy VM, no node would run the check:

    let origin_of s = Db.VDI.get_snapshot_of ~__context ~self:s in
    let orphans =
      snapshot_vdis
      |> List.filter (fun s -> not (List.mem (origin_of s) active_vdis))
      |> List.filter is_v3_vdi
    in

[minglumlu]

I think the checking is that a snapshot has an active leaf but

1. the active leaf is invalid; or
2. the active leaf is not in active leafs to be migrated.

The error message should split these two cases.

[LunfanZhang]

Agreed. That single filter finds both — a dangling ref can never equal a live VDI's ref — and List.partition only has to decide which message applies:

    let deleted, detached =
      List.partition
        (fun s -> not (Db.is_valid_ref __context (origin_of s)))
        orphans
    in

• deleted — snapshot_of no longer resolves; the active disk was destroyed and there is nothing at the destination to anchor the chain on. "…whose active disk has been deleted; delete these snapshots before migrating".
• detached — snapshot_of resolves, but that VDI is not part of this migration. "…whose active disk is not part of this migration", and the message names the disk.

[minglumlu]

Just a few comments for style.