Re: [Qemu-devel] [RFC PATCH] vpc: Ignore geometry for large images

[Peter Lieven <pl@kamp.de>]

[-- Attachment #1: Type: text/plain, Size: 6437 bytes --]

Am 12.02.2015 um 11:23 schrieb Kevin Wolf:
> 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 <kwolf@redhat.com> ---
>>>>>>>>>>>>>
>>>>>>>>>>>>> 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.

The RFC patch as is.

>
> 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.

The only spec I have access to is [1].
If you check the CHS calculation code in the appendix, you will find:

if (totalSectors > 65535 * 16 * 255)

{

totalSectors = 65535 * 16 * 255;

}



[1] http://download.microsoft.com/download/f/f/e/ffef50a5-07dd-4cf8-aaa3-442c0673a029/Virtual%20Hard%20Disk%20Format%20Spec_10_18_06.doc

Peter


[-- Attachment #2: Type: text/html, Size: 10277 bytes --]