Make virtio flush disabled by default

src/api_server/src/parsed_request.rs

@@ -614,7 +614,7 @@ pub(crate) mod tests {
             .write_all(
                 b"PUT /drives/string HTTP/1.1\r\n\
                 Content-Type: application/json\r\n\
-                Content-Length: 266\r\n\r\n{ \
+                Content-Length: 287\r\n\r\n{ \
                 \"drive_id\": \"string\", \
                 \"path_on_host\": \"string\", \
                 \"is_root_device\": true, \
@@ -631,7 +631,8 @@ pub(crate) mod tests {
                         \"one_time_burst\": 0, \
                         \"refill_time\": 0 \
                     } \
-                } \
+                }, \
+                \"want_flush\": false \
             }",
             )
             .unwrap();

src/api_server/src/request/drive.rs

@@ -249,7 +249,8 @@ mod tests {
                     "one_time_burst": 0,
                     "refill_time": 0
                     }
-                }
+                },
+                "want_flush": false
             }"#;
         assert!(parse_put_drive(&Body::new(body), Some(&"1000")).is_ok());
 

src/api_server/swagger/firecracker.yaml

@@ -556,6 +556,9 @@ definitions:
         description: Host level path for the guest drive
       rate_limiter:
         $ref: "#/definitions/RateLimiter"
+      want_flush:
+        type: boolean
+        description: advertise synchronous virtio flush capability
 
   Error:
     type: object

src/devices/src/virtio/block/device.rs

