hypervisor: make available VIRTIO_BLK_F_SEG_MAX and VIRTIO_RING_F_INDIRECT_DESC#575
Conversation
This allows the guest to put in more than one segment per request. It can improve the throughput of the system. Introduce a new check to make sure the queue size configured by the user is large enough to hold at least one segment. Cherry-picked from upstream cloud-hypervisor commit 754d07b3. Signed-off-by: Songqian Li <sionli@tencent.com>
This improves sequential write performance using fio (2888MiB/s -> 3293MiB/s) Cherry-picked from upstream cloud-hypervisor commit 674e834f. Signed-off-by: Songqian Li <sionli@tencent.com>
![cubesandboxbot[bot]](https://avatars.githubusercontent.com/u/20101770?s=60&v=4){kind=small}
| const SECTOR_SHIFT: u8 = 9; | ||
| pub const SECTOR_SIZE: u64 = 0x01 << SECTOR_SHIFT; | ||
|
|
||
| pub const MINIMUM_BLOCK_QUEUE_SIZE: u16 = 2; |
There was a problem hiding this comment.
Naming consideration: MINIMUM_BLOCK_QUEUE_SIZE = 2 suggests 2 is a valid minimum, but the actual minimum acceptable queue size is 3 (since the validation rejects <= 2). Consider renaming to something like BLOCK_QUEUE_OVERHEAD to more accurately describe that this is the number of descriptors reserved per request (header + status), not the minimum acceptable value.
| InvalidQueueSize(s) => { | ||
| write!( | ||
| f, | ||
| "Queue size is smaller than {MINIMUM_BLOCK_QUEUE_SIZE}: {s}" |
There was a problem hiding this comment.
Error message accuracy: When queue_size = 2, the message reads "Queue size is smaller than 2: 2", which is technically incorrect (2 is not smaller than 2, but it is rejected). Consider changing to something like: "Queue size must be greater than {MINIMUM_BLOCK_QUEUE_SIZE}, got {s}" to accurately describe the constraint.
| physical_block_exp, | ||
| min_io_size: (topology.minimum_io_size / logical_block_size) as u16, | ||
| opt_io_size: (topology.optimal_io_size / logical_block_size) as u32, | ||
| seg_max: (queue_size - MINIMUM_BLOCK_QUEUE_SIZE) as u32, |
There was a problem hiding this comment.
Defense-in-depth: Block::new() performs no internal validation of queue_size — it trusts the caller. If a future code path calls it without going through DiskConfig::validate(), the subtraction queue_size - MINIMUM_BLOCK_QUEUE_SIZE would underflow (wrap to u16::MAX - 1 in release, panic in debug). Consider adding a defensive check:
if queue_size <= MINIMUM_BLOCK_QUEUE_SIZE {
return Err(io::Error::new(io::ErrorKind::InvalidInput, ...));
}
| const SECTOR_SHIFT: u8 = 9; | ||
| pub const SECTOR_SIZE: u64 = 0x01 << SECTOR_SHIFT; | ||
|
|
||
| pub const MINIMUM_BLOCK_QUEUE_SIZE: u16 = 2; |
There was a problem hiding this comment.
Naming consideration: MINIMUM_BLOCK_QUEUE_SIZE = 2 suggests 2 is a valid minimum, but the actual minimum acceptable queue size is 3 (since the validation rejects <= 2). Consider renaming to something like BLOCK_QUEUE_OVERHEAD to more accurately describe that this is the number of descriptors reserved per request (header + status), not the minimum acceptable value.
| InvalidQueueSize(s) => { | ||
| write!( | ||
| f, | ||
| "Queue size is smaller than {MINIMUM_BLOCK_QUEUE_SIZE}: {s}" |
There was a problem hiding this comment.
Error message accuracy: When queue_size = 2, the message reads "Queue size is smaller than 2: 2", which is technically incorrect (2 is not smaller than 2, but it is rejected). Consider changing to something like: "Queue size must be greater than {MINIMUM_BLOCK_QUEUE_SIZE}, got {s}" to accurately describe the constraint.
| physical_block_exp, | ||
| min_io_size: (topology.minimum_io_size / logical_block_size) as u16, | ||
| opt_io_size: (topology.optimal_io_size / logical_block_size) as u32, | ||
| seg_max: (queue_size - MINIMUM_BLOCK_QUEUE_SIZE) as u32, |
There was a problem hiding this comment.
Defense-in-depth: Block::new() performs no internal validation of queue_size — it trusts the caller. If a future code path calls it without going through DiskConfig::validate(), the subtraction queue_size - MINIMUM_BLOCK_QUEUE_SIZE would underflow (wrap to u16::MAX - 1 in release, panic in debug). Consider adding a defensive check at the start of new():
if queue_size <= MINIMUM_BLOCK_QUEUE_SIZE {
return Err(io::Error::new(io::ErrorKind::InvalidInput, ...));
}
2 participants
block: make available VIRTIO_BLK_F_SEG_MAX and VIRTIO_RING_F_INDIRECT_DESC
VIRTIO_BLK_F_SEG_MAX allows the guest to put in more than one segment per request. It can improve the throughput of the system.