Patchwork [for,1.4] block/vpc: Fix size calculation

login
register
mail settings
Submitter Stefan Weil
Date Feb. 7, 2013, 7:26 p.m.
Message ID <1360265212-22037-1-git-send-email-sw@weilnetz.de>
Download mbox | patch
Permalink /patch/218978/
State New
Headers show

Comments

Stefan Weil - Feb. 7, 2013, 7:26 p.m.
From: Stefan Weil <stefan@kiwi.(none)>

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. All modern operating systems use the real size.

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

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---

This is a rebased extract from my patch series for block/vpc.c.
It's the minimum needed to fix the open bug for QEMU 1.4.

The rest of the series can be discussed and applied after 1.4.

Regards

Stefan W.

PS. Please excuse a previous personal mail which I had sent
with a wrong signature and without addressing qemu-devel.

 block/vpc.c |   14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)
Stefan Hajnoczi - Feb. 8, 2013, 8:18 a.m.
On Thu, Feb 07, 2013 at 08:26:52PM +0100, Stefan Weil wrote:
> From: Stefan Weil <stefan@kiwi.(none)>

Should be "From: Stefan Weil <sw@weilnetz.de>"?

> 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. All modern operating systems use the real size.
> 
> This patch fixes https://bugs.launchpad.net/qemu/+bug/1105670/.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
> 
> This is a rebased extract from my patch series for block/vpc.c.
> It's the minimum needed to fix the open bug for QEMU 1.4.
> 
> The rest of the series can be discussed and applied after 1.4.
> 
> Regards
> 
> Stefan W.
> 
> PS. Please excuse a previous personal mail which I had sent
> with a wrong signature and without addressing qemu-devel.
> 
>  block/vpc.c |   14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Kevin Wolf - Feb. 8, 2013, 8:38 a.m.
Am 07.02.2013 20:26, schrieb Stefan Weil:
> From: Stefan Weil <stefan@kiwi.(none)>
> 
> 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. All modern operating systems use the real size.
> 
> This patch fixes https://bugs.launchpad.net/qemu/+bug/1105670/.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
> 
> This is a rebased extract from my patch series for block/vpc.c.
> It's the minimum needed to fix the open bug for QEMU 1.4.
> 
> The rest of the series can be discussed and applied after 1.4.
> 
> Regards
> 
> Stefan W.
> 
> PS. Please excuse a previous personal mail which I had sent
> with a wrong signature and without addressing qemu-devel.
> 
>  block/vpc.c |   14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/block/vpc.c b/block/vpc.c
> index 82229ef..b4ff564 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -34,6 +34,8 @@
>  
>  #define HEADER_SIZE 512
>  
> +#define VHD_SECTOR_SIZE 512
> +
>  //#define CACHE
>  
>  enum vhd_type {
> @@ -204,11 +206,13 @@ 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 a image in Virtual PC depends on the guest:
> +     * QEMU and other emulators report the real size (here in sectors).
> +     * All modern operating systems use this real size.
> +     * Very old operating systems use CHS values to calculate the total size.
> +     * This calculated size is usually smaller than the real size.
> +     */
> +    bs->total_sectors = be64_to_cpu(footer->size) / VHD_SECTOR_SIZE;

It's unfortunate that I don't have my old Virtual PC installation around
any more so I could prove that you're wrong for at least some versions.
Or does a Linux of 2009 already count as "very old"?

If we want to commit this - and I still feel uncomfortable about it -
then maybe it's best to remove the comment altogether instead of making
such claims. The new code is the intuitively expected one anyway.

Kevin
Jeff Cody - Feb. 8, 2013, 12:14 p.m.
On Fri, Feb 08, 2013 at 09:38:47AM +0100, Kevin Wolf wrote:
> Am 07.02.2013 20:26, schrieb Stefan Weil:
> > From: Stefan Weil <stefan@kiwi.(none)>
> > 
> > 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. All modern operating systems use the real size.
> > 
> > This patch fixes https://bugs.launchpad.net/qemu/+bug/1105670/.
> > 
> > Signed-off-by: Stefan Weil <sw@weilnetz.de>
> > ---
> > 
> > This is a rebased extract from my patch series for block/vpc.c.
> > It's the minimum needed to fix the open bug for QEMU 1.4.
> > 
> > The rest of the series can be discussed and applied after 1.4.
> > 
> > Regards
> > 
> > Stefan W.
> > 
> > PS. Please excuse a previous personal mail which I had sent
> > with a wrong signature and without addressing qemu-devel.
> > 
> >  block/vpc.c |   14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/block/vpc.c b/block/vpc.c
> > index 82229ef..b4ff564 100644
> > --- a/block/vpc.c
> > +++ b/block/vpc.c
> > @@ -34,6 +34,8 @@
> >  
> >  #define HEADER_SIZE 512
> >  
> > +#define VHD_SECTOR_SIZE 512
> > +
> >  //#define CACHE
> >  
> >  enum vhd_type {
> > @@ -204,11 +206,13 @@ 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 a image in Virtual PC depends on the guest:
> > +     * QEMU and other emulators report the real size (here in sectors).
> > +     * All modern operating systems use this real size.
> > +     * Very old operating systems use CHS values to calculate the total size.
> > +     * This calculated size is usually smaller than the real size.
> > +     */
> > +    bs->total_sectors = be64_to_cpu(footer->size) / VHD_SECTOR_SIZE;
> 
> It's unfortunate that I don't have my old Virtual PC installation around
> any more so I could prove that you're wrong for at least some versions.
> Or does a Linux of 2009 already count as "very old"?
> 
> If we want to commit this - and I still feel uncomfortable about it -
> then maybe it's best to remove the comment altogether instead of making
> such claims. The new code is the intuitively expected one anyway.
> 
> Kevin

Kevin,

I can test this on Virtual PC on a Win 7 install, as well as
Hyper-V (I've already tested the equivalent of this patch on Hyper-V
on Windows Server 2012).  I'll do some testing today with this - if
you have anything in particular you want me to look at, just let me
know.

Stefan, did you test if this behaves the same for all disk sizes, or
only for larger disk sizes of 127G and up?

Thanks,
Jeff
Stefan Weil - Feb. 8, 2013, 5:08 p.m.
Am 08.02.2013 09:18, schrieb Stefan Hajnoczi:
> On Thu, Feb 07, 2013 at 08:26:52PM +0100, Stefan Weil wrote:
>> From: Stefan Weil <stefan@kiwi.(none)>
> Should be "From: Stefan Weil <sw@weilnetz.de>"?

Yes, of course.

Maybe whoever applies the patch can fix this, please.

Thanks,

Stefan W.
Stefan Weil - Feb. 8, 2013, 5:43 p.m.
Am 08.02.2013 13:14, schrieb Jeff Cody:
> On Fri, Feb 08, 2013 at 09:38:47AM +0100, Kevin Wolf wrote:
>> Am 07.02.2013 20:26, schrieb Stefan Weil:
>>> From: Stefan Weil <stefan@kiwi.(none)>
>>>
>>> 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. All modern operating systems use the real size.
>>>
>>> This patch fixes https://bugs.launchpad.net/qemu/+bug/1105670/.
>>>
>>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>>> ---
>>>
>>> This is a rebased extract from my patch series for block/vpc.c.
>>> It's the minimum needed to fix the open bug for QEMU 1.4.
>>>
>>> The rest of the series can be discussed and applied after 1.4.
>>>
>>> Regards
>>>
>>> Stefan W.
>>>
>>> PS. Please excuse a previous personal mail which I had sent
>>> with a wrong signature and without addressing qemu-devel.
>>>
>>>  block/vpc.c |   14 +++++++++-----
>>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/block/vpc.c b/block/vpc.c
>>> index 82229ef..b4ff564 100644
>>> --- a/block/vpc.c
>>> +++ b/block/vpc.c
>>> @@ -34,6 +34,8 @@
>>>  
>>>  #define HEADER_SIZE 512
>>>  
>>> +#define VHD_SECTOR_SIZE 512
>>> +
>>>  //#define CACHE
>>>  
>>>  enum vhd_type {
>>> @@ -204,11 +206,13 @@ 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 a image in Virtual PC depends on the guest:
>>> +     * QEMU and other emulators report the real size (here in sectors).
>>> +     * All modern operating systems use this real size.
>>> +     * Very old operating systems use CHS values to calculate the total size.
>>> +     * This calculated size is usually smaller than the real size.
>>> +     */
>>> +    bs->total_sectors = be64_to_cpu(footer->size) / VHD_SECTOR_SIZE;
>> It's unfortunate that I don't have my old Virtual PC installation around
>> any more so I could prove that you're wrong for at least some versions.
>> Or does a Linux of 2009 already count as "very old"?

Linux is a modern OS per definition :-)

I made an additional test with Knoppix (from December 2006),
and it worked as expected.


>>
>> If we want to commit this - and I still feel uncomfortable about it -
>> then maybe it's best to remove the comment altogether instead of making
>> such claims. The new code is the intuitively expected one anyway.
>>
>> Kevin
> Kevin,
>
> I can test this on Virtual PC on a Win 7 install, as well as
> Hyper-V (I've already tested the equivalent of this patch on Hyper-V
> on Windows Server 2012).  I'll do some testing today with this - if
> you have anything in particular you want me to look at, just let me
> know.
>
> Stefan, did you test if this behaves the same for all disk sizes, or
> only for larger disk sizes of 127G and up?

This behaves the same in a test with a small (16 MiB) image.

Regards,

Stefan W.
Stefan Hajnoczi - Feb. 11, 2013, 8:42 a.m.
On Fri, Feb 08, 2013 at 07:14:33AM -0500, Jeff Cody wrote:
> On Fri, Feb 08, 2013 at 09:38:47AM +0100, Kevin Wolf wrote:
> > Am 07.02.2013 20:26, schrieb Stefan Weil:
> > > From: Stefan Weil <stefan@kiwi.(none)>
> > > 
> > > 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. All modern operating systems use the real size.
> > > 
> > > This patch fixes https://bugs.launchpad.net/qemu/+bug/1105670/.
> > > 
> > > Signed-off-by: Stefan Weil <sw@weilnetz.de>
> > > ---
> > > 
> > > This is a rebased extract from my patch series for block/vpc.c.
> > > It's the minimum needed to fix the open bug for QEMU 1.4.
> > > 
> > > The rest of the series can be discussed and applied after 1.4.
> > > 
> > > Regards
> > > 
> > > Stefan W.
> > > 
> > > PS. Please excuse a previous personal mail which I had sent
> > > with a wrong signature and without addressing qemu-devel.
> > > 
> > >  block/vpc.c |   14 +++++++++-----
> > >  1 file changed, 9 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/block/vpc.c b/block/vpc.c
> > > index 82229ef..b4ff564 100644
> > > --- a/block/vpc.c
> > > +++ b/block/vpc.c
> > > @@ -34,6 +34,8 @@
> > >  
> > >  #define HEADER_SIZE 512
> > >  
> > > +#define VHD_SECTOR_SIZE 512
> > > +
> > >  //#define CACHE
> > >  
> > >  enum vhd_type {
> > > @@ -204,11 +206,13 @@ 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 a image in Virtual PC depends on the guest:
> > > +     * QEMU and other emulators report the real size (here in sectors).
> > > +     * All modern operating systems use this real size.
> > > +     * Very old operating systems use CHS values to calculate the total size.
> > > +     * This calculated size is usually smaller than the real size.
> > > +     */
> > > +    bs->total_sectors = be64_to_cpu(footer->size) / VHD_SECTOR_SIZE;
> > 
> > It's unfortunate that I don't have my old Virtual PC installation around
> > any more so I could prove that you're wrong for at least some versions.
> > Or does a Linux of 2009 already count as "very old"?
> > 
> > If we want to commit this - and I still feel uncomfortable about it -
> > then maybe it's best to remove the comment altogether instead of making
> > such claims. The new code is the intuitively expected one anyway.
> > 
> > Kevin
> 
> Kevin,
> 
> I can test this on Virtual PC on a Win 7 install, as well as
> Hyper-V (I've already tested the equivalent of this patch on Hyper-V
> on Windows Server 2012).  I'll do some testing today with this - if
> you have anything in particular you want me to look at, just let me
> know.

Hi Jeff,
Thanks for testing this on your Virtual PC and Hyper-V setup.  I'll
merge this patch once you have confirmed your results.

Stefan
Kevin Wolf - Feb. 11, 2013, 8:54 a.m.
Am 08.02.2013 18:43, schrieb Stefan Weil:
> Am 08.02.2013 13:14, schrieb Jeff Cody:
>> On Fri, Feb 08, 2013 at 09:38:47AM +0100, Kevin Wolf wrote:
>>> Am 07.02.2013 20:26, schrieb Stefan Weil:
>>>> From: Stefan Weil <stefan@kiwi.(none)>
>>>>
>>>> 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. All modern operating systems use the real size.
>>>>
>>>> This patch fixes https://bugs.launchpad.net/qemu/+bug/1105670/.
>>>>
>>>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>>>> ---
>>>>
>>>> This is a rebased extract from my patch series for block/vpc.c.
>>>> It's the minimum needed to fix the open bug for QEMU 1.4.
>>>>
>>>> The rest of the series can be discussed and applied after 1.4.
>>>>
>>>> Regards
>>>>
>>>> Stefan W.
>>>>
>>>> PS. Please excuse a previous personal mail which I had sent
>>>> with a wrong signature and without addressing qemu-devel.
>>>>
>>>>  block/vpc.c |   14 +++++++++-----
>>>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/block/vpc.c b/block/vpc.c
>>>> index 82229ef..b4ff564 100644
>>>> --- a/block/vpc.c
>>>> +++ b/block/vpc.c
>>>> @@ -34,6 +34,8 @@
>>>>  
>>>>  #define HEADER_SIZE 512
>>>>  
>>>> +#define VHD_SECTOR_SIZE 512
>>>> +
>>>>  //#define CACHE
>>>>  
>>>>  enum vhd_type {
>>>> @@ -204,11 +206,13 @@ 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 a image in Virtual PC depends on the guest:
>>>> +     * QEMU and other emulators report the real size (here in sectors).
>>>> +     * All modern operating systems use this real size.
>>>> +     * Very old operating systems use CHS values to calculate the total size.
>>>> +     * This calculated size is usually smaller than the real size.
>>>> +     */
>>>> +    bs->total_sectors = be64_to_cpu(footer->size) / VHD_SECTOR_SIZE;
>>> It's unfortunate that I don't have my old Virtual PC installation around
>>> any more so I could prove that you're wrong for at least some versions.
>>> Or does a Linux of 2009 already count as "very old"?
> 
> Linux is a modern OS per definition :-)
> 
> I made an additional test with Knoppix (from December 2006),
> and it worked as expected.

So I'm afraid that the reason isn't the guest OS but the Virtual PC
version. I guess supporting newer versions of it is more important, so
if Jeff's testing passes, I'm not against taking the patch, even though
I don't feel completely comfortable about it. But I don't see how we
could support both ways at the same time.

Kevin
Jeff Cody - Feb. 11, 2013, 7:38 p.m.
On Mon, Feb 11, 2013 at 09:54:56AM +0100, Kevin Wolf wrote:
> Am 08.02.2013 18:43, schrieb Stefan Weil:
> > Am 08.02.2013 13:14, schrieb Jeff Cody:
> >> On Fri, Feb 08, 2013 at 09:38:47AM +0100, Kevin Wolf wrote:
> >>> Am 07.02.2013 20:26, schrieb Stefan Weil:
> >>>> From: Stefan Weil <stefan@kiwi.(none)>
> >>>>
> >>>> 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. All modern operating systems use the real size.
> >>>>
> >>>> This patch fixes https://bugs.launchpad.net/qemu/+bug/1105670/.
> >>>>
> >>>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> >>>> ---
> >>>>
> >>>> This is a rebased extract from my patch series for block/vpc.c.
> >>>> It's the minimum needed to fix the open bug for QEMU 1.4.
> >>>>
> >>>> The rest of the series can be discussed and applied after 1.4.
> >>>>
> >>>> Regards
> >>>>
> >>>> Stefan W.
> >>>>
> >>>> PS. Please excuse a previous personal mail which I had sent
> >>>> with a wrong signature and without addressing qemu-devel.
> >>>>
> >>>>  block/vpc.c |   14 +++++++++-----
> >>>>  1 file changed, 9 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/block/vpc.c b/block/vpc.c
> >>>> index 82229ef..b4ff564 100644
> >>>> --- a/block/vpc.c
> >>>> +++ b/block/vpc.c
> >>>> @@ -34,6 +34,8 @@
> >>>>  
> >>>>  #define HEADER_SIZE 512
> >>>>  
> >>>> +#define VHD_SECTOR_SIZE 512
> >>>> +
> >>>>  //#define CACHE
> >>>>  
> >>>>  enum vhd_type {
> >>>> @@ -204,11 +206,13 @@ 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 a image in Virtual PC depends on the guest:
> >>>> +     * QEMU and other emulators report the real size (here in sectors).
> >>>> +     * All modern operating systems use this real size.
> >>>> +     * Very old operating systems use CHS values to calculate the total size.
> >>>> +     * This calculated size is usually smaller than the real size.
> >>>> +     */
> >>>> +    bs->total_sectors = be64_to_cpu(footer->size) / VHD_SECTOR_SIZE;
> >>> It's unfortunate that I don't have my old Virtual PC installation around
> >>> any more so I could prove that you're wrong for at least some versions.
> >>> Or does a Linux of 2009 already count as "very old"?
> > 
> > Linux is a modern OS per definition :-)
> > 
> > I made an additional test with Knoppix (from December 2006),
> > and it worked as expected.
> 
> So I'm afraid that the reason isn't the guest OS but the Virtual PC
> version. I guess supporting newer versions of it is more important, so
> if Jeff's testing passes, I'm not against taking the patch, even though
> I don't feel completely comfortable about it. But I don't see how we
> could support both ways at the same time.
> 

I've done some testing back and forth between VPC, Win Server 2012
Hyper-V, and qemu/kvm with and without this patch applied.

The short: this patch will break Virtual PC VHD compatibility (as it
exists under Win7), but allow compatibility for VHD files created by
Hyper-V (at least under Server 2012).

VPC and Server 2012 Hyper-V are also incompatible with each other,
when it comes to VHD files, if that is any consolation.

I used sha1sums between the different combination of VHD files filled to
capacity with random data under VPC, qemu/kvm (with & without the
patch), and Hyper-V.  Without this patch we are an exact math with
VPC, and with this patch we are an exact match with Hyper-V.

However, perhaps we can support both, depending on who created the VHD
file.  There is a "Creator Application" field in the VHD header - for
the VPC-created VHD file, that field is ASCII "vpc ".  For the Hyper-V
created VHD file, that field is ASCII "win ".  If we see the field as
"vpc ", we could default to using the CHS geometry found in the
header - otherwise, do as this patch does, and go by the virtual disk
size.

Jeff
Kevin Wolf - Feb. 11, 2013, 8:09 p.m.
Am 11.02.2013 20:38, schrieb Jeff Cody:
> I've done some testing back and forth between VPC, Win Server 2012
> Hyper-V, and qemu/kvm with and without this patch applied.

First of all, thanks a lot for doing all the testing!

> The short: this patch will break Virtual PC VHD compatibility (as it
> exists under Win7), but allow compatibility for VHD files created by
> Hyper-V (at least under Server 2012).
> 
> VPC and Server 2012 Hyper-V are also incompatible with each other,
> when it comes to VHD files, if that is any consolation.

Okay. I had hoped that Win 7 would be fine as well and we might just get
away with applying the patch, but this we can't possibly ignore.

> I used sha1sums between the different combination of VHD files filled to
> capacity with random data under VPC, qemu/kvm (with & without the
> patch), and Hyper-V.  Without this patch we are an exact math with
> VPC, and with this patch we are an exact match with Hyper-V.
> 
> However, perhaps we can support both, depending on who created the VHD
> file.  There is a "Creator Application" field in the VHD header - for
> the VPC-created VHD file, that field is ASCII "vpc ".  For the Hyper-V
> created VHD file, that field is ASCII "win ".  If we see the field as
> "vpc ", we could default to using the CHS geometry found in the
> header - otherwise, do as this patch does, and go by the virtual disk
> size.

That they fill in different creator strings is good news. This isn't a
nice solution, but then it wasn't nice to change the semantics between
the two versions, so it's better than nothing. I'd vote for comparing
the creator field.

One open question is what to do with images that are neither "vpc " (or
"qemu") nor "win ". I think the newer behaviour is saner, but I don't
know if there are other tools for which the old semantics should be used.

The other question is what to do with newly created images. Maybe we
should add a creation option. At the same time we should probably switch
from the "qemu" creator string to something different if Hyper-V
semantics is used, in order to allow distinguishing the version. I'm
also not sure what the right default would be for now.

Kevin
Anthony Liguori - Feb. 11, 2013, 8:13 p.m.
Applied.  Thanks.

Regards,

Anthony Liguori
Jeff Cody - Feb. 11, 2013, 8:25 p.m.
On Mon, Feb 11, 2013 at 09:09:48PM +0100, Kevin Wolf wrote:
> Am 11.02.2013 20:38, schrieb Jeff Cody:
> > I've done some testing back and forth between VPC, Win Server 2012
> > Hyper-V, and qemu/kvm with and without this patch applied.
> 
> First of all, thanks a lot for doing all the testing!
> 
> > The short: this patch will break Virtual PC VHD compatibility (as it
> > exists under Win7), but allow compatibility for VHD files created by
> > Hyper-V (at least under Server 2012).
> > 
> > VPC and Server 2012 Hyper-V are also incompatible with each other,
> > when it comes to VHD files, if that is any consolation.
> 
> Okay. I had hoped that Win 7 would be fine as well and we might just get
> away with applying the patch, but this we can't possibly ignore.
> 
> > I used sha1sums between the different combination of VHD files filled to
> > capacity with random data under VPC, qemu/kvm (with & without the
> > patch), and Hyper-V.  Without this patch we are an exact math with
> > VPC, and with this patch we are an exact match with Hyper-V.
> > 
> > However, perhaps we can support both, depending on who created the VHD
> > file.  There is a "Creator Application" field in the VHD header - for
> > the VPC-created VHD file, that field is ASCII "vpc ".  For the Hyper-V
> > created VHD file, that field is ASCII "win ".  If we see the field as
> > "vpc ", we could default to using the CHS geometry found in the
> > header - otherwise, do as this patch does, and go by the virtual disk
> > size.
> 
> That they fill in different creator strings is good news. This isn't a
> nice solution, but then it wasn't nice to change the semantics between
> the two versions, so it's better than nothing. I'd vote for comparing
> the creator field.

+1 

> 
> One open question is what to do with images that are neither "vpc " (or
> "qemu") nor "win ". I think the newer behaviour is saner, but I don't
> know if there are other tools for which the old semantics should be used.
>

And it does get a bit trickier than that - it is possible to have a
creator field "lie".  Hyper-V does not give any warnings or errors
when opening the VPC VHD file, and since Hyper-V will just interpret
the disk as slightly larger, it will most likely work fine.  

But it is possible that Hyper-V will then modify the disk in a way
that uses the extra space (e.g. the guest reformatis / repartitions
the drive).  It does not update the creator field, so it still shows
the older "vpc ". If we went by that alone we would have the same
problem over again.

This is kind of a mess, but I think the best solution may be something
along the lines of:

1. By default, automatically use the creator field
2. Allow a way for the user to override this behavior, to allow manual
recovery from fringe cases

> The other question is what to do with newly created images. Maybe we
> should add a creation option. At the same time we should probably switch
> from the "qemu" creator string to something different if Hyper-V
> semantics is used, in order to allow distinguishing the version. I'm
> also not sure what the right default would be for now.
>
Paolo Bonzini - Feb. 11, 2013, 8:52 p.m.
Il 11/02/2013 21:13, Anthony Liguori ha scritto:
> Applied.  Thanks.
> 
> Regards,
> 
> Anthony Liguori

I guess this was a mistake, please revert.

Paolo
Anthony Liguori - Feb. 11, 2013, 9:14 p.m.
Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 11/02/2013 21:13, Anthony Liguori ha scritto:
>> Applied.  Thanks.
>> 
>> Regards,
>> 
>> Anthony Liguori
>
> I guess this was a mistake, please revert.

No, it wasn't.  The patch was reviewed and tested.  What's the problem.
It's proposed for 1.4 so it's now or never.

I saw some discussion about improving the detection for the different
varients of VHD...

I don't mind reverting it, but just want to understand what the issue
is.

Regards,

Anthony Liguori

>
> Paolo
Jeff Cody - Feb. 11, 2013, 9:20 p.m.
On Mon, Feb 11, 2013 at 03:14:36PM -0600, Anthony Liguori wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > Il 11/02/2013 21:13, Anthony Liguori ha scritto:
> >> Applied.  Thanks.
> >> 
> >> Regards,
> >> 
> >> Anthony Liguori
> >
> > I guess this was a mistake, please revert.
> 
> No, it wasn't.  The patch was reviewed and tested.  What's the problem.
> It's proposed for 1.4 so it's now or never.
> 
> I saw some discussion about improving the detection for the different
> varients of VHD...
> 
> I don't mind reverting it, but just want to understand what the issue
> is.
>

Hi Anthony,

It was tested, the problem is that this will break existing support
for Virtual PC VHD files under certain conditions (most likely
exporting from qemu to VPC, but I don't know if we could guarantee
that is the only way).


Jeff
Stefan Weil - Feb. 11, 2013, 9:33 p.m.
Am 11.02.2013 20:38, schrieb Jeff Cody:
> I've done some testing back and forth between VPC, Win Server 2012
> Hyper-V, and qemu/kvm with and without this patch applied.
>
> The short: this patch will break Virtual PC VHD compatibility (as it
> exists under Win7), but allow compatibility for VHD files created by
> Hyper-V (at least under Server 2012).
>
> VPC and Server 2012 Hyper-V are also incompatible with each other,
> when it comes to VHD files, if that is any consolation.
>
> I used sha1sums between the different combination of VHD files filled to
> capacity with random data under VPC, qemu/kvm (with & without the
> patch), and Hyper-V.  Without this patch we are an exact math with
> VPC, and with this patch we are an exact match with Hyper-V.
>
> However, perhaps we can support both, depending on who created the VHD
> file.  There is a "Creator Application" field in the VHD header - for
> the VPC-created VHD file, that field is ASCII "vpc ".  For the Hyper-V
> created VHD file, that field is ASCII "win ".  If we see the field as
> "vpc ", we could default to using the CHS geometry found in the
> header - otherwise, do as this patch does, and go by the virtual disk
> size.
>
> Jeff

Hi Jeff,

could you please give some more details of your VPC test scenario?

How did you create the VHD disk imagesfor VPC?
Which guest OS did you run?
Which disk size does the guest OS report?
Does VPC behave differently when it gets disk images with
a different signature (for example from Hyper V)?

I'd like to reproduce your test results in my environment.

Does the patch really break Virtual PC VHD compatibility?

Or does VPC simply pass the size calculated from CHSto the
guest - no matter how the disk image was created?In this case
I see no need that QEMU must show the same behaviour.
It's important to implement the VHD specification correctly,
but we should not duplicate bugs of other emulators just
to be compatible with them.

Thanks,

Stefan
Jeff Cody - Feb. 11, 2013, 10:33 p.m.
On Mon, Feb 11, 2013 at 10:33:53PM +0100, Stefan Weil wrote:
> Am 11.02.2013 20:38, schrieb Jeff Cody:
> > I've done some testing back and forth between VPC, Win Server 2012
> > Hyper-V, and qemu/kvm with and without this patch applied.
> >
> > The short: this patch will break Virtual PC VHD compatibility (as it
> > exists under Win7), but allow compatibility for VHD files created by
> > Hyper-V (at least under Server 2012).
> >
> > VPC and Server 2012 Hyper-V are also incompatible with each other,
> > when it comes to VHD files, if that is any consolation.
> >
> > I used sha1sums between the different combination of VHD files filled to
> > capacity with random data under VPC, qemu/kvm (with & without the
> > patch), and Hyper-V.  Without this patch we are an exact math with
> > VPC, and with this patch we are an exact match with Hyper-V.
> >
> > However, perhaps we can support both, depending on who created the VHD
> > file.  There is a "Creator Application" field in the VHD header - for
> > the VPC-created VHD file, that field is ASCII "vpc ".  For the Hyper-V
> > created VHD file, that field is ASCII "win ".  If we see the field as
> > "vpc ", we could default to using the CHS geometry found in the
> > header - otherwise, do as this patch does, and go by the virtual disk
> > size.
> >
> > Jeff
> 
> Hi Jeff,
> 
> could you please give some more details of your VPC test scenario?

Hello Stefan,

Sure, no problem.

I will answer each of your questions inline below, but before that
perhaps an overview would be useful:

Ultimately, it doesn't matter if VPC or Hyper-V created the image,
what matters is who fills the data in, or partitions those images.
Consider the two cases below:

A.) Virtual PC appears to go by the CHS data in the header/footer, as
qemu currently does without the patch.  

B.) Hyper-V, and qemu with the patch, use the virtual disk size. This
usually yields a disk size slightly larger than what is seen in A).

