[Qemu-devel] [PATCH] qemu-img: improve convert_iteration_sectors()

[Qemu-devel] [PATCH] qemu-img: improve convert_iteration_sectors()

[Vladimir Sementsov-Ogievskiy]

Do not do extra call to _get_block_status()

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---

Also, I'm not sure about last line:
s->status = s->target_has_backing ? BLK_BACKING_FILE : BLK_DATA;

(which is equal to old code)

may be, it should be
s->status = s->target_has_backing ? BLK_BACKING_FILE : BLK_ZERO;

as it is the case, when range is not allocated at all. Should we copy it in this case?

 qemu-img.c | 32 ++++++++++----------------------
 1 file changed, 10 insertions(+), 22 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index b220cf71d7..490f6e97f6 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1554,9 +1554,15 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
 
     if (s->sector_next_status <= sector_num) {
         BlockDriverState *file;
-        ret = bdrv_get_block_status(blk_bs(s->src[src_cur]),
-                                    sector_num - src_cur_offset,
-                                    n, &n, &file);
+        if (s->target_has_backing) {
+            ret = bdrv_get_block_status(blk_bs(s->src[src_cur]),
+                                        sector_num - src_cur_offset,
+                                        n, &n, &file);
+        } else {
+            ret = bdrv_get_block_status_above(blk_bs(s->src[src_cur]), NULL,
+                                              sector_num - src_cur_offset,
+                                              n, &n, &file);
+        }
         if (ret < 0) {
             return ret;
         }
@@ -1565,26 +1571,8 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
             s->status = BLK_ZERO;
         } else if (ret & BDRV_BLOCK_DATA) {
             s->status = BLK_DATA;
-        } else if (!s->target_has_backing) {
-            /* Without a target backing file we must copy over the contents of
-             * the backing file as well. */
-            /* Check block status of the backing file chain to avoid
-             * needlessly reading zeroes and limiting the iteration to the
-             * buffer size */
-            ret = bdrv_get_block_status_above(blk_bs(s->src[src_cur]), NULL,
-                                              sector_num - src_cur_offset,
-                                              n, &n, &file);
-            if (ret < 0) {
-                return ret;
-            }
-
-            if (ret & BDRV_BLOCK_ZERO) {
-                s->status = BLK_ZERO;
-            } else {
-                s->status = BLK_DATA;
-            }
         } else {
-            s->status = BLK_BACKING_FILE;
+            s->status = s->target_has_backing ? BLK_BACKING_FILE : BLK_DATA;
         }
 
         s->sector_next_status = sector_num + n;
-- 
2.11.1

Re: [Qemu-devel] [Qemu-block] [PATCH] qemu-img: improve convert_iteration_sectors()

[John Snow]

On 04/07/2017 07:34 AM, Vladimir Sementsov-Ogievskiy wrote:
> Do not do extra call to _get_block_status()
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> 
> Also, I'm not sure about last line:
> s->status = s->target_has_backing ? BLK_BACKING_FILE : BLK_DATA;
> 
> (which is equal to old code)
> 

What a weird function.

So, if the target has a backing file, literally nothing changes here.

If it doesn't, we skip the initial call to get_block_status and just
call the (effectively?) recursive version to find out if we have a ZERO
or DATA type of allocation.

The else clause here properly reflects the original reading of the code.

OK.

> may be, it should be
> s->status = s->target_has_backing ? BLK_BACKING_FILE : BLK_ZERO;
> 
> as it is the case, when range is not allocated at all. Should we copy it in this case?
> 

I am not really clear on if either ZERO or DATA are correct in the case
where we do not have a backing file, because maybe this depends on
has_zero_init?

If we are copying uninitialized data when has_zero_init is true on the
source, we want zeroes on the target. If the target also has_zero_init,
we can just skip this sector. If it doesn't, we want to copy zeroes.

If has_zero_init is false and we have unallocated data on the source...
what are we doing? Copying random debris?

I'm pretty confused, but I'm pretty sure your patch doesn't actually
change anything, so:

Reviewed-by: John Snow <jsnow@redhat.com>

