virtio-blk: add discard request support

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

@@ -319,6 +319,10 @@ impl VirtioBlock {
     ///
     /// The given file must be seekable and sizable.
     pub fn new(config: VirtioBlockConfig) -> Result<VirtioBlock, VirtioBlockError> {
+        if config.discard && config.is_read_only {
+            return Err(VirtioBlockError::DiscardReadOnlyUnsupported);
+        }
+
         if config.discard && config.file_engine_type == FileEngineType::Async {
             return Err(VirtioBlockError::DiscardAsyncUnsupported);
         }
@@ -896,6 +900,24 @@ mod tests {
             VirtioBlock::new(async_config),
             Err(VirtioBlockError::DiscardAsyncUnsupported)
         ));
+
+        let f = TempFile::new().unwrap();
+        f.as_file().set_len(0x1000).unwrap();
+        let read_only_config = VirtioBlockConfig {
+            drive_id: "test".to_string(),
+            path_on_host: f.as_path().to_str().unwrap().to_string(),
+            is_root_device: false,
+            partuuid: None,
+            is_read_only: true,
+            discard: true,
+            cache_type: CacheType::Unsafe,
+            rate_limiter: None,
+            file_engine_type: FileEngineType::Sync,
+        };
+        assert!(matches!(
+            VirtioBlock::new(read_only_config),
+            Err(VirtioBlockError::DiscardReadOnlyUnsupported)
+        ));
     }
 
     #[test]

src/vmm/src/devices/virtio/block/virtio/mod.rs