If you are importing from A->B, then there _likely_ will not be any
issues, but of course this cannot be 100% guaranteed.

If you are importing from B->A then there _will_ likely be issues, as
you are going from a larger drive to smaller drive.

This patch just changes which one is more supported from a VHD
perspective - Virtual PC, or Hyper-V.  If this was fresh code without
existing support, I would say being compatible with Hyper-V would be
more important. 

However, there are two important factors:

1.) QEMU currently is able to import and export dynamic VHD files
from/to Virtual PC.  This would break that..

2.) I would wager most use cases of Hyper-V would be using VHDX,
instead of VHD anyway.  I am working currently on implementing VHDX
support to address see (my work in progress is over at github: 
https://github.com/codyprime/qemu-kvm-jtc.git , branch jtc-vhdx-work).

3.) With some more effort, we can reasonably support both use cases,
as long as the user has a way to force method if needed.
> 
> How did you create the VHD disk imagesfor VPC?

I created the images with Virtual PC under Windows 7.  I created a new
virtual machine, with a new drive image.  I booted from a live ISO to
partition the image, and fill it with data. I created images of
varying sizes, but all of the same type: dynamic vhd images.

The easiest way to replicate the problem is to use a smaller image,
because you can fill it up with data without much trouble. For my
small image test, I used ~130MB image.

