From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59086) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YO4py-0004UC-Cc for qemu-devel@nongnu.org; Wed, 18 Feb 2015 08:38:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YO4ps-0006RY-Rc for qemu-devel@nongnu.org; Wed, 18 Feb 2015 08:38:45 -0500 Received: from mx-v6.kamp.de ([2a02:248:0:51::16]:56952 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YO4ps-0006Pf-Ch for qemu-devel@nongnu.org; Wed, 18 Feb 2015 08:38:40 -0500 Message-ID: <54E495D3.80900@kamp.de> Date: Wed, 18 Feb 2015 14:38:27 +0100 From: Peter Lieven MIME-Version: 1.0 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> <54DCB25502000091000EE465@prv-mh.provo.novell.com> In-Reply-To: <54DCB25502000091000EE465@prv-mh.provo.novell.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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: Charles Arnold Cc: Kevin Wolf , Jeff Cody , "" , "" Am 12.02.2015 um 22:01 schrieb Charles Arnold: >>>> On 2/12/2015 at 12:05 PM, Peter Lieven wrote: >> Am 12.02.2015 um 18:18 schrieb Charles Arnold : >> >>>>>> 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. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> 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. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> 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. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> This patch changes the vpc driver to ignore geometry in this case >>>>>>>>>>>>>>>> and only trust the size field in the header. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Signed-off-by: Kevin Wolf --- >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> 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. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> 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. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Check vhdCreateImage, vhdOpen in >>>>>>>>>>>>>>> http://www.virtualbox.org/svn/vbox/trunk/src/VBox/Storage/VHD.cpp >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> 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. >>>>>>>>>>>> >>>>>>>>>>>> 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 ;-) >>>>>>>>> >>>>>>>>> Regarding the C/H/S calculation. I was just wondering if we should >>>>>>>>> not set this to maximum (=invalid?) 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. >>>>>>> >>>>>>> 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_size, 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. >>>>> 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. >>>> Really the RFC patch or what we discussed above ("vpc" creator = CHS, >>>> everything else = footer->size)? Once I know what we prefer, I'll send >>>> the real patch. >>>> >>>> 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. >>>> >>>> (CCed Charles who wrote that commit) >>> 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 >>> reason not to keep the original commit and still use the footer->size field? >> do you have a Pointer to a spec that is newer than 2006? > No, that is the most recent. > >> the one i have >> describes CHS calculation up to 65535 x 16 x 255 sectors. that is set as >> Maximum if total sectors is higher. >> I would do the same when writing a >> footer. in vhd_open I would derive total_sectors from C x H x S except for >> the case that it is exactly 65535 x 16 x 255. In this case I would take >> footer->size / 512. >> Virtualbox does it that way and at the comment from Stefan in the commit >> message for your Patch suggest that you observed a similar behaviour for >> 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. Thats clear, the absolute limit is 2 TB. We still refuse large images with -EFBIG. Before your patch every image above 127GB was refused with that error. Would you agree to implement the following: a) Use footer->size / 512 for bs->total_sectors iff CxHxS === 65535 x 16 x 255 (Kevins RFC Patch). Setting these values seems to be the inofficial way to say look at the footer->size. Normally I would argue to use footer->size iff CxHxS >== 65535 x 16 x 255, but this would break opening of our old out of spec images created with qemu-img. b) change calculate_geometry() to do exactly what is in the old specs of 2006. In particular this will set CxHxS to 65535 x 16 x 255 for anything >127G and thus tells vpc_open to look at the footer->size. Cheers, Peter