From 64c1bb3bfc593fde621127e8d6f8b20626f62ca7 Mon Sep 17 00:00:00 2001 From: doge Date: Thu, 23 Jul 2026 00:26:09 +0800 Subject: [PATCH] block: Prevent QCOW data loss from stale punch-hole QCOW metadata published a fully deallocated cluster before the caller performed the host punch-hole. Under multi-queue load, a concurrent metadata flush could make the cluster allocatable, and another queue could reuse it before the delayed punch ran. If reused as an L2 table, the stale punch erased live metadata and made guest data unreachable. This was observed in production as confirmed guest data loss, with allocated guest clusters becoming refcounted but unreachable from the QCOW mapping. Keep punch-pending clusters out of both free lists. Publish a cluster only after the host punch succeeds, so another metadata flush is required before reuse. Add deterministic coverage for the cross-queue schedule and for host operation failures. Signed-off-by: doge --- block/src/formats/qcow/engine_sync.rs | 193 ++++++++++++++++++++++++- block/src/formats/qcow/engine_uring.rs | 8 +- block/src/formats/qcow/metadata.rs | 17 ++- 3 files changed, 215 insertions(+), 3 deletions(-) diff --git a/block/src/formats/qcow/engine_sync.rs b/block/src/formats/qcow/engine_sync.rs index 4bc3e4b5c..1d0ae3bbf 100644 --- a/block/src/formats/qcow/engine_sync.rs +++ b/block/src/formats/qcow/engine_sync.rs @@ -56,7 +56,13 @@ impl QcowSync { DeallocAction::PunchHole { host_offset, length, - } => self.data_file.file_mut().punch_hole(*host_offset, *length), + } => { + self.data_file + .file_mut() + .punch_hole(*host_offset, *length)?; + self.metadata.complete_punch_hole(*host_offset); + Ok(()) + } DeallocAction::WriteZeroes { host_offset, length, @@ -1980,6 +1986,113 @@ mod unit_tests { ); } + #[test] + fn test_partial_zero_action_failure_is_reported() { + const CLUSTER_SIZE: u64 = 1 << 16; + let data = vec![0xa5; CLUSTER_SIZE as usize]; + let (temp, disk) = create_disk_with_data(4 * CLUSTER_SIZE, &data, 0, true, false); + drop(disk); + + let raw = AlignedFile::new(temp.as_file().try_clone().unwrap(), false); + let (inner, backing, sparse) = super::super::parser::parse_qcow(raw, 0, true).unwrap(); + assert!(backing.is_none()); + let refcount_bits = 1u64 << inner.header.refcount_order; + let metadata = Arc::new(QcowMetadata::new(inner)); + + // Metadata remains writable, but use a read-only per-queue data fd so + // the partial-cluster WriteZeroes action deterministically fails. + let read_only = File::open(temp.as_path()).unwrap(); + let data_file = QcowRawFile::from( + AlignedFile::new(read_only, false), + CLUSTER_SIZE, + refcount_bits, + ) + .unwrap(); + let mut aio = QcowSync::new(Arc::clone(&metadata), data_file, None, sparse); + + aio.write_zeroes(4096, 4096, 901).unwrap(); + let (user_data, result) = next_completion(&mut aio); + assert_eq!(user_data, 901); + assert!(result < 0, "host WriteZeroes failure must reach the guest"); + + drop(aio); + metadata.shutdown(); + drop(metadata); + + let reopened = QcowDisk::new( + temp.as_file().try_clone().unwrap(), + false, + false, + true, + false, + ) + .unwrap(); + assert_eq!(async_read(&reopened, 0, data.len()), data); + } + + #[test] + fn test_failed_punch_is_reported_and_cluster_stays_reserved() { + const CLUSTER_SIZE: u64 = 1 << 16; + const L2_ENTRIES: u64 = CLUSTER_SIZE / 8; + const L1_SPAN: u64 = CLUSTER_SIZE * L2_ENTRIES; + + let data = vec![0x3c; CLUSTER_SIZE as usize]; + let (temp, disk) = create_disk_with_data(L1_SPAN + 2 * CLUSTER_SIZE, &data, 0, true, false); + drop(disk); + + let raw = AlignedFile::new(temp.as_file().try_clone().unwrap(), false); + let (inner, backing, sparse) = super::super::parser::parse_qcow(raw, 0, true).unwrap(); + assert!(backing.is_none()); + let refcount_bits = 1u64 << inner.header.refcount_order; + let writable_data_file = inner.raw_file.clone(); + let metadata = Arc::new(QcowMetadata::new(inner)); + let old_host_offset = match &metadata + .map_clusters_for_read(0, CLUSTER_SIZE as usize, false) + .unwrap()[0] + { + ClusterReadMapping::Allocated { offset, .. } => *offset, + other => panic!("expected allocated mapping, got {other:?}"), + }; + + // A read-only per-queue fd makes the host punch fail after the + // metadata deallocation has already completed. + let read_only = File::open(temp.as_path()).unwrap(); + let read_only_data_file = QcowRawFile::from( + AlignedFile::new(read_only, false), + CLUSTER_SIZE, + refcount_bits, + ) + .unwrap(); + let mut failing_aio = + QcowSync::new(Arc::clone(&metadata), read_only_data_file, None, sparse); + failing_aio.punch_hole(0, CLUSTER_SIZE, 902).unwrap(); + let (user_data, result) = next_completion(&mut failing_aio); + assert_eq!(user_data, 902); + assert!(result < 0, "host PunchHole failure must reach the guest"); + drop(failing_aio); + + // A FLUSH cannot make the failed-punch cluster reusable. + metadata.flush().unwrap(); + let mut writer = QcowSync::new(Arc::clone(&metadata), writable_data_file, None, sparse); + writer + .write_from_vec( + L1_SPAN as libc::off_t, + OwnedIoBuffer::from_vec(vec![0x7e; CLUSTER_SIZE as usize]), + 903, + ) + .unwrap(); + assert_eq!( + completion_tuple(&writer.next_completed_request().unwrap()), + (903, CLUSTER_SIZE as i32) + ); + writer.fsync(None).unwrap(); + + let mut inspect = temp.as_file().try_clone().unwrap(); + let l1_table_offset = read_be_u64_at(&mut inspect, TEST_HEADER_L1_TABLE_OFFSET); + let new_l2 = read_be_u64_at(&mut inspect, l1_table_offset + 8) & TEST_L1_L2_ADDR_MASK; + assert_ne!(new_l2, old_host_offset); + } + #[test] fn test_resize_grow() { let cluster_size = 1u64 << 16; @@ -2131,4 +2244,82 @@ mod unit_tests { let buf = async_read(&disk, 0, cluster_size); assert_eq!(buf, data); } + + // Regression test for a valid cross-queue schedule where a guest FLUSH + // and a new allocation run while a returned PunchHole side effect is + // still pending. + #[test] + fn stale_punch_cannot_reuse_or_destroy_new_l2() { + const CLUSTER_SIZE: u64 = 1 << 16; + const L2_ENTRIES: u64 = CLUSTER_SIZE / 8; + const L1_SPAN: u64 = CLUSTER_SIZE * L2_ENTRIES; + + let temp = TempFile::new().unwrap(); + let file = temp.as_file().try_clone().unwrap(); + let virtual_size = L1_SPAN + 2 * CLUSTER_SIZE; + qcow::create_image(&file, virtual_size, None).unwrap(); + + // Seed one allocated data cluster below L1[0]. + { + let disk = QcowDisk::new(file.try_clone().unwrap(), false, false, true, false).unwrap(); + async_write(&disk, 0, &vec![0x11; CLUSTER_SIZE as usize]); + async_fsync(&disk); + } + + let raw = AlignedFile::new(file.try_clone().unwrap(), false); + let (inner, backing, sparse) = super::super::parser::parse_qcow(raw, 0, true).unwrap(); + assert!(backing.is_none()); + let data_file = inner.raw_file.clone(); + let metadata = Arc::new(QcowMetadata::new(inner)); + let mut aio = QcowSync::new(Arc::clone(&metadata), data_file, None, sparse); + + // Queue A: update metadata, retain the old host side effect. + let actions = metadata + .deallocate_bytes(0, CLUSTER_SIZE as usize, true, false, None) + .unwrap(); + assert_eq!(actions.len(), 1); + let stale_punch_offset = match actions[0] { + DeallocAction::PunchHole { + host_offset, + length, + } => { + assert_eq!(length, CLUSTER_SIZE); + host_offset + } + _ => panic!("expected one full-cluster PunchHole"), + }; + + // Queue B: guest FLUSH must not publish the pending-punch cluster. + metadata.flush().unwrap(); + + // Queue C: allocate a new L2 and commit it. The pending-punch + // cluster must not be selected. + let new_guest_offset = L1_SPAN; + aio.write_from_vec( + new_guest_offset as libc::off_t, + OwnedIoBuffer::from_vec(vec![0x5a; CLUSTER_SIZE as usize]), + 77, + ) + .unwrap(); + let completion = aio.next_completed_request().unwrap(); + assert_eq!(completion_tuple(&completion), (77, CLUSTER_SIZE as i32)); + aio.fsync(None).unwrap(); + + let mut inspect = file.try_clone().unwrap(); + let l1_table_offset = read_be_u64_at(&mut inspect, TEST_HEADER_L1_TABLE_OFFSET); + let committed_l2 = read_be_u64_at(&mut inspect, l1_table_offset + 8) & TEST_L1_L2_ADDR_MASK; + assert_ne!(committed_l2, stale_punch_offset); + + // Queue A resumes. Completing the old action must not touch the new + // L2, and only now may the old data cluster enter unref_clusters. + aio.apply_dealloc_action(&actions[0]).unwrap(); + metadata.flush().unwrap(); + metadata.shutdown(); + drop(aio); + drop(metadata); + + let reopened = QcowDisk::new(file.try_clone().unwrap(), false, false, true, false).unwrap(); + let read_back = async_read(&reopened, new_guest_offset, CLUSTER_SIZE as usize); + assert!(read_back.iter().all(|&byte| byte == 0x5a)); + } } diff --git a/block/src/formats/qcow/engine_uring.rs b/block/src/formats/qcow/engine_uring.rs index 98dd33641..1c08c40a1 100644 --- a/block/src/formats/qcow/engine_uring.rs +++ b/block/src/formats/qcow/engine_uring.rs @@ -71,7 +71,13 @@ impl QcowAsync { DeallocAction::PunchHole { host_offset, length, - } => self.data_file.file_mut().punch_hole(*host_offset, *length), + } => { + self.data_file + .file_mut() + .punch_hole(*host_offset, *length)?; + self.metadata.complete_punch_hole(*host_offset); + Ok(()) + } DeallocAction::WriteZeroes { host_offset, length, diff --git a/block/src/formats/qcow/metadata.rs b/block/src/formats/qcow/metadata.rs index 18711c92c..d82a955bb 100644 --- a/block/src/formats/qcow/metadata.rs +++ b/block/src/formats/qcow/metadata.rs @@ -339,6 +339,18 @@ impl QcowMetadata { Ok(actions) } + /// Makes a fully deallocated data cluster eligible for reuse after the + /// caller has successfully completed the corresponding host punch-hole. + /// + /// The cluster is deliberately kept out of both free lists while the + /// destructive host operation is pending. Publishing it to + /// `unref_clusters` (rather than directly to `avail_clusters`) preserves + /// the existing rule that metadata must be flushed before a freed cluster + /// can be allocated again. + pub(super) fn complete_punch_hole(&self, host_offset: u64) { + self.inner.write().unwrap().unref_clusters.push(host_offset); + } + pub(super) fn virtual_size(&self) -> u64 { self.inner.read().unwrap().header.size } @@ -922,7 +934,10 @@ impl QcowState { self.set_cluster_refcount_track_freed(cluster_addr, new_refcount)?; self.l2_cache.get_mut(l1_index).unwrap()[l2_index] = dealloc_entry; if new_refcount == 0 { - self.unref_clusters.push(cluster_addr); + // The caller still has to punch this range in the host file. + // Do not publish it to either free list until that destructive + // operation has completed, otherwise another queue can reuse + // the cluster before the delayed punch runs. return Ok(Some(cluster_addr)); } } else if refcount == 1 {