add NVMe Discard Oximeter metrics

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

@@ -26,8 +26,9 @@ use propolis::block::{self, Operation};
 
 pub use self::virtual_disk::VirtualDisk;
 use self::virtual_disk::{
-    BytesRead, BytesWritten, FailedFlushes, FailedReads, FailedWrites, Flushes,
-    IoLatency, IoSize, Reads, Writes,
+    BytesDiscarded, BytesRead, BytesWritten, Discards, FailedDiscards,
+    FailedFlushes, FailedReads, FailedWrites, Flushes, IoLatency, IoSize,
+    Reads, Writes,
 };
 
 /// Type for tracking virtual disk stats.
@@ -52,6 +53,12 @@ struct VirtualDiskStats {
     bytes_written: BytesWritten,
     /// Cumulative number of failed writes, by failure reason.
     failed_writes: [FailedWrites; N_FAILURE_KINDS],
+    /// Cumulative number of discards.
+    discards: Discards,
+    /// Cumulative number of bytes discarded.
+    bytes_discarded: BytesDiscarded,
+    /// Cumulative number of failed discards, by failure reason.
+    failed_discards: [FailedDiscards; N_FAILURE_KINDS],
     /// Cumulative number of flushes.
     flushes: Flushes,
     /// Cumulative number of failed flushes, by failure reason.
@@ -77,9 +84,8 @@ impl VirtualDiskStats {
                 self.on_write_completion(result, len, duration)
             }
             Operation::Flush => self.on_flush_completion(result, duration),
-            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.
+            Operation::Discard(bytes) => {
+                self.on_discard_completion(result, bytes, duration)
             }
         }
     }
@@ -130,6 +136,29 @@ impl VirtualDiskStats {
         self.failed_writes[index].datum.increment();
     }
 
