Page MenuHomeFreeBSD

bhyve: snapshot impovements for 'blockif' backend
ClosedPublic

Authored by gusev.vitaliy_gmail.com on Sep 1 2020, 11:18 AM.
Tags
Referenced Files
Unknown Object (File)
Tue, Feb 13, 8:36 PM
Unknown Object (File)
Dec 22 2023, 12:17 AM
Unknown Object (File)
Dec 20 2023, 4:57 AM
Unknown Object (File)
Dec 18 2023, 7:42 PM
Unknown Object (File)
Dec 10 2023, 11:54 PM
Unknown Object (File)
Dec 10 2023, 4:09 AM
Unknown Object (File)
Dec 9 2023, 4:00 AM
Unknown Object (File)
Dec 8 2023, 3:50 AM

Details

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.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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.

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.

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?

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 edited the summary of this revision. (Show Details)

Corrected according to comments and suggestions.

gusev.vitaliy_gmail.com added inline comments.
usr.sbin/bhyve/block_if.c
905

yes, will clean it up.

usr.sbin/bhyve/pci_ahci.c
2639

Good point, Fixed. Thanks!

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)
  1. The rest of the ahci changes.
  1. The pci_virtio_block commit.
This revision is now accepted and ready to land.Jun 23 2022, 4:53 PM