diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index 2477f28dc..a038404b7 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -2547,6 +2547,17 @@ fcvm runs two microVM backends behind a pluggable `Hypervisor` trait (`src/hyper owner, so it would get a disk it cannot write or remove). `rm -rf` removes subvolumes, so teardown is unchanged, but btrfs reclaims a removed subvolume's space on its cleaner thread after the rmdir returns, where an unlinked reflink freed its space at once; `df` right after many clone teardowns can read high for a while. +- Each snapshot directory is a btrfs subvolume too, and a diff snapshot is staged as a subvolume snapshot of its + parent's directory, keeping only the parent's `memory.bin`, which the diff is then written into + (`stage_from_parent_subvolume`). A memory file written by in-place diffs fragments: one written by eight of them + held 11,824,233 extents, a reflink of it took 133 s and removing that copy 52 s, while the subvolume snapshot and + its delete each finished within a second (2026-10-27). The parent's directory was synced when its own snapshot + finished, so the flush a subvolume snapshot does first finds little. Snapshot directories are deleted with + `btrfs subvolume delete` (`remove_snapshot_dir`), which detaches them at once, where `rm -rf` would drop the memory + file's references one by one first. A parent that is not a subvolume (made before this, or on a store that cannot + make one) is reflinked as before. An unprivileged user may delete a subvolume only on a filesystem mounted with + `user_subvol_rm_allowed`; fcvm's own loopback store is mounted with it, and on a btrfs store mounted without it a + rootless delete falls back to removing the files one by one. - Initrd: `/mnt/fcvm-btrfs/initrd/fc-agent-{sha}.initrd` (injects fc-agent at boot) **Per-VM Disk Sizing (`--rootfs-size`):** diff --git a/src/commands/common.rs b/src/commands/common.rs index f34b42048..26cbff0a5 100644 --- a/src/commands/common.rs +++ b/src/commands/common.rs @@ -870,6 +870,29 @@ fn diff_bytes_written(snapshot: &str, measured: std::io::Result) -> u64 { } } +/// Why `file` cannot take an in-place diff, or None: it is not `memory_bytes` long +/// (Firecracker truncates a memory file of another length and writes only the dirty pages +/// into it), or its filesystem does not report which extents it shares, which is how the +/// snapshot finds the pages Firecracker wrote. +fn memory_file_refuses_in_place_diff( + file: &std::fs::File, + memory_bytes: u64, +) -> std::io::Result> { + let len = file.metadata()?.len(); + if len != memory_bytes { + return Ok(Some(format!( + "the parent's memory file is {len} bytes, the VM's memory {memory_bytes}" + ))); + } + Ok( + match crate::storage::extents::reports_shared_extents(file) { + Ok(true) => None, + Ok(false) => Some("the filesystem does not report shared extents".to_string()), + Err(e) => Some(format!("the filesystem cannot report extents: {e}")), + }, + ) +} + /// Clone the parent's memory file to `dst` for an in-place diff snapshot, with FICLONE. /// /// Returns false, leaving no `dst`, when the snapshot has to be written as a sparse diff and @@ -905,11 +928,8 @@ async fn clone_base_for_in_place_diff( .open(&dst) .with_context(|| format!("creating {}", dst.display()))?; let refused = match crate::storage::pmem_store::ficlone(&base_file, &dst_file) { - Ok(()) => match crate::storage::extents::reports_shared_extents(&dst_file) { - Ok(true) => None, - Ok(false) => Some("the filesystem does not report shared extents".to_string()), - Err(e) => Some(format!("the filesystem cannot report extents: {e}")), - }, + Ok(()) => memory_file_refuses_in_place_diff(&dst_file, memory_bytes) + .context("checking the cloned memory file")?, Err(e) => match e.raw_os_error() { Some(libc::EXDEV | libc::EOPNOTSUPP | libc::EINVAL | libc::ENOTTY) => Some( format!("the store cannot clone the parent's memory file: {e}"), @@ -947,6 +967,80 @@ async fn clone_base_for_in_place_diff( } } +/// Why the memory file a staged subvolume snapshot brought from the parent cannot take the +/// diff in place, or None. Firecracker opens the memory file by name and follows a symlink, so +/// the staged entry must be the regular file the subvolume snapshot made; opening it for +/// writing here finds a read-only file before the pause, where Firecracker would find it after. +fn staged_memory_refusal(path: &Path, memory_bytes: u64) -> Result> { + use std::os::unix::fs::OpenOptionsExt; + let meta = + std::fs::symlink_metadata(path).with_context(|| format!("reading {}", path.display()))?; + if !meta.file_type().is_file() { + return Ok(Some( + "the parent's memory file is not a regular file".to_string(), + )); + } + let file = match std::fs::OpenOptions::new() + .read(true) + .write(true) + .custom_flags(libc::O_NOFOLLOW) + .open(path) + { + Ok(file) => file, + Err(e) => { + return Ok(Some(format!( + "the staged memory file cannot be opened for writing: {e}" + ))) + } + }; + memory_file_refuses_in_place_diff(&file, memory_bytes) + .context("checking the staged memory file") +} + +/// Whether the memory file a staged subvolume snapshot brought from the parent can take the +/// diff in place (`memory_file_refuses_in_place_diff`). The caller discards a refused +/// staging as a whole subvolume, since removing the file alone drops its references one +/// extent at a time. +async fn staged_memory_takes_in_place_diff( + memory: &Path, + memory_bytes: u64, + snapshot: &str, +) -> Result { + let path = memory.to_path_buf(); + let refused = tokio::task::spawn_blocking(move || staged_memory_refusal(&path, memory_bytes)) + .await + .context("staged memory check task panicked")??; + match refused { + None => { + info!(snapshot = %snapshot, + "staged as a subvolume snapshot of the parent, the diff is written into its memory file"); + Ok(true) + } + Some(reason) => { + info!(snapshot = %snapshot, reason = %reason, "writing the diff to memory.diff and merging it"); + Ok(false) + } + } +} + +/// The free bytes on the filesystem holding `path`, or None when statvfs fails. When fewer +/// than `required` are free, it first waits for btrfs to free the space of deleted +/// subvolumes: a replaced or pruned snapshot directory is deleted as a subvolume, and its +/// space comes back on the cleaner thread after the delete returns. +async fn available_after_deletes(path: &Path, required: u64) -> Option { + let available = |path: &Path| { + nix::sys::statvfs::statvfs(path) + .ok() + .map(|stat| stat.blocks_available() * stat.fragment_size()) + }; + let now = available(path)?; + if now >= required { + return Some(now); + } + crate::storage::disk::wait_for_deleted_subvolumes(path).await; + available(path) +} + /// What giving an in-place diff's long zero runs back as holes did. #[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] pub struct InPlaceStats { @@ -4359,14 +4453,14 @@ async fn atomic_replace_dir(temp_dir: &Path, final_dir: &Path) -> Result<()> { async fn replace_dir(temp_dir: &Path, final_dir: &Path) -> Result<()> { if final_dir.exists() { let old = snapshot_sibling(final_dir, "old"); - let _ = tokio::fs::remove_dir_all(&old).await; + let _ = crate::storage::disk::remove_snapshot_dir(&old).await; tokio::fs::rename(final_dir, &old) .await .context("moving old snapshot out of the way")?; tokio::fs::rename(temp_dir, final_dir) .await .context("renaming temp snapshot to final location")?; - let _ = tokio::fs::remove_dir_all(&old).await; + let _ = crate::storage::disk::remove_snapshot_dir(&old).await; } else { tokio::fs::rename(temp_dir, final_dir) .await @@ -4893,7 +4987,7 @@ async fn publish_snapshot_dir( } .await; if let Err(e) = written { - let _ = tokio::fs::remove_dir_all(temp_snapshot_dir).await; + let _ = crate::storage::disk::remove_snapshot_dir(temp_snapshot_dir).await; return Err(e); } atomic_replace_dir(temp_snapshot_dir, snapshot_dir).await @@ -4934,8 +5028,8 @@ pub async fn create_disk_only_snapshot_core( .to_path_buf(); let temp_snapshot_dir = snapshot_sibling(&snapshot_dir, "creating"); - let _ = tokio::fs::remove_dir_all(&temp_snapshot_dir).await; - tokio::fs::create_dir_all(&temp_snapshot_dir) + crate::storage::disk::clear_staging(&temp_snapshot_dir).await?; + crate::storage::disk::create_dir_or_subvolume(&temp_snapshot_dir) .await .context("creating temp snapshot directory")?; @@ -4952,7 +5046,7 @@ pub async fn create_disk_only_snapshot_core( .await .context("checking provisioned marker in guest")?; if marker_out.exit_code != 0 { - let _ = tokio::fs::remove_dir_all(&temp_snapshot_dir).await; + let _ = crate::storage::disk::remove_snapshot_dir(&temp_snapshot_dir).await; anyhow::bail!( "source VM has no provisioned marker (/var/lib/fcvm/provisioned); a clone \ of this disk would wipe its container storage on boot. The source must be \ @@ -4972,7 +5066,7 @@ pub async fn create_disk_only_snapshot_core( .await .context("guest sync before freeze")?; if sync_out.exit_code != 0 { - let _ = tokio::fs::remove_dir_all(&temp_snapshot_dir).await; + let _ = crate::storage::disk::remove_snapshot_dir(&temp_snapshot_dir).await; anyhow::bail!( "guest sync failed before disk capture (exit {}): {}", sync_out.exit_code, @@ -5007,7 +5101,7 @@ pub async fn create_disk_only_snapshot_core( ]; let _ = run_exec_in_vm_captured(vsock_socket, &thaw, false, &snapshot_config.vm_id) .await; - let _ = tokio::fs::remove_dir_all(&temp_snapshot_dir).await; + let _ = crate::storage::disk::remove_snapshot_dir(&temp_snapshot_dir).await; return Err(e).context("freezing guest filesystems"); } }; @@ -5022,7 +5116,7 @@ pub async fn create_disk_only_snapshot_core( ), ]; let _ = run_exec_in_vm_captured(vsock_socket, &thaw, false, &snapshot_config.vm_id).await; - let _ = tokio::fs::remove_dir_all(&temp_snapshot_dir).await; + let _ = crate::storage::disk::remove_snapshot_dir(&temp_snapshot_dir).await; anyhow::bail!( "fsfreeze failed (exit {}): {}", freeze_out.exit_code, @@ -5064,14 +5158,14 @@ pub async fn create_disk_only_snapshot_core( run_exec_in_vm_captured(vsock_socket, &unfreeze_cmd, false, &snapshot_config.vm_id).await; if let Err(e) = copy_result { - let _ = tokio::fs::remove_dir_all(&temp_snapshot_dir).await; + let _ = crate::storage::disk::remove_snapshot_dir(&temp_snapshot_dir).await; return Err(e).context("reflinking disk for disk-only snapshot"); } match unfreeze_result { Ok(o) if o.exit_code == 0 => info!(snapshot = %snapshot_config.name, "guest unfrozen"), Ok(o) => { - let _ = tokio::fs::remove_dir_all(&temp_snapshot_dir).await; + let _ = crate::storage::disk::remove_snapshot_dir(&temp_snapshot_dir).await; anyhow::bail!( "fsfreeze --unfreeze failed (exit {}): {} — source VM may be wedged", o.exit_code, @@ -5079,7 +5173,7 @@ pub async fn create_disk_only_snapshot_core( ); } Err(e) => { - let _ = tokio::fs::remove_dir_all(&temp_snapshot_dir).await; + let _ = crate::storage::disk::remove_snapshot_dir(&temp_snapshot_dir).await; return Err(e).context("unfreezing guest — source VM may be wedged"); } } @@ -5095,14 +5189,14 @@ pub async fn create_disk_only_snapshot_core( // Atomic replace into the final location. if snapshot_dir.exists() { let old = snapshot_sibling(&snapshot_dir, "old"); - let _ = tokio::fs::remove_dir_all(&old).await; + let _ = crate::storage::disk::remove_snapshot_dir(&old).await; tokio::fs::rename(&snapshot_dir, &old) .await .context("moving old snapshot aside")?; tokio::fs::rename(&temp_snapshot_dir, &snapshot_dir) .await .context("finalizing snapshot")?; - let _ = tokio::fs::remove_dir_all(&old).await; + let _ = crate::storage::disk::remove_snapshot_dir(&old).await; } else { tokio::fs::rename(&temp_snapshot_dir, &snapshot_dir) .await @@ -5414,7 +5508,7 @@ pub async fn create_snapshot_core( // to prevent confusion and reclaim disk space. if snapshot_dir.exists() && !snapshot_dir.join("config.json").exists() { info!(snapshot = %snapshot_config.name, "cleaning up stale snapshot directory"); - let _ = tokio::fs::remove_dir_all(snapshot_dir).await; + let _ = crate::storage::disk::remove_snapshot_dir(snapshot_dir).await; } let memory_bytes = (snapshot_config.metadata.memory_mib as u64) * 1024 * 1024; @@ -5438,8 +5532,7 @@ pub async fn create_snapshot_core( } else { snapshot_dir.parent().unwrap_or(snapshot_dir) }; - if let Ok(stat) = nix::sys::statvfs::statvfs(statvfs_path) { - let available_bytes = stat.blocks_available() * stat.fragment_size(); + if let Some(available_bytes) = available_after_deletes(statvfs_path, required_bytes).await { if available_bytes < required_bytes { anyhow::bail!( "Not enough disk space for {} snapshot: need {} MiB, have {} MiB free on {}. \ @@ -5462,17 +5555,45 @@ pub async fn create_snapshot_core( ); // Clean up any leftover temp directory from previous failed attempt - let _ = tokio::fs::remove_dir_all(&temp_snapshot_dir).await; - tokio::fs::create_dir_all(&temp_snapshot_dir) - .await - .context("creating temp snapshot directory")?; + crate::storage::disk::clear_staging(&temp_snapshot_dir).await?; + + // A diff snapshot whose parent's directory is a btrfs subvolume is staged as a subvolume + // snapshot of it, which brings the parent's memory file along at the cost of one + // transaction commit. Any other snapshot directory is created as a subvolume where the + // store allows, so that the snapshots taken after it can be staged that way. A parent + // memory file of another length cannot take the diff in place, so it is not staged. + let parent_fits = base_memory_source + .as_ref() + .and_then(|base| std::fs::metadata(base).ok()) + .is_some_and(|meta| meta.len() == memory_bytes); + let staged = match parent_snapshot_dir { + Some(parent_dir) if has_base && parent_fits => { + crate::storage::disk::stage_from_parent_subvolume(parent_dir, &temp_snapshot_dir) + .await? + } + _ => false, + }; + if !staged { + crate::storage::disk::create_dir_or_subvolume(&temp_snapshot_dir) + .await + .context("creating temp snapshot directory")?; + } - // A diff snapshot is written in place when the store can clone the parent's memory file: - // Firecracker writes the dirty pages into the clone, so there is no diff file to merge - // afterwards. The clone is made here, before the pause. Otherwise Firecracker writes the - // dirty pages to a sparse memory.diff, merged onto a copy of the parent's memory.bin after - // the resume. A full snapshot writes memory.bin. + // A diff snapshot is written in place when its memory file shares the parent's extents: + // Firecracker writes the dirty pages into it, so there is no diff file to merge afterwards. + // The file comes from the staged subvolume snapshot, or is a clone of the parent's made + // here, before the pause. Otherwise Firecracker writes the dirty pages to a sparse + // memory.diff, merged onto a copy of the parent's memory.bin after the resume. A full + // snapshot writes memory.bin. let diff_in_place = match base_memory_source.as_ref() { + Some(_) if has_base && staged => { + staged_memory_takes_in_place_diff( + &temp_snapshot_dir.join("memory.bin"), + memory_bytes, + &snapshot_config.name, + ) + .await + } Some(base) if has_base => { clone_base_for_in_place_diff( base, @@ -5487,10 +5608,18 @@ pub async fn create_snapshot_core( let diff_in_place = match diff_in_place { Ok(in_place) => in_place, Err(e) => { - let _ = tokio::fs::remove_dir_all(&temp_snapshot_dir).await; + let _ = crate::storage::disk::remove_snapshot_dir(&temp_snapshot_dir).await; return Err(e); } }; + // A staged memory file that cannot take the diff goes with its whole subvolume, and the + // diff is written to memory.diff in a fresh directory and merged. + if staged && !diff_in_place { + crate::storage::disk::clear_staging(&temp_snapshot_dir).await?; + crate::storage::disk::create_dir_or_subvolume(&temp_snapshot_dir) + .await + .context("creating temp snapshot directory")?; + } let temp_memory_path = if has_base && !diff_in_place { temp_snapshot_dir.join("memory.diff") } else { @@ -5538,7 +5667,7 @@ pub async fn create_snapshot_core( ) .await .context("reopening guest network after Firecracker preparation failure"); - let _ = tokio::fs::remove_dir_all(&temp_snapshot_dir).await; + let _ = crate::storage::disk::remove_snapshot_dir(&temp_snapshot_dir).await; return combine_snapshot_boundary_results( Err(operation_error), Ok(()), @@ -5571,7 +5700,7 @@ pub async fn create_snapshot_core( resume_paused_source(vsock_socket_path, &snapshot_config.vm_id) .await .context("reopening guest network after Firecracker pause failure"); - let _ = tokio::fs::remove_dir_all(&temp_snapshot_dir).await; + let _ = crate::storage::disk::remove_snapshot_dir(&temp_snapshot_dir).await; return combine_snapshot_boundary_results( Err(pause_error), hypervisor_resume_result, @@ -5759,7 +5888,7 @@ pub async fn create_snapshot_core( hypervisor_resume_result, guest_network_resume_result, ) { - let _ = tokio::fs::remove_dir_all(&temp_snapshot_dir).await; + let _ = crate::storage::disk::remove_snapshot_dir(&temp_snapshot_dir).await; return Err(error); } @@ -5804,7 +5933,7 @@ pub async fn create_snapshot_core( let punched = match punched { Ok(punched) => punched, Err(e) => { - let _ = tokio::fs::remove_dir_all(&temp_snapshot_dir).await; + let _ = crate::storage::disk::remove_snapshot_dir(&temp_snapshot_dir).await; return Err(e); } }; @@ -5840,7 +5969,7 @@ pub async fn create_snapshot_core( // Copy base memory to temp dir as memory.bin (will merge diff into this copy) if let Err(e) = tokio::fs::copy(base_source, &final_memory_path).await { - let _ = tokio::fs::remove_dir_all(&temp_snapshot_dir).await; + let _ = crate::storage::disk::remove_snapshot_dir(&temp_snapshot_dir).await; return Err(e).context("copying base memory to temp for merge"); } @@ -5866,7 +5995,7 @@ pub async fn create_snapshot_core( Ok(merged) => merged, Err(e) => { // The unfinished directory holds a copy of the memory file. Nothing reads it again. - let _ = tokio::fs::remove_dir_all(&temp_snapshot_dir).await; + let _ = crate::storage::disk::remove_snapshot_dir(&temp_snapshot_dir).await; return Err(e); } }; @@ -5956,7 +6085,7 @@ pub async fn create_snapshot_ch( // Clean up a stale (incomplete) snapshot dir — one with no config.json. if snapshot_dir.exists() && !snapshot_dir.join("config.json").exists() { info!(snapshot = %snapshot_config.name, "cleaning up stale snapshot directory"); - let _ = tokio::fs::remove_dir_all(snapshot_dir).await; + let _ = crate::storage::disk::remove_snapshot_dir(snapshot_dir).await; } // Disk-space guard: a full memory dump needs ~memory_mib. Failing ENOSPC mid-dump @@ -5968,8 +6097,7 @@ pub async fn create_snapshot_ch( } else { snapshot_dir.parent().unwrap_or(snapshot_dir) }; - if let Ok(stat) = nix::sys::statvfs::statvfs(statvfs_path) { - let available_bytes = stat.blocks_available() * stat.fragment_size(); + if let Some(available_bytes) = available_after_deletes(statvfs_path, required_bytes).await { if available_bytes < required_bytes { anyhow::bail!( "Not enough disk space for Cloud Hypervisor snapshot: need {} MiB, \ @@ -5983,7 +6111,10 @@ pub async fn create_snapshot_ch( } // Fresh temp dir + the CH sub-dir CH dumps its files into. - let _ = tokio::fs::remove_dir_all(&temp_snapshot_dir).await; + crate::storage::disk::clear_staging(&temp_snapshot_dir).await?; + crate::storage::disk::create_dir_or_subvolume(&temp_snapshot_dir) + .await + .context("creating temp CH snapshot directory")?; let ch_dir = temp_snapshot_dir.join(CH_SNAPSHOT_SUBDIR); tokio::fs::create_dir_all(&ch_dir) .await @@ -6012,7 +6143,7 @@ pub async fn create_snapshot_ch( ) .await .context("reopening guest network after Cloud Hypervisor preparation failure"); - let _ = tokio::fs::remove_dir_all(&temp_snapshot_dir).await; + let _ = crate::storage::disk::remove_snapshot_dir(&temp_snapshot_dir).await; return combine_snapshot_boundary_results( Err(operation_error), Ok(()), @@ -6040,7 +6171,7 @@ pub async fn create_snapshot_ch( resume_paused_source(vsock_socket_path, &snapshot_config.vm_id) .await .context("reopening guest network after Cloud Hypervisor pause failure"); - let _ = tokio::fs::remove_dir_all(&temp_snapshot_dir).await; + let _ = crate::storage::disk::remove_snapshot_dir(&temp_snapshot_dir).await; return combine_snapshot_boundary_results( Err(pause_error), hypervisor_resume_result, @@ -6101,7 +6232,7 @@ pub async fn create_snapshot_ch( hypervisor_resume_result, guest_network_resume_result, ) { - let _ = tokio::fs::remove_dir_all(&temp_snapshot_dir).await; + let _ = crate::storage::disk::remove_snapshot_dir(&temp_snapshot_dir).await; return Err(error); } @@ -6125,6 +6256,24 @@ pub async fn create_snapshot_ch( #[cfg(test)] mod tests { + /// A staged memory.bin that is a symlink is refused, so Firecracker cannot write the diff + /// into the file it points at. + #[test] + fn a_staged_memory_symlink_is_refused() { + let dir = tempfile::tempdir().unwrap(); + let other = dir.path().join("other.bin"); + std::fs::write(&other, vec![0u8; 8192]).unwrap(); + let staged = dir.path().join("memory.bin"); + std::os::unix::fs::symlink(&other, &staged).unwrap(); + let refused = staged_memory_refusal(&staged, 8192).unwrap(); + assert!( + refused + .as_deref() + .is_some_and(|r| r.contains("not a regular file")), + "{refused:?}" + ); + } + use super::*; use crate::state::VmState; use crate::storage::SnapshotType; diff --git a/src/setup/storage.rs b/src/setup/storage.rs index 14233b57a..f1ea52d7f 100644 --- a/src/setup/storage.rs +++ b/src/setup/storage.rs @@ -233,9 +233,12 @@ fn ensure_storage_with( // Mount the loopback image info!("Mounting btrfs filesystem..."); + // user_subvol_rm_allowed lets an unprivileged owner delete the subvolumes fcvm makes for + // snapshot directories; without it a rootless delete removes them file by file, one + // reference per extent of each memory file. let status = Command::new("mount") .arg("-o") - .arg("loop") + .arg("loop,user_subvol_rm_allowed") .arg(&loopback_image) .arg(&mount_point) .status() diff --git a/src/storage/disk.rs b/src/storage/disk.rs index 64ac42c1a..fca65a8d5 100644 --- a/src/storage/disk.rs +++ b/src/storage/disk.rs @@ -39,7 +39,7 @@ fn is_snapshot_disk_subvolume(dir: &Path) -> bool { /// would get a disk it cannot open for writing and a directory it cannot /// remove at teardown; it reflinks the disk instead, which makes a copy it /// owns. Root can write and remove anything. -fn can_own_subvolume_snapshot_of(base_dir: &Path) -> bool { +pub fn can_own_subvolume_snapshot_of(base_dir: &Path) -> bool { use std::os::unix::fs::MetadataExt; let euid = nix::unistd::geteuid().as_raw(); euid == 0 @@ -76,13 +76,170 @@ pub async fn create_dir_or_subvolume(path: &Path) -> Result<()> { info!( path = %path.display(), %reason, - "not a btrfs subvolume; created as a directory (clones of it reflink the disk)" + "not a btrfs subvolume; created as a directory (copies of it reflink its files)" ); fs::create_dir_all(path) .await .with_context(|| format!("creating {}", path.display())) } +/// Remove a snapshot directory. A snapshot directory that is a btrfs subvolume (and its +/// `disk/` subvolume) is deleted as a subvolume, which detaches it at once and leaves the +/// cleaner thread to drop its extent references in the background: unlinking a memory file +/// written by in-place diffs drops one reference per extent first, 52 to 93 s for one with +/// 11.8 million extents. Anything else, or a subvolume this process may not delete (an +/// unprivileged caller without user_subvol_rm_allowed), is removed file by file. +pub async fn remove_snapshot_dir(dir: &Path) -> std::io::Result<()> { + if is_btrfs_subvolume(dir) { + let disk = dir.join(crate::storage::snapshot::SNAPSHOT_DISK_SUBDIR); + let disk_gone = !is_btrfs_subvolume(&disk) || delete_subvolume(&disk).await; + if disk_gone && delete_subvolume(dir).await && !dir.exists() { + return Ok(()); + } + } + match fs::remove_dir_all(dir).await { + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(()), + other => other, + } +} + +/// Remove a snapshot create's leftover staging directory, and fail if it is still there: a +/// subvolume snapshot made onto an existing directory lands inside it, and a subvolume +/// cannot be created over one, so a leftover would be silently reused. +pub async fn clear_staging(staging: &Path) -> Result<()> { + clear_staging_with(staging, remove_snapshot_dir(staging).await) +} + +/// `clear_staging` after its removal returned `removed`. +fn clear_staging_with(staging: &Path, removed: std::io::Result<()>) -> Result<()> { + if staging.exists() { + let reason = match removed { + Err(error) => error.to_string(), + Ok(()) => "it is still there after its removal".to_string(), + }; + bail!( + "could not remove the leftover staging directory {}: {reason}", + staging.display() + ); + } + Ok(()) +} + +/// Wait for btrfs to free the space of subvolumes deleted on the filesystem holding `path` +/// (`btrfs subvolume sync`, bounded): a deleted subvolume's space comes back on the cleaner +/// thread after the delete returns, so a free-space check right after one can read low. +pub async fn wait_for_deleted_subvolumes(path: &Path) { + let sync = tokio::process::Command::new("btrfs") + .args(["subvolume", "sync"]) + .arg(path) + .kill_on_drop(true) + .output(); + match tokio::time::timeout(std::time::Duration::from_secs(120), sync).await { + Ok(Ok(output)) if output.status.success() => {} + Ok(Ok(output)) => debug!(path = %path.display(), + stderr = %String::from_utf8_lossy(&output.stderr).trim(), "btrfs subvolume sync refused"), + Ok(Err(error)) => debug!(path = %path.display(), %error, "btrfs tool unavailable"), + Err(_) => warn!(path = %path.display(), "btrfs subvolume sync did not finish in 120 s"), + } +} + +/// `btrfs subvolume delete`, without waiting for the cleaner. Whether it succeeded. +async fn delete_subvolume(path: &Path) -> bool { + let output = tokio::process::Command::new("btrfs") + .args(["subvolume", "delete"]) + .arg(path) + .output() + .await; + match output { + Ok(output) if output.status.success() => true, + Ok(output) => { + debug!(path = %path.display(), stderr = %String::from_utf8_lossy(&output.stderr).trim(), + "btrfs subvolume delete refused; removing file by file"); + false + } + Err(error) => { + debug!(path = %path.display(), %error, "btrfs tool unavailable; removing file by file"); + false + } + } +} + +/// Stage a diff snapshot's directory as a subvolume snapshot of its parent's, keeping only +/// the parent's memory file, which the diff is then written into. A subvolume snapshot costs +/// one transaction commit whatever the memory file's extent count, where a reflink of the +/// file costs one reference per extent: 133 s for a memory file of 11.8 million extents +/// written by eight in-place diffs. Returns false, leaving no `staging`, when the parent's +/// directory is not a subvolume this process may snapshot or the snapshot fails; the caller +/// then reflinks the memory file. A failed staging that cannot be removed is an error, since +/// the reflink path would otherwise create its directory over the parent's leftover files. +/// +/// The parent's other files (its config, vmstate, recorded working set, locks) are removed +/// from the staging directory, and so is its `disk/`, which a subvolume snapshot shows as an +/// empty placeholder directory when it is a subvolume of its own. +pub async fn stage_from_parent_subvolume(parent: &Path, staging: &Path) -> Result { + if !is_btrfs_subvolume(parent) || !can_own_subvolume_snapshot_of(parent) { + return Ok(false); + } + if staging.exists() { + bail!( + "{} exists; a subvolume snapshot would land inside it", + staging.display() + ); + } + let started = std::time::Instant::now(); + let output = tokio::process::Command::new("btrfs") + .args(["subvolume", "snapshot"]) + .arg(parent) + .arg(staging) + .output() + .await; + let refused = match output { + Ok(output) if output.status.success() => None, + Ok(output) => Some(String::from_utf8_lossy(&output.stderr).trim().to_string()), + Err(error) => Some(format!("btrfs tool unavailable: {error}")), + }; + if let Some(reason) = refused { + info!(parent = %parent.display(), %reason, + "could not snapshot the parent's subvolume; reflinking its memory file instead"); + clear_staging(staging).await?; + return Ok(false); + } + let snapshot_ms = started.elapsed().as_millis() as u64; + if let Err(error) = keep_only_memory_file(staging).await { + warn!(staging = %staging.display(), %error, + "could not clear the parent's other files from the staged snapshot; reflinking instead"); + clear_staging(staging).await?; + return Ok(false); + } + debug!(parent = %parent.display(), staging = %staging.display(), snapshot_ms, + "staged the snapshot as a subvolume snapshot of its parent"); + Ok(true) +} + +/// Remove everything in `dir` but the memory file: files, directories, and the empty +/// placeholder a nested subvolume leaves in a subvolume snapshot. +async fn keep_only_memory_file(dir: &Path) -> Result<()> { + let mut entries = fs::read_dir(dir) + .await + .with_context(|| format!("reading {}", dir.display()))?; + while let Some(entry) = entries.next_entry().await? { + if entry.file_name() == crate::storage::snapshot::SNAPSHOT_MEMORY_FILE { + continue; + } + let path = entry.path(); + if entry.file_type().await?.is_dir() { + fs::remove_dir_all(&path) + .await + .with_context(|| format!("removing {}", path.display()))?; + } else { + fs::remove_file(&path) + .await + .with_context(|| format!("removing {}", path.display()))?; + } + } + Ok(()) +} + /// Configuration for a VM disk #[derive(Debug, Clone)] pub struct DiskConfig { @@ -544,3 +701,81 @@ pub fn parse_size(s: &str) -> Result { num.checked_mul(multiplier) .with_context(|| format!("size overflow: {} * {}", num, multiplier)) } + +#[cfg(test)] +mod snapshot_dir_tests { + use super::*; + + /// A leftover staging directory that cannot be removed fails the create: a subvolume + /// snapshot made onto it would land inside it. The removal's failure is given to the + /// check, since root removes files from a read-only directory. + #[tokio::test] + async fn an_unremovable_leftover_staging_directory_fails() { + let dir = tempfile::tempdir().unwrap(); + let staging = dir.path().join("snap.creating"); + std::fs::create_dir_all(staging.join("disk")).unwrap(); + let refused = std::io::Error::from_raw_os_error(libc::EPERM); + let error = clear_staging_with(&staging, Err(refused)) + .expect_err("an unremovable leftover must fail the create"); + assert!( + error.to_string().contains("leftover staging directory"), + "{error}" + ); + clear_staging_with(&staging, Ok(())).expect_err("a leftover still present must fail"); + clear_staging(&staging).await.unwrap(); + assert!(!staging.exists()); + clear_staging(&staging).await.unwrap(); + } + + /// A staged snapshot keeps the parent's memory file and nothing else of its directory. + #[tokio::test] + async fn staging_keeps_only_the_memory_file() { + let dir = tempfile::tempdir().unwrap(); + let staged = dir.path(); + std::fs::write(staged.join("memory.bin"), b"memory").unwrap(); + std::fs::write(staged.join("config.json"), b"{}").unwrap(); + std::fs::write(staged.join("memory.bin.working-set"), b"set").unwrap(); + std::fs::create_dir(staged.join("disk")).unwrap(); + std::fs::write(staged.join("disk").join("disk.raw"), b"disk").unwrap(); + std::fs::create_dir(staged.join("empty-placeholder")).unwrap(); + keep_only_memory_file(staged).await.unwrap(); + let mut left: Vec = std::fs::read_dir(staged) + .unwrap() + .map(|entry| entry.unwrap().file_name().to_string_lossy().into_owned()) + .collect(); + left.sort(); + assert_eq!(left, ["memory.bin"]); + assert_eq!(std::fs::read(staged.join("memory.bin")).unwrap(), b"memory"); + } + + /// A parent directory that is not a subvolume stages nothing, and the caller reflinks. + #[tokio::test] + async fn a_plain_parent_directory_is_not_staged() { + let dir = tempfile::tempdir().unwrap(); + let parent = dir.path().join("parent"); + std::fs::create_dir(&parent).unwrap(); + std::fs::write(parent.join("memory.bin"), b"memory").unwrap(); + let staging = dir.path().join("parent.creating"); + assert!(!stage_from_parent_subvolume(&parent, &staging) + .await + .unwrap()); + assert!( + !staging.exists(), + "a refused staging left {}", + staging.display() + ); + } + + /// A plain snapshot directory is removed file by file, and a missing one is not an error. + #[tokio::test] + async fn a_plain_snapshot_directory_is_removed() { + let dir = tempfile::tempdir().unwrap(); + let snapshot = dir.path().join("snap"); + std::fs::create_dir_all(snapshot.join("disk")).unwrap(); + std::fs::write(snapshot.join("memory.bin"), b"memory").unwrap(); + std::fs::write(snapshot.join("disk").join("disk.raw"), b"disk").unwrap(); + remove_snapshot_dir(&snapshot).await.unwrap(); + assert!(!snapshot.exists()); + remove_snapshot_dir(&snapshot).await.unwrap(); + } +} diff --git a/src/storage/snapshot.rs b/src/storage/snapshot.rs index 4baa78fb7..d3749918f 100644 --- a/src/storage/snapshot.rs +++ b/src/storage/snapshot.rs @@ -87,6 +87,9 @@ impl std::fmt::Display for SnapshotKind { /// kernel flushes nothing but the disks (reflinks, so already clean) to take it. pub const SNAPSHOT_DISK_SUBDIR: &str = "disk"; +/// The memory file's name in a snapshot's directory. +pub const SNAPSHOT_MEMORY_FILE: &str = "memory.bin"; + /// Where a snapshot directory keeps its disks. pub fn snapshot_disk_dir(snapshot_dir: &Path) -> PathBuf { snapshot_dir.join(SNAPSHOT_DISK_SUBDIR) @@ -482,7 +485,7 @@ impl SnapshotManager { .with_context(|| format!("locking snapshot '{}' for deletion", name))?; if snapshot_dir.exists() { - fs::remove_dir_all(&snapshot_dir) + crate::storage::disk::remove_snapshot_dir(&snapshot_dir) .await .context("removing snapshot directory")?; @@ -527,7 +530,7 @@ impl SnapshotManager { return Ok(false); } - fs::remove_dir_all(&snapshot_dir) + crate::storage::disk::remove_snapshot_dir(&snapshot_dir) .await .context("removing invalid snapshot generation")?; info!(snapshot = name, "snapshot generation deleted"); diff --git a/src/uffd/release.rs b/src/uffd/release.rs index 7d1d344f3..d4f943b7e 100644 --- a/src/uffd/release.rs +++ b/src/uffd/release.rs @@ -54,8 +54,7 @@ use std::path::Path; use std::time::Duration; use tracing::{info, warn}; -/// The memory file's name in a snapshot's directory. -const MEMORY_FILE: &str = "memory.bin"; +use crate::storage::snapshot::SNAPSHOT_MEMORY_FILE as MEMORY_FILE; /// The endings of the directories a snapshot create works in beside the installed /// snapshots (`commands::common::snapshot_sibling`). diff --git a/tests/test_diff_snapshot.rs b/tests/test_diff_snapshot.rs index 39af4dcc8..5047c8593 100644 --- a/tests/test_diff_snapshot.rs +++ b/tests/test_diff_snapshot.rs @@ -404,6 +404,37 @@ async fn test_user_snapshot_from_clone_uses_parent() -> Result<()> { anyhow::bail!("User snapshot from clone failed: {}", stderr); } println!(" ✓ User snapshot created: {}", snapshot2_name); + // On a btrfs store each snapshot directory is a subvolume, and a diff is staged as a + // subvolume snapshot of its parent's, which costs one commit where a reflink of the + // parent's memory file costs one reference per extent. + let snapshot1_dir = snapshot_dir().join(&snapshot1_name); + let on_btrfs = nix::sys::statfs::statfs(&snapshot1_dir) + .map(|fs| fs.filesystem_type() == nix::sys::statfs::BTRFS_SUPER_MAGIC) + .unwrap_or(false); + if on_btrfs { + assert!( + fcvm::storage::disk::is_btrfs_subvolume(&snapshot1_dir), + "the first snapshot's directory {} is not a btrfs subvolume", + snapshot1_dir.display() + ); + assert!( + stderr.contains("staged as a subvolume snapshot of the parent"), + "the diff was not staged from the parent's subvolume:\n{stderr}" + ); + let snapshot2_dir = snapshot_dir().join(&snapshot2_name); + assert!( + fcvm::storage::disk::is_btrfs_subvolume(&snapshot2_dir), + "the diff snapshot's directory is not a btrfs subvolume" + ); + // The staging snapshot holds an empty placeholder where the parent's disk subvolume + // was; the diff's own disks must be a subvolume again, or its clones reflink them. + assert!( + fcvm::storage::disk::is_btrfs_subvolume( + &snapshot2_dir.join(fcvm::storage::snapshot::SNAPSHOT_DISK_SUBDIR) + ), + "the diff snapshot's disk directory is not a btrfs subvolume" + ); + } // What the merge did, for the log of this run. if let Some(line) = stderr .lines() @@ -477,6 +508,23 @@ async fn test_user_snapshot_from_clone_uses_parent() -> Result<()> { stderr ); + // Deleting a snapshot that is a subvolume holding a subvolume removes both. + let deleted = tokio::process::Command::new(&fcvm_path) + .args(["snapshots", "delete", "--force", &snapshot2_name]) + .output() + .await + .context("running snapshots delete")?; + assert!( + deleted.status.success(), + "snapshots delete failed: {}", + String::from_utf8_lossy(&deleted.stderr) + ); + assert!( + !snapshot2_dir.exists(), + "snapshots delete left {}", + snapshot2_dir.display() + ); + println!("\n✅ USER SNAPSHOT FROM CLONE TEST PASSED!"); Ok(()) }