I also created images under Hyper-V, of the same type.  As you know,
those images work fine with the patch.  And again, it is not so much
the creator, but who partitioned or filled the drive with data.

One easy way to reproduce the discrepancy:

Using the same image, from either VPC or Hyper-V, inside the guest
fill the test image with pseudo-random data, and make note of the
sha1sum:

e.g., from VPC, inside a linux guest: 
    "cat /dev/urandom > /dev/sdX; sha1sum /dev/sdX"

Then, under either Hyper-V or qemu with your patch, verify that sum:
    "sha1sum /dev/vdX"  (qemu w/virtio)
    "sha1sum /dev/sdX"  (Hyper-V)

You'll notice that with your patch, the sha1sum will not match the one
computed from the guest with VPC as the host.  However, if you try
qemu _without_ your patch, the sha1sum will match.  Both Hyper-V and
qemu will compute the same sha1sum, however.

You can then do this in reverse, with the same image under Hyper-V:
fill it with pseudo-random data, make note of the sha1sum, and check
that sha1sum from VPC and qemu.  You'll notice the same - with your
patch, qemu and Hyper-V agree on sha1sums, and VPC does not.  Without
your patch, qemu and VPC agree on sha1sums, and Hyper-V does not.

> Which guest OS did you run?

I used Linux, either Fedora 18 or a lightweight live ISO of puppy
Linux (slacko 5.4).  Both gave the same results.  The version of Linux
did not seem to matter (as is to be expected).