>  qemu-img.c | 32 ++++++++++----------------------
>  1 file changed, 10 insertions(+), 22 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index b220cf71d7..490f6e97f6 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1554,9 +1554,15 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
>  
>      if (s->sector_next_status <= sector_num) {
>          BlockDriverState *file;
> -        ret = bdrv_get_block_status(blk_bs(s->src[src_cur]),
> -                                    sector_num - src_cur_offset,
> -                                    n, &n, &file);
> +        if (s->target_has_backing) {
> +            ret = bdrv_get_block_status(blk_bs(s->src[src_cur]),
> +                                        sector_num - src_cur_offset,
> +                                        n, &n, &file);
> +        } else {
> +            ret = bdrv_get_block_status_above(blk_bs(s->src[src_cur]), NULL,
> +                                              sector_num - src_cur_offset,
> +                                              n, &n, &file);
> +        }
>          if (ret < 0) {
>              return ret;
>          }
> @@ -1565,26 +1571,8 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
>              s->status = BLK_ZERO;
>          } else if (ret & BDRV_BLOCK_DATA) {
>              s->status = BLK_DATA;
> -        } else if (!s->target_has_backing) {
> -            /* Without a target backing file we must copy over the contents of
> -             * the backing file as well. */
> -            /* Check block status of the backing file chain to avoid
> -             * needlessly reading zeroes and limiting the iteration to the
> -             * buffer size */
> -            ret = bdrv_get_block_status_above(blk_bs(s->src[src_cur]), NULL,
> -                                              sector_num - src_cur_offset,
> -                                              n, &n, &file);
> -            if (ret < 0) {
> -                return ret;
> -            }
> -
> -            if (ret & BDRV_BLOCK_ZERO) {
> -                s->status = BLK_ZERO;
> -            } else {
> -                s->status = BLK_DATA;
> -            }
>          } else {
> -            s->status = BLK_BACKING_FILE;
> +            s->status = s->target_has_backing ? BLK_BACKING_FILE : BLK_DATA;
>          }
>  
>          s->sector_next_status = sector_num + n;
> 

Re: [Qemu-devel] [PATCH] qemu-img: improve convert_iteration_sectors()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 783 bytes --]

On 07.04.2017 13:34, Vladimir Sementsov-Ogievskiy wrote:
> Do not do extra call to _get_block_status()
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> 
> Also, I'm not sure about last line:
> s->status = s->target_has_backing ? BLK_BACKING_FILE : BLK_DATA;
> 
> (which is equal to old code)
> 
> may be, it should be
> s->status = s->target_has_backing ? BLK_BACKING_FILE : BLK_ZERO;
> 
> as it is the case, when range is not allocated at all. Should we copy it in this case?

Intuitively, I don't think the else branch can happen in that case at
all. If it does, BLK_DATA will be safe, so it seems the best choice to me.

Thanks, applied to my block-next branch:

https://github.com/XanClic/qemu/commits/block-next

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH] qemu-img: improve convert_iteration_sectors()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2156 bytes --]

On 20.04.2017 01:06, John Snow wrote:
> 
> 
> On 04/07/2017 07:34 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Do not do extra call to _get_block_status()
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>
>> Also, I'm not sure about last line:
>> s->status = s->target_has_backing ? BLK_BACKING_FILE : BLK_DATA;
>>
>> (which is equal to old code)
>>
> 
> What a weird function.
> 
> So, if the target has a backing file, literally nothing changes here.
> 
> If it doesn't, we skip the initial call to get_block_status and just
> call the (effectively?) recursive version to find out if we have a ZERO
> or DATA type of allocation.
> 
> The else clause here properly reflects the original reading of the code.
> 
> OK.

Well, this is what happens when optimizing parts of something without
looking at the bigger picture (263a6f4c3aa).

>> may be, it should be
>> s->status = s->target_has_backing ? BLK_BACKING_FILE : BLK_ZERO;
>>
>> as it is the case, when range is not allocated at all. Should we copy it in this case?
>>
> 
> I am not really clear on if either ZERO or DATA are correct in the case
> where we do not have a backing file, because maybe this depends on
> has_zero_init?
> 
> If we are copying uninitialized data when has_zero_init is true on the
> source, we want zeroes on the target. If the target also has_zero_init,
> we can just skip this sector. If it doesn't, we want to copy zeroes.
> 
> If has_zero_init is false and we have unallocated data on the source...
> what are we doing? Copying random debris?

Probably. Sounds fine to me, though. Because if you were to read from
the source I'd expect you'd read random debris, so it's fine to write
that to the destination, too.

Alternatively, we might want to add a new status BLK_UNALLOCATED which
would simply skip both reading and writing these areas.

Alas! my craving does not suffice to compel my lowly self to excogitate
a patch.

Max

> I'm pretty confused, but I'm pretty sure your patch doesn't actually
> change anything, so:
> 
> Reviewed-by: John Snow <jsnow@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]