[Qemu-devel] block/vpc: Fix vhd extra sectors issue

[Qemu-devel] block/vpc: Fix vhd extra sectors issue

[Lucian Petrut]

[-- Attachment #1: Type: text/plain, Size: 597 bytes --]


At the moment, qemu-img extends new image virtual sizes based
on the CHS algorithm provided by the VHD specs in order to
ensure that the disk geometry (and payload as seen by some
guests which use the CHS value) can fit in the requested disk.

This patch drops this behavior, as it breaks compatibility with
Azure, which requires the MB alignment to be preserved.

Signed-off-by: Lucian Petrut <lpetrut@cloudbasesolutions.com>
---
Proposed fix for https://bugs.launchpad.net/qemu/+bug/1490611

 block/vpc.c | 32 ++++++++------------------------
 1 file changed, 8 insertions(+), 24 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-block-vpc-Fix-vhd-extra-sectors-issue.patch --]
[-- Type: text/x-patch; name="0001-block-vpc-Fix-vhd-extra-sectors-issue.patch", Size: 1978 bytes --]

diff --git a/block/vpc.c b/block/vpc.c
index 299d373..77c0a28 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -762,7 +762,6 @@ static int vpc_create(const char *filename, QemuOpts *opts, Error **errp)
     uint8_t buf[1024];
     VHDFooter *footer = (VHDFooter *) buf;
     char *disk_type_param;
-    int i;
     uint16_t cyls = 0;
     uint8_t heads = 0;
     uint8_t secs_per_cyl = 0;
@@ -802,31 +801,16 @@ static int vpc_create(const char *filename, QemuOpts *opts, Error **errp)
         goto out;
     }
 
