Skip to content

Commit 9eedd55

Browse files
committed
fix(block): only trigger notification if there's any used descriptor
We shouldn't be sending the notification when we don't write any new used descriptor. This changes the code to only send a notification when a new descriptor is added to the ring and reverts the test expectations to the previous conditions (before 1083c09). Fixes: 1083c09 ("fix(block): only notify guest once") Signed-off-by: Riccardo Mancini <[email protected]>
1 parent a15b50d commit 9eedd55

File tree

1 file changed

+12
-17
lines changed
  • src/vmm/src/devices/virtio/block/virtio

1 file changed

+12
-17
lines changed

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

Lines changed: 12 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -404,7 +404,6 @@ impl VirtioBlock {
404404
break;
405405
}
406406

407-
used_any = true;
408407
request.process(&mut self.disk, head.index, mem, &self.metrics)
409408
}
410409
Err(err) => {
@@ -425,6 +424,7 @@ impl VirtioBlock {
425424
break;
426425
}
427426
ProcessingResult::Executed(finished) => {
427+
used_any = true;
428428
queue
429429
.add_used(head.index, finished.num_bytes_to_mem)
430430
.unwrap_or_else(|err| {
@@ -438,7 +438,7 @@ impl VirtioBlock {
438438
}
439439
queue.advance_used_ring_idx();
440440

441-
if queue.prepare_kick() {
441+
if used_any && queue.prepare_kick() {
442442
self.irq_trigger
443443
.trigger_irq(IrqType::Vring)
444444
.unwrap_or_else(|_| {
@@ -1574,14 +1574,14 @@ mod tests {
15741574

15751575
// Run scenario that doesn't trigger FullSq BlockError: Add sq_size flush requests.
15761576
add_flush_requests_batch(&mut block, &vq, IO_URING_NUM_ENTRIES);
1577-
simulate_queue_event(&mut block, Some(true));
1577+
simulate_queue_event(&mut block, Some(false));
15781578
assert!(!block.is_io_engine_throttled);
15791579
simulate_async_completion_event(&mut block, true);
15801580
check_flush_requests_batch(IO_URING_NUM_ENTRIES, &vq);
15811581

15821582
// Run scenario that triggers FullSqError : Add sq_size + 10 flush requests.
15831583
add_flush_requests_batch(&mut block, &vq, IO_URING_NUM_ENTRIES + 10);
1584-
simulate_queue_event(&mut block, Some(true));
1584+
simulate_queue_event(&mut block, Some(false));
15851585
assert!(block.is_io_engine_throttled);
15861586
// When the async_completion_event is triggered:
15871587
// 1. sq_size requests should be processed processed.
@@ -1608,16 +1608,16 @@ mod tests {
16081608
// Run scenario that triggers FullCqError. Push 2 * IO_URING_NUM_ENTRIES and wait for
16091609
// completion. Then try to push another entry.
16101610
add_flush_requests_batch(&mut block, &vq, IO_URING_NUM_ENTRIES);
1611-
simulate_queue_event(&mut block, Some(true));
1611+
simulate_queue_event(&mut block, Some(false));
16121612
assert!(!block.is_io_engine_throttled);
16131613
thread::sleep(Duration::from_millis(150));
16141614
add_flush_requests_batch(&mut block, &vq, IO_URING_NUM_ENTRIES);
1615-
simulate_queue_event(&mut block, Some(true));
1615+
simulate_queue_event(&mut block, Some(false));
16161616
assert!(!block.is_io_engine_throttled);
16171617
thread::sleep(Duration::from_millis(150));
16181618

16191619
add_flush_requests_batch(&mut block, &vq, 1);
1620-
simulate_queue_event(&mut block, Some(true));
1620+
simulate_queue_event(&mut block, Some(false));
16211621
assert!(block.is_io_engine_throttled);
16221622
simulate_async_completion_event(&mut block, true);
16231623
assert!(!block.is_io_engine_throttled);
@@ -1673,15 +1673,13 @@ mod tests {
16731673
vq.dtable[1].len.set(512);
16741674
mem.write_obj::<u64>(123_456_789, data_addr).unwrap();
16751675

1676-
// This will fail because of bandwidth rate limiting.
1677-
// The irq is still triggered because notification suppression
1678-
// is not enabled.
1676+
// Following write procedure should fail because of bandwidth rate limiting.
16791677
{
16801678
// Trigger the attempt to write.
16811679
check_metric_after_block!(
16821680
&block.metrics.rate_limiter_throttled_events,
16831681
1,
1684-
simulate_queue_event(&mut block, Some(true))
1682+
simulate_queue_event(&mut block, Some(false))
16851683
);
16861684

16871685
// Assert that limiter is blocked.
@@ -1743,15 +1741,13 @@ mod tests {
17431741
vq.dtable[1].len.set(512);
17441742
mem.write_obj::<u64>(123_456_789, data_addr).unwrap();
17451743

1746-
// This will fail because of ops rate limiting.
1747-
// The irq is still triggered because notification suppression
1748-
// is not enabled.
1744+
// Following write procedure should fail because of ops rate limiting.
17491745
{
17501746
// Trigger the attempt to write.
17511747
check_metric_after_block!(
17521748
&block.metrics.rate_limiter_throttled_events,
17531749
1,
1754-
simulate_queue_event(&mut block, Some(true))
1750+
simulate_queue_event(&mut block, Some(false))
17551751
);
17561752

17571753
// Assert that limiter is blocked.
@@ -1760,8 +1756,7 @@ mod tests {
17601756
assert_eq!(vq.used.idx.get(), 0);
17611757
}
17621758

1763-
// Do a second write that still fails but this time on the fast path
1764-
// which does not call `process_queue`, so no irq notifications.
1759+
// Do a second write that still fails but this time on the fast path.
17651760
{
17661761
// Trigger the attempt to write.
17671762
check_metric_after_block!(

0 commit comments

Comments
 (0)