Merge #1548

1548: nvme/opts: parse timeouts in humantime r=tiagolobocastro a=tiagolobocastro

    fix(nvme/kato): adjust to spdk minimum
    
    SPDK minimum KATO is 10s, so setting 1s is pointless.
    Also the spec does suggest setting a sufficiently high KATO on a fabric, so perhaps
    we might want to consider increasing it in the future.
    
    Signed-off-by: Tiago Castro <[email protected]>

---

    fix(iobuf): increase large iobuf pool size
    
    Addresses warning from spdk itself:
    mayastor::spdk:transport.c:282]
    The num_shared_buffers value (2048) is larger than the available iobuf pool size (1024).
     Please increase the iobuf pool sizes.

    Also reduce size to power of 2 - 1 as recommended in spdk ticket:
    It’s always best to specify a value for –n that is one less than a power of two (2^x -1)
    because of the way rings work in DPDK. You end up allocating a ring twice the size for
    the one extra element between 511 and 512 for example.
    
    Signed-off-by: Tiago Castro <[email protected]>

---

    chore: silence flush not supported IO
    
    We don't keep any caching, so we don't actually need to make use of it anyway.
    TODO: do we need to even send flush?
    
    Signed-off-by: Tiago Castro <[email protected]>

---

    chore: show starting configuration as info
    
    Signed-off-by: Tiago Castro <[email protected]>

---

    feat(nvme/opts): parse timeouts in humantime
    
    This allows us to specify, for example, NVME_TIMEOUT=60s
    
    Signed-off-by: Tiago Castro <[email protected]>


Co-authored-by: Tiago Castro <[email protected]>

io-engine/src/bdev/nexus/nexus_bdev.rs

@@ -1265,11 +1265,20 @@ impl<'n> BdevOps for Nexus<'n> {
             | IoType::WriteZeros => {
                 let supported = self.io_is_supported(io_type);
                 if !supported {
-                    debug!(
-                        "{:?}: I/O type '{:?}' not supported by at least \
-                        one of child devices",
-                        self, io_type
-                    );
+                    if io_type == IoType::Flush {
+                        trace!(
+                            "{:?}: I/O type '{:?}' not supported by at least \
+                            one of child devices",
+                            self,
+                            io_type
+                        );
+                    } else {
+                        debug!(
+                            "{:?}: I/O type '{:?}' not supported by at least \
+                            one of child devices",
+                            self, io_type
+                        );
+                    }
                 }
                 supported
             }

io-engine/src/bin/io-engine-client/v0/jsonrpc_cli.rs

@@ -11,7 +11,6 @@ use tracing::debug;
 
 pub fn subcommands() -> Command {
     Command::new("jsonrpc")
-        .subcommand_required(true)
         .arg_required_else_help(true)
         .about("Call a json-rpc method with a raw JSON payload")
         .arg(

io-engine/src/bin/io-engine-client/v1/jsonrpc_cli.rs

@@ -24,7 +24,6 @@ pub fn subcommands() -> Command {
                 .index(2)
                 .help("Parameters (JSON string) to pass to method call"),
         )
-        .subcommand_required(true)
         .arg_required_else_help(true)
 }
 

io-engine/src/subsys/config/mod.rs

@@ -227,6 +227,6 @@ impl Config {
         assert!(self.socket_opts.set());
         assert!(self.iobuf_opts.set());
 
-        debug!("{:#?}", self);
+        info!("{:#?}", self);
     }
 }

io-engine/src/subsys/config/opts.rs

@@ -26,6 +26,7 @@ use spdk_rs::{
 };
 
 use std::{
+    convert::TryFrom,
     fmt::{Debug, Display},
     str::FromStr,
 };
@@ -166,14 +167,82 @@ where
                    val
                },
                Err(e) => {
-                   error!("Invalid value: {} (error {}) specified for {}. Reverting to default ({})", v, e, name, default);
+                   error!("Invalid value: {} (error {}) specified for {}. Reverting to default value ({})", v, e, name, default);
                    default
                }
             }
         },
     )
 }
 