> Which disk size does the guest OS report?

The guest OS under Virtual PC reports a smaller size, as observed by
fdisk.  For instance, on my smaller image, the disk size under Virtual
PC was reported as "134111232 bytes".  Under Hyper-V and qemu/kvm with
your patch, that same image is reported as "134217728 bytes".  Without
your patch, qemu again agrees with VPC, with "134111232 bytes".

> Does VPC behave differently when it gets disk images with
> a different signature (for example from Hyper V)?

Yes, and this is the case most likely to have an immediately noticeable
failure.  Linux refuses to mount the internal partition, because of
the incorrect geometry and too-large partition.

> 
> I'd like to reproduce your test results in my environment.
> 
> Does the patch really break Virtual PC VHD compatibility?
> 

Most likely, with this patch import of a VHD will in most cases be
fine, because we will just detect a slightly larger disk in qemu.

However, if the guest modifies the image, we don't know that it won't
do so in a way that will case the image to not be readable by Virtual
PC.  Most likely, the operation that would cause this would be
something like repartitioning the image - but anything that makes use
of the slightly large space would naturally lose that data going to
Virtual PC.

> Or does VPC simply pass the size calculated from CHSto the
> guest - no matter how the disk image was created?In this case
> I see no need that QEMU must show the same behaviour.
> It's important to implement the VHD specification correctly,
> but we should not duplicate bugs of other emulators just
> to be compatible with them.
> 

