block/vpc optimizations

*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 <[email protected]>

---
  block/vpc.c |   15 ++++++---------
  1 file changed, 6 insertions(+), 9 deletions(-)

Quoted 7 lines

*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 <[email protected]>

---
  block/vpc.c |   15 ++++++---------
  1 file changed, 6 insertions(+), 9 deletions(-)

I would like a comment about this in or above the "if (allocated)" in the do-while loop, but in any case:

Reviewed-by Max Reitz <[email protected]>

Signed-off-by Peter Lieven <[email protected]>

---
  block/vpc.c |  116 +++++++++++++++++++++++++++--------------------------------
  1 file changed, 52 insertions(+), 64 deletions(-)

block/vpc.c +0 -0

index 326c2bb..4e5ba85 100644

block/vpc.c +31 -17

  {
      BDRVVPCState *s = bs->opaque;
-    int ret;
-    int64_t offset;
-    int64_t sectors, sectors_per_block;
-    VHDFooter *footer = (VHDFooter *) s->footer_buf;
+    VHDFooter *footer = (VHDFooter*) s->footer_buf;
+    int64_t start, offset;
+    bool allocated;
+    int n;
 
      if (be32_to_cpu(footer->type) == VHD_FIXED) {
-        return bdrv_read(bs->file, sector_num, buf, nb_sectors);
+        *pnum = nb_sectors;
+        return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
+               (sector_num << BDRV_SECTOR_BITS);
      }
-    while (nb_sectors > 0) {
-        offset = get_sector_offset(bs, sector_num, 0);
 
-        sectors_per_block = s->block_size >> BDRV_SECTOR_BITS;
-        sectors = sectors_per_block - (sector_num % sectors_per_block);
-        if (sectors > nb_sectors) {
-            sectors = nb_sectors;
+    offset = get_sector_offset(bs, sector_num, 0);
+    start = offset;
+    allocated = (offset != -1);
+    *pnum = 0;
+
+    do {
+        /* All sectors in a block are contiguous (without using the bitmap) */
+        n = ROUND_UP(sector_num + 1, s->block_size / BDRV_SECTOR_SIZE)
+          - sector_num;
+        n = MIN(n, nb_sectors);
+
+        *pnum += n;
+        sector_num += n;
+        nb_sectors -= n;
+
+        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 (offset == -1);
 
-        if (offset == -1) {
-            memset(buf, 0, sectors * BDRV_SECTOR_SIZE);
-        } else {
-            ret = bdrv_pread(bs->file, offset, buf,
-                sectors * BDRV_SECTOR_SIZE);
-            if (ret != sectors * BDRV_SECTOR_SIZE) {
+    return 0;
+}

block/vpc.c +5 -45

              }
+        } else {
+            memset(buf, 0x00, n * BDRV_SECTOR_SIZE);
          }
 
-        nb_sectors -= sectors;
-        sector_num += sectors;
-        buf += sectors * BDRV_SECTOR_SIZE;
+        sector_num += n;
+        nb_sectors -= n;
+        buf += n * BDRV_SECTOR_SIZE;
      }
      return 0;
  }

@@ -597,48 +627,6 @@ static coroutine_fn int vpc_co_write(BlockDriverState *bs, int64_t sector_num,
      return ret;
  }
 
-static int64_t coroutine_fn vpc_co_get_block_status(BlockDriverState *bs,
-        int64_t sector_num, int nb_sectors, int *pnum)
-{
-    BDRVVPCState *s = bs->opaque;
-    VHDFooter *footer = (VHDFooter*) s->footer_buf;
-    int64_t start, offset;
-    bool allocated;
-    int n;
-
-    if (be32_to_cpu(footer->type) == VHD_FIXED) {
-        *pnum = nb_sectors;
-        return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
-               (sector_num << BDRV_SECTOR_BITS);
-    }
-
-    offset = get_sector_offset(bs, sector_num, 0);
-    start = offset;
-    allocated = (offset != -1);
-    *pnum = 0;
-
-    do {
-        /* All sectors in a block are contiguous (without using the bitmap) */
-        n = ROUND_UP(sector_num + 1, s->block_size / BDRV_SECTOR_SIZE)
-          - sector_num;
-        n = MIN(n, nb_sectors);
-
-        *pnum += n;
-        sector_num += n;
-        nb_sectors -= n;
-
-        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 (offset == -1);
-
-    return 0;
-}
-
  /*
   * Calculates the number of cylinders, heads and sectors per cylinder
   * based on a given number of sectors. This is the algorithm described

Quoted 10 lines

Signed-off-by Peter Lieven <[email protected]>

---
  block/vpc.c |  116 +++++++++++++++++++++++++++--------------------------------
  1 file changed, 52 insertions(+), 64 deletions(-)

block/vpc.c +0 -0

index 326c2bb..4e5ba85 100644

block/vpc.c static int vpc_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) · line 501

Quoted 6 lines

block/vpc.c +2 -1

@@ -497,40 +497,70 @@ static int vpc_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
      return 0;
  }
  -static int vpc_read(BlockDriverState *bs, int64_t sector_num,
-                    uint8_t *buf, int nb_sectors)
+static int64_t coroutine_fn vpc_co_get_block_status(BlockDriverState *bs,
+        int64_t sector_num, int nb_sectors, int *pnum)

How about just putting the function header here? If you really have to move vpc_co_get_block_status() up, I'd rather like it to be in a separate patch.

Second, while apparently vpc_read() is actually called in a coroutine, that is pretty hard to know. Most importantly, it's not marked as a coroutine_fn. Therefore I don't think it's a good idea to call vpc_co_get_block_status() directly; I'd vote for either using bdrv_get_block_status(), or moving the content of vpc_co_get_block_status() to a non-coroutine_fn (it doesn't contain an coroutine-related function calls, so this is fine) and then making vpc_co_get_block_status() a wrapper around that, or just dropping this patch.

The latter I'm proposing because I don't really see what this patch improves. The previous vpc_read() function was pretty straightforward, too, and I don't think it was unbearably longer.

One could argue that the coroutine_fn stuff doesn't really matter in this situation because it doesn't actually do anything right now and vpc_co_get_block_status() does not call any other coroutine_fn functions in turn; however, it is a semantic contract established by include/block/coroutine.h and as far as I remember, Stefan did eventually want to have something to error out on compile-time if a non-coroutine_fn function calls a coroutine_fn. I don't like breaking this contract even if it's not bad in this specific case.

Considering you probably think bdrv_get_block_status() to be too much overhead (it will fall down to the protocol layer on VHD_FIXED) and you probably find making vpc_read() shorter justified (again, which I don't necessarily), I think moving the contents of vpc_co_get_block_status() to a non-coroutine_fn might be the best way to go.

Quoted 53 lines

block/vpc.c +31 -15

  {
      BDRVVPCState *s = bs->opaque;
-    int ret;
-    int64_t offset;
-    int64_t sectors, sectors_per_block;
-    VHDFooter *footer = (VHDFooter *) s->footer_buf;
+    VHDFooter *footer = (VHDFooter*) s->footer_buf;
+    int64_t start, offset;
+    bool allocated;
+    int n;
        if (be32_to_cpu(footer->type) == VHD_FIXED) {
-        return bdrv_read(bs->file, sector_num, buf, nb_sectors);
+        *pnum = nb_sectors;
+        return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
+               (sector_num << BDRV_SECTOR_BITS);
      }
-    while (nb_sectors > 0) {
-        offset = get_sector_offset(bs, sector_num, 0);
  -        sectors_per_block = s->block_size >> BDRV_SECTOR_BITS;
-        sectors = sectors_per_block - (sector_num % sectors_per_block);
-        if (sectors > nb_sectors) {
-            sectors = nb_sectors;
+    offset = get_sector_offset(bs, sector_num, 0);
+    start = offset;
+    allocated = (offset != -1);
+    *pnum = 0;
+
+    do {
+        /* All sectors in a block are contiguous (without using the bitmap) */
+        n = ROUND_UP(sector_num + 1, s->block_size / BDRV_SECTOR_SIZE)
+          - sector_num;
+        n = MIN(n, nb_sectors);
+
+        *pnum += n;
+        sector_num += n;
+        nb_sectors -= n;
+
+        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 (offset == -1);
  -        if (offset == -1) {
-            memset(buf, 0, sectors * BDRV_SECTOR_SIZE);
-        } else {
-            ret = bdrv_pread(bs->file, offset, buf,
-                sectors * BDRV_SECTOR_SIZE);
-            if (ret != sectors * BDRV_SECTOR_SIZE) {
+    return 0;
+}

block/vpc.c static int vpc_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) · line 550

Quoted 10 lines

block/vpc.c +10 -0

+
+static int vpc_read(BlockDriverState *bs, int64_t sector_num,
+                    uint8_t *buf, int nb_sectors)
+{
+    int ret, n;
+    int64_t ret2;
+
+    while (nb_sectors > 0) {
+        ret2 = vpc_co_get_block_status(bs, sector_num, nb_sectors, &n);
+

Superfluous whitespace here.

block/vpc.c static int vpc_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) · line 555

Quoted 5 lines

block/vpc.c +4 -0

+        if (ret2 & BDRV_BLOCK_OFFSET_VALID) {
+            ret = bdrv_pread(bs->file, ret2 & BDRV_SECTOR_MASK, buf,
+                             n * BDRV_SECTOR_SIZE);
+            if (ret != n * BDRV_SECTOR_SIZE) {
                  return -1;

Please make that "return ret" (and possibly "if (ret < 0)", if you want to).

Am 23.02.2015 um 19:29 schrieb Max Reitz:

Quoted 107 lines · Peter Lieven

Quoted 11 lines

Signed-off-by Peter Lieven <[email protected]>

---
  block/vpc.c |  116 
+++++++++++++++++++++++++++--------------------------------
  1 file changed, 52 insertions(+), 64 deletions(-)

block/vpc.c +0 -0

index 326c2bb..4e5ba85 100644

block/vpc.c static int vpc_get_info(BlockDriverState *bs, · line 503

Quoted 8 lines

block/vpc.c +2 -1

@@ -497,40 +497,70 @@ static int vpc_get_info(BlockDriverState *bs, 
 BlockDriverInfo *bdi)
      return 0;
  }
  -static int vpc_read(BlockDriverState *bs, int64_t sector_num,
-                    uint8_t *buf, int nb_sectors)
+static int64_t coroutine_fn 
 vpc_co_get_block_status(BlockDriverState *bs,
+        int64_t sector_num, int nb_sectors, int *pnum)

How about just putting the function header here? If you really have to move vpc_co_get_block_status() up, I'd rather like it to be in a separate patch.

Second, while apparently vpc_read() is actually called in a coroutine, that is pretty hard to know. Most importantly, it's not marked as a coroutine_fn. Therefore I don't think it's a good idea to call vpc_co_get_block_status() directly; I'd vote for either using bdrv_get_block_status(), or moving the content of vpc_co_get_block_status() to a non-coroutine_fn (it doesn't contain an coroutine-related function calls, so this is fine) and then making vpc_co_get_block_status() a wrapper around that, or just dropping this patch.

The latter I'm proposing because I don't really see what this patch improves. The previous vpc_read() function was pretty straightforward, too, and I don't think it was unbearably longer.

One could argue that the coroutine_fn stuff doesn't really matter in this situation because it doesn't actually do anything right now and vpc_co_get_block_status() does not call any other coroutine_fn functions in turn; however, it is a semantic contract established by include/block/coroutine.h and as far as I remember, Stefan did eventually want to have something to error out on compile-time if a non-coroutine_fn function calls a coroutine_fn. I don't like breaking this contract even if it's not bad in this specific case.

Considering you probably think bdrv_get_block_status() to be too much overhead (it will fall down to the protocol layer on VHD_FIXED) and you probably find making vpc_read() shorter justified (again, which I don't necessarily), I think moving the contents of vpc_co_get_block_status() to a non-coroutine_fn might be the best way to go.

Quoted 65 lines

block/vpc.c +6 -5

  {
      BDRVVPCState *s = bs->opaque;
-    int ret;
-    int64_t offset;
-    int64_t sectors, sectors_per_block;
-    VHDFooter *footer = (VHDFooter *) s->footer_buf;
+    VHDFooter *footer = (VHDFooter*) s->footer_buf;
+    int64_t start, offset;
+    bool allocated;
+    int n;
        if (be32_to_cpu(footer->type) == VHD_FIXED) {
-        return bdrv_read(bs->file, sector_num, buf, nb_sectors);
+        *pnum = nb_sectors;
+        return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | 

BDRV_BLOCK_DATA |

block/vpc.c +1 -3

+               (sector_num << BDRV_SECTOR_BITS);
      }
-    while (nb_sectors > 0) {
-        offset = get_sector_offset(bs, sector_num, 0);
  -        sectors_per_block = s->block_size >> BDRV_SECTOR_BITS;
-        sectors = sectors_per_block - (sector_num % 

sectors_per_block);

block/vpc.c +7 -2

-        if (sectors > nb_sectors) {
-            sectors = nb_sectors;
+    offset = get_sector_offset(bs, sector_num, 0);
+    start = offset;
+    allocated = (offset != -1);
+    *pnum = 0;
+
+    do {
+        /* All sectors in a block are contiguous (without using the 

bitmap) */

block/vpc.c +26 -5

+        n = ROUND_UP(sector_num + 1, s->block_size / BDRV_SECTOR_SIZE)
+          - sector_num;
+        n = MIN(n, nb_sectors);
+
+        *pnum += n;
+        sector_num += n;
+        nb_sectors -= n;
+
+        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 (offset == -1);
  -        if (offset == -1) {
-            memset(buf, 0, sectors * BDRV_SECTOR_SIZE);
-        } else {
-            ret = bdrv_pread(bs->file, offset, buf,
-                sectors * BDRV_SECTOR_SIZE);
-            if (ret != sectors * BDRV_SECTOR_SIZE) {
+    return 0;
+}
+
+static int vpc_read(BlockDriverState *bs, int64_t sector_num,
+                    uint8_t *buf, int nb_sectors)
+{
+    int ret, n;
+    int64_t ret2;
+
+    while (nb_sectors > 0) {
+        ret2 = vpc_co_get_block_status(bs, sector_num, nb_sectors, 

Quoted 2 lines

&n);
+

Superfluous whitespace here.

Quoted 5 lines

block/vpc.c +4 -0

+        if (ret2 & BDRV_BLOCK_OFFSET_VALID) {
+            ret = bdrv_pread(bs->file, ret2 & BDRV_SECTOR_MASK, buf,
+                             n * BDRV_SECTOR_SIZE);
+            if (ret != n * BDRV_SECTOR_SIZE) {
                  return -1;

Quoted 1 line

Please make that "return ret" (and possibly "if (ret < 0)", if you want to).

I took this from the orignal vpc_read function. Maybe the author also wanted to catch short reads.

I tend to drop the whole patch anyway. I was tempted to use that new vpc_co_get_block_status function somehow because I saw that part of its logic is in vpc_read as well.

If it should stay maybe it would be an option to inline vpc_read in vpc_co_read (and the same for write)?

From: Kevin Wolf <[email protected]>

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 <[email protected]>

---
  block/vpc.c |   10 ++++------
  1 file changed, 4 insertions(+), 6 deletions(-)

Quoted 16 lines

From: Kevin Wolf <[email protected]>

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.

Quoted 4 lines

Signed-off-by Kevin Wolf <[email protected]>

---
  block/vpc.c |   10 ++++------
  1 file changed, 4 insertions(+), 6 deletions(-)

I'm trusting you this works for disk2vhd; at least it's in accordance with the VHD specification.

Am 23.02.2015 um 19:34 schrieb Max Reitz:

Quoted 12 lines · Peter Lieven

Quoted 11 lines

From: Kevin Wolf <[email protected]>

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

Quoted 10 lines

Quoted 9 lines

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 <[email protected]>

---
  block/vpc.c |   10 ++++------
  1 file changed, 4 insertions(+), 6 deletions(-)

Quoted 1 line

I'm trusting you this works for disk2vhd; at least it's in accordance with the VHD specification.

When I wrote the hack for disk2vhd some time ago I just found that the reported size was too big. I was not aware that it was exactly this special value.

You say in the specs. Do you have a spec that is actually stating that (65536/16/255) is special value that says ignore CHS and look at the footer?

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 <[email protected]>

---
  block/vpc.c |   45 ++++++++++++++++++++++++---------------------
  1 file changed, 24 insertions(+), 21 deletions(-)

block/vpc.c +2 -1

index 11d3c86..9c5301b 100644
@@ -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,
  {

block/vpc.c +3 -6

          *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

block/vpc.c +0 -3

      for (i = 0; total_sectors > (int64_t)cyls * heads * secs_per_cyl; i++) {
-        if (calculate_geometry(total_sectors + i, &cyls, &heads,
-                               &secs_per_cyl))
-        {

block/vpc.c +0 -0

      /* Prepare the Hard Disk Footer */
      memset(buf, 0, 1024);

Quoted 44 lines

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 <[email protected]>

---
  block/vpc.c |   45 ++++++++++++++++++++++++---------------------
  1 file changed, 24 insertions(+), 21 deletions(-)

block/vpc.c +2 -1

index 11d3c86..9c5301b 100644
@@ -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 */

block/vpc.c static int calculate_geometry(int64_t total_sectors, uint16_t* cyls, · line 648

Quoted 6 lines

block/vpc.c +2 -3

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

Quoted 25 lines

          *secs_per_cyl = 255;

block/vpc.c +5 -5

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

block/vpc.c static int vpc_create(const char *filename, QemuOpts *opts, Error **errp) · line 821

Quoted 6 lines

block/vpc.c +4 -1

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

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

Quoted 4 lines

block/vpc.c +0 -2

      for (i = 0; total_sectors > (int64_t)cyls * heads * secs_per_cyl; i++) {
-        if (calculate_geometry(total_sectors + i, &cyls, &heads,
-                               &secs_per_cyl))

block/vpc.c static int vpc_create(const char *filename, QemuOpts *opts, Error **errp) · line 827

Quoted 10 lines

block/vpc.c +3 -5

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

block/vpc.c static int vpc_create(const char *filename, QemuOpts *opts, Error **errp) · line 830

Quoted 3 lines

block/vpc.c +3 -0

+        /* Allow a maximum disk size of approximately 2 TB */
+        if (total_sectors > VHD_MAX_SECTORS) {
+            return -EFBIG;

No goto out;?

block/vpc.c static int vpc_create(const char *filename, QemuOpts *opts, Error **errp) · line 835

Quoted 6 lines

block/vpc.c +5 -0

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

Am 23.02.2015 um 19:59 schrieb Max Reitz:

Quoted 50 lines · Peter Lieven

Quoted 49 lines

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 <[email protected]>

---
  block/vpc.c |   45 ++++++++++++++++++++++++---------------------
  1 file changed, 24 insertions(+), 21 deletions(-)

block/vpc.c +1 -0

index 11d3c86..9c5301b 100644
@@ -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. */

block/vpc.c +1 -1

-    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 */

Quoted 8 lines

block/vpc.c static int calculate_geometry(int64_t total_sectors, uint16_t* cyls, · line 648

Quoted 6 lines

block/vpc.c +2 -3

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

Quoted 30 lines

Quoted 30 lines

          *secs_per_cyl = 255;

block/vpc.c +3 -5

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

block/vpc.c +2 -0

+     *
+     * If the image size can't be represented by a spec conform CHS 

geometry,

Quoted 8 lines

block/vpc.c static int vpc_create(const char *filename, QemuOpts *opts, Error **errp) · line 821

Quoted 6 lines

block/vpc.c +4 -1

+     * 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

Quoted 2 lines

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.

Quoted 19 lines

Quoted 13 lines

      for (i = 0; total_sectors > (int64_t)cyls * heads * 

secs_per_cyl; i++) {

block/vpc.c +1 -6

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

block/vpc.c static int vpc_create(const char *filename, QemuOpts *opts, Error **errp) · line 827

Quoted 3 lines

block/vpc.c +2 -1

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

Quoted 7 lines

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.

block/vpc.c static int vpc_create(const char *filename, QemuOpts *opts, Error **errp) · line 830

Quoted 3 lines

block/vpc.c +3 -0

+        /* Allow a maximum disk size of approximately 2 TB */
+        if (total_sectors > VHD_MAX_SECTORS) {
+            return -EFBIG;

No goto out;?

Oops.

Quoted 18 lines

block/vpc.c static int vpc_create(const char *filename, QemuOpts *opts, Error **errp) · line 835

Quoted 6 lines

block/vpc.c +5 -0

+        }
+    } 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;

Quoted 1 line

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.

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

Signed-off-by Peter Lieven <[email protected]>

---
  block/vpc.c |    9 +++++----
  1 file changed, 5 insertions(+), 4 deletions(-)