vmm: Support O_DIRECT for block devices#8
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an is_direct option to virtio-block configuration so backing block devices can be opened with O_DIRECT, and propagates this through config conversion and snapshot persistence.
Changes:
- Add
is_directto block device configs (BlockDeviceConfig/VirtioBlockConfig) and conversion logic. - Open/update backing files with
O_DIRECTwhenis_directis enabled. - Persist/restore the direct-I/O setting and update unit tests accordingly.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/vmm/src/vmm_config/drive.rs |
Adds is_direct to the user-facing block device config struct. |
src/vmm/src/devices/virtio/block/virtio/test_utils.rs |
Extends default virtio-block test configs with is_direct: false. |
src/vmm/src/devices/virtio/block/virtio/persist.rs |
Persists/restores the direct-I/O setting in snapshots. |
src/vmm/src/devices/virtio/block/virtio/device.rs |
Implements O_DIRECT open flag plumbing and threads the setting through device lifecycle; updates tests. |
src/vmm/src/devices/virtio/block/vhost_user/device.rs |
Ensures vhost-user block config serialization includes the new field (as None) and updates tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| @@ -61,6 +61,7 @@ pub struct VirtioBlockState { | |||
| pub virtio_state: VirtioDeviceState, | |||
| rate_limiter_state: RateLimiterState, | |||
| file_engine_type: FileEngineTypeState, | |||
There was a problem hiding this comment.
VirtioBlockState adds a new required field is_direct: bool without a serde default. Because snapshots accept any patch version within the same major.minor, restoring an older snapshot that lacks this field will fail deserialization. Make this field backward-compatible (e.g., add #[serde(default)] and default to false, or change to Option<bool> and treat None as false during restore).
| file_engine_type: FileEngineTypeState, | |
| file_engine_type: FileEngineTypeState, | |
| #[serde(default)] |
| String::from(f.as_path().to_str().unwrap()), | ||
| true, | ||
| engine, | ||
| true, |
There was a problem hiding this comment.
This test now calls DiskProperties::new(..., is_direct: true) on a TempFile. Opening temp files with O_DIRECT is not supported on some common CI filesystems (e.g., tmpfs), which can cause spurious EINVAL failures. Consider keeping is_direct as false for these temp-file-based tests, or explicitly skip/feature-gate the test when O_DIRECT is unsupported on the backing filesystem.
| true, | |
| false, |
| is_read_only: Some(true), | ||
| path_on_host: Some("path".to_string()), | ||
| rate_limiter: None, | ||
| file_engine_type: Some(FileEngineType::Sync), | ||
| is_direct: Some(true), | ||
|
|
There was a problem hiding this comment.
The vhost-user TryFrom<&BlockDeviceConfig> validation currently doesn’t reject is_direct being set when socket is used, so a request like { socket: "...", is_direct: true } would be accepted and the field silently ignored. Add a negative test case covering socket: Some(..) + is_direct: Some(true) and update the conversion validation to require value.is_direct.is_none() for vhost-user configs.
saleemrashid
force-pushed
the
saleem/o-direct
branch
2 times, most recently
from
82896f5 to
6f5aee6
Compare
saleemrashid
force-pushed
the
saleem/o-direct
branch
from
6f5aee6 to
b1a1f28
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| /// If set to true, the drive is opened with O_DIRECT. | ||
| pub is_direct: Option<bool>, |
There was a problem hiding this comment.
BlockDeviceConfig gained a new is_direct field, but the test-only manual Clone impl and the many BlockDeviceConfig { ... } struct literals in this file haven't been updated to include it. As written, this will fail to compile; please add is_direct to the Clone impl and initialize it (e.g. None) in test configs as needed (or use ..Default::default()).
| let disk_properties = DiskProperties::new( | ||
| String::from(f.as_path().to_str().unwrap()), | ||
| true, | ||
| engine, | ||
| true, | ||
| ) | ||
| .unwrap(); |
There was a problem hiding this comment.
The test_disk_backing_file_helper unit test now uses is_direct = true with a TempFile. O_DIRECT is not supported on some common temp filesystems (e.g. tmpfs), which can make this test fail depending on where it runs. Consider using is_direct = false here, or conditionally skipping when opening with O_DIRECT returns EINVAL.
No description provided.