Re: [libvirt] [PATCH 1/2] hostdev: Detect untracked inactive devices

On Tue, 2016-03-22 at 07:45 -0400, John Ferlan wrote: > On 03/17/2016 01:24 PM, Andrea Bolognani wrote: >> >> Unmanaged devices are attached to guests in two steps: first, >> the device is detached from the host and marked as inactive; >> subsequently, it is marked as active and attached to the guest. >> >> If the daemon is restarted between these two operations, we lose >> track of the inactive device. >> >> Steps 5 and 6 of virHostdevPreparePCIDevices() already subtly >> take care of this situation, but some planned changes will make >> it so that's no longer the case. Plus, explicit is always better >> than implicit. >> --- >> src/util/virhostdev.c | 47 ++++++++++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 44 insertions(+), 3 deletions(-) >> >> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c >> index e0d6465..7204bd7 100644 >> --- a/src/util/virhostdev.c >> +++ b/src/util/virhostdev.c >> @@ -560,7 +560,8 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, >> } >> } >> >> - /* Step 2: detach managed devices (i.e. bind to appropriate stub driver) */ >> + /* Step 2: detach managed devices and make sure unmanaged devices >> + * have already been taken care of */ >> for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { >> virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); >> >> @@ -577,8 +578,48 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, >> mgr->inactivePCIHostdevs) < 0) >> goto reattachdevs; >> } else { >> - VIR_DEBUG("Not detaching unmanaged PCI device %s", >> - virPCIDeviceGetName(pci)); >> + char *driverPath; >> + char *driverName; >> + int stub; > > stub = -1; May not be needed, see below. >> + >> + /* Unmanaged devices should already have been marked as >> + * inactive: if that's the case, we can simply move on */ >> + if (virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci)) { >> + VIR_DEBUG("Not detaching unmanaged PCI device %s", >> + virPCIDeviceGetName(pci)); >> + continue; >> + } >> + >> + /* If that's not the case, though, it might be because the >> + * daemon has been restarted, causing us to lose track of the >> + * device. Try and recover by marking the device as inactive >> + * if it happens to be bound to a known stub driver. >> + * > > Just to make it clearer (for me) - this is because using the node device > API virHostdevPCINodeDeviceDetach it's possible that somewhere between > virPCIDeviceBindToStub and the virPCIDeviceListAddCopy we had a daemon > restart causing us to lose track of the device, so this code tries to > detect that the BindToStub was done and complete the task. > > Of course it's also possible that BindToStub was "in process" when we > failed, but I'm not sure *how* to detect that... I suppose though all > that matters is we had a completed BindToStub. Mh, not really what I had in mind :) What this code is meant to take care of is the case when virHostdevPCINodeDeviceDetach() completes successfully, marking the device as inactive, but the daemon is restarted before the device can be attached to a guest. That causes us to lose all information in the inactive list.

The current code copes with this because step 5 adds the device to the active list (even if it was never in the inactive list) and step 6 removes the device from the inactive list (even if it was never in the inactive list), but that's extremely opaque and most importantly will stop working once we start moving device objects from one list to the other instead of copying them around. The new code deals with the situation explicitly by adding devices to the inactive list if they look like they belong there - namely, they are bound to a stub driver. As noted in the comment right below, this is sub-optimal and a follow-up patch should take care of storing both the active and inactive list to disk properly so that daemon restarts are no longer a problem. That's for another day, though :) >> + * FIXME Get rid of this once a proper way to keep track of >> + * information about active / inactive device across >> + * daemon restarts has been implemented */ >> + >> + if (virPCIDeviceGetDriverPathAndName(pci, >> + &driverPath, &driverName) < 0) >> + goto reattachdevs; > > I note that virPCIDeviceUnbindFromStub can return 0, but driverName == > NULL. If driverName == NULL, then it declares it's not bound to a known > stub. However, for our purposes, it more than likely means the > BindToStub may not have completed successfully. But still we only *care* > that it did. > >> + >> + stub = virPCIStubDriverTypeFromString(driverName); > > So, I think this becomes, > > if (driverName) > stub = virPCIStubDriverTypeFromString(driverName); virPCIStubDriverTypeFromString() is implemented using virEnumFromString(), which handles NULL gracefully and simply returns -1. That's why I think initializing stub above and adding the check on driverName are not required changes, but I'm not opposed to adding them if you think they make the code clearer.

>> + >> + VIR_FREE(driverPath); >> + VIR_FREE(driverName); >> + >> + if (stub >= 0 && >> + stub != VIR_PCI_STUB_DRIVER_NONE) { > > Strange check since VIR_PCI_STUB_DRIVER_NONE = 0 > > if (stub >= 0 && stub != 0) > > a change to seems to be what you're after > > if (stub > VIR_PCI_STUB_DRIVER_NONE && > stub < VIR_PCI_STUB_DRIVER_LAST) The idea here is that 'stub >= 0' makes sure that virPCIStubDriverTypeFromString() didn't fail, and the second check excludes a value that, while part of the virPCIStub enumeration, we want to reject in this case. But I like your version better, so consider it changed :)

>> + /* The device is bound to a known stub driver: store this >> + * information and add a copy to the inactive list */ >> + virPCIDeviceSetStubDriver(pci, stub); >> + >> + VIR_DEBUG("Adding PCI device %s to inactive list", >> + virPCIDeviceGetName(pci)); >> + if (virPCIDeviceListAddCopy(mgr->inactivePCIHostdevs, pci) < 0) >> + goto reattachdevs; >> + } > > I think perhaps patch 2 should be merged into here - I was kind of > wondering what the "else" condition would be... Until I looked at patch 2. > > ACK with the adjustments - including the merge; otherwise, we fall into > Step 3 still. Patch 2 actually deals with a separate issue, so I'm not sure squashing them together is warranted. If the 'else' branch is missing here, we'll just keep on doing what we're already doing: try to pass a device to the guest, and have QEMU spew out a kinda cryptic error message when it realizes it can't work the VFIO magic on it.