support NVMe Deallocate

bin/propolis-server/src/lib/stats/virtual_disk.rs

@@ -77,9 +77,9 @@ impl VirtualDiskStats {
                 self.on_write_completion(result, len, duration)
             }
             Operation::Flush => self.on_flush_completion(result, duration),
-            Operation::Discard(..) => {
-                // Discard is not wired up in backends we care about for now, so
-                // it can safely be ignored.
+            Operation::Discard => {
+                // Discard is now wired up for local disks. We need to add support for it to the
+                // schema in Omicron before we can report stats for it. For now, just ignore it.
             }
         }
     }

lib/propolis/Cargo.toml

@@ -44,6 +44,7 @@ async-trait.workspace = true
 iddqd.workspace = true
 nix.workspace = true
 vm-attest.workspace = true
+itertools.workspace = true
 
 # falcon
 libloading = { workspace = true, optional = true }

lib/propolis/src/block/crucible.rs

@@ -145,9 +145,11 @@ impl WorkerState {
                     let _ = block.flush(None).await?;
                 }
             }
-            block::Operation::Discard(..) => {
-                // Crucible does not support discard operations for now
-                return Err(Error::Unsupported);
+            block::Operation::Discard => {
+                // Crucible does not support discard operations for now, so we implement this as
+                // a no-op (which technically is a valid implementation of discard, just one that
+                // doesn't actually free any space).
+                return Ok(());
             }
         }
         Ok(())

lib/propolis/src/block/file.rs

@@ -113,12 +113,20 @@ impl SharedState {
                     self.fp.sync_data().map_err(|_| "io error")?;
                 }
             }
-            block::Operation::Discard(off, len) => {
+            block::Operation::Discard => {
                 if let Some(mech) = self.discard_mech {
-                    dkioc::do_discard(&self.fp, mech, off as u64, len as u64)
+                    for &(off, len) in &req.ranges {
+                        // There might be some performance benefits to combining the ranges into
+                        // one DKIOCFREE call, but ZFS will only issue one range to the
+                        // underlying disk at a time, so we expect the benefit to be minimal in
+                        // practice.
+                        dkioc::do_discard(
+                            &self.fp, mech, off as u64, len as u64,
+                        )
                         .map_err(|_| {
-                        "io error while attempting to free block(s)"
-                    })?;
+                            "io error while attempting to free block(s)"
+                        })?;
+                    }
                 } else {
                     unreachable!("handled above in processing_loop()");
                 }

lib/propolis/src/block/in_memory.rs

