ixi code review

Author: ahrens

Cargo.lock

@@ -78,9 +78,8 @@ impl VirtualDiskStats {
             }
             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.
-                // XXX no longer true
+                // Discard is only partially wired up in backends we care about, and it's not
+                // clear what stats (if any) to report yet.
             }
         }
     }

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

@@ -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/Cargo.toml

@@ -146,8 +146,10 @@ impl WorkerState {
                 }
             }
             block::Operation::Discard => {
-                // Crucible does not support discard operations for now
-                return Err(Error::Unsupported);
+                // 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/crucible.rs

@@ -31,6 +31,10 @@ pub enum ParseErr {
     /// An invalid value was specified in the FUSE bits of `CDW0`.
     #[error("reserved FUSE value specified")]
     ReservedFuse,
+
+    /// A reserved field was set to a non-zero value.
+    #[error("reserved field value specified")]
+    Reserved,
 }
 
 /// A parsed Admin Command
@@ -715,14 +719,19 @@ impl NvmCmd {
                 prp2: raw.prp2,
             }),
             bits::NVM_OPC_DATASET_MANAGEMENT => {
+                if (raw.cdw11 & !0b111) != 0 {
+                    // Only the lowest 3 bits of CDW11 are used for Dataset
+                    // Management, so reject if any other bits are set.
+                    return Err(ParseErr::Reserved);
+                }
                 NvmCmd::DatasetManagement(DatasetManagementCmd {
                     prp1: raw.prp1,
                     prp2: raw.prp2,
                     // Convert from 0's based value
                     nr: (raw.cdw10 & 0xFF) as u16 + 1,
                     ad: raw.cdw11 & (1 << 2) != 0,
-                    idw: raw.cdw11 & (1 << 1) != 0,
-                    idr: raw.cdw11 & (1 << 0) != 0,
+                    _idw: raw.cdw11 & (1 << 1) != 0,
+                    _idr: raw.cdw11 & (1 << 0) != 0,
                 })
             }
             _ => NvmCmd::Unknown(raw),
@@ -797,7 +806,6 @@ impl ReadCmd {
 
 /// Dataset Management Command Parameters
 #[derive(Debug)]
-#[allow(dead_code)]
 pub struct DatasetManagementCmd {
     /// PRP Entry 1 (PRP1)
     ///
@@ -806,14 +814,13 @@ pub struct DatasetManagementCmd {
 
     /// PRP Entry 2 (PRP2)
     ///
-    /// This field contains the second PRP entry that specifies the location where data should be
-    /// transferred from (if there is a physical discontinuity).
+    /// Indicates a second data buffer that contains LBA range information.  It may not be a PRP
+    /// List.
     prp2: u64,
 
     /// Number of Ranges (NR)
     ///
-    /// Indicates the number of 16 byte range sets that are specified in the command. This is a
-    /// 0’s based value.
+    /// Indicates the number of 16 byte range sets that are specified in the command.
     pub nr: u16,
 
     /// Attribute – Deallocate (AD)
@@ -832,15 +839,19 @@ pub struct DatasetManagementCmd {
     /// The host expects to perform operations on all ranges provided as an integral unit for
     /// writes, indicating that if a portion of the dataset is written it is expected that all of
     /// the ranges in the dataset are going to be written.
-    idw: bool,
+    ///
+    /// Note: this field is advisory, and we ignore it.
+    _idw: bool,
 
     /// Attribute – Integral Dataset for Read (IDR)
     ///
     /// If set to ‘1’ then the dataset should be optimized for read access as an integral unit.
     /// The host expects to perform operations on all ranges provided as an integral unit for
     /// reads, indicating that if a portion of the dataset is read it is expected that all of the
     /// ranges in the dataset are going to be read.
-    idr: bool,
+    ///
+    /// Note: this field is advisory, and we ignore it.
+    _idr: bool,
 }
 
 impl DatasetManagementCmd {
@@ -855,27 +866,23 @@ impl DatasetManagementCmd {
         )
     }
 
-    /// Returns an Iterator that yields the LBA ranges specified in this command.  Note that if
-    /// some of the ranges couldn't be read from guest memory, this will yield fewer than
-    /// `Self.nr` ranges.
+    /// Returns an Iterator that yields the LBA ranges specified in this command.  If any of the
+    /// ranges cannot be read from guest memory, yields an error for that range instead.
     pub fn ranges<'a>(
         &self,
         mem: &'a MemCtx,
-    ) -> impl Iterator<Item = DatasetManagementRangeDefinition> + 'a {
+    ) -> impl Iterator<
+        Item = Result<DatasetManagementRangeDefinition, &'static str>,
+    > + 'a {
         self.data(mem).flat_map(|region| {
-            let mut ranges = Vec::new();
-            if let Some(mapping) = mem.readable_region(&region) {
-                ranges.resize_with(
-                    mapping.len()
-                        / size_of::<DatasetManagementRangeDefinition>(),
-                    Default::default,
-                );
-                if mapping.read_many(&mut ranges).is_err() {
-                    ranges.clear();
-                }
-            };
-
-            ranges.into_iter()
+            if let Some(Ok(defs)) = mem
+                .readable_region(&region)
+                .map(|mapping| mapping.read_many_owned())
+            {
+                defs.into_iter().map(Ok).collect::<Vec<_>>().into_iter()
+            } else {
+                vec![Err("Failed to read LBA range")].into_iter()
+            }
         })
     }
 

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

