in such case, bdrv_get_block_status() shall return 0, *nr == 0

iotest 154 updated accordingly: write-zeroes tail alignment can be detected
as zeroes now, so pwrite_zeroes succeeds

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
---
 block/qcow2.c              | 6 ++++--
 tests/qemu-iotests/154.out | 4 ++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 2e6a0ec..b885dfc 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2482,7 +2482,7 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start,
     int64_t res;
 
     if (start + count > bs->total_sectors) {
-        count = bs->total_sectors - start;
+        count = start < bs->total_sectors ? bs->total_sectors - start : 0;
     }
 
     if (!count) {
@@ -2490,7 +2490,9 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start,
     }
     res = bdrv_get_block_status_above(bs, NULL, start, count,
                                       &nr, &file);
-    return res >= 0 && (res & BDRV_BLOCK_ZERO) && nr == count;
+    return res >= 0
+        && (((res & BDRV_BLOCK_ZERO) && nr == count)
+            || nr == 0);
 }
 
 static coroutine_fn int qcow2_co_pwrite_zeroes(BlockDriverState *bs,
diff --git a/tests/qemu-iotests/154.out b/tests/qemu-iotests/154.out
index d8485ee..259340e 100644
--- a/tests/qemu-iotests/154.out
+++ b/tests/qemu-iotests/154.out
@@ -322,7 +322,7 @@ wrote 1024/1024 bytes at offset 134218240
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 2048/2048 bytes allocated at offset 128 MiB
 [{ "start": 0, "length": 134217728, "depth": 1, "zero": true, "data": false},
-{ "start": 134217728, "length": 2048, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
+{ "start": 134217728, "length": 2048, "depth": 0, "zero": true, "data": false}]
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134219776 backing_file=TEST_DIR/t.IMGFMT.base
 wrote 2048/2048 bytes at offset 134217728
 2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
@@ -348,7 +348,7 @@ wrote 1024/1024 bytes at offset 134218240
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 2048/2048 bytes allocated at offset 128 MiB
 [{ "start": 0, "length": 134217728, "depth": 1, "zero": true, "data": false},
-{ "start": 134217728, "length": 2048, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
+{ "start": 134217728, "length": 2048, "depth": 0, "zero": true, "data": false}]
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134219776 backing_file=TEST_DIR/t.IMGFMT.base
 wrote 2048/2048 bytes at offset 134217728
 2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-- 
2.7.4

On 05/19/2017 04:34 AM, Anton Nefedov wrote:
> in such case, bdrv_get_block_status() shall return 0, *nr == 0

Sounds better as s/shall/will/

> 
> iotest 154 updated accordingly: write-zeroes tail alignment can be detected
> as zeroes now, so pwrite_zeroes succeeds
> 
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> ---
>  block/qcow2.c              | 6 ++++--
>  tests/qemu-iotests/154.out | 4 ++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 

You'll want to also patch test 154 proper to remove these two TODO comments:

# TODO: Note that this forces an allocation, because we aren't yet able to
# quickly detect that reads beyond EOF of the backing file are always zero


> diff --git a/block/qcow2.c b/block/qcow2.c
> index 2e6a0ec..b885dfc 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2482,7 +2482,7 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start,
>      int64_t res;
>  
>      if (start + count > bs->total_sectors) {
> -        count = bs->total_sectors - start;
> +        count = start < bs->total_sectors ? bs->total_sectors - start : 0;
>      }
>  
>      if (!count) {
> @@ -2490,7 +2490,9 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start,
>      }
>      res = bdrv_get_block_status_above(bs, NULL, start, count,
>                                        &nr, &file);
> -    return res >= 0 && (res & BDRV_BLOCK_ZERO) && nr == count;
> +    return res >= 0
> +        && (((res & BDRV_BLOCK_ZERO) && nr == count)
> +            || nr == 0);

The logic makes sense to me, although the formatting is unusual (we tend
to split && and || with the operator at the tail of the previous line,
not the head of the new line).

This quick check may make me revisit whether it is worth my my RFC
series about adding BDRV_BLOCK_EOF for more quickly tracking reads
beyond EOF (my solution in that series was able to make the same change
to test 154, but by doing it at the block layer instead of the qcow2.c
code).  https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01127.html


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On 05/22/2017 02:12 PM, Eric Blake wrote:

>> +++ b/block/qcow2.c
>> @@ -2482,7 +2482,7 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start,
>>      int64_t res;
>>  
>>      if (start + count > bs->total_sectors) {
>> -        count = bs->total_sectors - start;
>> +        count = start < bs->total_sectors ? bs->total_sectors - start : 0;
>>      }
>>  
>>      if (!count) {
>> @@ -2490,7 +2490,9 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start,
>>      }
>>      res = bdrv_get_block_status_above(bs, NULL, start, count,
>>                                        &nr, &file);
>> -    return res >= 0 && (res & BDRV_BLOCK_ZERO) && nr == count;
>> +    return res >= 0
>> +        && (((res & BDRV_BLOCK_ZERO) && nr == count)
>> +            || nr == 0);
> 
> The logic makes sense to me, although the formatting is unusual (we tend
> to split && and || with the operator at the tail of the previous line,
> not the head of the new line).
> 
> This quick check may make me revisit whether it is worth my my RFC
> series about adding BDRV_BLOCK_EOF for more quickly tracking reads
> beyond EOF (my solution in that series was able to make the same change
> to test 154, but by doing it at the block layer instead of the qcow2.c
> code).  https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01127.html

Actually, re-reading my RFC - I was able to change 6 instances in test
154, while your tweak only affected 2 instances (you still left four
instances that were allocating).  So my approach may still be more
optimal in the long run.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On 05/22/2017 10:14 PM, Eric Blake wrote:
> On 05/22/2017 02:12 PM, Eric Blake wrote:
>
>>> +++ b/block/qcow2.c
>>> @@ -2482,7 +2482,7 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start,
>>>      int64_t res;
>>>
>>>      if (start + count > bs->total_sectors) {
>>> -        count = bs->total_sectors - start;
>>> +        count = start < bs->total_sectors ? bs->total_sectors - start : 0;
>>>      }
>>>
>>>      if (!count) {
>>> @@ -2490,7 +2490,9 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start,
>>>      }
>>>      res = bdrv_get_block_status_above(bs, NULL, start, count,
>>>                                        &nr, &file);
>>> -    return res >= 0 && (res & BDRV_BLOCK_ZERO) && nr == count;
>>> +    return res >= 0
>>> +        && (((res & BDRV_BLOCK_ZERO) && nr == count)
>>> +            || nr == 0);
>>
>> The logic makes sense to me, although the formatting is unusual (we tend
>> to split && and || with the operator at the tail of the previous line,
>> not the head of the new line).
>>
>> This quick check may make me revisit whether it is worth my my RFC
>> series about adding BDRV_BLOCK_EOF for more quickly tracking reads
>> beyond EOF (my solution in that series was able to make the same change
>> to test 154, but by doing it at the block layer instead of the qcow2.c
>> code).  https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01127.html
>
> Actually, re-reading my RFC - I was able to change 6 instances in test
> 154, while your tweak only affected 2 instances (you still left four
> instances that were allocating).  So my approach may still be more
> optimal in the long run.
>

Yes, looks like your approach is more general; let's drop this one then

/Anton