Skip to content

Commit 7d674c0

Browse files
Iulian Barbuluminitavoicu
Iulian Barbu
authored and
luminitavoicu
committed
block: fixed used bytes reporting
Signed-off-by: Iulian Barbu <[email protected]> Signed-off-by: Laura Loghin <[email protected]>
1 parent c0f1bae commit 7d674c0

File tree

3 files changed

+160
-36
lines changed

3 files changed

+160
-36
lines changed

‎src/devices/src/virtio/block/device.rs‎

Lines changed: 133 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ use logger::{Metric, METRICS};
1919
use rate_limiter::{RateLimiter, TokenType};
2020
use utils::eventfd::EventFd;
2121
use virtio_gen::virtio_blk::*;
22-
use vm_memory::{Bytes, GuestMemoryMmap};
22+
use vm_memory::{Bytes, GuestMemoryError, GuestMemoryMmap};
2323

2424
use super::{
2525
super::{ActivateResult, DeviceState, Queue, VirtioDevice, TYPE_BLOCK, VIRTIO_MMIO_INT_VRING},
@@ -220,13 +220,43 @@ impl Block {
220220
&self.disk_image_id,
221221
) {
222222
Ok(l) => {
223-
len = l;
224-
VIRTIO_BLK_S_OK
223+
// Account for the status byte as well.
224+
// With a non-faulty driver, we shouldn't get to the point where we
225+
// overflow here (since data len must be a multiple of 512 bytes, so
226+
// it can't be u32::MAX). In the future, this should be fixed at the
227+
// request parsing level, so no data will actually be transferred in
228+
// scenarios like this one.
229+
if let Some(l) = l.checked_add(1) {
230+
len = l;
231+
VIRTIO_BLK_S_OK
232+
} else {
233+
len = l;
234+
VIRTIO_BLK_S_IOERR
235+
}
225236
}
226237
Err(e) => {
227-
error!("Failed to execute request: {:?}", e);
228238
METRICS.block.invalid_reqs_count.inc();
229-
len = 1; // We need at least 1 byte for the status.
239+
match e {
240+
ExecuteError::Read(GuestMemoryError::PartialBuffer {
241+
completed,
242+
expected,
243+
}) => {
244+
error!(
245+
"Failed to execute virtio block read request: can only \
246+
write {} of {} bytes.",
247+
completed, expected
248+
);
249+
METRICS.block.read_bytes.add(completed);
250+
// This can not overflow since `completed` < data len which is
251+
// an u32.
252+
len = completed as u32 + 1;
253+
}
254+
_ => {
255+
error!("Failed to execute virtio block request: {:?}", e);
256+
// Status byte only.
257+
len = 1;
258+
}
259+
};
230260
e.status()
231261
}
232262
};
@@ -702,7 +732,7 @@ pub(crate) mod tests {
702732

703733
assert_eq!(vq.used.idx.get(), 1);
704734
assert_eq!(vq.used.ring[0].get().id, 0);
705-
assert_eq!(vq.used.ring[0].get().len, 0);
735+
assert_eq!(vq.used.ring[0].get().len, 1);
706736
assert_eq!(mem.read_obj::<u32>(status_addr).unwrap(), VIRTIO_BLK_S_OK);
707737
}
708738

@@ -725,10 +755,100 @@ pub(crate) mod tests {
725755

726756
assert_eq!(vq.used.idx.get(), 1);
727757
assert_eq!(vq.used.ring[0].get().id, 0);
728-
assert_eq!(vq.used.ring[0].get().len, vq.dtable[1].len.get());
758+
// Added status byte length.
759+
assert_eq!(vq.used.ring[0].get().len, vq.dtable[1].len.get() + 1);
729760
assert_eq!(mem.read_obj::<u32>(status_addr).unwrap(), VIRTIO_BLK_S_OK);
761+
}
762+
763+
// Read with error.
764+
{
765+
vq.used.idx.set(0);
766+
block.set_queue(0, vq.create_queue());
767+
768+
mem.write_obj::<u32>(VIRTIO_BLK_T_IN, request_type_addr)
769+
.unwrap();
770+
vq.dtable[1]
771+
.flags
772+
.set(VIRTQ_DESC_F_NEXT | VIRTQ_DESC_F_WRITE);
773+
774+
let size = block.disk_image.seek(SeekFrom::End(0)).unwrap();
775+
block.disk_image.set_len(size / 2).unwrap();
776+
mem.write_obj(10, GuestAddress(request_type_addr.0 + 8))
777+
.unwrap();
778+
779+
invoke_handler_for_queue_event(&mut block);
780+
781+
assert_eq!(vq.used.idx.get(), 1);
782+
assert_eq!(vq.used.ring[0].get().id, 0);
783+
784+
// Only status byte length.
785+
assert_eq!(vq.used.ring[0].get().len, 1);
786+
assert_eq!(
787+
mem.read_obj::<u32>(status_addr).unwrap(),
788+
VIRTIO_BLK_S_IOERR
789+
);
730790
assert_eq!(mem.read_obj::<u64>(data_addr).unwrap(), 123_456_789);
731791
}
792+
793+
// Partial buffer error on read.
794+
{
795+
vq.used.idx.set(0);
796+
block.set_queue(0, vq.create_queue());
797+
798+
mem.write_obj::<u32>(VIRTIO_BLK_T_IN, request_type_addr)
799+
.unwrap();
800+
vq.dtable[1]
801+
.flags
802+
.set(VIRTQ_DESC_F_NEXT | VIRTQ_DESC_F_WRITE);
803+
804+
let size = block.disk_image.seek(SeekFrom::End(0)).unwrap();
805+
block.disk_image.set_len(size / 2).unwrap();
806+
// Update sector number: stored at `request_type_addr.0 + 8`
807+
mem.write_obj(5, GuestAddress(request_type_addr.0 + 8))
808+
.unwrap();
809+
810+
// This will attempt to read past end of file.
811+
invoke_handler_for_queue_event(&mut block);
812+
813+
assert_eq!(vq.used.idx.get(), 1);
814+
assert_eq!(vq.used.ring[0].get().id, 0);
815+
816+
// No data since can't read past end of file, only status byte length.
817+
assert_eq!(vq.used.ring[0].get().len, 1);
818+
assert_eq!(
819+
mem.read_obj::<u32>(status_addr).unwrap(),
820+
VIRTIO_BLK_S_IOERR
821+
);
822+
assert_eq!(mem.read_obj::<u64>(data_addr).unwrap(), 123_456_789);
823+
}
824+
825+
{
826+
vq.used.idx.set(0);
827+
block.set_queue(0, vq.create_queue());
828+
829+
mem.write_obj::<u32>(VIRTIO_BLK_T_IN, request_type_addr)
830+
.unwrap();
831+
vq.dtable[1]
832+
.flags
833+
.set(VIRTQ_DESC_F_NEXT | VIRTQ_DESC_F_WRITE);
834+
vq.dtable[1].len.set(1024);
835+
836+
mem.write_obj(1, GuestAddress(request_type_addr.0 + 8))
837+
.unwrap();
838+
839+
invoke_handler_for_queue_event(&mut block);
840+
841+
assert_eq!(vq.used.idx.get(), 1);
842+
assert_eq!(vq.used.ring[0].get().id, 0);
843+
844+
// File has 2 sectors and we try to read from the second sector, which means we will
845+
// read 512 bytes (instead of 1024).
846+
assert_eq!(vq.used.ring[0].get().len, 513);
847+
assert_eq!(
848+
mem.read_obj::<u32>(status_addr).unwrap(),
849+
VIRTIO_BLK_S_IOERR
850+
);
851+
}
732852
}
733853