@@ -58,6 +58,8 @@ pub enum VirtioBlockError {
     InvalidOffset,
     /// Discard is not supported with the async IO engine.
     DiscardAsyncUnsupported,
+    /// Discard is not supported with read-only drives.
+    DiscardReadOnlyUnsupported,
     /// Guest gave us a read only descriptor that protocol says to write to.
     UnexpectedReadOnlyDescriptor,
     /// Guest gave us a write only descriptor that protocol says to read from.

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

@@ -10,7 +10,7 @@ use std::mem::size_of;
 
 use vm_memory::GuestMemoryError;
 
-use super::{io as block_io, VirtioBlockError, MAX_DISCARD_SECTORS, SECTOR_SHIFT, SECTOR_SIZE};
+use super::{MAX_DISCARD_SECTORS, SECTOR_SHIFT, SECTOR_SIZE, VirtioBlockError, io as block_io};
 use crate::devices::virtio::block::virtio::device::DiskProperties;
 use crate::devices::virtio::block::virtio::metrics::BlockDeviceMetrics;
 pub use crate::devices::virtio::generated::virtio_blk::{
@@ -19,7 +19,7 @@ pub use crate::devices::virtio::generated::virtio_blk::{
     VIRTIO_BLK_T_OUT,
 };
 use crate::devices::virtio::queue::DescriptorChain;
-use crate::logger::{error, IncMetric};
+use crate::logger::{IncMetric, error};
 use crate::rate_limiter::{RateLimiter, TokenType};
 use crate::vstate::memory::{Address, ByteValued, Bytes, GuestAddress, GuestMemoryMmap};
 
@@ -500,7 +500,7 @@ mod tests {
 
     use super::*;
     use crate::devices::virtio::queue::{Queue, VIRTQ_DESC_F_NEXT, VIRTQ_DESC_F_WRITE};
-    use crate::devices::virtio::test_utils::{default_mem, VirtQueue};
+    use crate::devices::virtio::test_utils::{VirtQueue, default_mem};
     use crate::vstate::memory::{Address, GuestAddress, GuestMemory};
 
     const NUM_DISK_SECTORS: u64 = 1024;
@@ -915,8 +915,8 @@ mod tests {
     }
 
     #[allow(clippy::let_with_type_underscore)]
-    fn random_request_parse(
-    ) -> impl Strategy<Value = (Result<Request, VirtioBlockError>, GuestMemoryMmap, Queue)> {
+    fn random_request_parse()
+    -> impl Strategy<Value = (Result<Request, VirtioBlockError>, GuestMemoryMmap, Queue)> {
         // In this strategy we are going to generate random Requests/Errors and map them
         // to an input descriptor chain.
         //

tests/framework/microvm.py

@@ -910,6 +910,7 @@ def add_drive(
         partuuid=None,
         cache_type=None,
         io_engine=None,
+        discard=None,
     ):
         """Add a block device."""
 
@@ -922,6 +923,7 @@ def add_drive(
             partuuid=partuuid,
             cache_type=cache_type,
             io_engine=io_engine,
+            discard=discard,
         )
         self.disks[drive_id] = path_on_host
 

tests/integration_tests/functional/test_api.py

@@ -635,6 +635,65 @@ def test_rate_limiters_api_config(uvm, io_engine):
     )
 
 
+@pin_guest_kernel(GUEST_KERNEL_DEFAULT)
+def test_discard_drive_api_config(uvm):
+    """
+    Test the discard API configuration and unsupported combinations.
+    """
+    test_microvm = uvm
+    test_microvm.spawn()
+
+    test_microvm.basic_config(add_root_device=False)
+
+    fs = drive_tools.FilesystemFile(os.path.join(test_microvm.fsfiles, "discard"))
+    test_microvm.api.drive.put(
+        drive_id="discard",
+        path_on_host=test_microvm.create_jailed_resource(fs.path),
+        is_root_device=False,
+        is_read_only=False,
+        io_engine="Sync",
+        discard=True,
+    )
+
+    vm_config = test_microvm.api.vm_config.get().json()
+    discard_drive = next(
+        drive for drive in vm_config["drives"] if drive["drive_id"] == "discard"
+    )
+    assert discard_drive["discard"] is True
+
+    fs_read_only = drive_tools.FilesystemFile(
+        os.path.join(test_microvm.fsfiles, "discard_read_only")
+    )
+    with pytest.raises(
+        RuntimeError,
+        match="Discard is not supported with read-only drives",
+    ):
+        test_microvm.api.drive.put(
+            drive_id="discard_read_only",
+            path_on_host=test_microvm.create_jailed_resource(fs_read_only.path),
+            is_root_device=False,
+            is_read_only=True,
+            io_engine="Sync",
+            discard=True,
+        )
+
+    fs_async = drive_tools.FilesystemFile(
+        os.path.join(test_microvm.fsfiles, "discard_async")
+    )
+    with pytest.raises(
+        RuntimeError,
+        match="Discard is not supported with the async IO engine",
+    ):
+        test_microvm.api.drive.put(
+            drive_id="discard_async",
+            path_on_host=test_microvm.create_jailed_resource(fs_async.path),
+            is_root_device=False,
+            is_read_only=False,
+            io_engine="Async",
+            discard=True,
+        )
+
+
 @pin_guest_kernel(GUEST_KERNEL_DEFAULT)
 def test_api_patch_pre_boot(uvm, io_engine):
     """
@@ -890,6 +949,7 @@ def _drive_patch(test_microvm, io_engine):
             "is_root_device": True,
             "cache_type": "Unsafe",
             "is_read_only": True,
+            "discard": False,
             "path_on_host": "/" + test_microvm.rootfs_file.name,
             "rate_limiter": None,
             "io_engine": "Sync",
@@ -901,6 +961,7 @@ def _drive_patch(test_microvm, io_engine):
             "is_root_device": False,
             "cache_type": "Unsafe",
             "is_read_only": False,
+            "discard": False,
             "path_on_host": "/scratch_new.ext4",
             "rate_limiter": {
                 "bandwidth": {"size": 5000, "one_time_burst": None, "refill_time": 100},
@@ -915,6 +976,7 @@ def _drive_patch(test_microvm, io_engine):
             "is_root_device": False,
             "cache_type": "Unsafe",
             "is_read_only": None,
+            "discard": None,
             "path_on_host": None,
             "rate_limiter": None,
             "io_engine": None,
@@ -1304,6 +1366,7 @@ def test_get_full_config_after_restoring_snapshot(microvm_factory, uvm_configure
             "is_root_device": True,
             "cache_type": "Unsafe",
             "is_read_only": True,
+            "discard": False,
             "path_on_host": f"/{uvm_configured.rootfs_file.name}",
             "rate_limiter": None,
             "io_engine": "Sync",
@@ -1444,6 +1507,7 @@ def test_get_full_config(uvm):
             "is_root_device": True,
             "cache_type": "Unsafe",
             "is_read_only": True,
+            "discard": False,
             "path_on_host": "/" + test_microvm.rootfs_file.name,
             "rate_limiter": None,
             "io_engine": "Sync",

tests/integration_tests/functional/test_drive_virtio.py

@@ -8,7 +8,12 @@
 
 import host_tools.drive as drive_tools
 from framework import utils
-from framework.artifacts import GUEST_KERNEL_DEFAULT, pin_guest_kernel, pin_rootfs_mode
+from framework.artifacts import (
+    GUEST_KERNEL_DEFAULT,
+    pin_guest_kernel,
+    pin_pci,
+    pin_rootfs_mode,
+)
 from framework.utils_drive import partuuid_and_disk_path
 
 MB = 1024 * 1024
@@ -324,6 +329,47 @@ def test_no_flush(uvm, io_engine):
     assert fc_metrics["block"]["flush_count"] == 0
 
 
+@pin_guest_kernel(GUEST_KERNEL_DEFAULT)
+@pin_pci(False)
+def test_discard(uvm):
+    """
+    Verify discard is advertised and punches holes in a file-backed drive.
+    """
+    test_microvm = uvm
+    test_microvm.spawn()
+
+    test_microvm.basic_config(vcpu_count=1)
+    test_microvm.add_net_iface()
+
+    fs = drive_tools.FilesystemFile(
+        os.path.join(test_microvm.fsfiles, "discard"), size=64
+    )
+    test_microvm.add_drive("discard", fs.path, io_engine="Sync", discard=True)
+
+    test_microvm.start()
+
+    _, stdout, stderr = test_microvm.ssh.run(
+        "cat /sys/block/vdb/queue/discard_granularity"
+    )
+    assert stderr == ""
+    assert int(stdout.strip()) > 0
+
+    _, _, stderr = test_microvm.ssh.run(
+        "dd if=/dev/zero of=/dev/vdb bs=1M count=32 oflag=direct conv=fsync status=none",
+        timeout=30.0,
+    )
+    assert stderr == ""
+
+    blocks_before = os.stat(fs.path).st_blocks
+    assert blocks_before > 0
+
+    _, _, stderr = test_microvm.ssh.run("blkdiscard -f /dev/vdb", timeout=30.0)
+    assert stderr in ("", "blkdiscard: Operation forced, data will be lost!\n")
+
+    blocks_after = os.stat(fs.path).st_blocks
+    assert blocks_after < blocks_before
+
+
 @pin_guest_kernel(GUEST_KERNEL_DEFAULT)
 @pin_rootfs_mode("rw")
 def test_flush(uvm, io_engine):