[disk_delay] A disk wrapper with runtime configurable I/O read and write delays to any underlying disk #1614
Conversation
|
Thanks for making these changes, Guramrit! |
gurasinghMS
changed the title
gurasinghMS
changed the title
|
Adding @axelandrejs for visibility |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new disk_delay wrapper to inject configurable I/O delays and propagates the VM task driver source (VmTaskDriverSource) through existing disk resolvers to enable timer-based operations.
- Adds a
disk_delaycrate withDelayDiskand its resolver - Updates all existing resolvers (
scsi,nvme,layered,crypt, backend, guest/emulation) to accept and forwarddriver_source - Adjusts workspace Cargo.toml files to include new dependencies and the
disk_delaymember
Reviewed Changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| vm/devices/storage/scsidisk/src/resolver.rs | Pass driver_source into SCSI disk and DVD resolvers |
| vm/devices/storage/nvme/src/resolver.rs | Forward driver_source in NVMe resolver |
| vm/devices/storage/disk_layered/src/resolver.rs | Forward driver_source in layered disk resolver |
| vm/devices/storage/disk_layered/src/resolve.rs | Add driver_source field to ResolveDiskLayerParameters |
| vm/devices/storage/disk_layered/Cargo.toml | Add vmcore workspace dependency |
| vm/devices/storage/disk_delay/src/resolver.rs | Implement resolver for DelayDisk |
| vm/devices/storage/disk_delay/src/lib.rs | Define DelayDisk wrapper with delay timer integration |
| vm/devices/storage/disk_delay/Cargo.toml | Introduce disk_delay crate and dependencies |
| vm/devices/storage/disk_crypt/src/resolver.rs | Forward driver_source in disk-crypt resolver |
| vm/devices/storage/disk_backend_resources/src/lib.rs | Define new DelayDiskHandle resource |
| vm/devices/storage/disk_backend/src/resolve.rs | Add driver_source to core disk resolver parameters |
| vm/devices/storage/disk_backend/Cargo.toml | Add vmcore workspace dependency |
| vm/devices/get/guest_emulation_device/src/resolver.rs | Forward driver_source in guest-emulation-device resolver |
| openvmm/hvlite_core/src/worker/dispatch.rs | Update open_simple_disk calls to include driver_source |
| openhcl/underhill_core/src/worker.rs | Update disk_from_disk_type calls to include driver_source |
| openhcl/underhill_core/src/dispatch/vtl2_settings_worker.rs | Add driver_source to disk_from_disk_type in VTL2 settings |
| Cargo.toml | Add vm/devices/storage/disk_delay to workspace members |
Comments suppressed due to low confidence (1)
vm/devices/storage/disk_delay/src/lib.rs:25
- The new
DelayDiskwrapper is not covered by any unit or integration tests. Adding tests to verify that delays are applied correctly (including boundary cases) would improve reliability.
pub struct DelayDisk {
There was a problem hiding this comment.
My comments are mostly stylistic. I think the behavior looks perfectly great for a first pass. Certainly we'd like this to do more (as you say in the notes). Please create an issue to track all the things you think we should do here, so we can communicate the roadmap. I'm happy to do that with you, if you like.
| @@ -23,6 +24,8 @@ pub struct ResolveDiskParameters<'a> { | |||
| // with missing lifetimes. Remove once we stop using async_trait for async | |||
| // resolvers. | |||
| pub _async_trait_workaround: &'a (), | |||
There was a problem hiding this comment.
Super micro nit: in general, I'd suggest putting this type of stuff (workarounds for language niggles) at the end of a type. Separately, I wonder if this could be a std::marker::PhantomData<``a> instead. But that stuff still confuses me.
There was a problem hiding this comment.
Though I wonder: since this struct now has a member that uses the ```alifetime, can we just remove_async_trait_workaround`? I'll try this myself.
There was a problem hiding this comment.
Yeah, that works I think. See gurasinghMS#11 (take my commit if you like)
| /// A disk with delay on every I/O operation. | ||
| #[derive(Inspect)] | ||
| pub struct DelayDisk { | ||
| #[inspect(skip)] |
There was a problem hiding this comment.
Of all the fields here, I think delay is the one we'd want to inspect :) Is there a problem with Inspect and Cells?
There was a problem hiding this comment.
Yep, Cell apparently doesn't allow inspect :(
There was a problem hiding this comment.
You can use inspect's with helper in the derive to make a custom reader. Check out the crate's docs for more, it has a lot of functionality.
Co-authored-by: Matt LaFayette (Kurjanowicz) <mattkur@microsoft.com>
This change is part of changes for nvme keepalive testing and it will be used in the nvme emulator to induce artificial delays and thus attempt to capture perf of keepalive during servicing.
As a long term goal, the functionality in this crate will be expanded to include configurable I/O drops, randomized slowdown and much more in order to make testing of devices easier.