Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make crutest know extent info for each sub volume. #1474

Closed
wants to merge 5 commits into from
Closed

Make crutest know extent info for each sub volume. #1474

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
34c4960
Make crutest know extent info for each sub volume.
leftwo Sep 26, 2024
231d563
Remove file I did not want to add
leftwo Sep 26, 2024
71a0254
Undo debug comment to make things go faster
leftwo Sep 26, 2024
0ba09e4
more comment cleanup
leftwo Sep 27, 2024
6901b9e
Fix target generation for hand crafted volumes
Sep 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions common/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,10 @@ use tokio::time::{Duration, Instant};

mod region;
pub use region::{
config_path, Block, BlockIndex, BlockOffset, ExtentId, RegionDefinition,
RegionOptions, DATABASE_READ_VERSION, DATABASE_WRITE_VERSION,
MAX_BLOCK_SIZE, MAX_SHIFT, MIN_BLOCK_SIZE, MIN_SHIFT,
config_path, Block, BlockIndex, BlockOffset, ExtentId, ExtentInfo,
RegionDefinition, RegionOptions, DATABASE_READ_VERSION,
DATABASE_WRITE_VERSION, MAX_BLOCK_SIZE, MAX_SHIFT, MIN_BLOCK_SIZE,
MIN_SHIFT,
};

pub mod impacted_blocks;
Expand Down
7 changes: 7 additions & 0 deletions common/src/region.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,13 @@ impl Default for RegionOptions {
}
}

#[derive(Copy, Clone, Debug, PartialEq, Serialize, Deserialize)]
/// Info about extents in a region.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This data structure gives me bad vibes for a few reasons.

  • I have a pre-existing dislike for Block, because its value field is ambiguous and used in different ways
  • It looks like we're usually returning a Vec<ExtentInfo>, in which all of the extent_size.value must be identical (we don't support volumes with mixed block sizes). However, this isn't prevented by the data structure!
  • Pedantically, this isn't info about an extent; it's info about a region / volume / subvolume
  • We already have crucible_client_types::RegionExtentInfo, which contains the same data as this struct

If we want an unambiguous data structure, we could do something like the following:

pub struct VolumeInfo {
    pub block_size: u64,
    pub volumes: Vec<SubVolumeInfo>,
}

struct SubVolumeInfo {
    pub blocks_per_extent: u64,
    pub extent_count: u32,
}

Using this data structure, we're promising that we've checked and all of the block sizes are the same; we would need to add that check to Volume::query_extent_info. Volumes without any subvolumes could still return the top-level VolumeInfo with a single value in volumes (equivalen to return a single-item Vec<ExtentInfo>).

pub struct ExtentInfo {
pub extent_size: Block,
pub extent_count: u32,
}

/// Append the region description file to the end of a provided path.
pub fn config_path<P: AsRef<Path>>(dir: P) -> PathBuf {
let mut out = dir.as_ref().to_path_buf();
Expand Down
35 changes: 35 additions & 0 deletions crucible-client-types/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,41 @@ pub enum VolumeConstructionRequest {
},
}

impl VolumeConstructionRequest {
/// Return all the targets that are part of this VCR.
pub fn targets(&self) -> Vec<SocketAddr> {
let mut targets = Vec::new();
match self {
VolumeConstructionRequest::Volume {
id: _,
block_size: _,
sub_volumes,
read_only_parent: _,
} => {
for subreq in sub_volumes {
let new_targets = subreq.targets();
for nt in new_targets {
targets.push(nt);
}
}
}
VolumeConstructionRequest::Region {
block_size: _,
blocks_per_extent: _,
extent_count: _,
opts,
gen: _,
} => {
for nt in &opts.target {
targets.push(*nt);
}
}
_ => {}
}
targets
}
}
Comment on lines +43 to +74
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to manually accumulate into the Vec<SocketAddr>:

