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).
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,
>
signature.asc
Description: OpenPGP digital signature
