| Message ID | 1359755494-28012-6-git-send-email-sw@weilnetz.de |
|---|---|
| State | Under Review |
| Headers | show |
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
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
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
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
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.
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) {
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(-)