[Qemu-devel] [PATCH v2 0/5] block/vpc optimizations

[Qemu-devel] [PATCH v2 0/5] block/vpc optimizations

[Peter Lieven]

This series covers VPC format changes discussed during the last weeks.

Peter

v1->v2:
  Patch 1: added comment why we can exit after the first allocated sector [Max]
  Patch 2: Fixed gemotry in commit msg [Max]
  Patch 3: Fixed comparision from > to >= [Max]
           Fixed exit in case of -EFBIG [Max]
           Removed DIV_ROUND_UP [Max]
  Patch 5: added

Kevin Wolf (1):
  vpc: Ignore geometry for large images

Peter Lieven (4):
  block/vpc: optimize vpc_co_get_block_status
  block/vpc: make calculate_geometry spec conform
  block/vpc: rename footer->size -> footer->current_size
  block/vpc: remove disabled code from get_sector_offset

 block/vpc.c | 106 +++++++++++++++++++++---------------------------------------
 1 file changed, 37 insertions(+), 69 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v2 1/5] block/vpc: optimize vpc_co_get_block_status

[Peter Lieven]

*pnum can't be greater than s->block_size / BDRV_SECTOR_SIZE for allocated
sectors since there is always a bitmap in between.

Signed-off-by: Peter Lieven <pl@kamp.de>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/vpc.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index 1533b6a..c8e17cb 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -602,7 +602,7 @@ static int64_t coroutine_fn vpc_co_get_block_status(BlockDriverState *bs,
 {
     BDRVVPCState *s = bs->opaque;
     VHDFooter *footer = (VHDFooter*) s->footer_buf;
-    int64_t start, offset, next;
+    int64_t start, offset;
     bool allocated;
     int n;
 
@@ -626,20 +626,18 @@ static int64_t coroutine_fn vpc_co_get_block_status(BlockDriverState *bs,
         *pnum += n;
         sector_num += n;
         nb_sectors -= n;
-        next = start + (*pnum * BDRV_SECTOR_SIZE);
-
+        /* *pnum can't be greater than one block for allocated
+         * sectors since there is always a bitmap in between. */
+        if (allocated) {
+            return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;
+        }
         if (nb_sectors == 0) {
             break;
         }
-
         offset = get_sector_offset(bs, sector_num, 0);
-    } while ((allocated && offset == next) || (!allocated && offset == -1));
+    } while (offset == -1);
 
-    if (allocated) {
-        return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;
-    } else {
-        return 0;
-    }
+    return 0;
 }
 
 /*
-- 
1.9.1

[Qemu-devel] [PATCH v2 2/5] vpc: Ignore geometry for large images

[Peter Lieven]

From: Kevin Wolf <kwolf@redhat.com>

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
(65535/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>
[PL: Fixed maximum geometry in the commit msg]
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/vpc.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index c8e17cb..1c9592c 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -215,12 +215,10 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
     bs->total_sectors = (int64_t)
         be16_to_cpu(footer->cyls) * footer->heads * footer->secs_per_cyl;
 
-    /* images created with disk2vhd report a far higher virtual size
-     * than expected with the cyls * heads * sectors_per_cyl formula.
-     * use the footer->size instead if the image was created with
-     * disk2vhd.
-     */
-    if (!strncmp(footer->creator_app, "d2v", 4)) {
+    /* 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) {
         bs->total_sectors = be64_to_cpu(footer->size) / BDRV_SECTOR_SIZE;
     }
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2 2/5] vpc: Ignore geometry for large images

[Max Reitz]

On 2015-03-03 at 05:41, Peter Lieven wrote:
> From: Kevin Wolf <kwolf@redhat.com>
>
> 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
> (65535/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>
> [PL: Fixed maximum geometry in the commit msg]
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>   block/vpc.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

[Qemu-devel] [PATCH v2 3/5] block/vpc: make calculate_geometry spec conform

[Peter Lieven]

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 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 to 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 | 41 ++++++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index 1c9592c..7e99a69 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;
     }
 
@@ -655,26 +656,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) {
         *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;
@@ -830,20 +825,28 @@ 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, total_size / BDRV_SECTOR_SIZE);
     for (i = 0; total_sectors > (int64_t)cyls * heads * secs_per_cyl; i++) {
-        if (calculate_geometry(total_sectors + i, &cyls, &heads,
-                               &secs_per_cyl))
-        {
+        calculate_geometry(total_sectors + i, &cyls, &heads, &secs_per_cyl);
+    }
+
+    if ((int64_t)cyls * heads * secs_per_cyl == VHD_MAX_GEOMETRY) {
+        total_sectors = total_size / BDRV_SECTOR_SIZE;
+        /* Allow a maximum disk size of approximately 2 TB */
+        if (total_sectors > VHD_MAX_SECTORS) {
             ret = -EFBIG;
             goto out;
         }
+    } else {
+        total_sectors = (int64_t)cyls * heads * secs_per_cyl;
+        total_size = total_sectors * BDRV_SECTOR_SIZE;
     }
 
