[libvirt] [PATCH v2] nwfilter: fix deadlock caused updating network device and nwfilter

[Pavel Hrdina]

On 11/12/2014 05:17 PM, John Ferlan wrote:
>
>
> On 11/12/2014 04:51 AM, Pavel Hrdina wrote:
>> Commit 6e5c79a1 tried to fix deadlock between nwfilter{Define,Undefine}
>> and starting of guest, but this same deadlock is also for
>
> s/is also/exists/
>
>> updating/attaching network device to domain.
>>
>> The deadlock was introduced by removing global QEMU driver lock because
>> nwfilter was counting on this lock and ensure that all driver locks are
>> locked inside of nwfilter{Define,Undefine}.
>>
>> This patch extends usage of virNWFilterReadLockFilterUpdates to prevent
>> the deadlock for all possible paths in QEMU driver. LXC and UML drivers
>> still have global lock.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1143780
>>
>> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
>> ---
>>   src/qemu/qemu_driver.c    | 12 ++++++++++++
>>   src/qemu/qemu_migration.c |  3 +++
>>   src/qemu/qemu_process.c   |  4 ++++
>>   3 files changed, 19 insertions(+)
>>
>
> I thought I had built these yesterday when reviewing, but apparently not
> as doing a build just now failed because the symbols are not defined in
> qemu_migration.c and qemu_process.c (I have gcc version 4.8.3 20140911
> (Red Hat 4.8.3-7) (GCC) on my f20 box)
>
> I squashed the following and it builds...
>
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 533bb35..9106800 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -50,6 +50,7 @@
>   #include "viruuid.h"
>   #include "virtime.h"
>   #include "locking/domain_lock.h"
> +#include "nwfilter_conf.h"
>   #include "rpc/virnetsocket.h"
>   #include "virstoragefile.h"
>   #include "viruri.h"
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 0078a70..1c1476c 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -57,6 +57,7 @@
>   #include "domain_nwfilter.h"
>   #include "locking/domain_lock.h"
>   #include "network/bridge_driver.h"
> +#include "nwfilter_conf.h"
>   #include "viruuid.h"
>   #include "virprocess.h"
>   #include "virtime.h"
>
> ACK with the squashed in #includes
>

Thanks, I've already found that build issue and fixed it.

>
> John
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 56e8430..716e9a4 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -5902,6 +5902,8 @@ qemuDomainRestoreFlags(virConnectPtr conn,
>>                     VIR_DOMAIN_SAVE_PAUSED, -1);
>>
>>
>> +    virNWFilterReadLockFilterUpdates();
>> +
>>       fd = qemuDomainSaveImageOpen(driver, path, &def, &header, &xml,
>>                                    (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0,
>>                                    &wrapperFd, false, false);
>> @@ -5979,6 +5981,7 @@ qemuDomainRestoreFlags(virConnectPtr conn,
>>       virFileWrapperFdFree(wrapperFd);
>>       if (vm)
>>           virObjectUnlock(vm);
>> +    virNWFilterUnlockFilterUpdates();
>>       return ret;
>>   }
>>
>> @@ -7500,6 +7503,8 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
>>       virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
>>                     VIR_DOMAIN_AFFECT_CONFIG, -1);
>>
>> +    virNWFilterReadLockFilterUpdates();
>> +
>>       cfg = virQEMUDriverGetConfig(driver);
>>
>>       affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG);
>> @@ -7616,6 +7621,7 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
>>           virObjectUnlock(vm);
>>       virObjectUnref(caps);
>>       virObjectUnref(cfg);
>> +    virNWFilterUnlockFilterUpdates();
>>       return ret;
>>   }
>>
>> @@ -7646,6 +7652,8 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
>>                     VIR_DOMAIN_AFFECT_CONFIG |
>>                     VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1);
>>
>> +    virNWFilterReadLockFilterUpdates();
>> +
>>       cfg = virQEMUDriverGetConfig(driver);
>>
>>       affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG);
>> @@ -7762,6 +7770,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
>>           virObjectUnlock(vm);
>>       virObjectUnref(caps);
>>       virObjectUnref(cfg);
>> +    virNWFilterUnlockFilterUpdates();
>>       return ret;
>>   }
>>
>> @@ -14503,6 +14512,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>>        * and use of FORCE can cause multiple transitions.
>>        */
>>
>> +    virNWFilterReadLockFilterUpdates();
>> +
>>       if (!(vm = qemuDomObjFromSnapshot(snapshot)))
>>           return -1;
>>
>> @@ -14824,6 +14835,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>>           virObjectUnlock(vm);
>>       virObjectUnref(caps);
>>       virObjectUnref(cfg);
>> +    virNWFilterUnlockFilterUpdates();
>>
>>       return ret;
>>   }
>> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
>> index c797206..533bb35 100644
>> --- a/src/qemu/qemu_migration.c
>> +++ b/src/qemu/qemu_migration.c
>> @@ -2525,6 +2525,8 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
>>       if (virTimeMillisNow(&now) < 0)
>>           return -1;
>>
>> +    virNWFilterReadLockFilterUpdates();
>> +
>>       if (flags & VIR_MIGRATE_OFFLINE) {
>>           if (flags & (VIR_MIGRATE_NON_SHARED_DISK |
>>                        VIR_MIGRATE_NON_SHARED_INC)) {
>> @@ -2826,6 +2828,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
>>           qemuDomainEventQueue(driver, event);
>>       qemuMigrationCookieFree(mig);
>>       virObjectUnref(caps);
>> +    virNWFilterUnlockFilterUpdates();
>>       return ret;
>>
>>    stop:
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 24e5f0f..0078a70 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -3438,6 +3438,8 @@ qemuProcessReconnect(void *opaque)
>>
>>       VIR_FREE(data);
>>
>> +    virNWFilterReadLockFilterUpdates();
>> +
>>       virObjectLock(obj);
>>
>>       cfg = virQEMUDriverGetConfig(driver);
>> @@ -3589,6 +3591,7 @@ qemuProcessReconnect(void *opaque)
>>
>>       virObjectUnref(conn);
>>       virObjectUnref(cfg);
>> +    virNWFilterUnlockFilterUpdates();
>>
>>       return;
>>
>> @@ -3624,6 +3627,7 @@ qemuProcessReconnect(void *opaque)
>>       }
>>       virObjectUnref(conn);
>>       virObjectUnref(cfg);
>> +    virNWFilterUnlockFilterUpdates();
>>   }
>>
>>   static int
>>