Re: [Qemu-devel] [PATCH v2] qemu-img: use blk_co_pwrite_zeroes for zero sectors when compressed

[858585 jemmy]

I test four test case for this patch.
1.qcow2 image with lots of zero cluster, and convert with compress
2.qcow2 image with lots of zero cluster, and convert with no compress
3.qcow2 image with lots of non-zero clusters, and convert with compress
4.qcow2 image with lots of non-zero clusters, and convert with no compress
all test case pass.

the qcow2 image with lots of zero clusters, the time reduce obviously.
and have no bad effort in other cases.


On Fri, Apr 21, 2017 at 5:57 PM,  <address@hidden> wrote:
> From: Lidong Chen <address@hidden>
>
> when the buffer is zero, blk_co_pwrite_zeroes is more effectively than
> blk_co_pwritev with BDRV_REQ_WRITE_COMPRESSED. this patch can reduces
> the time when converts the qcow2 image with lots of zero.
>
> Signed-off-by: Lidong Chen <address@hidden>
> ---
> v2 changelog:
>     unify the compressed and non-compressed code paths
> ---
>  qemu-img.c | 41 +++++++++++------------------------------
>  1 file changed, 11 insertions(+), 30 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index b220cf7..60c9adf 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1661,6 +1661,8 @@ static int coroutine_fn 
> convert_co_write(ImgConvertState *s, int64_t sector_num,
>
>      while (nb_sectors > 0) {
>          int n = nb_sectors;
> +        BdrvRequestFlags flags = s->compressed ? BDRV_REQ_WRITE_COMPRESSED : 
> 0;
> +
>          switch (status) {
>          case BLK_BACKING_FILE:
>              /* If we have a backing file, leave clusters unallocated that are
> @@ -1670,43 +1672,21 @@ static int coroutine_fn 
> convert_co_write(ImgConvertState *s, int64_t sector_num,
>              break;
>
>          case BLK_DATA:
> -            /* We must always write compressed clusters as a whole, so don't
> -             * try to find zeroed parts in the buffer. We can only save the
> -             * write if the buffer is completely zeroed and we're allowed to
> -             * keep the target sparse. */
> -            if (s->compressed) {
> -                if (s->has_zero_init && s->min_sparse &&
> -                    buffer_is_zero(buf, n * BDRV_SECTOR_SIZE))
> -                {
> -                    assert(!s->target_has_backing);
> -                    break;
> -                }
> -
> -                iov.iov_base = buf;
> -                iov.iov_len = n << BDRV_SECTOR_BITS;
> -                qemu_iovec_init_external(&qiov, &iov, 1);
> -
> -                ret = blk_co_pwritev(s->target, sector_num << 
> BDRV_SECTOR_BITS,
> -                                     n << BDRV_SECTOR_BITS, &qiov,
> -                                     BDRV_REQ_WRITE_COMPRESSED);
> -                if (ret < 0) {
> -                    return ret;
> -                }
> -                break;
> -            }
> -
> -            /* If there is real non-zero data or we're told to keep the 
> target
> -             * fully allocated (-S 0), we must write it. Otherwise we can 
> treat
> +            /* If we're told to keep the target fully allocated (-S 0) or 
> there
> +             * is real non-zero data, we must write it. Otherwise we can 
> treat
>               * it as zero sectors. */
>              if (!s->min_sparse ||
> -                is_allocated_sectors_min(buf, n, &n, s->min_sparse))
> -            {
> +                (!s->compressed &&
> +                 is_allocated_sectors_min(buf, n, &n, s->min_sparse)) ||
> +                (s->compressed &&
> +                 !buffer_is_zero(buf, n * BDRV_SECTOR_SIZE))) {
> +
>                  iov.iov_base = buf;
>                  iov.iov_len = n << BDRV_SECTOR_BITS;
>                  qemu_iovec_init_external(&qiov, &iov, 1);
>
>                  ret = blk_co_pwritev(s->target, sector_num << 
> BDRV_SECTOR_BITS,
> -                                     n << BDRV_SECTOR_BITS, &qiov, 0);
> +                                     n << BDRV_SECTOR_BITS, &qiov, flags);
>                  if (ret < 0) {
>                      return ret;
>                  }
> @@ -1716,6 +1696,7 @@ static int coroutine_fn 
> convert_co_write(ImgConvertState *s, int64_t sector_num,
>
>          case BLK_ZERO:
>              if (s->has_zero_init) {
> +                assert(!s->target_has_backing);
>                  break;
>              }
>              ret = blk_co_pwrite_zeroes(s->target,
> --
> 1.8.3.1
>