qemu-img.c | 41 +++++++++++------------------------------ 1 file changed, 11 insertions(+), 30 deletions(-)
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
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 >
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 >>
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, >
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, >> > >
© 2016 - 2024 Red Hat, Inc.