bhyve: snapshot impovements for 'blockif' backend

[gusev.vitaliy_gmail.com]

Summary

Fixed:

virtio-blk - not fully supported suspend/resume,
                   queued requests are discarded

Improved:

ahci-cd    - simpify snapshot code

It will also simplify adding NVMe snapshot support.

Sponsored by vStack.

Test Plan

Suspend VM when disk is under heavy loaded IO with dd, fio

Resume - VM should keep working.

[darius.mihaim_gmail.com]

usr.sbin/bhyve/block_if.c

922–923

Is there any way the snapshot process could have this assert fail, while creating the VM snapshot, causing it to be killed? If so, it might be better to have it fail, but let the VM continue running.

[gusev.vitaliy_gmail.com]

usr.sbin/bhyve/block_if.c

922–923

Here are a pair of asserts: one in blockif_snapshot() and another in blockif_request().

The purpose of this pair is to prevent against inconsistent changes in blockif_pause or in blockif_request().

assert should never fail because of :

blockif_pause:
       ...
       while (!bc_empty(bc)) {
          ...
       }

And adding "if(!bc_empty(bc)) goto err" to blockif_snapshot() looks redundant and needlessly.

So this assert is just sanity check.

[jhb]

usr.sbin/bhyve/block_if.c

905

This comment seems stale now?

usr.sbin/bhyve/pci_ahci.c

2639

In this case, won't the cfis always be NULL now so we shouldn't save/restore it at all up above? This would also mean that we could probably replace the cfis changes in D 26266 entirely with this. In addition, if the I/O requests are always idle, we can probably avoid saving/restoring state for these requests since they are all initialized as idle when the device model is instantiated?

[markj]

Overall I like it.

usr.sbin/bhyve/block_if.c

905

Looks like it should simply be deleted.

usr.sbin/bhyve/pci_ahci.c

2639

Indeed, I don't understand the need for D26266 in light of the fact that we're now draining the blockif pending queue when pausing a device. In particular, do we need to save ioreq->cfis at all?

Can't this just be assert(TAILQ_EMPTY(&port->iobhd))?

[gusev.vitaliy_gmail.com]

Corrected according to comments and suggestions.

[gusev.vitaliy_gmail.com]

usr.sbin/bhyve/block_if.c

905

yes, will clean it up.

usr.sbin/bhyve/pci_ahci.c

2639

Good point, Fixed. Thanks!

[gusev.vitaliy_gmail.com]

@jhb @markj Could you look again and approve ?

[jhb]

I will work on merging this. I think it probably makes sense to do this as a couple of commits:

1. The block_if changes (and updating ahci for the block_if changes)

2. The rest of the ahci changes.

3. The pci_virtio_block commit.