feat(virtio-block): Add support for VIRTIO_BLK_T_DISCARD request type #5168
Conversation
- Implemented parsing and validation for `VIRTIO_BLK_T_DISCARD` requests. - Added `SyncIoError::Discard` to represent errors specific to discard operations. - Updated `Request::process` to handle discard requests using `handle_discard` in `FileEngine`. - Integrated discard metrics (`discard_count` and `invalid_reqs_count`) for successful and invalid discard operations. Signed-off-by: Leul Dagnachew <leulmdagnachew@gmail.com> Signed-off-by: LDagnachew <leulmdagnachew@gmail.com>
|
As far as checking whether the system even supports |
There was a problem hiding this comment.
Regarding the check if fallocate is supported: for now let's assume it is supported.
Doing a test fallocate call might not a bad idea. Alternatively, we can always make this an optional feature, so users will need to configure it manually (line read_only) if they know their fs supports it.
Also found a patch series for qemu with this feature: https://lore.kernel.org/all/20190208134950.187665-5-sgarzare@redhat.com/ might be useful for a reference.
| bitflags::bitflags! { | ||
| pub struct FallocateFlags: c_int { | ||
| const FALLOC_FL_KEEP_SIZE = 0x01; | ||
| const FALLOC_FL_PUNCH_HOLE = 0x02; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
libc crate already has these constants
| let fd = self.file().as_raw_fd(); | ||
| let result = Self::fallocate( | ||
| fd, | ||
| FallocateFlags::FALLOC_FL_PUNCH_HOLE | FallocateFlags::FALLOC_FL_KEEP_SIZE, | ||
| offset as i64, | ||
| len as i64, | ||
| ); | ||
| if let Err(e) = result { | ||
| eprintln!("Discard failed: {}", e); | ||
| return Err(std::io::Error::last_os_error()); | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
Since we have 2 backends for block device, each need to implement it's own version of discard. For sync version this impl is basically what it needs to be, for io_uring backend there is IORING_OP_FALLOCATE op.
| pub struct DiscardSegment { | ||
| sector: u64, | ||
| num_sectors: u32, | ||
| flags: u32, | ||
| } |
There was a problem hiding this comment.
flags is not just a 32 bit integer, it is a struct with 2 u32 inside. Also it needs to have a C struct layout attribute.
There was a problem hiding this comment.
Sorry for delay, was busy with other things but- I thought that flags was a single 32-bit integer, where 1 bit was for unmap, and the other 31 bits are reserved for struct alignment I'm assuming.
For reference, I'm using VirtI/O 1.2 Documentation.
There was a problem hiding this comment.
Alright, you are right, I totally missed the bitfield syntax in C definition.
Still you need to add the [repr(C)] to the struct definition.
| if num_segments == 0 { | ||
| return Err(VirtioBlockError::InvalidDiscardRequest); | ||
| } | ||
| let mut segments = Vec::with_capacity(num_segments as usize); |
There was a problem hiding this comment.
I think we can avoid having to deal with multiple segments if we just limit their number to 1 in the config space of the device. You will need to expand the ConfigSpace type to include max_discard_sectors and everything before it. Also in all other code please avoid dynamic allocations, it can cause issues in the future.
| match res { | ||
| Ok(()) => Ok(block_io::FileEngineOk::Executed(block_io::RequestOk { | ||
| req: pending, | ||
| count: 0, | ||
| })), | ||
| Err(e) => Err(block_io::RequestError { | ||
| req: pending, | ||
| error: BlockIoError::Sync(SyncIoError::Discard(e)), | ||
| }), | ||
| } |
There was a problem hiding this comment.
Nice hallucinations. The file_engine.handle_discard should return same result type as file_engine.read/file_engine.write and so on. And then it will be handled by the code bellow.
There was a problem hiding this comment.
Gotcha, that checks out, I'll make that change.
- Added Test for ext4fs support - Added Test for ensuring fstrim executes without error. Signed-off-by: LDagnachew <leulmdagnachew@gmail.com>
- Utilized `IORING_OP_FALLOCATE` for Fallocate Implementation in `AsyncFileEngine` - Resolved invalid return types for `discard` Signed-off-by: LDagnachew <leulmdagnachew@gmail.com>
(Reupload) Opening this as a Draft PR, as I am still working on fully integrating unmap/fstrim/discard. Right now I'm currently working on the functionality.
Changes
Expose discard feature bit
VIRTIO_BLK_F_DISCARDto the device’s advertised avail_features so guests can negotiate discard supportExtend RequestType enum
VIRTIO_BLK_T_DISCARD→RequestType::Discardin the request conversionUpdate request parsing (Request::parse) in
request.rsImplement handle_discard backend
handle_discard(offset, len)to callfallocate()on the host file descriptorReason
Attempting to resolve Issue #2708
Next Steps
VIRTIO_BLK_F_DISCARDsupport (need suggestions for this, not sure of an efficient way to check for Discard Support within a given VirtIO block).License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.PR Checklist
tools/devtool checkstyleto verify that the PR passes theautomated style checks.
how they are solving the problem in a clear and encompassing way.
in the PR.
CHANGELOG.md.Runbook for Firecracker API changes.
integration tests.
TODO.rust-vmm.