Fix greedy block device

CHANGELOG.md

@@ -35,6 +35,9 @@ and this project adheres to
   MMDS to set `Content-Type` header correctly (i.e. `Content-Type: text/plain`
   for IMDS-formatted or error responses and `Content-Type: application/json` for
   JSON-formatted responses).
+- [#5260](https://github.com/firecracker-microvm/firecracker/pull/5260): Fixed a
+  bug allowing the block device to starve all other devices when backed by a
+  sufficiently slow drive.
 
 ## [1.12.0]
 

src/firecracker/examples/uffd/uffd_utils.rs

@@ -191,7 +191,7 @@ impl UffdHandler {
 
     fn zero_out(&mut self, addr: u64) -> bool {
         match unsafe { self.uffd.zeropage(addr as *mut _, self.page_size, true) } {
-            Ok(r) if r >= 0 => true,
+            Ok(_) => true,
             Err(Error::ZeropageFailed(error)) if error as i32 == libc::EAGAIN => false,
             r => panic!("Unexpected zeropage result: {:?}", r),
         }

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

@@ -362,6 +362,7 @@ impl Balloon {
                 }
             }
         }
+        queue.advance_used_ring_idx();
 
         if needs_interrupt {
             self.signal_used_queue()?;
@@ -380,6 +381,7 @@ impl Balloon {
             queue.add_used(head.index, 0)?;
             needs_interrupt = true;
         }
+        queue.advance_used_ring_idx();
 
         if needs_interrupt {
             self.signal_used_queue()
@@ -453,6 +455,7 @@ impl Balloon {
         // and sending a used buffer notification
         if let Some(index) = self.stats_desc_index.take() {
             self.queues[STATS_INDEX].add_used(index, 0)?;
+            self.queues[STATS_INDEX].advance_used_ring_idx();
             self.signal_used_queue()
         } else {
             error!("Failed to update balloon stats, missing descriptor.");

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

@@ -384,24 +384,6 @@
         }
     }
 
-    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.
@@ -422,7 +404,6 @@
                         break;
                     }
 
-                    used_any = true;
                     request.process(&mut self.disk, head.index, mem, &self.metrics)
                 }
                 Err(err) => {
@@ -443,16 +424,27 @@
                     break;
                 }
                 ProcessingResult::Executed(finished) => {
-                    Self::add_used_descriptor(
-                        queue,
-                        head.index,
-                        finished.num_bytes_to_mem,
-                        &self.irq_trigger,
-                        &self.metrics,
-                    );
+                    used_any = true;
+                    queue
+                        .add_used(head.index, finished.num_bytes_to_mem)
+                        .unwrap_or_else(|err| {
+                            error!(
+                                "Failed to add available descriptor head {}: {}",
+                                head.index, err
+                            )
+                        });
                 }
             }
         }
+        queue.advance_used_ring_idx();
+
+        if used_any && 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() {
@@ -495,17 +487,26 @@
                         ),
                     };
                     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
+                            )
+                        });
                 }
             }
         }
+        queue.advance_used_ring_idx();
+
+        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) {

src/vmm/src/devices/virtio/mmio.rs

@@ -157,7 +157,7 @@
         //   eventfds, but nothing will happen other than supurious wakeups.
         // . Do not reset config_generation and keep it monotonically increasing
         for queue in self.locked_device().queues_mut() {
-            *queue = Queue::new(queue.get_max_size());
+            *queue = Queue::new(queue.max_size);
         }
     }
 
@@ -253,7 +253,7 @@
                         }
                         features
                     }
-                    0x34 => self.with_queue(0, |q| u32::from(q.get_max_size())),
+                    0x34 => self.with_queue(0, |q| u32::from(q.max_size)),
                     0x44 => self.with_queue(0, |q| u32::from(q.ready)),
                     0x60 => {
                         // For vhost-user backed devices we need some additional
@@ -489,17 +489,17 @@
         assert!(!d.are_queues_valid());
 
         d.queue_select = 0;
-        assert_eq!(d.with_queue(0, Queue::get_max_size), 16);
+        assert_eq!(d.with_queue(0, |q| q.max_size), 16);
         assert!(d.with_queue_mut(|q| q.size = 16));
         assert_eq!(d.locked_device().queues()[d.queue_select as usize].size, 16);
 
         d.queue_select = 1;
-        assert_eq!(d.with_queue(0, Queue::get_max_size), 32);
+        assert_eq!(d.with_queue(0, |q| q.max_size), 32);
         assert!(d.with_queue_mut(|q| q.size = 16));
         assert_eq!(d.locked_device().queues()[d.queue_select as usize].size, 16);
 
         d.queue_select = 2;
-        assert_eq!(d.with_queue(0, Queue::get_max_size), 0);
+        assert_eq!(d.with_queue(0, |q| q.max_size), 0);
         assert!(!d.with_queue_mut(|q| q.size = 16));
 
         assert!(!d.are_queues_valid());

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

@@ -207,7 +207,7 @@ impl RxBuffers {
     /// This will let the guest know that about all the `DescriptorChain` object that has been
     /// used to receive a frame from the TAP.
     fn finish_frame(&mut self, rx_queue: &mut Queue) {
-        rx_queue.advance_used_ring(self.used_descriptors);
+        rx_queue.advance_next_used(self.used_descriptors);
         self.used_descriptors = 0;
         self.used_bytes = 0;
     }
@@ -396,6 +396,7 @@ impl Net {
             NetQueue::Rx => &mut self.queues[RX_INDEX],
             NetQueue::Tx => &mut self.queues[TX_INDEX],
         };
+        queue.advance_used_ring_idx();
 
         if queue.prepare_kick() {
             self.irq_trigger
@@ -1070,6 +1071,7 @@ pub mod tests {
     impl Net {
         pub fn finish_frame(&mut self) {
             self.rx_buffer.finish_frame(&mut self.queues[RX_INDEX]);
+            self.queues[RX_INDEX].advance_used_ring_idx();
         }
     }