-    total_sectors = (int64_t) cyls * heads * secs_per_cyl;
-    total_size = total_sectors * BDRV_SECTOR_SIZE;
-
     /* Prepare the Hard Disk Footer */
     memset(buf, 0, 1024);
 
-- 
1.9.1

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

[Max Reitz]

On 2015-03-03 at 05:41, 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 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 to big to be represented in CHS we set the maximum

"too big" (as it was in v1) was correct, no need to make it wrong. :-)

> 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 | 41 ++++++++++++++++++++++-------------------
>   1 file changed, 22 insertions(+), 19 deletions(-)

With s/to big/too big/:

Reviewed-by: Max Reitz <mreitz@redhat.com>

[Qemu-devel] [PATCH v2 4/5] block/vpc: rename footer->size -> footer->current_size

[Peter Lieven]

the field is named current size in the spec. Name it accordingly.

Signed-off-by: Peter Lieven <pl@kamp.de>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/vpc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index 7e99a69..226be02 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -66,7 +66,7 @@ typedef struct vhd_footer {
     char        creator_os[4]; // "Wi2k"
 
     uint64_t    orig_size;
-    uint64_t    size;
+    uint64_t    current_size;
 
     uint16_t    cyls;
     uint8_t     heads;
@@ -218,9 +218,10 @@ 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. */
+     * footer->current_size for them. */
     if (bs->total_sectors == VHD_MAX_GEOMETRY) {
-        bs->total_sectors = be64_to_cpu(footer->size) / BDRV_SECTOR_SIZE;
+        bs->total_sectors = be64_to_cpu(footer->current_size) /
+                            BDRV_SECTOR_SIZE;
     }
 
     /* Allow a maximum disk size of approximately 2 TB */
@@ -868,7 +869,7 @@ static int vpc_create(const char *filename, QemuOpts *opts, Error **errp)
     footer->major = cpu_to_be16(0x0005);
     footer->minor = cpu_to_be16(0x0003);
     footer->orig_size = cpu_to_be64(total_size);
-    footer->size = cpu_to_be64(total_size);
+    footer->current_size = cpu_to_be64(total_size);
     footer->cyls = cpu_to_be16(cyls);
     footer->heads = heads;
     footer->secs_per_cyl = secs_per_cyl;
-- 
1.9.1

[Qemu-devel] [PATCH v2 5/5] block/vpc: remove disabled code from get_sector_offset

[Peter Lieven]

The code to check the bitmap for the allocation status of each sector
has been "disabled by reason" ever since the vpc driver existed.

The reason might be that we might end up reading sector by sector
in vpc_read if we really used it. This would be a performance desaster.

The current code would furthermore not work if the disabled parts get
reactivated since vpc_read and vpc_write only use get_sector_offset to
check the allocation status of the first sector of a read/write operation.
This might lead to sectors incorrectly treated as zero in vpc_read and
to sectors getting allocated twice in vpc_write.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/vpc.c | 32 --------------------------------
 1 file changed, 32 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index 226be02..43e768e 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -376,38 +376,6 @@ static inline int64_t get_sector_offset(BlockDriverState *bs,
         bdrv_pwrite_sync(bs->file, bitmap_offset, bitmap, s->bitmap_size);
     }
 
-//    printf("sector: %" PRIx64 ", index: %x, offset: %x, bioff: %" PRIx64 ", bloff: %" PRIx64 "\n",
-//	sector_num, pagetable_index, pageentry_index,
-//	bitmap_offset, block_offset);
-
-// disabled by reason
-#if 0
-#ifdef CACHE
-    if (bitmap_offset != s->last_bitmap)
-    {
-	lseek(s->fd, bitmap_offset, SEEK_SET);
-
-	s->last_bitmap = bitmap_offset;
-
-	// Scary! Bitmap is stored as big endian 32bit entries,
-	// while we used to look it up byte by byte
-	read(s->fd, s->pageentry_u8, 512);
-	for (i = 0; i < 128; i++)
-	    be32_to_cpus(&s->pageentry_u32[i]);
-    }
-
-    if ((s->pageentry_u8[pageentry_index / 8] >> (pageentry_index % 8)) & 1)
-	return -1;
-#else
-    lseek(s->fd, bitmap_offset + (pageentry_index / 8), SEEK_SET);
-
-    read(s->fd, &bitmap_entry, 1);
-
-    if ((bitmap_entry >> (pageentry_index % 8)) & 1)
-	return -1; // not allocated
-#endif
-#endif
-
     return block_offset;
 }
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2 5/5] block/vpc: remove disabled code from get_sector_offset

