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

[858585 jemmy]

Hi Everyone:
    Any suggestion about this patch?
    Thanks.

On Sun, Apr 23, 2017 at 5:53 PM, 858585 jemmy <address@hidden> 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,  <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
>>