+enum TimeUnit {
+    MilliSeconds,
+    MicroSeconds,
+}
+impl TimeUnit {
+    /// A backwards comptabile env variable name, which contains the
+    /// time units appended to the name.
+    fn backcompat_name(&self, name: &str) -> String {
+        format!(
+            "{name}{}",
+            match self {
+                TimeUnit::MilliSeconds => "_MS",
+                TimeUnit::MicroSeconds => "_US",
+            }
+        )
+    }
+    /// Collects a raw value in `Self` units for a given `Duration`.
+    fn value(&self, value: std::time::Duration) -> u128 {
+        match self {
+            TimeUnit::MilliSeconds => value.as_millis(),
+            TimeUnit::MicroSeconds => value.as_micros(),
+        }
+    }
+    fn units(&self) -> &str {
+        match self {
+            TimeUnit::MilliSeconds => "ms",
+            TimeUnit::MicroSeconds => "us",
+        }
+    }
+}
+
+/// Try to read an env variable in humantime, and if not found reverts back to
+/// old format to keep backward compatibility.
+fn time_try_from_env<T>(name: &str, default: T, unit: TimeUnit) -> T
+where
+    T: FromStr + Display + Copy + TryFrom<u128>,
+    <T as FromStr>::Err: Debug + Display,
+    <T as TryFrom<u128>>::Error: Display,
+{
+    match std::env::var(name) {
+        Ok(value) => {
+            let result = match humantime::parse_duration(&value) {
+                Ok(value) => {
+                    let in_units = unit.value(value);
+                    if in_units == 0 && !value.is_zero() {
+                        Err(format!("must be at least 1{}", unit.units()))
+                    } else {
+                        T::try_from(unit.value(value))
+                            .map_err(|error| error.to_string())
+                    }
+                }
+                Err(error) => Err(error.to_string()),
+            };
+            match result {
+                Ok(value) => {
+                    info!("Overriding {} value to '{}'", name, value);
+                    value
+                }
+                Err(e) => {
+                    error!("Invalid value: {} (error {}) specified for {}. Reverting to default value ({}{})", value, e, name, default, unit.units());
+                    default
+                }
+            }
+        }
+        Err(_) => try_from_env(&unit.backcompat_name(name), default),
+    }
+}
+
 impl Default for NvmfTcpTransportOpts {
     fn default() -> Self {
         Self {
@@ -185,7 +254,7 @@ impl Default for NvmfTcpTransportOpts {
                 "NVMF_TCP_MAX_QPAIRS_PER_CTRL",
                 32,
             ),
-            num_shared_buf: try_from_env("NVMF_TCP_NUM_SHARED_BUF", 2048),
+            num_shared_buf: try_from_env("NVMF_TCP_NUM_SHARED_BUF", 2047),
             buf_cache_size: try_from_env("NVMF_TCP_BUF_CACHE_SIZE", 64),
             dif_insert_or_strip: false,
             max_aq_depth: 32,
@@ -294,19 +363,36 @@ impl Default for NvmeBdevOpts {
     fn default() -> Self {
         Self {
             action_on_timeout: 4,
-            timeout_us: try_from_env("NVME_TIMEOUT_US", 5_000_000),
-            timeout_admin_us: try_from_env("NVME_TIMEOUT_ADMIN_US", 5_000_000),
-            keep_alive_timeout_ms: try_from_env("NVME_KATO_MS", 1_000),
+            timeout_us: time_try_from_env(
+                "NVME_TIMEOUT",
+                5_000_000,
+                TimeUnit::MicroSeconds,
+            ),
+            timeout_admin_us: time_try_from_env(
+                "NVME_TIMEOUT_ADMIN",
+                5_000_000,
+                TimeUnit::MicroSeconds,
+            ),
+            keep_alive_timeout_ms: time_try_from_env(
+                "NVME_KATO",
+                10_000,
+                TimeUnit::MilliSeconds,
+            ),
             transport_retry_count: try_from_env("NVME_RETRY_COUNT", 0),
             arbitration_burst: 0,
             low_priority_weight: 0,
             medium_priority_weight: 0,
             high_priority_weight: 0,
-            nvme_adminq_poll_period_us: try_from_env(
-                "NVME_ADMINQ_POLL_PERIOD_US",
+            nvme_adminq_poll_period_us: time_try_from_env(
+                "NVME_ADMINQ_POLL_PERIOD",
                 1_000,
+                TimeUnit::MilliSeconds,
+            ),
+            nvme_ioq_poll_period_us: time_try_from_env(
+                "NVME_IOQ_POLL_PERIOD",
+                0,
+                TimeUnit::MicroSeconds,
             ),
-            nvme_ioq_poll_period_us: try_from_env("NVME_IOQ_POLL_PERIOD_US", 0),
             io_queue_requests: 0,
             delay_cmd_submit: true,
             bdev_retry_count: try_from_env("NVME_BDEV_RETRY_COUNT", 0),
@@ -582,7 +668,7 @@ impl Default for IoBufOpts {
     fn default() -> Self {
         Self {
             small_pool_count: try_from_env("IOBUF_SMALL_POOL_COUNT", 8192),
-            large_pool_count: try_from_env("IOBUF_LARGE_POOL_COUNT", 1024),
+            large_pool_count: try_from_env("IOBUF_LARGE_POOL_COUNT", 2048),
             small_bufsize: try_from_env("IOBUF_SMALL_BUFSIZE", 8 * 1024),
             large_bufsize: try_from_env("IOBUF_LARGE_BUFSIZE", 132 * 1024),
         }