From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40991) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YM0ti-0000D2-5w for qemu-devel@nongnu.org; Thu, 12 Feb 2015 16:02:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YM0tf-0001LK-0y for qemu-devel@nongnu.org; Thu, 12 Feb 2015 16:02:06 -0500 Received: from prv-mh.provo.novell.com ([137.65.248.74]:50097) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YM0te-0001Ku-BQ for qemu-devel@nongnu.org; Thu, 12 Feb 2015 16:02:02 -0500 Message-Id: <54DCB25502000091000EE465@prv-mh.provo.novell.com> Date: Thu, 12 Feb 2015 14:01:57 -0700 From: "Charles Arnold" References: <20150210133414.GE5202@noname.str.redhat.com> <20150210134242.GB19775@localhost.localdomain> <20150210135439.GF5202@noname.str.redhat.com> <54DA0EEA.7050908@kamp.de> <20150210145329.GG5202@noname.str.redhat.com> <54DC7112.30809@kamp.de> <20150212095821.GE4189@noname.str.redhat.com> <54DC7A1E.6070405@kamp.de> <20150212100638.GF4189@noname.str.redhat.com> <54DC7BC4.8080709@kamp.de> <20150212102307.GG4189@noname.str.redhat.com> <54DC7DFB02000091000EE3A0@prv-mh.provo.novell.com> In-Reply-To: Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Content-Disposition: inline Subject: Re: [Qemu-devel] [RFC PATCH] vpc: Ignore geometry for large images List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Lieven Cc: Kevin Wolf , Jeff Cody , "" , "" >>> On 2/12/2015 at 12:05 PM, Peter Lieven wrote:=20 > Am 12.02.2015 um 18:18 schrieb Charles Arnold : >=20 >>>>> On 2/12/2015 at 03:23 AM, Kevin Wolf wrote: >>> Am 12.02.2015 um 11:09 hat Peter Lieven geschrieben: >>>>> Am 12.02.2015 um 11:06 schrieb Kevin Wolf: >>>>> Am 12.02.2015 um 11:02 hat Peter Lieven geschrieben: >>>>>>> Am 12.02.2015 um 10:58 schrieb Kevin Wolf: >>>>>>> Am 12.02.2015 um 10:23 hat Peter Lieven geschrieben: >>>>>>>>> Am 10.02.2015 um 15:53 schrieb Kevin Wolf: >>>>>>>>> Am 10.02.2015 um 15:00 hat Peter Lieven geschrieben: >>>>>>>>>>> Am 10.02.2015 um 14:54 schrieb Kevin Wolf: >>>>>>>>>>> Am 10.02.2015 um 14:42 hat Jeff Cody geschrieben: >>>>>>>>>>>> On Tue, Feb 10, 2015 at 02:34:14PM +0100, Kevin Wolf wrote: >>>>>>>>>>>>> Am 10.02.2015 um 12:41 hat Peter Lieven geschrieben: >>>>>>>>>>>>>> Am 09.02.2015 um 17:09 schrieb Kevin Wolf: >>>>>>>>>>>>>>> The CHS calculation as done per the VHD spec imposes a = maximum >>>>>>>>>>>>>>> image size of ~127 GB. Real VHD images exist that are = larger than >>>>>>>>>>>>>>> that. >>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>> Apparently there are two separate non-standard ways to = achieve >>>>>>>>>>>>>>> this: You could use more heads than the spec does - this = is the >>>>>>>>>>>>>>> option that qemu-img create chooses. >>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>> However, other images exist where the geometry is set to = the >>>>>>>>>>>>>>> maximum (65536/16/255), but the actual image size is = larger. >>>>>>>>>>>>>>> Until now, such images are truncated at 127 GB when = opening them >>>>>>>>>>>>>>> with qemu. >>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>> This patch changes the vpc driver to ignore geometry in = this case >>>>>>>>>>>>>>> and only trust the size field in the header. >>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>> Signed-off-by: Kevin Wolf --- >>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>> Peter, I'm replacing some of your code in the hope that = the new >>>>>>>>>>>>>>> approach is more generally valid. Of course, I haven't = tested if >>>>>>>>>>>>>>> your case with disk2vhd is still covered. Could you check = this, >>>>>>>>>>>>>>> please? >>>>>>>>>>>>>> I checked this and found that disk2vhd always sets CHS to = 65535ULL >>>>>>>>>>>>>> * 16 * 255 independed of the real size. >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>> But, as the conversion to CHS may have an error its maybe = the best >>>>>>>>>>>>>> solution to ignore CHS completely and always derive = total_sectors >>>>>>>>>>>>>> from footer->size unconditionally. >>>>>>>>>>>>>> I had a look at what virtualbox does and they only rely on >>>>>>>>>>>>>> footer->size. If they alter the size or create an image the = write >>>>>>>>>>>>>> the new size into the footer and recalculate CHS by the = formula >>>>>>>>>>>>>> found in the appendix of the original spec. >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>> Check vhdCreateImage, vhdOpen in >>>>>>>>>>>>>> http://www.virtualbox.org/svn/vbox/trunk/src/VBox/Storage/VH= D.cpp >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>> The original spec also says that CHS values purpose is the = use in >>>>>>>>>>>>>> an ATA controller only. >>>>>>>>>>>>> The problem with just using footer->size back then when I >>>>>>>>>>>>> implemented this was that from the perspective of a = VirtualPC guest >>>>>>>>>>>>> run in qemu, the size of its hard disk would change, which = you don't >>>>>>>>>>>>> want either. Going from VPC to qemu would be ugly, but = mostly >>>>>>>>>>>>> harmless as the disk only grows. But if you use an image in = qemu >>>>>>>>>>>>> where the disk looks larger and then go back to VPC which = respects >>>>>>>>>>>>> geometry, your data may be truncated. >>>>>>>>>>>> I believe the vpc "creator" field is different if the image = was >>>>>>>>>>>> created by Virtual PC, versus created by Hyper-V ("vpc" and = "win", >>>>>>>>>>>> respectively, I think). Perhaps we could use that to infer a = guest >>>>>>>>>>>> image came from VirtualPC, and thus not use footer->size in = that >>>>>>>>>>>> scenario? >>>>>>>>>>> Right, I think we discussed that before. Do you remember the = outcome of >>>>>>>>>>> that discussion? I seem to remember that we had a conclusion, = but >>>>>>>>>>> apparently it was never actually implemented. >>>>>>>>>>>=20 >>>>>>>>>>> Would your proposal be to special-case "vpc" to apply the = geometry, and >>>>>>>>>>> everything else (including "win", "d2v" and "qemu") would use = the footer >>>>>>>>>>> field? >>>>>>>>>> That sounds reasonable. In any case we have to fix qemu-img = create >>>>>>>>>> to do not create out of spec geometry for images larger than = 127G. >>>>>>>>>> It should set the correct footer->size and then calculate the = geometry. >>>>>>>>> Do I understand correctly that you just volunteered to fix up = that whole >>>>>>>>> thing? ;-) >>>>>>>> I knew that this would happen ;-) >>>>>>>>=20 >>>>>>>> Regarding the C/H/S calculation. I was just wondering if we = should >>>>>>>> not set this to maximum (=3Dinvalid?) for all newly created = images. >>>>>>>> That is what disk2vhd does. >>>>>>> CHS is what Virtual PC relies on. So I guess if you did that, you >>>>>>> would render images unusable by it. Are you sure that disk2vhd = does this >>>>>>> always? I would have thought that it only does it for large = images. >>>>>> At least 2.0.1 (latest available version) does this as well as the = version >>>>>> that I used when I added the hack for d2v creator. >>>>>>=20 >>>>>> Virtual PC would not be able to use images we create with qemu-img = create >>>>>> if we use footer->size (which I suppose to reanme to footer->cur_siz= e, btw) >>>>>> to calculate bs->total_sectors because we might write data to the = end of >>>>>> the image which gets truncated in CHS format. >>>>> These kinds of problems are why I'd like to keep CHS and size always >>>>> consistent when creating an image with qemu-img. >>>>=20 >>>> Okay, then I would vote for your RFC patch + fixing qemu-img create >>>> to not generate out of spec CHS values and just set maximum which >>>> then would make vpc_open use footer->size. >>>=20 >>> Really the RFC patch or what we discussed above ("vpc" creator =3D = CHS, >>> everything else =3D footer->size)? Once I know what we prefer, I'll = send >>> the real patch. >>>=20 >>> As for heads > 16, that would essentially mean reverting 258d2edb. >>> Should be easy to do, the harder part is probably the commit message >>> explaining why it's helpful and safe. Note that the commit message of >>> 258d2edb claims that it's not out of spec. I _think_ we can do the >>> revert with a good explanation, but I'll leave that to you. >>>=20 >>> (CCed Charles who wrote that commit) >>=20 >> IIUC, the plan is to revert my old commit and use the footer->size = field to >> describe images greater than 127 GB. This change would break other = tools >> from Virtual PC, Xens vhd-util and maybe others from reading images = greater >> than 127 GB because the head field would be forced back to using 16 and >> these tools won't know to check the footer->size field. Is there = any=20 >> reason not to keep the original commit and still use the footer->size = field? >=20 > do you have a Pointer to a spec that is newer than 2006? No, that is the most recent. > the one i have=20 > describes CHS calculation up to 65535 x 16 x 255 sectors. that is set = as=20 > Maximum if total sectors is higher. > I would do the same when writing a=20 > footer. in vhd_open I would derive total_sectors from C x H x S except = for=20 > the case that it is exactly 65535 x 16 x 255. In this case I would = take=20 > footer->size / 512. > Virtualbox does it that way and at the comment from Stefan in the = commit=20 > message for your Patch suggest that you observed a similar behaviour = for=20 > HyperV. Right, although this was a long time ago for me to remember the specifics = :) In the end I think supporting at least the 2 TB size allowed by the spec = is what we need while not breaking existing images. - Charles