-    /*
-     * Calculate matching total_size and geometry. Increase the number of
-     * sectors requested until we get enough (or fail). This ensures that
-     * qemu-img convert doesn't truncate images, but rather rounds up.
-     *
-     * If the image size can't be represented by a spec conform CHS geometry,
-     * we set the geometry to 65535 x 16 x 255 (CxHxS) sectors and use
-     * the image size from the VHD footer to calculate total_sectors.
-     */
-    total_sectors = MIN(VHD_MAX_GEOMETRY, total_size / BDRV_SECTOR_SIZE);
-    for (i = 0; total_sectors > (int64_t)cyls * heads * secs_per_cyl; i++) {
-        calculate_geometry(total_sectors + i, &cyls, &heads, &secs_per_cyl);
-    }
-
-    if ((int64_t)cyls * heads * secs_per_cyl == VHD_MAX_GEOMETRY) {
-        total_sectors = total_size / BDRV_SECTOR_SIZE;
+    total_sectors = total_size / BDRV_SECTOR_SIZE;
+    if (total_sectors > VHD_MAX_SECTORS) {
         /* Allow a maximum disk size of approximately 2 TB */
-        if (total_sectors > VHD_MAX_SECTORS) {
-            ret = -EFBIG;
-            goto out;
-        }
-    } else {
-        total_sectors = (int64_t)cyls * heads * secs_per_cyl;
-        total_size = total_sectors * BDRV_SECTOR_SIZE;
+        ret = -EFBIG;
+        goto out;
     }
+    /*
+     * Calculate geometry.
+     */
+    calculate_geometry(total_sectors, &cyls, &heads, &secs_per_cyl);
 
     /* Prepare the Hard Disk Footer */
     memset(buf, 0, 1024);

Re: [Qemu-devel] block/vpc: Fix vhd extra sectors issue

[Kevin Wolf]

Am 13.11.2015 um 16:32 hat Lucian Petrut geschrieben:
> 
> At the moment, qemu-img extends new image virtual sizes based
> on the CHS algorithm provided by the VHD specs in order to
> ensure that the disk geometry (and payload as seen by some
> guests which use the CHS value) can fit in the requested disk.
> 
> This patch drops this behavior, as it breaks compatibility with
> Azure, which requires the MB alignment to be preserved.
> 
> Signed-off-by: Lucian Petrut <lpetrut@cloudbasesolutions.com>
> ---
> Proposed fix for https://bugs.launchpad.net/qemu/+bug/1490611

This may fix one scenario, but it's sure to break others which are
currently working. The problem has been discussed more than once and
it's essentially a problem with MS using their own file format
inconsistently.

I think we once came to the conclusion that looking at the creator
string might be a working heuristics. Apparently this was never
implemented - I don't remember whether that was because we noticed a
problem with it, or just because noone got to it.

Jeff and Peter, I seem to remember that you were involved the last time
we discussed this, so does one of you remember why we didn't implement
this heuristics in the end?

Kevin

> diff --git a/block/vpc.c b/block/vpc.c
> index 299d373..77c0a28 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -762,7 +762,6 @@ static int vpc_create(const char *filename, QemuOpts *opts, Error **errp)
>      uint8_t buf[1024];
>      VHDFooter *footer = (VHDFooter *) buf;
>      char *disk_type_param;
> -    int i;
>      uint16_t cyls = 0;
>      uint8_t heads = 0;
>      uint8_t secs_per_cyl = 0;
> @@ -802,31 +801,16 @@ static int vpc_create(const char *filename, QemuOpts *opts, Error **errp)
>          goto out;
>      }
>  
> -    /*
> -     * Calculate matching total_size and geometry. Increase the number of
> -     * sectors requested until we get enough (or fail). This ensures that
> -     * qemu-img convert doesn't truncate images, but rather rounds up.
> -     *
> -     * If the image size can't be represented by a spec conform CHS geometry,
> -     * we set the geometry to 65535 x 16 x 255 (CxHxS) sectors and use
> -     * the image size from the VHD footer to calculate total_sectors.
> -     */
> -    total_sectors = MIN(VHD_MAX_GEOMETRY, total_size / BDRV_SECTOR_SIZE);
> -    for (i = 0; total_sectors > (int64_t)cyls * heads * secs_per_cyl; i++) {
> -        calculate_geometry(total_sectors + i, &cyls, &heads, &secs_per_cyl);
> -    }
> -
> -    if ((int64_t)cyls * heads * secs_per_cyl == VHD_MAX_GEOMETRY) {
> -        total_sectors = total_size / BDRV_SECTOR_SIZE;
> +    total_sectors = total_size / BDRV_SECTOR_SIZE;
> +    if (total_sectors > VHD_MAX_SECTORS) {
>          /* Allow a maximum disk size of approximately 2 TB */
> -        if (total_sectors > VHD_MAX_SECTORS) {
> -            ret = -EFBIG;
> -            goto out;
> -        }
> -    } else {
> -        total_sectors = (int64_t)cyls * heads * secs_per_cyl;
> -        total_size = total_sectors * BDRV_SECTOR_SIZE;
> +        ret = -EFBIG;
> +        goto out;
>      }
> +    /*
> +     * Calculate geometry.
> +     */
> +    calculate_geometry(total_sectors, &cyls, &heads, &secs_per_cyl);
>  
>      /* Prepare the Hard Disk Footer */
>      memset(buf, 0, 1024);

Re: [Qemu-devel] block/vpc: Fix vhd extra sectors issue

[Jeff Cody]

On Mon, Nov 16, 2015 at 10:17:11AM +0100, Kevin Wolf wrote:
> Am 13.11.2015 um 16:32 hat Lucian Petrut geschrieben:
> > 
> > At the moment, qemu-img extends new image virtual sizes based
> > on the CHS algorithm provided by the VHD specs in order to
> > ensure that the disk geometry (and payload as seen by some
> > guests which use the CHS value) can fit in the requested disk.
> > 
> > This patch drops this behavior, as it breaks compatibility with
> > Azure, which requires the MB alignment to be preserved.
> > 
> > Signed-off-by: Lucian Petrut <lpetrut@cloudbasesolutions.com>
> > ---
> > Proposed fix for https://bugs.launchpad.net/qemu/+bug/1490611
> 
> This may fix one scenario, but it's sure to break others which are
> currently working. The problem has been discussed more than once and
> it's essentially a problem with MS using their own file format
> inconsistently.
> 
> I think we once came to the conclusion that looking at the creator
> string might be a working heuristics. Apparently this was never
> implemented - I don't remember whether that was because we noticed a
> problem with it, or just because noone got to it.
> 
> Jeff and Peter, I seem to remember that you were involved the last time
> we discussed this, so does one of you remember why we didn't implement
> this heuristics in the end?
> 

I believe we can look at the creator field.  The VHD files created by
Virtual PC and Hyper-V differed, but we could theoretically
differentiate between by the creator field.

I'm not sure if there was anything that actually blocked the
implementation.  It may have just been because we weren't sure if this
was the case across all versions of VPC and Hyper-V, although I
suspect it is (as an aside - is VirtualPC essentially dead, or is it
maintained / supported by MS?).

-Jeff

Re: [Qemu-devel] block/vpc: Fix vhd extra sectors issue

[Peter Lieven]

Am 16.11.2015 um 13:22 schrieb Jeff Cody:
> On Mon, Nov 16, 2015 at 10:17:11AM +0100, Kevin Wolf wrote:
>> Am 13.11.2015 um 16:32 hat Lucian Petrut geschrieben:
>>> At the moment, qemu-img extends new image virtual sizes based
>>> on the CHS algorithm provided by the VHD specs in order to
>>> ensure that the disk geometry (and payload as seen by some
>>> guests which use the CHS value) can fit in the requested disk.
>>>
>>> This patch drops this behavior, as it breaks compatibility with
>>> Azure, which requires the MB alignment to be preserved.
>>>
>>> Signed-off-by: Lucian Petrut <lpetrut@cloudbasesolutions.com>
>>> ---
>>> Proposed fix for https://bugs.launchpad.net/qemu/+bug/1490611
>> This may fix one scenario, but it's sure to break others which are
>> currently working. The problem has been discussed more than once and
>> it's essentially a problem with MS using their own file format
>> inconsistently.
>>
>> I think we once came to the conclusion that looking at the creator
>> string might be a working heuristics. Apparently this was never
>> implemented - I don't remember whether that was because we noticed a
>> problem with it, or just because noone got to it.
>>
>> Jeff and Peter, I seem to remember that you were involved the last time
>> we discussed this, so does one of you remember why we didn't implement
>> this heuristics in the end?
>>
> I believe we can look at the creator field.  The VHD files created by
> Virtual PC and Hyper-V differed, but we could theoretically
> differentiate between by the creator field.
>
> I'm not sure if there was anything that actually blocked the
> implementation.  It may have just been because we weren't sure if this
> was the case across all versions of VPC and Hyper-V, although I
> suspect it is (as an aside - is VirtualPC essentially dead, or is it
> maintained / supported by MS?).

If I remember correctly we were looking at the creator field for disk2vhd images and
used the size from the footer in this case. Later we changed that to use the size from
the footer if CHS is 65535x255x255. disk2vhd always sets this geometry regardless of
the size. The issue is whatever we do if we interpret the disk size larger than expected
by some other application. If I remember correctly VirtualBox uses the size from the footer
anytime we do not mount the container as an IDE device.

Peter

Re: [Qemu-devel] block/vpc: Fix vhd extra sectors issue

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2219 bytes --]

