On Thu, Apr 27, 2017 at 4:25 AM, Max Reitz <mre...@redhat.com> wrote:
> On 21.04.2017 11:57, jemmy858...@gmail.com wrote:
>> From: Lidong Chen <lidongc...@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 <lidongc...@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,
>>
>
>