+    fn on_discard_completion(
+        &mut self,
+        result: block::Result,
+        bytes: usize,
+        duration: Duration,
+    ) {
+        let index = match result {
+            block::Result::Success => {
+                let _ = self.io_latency[DISCARD_INDEX]
+                    .datum
+                    .sample(duration.as_nanos() as u64);
+                let _ = self.io_size[DISCARD_INDEX].datum.sample(bytes as u64);
+                self.discards.datum += 1;
+                self.bytes_discarded.datum += bytes as u64;
+                return;
+            }
+            block::Result::Failure => FAILURE_INDEX,
+            block::Result::ReadOnly => READONLY_INDEX,
+            block::Result::Unsupported => UNSUPPORTED_INDEX,
+        };
+        self.failed_discards[index].datum.increment();
+    }
+
     fn on_flush_completion(
         &mut self,
         result: block::Result,
@@ -152,16 +181,18 @@ impl VirtualDiskStats {
 }
 
 /// Number of I/O kinds we track.
-const N_IO_KINDS: usize = 3;
+const N_IO_KINDS: usize = 4;
 
 /// Indices into arrays tracking operations broken out by I/O kind.
 const READ_INDEX: usize = 0;
 const WRITE_INDEX: usize = 1;
-const FLUSH_INDEX: usize = 2;
+const DISCARD_INDEX: usize = 2;
+const FLUSH_INDEX: usize = 3; // Note that flush must be last since it does not have a size histogram (io_size)
 
 /// String representations of I/O kinds we report to Oximeter.
 const READ_KIND: &str = "read";
 const WRITE_KIND: &str = "write";
+const DISCARD_KIND: &str = "discard";
 const FLUSH_KIND: &str = "flush";
 
 /// Number of failure kinds we track.
@@ -229,6 +260,16 @@ impl BlockMetrics {
                 FailedWrites { failure_reason: READONLY_KIND.into(), datum },
                 FailedWrites { failure_reason: UNSUPPORTED_KIND.into(), datum },
             ],
+            discards: Discards { datum },
+            bytes_discarded: BytesDiscarded { datum },
+            failed_discards: [
+                FailedDiscards { failure_reason: FAILURE_KIND.into(), datum },
+                FailedDiscards { failure_reason: READONLY_KIND.into(), datum },
+                FailedDiscards {
+                    failure_reason: UNSUPPORTED_KIND.into(),
+                    datum,
+                },
+            ],
             flushes: Flushes { datum },
             failed_flushes: [
                 FailedFlushes { failure_reason: FAILURE_KIND.into(), datum },
@@ -247,6 +288,10 @@ impl BlockMetrics {
                     io_kind: WRITE_KIND.into(),
                     datum: latency_histogram.clone(),
                 },
+                IoLatency {
+                    io_kind: DISCARD_KIND.into(),
+                    datum: latency_histogram.clone(),
+                },
                 IoLatency {
                     io_kind: FLUSH_KIND.into(),
                     datum: latency_histogram.clone(),
@@ -261,6 +306,10 @@ impl BlockMetrics {
                     io_kind: WRITE_KIND.into(),
                     datum: size_histogram.clone(),
                 },
+                IoSize {
+                    io_kind: DISCARD_KIND.into(),
+                    datum: size_histogram.clone(),
+                },
             ],
         };
         let mut sample_buffer = Vec::with_capacity(max_queues.get());
@@ -354,10 +403,10 @@ impl Producer for VirtualDiskProducer {
         // Consolidate any buffer samples first
         self.0.consolidate_all();
 
-        // 5 scalar samples (reads, writes, flushes, bytes read / written)
-        // 3 scalars broken out by failure kind
+        // 7 scalar samples (reads, writes, discards, flushes, bytes read / written / discarded)
+        // 4 scalars broken out by failure kind (reads, writes, discards, flushes)
         // 2 histograms broken out by I/O kind
-        const N_SAMPLES: usize = 5 + 3 * N_FAILURE_KINDS + 2 * N_IO_KINDS;
+        const N_SAMPLES: usize = 7 + 4 * N_FAILURE_KINDS + 2 * N_IO_KINDS;
         let mut out = Vec::with_capacity(N_SAMPLES);
         let stats = self.0.stats.lock().unwrap();
 
@@ -375,6 +424,13 @@ impl Producer for VirtualDiskProducer {
             out.push(Sample::new(&stats.disk, failed)?);
         }
 
+        // Discard statistics.
+        out.push(Sample::new(&stats.disk, &stats.discards)?);
+        out.push(Sample::new(&stats.disk, &stats.bytes_discarded)?);
+        for failed in stats.failed_discards.iter() {
+            out.push(Sample::new(&stats.disk, failed)?);
+        }
+
         // Flushes
         out.push(Sample::new(&stats.disk, &stats.flushes)?);
         for failed in stats.failed_flushes.iter() {

lib/propolis/src/block/crucible.rs

@@ -146,7 +146,7 @@ impl WorkerState {
                     let _ = block.flush(None).await?;
                 }
             }
-            block::Operation::Discard => {
+            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).

lib/propolis/src/block/file.rs

@@ -117,7 +117,7 @@ impl SharedState {
                     self.fp.sync_data().map_err(|_| "io error")?;
                 }
             }
-            block::Operation::Discard => {
+            block::Operation::Discard(_bytes) => {
                 if let Some(mech) = self.discard_mech {
                     for &(off, len) in &req.ranges {
                         // There might be some performance benefits to combining the ranges into

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 => {
+                Operation::Discard(bytes) => {
                     probes::block_begin_discard!(|| {
-                        (devqid, id, req.ranges.len() as u64)
+                        (devqid, id, req.ranges.len() as u64, bytes 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, nr: u64) {}
+    fn block_begin_discard(devq_id: u64, req_id: u64, nr: u64, bytes: u64) {}
 
     fn block_complete_read(
         devq_id: u64,
@@ -106,8 +106,9 @@ pub enum Operation {
     Write(ByteOffset, ByteLen),
     /// Flush buffer(s)
     Flush,
-    /// Discard/UNMAP/deallocate some ranges, which are specified in Request::ranges
-    Discard,
+    /// Discard/UNMAP/deallocate some ranges, which are specified in Request::ranges.
+    /// The ByteLen is the sum of the lengths of the ranges to be discarded, and is used for metrics.
+    Discard(ByteLen),
 }
 impl Operation {
     pub const fn is_read(&self) -> bool {
@@ -120,7 +121,7 @@ impl Operation {
         matches!(self, Operation::Flush)
     }
     pub const fn is_discard(&self) -> bool {
-        matches!(self, Operation::Discard)
+        matches!(self, Operation::Discard(..))
     }
 }
 
@@ -231,7 +232,7 @@ impl Request {
     }
 
     pub fn new_discard(ranges: Vec<(ByteOffset, ByteLen)>) -> Self {
-        let op = Operation::Discard;
+        let op = Operation::Discard(ranges.iter().map(|(_, len)| *len).sum());
         Self { op, regions: Vec::new(), ranges }
     }
 
@@ -243,7 +244,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/requests.rs

@@ -233,7 +233,7 @@ impl block::DeviceQueue for NvmeBlockQueue {
             Operation::Flush => {
                 probes::nvme_flush_complete!(|| (devsq_id, cid, resnum));
             }
-            Operation::Discard => {
+            Operation::Discard(..) => {
                 probes::nvme_discard_complete!(|| (devsq_id, cid, resnum));
             }
         }

lib/propolis/src/hw/virtio/block.rs

@@ -246,7 +246,7 @@ impl block::DeviceQueue for BlockVq {
                 block::Operation::Flush => {
                     probes::vioblk_flush_complete!(|| (rid, resnum));
                 }
-                block::Operation::Discard => {
+                block::Operation::Discard(..) => {
                     probes::vioblk_discard_complete!(|| (rid, resnum));
                 }
             }