Skip to content

Commit

Permalink
[block] simplify request status logic
Browse files Browse the repository at this point in the history 
The request status logic is split between request.rs and device.rs. We
simplify it and move it entirely to request.rs in order to make it
easier to follow.

Signed-off-by: Serban Iorga <seriorga@amazon.com>
  • Loading branch information
Serban Iorga committed Jun 23, 2021
1 parent db710ba commit 216ddff
Show file tree
Hide file tree
Showing 2 changed files with 155 additions and 85 deletions.
55 changes: 17 additions & 38 deletions src/devices/src/virtio/block/device.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use logger::{error, warn, IncMetric, METRICS};
use rate_limiter::{BucketUpdate, RateLimiter, TokenType};
use utils::eventfd::EventFd;
use virtio_gen::virtio_blk::*;
use vm_memory::{Bytes, GuestMemoryError, GuestMemoryMmap};
use vm_memory::{Bytes, GuestMemoryMmap};

use super::{
super::{ActivateResult, DeviceState, Queue, VirtioDevice, TYPE_BLOCK, VIRTIO_MMIO_INT_VRING},
Expand Down Expand Up @@ -283,8 +283,7 @@ impl Block {
let queue = &mut self.queues[queue_index];
let mut used_any = false;
while let Some(head) = queue.pop(mem) {
let len;
match Request::parse(&head, mem) {
let len = match Request::parse(&head, mem) {
Ok(request) => {
// If limiter.consume() fails it means there is no more TokenType::Ops
// budget and rate limiting is in effect.
Expand Down Expand Up @@ -315,49 +314,29 @@ impl Block {
}
}

let status = match request.execute(&mut self.disk, mem) {
Ok(l) => {
// Account for the status byte as well.
// We shouldn't overflow here since inside `request.execute()` we check
// if data_len is a multiple of 512 bytes.
// So the result can't be u32::MAX.
len = l + 1;
VIRTIO_BLK_S_OK
}
Err(e) => {
METRICS.block.invalid_reqs_count.inc();
error!(
"Failed to execute {:?} virtio block request: {:?}",
request.request_type, e
);
len = match e {
ExecuteError::Read(GuestMemoryError::PartialBuffer {
completed,
..
}) => {
// This can not overflow since `completed` < data len which is
// an u32.
completed as u32 + 1
}
_ => {
// Status byte only.
1
}
};
e.status()
}
};
let status = Status::from_result(request.execute(&mut self.disk, mem));
let virtio_blk_status = status.virtio_blk_status();
let num_used_bytes = status.num_used_bytes();
if let Status::Err(err_status) = status {
METRICS.block.invalid_reqs_count.inc();
error!(
"Failed to execute {:?} virtio block request: {:?}",
request.request_type, err_status
);
}

if let Err(e) = mem.write_obj(status, request.status_addr) {
if let Err(e) = mem.write_obj(virtio_blk_status, request.status_addr) {
error!("Failed to write virtio block status: {:?}", e)
}

num_used_bytes
}
Err(e) => {
error!("Failed to parse available descriptor chain: {:?}", e);
METRICS.block.execute_fails.inc();
len = 0;
0
}
}
};

queue.add_used(mem, head.index, len).unwrap_or_else(|e| {
error!(
Expand Down
185 changes: 138 additions & 47 deletions src/devices/src/virtio/block/request.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,28 +18,61 @@ use super::device::{CacheType, DiskProperties};
use super::{Error, SECTOR_SHIFT, SECTOR_SIZE};

#[derive(Debug)]
pub enum ExecuteError {
pub enum IoErrStatus {
BadRequest(Error),
Flush(io::Error),
Read(GuestMemoryError),
// Read(num_used_bytes, GuestMemoryError)
Read(u32, GuestMemoryError),
Seek(io::Error),
SyncAll(io::Error),
Write(GuestMemoryError),
}

#[derive(Debug)]
pub enum ErrStatus {
IoErr(IoErrStatus),
Unsupported(u32),
}

impl ExecuteError {
pub fn status(&self) -> u32 {
match *self {
ExecuteError::BadRequest(_) => VIRTIO_BLK_S_IOERR,
ExecuteError::Flush(_) => VIRTIO_BLK_S_IOERR,
ExecuteError::Read(_) => VIRTIO_BLK_S_IOERR,
ExecuteError::Seek(_) => VIRTIO_BLK_S_IOERR,
ExecuteError::SyncAll(_) => VIRTIO_BLK_S_IOERR,
ExecuteError::Write(_) => VIRTIO_BLK_S_IOERR,
ExecuteError::Unsupported(_) => VIRTIO_BLK_S_UNSUPP,
#[derive(Debug)]
pub enum Status {
// Ok(num_used_bytes)
Ok(u32),
Err(ErrStatus),
}

impl Status {
pub fn from_result(result: result::Result<u32, ErrStatus>) -> Status {
match result {
Ok(status) => Status::Ok(status),
Err(status) => Status::Err(status),
}
}

pub fn virtio_blk_status(&self) -> u32 {
match self {
Status::Ok(_) => VIRTIO_BLK_S_OK,
Status::Err(status) => match status {
ErrStatus::IoErr(_) => VIRTIO_BLK_S_IOERR,
ErrStatus::Unsupported(_) => VIRTIO_BLK_S_UNSUPP,
},
}
}

pub fn num_used_bytes(&self) -> u32 {
let num_used_bytes = match self {
Status::Ok(num_used_bytes) => *num_used_bytes,
Status::Err(status) => match status {
ErrStatus::IoErr(io_err) => match io_err {
IoErrStatus::Read(num_used_bytes, _) => *num_used_bytes,
_ => 0,
},
ErrStatus::Unsupported(_) => 0,
},
};
// account for the status byte
num_used_bytes + 1
}
}

#[derive(Clone, Copy, Debug, PartialEq)]
Expand Down Expand Up @@ -178,23 +211,29 @@ impl Request {
Ok(req)
}

fn execute_seek(&self, disk: &mut DiskProperties) -> result::Result<(), ExecuteError> {
fn execute_seek(&self, disk: &mut DiskProperties) -> result::Result<(), ErrStatus> {
// TODO: perform this logic at request parsing level in the future.
// Check that the data length is a multiple of 512 as specified in the virtio standard.
if u64::from(self.data_len) % SECTOR_SIZE != 0 {
return Err(ExecuteError::BadRequest(Error::InvalidDataLength));
return Err(ErrStatus::IoErr(IoErrStatus::BadRequest(
Error::InvalidDataLength,
)));
}
let top_sector = self
.sector
.checked_add(u64::from(self.data_len) >> SECTOR_SHIFT)
.ok_or(ExecuteError::BadRequest(Error::InvalidOffset))?;
.ok_or(ErrStatus::IoErr(IoErrStatus::BadRequest(
Error::InvalidOffset,
)))?;
if top_sector > disk.nsectors() {
return Err(ExecuteError::BadRequest(Error::InvalidOffset));
return Err(ErrStatus::IoErr(IoErrStatus::BadRequest(
Error::InvalidOffset,
)));
}

disk.file_mut()
.seek(SeekFrom::Start(self.sector << SECTOR_SHIFT))
.map_err(ExecuteError::Seek)?;
.map_err(|e| ErrStatus::IoErr(IoErrStatus::Seek(e)))?;

Ok(())
}
Expand All @@ -203,7 +242,7 @@ impl Request {
&self,
disk: &mut DiskProperties,
mem: &GuestMemoryMmap,
) -> result::Result<u32, ExecuteError> {
) -> result::Result<u32, ErrStatus> {
let cache_type = disk.cache_type();

match self.request_type {
Expand All @@ -216,10 +255,13 @@ impl Request {
self.data_len
})
.map_err(|e| {
let mut num_used_bytes = self.data_len;
if let GuestMemoryError::PartialBuffer { completed, .. } = e {
METRICS.block.read_bytes.add(completed);
// It's safe to cast to u32 since completed < data_len.
num_used_bytes = completed as u32;
}
ExecuteError::Read(e)
ErrStatus::IoErr(IoErrStatus::Read(num_used_bytes, e))
})
}
RequestType::Out => {
Expand All @@ -234,16 +276,20 @@ impl Request {
if let GuestMemoryError::PartialBuffer { completed, .. } = e {
METRICS.block.write_bytes.add(completed);
}
ExecuteError::Write(e)
ErrStatus::IoErr(IoErrStatus::Write(e))
})
}
RequestType::Flush => {
match cache_type {
CacheType::Writeback => {
// flush() first to force any cached data out.
disk.file_mut().flush().map_err(ExecuteError::Flush)?;
disk.file_mut()
.flush()
.map_err(|e| ErrStatus::IoErr(IoErrStatus::Flush(e)))?;
// Sync data out to physical media on host.
disk.file_mut().sync_all().map_err(ExecuteError::SyncAll)?;
disk.file_mut()
.sync_all()
.map_err(|e| ErrStatus::IoErr(IoErrStatus::SyncAll(e)))?;
METRICS.block.flush_count.inc();
}
CacheType::Unsafe => {
Expand All @@ -255,13 +301,15 @@ impl Request {
RequestType::GetDeviceID => {
let disk_id = disk.image_id();
if (self.data_len as usize) < disk_id.len() {
return Err(ExecuteError::BadRequest(Error::InvalidOffset));
return Err(ErrStatus::IoErr(IoErrStatus::BadRequest(
Error::InvalidOffset,
)));
}
mem.write_slice(disk_id, self.data_addr)
.map(|_| VIRTIO_BLK_ID_BYTES)
.map_err(ExecuteError::Write)
.map_err(|e| ErrStatus::IoErr(IoErrStatus::Write(e)))
}
RequestType::Unsupported(t) => Err(ExecuteError::Unsupported(t)),
RequestType::Unsupported(op) => Err(ErrStatus::Unsupported(op)),
}
}
}
Expand Down Expand Up @@ -316,28 +364,71 @@ mod tests {
}

#[test]
fn test_execute_error_status() {
assert_eq!(
ExecuteError::BadRequest(Error::InvalidOffset).status(),
VIRTIO_BLK_S_IOERR
);
assert_eq!(
ExecuteError::Flush(io::Error::from_raw_os_error(42)).status(),
VIRTIO_BLK_S_IOERR
);
assert_eq!(
ExecuteError::Read(GuestMemoryError::InvalidBackendAddress).status(),
VIRTIO_BLK_S_IOERR
);
assert_eq!(
ExecuteError::Seek(io::Error::from_raw_os_error(42)).status(),
VIRTIO_BLK_S_IOERR
);
assert_eq!(
ExecuteError::Write(GuestMemoryError::InvalidBackendAddress).status(),
VIRTIO_BLK_S_IOERR
);
assert_eq!(ExecuteError::Unsupported(42).status(), VIRTIO_BLK_S_UNSUPP);
fn test_status() {
{
let status = Status::from_result(Ok(10));
assert_eq!(status.virtio_blk_status(), VIRTIO_BLK_S_OK);
assert_eq!(status.num_used_bytes(), 11);
}

{
let status = Status::from_result(Err(ErrStatus::IoErr(IoErrStatus::BadRequest(
Error::InvalidOffset,
))));
assert_eq!(status.virtio_blk_status(), VIRTIO_BLK_S_IOERR);
assert_eq!(status.num_used_bytes(), 1);
}

{
let status = Status::from_result(Err(ErrStatus::IoErr(IoErrStatus::Flush(
io::Error::from_raw_os_error(42),
))));
assert_eq!(status.virtio_blk_status(), VIRTIO_BLK_S_IOERR);
assert_eq!(status.num_used_bytes(), 1);
}

{
let status = Status::from_result(Err(ErrStatus::IoErr(IoErrStatus::Read(
0,
GuestMemoryError::InvalidBackendAddress,
))));
assert_eq!(status.virtio_blk_status(), VIRTIO_BLK_S_IOERR);
assert_eq!(status.num_used_bytes(), 1);
}

{
let status = Status::from_result(Err(ErrStatus::IoErr(IoErrStatus::Read(
10,
GuestMemoryError::PartialBuffer {
expected: 10,
completed: 20,
},
))));
assert_eq!(status.virtio_blk_status(), VIRTIO_BLK_S_IOERR);
assert_eq!(status.num_used_bytes(), 11);
}

{
let status = Status::from_result(Err(ErrStatus::IoErr(IoErrStatus::Seek(
io::Error::from_raw_os_error(42),
))));
assert_eq!(status.virtio_blk_status(), VIRTIO_BLK_S_IOERR);
assert_eq!(status.num_used_bytes(), 1);
}

{
let status = Status::from_result(Err(ErrStatus::IoErr(IoErrStatus::Write(
GuestMemoryError::InvalidBackendAddress,
))));
assert_eq!(status.virtio_blk_status(), VIRTIO_BLK_S_IOERR);
assert_eq!(status.num_used_bytes(), 1);
}

{
let status = Status::from_result(Err(ErrStatus::Unsupported(0)));
assert_eq!(status.virtio_blk_status(), VIRTIO_BLK_S_UNSUPP);
assert_eq!(status.num_used_bytes(), 1);
}
}

#[test]
Expand Down

0 comments on commit 216ddff

Please sign in to comment.