Page MenuHomeFreeBSD

bhyve - Snapshot Save and Restore multiple devices
Needs ReviewPublic

Authored by ionut.mihalache1506_gmail.com on Thu, Sep 10, 10:21 AM.

Details

Reviewers
jhb
Group Reviewers
bhyve
Summary

This feature implements save-restore for multiple devices of the same type.

Initially the restore feature only restored the first device of a certain type.

The feature was rebased over commit 4a16bd8bb4cf1217720c21ae363626df51474edd.

Test Plan

For a guest that has 2 ahci disks and 2 virtio-net network adapters the teste cases were:

  • create a big file on one of the disk
  • start a copy process of the large file from one disk to the other
  • suspend the guest
  • restore the guest and the copy process should finish and the files are the same in data and size
  • initiate a ping process one virtio-net adapter at a time and suspend the guest
  • on restore the ping process should continue for the used adapter

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

ionut.mihalache1506_gmail.com requested review of this revision.Thu, Sep 10, 10:21 AM

vm_snapshot_kern_structs was called two times and probably caused a problem with save restore for multiple devices

vm_snapshot_kern_structs was called two times and probably caused a problem with save restore for multiple devices

Besides my another comments and questions, what is advantage of changing order of the vm_snapshot_kern_structs and vm_snapshot_user_devs (if look at the diff) ?

usr.sbin/bhyve/atkbdc.c
570

calloc(3) initialises all memory with zero, so you don't need explicitly assign was_restored to 0.

Also this is related to all below "was_restored = 0" changes.

usr.sbin/bhyve/pci_ahci.c
2507

Instead allocating dev_info, I would like to suggest to use one of the following:

  1. Use "sc" that already allocated in the line 2345, i.e. put some fields to sc's struct for every PCI type.
  2. Introduce a functions like:

    snapshot_pci_device_register(pi)"; for pci devices snapshot_device_register(cb, data, name); for non-pci devices

And replace all init code for all devices with those functions.

This make code simpler and incapsulate all allocations, etc. Thus each decl "struct vm_snapshot_dev_info *dev_info" and code below will be removed in xxx_init() functions.

usr.sbin/bhyve/pci_emul.c
2063

If meta_data always should be not NULL, just place assert() here.

2070

I would suggest to call pci_pause()/pci_resume() unconditionally for each PCI device and if pe_pause is NULL do not do anything. The same for pci_resume().
That will simplify initialisation and if PCI device has implemented pe_pause/pe_resume, a handler will be called during suspend/resume. If PCI device does not implement pe_pause/pe_resume, just ignore it.

usr.sbin/bhyve/snapshot.c
529

Probably you should correct a code to conform TODO's requirements and then remove TODO comment.

984

Is this comment still required?

986

What is this function currently used for? It only calls lookup_devs().

Probably lookup_devs() should be renamed with walk_and_restore_devices()

1202

commented code - bad style ?

1266

Put fprintf under ifdef DEBUG or even replace with dprintf?

usr.sbin/bhyve/pci_ahci.c
2507

Some of the information will need to be used outside the context of the device itself - the static array of user devices has been removed, so the snapshot component must keep track of the functions that need to be called when performing a snapshot or restore. Also, for devices like atkbdc, the snapshot_pci_device_register will likely not be compatible.

Before having support for atkbdc, the static userspace device array was also not required, since you could discover most devices through the PCI bus; however, if other device types are to be supported, a more generic approach is required.

However, I agree that some of the logic, like allocating the structure and performing a part of the initialization could be done separately.