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

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

[Peter Lieven]

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

Peter

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

Peter Lieven (4):
  block/vpc: optimize vpc_co_get_block_status
  block/vpc: simplify vpc_read
  block/vpc: make calculate_geometry spec conform
  block/vpc: rename footer->size -> footer->current_size

 block/vpc.c |  179 +++++++++++++++++++++++++++--------------------------------
 1 file changed, 83 insertions(+), 96 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH 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>
---
 block/vpc.c |   15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index 1533b6a..326c2bb 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,17 @@ 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);
 
+        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.7.9.5

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

[Max Reitz]

On 2015-02-23 at 09:27, Peter Lieven wrote:
> *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>
> ---
>   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 <mreitz@redhat.com>

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

[Peter Lieven]

Am 23.02.2015 um 19:08 schrieb Max Reitz:
> On 2015-02-23 at 09:27, Peter Lieven wrote:
>> *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>
>> ---
>>   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:

Will add in v2.

Peter

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

[Qemu-devel] [PATCH 2/5] block/vpc: simplify vpc_read

[Peter Lieven]

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

diff --git a/block/vpc.c b/block/vpc.c
index 326c2bb..4e5ba85 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -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)
 {
     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;
+}
+
+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);
+        
+        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;
             }
+        } 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
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 2/5] block/vpc: simplify vpc_read

[Max Reitz]

On 2015-02-23 at 09:27, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>   block/vpc.c |  116 +++++++++++++++++++++++++++--------------------------------
>   1 file changed, 52 insertions(+), 64 deletions(-)
>
> diff --git a/block/vpc.c b/block/vpc.c
> index 326c2bb..4e5ba85 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -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.

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

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

Max

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

Re: [Qemu-devel] [PATCH 2/5] block/vpc: simplify vpc_read

[Peter Lieven]

Am 23.02.2015 um 19:29 schrieb Max Reitz:
> On 2015-02-23 at 09:27, Peter Lieven wrote:
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   block/vpc.c |  116 +++++++++++++++++++++++++++--------------------------------
>>   1 file changed, 52 insertions(+), 64 deletions(-)
>>
>> diff --git a/block/vpc.c b/block/vpc.c
>> index 326c2bb..4e5ba85 100644
>> --- a/block/vpc.c
>> +++ b/block/vpc.c
>> @@ -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.
>
>>   {
>>       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;
>> +}
>> +
>> +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.
>
>> +        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).

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

Re: [Qemu-devel] [PATCH 2/5] block/vpc: simplify vpc_read

[Max Reitz]

On 2015-02-24 at 01:44, Peter Lieven wrote:
> Am 23.02.2015 um 19:29 schrieb Max Reitz:
>> On 2015-02-23 at 09:27, Peter Lieven wrote:
>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>> ---
>>>   block/vpc.c |  116 
>>> +++++++++++++++++++++++++++--------------------------------
>>>   1 file changed, 52 insertions(+), 64 deletions(-)
>>>
>>> diff --git a/block/vpc.c b/block/vpc.c
>>> index 326c2bb..4e5ba85 100644
>>> --- a/block/vpc.c
>>> +++ b/block/vpc.c
>>> @@ -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.
>>
>>>   {
>>>       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;
>>> +}
>>> +
>>> +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.
>>
>>> +        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).
>
> I took this from the orignal vpc_read function. Maybe the author also 
> wanted
> to catch short reads.

There are no short reads (any more), though. ;-)

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

Oh, I totally missed the vpc_co_read(). That would make sense, because 
you actually can call vpc_co_get_block_status() from there, or you just 
add the coroutine_fn specifier to vpc_read(). It's up to you, I'm fine 
with all of dropping, inlining, and making vpc_read() a coroutine_fn.

Max

[Qemu-devel] [PATCH 3/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
(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>
---
 block/vpc.c |   10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index 4e5ba85..11d3c86 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.7.9.5

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

[Max Reitz]

On 2015-02-23 at 09:27, 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
> (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>
> ---
>   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.

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

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

[Peter Lieven]

Am 23.02.2015 um 19:34 schrieb Max Reitz:
> On 2015-02-23 at 09:27, 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
>> (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>
>> ---
>>   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.

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?

Peter

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

[Max Reitz]

On 2015-02-24 at 01:45, Peter Lieven wrote:
> Am 23.02.2015 um 19:34 schrieb Max Reitz:
>> On 2015-02-23 at 09:27, 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
>>> (65536/16/255), but the actual image size is larger. Until now, such

Notice just now: This should be 65535/16/255.

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

No, but I do have a spec that says "if the number of sectors is greater 
than 65535 * 16 * 255, clamp it to 65535 * 16 * 255 for the geometry 
calculation". This implies to me that this value means that one cannot 
trust the geometry.

Max

[Qemu-devel] [PATCH 4/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, 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) {
         *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));
     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);
+        /* Allow a maximum disk size of approximately 2 TB */
+        if (total_sectors > VHD_MAX_SECTORS) {
+            return -EFBIG;
+        }
+    } else {
+        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.7.9.5

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

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

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

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

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

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

Maybe you like that better. I do, but this is not my patch, so it's up 
to you.

All in all, apart from the "return -EFBIG" instead of "goto out", the 
patch is correct (if you ignore the minor difference for total_sectors 
== 65535 * 16 * 63).

Max

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

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

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

[Max Reitz]

On 2015-02-24 at 01:49, Peter Lieven wrote:
> 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.

Right. Very well then.

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

Well, it won't work so well with the MIN() being necessary...

Max

[Qemu-devel] [PATCH 5/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>
---
 block/vpc.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index 9c5301b..a9d2b62 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 */
@@ -855,7 +856,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.7.9.5

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

[Max Reitz]

On 2015-02-23 at 09:27, Peter Lieven wrote:
> the field is named current size in the spec. Name it accordingly.
>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>   block/vpc.c |    9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)

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