@@ -2,12 +2,14 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
+use itertools::Itertools;
 use std::sync::Arc;
 use std::time::Instant;
 
 use super::{cmds::NvmCmd, queue::Permit, PciNvme};
 use crate::accessors::MemAccessor;
 use crate::block::{self, Operation, Request};
+use crate::hw::nvme::cmds::ParseErr;
 use crate::hw::nvme::{bits, cmds::Completion, queue::SubQueue};
 
 #[usdt::provider(provider = "propolis")]
@@ -147,7 +149,7 @@ impl block::DeviceQueue for NvmeBlockQueue {
                 }
                 Ok(NvmCmd::DatasetManagement(cmd)) => {
                     // We only support the "deallocate" (discard/trim) operation of Dataset
-                    // Management.  XXX should we return success?
+                    // Management.
                     if !cmd.is_deallocate() {
                         permit.complete(
                             Completion::generic_err(bits::STS_INVAL_FIELD)
@@ -161,18 +163,18 @@ impl block::DeviceQueue for NvmeBlockQueue {
                         cid,
                         cmd.nr,
                     ));
-                    let ranges: Vec<_> = cmd
+                    let Ok(ranges): Result<Vec<_>, _> = cmd
                         .ranges(&mem)
-                        .map(|r| r.offset_len(params.lba_data_size))
-                        .collect();
-                    if ranges.len() != cmd.nr as usize {
-                        // If we couldn't read all the ranges, fail the command
+                        .map_ok(|r| r.offset_len(params.lba_data_size))
+                        .try_collect()
+                    else {
+                        // If we couldn't read the ranges, fail the command
                         permit.complete(
                             Completion::generic_err(bits::STS_DATA_XFER_ERR)
                                 .dnr(),
                         );
                         continue;
-                    }
+                    };
                     let req = Request::new_discard(ranges);
                     return Some((req, permit, None));
                 }
@@ -181,6 +183,13 @@ impl block::DeviceQueue for NvmeBlockQueue {
                     let req = Request::new_flush();
                     return Some((req, permit, None));
                 }
+                Err(ParseErr::ReservedFuse) | Err(ParseErr::Reserved) => {
+                    // For commands that fail parsing due to reserved fields being set,
+                    // complete with an invalid field error
+                    let comp =
+                        Completion::generic_err(bits::STS_INVAL_FIELD).dnr();
+                    permit.complete(comp);
+                }
                 Ok(NvmCmd::Unknown(_)) | Err(_) => {
                     // For any other unrecognized or malformed command,
                     // just immediately complete it with an error

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

@@ -557,6 +557,28 @@ impl SubMapping<'_> {
         Ok(unsafe { typed.read_unaligned() })
     }
 
+    /// Read the entire mapping as an array of `T` objects.
+    /// The size of the mapping must be aligned to `size_of::<T>()`.
+    pub fn read_many_owned<T: Copy + FromBytes>(&self) -> Result<Vec<T>> {
+        self.check_read_access()?;
+        if !self.len.is_multiple_of(size_of::<T>()) {
+            return Err(Error::new(
+                ErrorKind::InvalidInput,
+                "Mapping size not aligned to value type",
+            ));
+        }
+        let count = self.len / size_of::<T>();
+        let mut vec = Vec::with_capacity(count);
+
+        self.read_many(&mut vec.spare_capacity_mut()[..count])?;
+        // Safety: read_many() was successful and just initialized the first `count` elements of
+        // the vector.
+        unsafe {
+            vec.set_len(count);
+        }
+        Ok(vec)
+    }
+
     /// Read `values` from the mapping.
     pub fn read_many<T: Copy + FromBytes>(
         &self,