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.

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

Still more updates to support Volume layer activities. #1508

Merged
merged 4 commits into from
Oct 31, 2024

Conversation

leftwo
Copy link
Contributor

@leftwo leftwo commented Oct 15, 2024

This is a re-do, re-work of what was in: Make crutest know extent info for each sub volume. #1474

Two new structs to describe a Volume:

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

Added a new method to the Volume type that will gather extent info from each sub-volume and return a VolumeInfo struct that describes the whole volume.

In the BlockIO trait, replaced query_extent_size with query_extent_info. Objects that support this call will return the RegionExtentInfo type filled out. Things that do not will just return None.

Updated crutest's RegionInfo struct to have a VolumeInfo field.

Updated the fill_sparse and span crutest tests to perform their work on all sub-volumes.

print_region_description looks like this:

----------------------------------------------
random read with 4 KiB chunks (1 block)
region info:
  block size:                   4096 bytes
  sub_volume 0 blocks / extent: 100
  sub_volume 0 extent count:    15
  sub_volume 0 extent size:     400 KiB
  sub_volume 1 blocks / extent: 100
  sub_volume 1 extent count:    15
  sub_volume 1 extent size:     400 KiB
  total blocks:                 3000
  total size:                   11.7 MiB
  encryption:                   no
----------------------------------------------

Added fill-sparse test to crutest cli

Coming in a follow up PR I would like to rename RegionInfo in crutest to be DiskInfo. This would just be name changes, no functional changes. I decided to break that out to reduce review overhead.

Two new structs to describe a Volume:

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

Added a new method to the Volume type that will gather extent
info from each sub-volume and return a VolumeInfo struct that
describes the whole volume.

In the BlockIO trait, replaced query_extent_size with
query_extent_info.  Objects that support this call will return the
RegionExtentInfo type filled out.  Things that do not will just
return None.

Updated crutest's RegionInfo struct to have a VolumeInfo field.

Updated fill_sparse and span crutest tests to perform their work
on all sub-volumes.

print_region_description looks like this:
----------------------------------------------
random read with 4 KiB chunks (1 block)
region info:
  block size:                   4096 bytes
  sub_volume 0 blocks / extent: 100
  sub_volume 0 extent count:    15
  sub_volume 0 extent size:     400 KiB
  sub_volume 1 blocks / extent: 100
  sub_volume 1 extent count:    15
  sub_volume 1 extent size:     400 KiB
  total blocks:                 3000
  total size:                   11.7 MiB
  encryption:                   no
----------------------------------------------

Added fill-sparse test to crutest cli

Coming in a follow up PR I would like to rename RegionInfo in
crutest to be DiskInfo.  This would just be name changes, no
functional changes.  I decided to break that out to reduce review
overhead.
crutest/src/cli.rs Outdated Show resolved Hide resolved
crutest/src/cli.rs Outdated Show resolved Hide resolved
Comment on lines +3117 to +3120
let mut es = 0;
for sv in ri.volume_info.volumes.iter() {
es += sv.blocks_per_extent;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't make sense to me; the sum of blocks-per-extent across all subvolumes isn't a meaningful value.

In a subvolume-flavored world, there is no single extent size (es), so we should just remove it. Extent count ec remains meaningful, but should be a function on VolumeInfo, e.g.

impl VolumeInfo {
    /// Returns the total number of extent files in this volume
    fn extent_count(&self) -> usize {
        self.subvolumes.iter().map(|s| s.extent_count).sum().unwrap_or(0)
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, for this I'm going to remove the whole crutest perf test completely, so this was just to make it compile.
It's all going away.

crutest/src/protocol.rs Outdated Show resolved Hide resolved
upstairs/src/guest.rs Outdated Show resolved Hide resolved
Alan Hanson added 2 commits October 28, 2024 18:26
Updated crutest cli.rs to have Option<RegionInfo> instead of using an
uninitialized struct.

Exposed the  as a FastFill test.
Made total_blocks and total_size methods for VolumeInfo
Made a block_size() method for RegionInfo
Removed block_size and total_size from RegionInfo

Fixed a bug where the sparse fill was not correctly writing to all extents.
@leftwo
Copy link
Contributor Author

leftwo commented Oct 29, 2024

Should also fix:
#1477

Partial fix for this #1455, though we may want to re-think how jobs are returned and displayed at the volume level.

We can probably call this one fixed, as we are full volume now: #1454

@leftwo leftwo requested a review from mkeeter October 30, 2024 16:21
if let Some(ri) = ri_option {
if ri.write_log.is_empty() {
fw.send(CliMessage::Error(CrucibleError::GenericError(
"Info not initialized".to_string(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the correct error message? It's confusing to me, because this message is used for both write_log.is_empty() and ri_option.is_none() cases. Above, we use "Internal write count buffer empty" for the former.

(same question applies below in a bunch of places)

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's not the correct message any longer, but even better, it's also not possible.
If we are at the place where we check for a write log, then it must exist, as we now have an <Option<RegionInfo>> instead of an "empty" one. So, if the RegionInfo exists, then we have a write log and we can avoid this whole check. I'll make that change in all the places where we check for it.

@leftwo leftwo merged commit 540fd3a into main Oct 31, 2024
16 checks passed
@leftwo leftwo deleted the alan/crutest-volume-info branch October 31, 2024 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants