From 55ef29aaa32b8916c0394bc97668e186c35f2000 Mon Sep 17 00:00:00 2001 From: Daniel Verkamp Date: Wed, 21 Oct 2020 14:09:59 -0700 Subject: [PATCH] disk: qcow: fix write_zeroes with backing file The qcow disk format implemented the discard and write zeroes requests by deallocating the underlying cluster when possible, with the assumption that a deallocated cluster would return all zeroes if it is read until it is written again. Add an alternate implementation of write_zeroes that is used when a backing file is present that uses the straightforward approach of allocating all clusters and zeroing them out in the raw file rather than using deallocation. BUG=b:171230599 TEST=BLKZEROOUT test case TEST=cargo test -p disk Change-Id: I745f6dc7aa411ec9b1be0150ba1bc96c011ada9a Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2490605 Reviewed-by: Dylan Reid Reviewed-by: Cody Schuffelen Tested-by: kokoro Commit-Queue: Daniel Verkamp --- disk/src/qcow/mod.rs | 62 +++++++++++++++++++++++++++++++++++++------- 1 file changed, 52 insertions(+), 10 deletions(-) diff --git a/disk/src/qcow/mod.rs b/disk/src/qcow/mod.rs index 572e118da7..2ef00ab9d9 100644 --- a/disk/src/qcow/mod.rs +++ b/disk/src/qcow/mod.rs @@ -1251,7 +1251,7 @@ impl QcowFile { } // Deallocate the storage for the cluster starting at `address`. - // Any future reads of this cluster will return all zeroes. + // Any future reads of this cluster will return all zeroes (or the backing file, if in use). fn deallocate_cluster(&mut self, address: u64) -> std::io::Result<()> { if address >= self.virtual_size() as u64 { return Err(std::io::Error::from_raw_os_error(EINVAL)); @@ -1322,9 +1322,10 @@ impl QcowFile { Ok(()) } - // Deallocate the storage for `length` bytes starting at `address`. + // Fill a range of `length` bytes starting at `address` with zeroes. // Any future reads of this range will return all zeroes. - fn deallocate_bytes(&mut self, address: u64, length: usize) -> std::io::Result<()> { + // If there is no backing file, this will deallocate cluster storage when possible. + fn zero_bytes(&mut self, address: u64, length: usize) -> std::io::Result<()> { let write_count: usize = self.limit_range_file(address, length); let mut nwritten: usize = 0; @@ -1332,14 +1333,22 @@ impl QcowFile { let curr_addr = address + nwritten as u64; let count = self.limit_range_cluster(curr_addr, write_count - nwritten); - if count == self.raw_file.cluster_size() as usize { - // Full cluster - deallocate the storage. + if self.backing_file.is_none() && count == self.raw_file.cluster_size() as usize { + // Full cluster and no backing file in use - deallocate the storage. self.deallocate_cluster(curr_addr)?; } else { - // Partial cluster - zero out the relevant bytes if it was allocated. - // Any space in unallocated clusters can be left alone, since - // unallocated clusters already read back as zeroes. - if let Some(offset) = self.file_offset_read(curr_addr)? { + // Partial cluster - zero out the relevant bytes. + let offset = if self.backing_file.is_some() { + // There is a backing file, so we need to allocate a cluster in order to + // zero out the hole-punched bytes such that the backing file contents do not + // show through. + Some(self.file_offset_write(curr_addr)?) + } else { + // Any space in unallocated clusters can be left alone, since + // unallocated clusters already read back as zeroes. + self.file_offset_read(curr_addr)? + }; + if let Some(offset) = offset { // Partial cluster - zero it out. self.raw_file .file_mut() @@ -1695,7 +1704,7 @@ impl PunchHole for QcowFile { let mut offset = offset; while remaining > 0 { let chunk_length = min(remaining, std::usize::MAX as u64) as usize; - self.deallocate_bytes(offset, chunk_length)?; + self.zero_bytes(offset, chunk_length)?; remaining -= chunk_length as u64; offset += chunk_length as u64; } @@ -2083,6 +2092,39 @@ mod tests { }); } + #[test] + fn write_zeroes_backing() { + let disk_file = basic_file(&valid_header()); + let mut backing = QcowFile::from(disk_file).unwrap(); + // Write some test data. + let b = [0x55u8; 0x1000]; + backing + .seek(SeekFrom::Start(0xfff2000)) + .expect("Failed to seek."); + backing.write(&b).expect("Failed to write test string."); + let wrapping_disk_file = basic_file(&valid_header()); + let mut wrapping = QcowFile::from(wrapping_disk_file).unwrap(); + wrapping.set_backing_file(Some(Box::new(backing))); + // Overwrite the test data with zeroes. + // This should allocate new clusters in the wrapping file so that they can be zeroed. + wrapping + .seek(SeekFrom::Start(0xfff2000)) + .expect("Failed to seek."); + wrapping + .write_zeroes_all(0x200) + .expect("Failed to write zeroes."); + // Verify that the correct part of the data was zeroed out. + let mut buf = [0u8; 0x1000]; + wrapping + .seek(SeekFrom::Start(0xfff2000)) + .expect("Failed to seek."); + wrapping.read(&mut buf).expect("Failed to read."); + assert_eq!(buf[0], 0); + assert_eq!(buf[0x1FF], 0); + assert_eq!(buf[0x200], 0x55); + assert_eq!(buf[0xFFF], 0x55); + } + #[test] fn test_header() { with_basic_file(&valid_header(), |disk_file: File| {