fix(block): only notify guest once

Currently block device has a guest notification logic
inside it's request processing loop. This can create a
situation when guest can continuously add more requests to the
queue, making the whole request processing loop arbitrary long.
This is an issue, since it block any other IO from being processed.

The solution is to simply notify guest one time, after all current
requests are processed.

Signed-off-by: Egor Lazarchuk <[email protected]>

Author: ShadowCurse

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

@@ -384,24 +384,6 @@ impl VirtioBlock {
         }
     }
 
-    fn add_used_descriptor(
-        queue: &mut Queue,
-        index: u16,
-        len: u32,
-        irq_trigger: &IrqTrigger,
-        block_metrics: &BlockDeviceMetrics,
-    ) {
-        queue.add_used(index, len).unwrap_or_else(|err| {
-            error!("Failed to add available descriptor head {}: {}", index, err)
-        });
-
-        if queue.prepare_kick() {
-            irq_trigger.trigger_irq(IrqType::Vring).unwrap_or_else(|_| {
-                block_metrics.event_fails.inc();
-            });
-        }
-    }
-
     /// Device specific function for peaking inside a queue and processing descriptors.
     pub fn process_queue(&mut self, queue_index: usize) -> Result<(), InvalidAvailIdx> {
         // This is safe since we checked in the event handler that the device is activated.
@@ -443,17 +425,26 @@ impl VirtioBlock {
                     break;
                 }
                 ProcessingResult::Executed(finished) => {
-                    Self::add_used_descriptor(
-                        queue,
-                        head.index,
-                        finished.num_bytes_to_mem,
-                        &self.irq_trigger,
-                        &self.metrics,
-                    );
+                    queue
+                        .add_used(head.index, finished.num_bytes_to_mem)
+                        .unwrap_or_else(|err| {
+                            error!(
+                                "Failed to add available descriptor head {}: {}",
+                                head.index, err
+                            )
+                        });
                 }
             }
         }
 
+        if queue.prepare_kick() {
+            self.irq_trigger
+                .trigger_irq(IrqType::Vring)
+                .unwrap_or_else(|_| {
+                    self.metrics.event_fails.inc();
+                });
+        }
+
         if let FileEngine::Async(ref mut engine) = self.disk.file_engine {
             if let Err(err) = engine.kick_submission_queue() {
                 error!("BlockError submitting pending block requests: {:?}", err);
@@ -495,17 +486,25 @@ impl VirtioBlock {
                         ),
                     };
                     let finished = pending.finish(mem, res, &self.metrics);
-
-                    Self::add_used_descriptor(
-                        queue,
-                        finished.desc_idx,
-                        finished.num_bytes_to_mem,
-                        &self.irq_trigger,
-                        &self.metrics,
-                    );
+                    queue
+                        .add_used(finished.desc_idx, finished.num_bytes_to_mem)
+                        .unwrap_or_else(|err| {
+                            error!(
+                                "Failed to add available descriptor head {}: {}",
+                                finished.desc_idx, err
+                            )
+                        });
                 }
             }
         }
+
+        if queue.prepare_kick() {
+            self.irq_trigger
+                .trigger_irq(IrqType::Vring)
+                .unwrap_or_else(|_| {
+                    self.metrics.event_fails.inc();
+                });
+        }
     }
 
     pub fn process_async_completion_event(&mut self) {
@@ -1572,14 +1571,14 @@ mod tests {
 
             // Run scenario that doesn't trigger FullSq BlockError: Add sq_size flush requests.
             add_flush_requests_batch(&mut block, &vq, IO_URING_NUM_ENTRIES);
-            simulate_queue_event(&mut block, Some(false));
+            simulate_queue_event(&mut block, Some(true));
             assert!(!block.is_io_engine_throttled);
             simulate_async_completion_event(&mut block, true);
             check_flush_requests_batch(IO_URING_NUM_ENTRIES, &vq);
 
             // Run scenario that triggers FullSqError : Add sq_size + 10 flush requests.
             add_flush_requests_batch(&mut block, &vq, IO_URING_NUM_ENTRIES + 10);
-            simulate_queue_event(&mut block, Some(false));
+            simulate_queue_event(&mut block, Some(true));
             assert!(block.is_io_engine_throttled);
             // When the async_completion_event is triggered:
             // 1. sq_size requests should be processed processed.
@@ -1606,16 +1605,16 @@ mod tests {
             // Run scenario that triggers FullCqError. Push 2 * IO_URING_NUM_ENTRIES and wait for
             // completion. Then try to push another entry.
             add_flush_requests_batch(&mut block, &vq, IO_URING_NUM_ENTRIES);
-            simulate_queue_event(&mut block, Some(false));
+            simulate_queue_event(&mut block, Some(true));
             assert!(!block.is_io_engine_throttled);
             thread::sleep(Duration::from_millis(150));
             add_flush_requests_batch(&mut block, &vq, IO_URING_NUM_ENTRIES);
-            simulate_queue_event(&mut block, Some(false));
+            simulate_queue_event(&mut block, Some(true));
             assert!(!block.is_io_engine_throttled);
             thread::sleep(Duration::from_millis(150));
 
             add_flush_requests_batch(&mut block, &vq, 1);
-            simulate_queue_event(&mut block, Some(false));
+            simulate_queue_event(&mut block, Some(true));
             assert!(block.is_io_engine_throttled);
             simulate_async_completion_event(&mut block, true);
             assert!(!block.is_io_engine_throttled);
@@ -1671,13 +1670,15 @@ mod tests {
             vq.dtable[1].len.set(512);
             mem.write_obj::<u64>(123_456_789, data_addr).unwrap();
 
-            // Following write procedure should fail because of bandwidth rate limiting.
+            // This will fail because of bandwidth rate limiting.
+            // The irq is still triggered because notification suppression
+            // is not enabled.
             {
                 // Trigger the attempt to write.
                 check_metric_after_block!(
                     &block.metrics.rate_limiter_throttled_events,
                     1,
-                    simulate_queue_event(&mut block, Some(false))
+                    simulate_queue_event(&mut block, Some(true))
                 );
 
                 // Assert that limiter is blocked.
@@ -1739,13 +1740,15 @@ mod tests {
             vq.dtable[1].len.set(512);
             mem.write_obj::<u64>(123_456_789, data_addr).unwrap();
 
-            // Following write procedure should fail because of ops rate limiting.
+            // This will fail because of ops rate limiting.
+            // The irq is still triggered because notification suppression
+            // is not enabled.
             {
                 // Trigger the attempt to write.
                 check_metric_after_block!(
                     &block.metrics.rate_limiter_throttled_events,
                     1,
-                    simulate_queue_event(&mut block, Some(false))
+                    simulate_queue_event(&mut block, Some(true))
                 );
 
                 // Assert that limiter is blocked.
@@ -1754,7 +1757,8 @@ mod tests {
                 assert_eq!(vq.used.idx.get(), 0);
             }
 
-            // Do a second write that still fails but this time on the fast path.
+            // Do a second write that still fails but this time on the fast path
+            // which does not call `process_queue`, so no irq notifications.
             {
                 // Trigger the attempt to write.
                 check_metric_after_block!(