block: qcow: Share the synchronous write path between the two engines

Both engines ran the same copy on write loop for a partial cluster
write. In the uring engine it was a separate helper, cow_write_sync.
In the sync engine the same loop sat directly in the write path. Move
cow_write_sync into common.rs and call it from both. The uring write
already runs synchronously, so its behavior does not change.

Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>

block/src/formats/qcow/common.rs

@@ -8,14 +8,17 @@
 
 //! Shared helpers for QCOW2 sync and async backends.
 
+use std::cmp::min;
 use std::io;
+use std::os::unix::fs::FileExt;
+use std::sync::Arc;
 
 use vmm_sys_util::write_zeroes::{PunchHole, WriteZeroesAt};
 
 use super::decoder::Decoder;
-use super::metadata::{BackingRead, DeallocAction, QcowMetadata};
+use super::metadata::{BackingRead, ClusterWriteMapping, DeallocAction, QcowMetadata};
 use super::qcow_raw_file::QcowRawFile;
-use crate::async_io::AsyncIoError;
+use crate::async_io::{AsyncIoError, AsyncIoOperation, AsyncIoResult};
 
 /// Decompress a full QCOW2 cluster from compressed data.
 ///
@@ -99,6 +102,62 @@ pub(super) fn deallocate_range_result(
     }
 }
 