Suggested change
pub fn targets(&self) -> Vec<SocketAddr> {
let mut targets = Vec::new();
match self {
VolumeConstructionRequest::Volume {
id: _,
block_size: _,
sub_volumes,
read_only_parent: _,
} => {
for subreq in sub_volumes {
let new_targets = subreq.targets();
for nt in new_targets {
targets.push(nt);
}
}
}
VolumeConstructionRequest::Region {
block_size: _,
blocks_per_extent: _,
extent_count: _,
opts,
gen: _,
} => {
for nt in &opts.target {
targets.push(*nt);
}
}
_ => {}
}
targets
}
}
pub fn targets(&self) -> Vec<SocketAddr> {
match self {
VolumeConstructionRequest::Volume {
id: _,
block_size: _,
sub_volumes,
read_only_parent: _,
} => sub_volumes.iter().flat_map(|s| s.targets()).collect(),
VolumeConstructionRequest::Region {
block_size: _,
blocks_per_extent: _,
extent_count: _,
opts,
gen: _,
} => opts.target.clone(),
_ => vec![],
}
}


#[allow(clippy::derive_partial_eq_without_eq)]
#[derive(
Debug, Clone, Default, Serialize, Deserialize, JsonSchema, PartialEq,
Expand Down
37 changes: 28 additions & 9 deletions crutest/src/cli.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,8 @@ enum CliCommand {
#[clap(long, action)]
skip_verify: bool,
},
/// Run the sparse fill workload, no verify
FillSparse,
/// Flush
Flush,
/// Run Generic workload
Expand Down Expand Up @@ -499,6 +501,9 @@ async fn cmd_to_msg(
CliCommand::Fill { skip_verify } => {
fw.send(CliMessage::Fill(skip_verify)).await?;
}
CliCommand::FillSparse => {
fw.send(CliMessage::FillSparse).await?;
}
CliCommand::Flush => {
fw.send(CliMessage::Flush).await?;
}
Expand Down Expand Up @@ -571,8 +576,8 @@ async fn cmd_to_msg(
Some(CliMessage::MyUuid(uuid)) => {
println!("uuid: {}", uuid);
}
Some(CliMessage::Info(es, bs, bl)) => {
println!("Got info: {} {} {}", es, bs, bl);
Some(CliMessage::Info(ri)) => {
println!("Got info: {:?}", ri);
}
Some(CliMessage::DoneOk) => {
println!("Ok");
Expand Down Expand Up @@ -871,6 +876,23 @@ async fn process_cli_command<T: BlockIO + Send + Sync + 'static>(
}
}
}
CliMessage::FillSparse => {
if ri.write_log.is_empty() {
fw.send(CliMessage::Error(CrucibleError::GenericError(
"Info not initialized".to_string(),
)))
.await
} else {
match fill_sparse_workload(block_io.as_ref(), ri).await {
Ok(_) => fw.send(CliMessage::DoneOk).await,
Err(e) => {
let msg = format!("FillSparse failed with {}", e);
let e = CrucibleError::GenericError(msg);
fw.send(CliMessage::Error(e)).await
}
}
}
}
CliMessage::Flush => {
println!("Flush");
match block_io.flush(None).await {
Expand All @@ -883,13 +905,10 @@ async fn process_cli_command<T: BlockIO + Send + Sync + 'static>(
Err(e) => fw.send(CliMessage::Error(e)).await,
},
CliMessage::InfoPlease => {
let new_ri = get_region_info(block_io).await;
let new_ri = get_region_info(block_io, false).await;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we know is_encrypted is false here?

match new_ri {
Ok(new_ri) => {
let bs = new_ri.block_size;
let es = new_ri.extent_size.value;
let ts = new_ri.total_size;
*ri = new_ri;
*ri = new_ri.clone();
/*
* We may only want to read input from the file once.
* Maybe make a command to specifically do it, but it
Expand All @@ -902,7 +921,7 @@ async fn process_cli_command<T: BlockIO + Send + Sync + 'static>(
*wc_filled = true;
}
}
fw.send(CliMessage::Info(bs, es, ts)).await
fw.send(CliMessage::Info(new_ri)).await
}
Err(e) => fw.send(CliMessage::Error(e)).await,
}
Expand Down Expand Up @@ -1074,7 +1093,7 @@ pub async fn start_cli_server<T: BlockIO + Send + Sync + 'static>(
*/
let mut ri: RegionInfo = RegionInfo {
block_size: 0,
extent_size: Block::new_512(0),
sub_volume_info: Vec::new(),
total_size: 0,
total_blocks: 0,
write_log: WriteLog::new(0),
Expand Down
Loading