[Qemu-devel] [PATCH v4 0/1] qemu-img: check block status of backing file when converting.

[Qemu-devel] [PATCH v4 0/1] qemu-img: check block status of backing file when converting.

[Ren Kimura]

I've just changed last patch to new one that uses loop iteration instead of recursion. https://lists.gnu.org/archive/html/qemu-block/2016-04/msg00584.html

At first, a head of chain has checked it's status by bdrv_get_block_status in "convert_iteration_sectors".
If this status is not BDRV_BLOCK_DATA nor BDRV_BLOCK_ZERO, then check backing files by using new function, get_backing_status.
This function iterates backing files chain and return the status BDRV_BLOCK_DATA, BDRV_BLOCK_ZERO or error(< 0) caused by bdrv_get_block_status.
When none of backing files has valid status(i.e. backing file which must have data doesn't exist), then return -1(< 0) and goto fail.

[Qemu-devel] [PATCH v4 1/1] qemu-img: check block status of backing file when converting.

[Ren Kimura]

When converting images, check the block status of its backing file chain
to avoid needlessly reading zeros.

Signed-off-by: Ren Kimura <rkx1209dev@gmail.com>
---
 qemu-img.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 06264d9..b771227 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1451,6 +1451,22 @@ static void convert_select_part(ImgConvertState *s, int64_t sector_num)
     }
 }
 
+static int64_t get_backing_status(BlockDriverState *bs,
+                                  int64_t sector_num,
+                                  int nb_sectors, int *pnum)
+{
+    while (bs->backing) {
+        int64_t ret;
+        BlockDriverState *file;
+        bs = bs->backing->bs;
+        ret = bdrv_get_block_status(bs, sector_num, nb_sectors, pnum, &file);
+        if (ret < 0 || ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) {
+            return ret;
+        }
+    }
+    return -1;
+}
+
 static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
 {
     int64_t ret;
@@ -1477,10 +1493,21 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
         } else if (!s->target_has_backing) {
             /* Without a target backing file we must copy over the contents of
              * the backing file as well. */
-            /* TODO Check block status of the backing file chain to avoid
+            /* Check block status of the backing file chain to avoid
              * needlessly reading zeroes and limiting the iteration to the
              * buffer size */
-            s->status = BLK_DATA;
+            ret = get_backing_status(blk_bs(s->src[s->src_cur]),
+                                     sector_num - s->src_cur_offset,
+                                     n, &n);
+            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;
         }
-- 
2.5.0

Re: [Qemu-devel] [PATCH v4 1/1] qemu-img: check block status of backing file when converting.

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 3149 bytes --]

On 23.04.2016 08:26, Ren Kimura wrote:
> When converting images, check the block status of its backing file chain
> to avoid needlessly reading zeros.
> 
> Signed-off-by: Ren Kimura <rkx1209dev@gmail.com>
> ---
>  qemu-img.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 06264d9..b771227 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1451,6 +1451,22 @@ static void convert_select_part(ImgConvertState *s, int64_t sector_num)
>      }
>  }
>  
> +static int64_t get_backing_status(BlockDriverState *bs,
> +                                  int64_t sector_num,
> +                                  int nb_sectors, int *pnum)
> +{
> +    while (bs->backing) {
> +        int64_t ret;
> +        BlockDriverState *file;
> +        bs = bs->backing->bs;
> +        ret = bdrv_get_block_status(bs, sector_num, nb_sectors, pnum, &file);
> +        if (ret < 0 || ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) {
> +            return ret;
> +        }

Denis' suggestion was to basically drop this function and call
bdrv_get_block_status_above() instead.

I wouldn't strictly oppose keeping this function because of the
BDRV_BLOCK_{DATA,ZERO} vs. BDRV_BLOCK_ALLOCATED issue I talked about before.

One thing we can definitely learn from bdrv_get_block_status_above(),
though, is that a

nb_sectors = MIN(nb_sectors, *pnum);

is missing here (see bdrv_co_get_block_status_above()).

(Sorry for not noticing this before)

> +    }
> +    return -1;
> +}
> +
>  static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
>  {
>      int64_t ret;
> @@ -1477,10 +1493,21 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
>          } else if (!s->target_has_backing) {
>              /* Without a target backing file we must copy over the contents of
>               * the backing file as well. */
> -            /* TODO Check block status of the backing file chain to avoid
> +            /* Check block status of the backing file chain to avoid
>               * needlessly reading zeroes and limiting the iteration to the
>               * buffer size */
> -            s->status = BLK_DATA;
> +            ret = get_backing_status(blk_bs(s->src[s->src_cur]),
> +                                     sector_num - s->src_cur_offset,
> +                                     n, &n);

In any case, I think I agree with Denis that just calling

ret = bdrv_get_block_status_above(blk_bs(s->src[s->src_cur]), NULL,
                                  sector_num - s->src_cur_offset, n, &n,
                                  &file);

would be sufficient.

(Sorry for not noticing this either...)

Max

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



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

Re: [Qemu-devel] [PATCH v4 1/1] qemu-img: check block status of backing file when converting.

[Ren Kimura]

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

Aha. I realized what bdrv_get_block_status_above(bs, NULL...) meant.
Sorry for many times resendings.
I'll fix it next time then.

Ren

[-- Attachment #2: Type: text/html, Size: 497 bytes --]