I disagree. The purpose of supporting VHD is to be compatible with
other emulators, so to the extent possible that we can do that, we
should. 

> Thanks,
> 
> Stefan
> 

Thanks,
Jeff
Kevin Wolf - Feb. 12, 2013, 8:42 a.m.
Am 11.02.2013 22:14, schrieb Anthony Liguori:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> Il 11/02/2013 21:13, Anthony Liguori ha scritto:
>>> Applied.  Thanks.
>>>
>>> Regards,
>>>
>>> Anthony Liguori
>>
>> I guess this was a mistake, please revert.
> 
> No, it wasn't.  The patch was reviewed and tested.  What's the problem.
> It's proposed for 1.4 so it's now or never.

The problem is that the test failed. Why do we even test things if we
commit anyway when the test fails?

And why do we have subsystem trees when a committer comes (without even
having taken part in the discussion) and applies patches that the
subsystem maintainers are very obviously not happy with? If you like,
you can have the block subsystem back, just send - or better just apply
- a patch against MAINTAINERS.

> I saw some discussion about improving the detection for the different
> varients of VHD...
> 
> I don't mind reverting it, but just want to understand what the issue
> is.

It makes qemu use the wrong size for VHD images created by Virtual PC
that used to work with qemu. This is a regression that can result in
images appearing corrupted in Virtual PC, and we need very good
justification to apply such a patch.

