virtio-blk: blockdev-mirroring for live storage migration#175
virtio-blk: blockdev-mirroring for live storage migration#175Coffeeri wants to merge 40 commits into
Conversation
ee744d1 to
403fb4e
Compare
phip1611
left a comment
There was a problem hiding this comment.
I just finished my first review round. Given the nature and size of the PR, it is still fairly coarse-grained at this point.
First of all, thank you for your patience and for working on this over the last couple of weeks. IIRC, you have already successfully tested this with our customer in their infrastructure.
At this early stage, my main concerns are:
- Is this the simplest and/or least invasive design?
- All the locking primitives make me a little anxious. We really need to avoid deadlocks.
I like that you already thought about cancellation and about preventing changes to the VM while a disk is being migrated. This is good!
That being said, I am not entirely sure how to continue here given the complexity. I think some LLM-assisted reviews could help identify the design space and check the implementation. Combined with a presentation to the team and an open discussion, this should help us move this forward.
Thanks!
| Ready, | ||
| /// Switch-over to the destination is in progress. | ||
| Completing, | ||
| /// All virtqueues switched to the destination. |
There was a problem hiding this comment.
this sounds a little weird to me. I thought that the new location is some host-visible NFS volume and in the end the file will be in /mnt/nfs_share2 instead of /mnt/nfs_share1. So I'd expect the backend (workers) just uses the new file location? I feel like virtqueues is the wrong termonilogy here or I am missing something
There was a problem hiding this comment.
Each virtqueue worker has its own AsyncIo backend. The Block device instructs each worker to switch to a different backend, in this case, one pointing to the destination image disk path. So the switch happens per queue.
Are you missing the word “workers” here?
| /// State shared by the copy worker and the per-queue mirroring | ||
| /// `AsyncIo` handles. | ||
| /// | ||
| /// Held in an `Arc` so all threads see the same phase. |
There was a problem hiding this comment.
"Held in an Arc so all threads see the same phase" is not necessary. That is basic Rust / basic programming knowledge. Above you are already mentioning it is shared
| /// Held in an `Arc` so all threads see the same phase. | ||
| pub struct MirrorState { | ||
| /// Current phase of the mirror. | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
please use expect() rather than allow() - allow() is removed from upstream
| phase: Mutex<MirrorPhase>, | ||
| } | ||
|
|
||
| #[allow(dead_code)] |
|
|
||
| #[allow(dead_code)] | ||
| impl MirrorState { | ||
| pub fn new() -> Arc<Self> { |
There was a problem hiding this comment.
nit: Normally, on the caller side you wrap it in an arc. I think this pattern in Rust is rather unusual
There was a problem hiding this comment.
Generally, yes. But, as I understand this is a pattern in the case where the type is indented to be soly shared, as it is the case with MirrorState.
There was a problem hiding this comment.
If MirrorState should never not be in an Arc, then the taking it out of the Arc should not be possible. Hiding the Arc within MirrorState with a MirrorStateInner would be an option.
| /// to the VM, or the destination cannot be created or opened. | ||
| pub fn mirror_disk(&self, device_id: &str, dest_path: &Path) -> DeviceManagerResult<()> { | ||
| for dev in &self.block_devices { | ||
| let mut disk = dev.lock().unwrap(); |
There was a problem hiding this comment.
effectively you are looking for one single disk. Using a loop doesn'r read nicely. how about self.block_devices.iter().find()?
| for dev in &self.block_devices { | ||
| let disk = dev.lock().unwrap(); | ||
|
|
||
| if disk.id() == device_id { |
| )); | ||
| } | ||
|
|
||
| drop(ack_tx); |
| // queues are dropped by common.reset() and rebuilt by activate() anyway. | ||
| if let Some(handle) = self.mirror_handle.take() { | ||
| handle.state.transition_to_phase(MirrorPhase::Cancelling); | ||
| drop(handle); |
There was a problem hiding this comment.
I think the handle is automatically dropped at the end of the block - this is not let Some(handle) =
There was a problem hiding this comment.
This is true, we make it explicitly here, to make the side effect of joining the CopyWorker thread visible here. But I agree, a comment would be sufficient
| state.transition_to_phase(MirrorPhase::Cancelling); | ||
| self.revert_queues_to_source()?; | ||
|
|
||
| drop(self.mirror_handle.take().unwrap()); |
There was a problem hiding this comment.
no different than let _ = self.mirror_handle.take().unwrap() but the latter is more a typical pattern
There was a problem hiding this comment.
They're semantically identical, but I'd keep drop(...) here: this drop has a real side effect (it joins the copy worker and releases the destination), and drop(...) names that intent while let _ = ... reads like discarding an unwanted return value.
There was a problem hiding this comment.
I don't think either option makes that intent clear. I'd much rather prefer a method that does the "side effects" explicitly. It's a lot easier to follow if the reader doesn't have to figure out what the Drop impl for the specific type does. As a bonus, you can actually document the method properly.
There was a problem hiding this comment.
I agree @arctic-alpaca, a clear teardown/join method here provides less ambiguity and clarifies the intent.
There was a problem hiding this comment.
no different than
let _ = self.mirror_handle.take().unwrap()
You kinda nerd-sniped me into looking into this because I vaguely remembered let _ = not necessarily dropping the right hand side. In this specific usage, yes it's mostly equivalent since a temporary value is returned from the right hand side, which is dropped if it's not assigned to anything. But just assigning let _ = foo doesn't automatically drop foo since foo isn't moved.
There was a problem hiding this comment.
I thought that let _ = drops immediately and let _something = keeps it until the end of the scope. Subtle difference?
There was a problem hiding this comment.
That is only true for temporary right hand sides because let _ = doesn't move the right hand side. If the right hand side is already bound elsewhere, it lives happily ever after: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=4c4e198f348e1ab0b4b510509bfedb3e
9b09630 to
f13781a
Compare
3a4a6e4 to
f64a263
Compare
e618b81 to
dddb76c
Compare
scholzp
left a comment
There was a problem hiding this comment.
Some remarks. I'm not done yet and need more time. So expect more feedback in the next days.
What I've seen so far looks quite good! :)
a48bf3d to
0a8e5a1
Compare
After starting the blockdev-mirror, the operator must be able to observe the current progress of the copy worker to decide whether to complete to the destination disk or to react to a failure. Introduce a Block::mirror_handle field and a dedicated MirrorStatus struct, to be used in the upcoming API endpoint. Block now owns the worker handle and explicitly joins it when dropped. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
Start exposes the lifecycle entrypoint that the upcoming REST endpoint will route to. The operator must provide an existing destination image with the same format and logical size as the source. The destination is opened with the source disk's backend options before mirroring starts. Status surfaces the shared mirror state through the device manager so operators can poll and observe progress. Adds three DeviceManagerError variants for the new failure modes and a BlockErrorKind::AlreadyExists for the create_disk pre-condition. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
Wire disk mirroring through to the HTTP API so a running VM can be told to start mirroring a disk onto a destination path. Add the /vm.disk-mirror-start endpoint and its request handler, the vm_disk_mirror_start dispatch on the VMM, and the Vm::mirror_disk wrapper that locks the device manager and maps its error into the vmm error type. A new DiskMirrorStart error covers the case where no VM owns the device manager. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
Expose the disk mirror status operation as a REST entrypoint so operators can poll progress and detect terminal phases. The endpoint returns the current phase, copied bytes, total bytes, and a failure reason when the mirror is in the failed phase. The PutHandler maps unknown disk id and inactive mirror to 404 so management layers can distinguish operator errors from server faults. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
The virtio-blk queue worker calls submit_batch_requests unconditionally on the disk image, ignoring batch_requests_enabled. The previous stub panicked, which crashed the VM as soon as a mirrored disk processed a batched read or write. Dispatch In and Out to the existing read_vectored and write_vectored methods, which already fan out to source and destination. Other request types do not reach this path under the current request pipeline. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
A virtqueue worker may still hold source and destination write-pairs when complete or cancel arrives. Swapping the wrapper out at that moment would orphan the pending completions, leaving the guest waiting on writes that will never be acked. Stage the incoming BlockQueueCommand in pending_block_queue_command and apply it only once the handler reports no in-flight requests. The submit path is gated for the duration of the drain, otherwise sustained guest writes would keep the in-flight count from ever reaching zero. After applying the command the worker sends the acknowledgement and processes the avail ring directly: while the command was pending, QUEUE_AVAIL_EVENT handling consumed the guest's kicks without submitting, and the guest will not kick again for descriptors it already queued. The same protocol is reused by the upcoming complete and cancel endpoints. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
After the copy worker reports the mirror ready, the operator needs to switch the device to the destination disk so the source can be detached. Completion is allowed from MirrorPhase `Ready`, or from `Completing` as a retry. A plain destination AsyncIo per virtqueue is pre-built before any command is sent, so a create_async_io failure leaves the device unchanged and the operator can retry. The state transitions to `Completing` before the first CompleteToDestination command goes out: from that point a queue may already write to the destination only, so the source stops being a safe fallback and cancel is no longer allowed. The drain protocol from the previous commit makes each worker finish its in-flight pairings before swapping. Failures of the command send or the acknowledgement wait panic instead of returning an error. A partial completion splits the queues between destination-only writers and source readers, which can serve stale reads to the guest. There is no revert that does not lose acknowledged writes, so we prefer the panic. After all acknowledgements the state becomes `Completed`, the copy worker is joined, and the control plane disk_image is replaced by the destination. Two BlockErrorKind variants cover the operator-visible preconditions. Device manager and REST plumbing follow. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
Operators trigger the complete stage of blockdev-mirroring through this entrypoint. The endpoint switches the device to the destination disk after the copy worker reports the mirror ready. The PutHandler maps device manager errors to HTTP status codes so management layers can distinguish operator errors (404 for unknown disk or no active mirror, 400 for not-yet-ready) from server faults. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
Cancel reverts every virtqueue worker to a plain AsyncIo on the source disk, transitions the mirror to Cancelling state and joins the copy worker, releasing the destination disk. The copy worker now exits before the next block once the phase is terminal instead of copying the remainder. Cancel is rejected once a completion was attempted: a queue may already write to the destination only, so reverting would lose acknowledged guest writes. A guest-initiated device reset cancels an active mirror before VirtioCommon::reset tears down the virtqueue workers, which must still be alive to acknowledge the revert. The REST plumbing for cancel comes in a follow-up commit. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
Resizing or snapshotting the disk, removing the device, shutting down, rebooting or deleting the VM and starting a live migration all invalidate an active mirror: the destination silently falls behind or the mirror state is lost, since it is not migratable. Reject these operations while a mirror is active so the operator has to complete or cancel first. DeviceManager::active_block_mirrors lists the active mirrors and backs the new Vm::any_active_block_mirrors check. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
The virtqueue worker only owns the active AsyncIo and cannot create a plain source backend after a mirror failure. `Block` retains the source disk and must coordinate the backend switch across all virtqueues. Until then, we keep the failed MirroringAsyncIo in source passthrough. Requests bypass the destination and continue on the existing source backend until `Block` cancels the mirror and swaps the queues back to the source. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
Block::cancel_mirror was only reachable through a guest-initiated device reset. Give the operator a way to abort a mirror and keep the guest on the source disk. Wire the call through the layers: DeviceManager::mirror_disk_cancel resolves the device and maps errors, Vm and the RequestHandler forward the call, and a new VmDiskMirrorCancel action backs the PUT /vm.disk-mirror-cancel endpoint. Unknown device ids and inactive mirrors map to 404, a cancel after an attempted completion maps to 400, and revert failures surface as internal errors. A failed cancel keeps the mirror handle and leaves the mirror in the Cancelling phase. Cancel accepts that phase as a retry, so the request can simply be retried. CancelToSource commands are idempotent per virtqueue. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
During block-dev mirroring, the CopyWorker reads each block from the source disk and writes it to the destination. It used to write zero-filled blocks as well, which allocates storage on the destination for regions that hold no data. When the destination supports sparse operations, we now check whether a block is all zeros. If it is, we call punch_hole instead of write_vectored. This keeps the destination as sparse as the source. The check currently looks at the full MIRROR_BLOCK_SIZE block, so only all-zero blocks become holes. A smaller granularity would also punch holes inside partly-zero blocks and save more space, at the cost of more compute per block. We leave this for a later change. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
Cover the range-lock and passthrough behaviour of MirroringAsyncIo with a mock AsyncIo, so the synchronization invariants are checked without real disk I/O. The tests cover: - overlapping mirror writes complete in order under the range lock - a copy-worker range hold blocks an overlapping guest write until it is released - reads pass through to the source only - a destination submit failure degrades the mirror to source passthrough A watchdog thread fails a test on timeout, so a locking regression surfaces as a failure rather than a hang. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
The synchronous mirrored write holds its range guard from acquisition through both the source and destination completions. Nothing else pins that lifetime, so a regression to dropping the guard early (`let _` instead of `let _guard`) would let an overlapping lock_range acquire while the write is still in flight, the exact race the range lock exists to prevent. Add guard_is_held_across_submit_and_wait and a GatedMockAsyncIo backend whose destination completion is withheld until released from another thread. The write parks in wait_for_completions holding its guard while the test asserts an overlapping lock_range blocks, then acquires only once the completion is released and the write drops the guard. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
A paused virtqueue worker is parked on its pause barrier and never reaches its epoll loop, so it cannot pick up a staged BlockQueueCommand. start_mirror, complete_mirror, and cancel_mirror staged the command anyway and blocked in wait_for_mirror_queue_command_acks until the ack timeout, then returned an error while the command lingered in the slot and was applied late once the VM resumed, leaving the mirror half-installed. complete_mirror additionally panicked on that timeout. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
Cloud Hypervisor holds the disk image lock process-wide, so re-opening the destination here would not trip the lock. Compare canonicalized paths instead and refuse a destination that already backs one of the VM's disks. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
A virtio-blk device activated by the guest, and the blockdev mirror rebuilding a backend on a guest-initiated reset, set up io_uring rings and eventfds on the activating vcpu thread, after that thread's seccomp filter is installed. Allow io_uring_setup and io_uring_register plus eventfd2 for the notifier, matching what the vmm thread already permits, so the backend is not killed with SIGSYS. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
submit_batch_requests serializes each entry and queues a completion per write, a submit failure mid-batch must still return Ok with one completion per entry. Otherwise the virtqueue worker, which records the batch as in-flight only on Ok, strands the completions already queued for earlier entries and dies with MissingEntryRequestList. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
Test the MirrorState phase state machine (allowed transitions, rejected ones, terminal Completed, and Failed keeping its first reason), the tracked-vs-barrier fsync split, write_zeroes mirroring, and that a degraded mirror passes every op through to the source alone. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
Document the operator workflow (start, status, complete, cancel, failure handling, unrecoverable errors, and conflicting operations) and the design behind it: the CopyWorker, the MirroringAsyncIo write fan-out, and the range lock. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
Refactor the redundant find-by-id scan over block_devices. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
Calculate advisory lock granularity for an explicitly supplied disk backend and path instead of always using Block::disk_image. This lets mirror destinations reuse the configured locking policy. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
Extract advisory lock acquisition into a helper that accepts a disk backend, path, requested mode, and current mode. Keep try_lock_image as the source-disk wrapper. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
Acquire a write lock on the destination before installing mirror queue backends. The destination uses the source disk locking granularity and retains the lock through its open file description. On completion, retain a write lock for writable disks and downgrade to a read lock for read-only disks. Report a lock conflict as HTTP 400. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
QEMU-compatible lock transitions acquire the marker bytes needed for the new state before checking for conflicts. On failure, they restore the previous state. After a successful downgrade, however, marker bytes used only by the previous state remain locked. This affects mirroring of read-only disks. The destination needs a write lock while Cloud Hypervisor copies data to it. When the mirror completes, the destination replaces the source and must return to the source disk's read-only lock state. Keeping the stale write marker prevents another reader from locking the image. Release marker bytes that are not needed by the new state after the conflict checks succeed. Add a test that downgrades a write lock to a read lock and verifies that another reader can acquire the image. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
Block mirroring copies a disk's logical contents to the destination. For QCOW2 images with backing files, this would flatten the image at the destination and discard the backing-chain structure. We add a disk backend validation hook and reject mirroring when either the source or destination QCOW2 image has a backing file. This validates the source before creating the destination. Hence, a rejected request does not leave a new file behind. Return HTTP 400 for unsupported images. Standalone QCOW2 images remain supported. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
The postponed lifecycle event describes a deferred reboot or shutdown, but its type and helpers are named after live migration. Rename them to describe the lifecycle operation itself and centralize event replay. This removes duplicate reboot and shutdown dispatch while preserving live-migration behavior. It enables reuse within blockdev-mirroring. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
Guest-initiated reboot and shutdown currently reset block devices and cancel active mirrors. This loses the mirror operation before the operator can complete or cancel it. Instead, this change keeps the mirror and copy worker alive when the VM stops. Record the guest lifecycle request and replay it after the operator resolves the last active mirror. Clear stale queue command senders during reset so offline mirror completion and cancellation do not wait for terminated workers. When replaying, skip a second Vm::shutdown() because the VM was already stopped while postponing the event. On-behalf-of: SAP leander.kohler@sap.com Signed-off-by: Leander Kohler <leander.kohler@cyberus-technology.de>
51262ae to
10dddd4
Compare
TLDR
Add blockdev-mirroring as live storage migration for the virtio-blk device. Without stopping the guest, the operator starts a mirror of a disk onto a destination file, waits for it to reach an in-sync state, then switches the VM to the destination. The whole flow runs over four REST endpoints.
Motivation
Some operators serve VM disk images from e.g. NFS shares mounted on the host. A share can fill up, and the operator then needs to migrate one disk image to another share with free space, without stopping the VM. Because the shares are mounted on the host, the VMM has filesystem-level access to both the source and the destination file.
We implement this in VMM rather than in a separate process. The mirror has to coordinate with VMM-level state: while it runs, the VMM must reject operations that would disturb the disk (see Design), and only the VMM can gate those.
A vhost-user-blk process could run the mirror itself and keep the swap transparent to the VMM, but it would still depend on the VMM for that gating, so we keep the mirror and the gating in one binary, which also keeps the libvirt integration simple. The vhost-user control interface is also too restricted to carry the start, progress, complete, and cancel commands and the mirror's state.
Design
CopyWorker: a background thread copies the source to the destination in 512 KiB blocks. All-zero blocks are punched as holes so sparse images stay sparse. Note: the granularity is somewhat arbitrarily chosen and needs to be discussed, especially regarding a finer granularity for hole punching.MirroringAsyncIo: each virtqueue worker'sAsyncIobackend is swapped for one that forwards reads to the source and every mutating op to both disks, and waits for both completions before acknowledging the write to the guest. A destination error degrades that queue to source-only and fails the mirror, so the guest never reads corrupted data.RangeLockManager: exclusive per-range locks shared between the copy worker and the guest writes, so the background copy and a concurrent guest write never race on the same range and the destination stays consistent.The mirror's phase is shared between the copy worker and the per-queue backends:
stateDiagram-v2 direction LR [*] --> running: start running --> ready: copy done ready --> completing: complete completing --> completed: switched completed --> [*] running --> failed: I/O error ready --> failed: I/O error running --> cancelling: cancel ready --> cancelling: cancel failed --> cancelling: cancel cancelling --> [*]A mirror can be cancelled at any time before completion, which reverts every virtqueue worker to the source and keeps the VM on the source disk. After completion it cannot be undone: by then some virtqueue workers may have switched to the destination and written there only, so there is no consistent state to roll back to without losing acknowledged writes.
A mirror that fails on a destination I/O error stays in
failed, keeps the VM on the source disk, and must be cancelled to clear it.While a mirror is active, the VMM rejects operations that would disturb the disk or the mirror's state:
vm.reboot), shutting down, or deleting the VMPausing the VM is allowed during an active mirror, but starting, completing, or cancelling a mirror is rejected while the device is paused (
MirrorDevicePaused). An orderly guest reboot or shutdown resets the virtio-blk device, which cancels the mirror and reverts the queues to the source disk. The guest then restarts or powers off, with the mirror dropped rather than completed.This approach is analogous to QEMU's
blockdev-mirrorwithsync=fullandcopy-mode=write-blocking: a full background copy plus synchronous propagation of every guest write to the destination. Unlike QEMU it keeps no dirty bitmap or convergence loop, because with every write already current on the destination a single linear pass reaches a consistent state and a deterministic in-sync point. The trade-off is added write latency, which is fine for storage rebalancing.API
All endpoints are
PUTon the VMM API socket.vm.disk-mirror-start- begin mirroring diskidontodestination_path.vm.disk-mirror-status- report the current phase and copy progress.vm.disk-mirror-complete- switch the VM to the destination (accepted only fromready).vm.disk-mirror-cancel- abort and keep the VM on the source.See
docs/disk_mirroring.mdfor the operator workflow, failure handling, and full design.Missing / TODO
will be introduced in follow up PRch-remotesubcommands for the disk-mirror endpoints (currently API-only).vm.disk-mirror-start(create a new disk vs. reuse an existing one), defaulting to requiring an existing destination.