@@ -86,7 +86,7 @@ impl SharedState {
             block::Operation::Flush => {
                 // nothing to do
             }
-            block::Operation::Discard(..) => {
+            block::Operation::Discard => {
                 unreachable!("handled in processing_loop()");
             }
         }

lib/propolis/src/block/mem_async.rs

@@ -89,7 +89,7 @@ impl SharedState {
             block::Operation::Flush => {
                 // nothing to do
             }
-            block::Operation::Discard(..) => {
+            block::Operation::Discard => {
                 unreachable!("handled in processing_loop()")
             }
         }

lib/propolis/src/block/minder.rs

@@ -283,9 +283,9 @@ impl QueueMinder {
                 Operation::Flush => {
                     probes::block_begin_flush!(|| { (devqid, id) });
                 }
-                Operation::Discard(off, len) => {
+                Operation::Discard => {
                     probes::block_begin_discard!(|| {
-                        (devqid, id, off as u64, len as u64)
+                        (devqid, id, req.ranges.len() as u64)
                     });
                 }
             }
@@ -355,7 +355,7 @@ impl QueueMinder {
                     (devqid, id, rescode, ns_processed, ns_queued)
                 });
             }
-            Operation::Discard(..) => {
+            Operation::Discard => {
                 probes::block_complete_discard!(|| {
                     (devqid, id, rescode, ns_processed, ns_queued)
                 });

lib/propolis/src/block/mod.rs

@@ -50,7 +50,7 @@ mod probes {
     fn block_begin_read(devq_id: u64, req_id: u64, offset: u64, len: u64) {}
     fn block_begin_write(devq_id: u64, req_id: u64, offset: u64, len: u64) {}
     fn block_begin_flush(devq_id: u64, req_id: u64) {}
-    fn block_begin_discard(devq_id: u64, req_id: u64, offset: u64, len: u64) {}
+    fn block_begin_discard(devq_id: u64, req_id: u64, nr: u64) {}
 
     fn block_complete_read(
         devq_id: u64,
@@ -106,8 +106,8 @@ pub enum Operation {
     Write(ByteOffset, ByteLen),
     /// Flush buffer(s)
     Flush,
-    /// Discard/UNMAP/deallocate region
-    Discard(ByteOffset, ByteLen),
+    /// Discard/UNMAP/deallocate some ranges, which are specified in Request::ranges
+    Discard,
 }
 impl Operation {
     pub const fn is_read(&self) -> bool {
@@ -120,7 +120,7 @@ impl Operation {
         matches!(self, Operation::Flush)
     }
     pub const fn is_discard(&self) -> bool {
-        matches!(self, Operation::Discard(..))
+        matches!(self, Operation::Discard)
     }
 }
 
@@ -203,32 +203,36 @@ pub struct Request {
     /// A list of regions of guest memory to read/write into as part of the I/O
     /// request
     pub regions: Vec<GuestRegion>,
+
+    /// A list of byte ranges to discard as part of the I/O request.  This is only
+    /// relevant for discard operations, and is expected to be empty otherwise.
+    pub ranges: Vec<(ByteOffset, ByteLen)>,
 }
 impl Request {
     pub fn new_read(
         off: ByteOffset,
         len: ByteLen,
         regions: Vec<GuestRegion>,
     ) -> Self {
-        Self { op: Operation::Read(off, len), regions }
+        Self { op: Operation::Read(off, len), regions, ranges: Vec::new() }
     }
 
     pub fn new_write(
         off: ByteOffset,
         len: ByteLen,
         regions: Vec<GuestRegion>,
     ) -> Self {
-        Self { op: Operation::Write(off, len), regions }
+        Self { op: Operation::Write(off, len), regions, ranges: Vec::new() }
     }
 
     pub fn new_flush() -> Self {
         let op = Operation::Flush;
-        Self { op, regions: Vec::new() }
+        Self { op, regions: Vec::new(), ranges: Vec::new() }
     }
 
-    pub fn new_discard(off: ByteOffset, len: ByteLen) -> Self {
-        let op = Operation::Discard(off, len);
-        Self { op, regions: Vec::new() }
+    pub fn new_discard(ranges: Vec<(ByteOffset, ByteLen)>) -> Self {
+        let op = Operation::Discard;
+        Self { op, regions: Vec::new(), ranges }
     }
 
     pub fn mappings<'a>(&self, mem: &'a MemCtx) -> Option<Vec<SubMapping<'a>>> {
@@ -239,7 +243,7 @@ impl Request {
             Operation::Write(..) => {
                 self.regions.iter().map(|r| mem.readable_region(r)).collect()
             }
-            Operation::Flush | Operation::Discard(..) => None,
+            Operation::Flush | Operation::Discard => None,
         }
     }
 }

lib/propolis/src/hw/nvme/bits.rs

@@ -4,6 +4,7 @@
 
 #![allow(dead_code)]
 
+use crate::block::{ByteLen, ByteOffset};
 use bitstruct::bitstruct;
 use zerocopy::FromBytes;
 
@@ -176,6 +177,50 @@ impl CompletionQueueEntry {
     }
 }
 
+/// A Dataset Management Range Definition as represented in memory.
+///
+/// See NVMe 1.0e Section 6.6 Figure 114: Dataset Management – Range Definition
+#[derive(Debug, Default, Copy, Clone, PartialEq, Eq, FromBytes)]
+#[repr(C, packed(1))]
+pub struct DatasetManagementRangeDefinition {
+    /// The context attributes specified for each range provides information about how the range
+    /// is intended to be used by host software. The use of this information is optional and the
+    /// controller is not required to perform any specific action.
+    pub context_attributes: u32,
+
+    pub number_logical_blocks: u32,
+
+    pub starting_lba: u64,
+}
+impl DatasetManagementRangeDefinition {
+    pub fn new(
+        context_attributes: u32,
+        number_logical_blocks: u32,
+        starting_lba: u64,
+    ) -> Self {
+        Self { context_attributes, number_logical_blocks, starting_lba }
+    }
+
+    pub fn offset_len(
+        &self,
+        lba_data_size: u64,
+    ) -> Result<(ByteOffset, ByteLen), &'static str> {
+        // Check for overflow in the byte offset calculation
+        let byte_offset = self.starting_lba.checked_mul(lba_data_size).ok_or(
+            "Starting LBA and LBA data size multiplication overflowed",
+        )?;
+        // Check for overflow in the byte length calculation
+        let byte_len = (u64::from(self.number_logical_blocks))
+                .checked_mul(lba_data_size)
+                .ok_or("Number of logical blocks and LBA data size multiplication overflowed")?;
+        // Check for overflow of offset + length
+        byte_offset
+            .checked_add(byte_len)
+            .ok_or("Byte offset and byte length addition overflowed")?;
+        Ok((byte_offset as ByteOffset, byte_len as ByteLen))
+    }
+}
+
 // Register bits
 
 bitstruct! {
@@ -539,6 +584,8 @@ pub const NVM_OPC_FLUSH: u8 = 0x00;
 pub const NVM_OPC_WRITE: u8 = 0x01;
 /// Read Command Opcode
 pub const NVM_OPC_READ: u8 = 0x02;
+/// Dataset Mangement Command Opcode
+pub const NVM_OPC_DATASET_MANAGEMENT: u8 = 0x09;
 
 // Generic Command Status values
 // See NVMe 1.0e Section 4.5.1.2.1, Figure 17 Status Code - Generic Command Status Values
@@ -1195,5 +1242,6 @@ mod test {
         assert_eq!(size_of::<IdentifyController>(), 4096);
         assert_eq!(size_of::<LbaFormat>(), 4);
         assert_eq!(size_of::<IdentifyNamespace>(), 4096);
+        assert_eq!(size_of::<DatasetManagementRangeDefinition>(), 16);
     }
 }