-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat(virtio-block): Add support for VIRTIO_BLK_T_DISCARD request type #5168
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -110,7 +110,7 @@ impl AsyncFileEngine { | |
Ok(()) | ||
} | ||
|
||
#[cfg(test)] | ||
|
||
pub fn file(&self) -> &File { | ||
&self.file | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,8 @@ pub mod sync_io; | |
|
||
use std::fmt::Debug; | ||
use std::fs::File; | ||
use libc::{c_int, off64_t}; | ||
use std::os::unix::io::AsRawFd; | ||
|
||
pub use self::async_io::{AsyncFileEngine, AsyncIoError}; | ||
pub use self::sync_io::{SyncFileEngine, SyncIoError}; | ||
|
@@ -33,6 +35,13 @@ pub enum BlockIoError { | |
Async(AsyncIoError), | ||
} | ||
|
||
bitflags::bitflags! { | ||
pub struct FallocateFlags: c_int { | ||
const FALLOC_FL_KEEP_SIZE = 0x01; | ||
const FALLOC_FL_PUNCH_HOLE = 0x02; | ||
} | ||
} | ||
|
||
impl BlockIoError { | ||
pub fn is_throttling_err(&self) -> bool { | ||
match self { | ||
|
@@ -75,7 +84,7 @@ impl FileEngine { | |
Ok(()) | ||
} | ||
|
||
#[cfg(test)] | ||
|
||
pub fn file(&self) -> &File { | ||
match self { | ||
FileEngine::Async(engine) => engine.file(), | ||
|
@@ -172,6 +181,34 @@ impl FileEngine { | |
FileEngine::Sync(engine) => engine.flush().map_err(BlockIoError::Sync), | ||
} | ||
} | ||
|
||
pub fn handle_discard(&self, offset: u64, len: u32) -> Result<(), std::io::Error> { | ||
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(()) | ||
Comment on lines
+186
to
+197
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
} | ||
|
||
pub fn fallocate(fd: c_int, mode: FallocateFlags, offset: off64_t, len: off64_t) -> Result<(), std::io::Error> { | ||
// need to refer to libc library. | ||
let ret: i32 = unsafe { libc::fallocate64(fd, mode.bits(), offset, len) }; | ||
if ret == 0 { | ||
Ok(()) | ||
} else { | ||
Err(std::io::Error::last_os_error()) | ||
} | ||
} | ||
|
||
|
||
|
||
} | ||
|
||
#[cfg(test)] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,17 +9,19 @@ use std::convert::From; | |
|
||
use vm_memory::GuestMemoryError; | ||
|
||
use super::io::{BlockIoError, SyncIoError}; | ||
use super::{SECTOR_SHIFT, SECTOR_SIZE, VirtioBlockError, io as block_io}; | ||
use crate::devices::virtio::block::virtio::device::DiskProperties; | ||
use crate::devices::virtio::block::virtio::metrics::BlockDeviceMetrics; | ||
pub use crate::devices::virtio::generated::virtio_blk::{ | ||
VIRTIO_BLK_ID_BYTES, VIRTIO_BLK_S_IOERR, VIRTIO_BLK_S_OK, VIRTIO_BLK_S_UNSUPP, | ||
VIRTIO_BLK_T_FLUSH, VIRTIO_BLK_T_GET_ID, VIRTIO_BLK_T_IN, VIRTIO_BLK_T_OUT, | ||
VIRTIO_BLK_T_DISCARD | ||
}; | ||
use crate::devices::virtio::queue::DescriptorChain; | ||
use crate::logger::{IncMetric, error}; | ||
use crate::rate_limiter::{RateLimiter, TokenType}; | ||
use crate::vstate::memory::{ByteValued, Bytes, GuestAddress, GuestMemoryMmap}; | ||
use crate::vstate::memory::{ByteValued, Bytes, GuestAddress, GuestMemoryMmap, Address}; | ||
|
||
#[derive(Debug, derive_more::From)] | ||
pub enum IoErr { | ||
|
@@ -34,6 +36,7 @@ pub enum RequestType { | |
Out, | ||
Flush, | ||
GetDeviceID, | ||
Discard, | ||
Unsupported(u32), | ||
} | ||
|
||
|
@@ -44,6 +47,7 @@ impl From<u32> for RequestType { | |
VIRTIO_BLK_T_OUT => RequestType::Out, | ||
VIRTIO_BLK_T_FLUSH => RequestType::Flush, | ||
VIRTIO_BLK_T_GET_ID => RequestType::GetDeviceID, | ||
VIRTIO_BLK_T_DISCARD => RequestType::Discard, | ||
t => RequestType::Unsupported(t), | ||
} | ||
} | ||
|
@@ -176,6 +180,12 @@ impl PendingRequest { | |
(Ok(transferred_data_len), RequestType::GetDeviceID) => { | ||
Status::from_data(self.data_len, transferred_data_len, true) | ||
} | ||
(Ok(_), RequestType::Discard) => { | ||
block_metrics.discard_count.inc(); | ||
Status::Ok { | ||
num_bytes_to_mem: 0, | ||
} | ||
} | ||
(_, RequestType::Unsupported(op)) => Status::Unsupported { op }, | ||
(Err(err), _) => Status::IoErr { | ||
num_bytes_to_mem: 0, | ||
|
@@ -231,13 +241,23 @@ impl RequestHeader { | |
} | ||
} | ||
|
||
// For this, please see VirtIO v1.2. This struct mimics that of the implementation in Virtio. | ||
#[derive(Debug, Copy, Clone, PartialEq, Eq)] | ||
pub struct DiscardSegment { | ||
sector: u64, | ||
num_sectors: u32, | ||
flags: u32, | ||
} | ||
Comment on lines
+246
to
+250
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
unsafe impl ByteValued for DiscardSegment {} | ||
|
||
#[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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this comment? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh lol that was just a mental note to myself, forgot to take it out. |
||
} | ||
|
||
impl Request { | ||
|
@@ -258,10 +278,11 @@ impl Request { | |
data_addr: GuestAddress(0), | ||
data_len: 0, | ||
status_addr: GuestAddress(0), | ||
discard_segments: None | ||
}; | ||
|
||
let data_desc; | ||
let status_desc; | ||
let mut status_desc; | ||
let desc = avail_desc | ||
.next_descriptor() | ||
.ok_or(VirtioBlockError::DescriptorChainTooShort)?; | ||
|
@@ -313,6 +334,52 @@ impl Request { | |
return Err(VirtioBlockError::InvalidDataLength); | ||
} | ||
} | ||
RequestType::Discard => { | ||
// Get data descriptor | ||
let data_desc = avail_desc.next_descriptor() | ||
.ok_or(VirtioBlockError::DescriptorChainTooShort)?; | ||
Comment on lines
+338
to
+340
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not a place to parse the request. It needs to be done above. There is already a place where |
||
if data_desc.is_write_only() { | ||
return Err(VirtioBlockError::UnexpectedWriteOnlyDescriptor); | ||
} | ||
|
||
// Validate data length | ||
let segment_size = std::mem::size_of::<DiscardSegment>() as u32; // 16 bytes | ||
if data_desc.len % segment_size != 0 { | ||
return Err(VirtioBlockError::InvalidDataLength); | ||
} | ||
|
||
// Calculate number of segments | ||
let num_segments = data_desc.len / segment_size; | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
// Populate DiscardSegments vector | ||
for i in 0..num_segments { | ||
let offset = i * segment_size; | ||
let segment: DiscardSegment = mem.read_obj(data_desc.addr.unchecked_add(offset as u64)) | ||
.map_err(VirtioBlockError::GuestMemory)?; | ||
if segment.flags & !0x1 != 0 { | ||
return Err(VirtioBlockError::InvalidDiscardFlags); | ||
} | ||
let end_sector = segment.sector.checked_add(segment.num_sectors as u64) | ||
.ok_or(VirtioBlockError::SectorOverflow)?; | ||
if end_sector > num_disk_sectors { | ||
return Err(VirtioBlockError::BeyondDiskSize); | ||
} | ||
segments.push(segment); | ||
} | ||
|
||
// Assign to req.discard_segments | ||
req.discard_segments = Some(segments); | ||
req.data_len = data_desc.len; | ||
status_desc = data_desc.next_descriptor() | ||
.ok_or(VirtioBlockError::DescriptorChainTooShort)?; | ||
if !status_desc.is_write_only() { | ||
return Err(VirtioBlockError::UnexpectedReadOnlyDescriptor); | ||
} | ||
} | ||
_ => {} | ||
} | ||
|
||
|
@@ -390,6 +457,22 @@ impl Request { | |
.map_err(IoErr::GetId); | ||
return ProcessingResult::Executed(pending.finish(mem, res, block_metrics)); | ||
} | ||
RequestType::Discard => { | ||
let res = disk | ||
.file_engine | ||
.handle_discard(self.offset(), self.data_len); | ||
|
||
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)), | ||
}), | ||
} | ||
Comment on lines
+465
to
+474
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice hallucinations. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha, that checks out, I'll make that change. |
||
} | ||
RequestType::Unsupported(_) => { | ||
return ProcessingResult::Executed(pending.finish(mem, Ok(0), block_metrics)); | ||
} | ||
|
@@ -730,6 +813,7 @@ mod tests { | |
RequestType::Out => VIRTIO_BLK_T_OUT, | ||
RequestType::Flush => VIRTIO_BLK_T_FLUSH, | ||
RequestType::GetDeviceID => VIRTIO_BLK_T_GET_ID, | ||
RequestType::Discard => VIRTIO_BLK_T_DISCARD, | ||
RequestType::Unsupported(id) => id, | ||
} | ||
} | ||
|
@@ -742,6 +826,7 @@ mod tests { | |
RequestType::Out => VIRTQ_DESC_F_NEXT, | ||
RequestType::Flush => VIRTQ_DESC_F_NEXT, | ||
RequestType::GetDeviceID => VIRTQ_DESC_F_NEXT | VIRTQ_DESC_F_WRITE, | ||
RequestType::Discard => VIRTQ_DESC_F_NEXT, | ||
RequestType::Unsupported(_) => VIRTQ_DESC_F_NEXT, | ||
} | ||
} | ||
|
@@ -833,6 +918,7 @@ mod tests { | |
status_addr, | ||
sector: sector & (NUM_DISK_SECTORS - sectors_len), | ||
data_addr, | ||
discard_segments: None | ||
}; | ||
let mut request_header = RequestHeader::new(virtio_request_id, request.sector); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
libc
crate already has these constants