At the same time the patch enables the use of the correct size for
Hyper-V images, but that has never worked with qemu, so if we can't get
it to work with both for 1.4, I'd rather compromise on this one and not
apply the patch.

Please revert.

Kevin
Paolo Bonzini - Feb. 12, 2013, 8:46 a.m.
Il 12/02/2013 09:42, Kevin Wolf ha scritto:
>> > No, it wasn't.  The patch was reviewed and tested.  What's the problem.
>> > It's proposed for 1.4 so it's now or never.
> The problem is that the test failed. Why do we even test things if we
> commit anyway when the test fails?
> 
> And why do we have subsystem trees when a committer comes (without even
> having taken part in the discussion) and applies patches that the
> subsystem maintainers are very obviously not happy with? If you like,
> you can have the block subsystem back, just send - or better just apply
> - a patch against MAINTAINERS.
> 

I think this is just a problem with the new tools, that didn't make it
clear that you were not happy with the patch.  (It was clear from the
mailing list).

In general, the new tools sound like an improvement, but it's normal to
have a misunderstanding or two in the initial period.

Paolo
Kevin Wolf - Feb. 12, 2013, 8:47 a.m.
Am 11.02.2013 22:33, schrieb Stefan Weil:
> Does the patch really break Virtual PC VHD compatibility?
> 
> Or does VPC simply pass the size calculated from CHSto the
> guest - no matter how the disk image was created?In this case
> I see no need that QEMU must show the same behaviour.
> It's important to implement the VHD specification correctly,
> but we should not duplicate bugs of other emulators just
> to be compatible with them.

Bollocks. The whole point of supporting non-native image formats is
compatibility with other software. It's a good thing to be able to read
standard compliant images, but it's much more important to be able to
read real life images. We have real life images of both VPC and Hyper-V,
so we need to do our best to support both. Failing that, we need to stay
consistent with older qemu versions.

Kevin
Markus Armbruster - Feb. 12, 2013, 9:12 a.m.
Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 12/02/2013 09:42, Kevin Wolf ha scritto:
>>> > No, it wasn't.  The patch was reviewed and tested.  What's the problem.
>>> > It's proposed for 1.4 so it's now or never.
>> The problem is that the test failed. Why do we even test things if we
>> commit anyway when the test fails?
>> 
>> And why do we have subsystem trees when a committer comes (without even
>> having taken part in the discussion) and applies patches that the
>> subsystem maintainers are very obviously not happy with? If you like,
>> you can have the block subsystem back, just send - or better just apply
>> - a patch against MAINTAINERS.
>> 
>
> I think this is just a problem with the new tools, that didn't make it
> clear that you were not happy with the patch.  (It was clear from the
> mailing list).
>
> In general, the new tools sound like an improvement, but it's normal to
> have a misunderstanding or two in the initial period.

Yup.  No need to get excited.

Let's revert this patch for 1.4, keeping QEMU exactly as working and as
broken as it has always been.  Then figure out which additional cases we
can make work for 1.5, and which ones we have to break for that, if any.
Stefan Hajnoczi - Feb. 12, 2013, 11:24 a.m.
On Tue, Feb 12, 2013 at 10:12:59AM +0100, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > Il 12/02/2013 09:42, Kevin Wolf ha scritto:
> >>> > No, it wasn't.  The patch was reviewed and tested.  What's the problem.
> >>> > It's proposed for 1.4 so it's now or never.
> >> The problem is that the test failed. Why do we even test things if we
> >> commit anyway when the test fails?
> >> 
> >> And why do we have subsystem trees when a committer comes (without even
> >> having taken part in the discussion) and applies patches that the
> >> subsystem maintainers are very obviously not happy with? If you like,
> >> you can have the block subsystem back, just send - or better just apply
> >> - a patch against MAINTAINERS.
> >> 
> >
> > I think this is just a problem with the new tools, that didn't make it
> > clear that you were not happy with the patch.  (It was clear from the
> > mailing list).
> >
> > In general, the new tools sound like an improvement, but it's normal to
> > have a misunderstanding or two in the initial period.
> 
> Yup.  No need to get excited.
> 
> Let's revert this patch for 1.4, keeping QEMU exactly as working and as
> broken as it has always been.  Then figure out which additional cases we
> can make work for 1.5, and which ones we have to break for that, if any.

I'll send a block pull request that includes a revert.

Stefan
Stefan Weil - Feb. 12, 2013, 2:39 p.m.
Am 12.02.2013 09:42, schrieb Kevin Wolf:
> It makes qemu use the wrong size for VHD images created by Virtual PC
> that used to work with qemu. This is a regression that can result in
> images appearing corrupted in Virtual PC, and we need very good
> justification to apply such a patch.
>
> At the same time the patch enables the use of the correct size for
> Hyper-V images, but that has never worked with qemu, so if we can't get
> it to work with both for 1.4, I'd rather compromise on this one and not
> apply the patch.
>
> Please revert.
>
> Kevin

I'd appreciate if we could use a more precise description of
the results and the problems with or without the patch.

