[PATCH 7/8] qemu-img: convert: don't use unallocated_blocks_are_zero

Vladimir Sementsov-Ogievskiy posted 8 patches 5 years, 6 months ago
Maintainers: Kevin Wolf <kwolf@redhat.com>, Jeff Cody <codyprime@gmail.com>, Fam Zheng <fam@euphon.net>, Stefan Hajnoczi <stefanha@redhat.com>, Ronnie Sahlberg <ronniesahlberg@gmail.com>, Max Reitz <mreitz@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Stefan Weil <sw@weilnetz.de>, Peter Lieven <pl@kamp.de>
There is a newer version of this series
[PATCH 7/8] qemu-img: convert: don't use unallocated_blocks_are_zero
Posted by Vladimir Sementsov-Ogievskiy 5 years, 6 months ago
qemu-img convert wants to distinguish ZERO which comes from short
backing files. unallocated_blocks_are_zero field of bdi is unrelated:
space after EOF is always considered to be zero anyway. So, just make
post_backing_zero true in case of short backing file.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qemu-img.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 6a4327aaba..4fe751878b 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1632,7 +1632,6 @@ typedef struct ImgConvertState {
     BlockBackend *target;
     bool has_zero_init;
     bool compressed;
-    bool unallocated_blocks_are_zero;
     bool target_is_new;
     bool target_has_backing;
     int64_t target_backing_sectors; /* negative if unknown */
@@ -1677,7 +1676,7 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
 
     if (s->target_backing_sectors >= 0) {
         if (sector_num >= s->target_backing_sectors) {
-            post_backing_zero = s->unallocated_blocks_are_zero;
+            post_backing_zero = true;
         } else if (sector_num + n > s->target_backing_sectors) {
             /* Split requests around target_backing_sectors (because
              * starting from there, zeros are handled differently) */
@@ -2580,7 +2579,6 @@ static int img_convert(int argc, char **argv)
     } else {
         s.compressed = s.compressed || bdi.needs_compressed_writes;
         s.cluster_sectors = bdi.cluster_size / BDRV_SECTOR_SIZE;
-        s.unallocated_blocks_are_zero = bdi.unallocated_blocks_are_zero;
     }
 
     ret = convert_do_copy(&s);
-- 
2.21.0


Re: [PATCH 7/8] qemu-img: convert: don't use unallocated_blocks_are_zero
Posted by Eric Blake 5 years, 6 months ago
On 5/6/20 4:25 AM, Vladimir Sementsov-Ogievskiy wrote:
> qemu-img convert wants to distinguish ZERO which comes from short
> backing files. unallocated_blocks_are_zero field of bdi is unrelated:
> space after EOF is always considered to be zero anyway. So, just make
> post_backing_zero true in case of short backing file.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   qemu-img.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)

This patch should come first in the series.  It would have saved me a 
lot of time in reviewing 1/8.

> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 6a4327aaba..4fe751878b 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1632,7 +1632,6 @@ typedef struct ImgConvertState {
>       BlockBackend *target;
>       bool has_zero_init;
>       bool compressed;
> -    bool unallocated_blocks_are_zero;
>       bool target_is_new;
>       bool target_has_backing;
>       int64_t target_backing_sectors; /* negative if unknown */
> @@ -1677,7 +1676,7 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
>   
>       if (s->target_backing_sectors >= 0) {
>           if (sector_num >= s->target_backing_sectors) {
> -            post_backing_zero = s->unallocated_blocks_are_zero;
> +            post_backing_zero = true;
>           } else if (sector_num + n > s->target_backing_sectors) {
>               /* Split requests around target_backing_sectors (because
>                * starting from there, zeros are handled differently) */
> @@ -2580,7 +2579,6 @@ static int img_convert(int argc, char **argv)
>       } else {
>           s.compressed = s.compressed || bdi.needs_compressed_writes;
>           s.cluster_sectors = bdi.cluster_size / BDRV_SECTOR_SIZE;
> -        s.unallocated_blocks_are_zero = bdi.unallocated_blocks_are_zero;
>       }

My question in 1/8 about whether we have iotest coverage of this 
optimization remains, but I concur that the block layer takes care of 
reading zero when encountering unallocated blocks beyond EOF of a short 
backing file, and therefore performing this optimization always rather 
than just when the driver claims that unallocated blocks read as zero 
should be safe.

Reviewed-by: Eric Blake <eblake@redhat.com>

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


Re: [PATCH 7/8] qemu-img: convert: don't use unallocated_blocks_are_zero
Posted by Vladimir Sementsov-Ogievskiy 5 years, 6 months ago
06.05.2020 17:49, Eric Blake wrote:
> On 5/6/20 4:25 AM, Vladimir Sementsov-Ogievskiy wrote:
>> qemu-img convert wants to distinguish ZERO which comes from short
>> backing files. unallocated_blocks_are_zero field of bdi is unrelated:
>> space after EOF is always considered to be zero anyway. So, just make
>> post_backing_zero true in case of short backing file.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qemu-img.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> This patch should come first in the series.  It would have saved me a lot of time in reviewing 1/8.

I'm sorry :(

> 
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 6a4327aaba..4fe751878b 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1632,7 +1632,6 @@ typedef struct ImgConvertState {
>>       BlockBackend *target;
>>       bool has_zero_init;
>>       bool compressed;
>> -    bool unallocated_blocks_are_zero;
>>       bool target_is_new;
>>       bool target_has_backing;
>>       int64_t target_backing_sectors; /* negative if unknown */
>> @@ -1677,7 +1676,7 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
>>       if (s->target_backing_sectors >= 0) {
>>           if (sector_num >= s->target_backing_sectors) {
>> -            post_backing_zero = s->unallocated_blocks_are_zero;
>> +            post_backing_zero = true;
>>           } else if (sector_num + n > s->target_backing_sectors) {
>>               /* Split requests around target_backing_sectors (because
>>                * starting from there, zeros are handled differently) */
>> @@ -2580,7 +2579,6 @@ static int img_convert(int argc, char **argv)
>>       } else {
>>           s.compressed = s.compressed || bdi.needs_compressed_writes;
>>           s.cluster_sectors = bdi.cluster_size / BDRV_SECTOR_SIZE;
>> -        s.unallocated_blocks_are_zero = bdi.unallocated_blocks_are_zero;
>>       }
> 
> My question in 1/8 about whether we have iotest coverage of this optimization remains, but I concur that the block layer takes care of reading zero when encountering unallocated blocks beyond EOF of a short backing file, and therefore performing this optimization always rather than just when the driver claims that unallocated blocks read as zero should be safe.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Thanks!

-- 
Best regards,
Vladimir