Re: [PATCH v5 2/4] blockjob: properly track blockcopy xml changes on disk

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

 



On 07/29/2014 05:40 AM, Peter Krempa wrote:

>> Fix things by saving state any time we modify live XML, while
>> delaying XML disk modifications until after the event completes.  We
>> still need a to teach libvirtd restarts to examine all existing
>> <mirror> elements to see if the job completed in the meantime (that
>> is, if libvirtd misses the event, the updated state still needs to be
>> updated in live XML), but that will be a later patch, in part because
>> we also need to to start taking advantage of newer qemu's ability to
>> keep the job around after completion rather than the current usage
>> where the job disappears both on error and on success.
>>

>> +        /* If we completed a block pull or commit, then update the XML
>> +         * to match.  */
>> +        if (status == VIR_DOMAIN_BLOCK_JOB_COMPLETED) {
>> +            bool has_mirror = !!disk->mirror;
> 
> 
> The control flow is becoming less obvious in this function.

Yeah, I struggled on that for a while.

> 
>> +
>> +            if (disk->mirrorState == VIR_DOMAIN_DISK_MIRROR_PIVOT) {
>> +                /* XXX We want to revoke security labels and disk
>> +                 * lease, as well as audit that revocation, before
>> +                 * dropping the original source.  But it gets tricky
>> +                 * if both source and mirror share common backing
>> +                 * files (we want to only revoke the non-shared
>> +                 * portion of the chain); so for now, we leak the
>> +                 * access to the original.  */
>> +                virStorageSourceFree(disk->src);
>> +                disk->src = disk->mirror;
>> +                disk->mirror = NULL;
> 
> If we were here, then
> 
>> +            }
>> +            if (has_mirror) {
> 
> This condition is also true and we try to free the source of the mirror
> again, even if we did it above. On the other hand, this is reached even
> if the _ABORT job is completed, where this makes sense.

Basically, once a job completes, we have to clean up disk->mirror
whether it was abort or pivot; but if it was pivot, we also have to
adjust the source to be the former mirror contents.  But I agree that a
minor tweak to the flow makes it easier to read.


>>              qemuDomainDetermineDiskChain(driver, vm, disk, true);
>> -        if (disk->mirror && type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) {
>> +        }
>> +
>> +        if (disk->mirror &&
> 
> Now this really is an else section to the above as both paths above
> clear disk->mirror.

Also correct.

> 
> ACK, the function flow isn't obvious but it's correct from my POV

After rebasing on top of 1/4, here's what I squashed in to improve the
control flow, before pushing 1 and 2.  I'll wait on 3 and 4 for any
other opinions on whether active commit deserves to be in rc2.

diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c
index 1340c8d..48da843 100644
--- i/src/qemu/qemu_process.c
+++ w/src/qemu/qemu_process.c
@@ -1033,8 +1033,6 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon
ATTRIBUTE_UNUSED,
         /* If we completed a block pull or commit, then update the XML
          * to match.  */
         if (status == VIR_DOMAIN_BLOCK_JOB_COMPLETED) {
-            bool has_mirror = !!disk->mirror;
-
             if (disk->mirrorState == VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT) {
                 /* XXX We want to revoke security labels and disk
                  * lease, as well as audit that revocation, before
@@ -1045,22 +1043,19 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon
ATTRIBUTE_UNUSED,
                  * access to the original.  */
                 virStorageSourceFree(disk->src);
                 disk->src = disk->mirror;
-                disk->mirror = NULL;
-            }
-            if (has_mirror) {
+            } else {
                 virStorageSourceFree(disk->mirror);
-                disk->mirror = NULL;
-                save = disk->mirrorState !=
VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
-                disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
             }
+
             /* Recompute the cached backing chain to match our
              * updates.  Better would be storing the chain ourselves
              * rather than reprobing, but we haven't quite completed
              * that conversion to use our XML tracking. */
+            disk->mirror = NULL;
+            save = disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
+            disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
             qemuDomainDetermineDiskChain(driver, vm, disk, true);
-        }
-
-        if (disk->mirror && type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) {
+        } else if (disk->mirror && type ==
VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) {
             if (status == VIR_DOMAIN_BLOCK_JOB_READY) {
                 disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY;
                 save = true;


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]