On 13.11.2015 16:32, Lucian Petrut wrote:
> 
> At the moment, qemu-img extends new image virtual sizes based
> on the CHS algorithm provided by the VHD specs in order to
> ensure that the disk geometry (and payload as seen by some
> guests which use the CHS value) can fit in the requested disk.
> 
> This patch drops this behavior, as it breaks compatibility with
> Azure, which requires the MB alignment to be preserved.
> 
> Signed-off-by: Lucian Petrut <lpetrut@cloudbasesolutions.com>
> ---
> Proposed fix for https://bugs.launchpad.net/qemu/+bug/1490611
> 
>  block/vpc.c | 32 ++++++++------------------------
>  1 file changed, 8 insertions(+), 24 deletions(-)
> 

CC-ing Peter.

Looks right, considering page 7 of the spec:

(http://download.microsoft.com/download/f/f/e/ffef50a5-07dd-4cf8-aaa3-442c0673a029/Virtual%20Hard%20Disk%20Format%20Spec_10_18_06.doc)

"When the user creates a hard disk of a certain size, the size of the
hard disk image in the virtual machine is smaller than that created by
the user. This is because CHS value calculated from the hard disk size
is rounded down."

However, this patch looks incomplete, note the comment you are removing:
"This ensures that qemu-img convert doesn't truncate images". This is
because vpc_open() prefers the CHS size over the current_size value, and
as far as I remember there was indeed a good reason for that (some
application had invalid values for current_size, or something like that).

So at the very least, we will have to change vpc_open() to always use
the value from footer->current_size and ignore
footer->{cyls,heads,secs_per_cyl}, otherwise qemu will always consider
these images smaller than they actually are (which is bad, at least when
converting images).

But I'd like to know from Peter whether he remembers the reason why
vpc_open() tries to ignore footer->current_size in the first place...

Max


PS: If possible, the "From: " header in an emailed patch should match
the Signed-off-by line. I think. It doesn't state that in
http://wiki.qemu.org/Contribute/SubmitAPatch, but the commit looks
strange otherwise. So I guess if it isn't reasonably possible, it's not
too bad. :-)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] block/vpc: Fix vhd extra sectors issue

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2433 bytes --]

