[PATCH v2 2/9] block: inline bdrv_unallocated_blocks_are_zero()

Vladimir Sementsov-Ogievskiy posted 9 patches 5 years, 6 months ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Fam Zheng <fam@euphon.net>, Peter Lieven <pl@kamp.de>, Stefan Hajnoczi <stefanha@redhat.com>, Kevin Wolf <kwolf@redhat.com>, Ronnie Sahlberg <ronniesahlberg@gmail.com>, Max Reitz <mreitz@redhat.com>, Stefan Weil <sw@weilnetz.de>, Jeff Cody <codyprime@gmail.com>
There is a newer version of this series
[PATCH v2 2/9] block: inline bdrv_unallocated_blocks_are_zero()
Posted by Vladimir Sementsov-Ogievskiy 5 years, 6 months ago
The function has the only user: bdrv_co_block_status(). Inline it to
simplify reviewing of the following patches, which will finally drop
unallocated_blocks_are_zero field too.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/block.h |  1 -
 block.c               | 15 ---------------
 block/io.c            | 11 ++++++++---
 3 files changed, 8 insertions(+), 19 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 8b62429aa4..931003a476 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -431,7 +431,6 @@ int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes);
 int bdrv_has_zero_init_1(BlockDriverState *bs);
 int bdrv_has_zero_init(BlockDriverState *bs);
 int bdrv_has_zero_init_truncate(BlockDriverState *bs);
-bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs);
 bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs);
 int bdrv_block_status(BlockDriverState *bs, int64_t offset,
                       int64_t bytes, int64_t *pnum, int64_t *map,
diff --git a/block.c b/block.c
index cf5c19b1db..0283fdecbc 100644
--- a/block.c
+++ b/block.c
@@ -5305,21 +5305,6 @@ int bdrv_has_zero_init_truncate(BlockDriverState *bs)
     return 0;
 }
 
-bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs)
-{
-    BlockDriverInfo bdi;
-
-    if (bs->backing) {
-        return false;
-    }
-
-    if (bdrv_get_info(bs, &bdi) == 0) {
-        return bdi.unallocated_blocks_are_zero;
-    }
-
-    return false;
-}
-
 bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs)
 {
     if (!(bs->open_flags & BDRV_O_UNMAP)) {
diff --git a/block/io.c b/block/io.c
index a4f9714230..00e7371d50 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2386,15 +2386,20 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
     if (ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) {
         ret |= BDRV_BLOCK_ALLOCATED;
     } else if (want_zero) {
-        if (bdrv_unallocated_blocks_are_zero(bs)) {
-            ret |= BDRV_BLOCK_ZERO;
-        } else if (bs->backing) {
+        if (bs->backing) {
             BlockDriverState *bs2 = bs->backing->bs;
             int64_t size2 = bdrv_getlength(bs2);
 
             if (size2 >= 0 && offset >= size2) {
                 ret |= BDRV_BLOCK_ZERO;
             }
+        } else {
+            BlockDriverInfo bdi;
+            int ret2 = bdrv_get_info(bs, &bdi);
+
+            if (ret2 == 0 && bdi.unallocated_blocks_are_zero) {
+                ret |= BDRV_BLOCK_ZERO;
+            }
         }
     }
 
-- 
2.21.0


Re: [PATCH v2 2/9] block: inline bdrv_unallocated_blocks_are_zero()
Posted by Eric Blake 5 years, 6 months ago
On 5/7/20 3:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> The function has the only user: bdrv_co_block_status(). Inline it to

s/the only/only one/

> simplify reviewing of the following patches, which will finally drop
> unallocated_blocks_are_zero field too.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   include/block/block.h |  1 -
>   block.c               | 15 ---------------
>   block/io.c            | 11 ++++++++---
>   3 files changed, 8 insertions(+), 19 deletions(-)
> 

> +++ b/block/io.c
> @@ -2386,15 +2386,20 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
>       if (ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) {
>           ret |= BDRV_BLOCK_ALLOCATED;
>       } else if (want_zero) {
> -        if (bdrv_unallocated_blocks_are_zero(bs)) {
> -            ret |= BDRV_BLOCK_ZERO;
> -        } else if (bs->backing) {
> +        if (bs->backing) {
>               BlockDriverState *bs2 = bs->backing->bs;
>               int64_t size2 = bdrv_getlength(bs2);
>   
>               if (size2 >= 0 && offset >= size2) {
>                   ret |= BDRV_BLOCK_ZERO;
>               }
> +        } else {
> +            BlockDriverInfo bdi;
> +            int ret2 = bdrv_get_info(bs, &bdi);
> +
> +            if (ret2 == 0 && bdi.unallocated_blocks_are_zero) {

Could perhaps condense to:

else {
     BlockDriverInfo bdi;

     if (bdrv_get_info(bs, &bd) == 0 &&
         bdi.unallocated_blocks_are_zero) {

but that's cosmetic.

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 v2 2/9] block: inline bdrv_unallocated_blocks_are_zero()
Posted by Vladimir Sementsov-Ogievskiy 5 years, 5 months ago
07.05.2020 17:08, Eric Blake wrote:
> On 5/7/20 3:47 AM, Vladimir Sementsov-Ogievskiy wrote:
>> The function has the only user: bdrv_co_block_status(). Inline it to
> 
> s/the only/only one/
> 
>> simplify reviewing of the following patches, which will finally drop
>> unallocated_blocks_are_zero field too.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   include/block/block.h |  1 -
>>   block.c               | 15 ---------------
>>   block/io.c            | 11 ++++++++---
>>   3 files changed, 8 insertions(+), 19 deletions(-)
>>
> 
>> +++ b/block/io.c
>> @@ -2386,15 +2386,20 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
>>       if (ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) {
>>           ret |= BDRV_BLOCK_ALLOCATED;
>>       } else if (want_zero) {
>> -        if (bdrv_unallocated_blocks_are_zero(bs)) {
>> -            ret |= BDRV_BLOCK_ZERO;
>> -        } else if (bs->backing) {
>> +        if (bs->backing) {
>>               BlockDriverState *bs2 = bs->backing->bs;
>>               int64_t size2 = bdrv_getlength(bs2);
>>               if (size2 >= 0 && offset >= size2) {
>>                   ret |= BDRV_BLOCK_ZERO;
>>               }
>> +        } else {
>> +            BlockDriverInfo bdi;
>> +            int ret2 = bdrv_get_info(bs, &bdi);
>> +
>> +            if (ret2 == 0 && bdi.unallocated_blocks_are_zero) {
> 
> Could perhaps condense to:
> 
> else {
>      BlockDriverInfo bdi;
> 
>      if (bdrv_get_info(bs, &bd) == 0 &&
>          bdi.unallocated_blocks_are_zero) {
> 
> but that's cosmetic.

I'll keep it as is, as it is to be removed in 09 anyway.

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


-- 
Best regards,
Vladimir