Patchwork [5/7] block/vpc: Fix size calculation

login
register
mail settings
Submitter Stefan Weil
Date Feb. 1, 2013, 9:51 p.m.
Message ID <1359755494-28012-6-git-send-email-sw@weilnetz.de>
Download mbox | patch
Permalink /patch/217598/
State Under Review
Headers show

Comments

Stefan Weil - Feb. 1, 2013, 9:51 p.m.
The size calculated from the CHS values is not the real image (disk) size,
but usually a smaller value (this is caused by rounding effects).

Only older operating systems use CHS. Such guests won't be able to use
the whole disk.

This patch fixes https://bugs.launchpad.net/qemu/+bug/1105670/.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 block/vpc.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
Kevin Wolf - Feb. 5, 2013, 10:54 a.m.
Am 01.02.2013 22:51, schrieb Stefan Weil:
> The size calculated from the CHS values is not the real image (disk) size,
> but usually a smaller value (this is caused by rounding effects).
> 
> Only older operating systems use CHS. Such guests won't be able to use
> the whole disk.
> 
> This patch fixes https://bugs.launchpad.net/qemu/+bug/1105670/.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  block/vpc.c |    9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/block/vpc.c b/block/vpc.c
> index fff103b..799b1c9 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -198,11 +198,10 @@ static int vpc_open(BlockDriverState *bs, int flags)
>      /* Write 'checksum' back to footer, or else will leave it with zero. */
>      footer->checksum = be32_to_cpu(checksum);
>  
> -    // The visible size of a image in Virtual PC depends on the geometry
> -    // rather than on the size stored in the footer (the size in the footer
> -    // is too large usually)
> -    bs->total_sectors = (int64_t)
> -        be16_to_cpu(footer->cyls) * footer->heads * footer->secs_per_cyl;
> +    /* The visible size of an image in Virtual PC may depend on the geometry
> +     * rather than on the size stored in the footer (the size in the footer
> +     * is usually larger). Nevertheless we must use the real size here. */
> +    bs->total_sectors = be64_to_cpu(footer->size) / 512;
>  
>      /* Allow a maximum disk size of approximately 2 TB */
>      if (bs->total_sectors >= 65535LL * 255 * 255) {

Now VMs created with qemu can't be run in Virtual PC any more because
they use more space than is really visible on VPC.

The bug report doesn't say anything about practical consequences of the
unexpected behaviour, so I'd tend to claim that just the expectation was
wrong. Having to deal with two differing sizes is ugly, but we had the
discussion before and using the geometry was considered the least bad
way, because it gives you the same disk size within the VM as on Virtual PC.

If you want to change it, you need to provide a real use case that breaks.

Kevin
Stefan Weil - Feb. 5, 2013, 11:10 a.m.
Am 05.02.2013 11:54, schrieb Kevin Wolf:
> Am 01.02.2013 22:51, schrieb Stefan Weil:
>> The size calculated from the CHS values is not the real image (disk) size,
>> but usually a smaller value (this is caused by rounding effects).
>>
>> Only older operating systems use CHS. Such guests won't be able to use
>> the whole disk.
>>
>> This patch fixes https://bugs.launchpad.net/qemu/+bug/1105670/.
>>
>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>> ---
>>  block/vpc.c |    9 ++++-----
>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/block/vpc.c b/block/vpc.c
>> index fff103b..799b1c9 100644
>> --- a/block/vpc.c
>> +++ b/block/vpc.c
>> @@ -198,11 +198,10 @@ static int vpc_open(BlockDriverState *bs, int flags)
>>      /* Write 'checksum' back to footer, or else will leave it with zero. */
>>      footer->checksum = be32_to_cpu(checksum);
>>  
>> -    // The visible size of a image in Virtual PC depends on the geometry
>> -    // rather than on the size stored in the footer (the size in the footer
>> -    // is too large usually)
>> -    bs->total_sectors = (int64_t)
>> -        be16_to_cpu(footer->cyls) * footer->heads * footer->secs_per_cyl;
>> +    /* The visible size of an image in Virtual PC may depend on the geometry
>> +     * rather than on the size stored in the footer (the size in the footer
>> +     * is usually larger). Nevertheless we must use the real size here. */
>> +    bs->total_sectors = be64_to_cpu(footer->size) / 512;
>>  
>>      /* Allow a maximum disk size of approximately 2 TB */
>>      if (bs->total_sectors >= 65535LL * 255 * 255) {
> Now VMs created with qemu can't be run in Virtual PC any more because
> they use more space than is really visible on VPC.

Now VMs created with QEMU show the size which is also
reported by MS tools. I don't think this is a problem.


> The bug report doesn't say anything about practical consequences of the
> unexpected behaviour, so I'd tend to claim that just the expectation was
> wrong. Having to deal with two differing sizes is ugly, but we had the
> discussion before and using the geometry was considered the least bad
> way, because it gives you the same disk size within the VM as on Virtual PC.
>
> If you want to change it, you need to provide a real use case that breaks.
>
> Kevin

Test case:

* Create a VHD disk image (either with QEMU or MS tools).
* Run a modern guest OS (which does not use CHS) on this image
  using Virtual PC or VirtualBox and write into the last disk blocks.
* Copy the image to another format using qemu-img.

=> The last blocks are missing now.

Stefan
Kevin Wolf - Feb. 5, 2013, 11:35 a.m.
Am 05.02.2013 12:10, schrieb Stefan Weil:
> Am 05.02.2013 11:54, schrieb Kevin Wolf:
>> Am 01.02.2013 22:51, schrieb Stefan Weil:
>>> The size calculated from the CHS values is not the real image (disk) size,
>>> but usually a smaller value (this is caused by rounding effects).
>>>
>>> Only older operating systems use CHS. Such guests won't be able to use
>>> the whole disk.
>>>
>>> This patch fixes https://bugs.launchpad.net/qemu/+bug/1105670/.
>>>
>>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>>> ---
>>>  block/vpc.c |    9 ++++-----
>>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/block/vpc.c b/block/vpc.c
>>> index fff103b..799b1c9 100644
>>> --- a/block/vpc.c
>>> +++ b/block/vpc.c
>>> @@ -198,11 +198,10 @@ static int vpc_open(BlockDriverState *bs, int flags)
>>>      /* Write 'checksum' back to footer, or else will leave it with zero. */
>>>      footer->checksum = be32_to_cpu(checksum);
>>>  
>>> -    // The visible size of a image in Virtual PC depends on the geometry
>>> -    // rather than on the size stored in the footer (the size in the footer
>>> -    // is too large usually)
>>> -    bs->total_sectors = (int64_t)
>>> -        be16_to_cpu(footer->cyls) * footer->heads * footer->secs_per_cyl;
>>> +    /* The visible size of an image in Virtual PC may depend on the geometry
>>> +     * rather than on the size stored in the footer (the size in the footer
>>> +     * is usually larger). Nevertheless we must use the real size here. */
>>> +    bs->total_sectors = be64_to_cpu(footer->size) / 512;
>>>  
>>>      /* Allow a maximum disk size of approximately 2 TB */
>>>      if (bs->total_sectors >= 65535LL * 255 * 255) {
>> Now VMs created with qemu can't be run in Virtual PC any more because
>> they use more space than is really visible on VPC.
> 
> Now VMs created with QEMU show the size which is also
> reported by MS tools. I don't think this is a problem.
> 
> 
>> The bug report doesn't say anything about practical consequences of the
>> unexpected behaviour, so I'd tend to claim that just the expectation was
>> wrong. Having to deal with two differing sizes is ugly, but we had the
>> discussion before and using the geometry was considered the least bad
>> way, because it gives you the same disk size within the VM as on Virtual PC.
>>
>> If you want to change it, you need to provide a real use case that breaks.
>>
>> Kevin
> 
> Test case:
> 
> * Create a VHD disk image (either with QEMU or MS tools).
> * Run a modern guest OS (which does not use CHS) on this image
>   using Virtual PC or VirtualBox and write into the last disk blocks.
> * Copy the image to another format using qemu-img.
> 
> => The last blocks are missing now.

When I wrote that code it wasn't like this. I doubt that Linux was
relying on the geometry until recently, so is this a change in the
behaviour of newer Virtual PC versions? If yes, that would be very nasty
to deal with.

I'm very sure that back then I needed to use the geometry so that images
could be exchanged between qemu and VPC. If I didn't, they would either
get truncated or appear to be larger on one of both.

Kevin
Jeff Cody - Feb. 5, 2013, 1:02 p.m.
On Tue, Feb 05, 2013 at 12:35:49PM +0100, Kevin Wolf wrote:
> Am 05.02.2013 12:10, schrieb Stefan Weil:
> > Am 05.02.2013 11:54, schrieb Kevin Wolf:
> >> Am 01.02.2013 22:51, schrieb Stefan Weil:
> >>> The size calculated from the CHS values is not the real image (disk) size,
> >>> but usually a smaller value (this is caused by rounding effects).
> >>>
> >>> Only older operating systems use CHS. Such guests won't be able to use
> >>> the whole disk.
> >>>
> >>> This patch fixes https://bugs.launchpad.net/qemu/+bug/1105670/.
> >>>
> >>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> >>> ---
> >>>  block/vpc.c |    9 ++++-----
> >>>  1 file changed, 4 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/block/vpc.c b/block/vpc.c
> >>> index fff103b..799b1c9 100644
> >>> --- a/block/vpc.c
> >>> +++ b/block/vpc.c
> >>> @@ -198,11 +198,10 @@ static int vpc_open(BlockDriverState *bs, int flags)
> >>>      /* Write 'checksum' back to footer, or else will leave it with zero. */
> >>>      footer->checksum = be32_to_cpu(checksum);
> >>>  
> >>> -    // The visible size of a image in Virtual PC depends on the geometry
> >>> -    // rather than on the size stored in the footer (the size in the footer
> >>> -    // is too large usually)
> >>> -    bs->total_sectors = (int64_t)
> >>> -        be16_to_cpu(footer->cyls) * footer->heads * footer->secs_per_cyl;
> >>> +    /* The visible size of an image in Virtual PC may depend on the geometry
> >>> +     * rather than on the size stored in the footer (the size in the footer
> >>> +     * is usually larger). Nevertheless we must use the real size here. */
> >>> +    bs->total_sectors = be64_to_cpu(footer->size) / 512;
> >>>  
> >>>      /* Allow a maximum disk size of approximately 2 TB */
> >>>      if (bs->total_sectors >= 65535LL * 255 * 255) {
> >> Now VMs created with qemu can't be run in Virtual PC any more because
> >> they use more space than is really visible on VPC.
> > 
> > Now VMs created with QEMU show the size which is also
> > reported by MS tools. I don't think this is a problem.
> > 
> > 
> >> The bug report doesn't say anything about practical consequences of the
> >> unexpected behaviour, so I'd tend to claim that just the expectation was
> >> wrong. Having to deal with two differing sizes is ugly, but we had the
> >> discussion before and using the geometry was considered the least bad
> >> way, because it gives you the same disk size within the VM as on Virtual PC.
> >>
> >> If you want to change it, you need to provide a real use case that breaks.
> >>
> >> Kevin
> > 
> > Test case:
> > 
> > * Create a VHD disk image (either with QEMU or MS tools).
> > * Run a modern guest OS (which does not use CHS) on this image
> >   using Virtual PC or VirtualBox and write into the last disk blocks.
> > * Copy the image to another format using qemu-img.
> > 
> > => The last blocks are missing now.
> 
> When I wrote that code it wasn't like this. I doubt that Linux was
> relying on the geometry until recently, so is this a change in the
> behaviour of newer Virtual PC versions? If yes, that would be very nasty
> to deal with.
> 
> I'm very sure that back then I needed to use the geometry so that images
> could be exchanged between qemu and VPC. If I didn't, they would either
> get truncated or appear to be larger on one of both.
>

Hi Kevin,

I can verify the issue he is seeing (see my comments on Bug 1105670 
(https://bugs.launchpad.net/qemu/+bug/1105670/comments/3), at least
using Hyper-V.  I haven't tried it against VPC from Win7.

Jeff
Stefan Weil - Feb. 7, 2013, 6:48 p.m.
Am 05.02.2013 14:02, schrieb Jeff Cody:
> Hi Kevin,
>
> I can verify the issue he is seeing (see my comments on Bug 1105670
> (https://bugs.launchpad.net/qemu/+bug/1105670/comments/3), at least
> using Hyper-V.  I haven't tried it against VPC from Win7.
>
> Jeff


Hi Kevin,

a rebased version of this patch was just sent to qemu-devel.
It is needed to fix the bug mentioned above and should be
applied to QEMU 1.4 and also earlier branches.

Here is the test scenario which I have run:

* Create a (dynamic) 3 GiB VHD image with MS tools (command line
   or explorer with Windows 7 or newer).

* Run a VM (Linux guest) which uses the new VHD image as 2nd disk.

* Check the disk size from the guest's point of view.
   Either run dmesg (look for /dev/sdb), a partitioning tool
   like fdisk or cfdisk, or simply dump the whole disk using
   od or dd,

I used VirtualBox and latest QEMU for the VM.

The VHD image can also be created using VirtualBox.
This gives identical results.

Here are my results:

* The expected size is 3 GiB = 3 * 1024 * 1024 * 1024 Byte = 3221225472 
Byte.

* Linux on VirtualBox reports 3221225472 Byte (correct)
   (tested with dmesg, fdisk, cfdisk and od - all give same value).

* Linux on QEMU reports 3220955136 Byte (wrong)
   (tested with dmesg, fdisk, cfdisk and od - all give same value).

* qemu-img (unpatched) reports 3220955136 Byte (wrong).

With the patch for block/vpc.c applied, Linux on QEMU and qemu-img
report the correct size.

To summarize,

* Guests using unpatched QEMU only see part of the VHD image.
   This is bad for images written with other emulators like VirtualBox,
   because the last blocks are invisible for QEMU.
   It works with images which were created by unpatched qemu-img and which
   were used only with unpatched QEMU.

* Unpatched qemu-img only converts part of the VHD image.
   This also can result in a corrupt file system because of missing
   last blocks.

* Unpatched qemu-img creates images which are larger than required
   because it increased the size until the unpatched QEMU got enough
   blocks. This won't do any harm beside using too much resources
   as long as those images are only used with unpatched QEMU.
   Guests running on other emulators or on the patched QEMU will see
   a larger disk. If they use the additional blocks, that image
   must no longer used with unpatched QEMU.

* As long as users only switch from unpatched to patched versions,
   they won't have a problem. Switching from patched to unpatched
   can be fatal, and the same already applies to switching from
   other VHD tools / emulators to unpatched QEMU.

=> All branches with VHD support (vpc.c) should be fixed.

=> Delaying the patch increases the problem, as VHD is quite common.

Regards

Stefan W.

Patch

diff --git a/block/vpc.c b/block/vpc.c
index fff103b..799b1c9 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -198,11 +198,10 @@  static int vpc_open(BlockDriverState *bs, int flags)
     /* Write 'checksum' back to footer, or else will leave it with zero. */
     footer->checksum = be32_to_cpu(checksum);
 
-    // The visible size of a image in Virtual PC depends on the geometry
-    // rather than on the size stored in the footer (the size in the footer
-    // is too large usually)
-    bs->total_sectors = (int64_t)
-        be16_to_cpu(footer->cyls) * footer->heads * footer->secs_per_cyl;
+    /* The visible size of an image in Virtual PC may depend on the geometry
+     * rather than on the size stored in the footer (the size in the footer
+     * is usually larger). Nevertheless we must use the real size here. */
+    bs->total_sectors = be64_to_cpu(footer->size) / 512;
 
     /* Allow a maximum disk size of approximately 2 TB */
     if (bs->total_sectors >= 65535LL * 255 * 255) {