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

[Peter Lieven]

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 <address@hidden>
> > ---
> >   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