@@ -159,10 +159,15 @@ impl Block {
         is_disk_read_only: bool,
         is_disk_root: bool,
         rate_limiter: RateLimiter,
+        want_flush: bool,
     ) -> io::Result<Block> {
         let disk_properties = DiskProperties::new(disk_image_path, is_disk_read_only)?;
 
-        let mut avail_features = (1u64 << VIRTIO_F_VERSION_1) | (1u64 << VIRTIO_BLK_F_FLUSH);
+        let mut avail_features = 1u64 << VIRTIO_F_VERSION_1;
+
+        if want_flush {
+            avail_features |= 1u64 << VIRTIO_BLK_F_FLUSH;
+        };
 
         if is_disk_read_only {
             avail_features |= 1u64 << VIRTIO_BLK_F_RO;
@@ -443,7 +448,8 @@ pub(crate) mod tests {
 
     use crate::check_metric_after_block;
     use crate::virtio::block::test_utils::{
-        default_block, invoke_handler_for_queue_event, set_queue, set_rate_limiter,
+        default_block, default_block_flush, invoke_handler_for_queue_event, set_queue,
+        set_rate_limiter,
     };
     use crate::virtio::test_utils::{default_mem, initialize_virtqueue, VirtQueue};
 
@@ -476,7 +482,7 @@ pub(crate) mod tests {
 
         assert_eq!(block.device_type(), TYPE_BLOCK);
 
-        let features: u64 = (1u64 << VIRTIO_F_VERSION_1) | (1u64 << VIRTIO_BLK_F_FLUSH);
+        let features: u64 = 1u64 << VIRTIO_F_VERSION_1;
 
         assert_eq!(block.avail_features_by_page(0), features as u32);
         assert_eq!(block.avail_features_by_page(1), (features >> 32) as u32);
@@ -714,8 +720,10 @@ pub(crate) mod tests {
         }
     }
 
+    // Verify injected flush request does not hang when feature not supported
     #[test]
-    fn test_flush() {
+    fn test_flush_default_nohang() {
+        // default block device has flush disabled
         let mut block = default_block();
         let mem = default_mem();
         let vq = VirtQueue::new(GuestAddress(0), &mem, 16);
@@ -757,6 +765,49 @@ pub(crate) mod tests {
         }
     }
 
+    // Verify injected flush request does not hang when feature enabled
+    #[test]
+    fn test_flush_enabled_nohang() {
+        let mut block = default_block_flush();
+        let mem = default_mem();
+        let vq = VirtQueue::new(GuestAddress(0), &mem, 16);
+        set_queue(&mut block, 0, vq.create_queue());
+        block.activate(mem.clone()).unwrap();
+        initialize_virtqueue(&vq);
+
+        let request_type_addr = GuestAddress(vq.dtable[0].addr.get());
+        let status_addr = GuestAddress(vq.dtable[2].addr.get());
+
+        // Flush completes successfully without a data descriptor.
+        {
+            vq.dtable[0].next.set(2);
+
+            mem.write_obj::<u32>(VIRTIO_BLK_T_FLUSH, request_type_addr)
+                .unwrap();
+
+            invoke_handler_for_queue_event(&mut block);
+            assert_eq!(vq.used.idx.get(), 1);
+            assert_eq!(vq.used.ring[0].get().id, 0);
+            assert_eq!(vq.used.ring[0].get().len, 0);
+            assert_eq!(mem.read_obj::<u32>(status_addr).unwrap(), VIRTIO_BLK_S_OK);
+        }
+
+        // Flush completes successfully even with a data descriptor.
+        {
+            vq.used.idx.set(0);
+            set_queue(&mut block, 0, vq.create_queue());
+            vq.dtable[0].next.set(1);
+
+            mem.write_obj::<u32>(VIRTIO_BLK_T_FLUSH, request_type_addr)
+                .unwrap();
+
+            invoke_handler_for_queue_event(&mut block);
+            assert_eq!(vq.used.idx.get(), 1);
+            assert_eq!(vq.used.ring[0].get().id, 0);
+            assert_eq!(vq.used.ring[0].get().len, 0);
+            assert_eq!(mem.read_obj::<u32>(status_addr).unwrap(), VIRTIO_BLK_S_OK);
+        }
+    }
     #[test]
     fn test_get_device_id() {
         let mut block = default_block();

src/devices/src/virtio/block/persist.rs

@@ -11,7 +11,7 @@ use rate_limiter::{persist::RateLimiterState, RateLimiter};
 use snapshot::Persist;
 use versionize::{VersionMap, Versionize, VersionizeResult};
 use versionize_derive::Versionize;
-use virtio_gen::virtio_blk::VIRTIO_BLK_F_RO;
+use virtio_gen::virtio_blk::{VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_RO};
 use vm_memory::GuestMemoryMmap;
 
 use super::*;
@@ -46,6 +46,7 @@ impl Persist<'_> for Block {
             disk_path: self.disk.file_path().clone(),
             virtio_state: VirtioDeviceState::from_device(self),
             rate_limiter_state: self.rate_limiter.save(),
+            // has_flush will come from advertised features
         }
     }
 
@@ -55,6 +56,7 @@ impl Persist<'_> for Block {
     ) -> Result<Self, Self::Error> {
         let is_disk_read_only = state.virtio_state.avail_features & (1u64 << VIRTIO_BLK_F_RO) != 0;
         let rate_limiter = RateLimiter::restore((), &state.rate_limiter_state)?;
+        let has_flush = state.virtio_state.avail_features & (1u64 << VIRTIO_BLK_F_FLUSH) != 0;
 
         let mut block = Block::new(
             state.id.clone(),
@@ -63,6 +65,7 @@ impl Persist<'_> for Block {
             is_disk_read_only,
             state.root_device,
             rate_limiter,
+            has_flush,
         )?;
 
         block.queues = state
@@ -104,6 +107,7 @@ mod tests {
             false,
             false,
             RateLimiter::default(),
+            false,
         )
         .unwrap();
         let guest_mem = default_mem();

src/devices/src/virtio/block/request.rs

@@ -22,6 +22,7 @@ use super::{Error, SECTOR_SHIFT, SECTOR_SIZE};
 pub enum ExecuteError {
     BadRequest(Error),
     Flush(io::Error),
+    SyncAll(io::Error),
     Read(GuestMemoryError),
     Seek(io::Error),
     Write(GuestMemoryError),
@@ -33,6 +34,7 @@ impl ExecuteError {
         match *self {
             ExecuteError::BadRequest(_) => VIRTIO_BLK_S_IOERR,
             ExecuteError::Flush(_) => VIRTIO_BLK_S_IOERR,
+            ExecuteError::SyncAll(_) => VIRTIO_BLK_S_IOERR,
             ExecuteError::Read(_) => VIRTIO_BLK_S_IOERR,
             ExecuteError::Seek(_) => VIRTIO_BLK_S_IOERR,
             ExecuteError::Write(_) => VIRTIO_BLK_S_IOERR,
@@ -227,13 +229,13 @@ impl Request {
                 METRICS.block.write_bytes.add(self.data_len as usize);
                 METRICS.block.write_count.inc();
             }
-            RequestType::Flush => match diskfile.flush() {
-                Ok(_) => {
-                    METRICS.block.flush_count.inc();
-                    return Ok(0);
-                }
-                Err(e) => return Err(ExecuteError::Flush(e)),
-            },
+            RequestType::Flush => {
+                // flush() first to force any cached data out
+                diskfile.flush().map_err(ExecuteError::Flush)?;
+                // sync data out to physical media on host
+                diskfile.sync_all().map_err(ExecuteError::SyncAll)?;
+                METRICS.block.flush_count.inc();
+            }
             RequestType::GetDeviceID => {
                 let disk_id = disk.image_id();
                 if (self.data_len as usize) < disk_id.len() {

src/devices/src/virtio/block/test_utils.rs

@@ -18,14 +18,32 @@ pub fn default_block() -> Block {
     default_block_with_path(f.as_path().to_str().unwrap().to_string())
 }
 
+/// Create a Block instance with flush enabled
+pub fn default_block_flush() -> Block {
+    // Create backing file.
+    let f = TempFile::new().unwrap();
+    f.as_file().set_len(0x1000).unwrap();
+
+    default_block_with_path_flush(f.as_path().to_str().unwrap().to_string())
+}
+
 /// Create a default Block instance using file at the specified path to be used in tests.
 pub fn default_block_with_path(path: String) -> Block {
     // Rate limiting is enabled but with a high operation rate (10 million ops/s).
     let rate_limiter = RateLimiter::new(0, 0, 0, 100_000, 0, 10).unwrap();
 
     let id = "test".to_string();
-    // The default block device is read-write and non-root.
-    Block::new(id, None, path, false, false, rate_limiter).unwrap()
+    // The default block device is read-write, non-root with flush disabled
+    Block::new(id, None, path, false, false, rate_limiter, false).unwrap()
+}
+
+/// Create a default Block instance using file at the specified path with flush enabled
+pub fn default_block_with_path_flush(path: String) -> Block {
+    // Rate limiting is enabled but with a high operation rate (10 million ops/s).
+    let rate_limiter = RateLimiter::new(0, 0, 0, 100_000, 0, 10).unwrap();
+
+    let id = "test".to_string();
+    Block::new(id, None, path, false, false, rate_limiter, true).unwrap()
 }
 
 pub fn invoke_handler_for_queue_event(b: &mut Block) {

src/vmm/Cargo.toml

@@ -26,6 +26,7 @@ rate_limiter = { path = "../rate_limiter" }
 seccomp = { path = "../seccomp" }
 snapshot = { path = "../snapshot"}
 utils = { path = "../utils" }
+virtio_gen = { path = "../virtio_gen" }
 
 [target.'cfg(target_arch = "x86_64")'.dependencies]
 cpuid = { path = "../cpuid" }

src/vmm/src/builder.rs

@@ -811,6 +811,7 @@ pub mod tests {
         is_root_device: bool,
         partuuid: Option<String>,
         is_read_only: bool,
+        want_flush: bool,
     }
 
     impl CustomBlockConfig {
@@ -819,12 +820,14 @@ pub mod tests {
             is_root_device: bool,
             partuuid: Option<String>,
             is_read_only: bool,
+            want_flush: bool,
         ) -> Self {
             CustomBlockConfig {
                 drive_id,
                 is_root_device,
                 partuuid,
                 is_read_only,
+                want_flush,
             }
         }
     }
@@ -906,6 +909,7 @@ pub mod tests {
                 partuuid: custom_block_cfg.partuuid.clone(),
                 is_read_only: custom_block_cfg.is_read_only,
                 rate_limiter: None,
+                want_flush: Some(custom_block_cfg.want_flush),
             };
             block_dev_configs.insert(block_device_config).unwrap();
         }
@@ -1061,7 +1065,13 @@ pub mod tests {
         // Use case 1: root block device is not specified through PARTUUID.
         {
             let drive_id = String::from("root");
-            let block_configs = vec![CustomBlockConfig::new(drive_id.clone(), true, None, true)];
+            let block_configs = vec![CustomBlockConfig::new(
+                drive_id.clone(),
+                true,
+                None,
+                true,
+                false,
+            )];
             let mut vmm = default_vmm();
             let mut cmdline = default_kernel_cmdline();
             insert_block_devices(&mut vmm, &mut cmdline, &mut event_manager, block_configs);
@@ -1080,6 +1090,7 @@ pub mod tests {
                 true,
                 Some("0eaa91a0-01".to_string()),
                 false,
+                false,
             )];
             let mut vmm = default_vmm();
             let mut cmdline = default_kernel_cmdline();
@@ -1099,6 +1110,7 @@ pub mod tests {
                 false,
                 Some("0eaa91a0-01".to_string()),
                 false,
+                false,
             )];
             let mut vmm = default_vmm();
             let mut cmdline = default_kernel_cmdline();
@@ -1119,9 +1131,10 @@ pub mod tests {
                     true,
                     Some("0eaa91a0-01".to_string()),
                     false,
+                    false,
                 ),
-                CustomBlockConfig::new(String::from("secondary"), false, None, true),
-                CustomBlockConfig::new(String::from("third"), false, None, false),
+                CustomBlockConfig::new(String::from("secondary"), false, None, true, false),
+                CustomBlockConfig::new(String::from("third"), false, None, false, false),
             ];
             let mut vmm = default_vmm();
             let mut cmdline = default_kernel_cmdline();
@@ -1151,7 +1164,13 @@ pub mod tests {
         // Use case 5: root block device is rw.
         {
             let drive_id = String::from("root");
-            let block_configs = vec![CustomBlockConfig::new(drive_id.clone(), true, None, false)];
+            let block_configs = vec![CustomBlockConfig::new(
+                drive_id.clone(),
+                true,
+                None,
+                false,
+                false,
+            )];
             let mut vmm = default_vmm();
             let mut cmdline = default_kernel_cmdline();
             insert_block_devices(&mut vmm, &mut cmdline, &mut event_manager, block_configs);
@@ -1170,6 +1189,7 @@ pub mod tests {
                 true,
                 Some("0eaa91a0-01".to_string()),
                 true,
+                false,
             )];
             let mut vmm = default_vmm();
             let mut cmdline = default_kernel_cmdline();
@@ -1180,6 +1200,26 @@ pub mod tests {
                 .get_device(DeviceType::Virtio(TYPE_BLOCK), drive_id.as_str())
                 .is_some());
         }
+
+        // Use case 7: root block device is rw with flush supported
+        {
+            let drive_id = String::from("root");
+            let block_configs = vec![CustomBlockConfig::new(
+                drive_id.clone(),
+                true,
+                None,
+                false,
+                true,
+            )];
+            let mut vmm = default_vmm();
+            let mut cmdline = default_kernel_cmdline();
+            insert_block_devices(&mut vmm, &mut cmdline, &mut event_manager, block_configs);
+            assert!(cmdline.as_str().contains("root=/dev/vda rw"));
+            assert!(vmm
+                .mmio_device_manager
+                .get_device(DeviceType::Virtio(TYPE_BLOCK), drive_id.as_str())
+                .is_some());
+        }
     }
 
     #[test]

src/vmm/src/default_syscalls/filters.rs

@@ -162,6 +162,7 @@ pub fn default_filter() -> Result<SeccompFilter, Error> {
                 or![and![Cond::new(1, ArgLen::DWORD, Eq, 0u64)?],],
             ),
             allow_syscall(libc::SYS_write),
+            allow_syscall(libc::SYS_fsync),
         ]
         .into_iter()
         .collect(),