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

Jump to bottom

Still more updates to support Volume layer activities. #1508

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Still more updates to support Volume layer activities. #1508

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
64 changes: 48 additions & 16 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 test. Write to a block in each extent.
FillSparse,
/// Flush
Flush,
/// Run Generic workload
Expand Down Expand Up @@ -234,7 +236,8 @@ async fn cli_read(
* Convert offset to its byte value.
*/
let offset = BlockIndex(block_index as u64);
let mut data = crucible::Buffer::repeat(255, size, ri.block_size as usize);
let mut data =
crucible::Buffer::repeat(255, size, ri.volume_info.block_size as usize);

println!("Read at block {:5}, len:{:7}", offset.0, data.len());
volume.read(offset, &mut data).await?;
Expand All @@ -244,7 +247,7 @@ async fn cli_read(
dl.clone(),
block_index,
&mut ri.write_log,
ri.block_size,
ri.volume_info.block_size,
false,
) {
ValidateStatus::Bad => {
Expand Down Expand Up @@ -311,13 +314,13 @@ async fn cli_write(
let data = if block_index + size > ri.total_blocks {
println!("Skip write log for invalid size {}", ri.total_blocks);
let mut out = BytesMut::new();
out.resize(size * ri.block_size as usize, 0);
out.resize(size * ri.volume_info.block_size as usize, 0);
out
} else {
for bi in block_index..block_index + size {
ri.write_log.update_wc(bi);
}
fill_vec(block_index, size, &ri.write_log, ri.block_size)
fill_vec(block_index, size, &ri.write_log, ri.volume_info.block_size)
};

println!("Write at block {:5}, len:{:7}", offset.0, data.len());
Expand Down Expand Up @@ -352,14 +355,18 @@ async fn cli_write_unwritten(
// like normal and update our internal counter to reflect that.

ri.write_log.update_wc(block_index);
fill_vec(block_index, 1, &ri.write_log, ri.block_size)
fill_vec(block_index, 1, &ri.write_log, ri.volume_info.block_size)
} else {
println!("This block has been written");
// Fill the write buffer with random data. We don't expect this
// to actually make it to disk.

let mut data = BytesMut::with_capacity(ri.block_size as usize);
data.extend((0..ri.block_size).map(|_| rand::thread_rng().gen::<u8>()));
let mut data =
BytesMut::with_capacity(ri.volume_info.block_size as usize);
data.extend(
(0..ri.volume_info.block_size)
.map(|_| rand::thread_rng().gen::<u8>()),
);
data
};

Expand Down Expand Up @@ -499,6 +506,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 +581,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(vi, bs, bl)) => {
println!("Got info: {:?} {} {}", vi, bs, bl);
}
Some(CliMessage::DoneOk) => {
println!("Ok");
Expand Down Expand Up @@ -871,6 +881,23 @@ async fn process_cli_command(
}
}
}
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(volume, 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 volume.flush(None).await {
Expand All @@ -886,10 +913,7 @@ async fn process_cli_command(
let new_ri = get_region_info(volume).await;
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 +926,12 @@ async fn process_cli_command(
*wc_filled = true;
}
}
fw.send(CliMessage::Info(bs, es, ts)).await
fw.send(CliMessage::Info(
new_ri.volume_info,
new_ri.total_size,
new_ri.total_blocks,
))
.await
}
Err(e) => fw.send(CliMessage::Error(e)).await,
}
Expand Down Expand Up @@ -1071,9 +1100,12 @@ pub async fn start_cli_server(
* If write_log len is zero, then the RegionInfo has
* not been filled.
*/
let volume_info = VolumeInfo {
Copy link
Contributor

@mkeeter mkeeter Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Should we be pulling this info from the volume argument? It's not obvious why initializing it with fictional values is okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was that way because I could not figure out how to make in an Option and pass it back and forth between process_cli_command and start_cli_server. Let me see if I can figure it out now

block_size: 512,
volumes: Vec::new(),
};
let mut ri: RegionInfo = RegionInfo {
block_size: 0,
extent_size: Block::new_512(0),
volume_info,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove total_size and total_blocks here, because they can both be calculated from volume_info?

Copy link
Contributor

Choose a reason for hiding this comment

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

Upon further reading, it would minimize the diff to also add a block_size() helper function, since lots of the diff in this PR is s/block_size/volume_info.block_size.

fn block_size(&self) -> u64 {
    self.volume_info.block_size
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I liked the idea of computing it once, then just passing it around, but don't really care if we compute it every time we need it. I don't think its anywhere inside the performance path, and if I find a place I'll handle it there.

total_size: 0,
total_blocks: 0,
write_log: WriteLog::new(0),
Expand Down
Loading