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

jemmy858585@gmail.com posted 1 patch 6 years, 11 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1492768654-11053-1-git-send-email-lidongchen@tencent.com
Test checkpatch passed
Test docker passed
Test s390x passed
There is a newer version of this series
qemu-img.c | 41 +++++++++++------------------------------
1 file changed, 11 insertions(+), 30 deletions(-)
[Qemu-devel] [PATCH v2] qemu-img: use blk_co_pwrite_zeroes for zero sectors when compressed
Posted by jemmy858585@gmail.com 6 years, 11 months ago
From: Lidong Chen <lidongchen@tencent.com>

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 <lidongchen@tencent.com>
---
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


Re: [Qemu-devel] [PATCH v2] qemu-img: use blk_co_pwrite_zeroes for zero sectors when compressed
Posted by 858585 jemmy 6 years, 11 months ago
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,  <jemmy858585@gmail.com> wrote:
> From: Lidong Chen <lidongchen@tencent.com>
>
> 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 <lidongchen@tencent.com>
> ---
> 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
>

Re: [Qemu-devel] [PATCH v2] qemu-img: use blk_co_pwrite_zeroes for zero sectors when compressed
Posted by 858585 jemmy 6 years, 11 months ago
Hi Everyone:
    Any suggestion about this patch?
    Thanks.

On Sun, Apr 23, 2017 at 5:53 PM, 858585 jemmy <jemmy858585@gmail.com> wrote:
> 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,  <jemmy858585@gmail.com> wrote:
>> From: Lidong Chen <lidongchen@tencent.com>
>>
>> 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 <lidongchen@tencent.com>
>> ---
>> 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
>>

Re: [Qemu-devel] [PATCH v2] qemu-img: use blk_co_pwrite_zeroes for zero sectors when compressed
Posted by Max Reitz 6 years, 11 months ago
On 21.04.2017 11:57, jemmy858585@gmail.com wrote:
> From: Lidong Chen <lidongchen@tencent.com>
> 
> when the buffer is zero, blk_co_pwrite_zeroes is more effectively than

s/when/When/, s/effectively/effective/

> blk_co_pwritev with BDRV_REQ_WRITE_COMPRESSED. this patch can reduces

s/this/This/, s/reduces/reduce/

> the time when converts the qcow2 image with lots of zero.

s/when converts the qcow2 image/for converting qcow2 images/,
s/zero/zero data/

> 
> Signed-off-by: Lidong Chen <lidongchen@tencent.com>
> ---
> v2 changelog:
>     unify the compressed and non-compressed code paths
> ---
>  qemu-img.c | 41 +++++++++++------------------------------
>  1 file changed, 11 insertions(+), 30 deletions(-)

Functionally, looks good to me. Just some stylistic nit picks:

> 
> 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. */

I think we should still mention why there is a difference depending on
s->compressed. Maybe like this:

/* 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.
 * Compressed clusters need to be written as a whole, so in that
 * case we can only save the write if the buffer is completely
 * zeroed. */

>              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))) {
> +

This newline is a bit weird. Normally we don't have newlines at the
start of a block.

If you (like me) think there should be a visual separation between the
if condition and the block, I'd suggest keeping the opening brace { on
its own line (as it is now).

Max

>                  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,
> 


Re: [Qemu-devel] [PATCH v2] qemu-img: use blk_co_pwrite_zeroes for zero sectors when compressed
Posted by 858585 jemmy 6 years, 11 months ago
On Thu, Apr 27, 2017 at 4:25 AM, Max Reitz <mreitz@redhat.com> wrote:
> On 21.04.2017 11:57, jemmy858585@gmail.com wrote:
>> From: Lidong Chen <lidongchen@tencent.com>
>>
>> when the buffer is zero, blk_co_pwrite_zeroes is more effectively than
>
> s/when/When/, s/effectively/effective/
>
>> blk_co_pwritev with BDRV_REQ_WRITE_COMPRESSED. this patch can reduces
>
> s/this/This/, s/reduces/reduce/
>
>> the time when converts the qcow2 image with lots of zero.
>
> s/when converts the qcow2 image/for converting qcow2 images/,
> s/zero/zero data/
>
>>
>> Signed-off-by: Lidong Chen <lidongchen@tencent.com>
>> ---
>> v2 changelog:
>>     unify the compressed and non-compressed code paths
>> ---
>>  qemu-img.c | 41 +++++++++++------------------------------
>>  1 file changed, 11 insertions(+), 30 deletions(-)
>
> Functionally, looks good to me. Just some stylistic nit picks:
>
>>
>> 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. */
>
> I think we should still mention why there is a difference depending on
> s->compressed. Maybe like this:
>
> /* 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.
>  * Compressed clusters need to be written as a whole, so in that
>  * case we can only save the write if the buffer is completely
>  * zeroed. */
>
>>              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))) {
>> +
>
> This newline is a bit weird. Normally we don't have newlines at the
> start of a block.
>
> If you (like me) think there should be a visual separation between the
> if condition and the block, I'd suggest keeping the opening brace { on
> its own line (as it is now).
>
Thanks for your review.

> Max
>
>>                  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,
>>
>
>