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.
| #[derive(Debug, PartialEq, Eq)] | ||
| pub struct Request { | ||
| pub r#type: RequestType, | ||
| pub data_len: u32, | ||
| pub status_addr: GuestAddress, | ||
| sector: u64, | ||
| data_addr: GuestAddress, | ||
| discard_segments: Option<Vec<DiscardSegment>>, // New field, holds ranges |
There was a problem hiding this comment.
oh lol that was just a mental note to myself, forgot to take it out.
| // Get data descriptor | ||
| let data_desc = avail_desc.next_descriptor() | ||
| .ok_or(VirtioBlockError::DescriptorChainTooShort)?; |
There was a problem hiding this comment.
This is not a place to parse the request. It needs to be done above. There is already a place where data_desc is assigned.
| 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.
(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.