From 47cd9b30884a34077d4ea5f288fb8d042d25914b Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Tue, 26 Nov 2024 13:27:55 -0500 Subject: [PATCH 1/3] Send deactivate flush if need_flush is set --- upstairs/src/upstairs.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/upstairs/src/upstairs.rs b/upstairs/src/upstairs.rs index 58ce09638..f4a7dd5a6 100644 --- a/upstairs/src/upstairs.rs +++ b/upstairs/src/upstairs.rs @@ -1278,7 +1278,7 @@ impl Upstairs { } UpstairsState::Active => (), } - if !self.downstairs.can_deactivate_immediately() { + if self.need_flush || !self.downstairs.can_deactivate_immediately() { debug!(self.log, "not ready to deactivate; submitting final flush"); let io_guard = self.try_acquire_io(0); self.submit_flush(None, None, io_guard); From 8913f0e2409154cc62826dd58f47d7e4e42cfc2a Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Tue, 26 Nov 2024 09:16:53 -0500 Subject: [PATCH 2/3] trying to test ROP deactivation --- integration_tests/src/lib.rs | 37 ++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/integration_tests/src/lib.rs b/integration_tests/src/lib.rs index 130510d18..bd6e6109b 100644 --- a/integration_tests/src/lib.rs +++ b/integration_tests/src/lib.rs @@ -5809,4 +5809,41 @@ mod test { } )); } + + #[tokio::test] + async fn test_auto_flush_deactivate() { + let log = csl(); + let child = TestDownstairsSet::small(false).await.unwrap(); + let vcr = VolumeConstructionRequest::Volume { + id: Uuid::new_v4(), + block_size: 512, + sub_volumes: vec![VolumeConstructionRequest::Region { + block_size: 512, + blocks_per_extent: child.blocks_per_extent(), + extent_count: child.extent_count(), + opts: child.opts(), + gen: 2, + }], + read_only_parent: None, + }; + + let volume = Volume::construct(vcr, None, log.clone()).await.unwrap(); + volume.activate().await.unwrap(); + + // Send exactly IO_CACHED_MAX_JOBS to force a Barrier to be sent. The + // barrier empties out the active list, so deactivation may proceed. + for _ in 0..10000 { + let mut buf = Buffer::new(2, 512); + volume.read(BlockIndex(0), &mut buf).await.unwrap(); + } + tokio::time::sleep(std::time::Duration::from_millis(100)).await; + + // At this point, need_flush is set, but there are no active jobs. + // Deactivation must be aware of the need_flush flag; otherwise, the + // Upstairs will attempt to submit that flush after deactivation. + volume.deactivate().await.unwrap(); + + // Make sure everything worked + volume.activate().await.unwrap(); + } } From 7dc0be16221dc0ce43144ff676801ae1255fa766 Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Tue, 26 Nov 2024 20:19:12 -0500 Subject: [PATCH 3/3] Re-export IO_CACHED_MAX_JOBS with new integration-tests feature --- integration_tests/Cargo.toml | 2 +- integration_tests/src/lib.rs | 2 +- upstairs/Cargo.toml | 1 + upstairs/src/lib.rs | 6 ++++++ 4 files changed, 9 insertions(+), 2 deletions(-) diff --git a/integration_tests/Cargo.toml b/integration_tests/Cargo.toml index 8ba51dad0..99b351b38 100644 --- a/integration_tests/Cargo.toml +++ b/integration_tests/Cargo.toml @@ -19,7 +19,7 @@ crucible-client-types.workspace = true crucible-downstairs = { workspace = true, features = ["integration-tests"] } crucible-pantry-client.workspace = true crucible-pantry.workspace = true -crucible.workspace = true +crucible = { workspace = true, features = ["integration-tests"] } dropshot.workspace = true futures-core.workspace = true futures.workspace = true diff --git a/integration_tests/src/lib.rs b/integration_tests/src/lib.rs index bd6e6109b..122220ec8 100644 --- a/integration_tests/src/lib.rs +++ b/integration_tests/src/lib.rs @@ -5832,7 +5832,7 @@ mod test { // Send exactly IO_CACHED_MAX_JOBS to force a Barrier to be sent. The // barrier empties out the active list, so deactivation may proceed. - for _ in 0..10000 { + for _ in 0..crucible::testing::IO_CACHED_MAX_JOBS { let mut buf = Buffer::new(2, 512); volume.read(BlockIndex(0), &mut buf).await.unwrap(); } diff --git a/upstairs/Cargo.toml b/upstairs/Cargo.toml index ca4e37b7d..5b866aa67 100644 --- a/upstairs/Cargo.toml +++ b/upstairs/Cargo.toml @@ -12,6 +12,7 @@ path = "src/lib.rs" [features] asm = ["usdt/asm"] notify-nexus = ["nexus-client", "internal-dns", "progenitor-client", "http", "omicron-uuid-kinds"] +integration-tests = [] [dependencies] anyhow.workspace = true diff --git a/upstairs/src/lib.rs b/upstairs/src/lib.rs index df0643c14..09508ef0f 100644 --- a/upstairs/src/lib.rs +++ b/upstairs/src/lib.rs @@ -110,6 +110,12 @@ const IO_CACHED_MAX_BYTES: u64 = 1024 * 1024 * 1024; // 1 GiB /// them comes back. const IO_CACHED_MAX_JOBS: u64 = 10000; +// Re-exports for unit testing +#[cfg(feature = "integration-tests")] +pub mod testing { + pub const IO_CACHED_MAX_JOBS: u64 = super::IO_CACHED_MAX_JOBS; +} + /// The BlockIO trait behaves like a physical NVMe disk (or a virtio virtual /// disk): there is no contract about what order operations that are submitted /// between flushes are performed in.