nvme: deadlock enabling doorbell buffer while doing I/O (#1080)

9a3a93a introduced a bug in the previously-okay SubQueue::pop; before,
acc_mem.access() would go through MemAccessor, acquire a reference on
the underlying MemCtx (an Arc refcount bump), drop the lock, and
continue. immediately after, SubQueue::pop would lock the SubQueue's
state and actually perform the pop.

this ordering is backwards from set_db_buf, which locks the SubQueue's
state *then* conditionally performs acc_mem.access() if the buffer is
set up as part of an import.

after 9a3a93a, SubQueue::pop uses access_borrowed() to avoid contention
on the refcount for MemCtx, the cost of retaining the lock on the
SubQueue's MemAccessor node while taking the lock on the same queue's
SubQueueState. at this point a concurrent SubQueue::pop and
SubQueue::set_db_buf could deadlock; one holds the queue state lock, the
other holds the accessor node lock, and both will try to take the other.

resolve this tension by picking a consistent ordering for the queue
state and acc_mem locks: SubQueueState first, then
acc_mem.{access,_borrow}(). this was already the ordering in
CompQueue::push and both set_db_buf, so pop() gets the trivial change.

Author: iximeow

lib/propolis/src/hw/nvme/queue.rs

@@ -113,7 +113,13 @@ struct QueueState<QS> {
     /// a `SubQueueState` for a Submission Queue.
     inner: Mutex<QueueInner<QS>>,
 
-    pub acc_mem: MemAccessor,
+    /// This queue's memory accessor node.
+    ///
+    /// Be careful about lock ordering when using this accessor; access_borrow()
+    /// holds this node's lock. If a user of this queue state requires both
+    /// `access_borrow()` and `QueueInner`, the protocol is to lock queue
+    /// state first and this accessor second.
+    acc_mem: MemAccessor,
 }
 impl<QS> QueueState<QS> {
     fn new(size: u32, acc_mem: MemAccessor, inner: QS) -> Self {
@@ -649,12 +655,15 @@ impl SubQueue {
     pub fn pop(
         self: &Arc<SubQueue>,
     ) -> Option<(GuestData<SubmissionQueueEntry>, Permit, u16)> {
+        // Lock the SubQueueState early to conform to lock ordering requirement;
+        // see docs on QueueState::acc_mem.
+        let mut state = self.state.lock();
+
         let Some(mem) = self.state.acc_mem.access_borrow() else { return None };
         let mem = mem.view();
 
         // Attempt to reserve an entry on the Completion Queue
         let permit = self.cq.reserve_entry(&self, &mem)?;
-        let mut state = self.state.lock();
 
         // Check for last-minute updates to the tail via any configured doorbell
         // page, prior to attempting the pop itself.