QEMU does _not_ use a wrong size for any VHD images with the patch.
It passes the correct real image size to the guest. This is compatible
with the behaviour of Hyper-V, Virtual Box and maybe other emulators
which implement the VHD specification as published by Microsoft.
According to the tests run, it is incompatible with VPC which
passes a calculated CHS size.

For images created with QEMU, the calculated CHS size and the real
size are always be the same, so these images work in any case.

It should also be possible to create such images with any other
tool by specifying an image size which is an exact CHS size,
but in most cases VHD images will have a size which is larger
than the calculated CHS size.

As far as we currently know, VPC (and no other tool) passes
the calculated CHS size to its guest.

Copying such images to any other format using qemu-img
works both with and without the patch because all valid
data is below the CHS size.

Copying any kind of non VHD image to VHD also works with and
without the patch because QEMU always calculates an image size
based on the CHS size for newly created images.

Copying a VHD image which was written beyond the CHS size to VHD
needs the patch because otherwise the data behind the CHS size
would get lost. The resulting image should work with any emulator,
also with VPC, because it again has a real size which is equal
to the CHS size.

This all means that the patch is still correct, does not introduce
a regression and should be in QEMU 1.4.

The only problem I see with the patch is my wrong mail address.
Anthony's semi-automated commit did not fix the wrong address
which I had supplied.

Cheers,

Stefan W.
Stefan Weil - Feb. 12, 2013, 2:47 p.m.
Am 11.02.2013 22:20, schrieb Jeff Cody:
> On Mon, Feb 11, 2013 at 03:14:36PM -0600, Anthony Liguori wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> Il 11/02/2013 21:13, Anthony Liguori ha scritto:
>>>> Applied.  Thanks.
>>>>
>>>> Regards,
>>>>
>>>> Anthony Liguori
>>> I guess this was a mistake, please revert.
>> No, it wasn't.  The patch was reviewed and tested.  What's the problem.
>> It's proposed for 1.4 so it's now or never.
>>
>> I saw some discussion about improving the detection for the different
>> varients of VHD...
>>
>> I don't mind reverting it, but just want to understand what the issue
>> is.
>>
> Hi Anthony,
>
> It was tested, the problem is that this will break existing support
> for Virtual PC VHD files under certain conditions (most likely
> exporting from qemu to VPC, but I don't know if we could guarantee
> that is the only way).
>
>
> Jeff

Hi Jeff,

exporting from QEMU to VPCwill work because QEMU uses
images where real size = CHS size, so those images work
no matter whether the patch was applied or not.

See my separate mail for more reasoning.

I see no way how the patch could break a realistic test scenario,
but I know such scenarios which are broken without the patch.


Stefan
Stefan Weil - Feb. 12, 2013, 2:49 p.m.
Am 12.02.2013 12:24, schrieb Stefan Hajnoczi:
> On Tue, Feb 12, 2013 at 10:12:59AM +0100, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> Il 12/02/2013 09:42, Kevin Wolf ha scritto:
>>>>>> No, it wasn't.  The patch was reviewed and tested.  What's the problem.
>>>>>> It's proposed for 1.4 so it's now or never.
>>>> The problem is that the test failed. Why do we even test things if we
>>>> commit anyway when the test fails?
>>>>
>>>> And why do we have subsystem trees when a committer comes (without even
>>>> having taken part in the discussion) and applies patches that the
>>>> subsystem maintainers are very obviously not happy with? If you like,
>>>> you can have the block subsystem back, just send - or better just apply
>>>> - a patch against MAINTAINERS.
>>>>
>>> I think this is just a problem with the new tools, that didn't make it
>>> clear that you were not happy with the patch.  (It was clear from the
>>> mailing list).
>>>
>>> In general, the new tools sound like an improvement, but it's normal to
>>> have a misunderstanding or two in the initial period.
>> Yup.  No need to get excited.
>>
>> Let's revert this patch for 1.4, keeping QEMU exactly as working and as
>> broken as it has always been.  Then figure out which additional cases we
>> can make work for 1.5, and which ones we have to break for that, if any.
> I'll send a block pull request that includes a revert.
>
> Stefan


I'm not convinced that this would be a good idea.
See my other mail for the reason.

Stefan
Jeff Cody - Feb. 12, 2013, 3:18 p.m.
On Tue, Feb 12, 2013 at 03:47:38PM +0100, Stefan Weil wrote:
> Am 11.02.2013 22:20, schrieb Jeff Cody:
> > On Mon, Feb 11, 2013 at 03:14:36PM -0600, Anthony Liguori wrote:
> >> Paolo Bonzini <pbonzini@redhat.com> writes:
> >>
> >>> Il 11/02/2013 21:13, Anthony Liguori ha scritto:
> >>>> Applied.  Thanks.
> >>>>
> >>>> Regards,
> >>>>
> >>>> Anthony Liguori
> >>> I guess this was a mistake, please revert.
> >> No, it wasn't.  The patch was reviewed and tested.  What's the problem.
> >> It's proposed for 1.4 so it's now or never.
> >>
> >> I saw some discussion about improving the detection for the different
> >> varients of VHD...
> >>
> >> I don't mind reverting it, but just want to understand what the issue
> >> is.
> >>
> > Hi Anthony,
> >
> > It was tested, the problem is that this will break existing support
> > for Virtual PC VHD files under certain conditions (most likely
> > exporting from qemu to VPC, but I don't know if we could guarantee
> > that is the only way).
> >
> >
> > Jeff
> 
> Hi Jeff,
> 
> exporting from QEMU to VPCwill work because QEMU uses
> images where real size = CHS size, so those images work
> no matter whether the patch was applied or not.
> 
> See my separate mail for more reasoning.
> 
> I see no way how the patch could break a realistic test scenario,
> but I know such scenarios which are broken without the patch.
> 
> 
> Stefan
> 

Hi Stefan,

I should have been clearer when I said export - I am talking about
qemu modifying an existing VHD image created by VPC, and then trying
to use image in VPC again.

Here is a simple way to see how the proposed patch breaks current VHD
compatibility:

1.) Under VPC, define a new VM that boots from a live dvd iso (I used
a puppy linux iso because it was small and quick to test:
http://puppylinux.org). For the hard drive for the machine, define a
128GB dynamic drive.  (test.vhd).  You don't need to boot this VM yet.

2.) Now, under QEMU, boot again to a live iso or existing linux image,
and use the newly created test.vhd.  Create a single partition, using
the fdisk defaults for the maximum size.  Shutdown.

3.) Use that same VHD image and boot the live iso with your VPC
machine.  Try to mount the partition you created under qemu with the
guest in VPC.  It will fail to detect the partition, and complain
about invalid geometry.

