Re: [Qemu-devel] [PATCH 4/5] block/vpc: make calculate_geometry spec conform

[Peter Lieven <pl@kamp.de>]

Am 23.02.2015 um 19:59 schrieb Max Reitz:
> On 2015-02-23 at 09:27, Peter Lieven wrote:
>> The VHD spec [1] allows for total_sectors of 65535 x 16 x 255 (~127GB)
>> represented by a CHS geometry. If total_sectors is greater
>> than 65535 x 16 x 255 this geometry is set as a maximum.
>>
>> Qemu, Hyper-V, VirtualBox and disk2vhd use this special geometry as an
>> indicator to use the image current size from the footer as disk size.
>>
>> This patch changes vpc_create to effectively calculate a CxHxS geometry
>> for the given image size if possible while rounding up if necessary.
>> If the image size is too big to be represented in CHS we set the maximum
>> and write the exact requested image size into the footer.
>>
>> This partly reverts commit 258d2edb, but leaves support for >127G disks
>> intact.
>>
>> [1] http://download.microsoft.com/download/f/f/e/ffef50a5-07dd-4cf8-aaa3-442c0673a029/Virtual%20Hard%20Disk%20Format%20Spec_10_18_06.doc
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   block/vpc.c |   45 ++++++++++++++++++++++++---------------------
>>   1 file changed, 24 insertions(+), 21 deletions(-)
>>
>> diff --git a/block/vpc.c b/block/vpc.c
>> index 11d3c86..9c5301b 100644
>> --- a/block/vpc.c
>> +++ b/block/vpc.c
>> @@ -46,6 +46,7 @@ enum vhd_type {
>>   #define VHD_TIMESTAMP_BASE 946684800
>>     #define VHD_MAX_SECTORS       (65535LL * 255 * 255)
>> +#define VHD_MAX_GEOMETRY      (65535LL *  16 * 255)
>>     // always big-endian
>>   typedef struct vhd_footer {
>> @@ -218,7 +219,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
>>       /* Images that have exactly the maximum geometry are probably bigger and
>>        * would be truncated if we adhered to the geometry for them. Rely on
>>        * footer->size for them. */
>> -    if (bs->total_sectors == 65535ULL * 16 * 255) {
>> +    if (bs->total_sectors == VHD_MAX_GEOMETRY) {
>>           bs->total_sectors = be64_to_cpu(footer->size) / BDRV_SECTOR_SIZE;
>>       }
>>   @@ -642,26 +643,20 @@ static int calculate_geometry(int64_t total_sectors, uint16_t* cyls,
>>   {
>>       uint32_t cyls_times_heads;
>>   -    /* Allow a maximum disk size of approximately 2 TB */
>> -    if (total_sectors > 65535LL * 255 * 255) {
>> -        return -EFBIG;
>> -    }
>> +    total_sectors = MIN(total_sectors, VHD_MAX_GEOMETRY);
>>   -    if (total_sectors > 65535 * 16 * 63) {
>> +    if (total_sectors > 65535LL * 16 * 63) {
>
> While it is >= in the specification, the "else" branch does work fine with the total_sectors = 65535 * 16 * 63 case. So I'm leaving it up to you whether to make it really really spec-conform or at least functionally spec-conform (although the spec says 
> 16 heads, 16191 cylinders, and 255 sectors per cylinder for this case, whereas the "else" branch leaves us with 16 heads, 65535 cylinders, and 63 sectors per cylinder).

I will adjust it to match the spec exactly.

>
>>           *secs_per_cyl = 255;
>> -        if (total_sectors > 65535 * 16 * 255) {
>> -            *heads = 255;
>> -        } else {
>> -            *heads = 16;
>> -        }
>> +        *heads = 16;
>>           cyls_times_heads = total_sectors / *secs_per_cyl;
>>       } else {
>>           *secs_per_cyl = 17;
>>           cyls_times_heads = total_sectors / *secs_per_cyl;
>>           *heads = (cyls_times_heads + 1023) / 1024;
>>   -        if (*heads < 4)
>> +        if (*heads < 4) {
>>               *heads = 4;
>> +        }
>>             if (cyls_times_heads >= (*heads * 1024) || *heads > 16) {
>>               *secs_per_cyl = 31;
>> @@ -817,19 +812,27 @@ static int vpc_create(const char *filename, QemuOpts *opts, Error **errp)
>>        * 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 = total_size / BDRV_SECTOR_SIZE;
>> +    total_sectors = MIN(VHD_MAX_GEOMETRY,
>> +                        DIV_ROUND_UP(total_size, BDRV_SECTOR_SIZE));
>
> DIV_ROUND_UP() is unnecessary, total_size is a multiple of BDRV_SECTOR_SIZE anyway.

ok

>
> I find the MIN() unnecessary, too, because calculate_geometry() will take care of that itself.

Actually the MIN() is the guarantee that the for-loop terminates for images with total_sectors > VHD_MAX_GEOMETRY.
Otherwise (int64_t)cyls * heads * secs_per_cyl would always be VHD_MAX_GEOMETRY which is always
smaller than VHD_MAX_GEOMETRY. This would keep us looping for good.

>
>>       for (i = 0; total_sectors > (int64_t)cyls * heads * secs_per_cyl; i++) {
>> -        if (calculate_geometry(total_sectors + i, &cyls, &heads,
>> -                               &secs_per_cyl))
>> -        {
>> -            ret = -EFBIG;
>> -            goto out;
>> -        }
>> +        calculate_geometry(total_sectors + i, &cyls, &heads, &secs_per_cyl);
>>       }
>>   -    total_sectors = (int64_t) cyls * heads * secs_per_cyl;
>> -    total_size = total_sectors * BDRV_SECTOR_SIZE;
>> +    if ((int64_t)cyls * heads * secs_per_cyl == VHD_MAX_GEOMETRY) {
>> +        total_sectors = DIV_ROUND_UP(total_size, BDRV_SECTOR_SIZE);
>
> Again, DIV_ROUND_UP() is unnecessary.
>
> So this is equivalent to total_sectors = total_size / BDRV_SECTOR_SIZE, which, if you omit the MIN() above, is exactly what total_sectors already contains, so you can just omit this assignment.
>
>> +        /* Allow a maximum disk size of approximately 2 TB */
>> +        if (total_sectors > VHD_MAX_SECTORS) {
>> +            return -EFBIG;
>
> No goto out;?

Oops.

>
>> +        }
>> +    } else {
>> +        total_sectors = (int64_t)cyls * heads * secs_per_cyl;
>> +        total_size = total_sectors * BDRV_SECTOR_SIZE;
>> +    }
>
> An alternative way of writing this would be:
>
> if ((int64_t)cyls * heads * secs_per_cyl < VHD_MAX_GEOMETRY) {
>     total_sectors = (int64_t)cyls * heads * secs_per_cyl;
> }
> if (total_sectors > VHD_MAX_SECTORS) {
>     ret = -EFBIG;
>     goto out;
> }
> total_size = total_sectors * BDRV_SECTOR_SIZE;
>
> Or, alternatively, it'd be possible to drop the total_size assignment and just use total_sectors * BDRV_SECTOR_SIZE in the assignments of footer->orig_size and footer->size.

Will look at this.

Thank you,
Peter