734854
#[test]
@@ -753,7 +873,7 @@ pub(crate) mod tests {
753873
invoke_handler_for_queue_event(&mut block);
754874
assert_eq!(vq.used.idx.get(), 1);
755875
assert_eq!(vq.used.ring[0].get().id, 0);
756-
assert_eq!(vq.used.ring[0].get().len, 0);
876+
assert_eq!(vq.used.ring[0].get().len, 1);
757877
assert_eq!(mem.read_obj::<u32>(status_addr).unwrap(), VIRTIO_BLK_S_OK);
758878
}
759879

@@ -769,7 +889,8 @@ pub(crate) mod tests {
769889
invoke_handler_for_queue_event(&mut block);
770890
assert_eq!(vq.used.idx.get(), 1);
771891
assert_eq!(vq.used.ring[0].get().id, 0);
772-
assert_eq!(vq.used.ring[0].get().len, 0);
892+
// status byte length.
893+
assert_eq!(vq.used.ring[0].get().len, 1);
773894
assert_eq!(mem.read_obj::<u32>(status_addr).unwrap(), VIRTIO_BLK_S_OK);
774895
}
775896
}
@@ -798,7 +919,7 @@ pub(crate) mod tests {
798919
invoke_handler_for_queue_event(&mut block);
799920
assert_eq!(vq.used.idx.get(), 1);
800921
assert_eq!(vq.used.ring[0].get().id, 0);
801-
assert_eq!(vq.used.ring[0].get().len, 0);
922+
assert_eq!(vq.used.ring[0].get().len, 21);
802923
assert_eq!(mem.read_obj::<u32>(status_addr).unwrap(), VIRTIO_BLK_S_OK);
803924

804925
assert!(blk_metadata.is_ok());
@@ -900,7 +1021,7 @@ pub(crate) mod tests {
9001021
// Make sure the data queue advanced.
9011022
assert_eq!(vq.used.idx.get(), 1);
9021023
assert_eq!(vq.used.ring[0].get().id, 0);
903-
assert_eq!(vq.used.ring[0].get().len, 0);
1024+
assert_eq!(vq.used.ring[0].get().len, 1);
9041025
assert_eq!(mem.read_obj::<u32>(status_addr).unwrap(), VIRTIO_BLK_S_OK);
9051026
}
9061027
}
@@ -979,7 +1100,7 @@ pub(crate) mod tests {
9791100
// Make sure the data queue advanced.
9801101
assert_eq!(vq.used.idx.get(), 1);
9811102
assert_eq!(vq.used.ring[0].get().id, 0);
982-
assert_eq!(vq.used.ring[0].get().len, 0);
1103+
assert_eq!(vq.used.ring[0].get().len, 1);
9831104
assert_eq!(mem.read_obj::<u32>(status_addr).unwrap(), VIRTIO_BLK_S_OK);
9841105
}
9851106
}

