forget less NVMe state across migration

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

@@ -1418,6 +1418,14 @@ impl MigrateMulti for PciNvme {
         output.push(ctrl.export().into())?;
         drop(ctrl);
 
+        // We leave `is_enabled` and `device_id` behind here:
+        // * is_enabled is just a mirror of the controller's enabled bit. We'll
+        //   recover it when importing controller state on the other side.
+        // * `device_id` is, like other `define_id` items, tied to non-migrated
+        //   statics in the Propolis process. Migrating it is, absent other
+        //   work, a bug waiting to happen (imagine ID gaps where devices with
+        //   ID 0, 1, and 4 are migrated)
+
         MigrateMulti::export(&self.pci_state, output, ctx)?;
 
         Ok(())
@@ -1432,6 +1440,9 @@ impl MigrateMulti for PciNvme {
 
         let mut ctrl = self.state.lock().unwrap();
         ctrl.import(input, self)?;
+        // Now that the controller is imported, update our mirror of its
+        // enablement bit.
+        self.is_enabled.store(ctrl.ctrl.cc.enabled(), Ordering::Release);
         drop(ctrl);
 
         MigrateMulti::import(&self.pci_state, offer, ctx)?;

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

@@ -733,6 +733,14 @@ impl SubQueue {
 
     pub(super) fn export(&self) -> migrate::NvmeSubQueueV1 {
         let inner = self.state.inner.lock().unwrap();
+
+        // As `cur_head` mirrors `inner.head` but outside the lock, these must
+        // agree. We are about to forget cur_head on the expectation that they
+        // do. If they do not, the device is already broken, we just hadn't
+        // noticed yet. And if they don't match, things will be more and
+        // differently broken.
+        assert_eq!(self.cur_head.load(Ordering::Acquire), inner.head);
+
         migrate::NvmeSubQueueV1 {
             id: self.id,
             size: self.state.size,
@@ -757,6 +765,10 @@ impl SubQueue {
         inner.head = state.head;
         inner.tail = state.tail;
 
+        // With queue state imported, repopulate cur_head from the canonical
+        // value behind the lock.
+        self.cur_head.store(inner.head, Ordering::Release);
+
         Ok(())
     }
 }

phd-tests/tests/src/migrate.rs

@@ -318,14 +318,22 @@ async fn multiple_migrations(ctx: &TestCtx) {
     );
 }
 
+#[phd_testcase]
+async fn migration_smoke_test(ctx: &TestCtx) {
+    let vm = ctx.spawn_default_vm("migration_smoke_test_0").await?;
+    run_smoke_test(ctx, vm).await?;
+}
+
 async fn run_smoke_test(ctx: &TestCtx, mut source: TestVm) -> Result<()> {
     source.launch().await?;
     source.wait_to_boot().await?;
     let lsout =
         source.run_shell_command("ls foo.bar 2> /dev/null").check_err().await?;
     assert_eq!(lsout, "");
 
-    // create an empty file on the source VM.
+    // create an empty file on the source VM. `/` might be a tmpfs, and the
+    // guest may have no writable filesystem even. validation post-migration is
+    // a bit tricky, below.
     source.run_shell_command("touch ./foo.bar").await?;
     source.run_shell_command("sync ./foo.bar").await?;
 
@@ -334,13 +342,72 @@ async fn run_smoke_test(ctx: &TestCtx, mut source: TestVm) -> Result<()> {
         &[Action::MigrateToPropolis(artifacts::DEFAULT_PROPOLIS_ARTIFACT)],
         |target: &TestVm| {
             Box::pin(async {
+                // forget about any pagecache data, dentries, inodes; we want to
+                // read these from the real backing storage, because we want to
+                // make sure it still works!
+                target
+                    .run_shell_command("echo 3 > /proc/sys/vm/drop_caches")
+                    .await
+                    .expect("can drop caches");
+
                 // the file should still exist on the target VM after migration.
                 let lsout = target
                     .run_shell_command("ls foo.bar")
                     .ignore_status()
                     .await
                     .expect("can try to run `ls foo.bar`");
                 assert_eq!(lsout, "foo.bar");
+
+                // we have a small conundrum: it's possible that the home
+                // directory we wrote into is a tmpfs anyway. so by succeeding
+                // this much, we may have really just tested memory is intact.
+                //
+                // ensure we test *some* device emulation by at least trying to
+                // `ls` the thing we booted out. it *should* be a disk, so we'll
+                // exercise some device emulation machinery.. unfortunately
+                // finding a productive place to `ls` is a bit ridiculous:
+
+                let mount_grep = target
+                    .run_shell_command("mount | grep ' / type tmpfs '")
+                    .ignore_status()
+                    .await
+                    .expect("can check mounted filesystems and grep");
+
+                let root_is_tmpfs = !mount_grep.is_empty();
+
+                if root_is_tmpfs {
+                    // the guest is *probably* an Alpine. `/` is a figment of
+                    // its imagination; a tmpfs with the actual disk somewhere
+                    // under /media, like /media/nvme0n1. try looking there, and
+                    // if that fails we'll need to improve the test, phd, or
+                    // both.
+                    target
+                        .run_shell_command("ls /media/nvme0n1/boot")
+                        .await
+                        .expect("can ls the boot data");
+                } else {
+                    // the guest probably has a read-write root. the above, it
+                    // turns out, probably also exercised the disk. check /boot
+                    // anyway for good measure.
+                    target
+                        .run_shell_command("ls /boot")
+                        .await
+                        .expect("can ls the boot data");
+                }
+
+                // the `ls` succeeded ... eventually. it's possible the disk
+                // *was* broken, the guest realized this, reset the disk, and
+                // that brought it back. if so, there's a line about resetting
+                // the controller in dmesg. so if we see that, we've actually
+                // failed to migrate reasonably.
+                let disk_reset = target
+                    .run_shell_command("dmesg | grep 'reset controller'")
+                    .await
+                    .expect("can grep dmesg");
+
+                if !disk_reset.is_empty() {
+                    panic!("controller reset during the test?! {}", disk_reset);
+                }
             })
         },
     )