[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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




On 03/22/2016 11:43 AM, Andrea Bolognani wrote:
> 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.

Oh right ... that too...  it's a tangled web.  At least we're not
dealing with detach as well ;-)  I can only imagine how much you look
forward to backporting any of these changes.

> 
> 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.
> 

No need...

>>> +
>>> +            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 :)
> 

Since you're doing this...

>>> +                /* 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.

OK - 13 of one, bakers dozen of another ;-)  Only would be an issue for
someone that falls into the git bisect game.

ACK (to both)

John


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]