On 05/08/2012 12:42 PM, Laine Stump wrote:
On 05/07/2012 07:00 PM, Eric Blake wrote:
It turns out that when cgroups are enabled, the use of a block device for a snapshot target was failing with EPERM due to libvirt failing to add the block device to the cgroup whitelist. See also https://bugzilla.redhat.com/show_bug.cgi?id=810200
* src/qemu/qemu_driver.c (qemuDomainSnapshotCreateSingleDiskActive) (qemuDomainSnapshotUndoSingleDiskActive): Account for cgroup. (qemuDomainSnapshotCreateDiskActive): Update caller. --- @@ -9948,8 +9949,15 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver,
if (virDomainLockDiskAttach(driver->lockManager, vm, disk) < 0) goto cleanup; + if (cgroup && qemuSetupDiskCgroup(driver, vm, cgroup, disk) < 0) {
As far as I can see, you can't get to here if cgroup is NULL.
cgroup can be NULL; see below...
Technically there's nothing wrong with checking it, but isn't it unnecessary?
No, it's necessary (unless we fix qemuSetupDiskCgroup to be a no-op when cgroup is NULL).
@@ -10084,6 +10095,7 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, int thaw = 0; /* 1 if freeze succeeded, -1 if freeze failed */ bool atomic = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC) != 0; bool reuse = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0; + virCgroupPtr cgroup = NULL;
if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) return -1; @@ -10094,6 +10106,14 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, goto endjob; }
+ if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES) && + virCgroupForDomain(driver->cgroup, vm->def->name, &cgroup, 0)) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to find cgroup for %s"), + vm->def->name); + goto endjob; + } +
By the time you get here, you're guaranteed the cgroup is non-NULL, right?
No. If qemuCgroupControllerActive() returns false (possible if your qemu.conf left "devices" out of cgroup_controllers, or if you don't have cgroups mounted in the kernel at all), then we never call virCgroupForDomain to populate cgroup with a non-NULL value.
Also, looking beyond the context provided by git - virCGroupFree() is called in cleanup, which is higher up in the function than end_job, but there are 2 occurrences of goto end_job past where you get the cgroup - if you encountered an error and took either of those goto's, you would end up leaking cgroup.
Eek; I'll fix that and provide a v2.
@@ -10156,7 +10176,7 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, } }
- ret = qemuDomainSnapshotCreateSingleDiskActive(driver, vm, + ret = qemuDomainSnapshotCreateSingleDiskActive(driver, vm, cgroup,
So back to the other topic - this is the only place this function is called, and you're guaranteed that cgroup will be non-NULL, so I don't think qemuDomainSnapshotCreateSingleDiskActive() actually needs to check for NULL cgroup.
Calling with NULL cgroup is permissible.
@@ -10214,6 +10234,8 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, }
cleanup: + if (cgroup) + virCgroupFree(&cgroup);
I think this either needs to be moved down to end_job, or the to goto end_job's I mentioned above need to goto cleanup (I think I prefer the former).
cleanup falls through to endjob - we have to use cleanup if we pause the cpus or if we modify state that needs saving. But since we do indeed have a 'goto endjob' that occurs after cgroup might be set, we do indeed need to move the virCgroupFree() into the endjob label. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org