| Message ID | 1447428724-28116-1-git-send-email-lpetrut@cloudbasesolutions.com |
|---|---|
| State | New |
| Headers | show |
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);
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
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
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. :-)
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. :-) >
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 :)
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);
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(-)