[Max Reitz]

On 2015-03-03 at 05:41, Peter Lieven wrote:
> The code to check the bitmap for the allocation status of each sector
> has been "disabled by reason" ever since the vpc driver existed.
>
> The reason might be that we might end up reading sector by sector
> in vpc_read if we really used it. This would be a performance desaster.
>
> The current code would furthermore not work if the disabled parts get
> reactivated since vpc_read and vpc_write only use get_sector_offset to
> check the allocation status of the first sector of a read/write operation.
> This might lead to sectors incorrectly treated as zero in vpc_read and
> to sectors getting allocated twice in vpc_write.
>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>   block/vpc.c | 32 --------------------------------
>   1 file changed, 32 deletions(-)

Awww, that code nearly had its ten-year anniversary. Too bad it won't 
live to see it (I hope...).

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH v2 0/5] block/vpc optimizations

[Max Reitz]

On 2015-03-03 at 05:41, Peter Lieven wrote:
> This series covers VPC format changes discussed during the last weeks.
>
> Peter
>
> v1->v2:
>    Patch 1: added comment why we can exit after the first allocated sector [Max]
>    Patch 2: Fixed gemotry in commit msg [Max]
>    Patch 3: Fixed comparision from > to >= [Max]
>             Fixed exit in case of -EFBIG [Max]
>             Removed DIV_ROUND_UP [Max]
>    Patch 5: added
>
> Kevin Wolf (1):
>    vpc: Ignore geometry for large images
>
> Peter Lieven (4):
>    block/vpc: optimize vpc_co_get_block_status
>    block/vpc: make calculate_geometry spec conform
>    block/vpc: rename footer->size -> footer->current_size
>    block/vpc: remove disabled code from get_sector_offset
>
>   block/vpc.c | 106 +++++++++++++++++++++---------------------------------------
>   1 file changed, 37 insertions(+), 69 deletions(-)

Thanks, applied to my block-next tree:

https://github.com/XanClic/qemu/commits/block-next

Re: [Qemu-devel] [PATCH v2 0/5] block/vpc optimizations

[Max Reitz]

On 2015-03-03 at 16:03, Max Reitz wrote:
> On 2015-03-03 at 05:41, Peter Lieven wrote:
>> This series covers VPC format changes discussed during the last weeks.
>>
>> Peter
>>
>> v1->v2:
>>    Patch 1: added comment why we can exit after the first allocated 
>> sector [Max]
>>    Patch 2: Fixed gemotry in commit msg [Max]
>>    Patch 3: Fixed comparision from > to >= [Max]
>>             Fixed exit in case of -EFBIG [Max]
>>             Removed DIV_ROUND_UP [Max]
>>    Patch 5: added
>>
>> Kevin Wolf (1):
>>    vpc: Ignore geometry for large images
>>
>> Peter Lieven (4):
>>    block/vpc: optimize vpc_co_get_block_status
>>    block/vpc: make calculate_geometry spec conform
>>    block/vpc: rename footer->size -> footer->current_size
>>    block/vpc: remove disabled code from get_sector_offset
>>
>>   block/vpc.c | 106 
>> +++++++++++++++++++++---------------------------------------
>>   1 file changed, 37 insertions(+), 69 deletions(-)
>
> Thanks, applied to my block-next tree:
>
> https://github.com/XanClic/qemu/commits/block-next

For the sake of completeness: Moved to the block tree, now merged to master.

Max