+/// Writes an operation to the data file cluster by cluster, allocating
+/// and copying up backing data as needed. Writes are synchronous because
+/// the host offset is only known after the metadata allocation.
+pub(super) fn cow_write_sync(
+    address: u64,
+    op: &AsyncIoOperation,
+    metadata: &QcowMetadata,
+    data_file: &QcowRawFile,
+    backing_file: &Option<Arc<dyn BackingRead>>,
+    cluster_size: u64,
+) -> AsyncIoResult<()> {
+    let total_len = op.total_len();
+    let mut buf_offset = 0usize;
+
+    while buf_offset < total_len {
+        let curr_addr = address + buf_offset as u64;
+        let intra_offset = curr_addr & (cluster_size - 1);
+        let remaining_in_cluster = (cluster_size - intra_offset) as usize;
+        let count = min(total_len - buf_offset, remaining_in_cluster);
+
+        let backing_data = if let Some(backing) = backing_file
+            .as_ref()
+            .filter(|_| intra_offset != 0 || count < cluster_size as usize)
+        {
+            let cluster_begin = curr_addr - intra_offset;
+            let mut data = vec![0u8; cluster_size as usize];
+            backing
+                .read_at(cluster_begin, &mut data)
+                .map_err(AsyncIoError::WriteVectored)?;
+            Some(data)
+        } else {
+            None
+        };
+
+        let mapping = metadata
+            .map_cluster_for_write(curr_addr, backing_data)
+            .map_err(AsyncIoError::WriteVectored)?;
+
+        match mapping {
+            ClusterWriteMapping::Allocated {
+                offset: host_offset,
+            } => {
+                let mut buf = vec![0u8; count];
+                op.read_bytes_at(buf_offset, &mut buf)
+                    .map_err(AsyncIoError::WriteVectored)?;
+                data_file
+                    .file()
+                    .write_all_at(&buf, host_offset)
+                    .map_err(AsyncIoError::WriteVectored)?;
+            }
+        }
+        buf_offset += count;
+    }
+    Ok(())
+}
+
 #[cfg(test)]
 pub(crate) mod unit_tests {
     use std::fs::File;

block/src/formats/qcow/engine_sync.rs

@@ -4,15 +4,14 @@
 //
 // SPDX-License-Identifier: Apache-2.0 AND BSD-3-Clause
 
-use std::cmp::min;
 use std::os::unix::fs::FileExt;
 use std::sync::Arc;
 
 use vmm_sys_util::eventfd::EventFd;
 
-use super::common::{deallocate_range_result, decompress_cluster};
+use super::common::{cow_write_sync, deallocate_range_result, decompress_cluster};
 use super::decoder::Decoder;
-use super::metadata::{BackingRead, ClusterReadMapping, ClusterWriteMapping, QcowMetadata};
+use super::metadata::{BackingRead, ClusterReadMapping, QcowMetadata};
 use super::qcow_raw_file::QcowRawFile;
 use crate::async_io::{
     AsyncIo, AsyncIoCompletion, AsyncIoError, AsyncIoOperation, AsyncIoResult, CompletionCommon,
@@ -119,58 +118,6 @@ impl QcowSync {
 
         Ok(total_len)
     }
-
-    fn write_operation(&mut self, op: &AsyncIoOperation) -> AsyncIoResult<usize> {
-        let address = op.offset() as u64;
-        let total_len = op.total_len();
-        let mut buf_offset = 0usize;
-
-        while buf_offset < total_len {
-            let curr_addr = address + buf_offset as u64;
-            let intra_offset = curr_addr & (self.cluster_size - 1);
-            let remaining_in_cluster = (self.cluster_size - intra_offset) as usize;
-            let count = min(total_len - buf_offset, remaining_in_cluster);
-
-            // Read backing data for COW if this is a partial cluster
-            // write to an unallocated cluster with a backing file.
-            let backing_data = if let Some(backing) = self
-                .backing_file
-                .as_ref()
-                .filter(|_| intra_offset != 0 || count < self.cluster_size as usize)
-            {
-                let cluster_begin = curr_addr - intra_offset;
-                let mut data = vec![0u8; self.cluster_size as usize];
-                backing
-                    .read_at(cluster_begin, &mut data)
-                    .map_err(AsyncIoError::WriteVectored)?;
-                Some(data)
-            } else {
-                None
-            };
-
-            let mapping = self
-                .metadata
-                .map_cluster_for_write(curr_addr, backing_data)
-                .map_err(AsyncIoError::WriteVectored)?;
-
-            match mapping {
-                ClusterWriteMapping::Allocated {
-                    offset: host_offset,
-                } => {
-                    let mut buf = vec![0u8; count];
-                    op.read_bytes_at(buf_offset, &mut buf)
-                        .map_err(AsyncIoError::WriteVectored)?;
-                    self.data_file
-                        .file()
-                        .write_all_at(&buf, host_offset)
-                        .map_err(AsyncIoError::WriteVectored)?;
-                }
-            }
-            buf_offset += count;
-        }
-
-        Ok(total_len)
-    }
 }
 
 impl AsyncIo for QcowSync {
@@ -179,11 +126,18 @@ impl AsyncIo for QcowSync {
     }
 
     fn submit_data_operation(&mut self, mut op: AsyncIoOperation) -> AsyncIoResult<()> {
-        let is_read = op.is_read();
+        let total_len = if op.is_read() {
-        let total_len = if is_read {
             self.read_operation(&mut op)?
         } else {
-            self.write_operation(&op)?
+            cow_write_sync(
+                op.offset() as u64,
+                &op,
+                &self.metadata,
+                &self.data_file,
+                &self.backing_file,
+                self.cluster_size,
+            )?;
+            op.total_len()
         };
         self.completions
             .complete(AsyncIoCompletion::from_operation(op, total_len as i32));

block/src/formats/qcow/engine_uring.rs

@@ -8,7 +8,6 @@
 
 //! QCOW2 async disk backend.
 
-use std::cmp::min;
 use std::io;
 use std::os::unix::fs::FileExt;
 use std::os::unix::io::AsRawFd;
@@ -16,9 +15,9 @@ use std::sync::Arc;
 
 use vmm_sys_util::eventfd::EventFd;
 
-use super::common::{deallocate_range_result, decompress_cluster};
+use super::common::{cow_write_sync, deallocate_range_result, decompress_cluster};
 use super::decoder::Decoder;
-use super::metadata::{BackingRead, ClusterReadMapping, ClusterWriteMapping, QcowMetadata};
+use super::metadata::{BackingRead, ClusterReadMapping, QcowMetadata};
 use super::qcow_raw_file::QcowRawFile;
 use crate::async_io::{
     AsyncIo, AsyncIoCompletion, AsyncIoError, AsyncIoOperation, AsyncIoResult, UringDataIo,
@@ -118,7 +117,7 @@ impl QcowAsync {
         // write, L2 commit) with per request buffer lifetime tracking
         // and write ordering.
         let total_len = op.total_len();
-        if let Err(e) = Self::cow_write_sync(
+        if let Err(e) = cow_write_sync(
             op.offset() as u64,
             &op,
             &self.metadata,
@@ -352,60 +351,6 @@ impl QcowAsync {
         }
         Ok(())
     }
-
-    /// Write owned operation data cluster-by-cluster with COW from backing file.
-    fn cow_write_sync(
-        address: u64,
-        op: &AsyncIoOperation,
-        metadata: &QcowMetadata,
-        data_file: &QcowRawFile,
-        backing_file: &Option<Arc<dyn BackingRead>>,
-        cluster_size: u64,
-    ) -> AsyncIoResult<()> {
-        let total_len = op.total_len();
-        let mut buf_offset = 0usize;
-
-        while buf_offset < total_len {
-            let curr_addr = address + buf_offset as u64;
-            let intra_offset = curr_addr & (cluster_size - 1);
-            let remaining_in_cluster = (cluster_size - intra_offset) as usize;
-            let count = min(total_len - buf_offset, remaining_in_cluster);
-
-            let backing_data = if let Some(backing) = backing_file
-                .as_ref()
-                .filter(|_| intra_offset != 0 || count < cluster_size as usize)
-            {
-                let cluster_begin = curr_addr - intra_offset;
-                let mut data = vec![0u8; cluster_size as usize];
-                backing
-                    .read_at(cluster_begin, &mut data)
-                    .map_err(AsyncIoError::WriteVectored)?;
-                Some(data)
-            } else {
-                None
-            };
-
-            let mapping = metadata
-                .map_cluster_for_write(curr_addr, backing_data)
-                .map_err(AsyncIoError::WriteVectored)?;
-
-            match mapping {
-                ClusterWriteMapping::Allocated {
-                    offset: host_offset,
-                } => {
-                    let mut buf = vec![0u8; count];
-                    op.read_bytes_at(buf_offset, &mut buf)
-                        .map_err(AsyncIoError::WriteVectored)?;
-                    data_file
-                        .file()
-                        .write_all_at(&buf, host_offset)
-                        .map_err(AsyncIoError::WriteVectored)?;
-                }
-            }
-            buf_offset += count;
-        }
-        Ok(())
-    }
 }
 
 #[cfg(test)]