-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Changes from 2 commits
54e5a1c
6a95328
269c313
88a6802
68d302c
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(()) | ||
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
+247
to
+251
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.
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. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright, you are right, I totally missed the bitfield syntax in C definition. |
||
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 | ||
LDagnachew marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
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)?; | ||
LDagnachew marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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)), | ||
}), | ||
} | ||
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