On 16.11.2015 21:58, Max Reitz wrote:
> On 13.11.2015 16:32, Lucian Petrut wrote:
>>
>> At the moment, qemu-img extends new image virtual sizes based
>> on the CHS algorithm provided by the VHD specs in order to
>> ensure that the disk geometry (and payload as seen by some
>> guests which use the CHS value) can fit in the requested disk.
>>
>> This patch drops this behavior, as it breaks compatibility with
>> Azure, which requires the MB alignment to be preserved.
>>
>> Signed-off-by: Lucian Petrut <lpetrut@cloudbasesolutions.com>
>> ---
>> Proposed fix for https://bugs.launchpad.net/qemu/+bug/1490611
>>
>>  block/vpc.c | 32 ++++++++------------------------
>>  1 file changed, 8 insertions(+), 24 deletions(-)
>>
> 
> CC-ing Peter.
> 
> Looks right, considering page 7 of the spec:
> 
> (http://download.microsoft.com/download/f/f/e/ffef50a5-07dd-4cf8-aaa3-442c0673a029/Virtual%20Hard%20Disk%20Format%20Spec_10_18_06.doc)
> 
> "When the user creates a hard disk of a certain size, the size of the
> hard disk image in the virtual machine is smaller than that created by
> the user. This is because CHS value calculated from the hard disk size
> is rounded down."
> 
> However, this patch looks incomplete, note the comment you are removing:
> "This ensures that qemu-img convert doesn't truncate images". This is
> because vpc_open() prefers the CHS size over the current_size value, and
> as far as I remember there was indeed a good reason for that (some
> application had invalid values for current_size, or something like that).
> 
> So at the very least, we will have to change vpc_open() to always use
> the value from footer->current_size and ignore
> footer->{cyls,heads,secs_per_cyl}, otherwise qemu will always consider
> these images smaller than they actually are (which is bad, at least when
> converting images).
> 
> But I'd like to know from Peter whether he remembers the reason why
> vpc_open() tries to ignore footer->current_size in the first place...

OK, so it was just qemu-devel having hiccups again. Now the mails are
rolling in...

> Max
> 
> 
> PS: If possible, the "From: " header in an emailed patch should match
> the Signed-off-by line. I think. It doesn't state that in
> http://wiki.qemu.org/Contribute/SubmitAPatch, but the commit looks
> strange otherwise. So I guess if it isn't reasonably possible, it's not
> too bad. :-)
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] block/vpc: Fix vhd extra sectors issue

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 543 bytes --]

On 11/16/2015 01:58 PM, Max Reitz wrote:

> PS: If possible, the "From: " header in an emailed patch should match
> the Signed-off-by line. I think. It doesn't state that in
> http://wiki.qemu.org/Contribute/SubmitAPatch, but the commit looks
> strange otherwise. So I guess if it isn't reasonably possible, it's not
> too bad. :-)

Thanks for the suggestion; the wiki states it now, in the section
talking about S-o-b :)

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]