nvme: deadlock enabling doorbell buffer while doing I/O

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 SubQueue
state and acc_mem locks: SubQueueState first, then
acc_mem.{access,_borrow}(). this was already the ordering in
SubQueueState::push and set_db_buf, so pop() gets the trivial change.

Author: iximeow

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

@@ -649,13 +649,13 @@ impl SubQueue {
     pub fn pop(
         self: &Arc<SubQueue>,
     ) -> Option<(GuestData<SubmissionQueueEntry>, Permit, u16)> {
-        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();
 
+        let Some(mem) = self.state.acc_mem.access_borrow() else { return None };
+        let mem = mem.view();
+
         // Check for last-minute updates to the tail via any configured doorbell
         // page, prior to attempting the pop itself.
         state.db_buf_read(self.devq_id(), &mem);