qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v2] qemu-img: use blk_co_pwrite_zeroes for zero sectors when compressed
@ 2017-04-21  9:57 jemmy858585
  2017-04-23  9:53 ` 858585 jemmy
  2017-04-26 20:25 ` Max Reitz
  0 siblings, 2 replies; 5+ messages in thread
From: jemmy858585 @ 2017-04-21  9:57 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, mreitz, qemu-block, Lidong Chen

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

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [Qemu-devel] [PATCH v2] qemu-img: use blk_co_pwrite_zeroes for zero sectors when compressed
  2017-04-21  9:57 [Qemu-devel] [PATCH v2] qemu-img: use blk_co_pwrite_zeroes for zero sectors when compressed jemmy858585
@ 2017-04-23  9:53 ` 858585 jemmy
  2017-04-24 13:17   ` 858585 jemmy
  2017-04-26 20:25 ` Max Reitz
  1 sibling, 1 reply; 5+ messages in thread
From: 858585 jemmy @ 2017-04-23  9:53 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, mreitz, qemu block, Lidong Chen

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
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Qemu-devel] [PATCH v2] qemu-img: use blk_co_pwrite_zeroes for zero sectors when compressed
  2017-04-23  9:53 ` 858585 jemmy
@ 2017-04-24 13:17   ` 858585 jemmy
  0 siblings, 0 replies; 5+ messages in thread
From: 858585 jemmy @ 2017-04-24 13:17 UTC (permalink / raw)
  To: qemu-devel, Kevin Wolf, mreitz; +Cc: qemu block, Lidong Chen

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Qemu-devel] [PATCH v2] qemu-img: use blk_co_pwrite_zeroes for zero sectors when compressed
  2017-04-21  9:57 [Qemu-devel] [PATCH v2] qemu-img: use blk_co_pwrite_zeroes for zero sectors when compressed jemmy858585
  2017-04-23  9:53 ` 858585 jemmy
@ 2017-04-26 20:25 ` Max Reitz
  2017-04-27  3:03   ` 858585 jemmy
  1 sibling, 1 reply; 5+ messages in thread
From: Max Reitz @ 2017-04-26 20:25 UTC (permalink / raw)
  To: jemmy858585, qemu-devel; +Cc: kwolf, qemu-block, Lidong Chen

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

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



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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Qemu-devel] [PATCH v2] qemu-img: use blk_co_pwrite_zeroes for zero sectors when compressed
  2017-04-26 20:25 ` Max Reitz
@ 2017-04-27  3:03   ` 858585 jemmy
  0 siblings, 0 replies; 5+ messages in thread
From: 858585 jemmy @ 2017-04-27  3:03 UTC (permalink / raw)
  To: Max Reitz; +Cc: qemu-devel, Kevin Wolf, qemu block, Lidong Chen

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2017-04-27  3:03 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-04-21  9:57 [Qemu-devel] [PATCH v2] qemu-img: use blk_co_pwrite_zeroes for zero sectors when compressed jemmy858585
2017-04-23  9:53 ` 858585 jemmy
2017-04-24 13:17   ` 858585 jemmy
2017-04-26 20:25 ` Max Reitz
2017-04-27  3:03   ` 858585 jemmy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).