‎src/devices/src/virtio/block/event_handler.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,7 @@ pub mod tests {
168168

169169
assert_eq!(vq.used.idx.get(), 1);
170170
assert_eq!(vq.used.ring[0].get().id, 0);
171-
assert_eq!(vq.used.ring[0].get().len, 0);
171+
assert_eq!(vq.used.ring[0].get().len, 1);
172172
assert_eq!(mem.read_obj::<u32>(status_addr).unwrap(), VIRTIO_BLK_S_OK);
173173
}
174174
}

‎src/devices/src/virtio/block/request.rs‎

Lines changed: 26 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -213,36 +213,39 @@ impl Request {
213213
.map_err(ExecuteError::Seek)?;
214214

215215
match self.request_type {
216-
RequestType::In => {
217-
mem.read_from(self.data_addr, disk, self.data_len as usize)
218-
.map_err(ExecuteError::Read)?;
219-
METRICS.block.read_bytes.add(self.data_len as usize);
220-
METRICS.block.read_count.inc();
221-
return Ok(self.data_len);
222-
}
223-
RequestType::Out => {
224-
mem.write_to(self.data_addr, disk, self.data_len as usize)
225-
.map_err(ExecuteError::Write)?;
226-
METRICS.block.write_bytes.add(self.data_len as usize);
227-
METRICS.block.write_count.inc();
228-
}
229-
RequestType::Flush => match disk.flush() {
230-
Ok(_) => {
216+
RequestType::In => mem
217+
.read_exact_from(self.data_addr, disk, self.data_len as usize)
218+
.map(|_| {
219+
METRICS.block.read_bytes.add(self.data_len as usize);
220+
METRICS.block.read_count.inc();
221+
self.data_len
222+
})
223+
.map_err(ExecuteError::Read),
224+
RequestType::Out => mem
225+
.write_all_to(self.data_addr, disk, self.data_len as usize)
226+
.map(|_| {
227+
METRICS.block.write_bytes.add(self.data_len as usize);
228+
METRICS.block.write_count.inc();
229+
0
230+
})
231+
.map_err(ExecuteError::Write),
232+
RequestType::Flush => disk
233+
.flush()
234+
.map(|_| {
231235
METRICS.block.flush_count.inc();
232-
return Ok(0);
233-
}
234-
Err(e) => return Err(ExecuteError::Flush(e)),
235-
},
236+
0
237+
})
238+
.map_err(ExecuteError::Flush),
236239
RequestType::GetDeviceID => {
237240
if (self.data_len as usize) < disk_id.len() {
238241
return Err(ExecuteError::BadRequest(Error::InvalidOffset));
239242
}
240243
mem.write_slice(disk_id, self.data_addr)
241-
.map_err(ExecuteError::Write)?;
244+
.map(|_| VIRTIO_BLK_ID_BYTES)
245+
.map_err(ExecuteError::Write)
242246
}
243-
RequestType::Unsupported(t) => return Err(ExecuteError::Unsupported(t)),
244-
};
245-
Ok(0)
247+
RequestType::Unsupported(t) => Err(ExecuteError::Unsupported(t)),
248+
}
246249
}
247250
}
248251

0 commit comments

Comments
 (0)