I think your patch is definitely needed, don't get me wrong, and I can
verify that it works - for the bug report, I even came to the same
conclusion.  I just think it needs to be done such that we don't break
something that used to work, and testing shows that it does,
unfortunately.

Thanks,
Jeff
Anthony Liguori - Feb. 12, 2013, 3:21 p.m.
Kevin Wolf <kwolf@redhat.com> writes:

> Am 11.02.2013 22:14, schrieb Anthony Liguori:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> Il 11/02/2013 21:13, Anthony Liguori ha scritto:
>>>> Applied.  Thanks.
>>>>
>>>> Regards,
>>>>
>>>> Anthony Liguori
>>>
>>> I guess this was a mistake, please revert.
>> 
>> No, it wasn't.  The patch was reviewed and tested.  What's the problem.
>> It's proposed for 1.4 so it's now or never.

(Note, I already have a revert queued, tested over night, and will push
this morning)

> The problem is that the test failed. Why do we even test things if we
> commit anyway when the test fails?

That wasn't at all clear to me.  Besides, by the time Jeff's note came,
the commit was already pushed.  The 'Thanks' note was delayed compared
to the actual push.

> And why do we have subsystem trees when a committer comes (without even
> having taken part in the discussion) and applies patches that the
> subsystem maintainers are very obviously not happy with? 

There are two block maintainers.  One of the block maintainers, Stefan,
offered a Reviewed-by.

> If you like,
> you can have the block subsystem back, just send - or better just apply
> - a patch against MAINTAINERS.

The real problem here was that it was not clear to me whether to expect
a pull request for -rc2 or not and from whom.  I received very few pull
requests during this -rc cycle and most submaintainers have asked me to
apply patches directly because of the short cycle.

In fact, I reached out twice on IRC attempting to clarify whether I was
going to get a block pull request for -rc2 and got no response from you
or Stefan.

Regards,

Anthony Liguori
Stefan Weil - Feb. 12, 2013, 4:26 p.m.
Am 12.02.2013 16:18, schrieb Jeff Cody:
> On Tue, Feb 12, 2013 at 03:47:38PM +0100, Stefan Weil wrote:
>> Am 11.02.2013 22:20, schrieb Jeff Cody:
>>> On Mon, Feb 11, 2013 at 03:14:36PM -0600, Anthony Liguori wrote:
>>>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>>>
>>>>> Il 11/02/2013 21:13, Anthony Liguori ha scritto:
>>>>>> Applied.  Thanks.
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> Anthony Liguori
>>>>> I guess this was a mistake, please revert.
>>>> No, it wasn't.  The patch was reviewed and tested.  What's the problem.
>>>> It's proposed for 1.4 so it's now or never.
>>>>
>>>> I saw some discussion about improving the detection for the different
>>>> varients of VHD...
>>>>
>>>> I don't mind reverting it, but just want to understand what the issue
>>>> is.
>>>>
>>> Hi Anthony,
>>>
>>> It was tested, the problem is that this will break existing support
>>> for Virtual PC VHD files under certain conditions (most likely
>>> exporting from qemu to VPC, but I don't know if we could guarantee
>>> that is the only way).
>>>
>>>
>>> Jeff
>> Hi Jeff,
>>
>> exporting from QEMU to VPCwill work because QEMU uses
>> images where real size = CHS size, so those images work
>> no matter whether the patch was applied or not.
>>
>> See my separate mail for more reasoning.
>>
>> I see no way how the patch could break a realistic test scenario,
>> but I know such scenarios which are broken without the patch.
>>
>>
>> Stefan
>>
> Hi Stefan,
>
> I should have been clearer when I said export - I am talking about
> qemu modifying an existing VHD image created by VPC, and then trying
> to use image in VPC again.
>
> Here is a simple way to see how the proposed patch breaks current VHD
> compatibility:
>
> 1.) Under VPC, define a new VM that boots from a live dvd iso (I used
> a puppy linux iso because it was small and quick to test:
> http://puppylinux.org). For the hard drive for the machine, define a
> 128GB dynamic drive.  (test.vhd).  You don't need to boot this VM yet.
>
> 2.) Now, under QEMU, boot again to a live iso or existing linux image,
> and use the newly created test.vhd.  Create a single partition, using
> the fdisk defaults for the maximum size.  Shutdown.
>
> 3.) Use that same VHD image and boot the live iso with your VPC
> machine.  Try to mount the partition you created under qemu with the
> guest in VPC.  It will fail to detect the partition, and complain
> about invalid geometry.
>
> I think your patch is definitely needed, don't get me wrong, and I can
> verify that it works - for the bug report, I even came to the same
> conclusion.  I just think it needs to be done such that we don't break
> something that used to work, and testing shows that it does,
> unfortunately.
>
> Thanks,
> Jeff


Hi Jeff,

the scenario which you describe here is not one which I'd call
"realistic". Why should a user create a VHD image with VPC
and then create the partitions using QEMU?Yes, if this is
done, the image is no longer compatible with VPC as soon as
blocks after the CHS size are used. The same kind of problem
would occur if Hyper-V or VirtualBox were use instead of QEMU.

I expect that the common use case is a user who creates and uses
a VHD image with VPC (this includes partitioning). This kind of
image can later be used with QEMU, VirtualBox or Hyper-V without
problems as long as the partitions are not modified to use the
additional blocks which are available after the CHS size.

This restriction seems acceptable for me. I see no way to avoid it.

The same kind of restriction exists for real hardware since the
introduction of LBA as soon as software (BIOS, bootloaders,
DOS, ...) tried using CHS.

Regards,

Stefan
Markus Armbruster - Feb. 12, 2013, 4:44 p.m.
Stefan Weil <sw@weilnetz.de> writes:

> Hi Jeff,
>
> the scenario which you describe here is not one which I'd call
> "realistic". Why should a user create a VHD image with VPC
[...]

Maybe, but why run a risk for 1.4?  Let's delay your fix to 1.5.  Gives
us plenty of time to hash out the issues.

Patch

diff --git a/block/vpc.c b/block/vpc.c
index 82229ef..b4ff564 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -34,6 +34,8 @@ 
 
 #define HEADER_SIZE 512
 
+#define VHD_SECTOR_SIZE 512
+
 //#define CACHE
 
 enum vhd_type {
@@ -204,11 +206,13 @@  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 a image in Virtual PC depends on the guest:
+     * QEMU and other emulators report the real size (here in sectors).
+     * All modern operating systems use this real size.
+     * Very old operating systems use CHS values to calculate the total size.
+     * This calculated size is usually smaller than the real size.
+     */
+    bs->total_sectors = be64_to_cpu(footer->size) / VHD_SECTOR_SIZE;
 
     /* Allow a maximum disk size of approximately 2 TB */
     if (bs->total_sectors >= 65535LL * 255 * 255) {