[libvirt] [PATCH] qemu: Adjust size for qcow2/qed if not on sector boundary

Wed May 7 19:05:12 UTC 2014

On 05/07/2014 10:34 AM, Ján Tomko wrote:
> On 05/07/2014 01:58 PM, John Ferlan wrote:
>>
>>
>> On 04/08/2014 12:26 PM, John Ferlan wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1002813
>>>
>>> If qemuDomainBlockResize() is passed a size not on a KiB boundary - that
>>> is passed a size based in bytes (VIR_DOMAIN_BLOCK_RESIZE_BYTES), then
>>> depending on the source format (qcow2 or qed), the value passed must
>>> be on a sector (or 512 byte) boundary. Since other libvirt code quietly
>>> adjusts the capacity values, then do so here as well - of course ensuring
>>> that adjustment still fits.
>>>
>>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>>> ---
>>>  src/qemu/qemu_driver.c | 22 ++++++++++++++++++++++
>>>  1 file changed, 22 insertions(+)
>>>
>>
>> Although discussion was taken off this list - the changes here were
>> ACK'd and pushed today...
> 
> I think ACKs should be on-list.
> 

Understood - Eric's CC/query to qemu-devel on his initial response got
no response other than my followup... So a more direct question was
asked to (I assume) someone more involved in the blockdev layer. We got
a direct response from that, but nothing else in general from qemu. So,
after a couple more weeks of receiving no feedback - I queried whether
the change was OK. Eric's response indicated to go ahead with this patch
- all these are attached to this response if interested. While the 3
letters weren't used, I took the response as implicitly being OK with
the change.

>>
>> Essentially the following API's will round up the value as well:
>>
>>     virStorageBackendCreateQcowCreate()
>>     virStorageBackendLogicalCreateVol()
>>     virStorageBackendCreateQemuImgCmd()
>>
>> For libvirt created volumes, virStorageBackendCreateQemuImgCmd() or
>> virStorageBackendCreateQcowCreate() is called - both will take the
>> capacity value and VIR_DIV_UP using 1024. For the vol-resize path (e.g.
>> non running vm case), virStorageBackendFilesystemResizeQemuImg() will
>> use ROUND_UP on 512 byte value because it knows (and comments) that
>> qemu-img will fail to resize on non sector boundaries.
>>
>> Additionally, it was noted that using "K" and "KiB" would produce 1024
>> based results, it's libvirt's allowance of "KB" for sizes that results
>> in the nuance. Being strict KB shouldn't be used for storage, but rather
>> than penalize for not knowing the difference between KiB and KB the code
>> just assumes KiB should have been used.
>>
>> John
>>
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index 4bb4819..3e407d7 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -9421,6 +9421,7 @@ qemuDomainBlockResize(virDomainPtr dom,
>>>      virDomainObjPtr vm;
>>>      qemuDomainObjPrivatePtr priv;
>>>      int ret = -1, idx;
>>> +    unsigned long long size_up;
>>>      char *device = NULL;
>>>      virDomainDiskDefPtr disk = NULL;
>>>  
>>> @@ -9467,6 +9474,21 @@ qemuDomainBlockResize(virDomainPtr dom,
>>>      }
>>>      disk = vm->def->disks[idx];
>>>  
>>> +    /* qcow2 and qed must be sized appropriately, so be sure our value
>>> +     * is sized appropriately and will fit
>>> +     */
>>> +    if (size != size_up &&
>>> +        (disk->src.format == VIR_STORAGE_FILE_QCOW2 ||
>>> +         disk->src.format == VIR_STORAGE_FILE_QED)) {
>>> +        if (size_up > ULLONG_MAX) {
> 
> This is always false.
> 

An OVERLy cautious check - I cannot remember what I was thinking about a
month ago... I think this was the check can VIR_ROUND_UP provide an
incorrect value. I can send a follow-up patch to remove those lines if
that's desired.

>>> +            virReportError(VIR_ERR_OVERFLOW,
>>> +                           _("size must be less than %llu KiB"),
>>> +                           ULLONG_MAX / 1024);
>>> +            goto endjob;
>>> +        }
>>> +        size = size_up;
> 
> Just a nitpick: rounding it up unconditionally here would get rid of the
> temporary variable and have no effect on values specified without the BYTES flag.
> 

Only qcow2 and qed have this issue regarding needing to be on a 512 byte
boundary. Since this is a generic routine I was limiting the rounding to
the two types from the bz rather than taking a chance that some generic
round up would cause some other issue.  Or am I misinterpreting your
comment?

John
-------------- next part --------------
An embedded message was scrubbed...
From: Kevin Wolf <kwolf at redhat.com>
Subject: Re: [libvirt] [PATCH] qemu: Adjust size for qcow2/qed if not on	sector boundary
Date: Fri, 11 Apr 2014 16:24:09 +0200
Size: 6429
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140507/d151325d/attachment-0003.eml>
-------------- next part --------------
An embedded message was scrubbed...
From: John Ferlan <jferlan at redhat.com>
Subject: Re: [libvirt] [PATCH] qemu: Adjust size for qcow2/qed if not on sector	boundary
Date: Tue, 06 May 2014 08:39:31 -0400
Size: 7967
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140507/d151325d/attachment-0004.eml>
-------------- next part --------------
An embedded message was scrubbed...
From: Eric Blake <eblake at redhat.com>
Subject: Re: [libvirt] [PATCH] qemu: Adjust size for qcow2/qed if not on sector	boundary
Date: Tue, 06 May 2014 10:32:17 -0600
Size: 5703
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140507/d151325d/attachment-0005.eml>