Remove remaining IOPS/bandwidth limiting code (#1525)

After chatting in #oxide-hubris, it sounds like we want to put IOPS / bandwidth limiting into Propolis. This PR removes the remaining stubs; the actual functionality was already removed in #1506.
oxidecomputer · Oct 31, 2024 · 6e29eb6 · 6e29eb6
1 parent 134c114
commit 6e29eb6
Show file tree

Hide file tree

Showing 5 changed files with 1 addition and 133 deletions.
diff --git a/aws_benchmark/bench.sh b/aws_benchmark/bench.sh
@@ -13,5 +13,5 @@ set -e
 # downstairs uses 512b sectors
 ./target/release/measure-iops \
   -t $D0:3801 -t $D1:3801 -t $D2:3801 --samples 30 \
-  --io-depth 8 --io-size-in-bytes $((512 * 5)) # --iop-limit 250
+  --io-depth 8 --io-size-in-bytes $((512 * 5))
 
diff --git a/measure_iops/README.md b/measure_iops/README.md
@@ -32,15 +32,8 @@ Tool arguments:
     --samples
         how many samples to take (default 100). also the number of seconds the tool runs.
 
-    --iop-limit
-        how many IOPs per second to set
-
     --io-size-in-bytes
         how large of a read or write operation to send. defaults to block size.
 
     --io-depth
         how many IOs to send at a time
-
-    --bw-limit-in-bytes
-        how many bytes per second to set
-
diff --git a/measure_iops/src/main.rs b/measure_iops/src/main.rs
@@ -37,18 +37,12 @@ pub struct Opt {
     #[clap(long, default_value = "100", action)]
     samples: usize,
 
-    #[clap(long, action)]
-    iop_limit: Option<usize>,
-
     #[clap(long, action)]
     io_size_in_bytes: Option<usize>,
 
     #[clap(long, action)]
     io_depth: Option<usize>,
 
-    #[clap(long, action)]
-    bw_limit_in_bytes: Option<usize>,
-
     /// Submit all zeroes instead of random data
     #[clap(long, action)]
     all_zeroes: bool,
@@ -94,23 +88,6 @@ async fn main() -> Result<()> {
     let (guest, io) = Guest::new(None);
     let guest = Arc::new(guest);
 
-    if let Some(_iop_limit) = opt.iop_limit {
-        // XXX fix this once implemented
-        return Err(CrucibleError::Unsupported(
-            "IOP limit is not implemented".to_owned(),
-        )
-        .into());
-    }
-
-    if let Some(_bw_limit) = opt.bw_limit_in_bytes {
-        // XXX fix this once implemented
-        return Err(CrucibleError::Unsupported(
-            "Bandwidth limit is not implemented".to_owned(),
-        )
-        .into());
-    }
-
-    let guest = Arc::new(guest);
     let _join_handle = up_main(crucible_opts, opt.gen, None, io, None)?;
     println!("Crucible runtime is spawned");
 

diff --git a/upstairs/src/lib.rs b/upstairs/src/lib.rs
@@ -1601,86 +1601,6 @@ pub(crate) enum BlockOp {
     },
 }
 
-macro_rules! ceiling_div {
-    ($a: expr, $b: expr) => {
-        ($a + ($b - 1)) / $b
-    };
-}
-
-#[allow(unused)] // pending IOP limits being reimplemented
-impl BlockOp {
-    /*
-     * Compute number of IO operations represented by this BlockOp, rounding
-     * up. For example, if IOP size is 16k:
-     *
-     *   A read of 8k is 1 IOP
-     *   A write of 16k is 1 IOP
-     *   A write of 16001b is 2 IOPs
-     *   A flush isn't an IOP
-     *
-     * We are not counting WriteUnwritten ops as IO toward the users IO
-     * limits.  Though, if too many volumes are created with scrubbers
-     * running, we may have to revisit that.
-     */
-    pub fn iops(&self, iop_sz: usize) -> Option<usize> {
-        match self {
-            BlockOp::Read { data, .. } => {
-                Some(ceiling_div!(data.len(), iop_sz))
-            }
-            BlockOp::Write { data, .. } => {
-                Some(ceiling_div!(data.len(), iop_sz))
-            }
-            _ => None,
-        }
-    }
-
-    pub fn consumes_iops(&self) -> bool {
-        matches!(self, BlockOp::Read { .. } | BlockOp::Write { .. })
-    }
-
-    // Return the total size of this BlockOp
-    pub fn sz(&self) -> Option<usize> {
-        match self {
-            BlockOp::Read { data, .. } => Some(data.len()),
-            BlockOp::Write { data, .. } => Some(data.len()),
-            _ => None,
-        }
-    }
-}
-
-#[tokio::test]
-async fn test_return_iops() {
-    const IOP_SZ: usize = 16000;
-
-    let op = BlockOp::Read {
-        offset: BlockIndex(1),
-        data: Buffer::new(1, 512),
-        done: BlockOpWaiter::pair().1,
-    };
-    assert_eq!(op.iops(IOP_SZ).unwrap(), 1);
-
-    let op = BlockOp::Read {
-        offset: BlockIndex(1),
-        data: Buffer::new(8, 512), // 4096 bytes
-        done: BlockOpWaiter::pair().1,
-    };
-    assert_eq!(op.iops(IOP_SZ).unwrap(), 1);
-
-    let op = BlockOp::Read {
-        offset: BlockIndex(1),
-        data: Buffer::new(31, 512), // 15872 bytes < 16000
-        done: BlockOpWaiter::pair().1,
-    };
-    assert_eq!(op.iops(IOP_SZ).unwrap(), 1);
-
-    let op = BlockOp::Read {
-        offset: BlockIndex(1),
-        data: Buffer::new(32, 512), // 16384 bytes > 16000
-        done: BlockOpWaiter::pair().1,
-    };
-    assert_eq!(op.iops(IOP_SZ).unwrap(), 2);
-}
-
 /**
  * Stat counters struct used by DTrace
  */

diff --git a/upstairs/src/upstairs.rs b/upstairs/src/upstairs.rs
@@ -38,9 +38,6 @@ use uuid::Uuid;
 /// How often to log stats for DTrace
 const STAT_INTERVAL: Duration = Duration::from_secs(1);
 
-/// How often to do IO / bandwidth limit checking
-const LEAK_INTERVAL: Duration = Duration::from_millis(1000);
-
 /// How often to do live-repair status checking
 const REPAIR_CHECK_INTERVAL: Duration = Duration::from_secs(10);
 
@@ -92,7 +89,6 @@ pub struct UpCounters {
     action_guest: u64,
     action_deferred_block: u64,
     action_deferred_message: u64,
-    action_leak_check: u64,
     action_flush_check: u64,
     action_stat_check: u64,
     action_repair_check: u64,
@@ -108,7 +104,6 @@ impl UpCounters {
             action_guest: 0,
             action_deferred_block: 0,
             action_deferred_message: 0,
-            action_leak_check: 0,
             action_flush_check: 0,
             action_stat_check: 0,
             action_repair_check: 0,
@@ -141,7 +136,6 @@ impl UpCounters {
 ///   - Client timeout
 ///   - Client ping intervals
 ///   - Live-repair checks
-///   - IOPS leaking
 ///   - Automatic flushes
 ///   - DTrace logging of stats
 /// - Control requests from the controller server
@@ -236,9 +230,6 @@ pub(crate) struct Upstairs {
     /// Next time to check for repairs
     repair_check_deadline: Option<Instant>,
 
-    /// Next time to leak IOP / bandwidth tokens from the Guest
-    leak_deadline: Instant,
-
     /// Next time to trigger an automatic flush
     flush_deadline: Instant,
 
@@ -278,7 +269,6 @@ pub(crate) enum UpstairsAction {
     /// A deferred message has arrived
     DeferredMessage(DeferredMessage),
 
-    LeakCheck,
     FlushCheck,
     StatUpdate,
     RepairCheck,
@@ -414,7 +404,6 @@ impl Upstairs {
             state: UpstairsState::Initializing,
             cfg,
             repair_check_deadline: None,
-            leak_deadline: now + LEAK_INTERVAL,
             flush_deadline: now + flush_interval,
             stat_deadline: now + STAT_INTERVAL,
             flush_interval,
@@ -525,9 +514,6 @@ impl Upstairs {
                 };
                 UpstairsAction::DeferredMessage(m)
             }
-            _ = sleep_until(self.leak_deadline) => {
-                UpstairsAction::LeakCheck
-            }
             _ = sleep_until(self.flush_deadline) => {
                 UpstairsAction::FlushCheck
             }
@@ -574,14 +560,6 @@ impl Upstairs {
                     .action_deferred_message));
                 self.on_client_message(m);
             }
-            UpstairsAction::LeakCheck => {
-                self.counters.action_leak_check += 1;
-                cdt::up__action_leak_check!(|| (self
-                    .counters
-                    .action_leak_check));
-                // XXX Leak check is currently not implemented
-                self.leak_deadline = Instant::now() + LEAK_INTERVAL;
-            }
             UpstairsAction::FlushCheck => {
                 self.counters.action_flush_check += 1;
                 cdt::up__action_flush_check!(|| (self