Skip to content

Disk operations should be u64 not usize #1116

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.

Sign up for GitHub

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

Open
iximeow wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Disk operations should be u64 not usize #1116

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions bin/propolis-server/src/lib/stats/virtual_disk.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,17 +87,17 @@ impl VirtualDiskStats {
fn on_read_completion(
&mut self,
result: block::Result,
len: usize,
len: u64,
duration: Duration,
) {
let index = match result {
block::Result::Success => {
let _ = self.io_latency[READ_INDEX]
.datum
.sample(duration.as_nanos() as u64);
let _ = self.io_size[READ_INDEX].datum.sample(len as u64);
let _ = self.io_size[READ_INDEX].datum.sample(len);
self.reads.datum += 1;
self.bytes_read.datum += len as u64;
self.bytes_read.datum += len;
return;
}
block::Result::Failure => FAILURE_INDEX,
Expand All @@ -110,17 +110,17 @@ impl VirtualDiskStats {
fn on_write_completion(
&mut self,
result: block::Result,
len: usize,
len: u64,
duration: Duration,
) {
let index = match result {
block::Result::Success => {
let _ = self.io_latency[WRITE_INDEX]
.datum
.sample(duration.as_nanos() as u64);
let _ = self.io_size[WRITE_INDEX].datum.sample(len as u64);
let _ = self.io_size[WRITE_INDEX].datum.sample(len);
self.writes.datum += 1;
self.bytes_written.datum += len as u64;
self.bytes_written.datum += len;
return;
}
block::Result::Failure => FAILURE_INDEX,
Expand Down
26 changes: 13 additions & 13 deletions lib/propolis/src/block/crucible.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ impl WorkerState {
readbuf: &mut Buffer,
mem: &MemCtx,
) -> Result<(), Error> {
let block_size = self.info.block_size as usize;
let block_size = self.info.block_size as u64;

match req.op {
block::Operation::Read(off, len) => {
Expand All @@ -94,7 +94,7 @@ impl WorkerState {

// Perform one large read from crucible, and write from data into
// mappings
readbuf.reset(len_blocks, block_size);
readbuf.reset(len_blocks as usize, block_size as usize);
let _ = block.read(off_blocks, readbuf).await?;

let mut nwritten = 0;
Expand All @@ -104,8 +104,8 @@ impl WorkerState {
)?;
}

if nwritten != len {
return Err(Error::CopyError(nwritten, len));
if nwritten as u64 != len {
return Err(Error::CopyError(nwritten as u64, len));
}
}
block::Operation::Write(off, len) => {
Expand All @@ -120,7 +120,7 @@ impl WorkerState {
// to crucible
let maps =
req.mappings(mem).ok_or_else(|| Error::BadGuestRegion)?;
let mut data = crucible::BytesMut::with_capacity(len);
let mut data = crucible::BytesMut::with_capacity(len as usize);
let mut nread = 0;
for mapping in maps {
let n = mapping.read_bytes_uninit(
Expand All @@ -133,8 +133,8 @@ impl WorkerState {
}
nread += n;
}
if nread != len {
return Err(Error::CopyError(nread, len));
if nread as u64 != len {
return Err(Error::CopyError(nread as u64, len));
}

let _ = block.write(off_blocks, data).await?;
Expand Down Expand Up @@ -406,7 +406,7 @@ pub enum Error {
BlocksizeMismatch,

#[error("copied length {0} did not match expectation {1}")]
CopyError(usize, usize),
CopyError(u64, u64),

#[error("IO Error")]
Io(#[from] io::Error),
Expand All @@ -426,15 +426,15 @@ impl From<Error> for block::Result {

/// Calculate offset (in crucible::Block form) and length in blocksize
fn block_offset_count(
off_bytes: usize,
len_bytes: usize,
block_size: usize,
) -> Result<(crucible::BlockIndex, usize), Error> {
off_bytes: u64,
len_bytes: u64,
block_size: u64,
) -> Result<(crucible::BlockIndex, u64), Error> {
if off_bytes.is_multiple_of(block_size)
&& len_bytes.is_multiple_of(block_size)
{
Ok((
crucible::BlockIndex((off_bytes / block_size) as u64),
crucible::BlockIndex(off_bytes / block_size),
len_bytes / block_size,
))
} else {
Expand Down
11 changes: 5 additions & 6 deletions lib/propolis/src/block/file.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ impl SharedState {
let nbytes = maps
.preadv(self.fp.as_raw_fd(), off as i64)
.map_err(|_| "io error")?;
if nbytes != len {
if nbytes as u64 != len {
return Err("bad read length");
}
}
Expand All @@ -104,7 +104,7 @@ impl SharedState {
let nbytes = maps
.pwritev(self.fp.as_raw_fd(), off as i64)
.map_err(|_| "io error")?;
if nbytes != len {
if nbytes as u64 != len {
return Err("bad write length");
}
}
Expand All @@ -115,10 +115,9 @@ impl SharedState {
}
block::Operation::Discard(off, len) => {
if let Some(mech) = self.discard_mech {
dkioc::do_discard(&self.fp, mech, off as u64, len as u64)
.map_err(|_| {
"io error while attempting to free block(s)"
})?;
dkioc::do_discard(&self.fp, mech, off, len).map_err(
|_| "io error while attempting to free block(s)",
)?;
} else {
unreachable!("handled above in processing_loop()");
}
Expand Down
12 changes: 6 additions & 6 deletions lib/propolis/src/block/in_memory.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ impl SharedState {
})?;

let bytes = self.bytes.lock().unwrap();
process_read_request(&bytes, off as u64, len, &maps)?;
process_read_request(&bytes, off, len, &maps)?;
}
block::Operation::Write(off, len) => {
if self.info.read_only {
Expand All @@ -81,7 +81,7 @@ impl SharedState {
})?;

let mut bytes = self.bytes.lock().unwrap();
process_write_request(&mut bytes, off as u64, len, &maps)?;
process_write_request(&mut bytes, off, len, &maps)?;
}
block::Operation::Flush => {
// nothing to do
Expand Down Expand Up @@ -182,11 +182,11 @@ impl block::Backend for InMemoryBackend {
fn process_read_request(
bytes: &[u8],
offset: u64,
len: usize,
len: u64,
mappings: &[SubMapping],
) -> Result<()> {
let start = offset as usize;
let end = offset as usize + len;
let end = offset as usize + len as usize;

if start >= bytes.len() || end > bytes.len() {
return Err(std::io::Error::new(
Expand Down Expand Up @@ -215,11 +215,11 @@ fn process_read_request(
fn process_write_request(
bytes: &mut [u8],
offset: u64,
len: usize,
len: u64,
mappings: &[SubMapping],
) -> Result<()> {
let start = offset as usize;
let end = offset as usize + len;
let end = offset as usize + len as usize;

if start >= bytes.len() || end > bytes.len() {
return Err(std::io::Error::new(
Expand Down
4 changes: 2 additions & 2 deletions lib/propolis/src/block/mem_async.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ impl SharedState {
unsafe {
let read_ptr = map.raw_writable()?;
let len = map.len();
seg.read(off + nread, read_ptr, len)
seg.read(off as usize + nread, read_ptr, len)
.then_some(nread + len)
}
})
Expand All @@ -80,7 +80,7 @@ impl SharedState {
unsafe {
let write_ptr = map.raw_readable()?;
let len = map.len();
seg.write(off + nwritten, write_ptr, len)
seg.write(off as usize + nwritten, write_ptr, len)
.then_some(nwritten + len)
}
})
Expand Down
12 changes: 3 additions & 9 deletions lib/propolis/src/block/minder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,22 +271,16 @@ impl QueueMinder {
let devqid = devq_id(self.device_id, self.queue_id);
match req.op {
Operation::Read(off, len) => {
probes::block_begin_read!(|| {
(devqid, id, off as u64, len as u64)
});
probes::block_begin_read!(|| { (devqid, id, off, len) });
}
Operation::Write(off, len) => {
probes::block_begin_write!(|| {
(devqid, id, off as u64, len as u64)
});
probes::block_begin_write!(|| { (devqid, id, off, len) });
}
Operation::Flush => {
probes::block_begin_flush!(|| { (devqid, id) });
}
Operation::Discard(off, len) => {
probes::block_begin_discard!(|| {
(devqid, id, off as u64, len as u64)
});
probes::block_begin_discard!(|| { (devqid, id, off, len) });
}
}
let when_started = Instant::now();
Expand Down
4 changes: 2 additions & 2 deletions lib/propolis/src/block/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ pub use attachment::{
};
pub use minder::{DeviceQueue, DeviceRequest};

pub type ByteOffset = usize;
pub type ByteLen = usize;
pub type ByteOffset = u64;
pub type ByteLen = u64;

/// When `block_size` is not specified in [BackendOpts], and the backend itself
/// is not choosing a block size, a default of 512B is used.
Expand Down
6 changes: 2 additions & 4 deletions lib/propolis/src/hw/nvme/requests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,7 @@ impl block::DeviceQueue for NvmeBlockQueue {
));

let bufs = cmd.data(size, &mem).collect();
let req =
Request::new_write(off as usize, size as usize, bufs);
let req = Request::new_write(off, size, bufs);
return Some((req, permit, None));
}
Ok(NvmCmd::Read(cmd)) => {
Expand All @@ -138,8 +137,7 @@ impl block::DeviceQueue for NvmeBlockQueue {
));

let bufs = cmd.data(size, &mem).collect();
let req =
Request::new_read(off as usize, size as usize, bufs);
let req = Request::new_read(off, size, bufs);
return Some((req, permit, None));
}
Ok(NvmCmd::Flush) => {
Expand Down
47 changes: 25 additions & 22 deletions lib/propolis/src/hw/virtio/block.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,17 @@ use futures::future::BoxFuture;
use lazy_static::lazy_static;

/// Sizing for virtio-block is specified in 512B sectors
const SECTOR_SZ: usize = 512;

/// Arbitrary limit to sectors permitted per discard request
const MAX_DISCARD_SECTORS: u32 = ((1024 * 1024) / SECTOR_SZ) as u32;
const SECTOR_SZ: u64 = 512;

/// Arbitrary limit to sectors permitted per discard request.
///
/// `u32` because that is the size of the register by which guests read this value.
const MAX_DISCARD_SECTORS: u32 = const {
let sectors: u64 = (1024 * 1024) / SECTOR_SZ;
// We can't `try_into` and unwrap in a const context, but we can assert and cast!
assert!(sectors <= u32::MAX as u64);
sectors as u32
};

pub struct PciVirtioBlock {
virtio_state: PciVirtioState,
Expand Down Expand Up @@ -68,7 +75,7 @@ impl PciVirtioBlock {
let total_bytes = info.total_size * u64::from(info.block_size);
match id {
BlockReg::Capacity => {
ro.write_u64(total_bytes / SECTOR_SZ as u64);
ro.write_u64(total_bytes / SECTOR_SZ);
}
BlockReg::SegMax => {
// XXX: Copy the static limit from qemu for now
Expand All @@ -82,7 +89,7 @@ impl PciVirtioBlock {
// Arbitrarily limit to 1MiB (or the device size, if smaller)
let sz = u32::min(
MAX_DISCARD_SECTORS,
(info.total_size / SECTOR_SZ as u64) as u32,
(info.total_size / SECTOR_SZ) as u32,
);
ro.write_u32(if info.supports_discard { sz } else { 0 });
}
Expand Down Expand Up @@ -140,18 +147,18 @@ impl block::DeviceQueue for BlockVq {
if !chain.read(&mut breq, &mem) {
todo!("error handling");
}
let off = breq.sector as usize * SECTOR_SZ;
// TODO: handle mul overflow
let off = breq.sector * SECTOR_SZ;
let req = match breq.rtype {
VIRTIO_BLK_T_IN => {
// should be (blocksize * 512) + 1 remaining writable byte for status
// TODO: actually enforce block size
let blocks = (chain.remain_write_bytes() - 1) / SECTOR_SZ;
let blocks =
(chain.remain_write_bytes() - 1) as u64 / SECTOR_SZ;
let sz = blocks * SECTOR_SZ;

if let Some(regions) = chain.writable_bufs(sz) {
probes::vioblk_read_enqueue!(|| (
rid, off as u64, sz as u64
));
if let Some(regions) = chain.writable_bufs(sz as usize) {
probes::vioblk_read_enqueue!(|| (rid, off, sz));
Ok((
block::Request::new_read(off, sz, regions),
CompletionToken { rid, chain },
Expand All @@ -163,13 +170,11 @@ impl block::DeviceQueue for BlockVq {
}
VIRTIO_BLK_T_OUT => {
// should be (blocksize * 512) remaining read bytes
let blocks = chain.remain_read_bytes() / SECTOR_SZ;
let blocks = chain.remain_read_bytes() as u64 / SECTOR_SZ;
let sz = blocks * SECTOR_SZ;

if let Some(regions) = chain.readable_bufs(sz) {
probes::vioblk_write_enqueue!(|| (
rid, off as u64, sz as u64
));
if let Some(regions) = chain.readable_bufs(sz as usize) {
probes::vioblk_write_enqueue!(|| (rid, off, sz));
Ok((
block::Request::new_write(off, sz, regions),
CompletionToken { rid, chain },
Expand All @@ -192,11 +197,9 @@ impl block::DeviceQueue for BlockVq {
if !chain.read(&mut detail, &mem) {
Err(chain)
} else {
let off = detail.sector as usize * SECTOR_SZ;
let sz = detail.num_sectors as usize * SECTOR_SZ;
probes::vioblk_discard_enqueue!(|| (
rid, off as u64, sz as u64,
));
let off = detail.sector * SECTOR_SZ;
let sz = u64::from(detail.num_sectors) * SECTOR_SZ;
probes::vioblk_discard_enqueue!(|| (rid, off, sz));
Ok((
block::Request::new_discard(off, sz),
CompletionToken { rid, chain },
Expand Down
Loading