Past the end of the source backing file, we memset() buf_old to zero, so
it is clearly easy to use blk_pwrite_zeroes() instead of blk_pwrite()
then.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
qemu-img.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index dd684d8bf0..2552e7dad6 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3403,6 +3403,8 @@ static int img_rebase(int argc, char **argv)
}
for (offset = 0; offset < size; offset += n) {
+ bool buf_old_is_zero = false;
+
/* How many bytes can we handle with the next read? */
n = MIN(IO_BUF_SIZE, size - offset);
@@ -3423,6 +3425,7 @@ static int img_rebase(int argc, char **argv)
*/
if (offset >= old_backing_size) {
memset(buf_old, 0, n);
+ buf_old_is_zero = true;
} else {
if (offset + n > old_backing_size) {
n = old_backing_size - offset;
@@ -3458,8 +3461,12 @@ static int img_rebase(int argc, char **argv)
if (compare_buffers(buf_old + written, buf_new + written,
n - written, &pnum))
{
- ret = blk_pwrite(blk, offset + written,
- buf_old + written, pnum, 0);
+ if (buf_old_is_zero) {
+ ret = blk_pwrite_zeroes(blk, offset + written, pnum, 0);
+ } else {
+ ret = blk_pwrite(blk, offset + written,
+ buf_old + written, pnum, 0);
+ }
if (ret < 0) {
error_report("Error while writing to COW image: %s",
strerror(-ret));
--
2.17.1
On 07/13/2018 06:14 AM, Max Reitz wrote:
> Past the end of the source backing file, we memset() buf_old to zero, so
> it is clearly easy to use blk_pwrite_zeroes() instead of blk_pwrite()
> then.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> qemu-img.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index dd684d8bf0..2552e7dad6 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3403,6 +3403,8 @@ static int img_rebase(int argc, char **argv)
> }
>
> for (offset = 0; offset < size; offset += n) {
> + bool buf_old_is_zero = false;
> +
> /* How many bytes can we handle with the next read? */
> n = MIN(IO_BUF_SIZE, size - offset);
>
> @@ -3423,6 +3425,7 @@ static int img_rebase(int argc, char **argv)
> */
> if (offset >= old_backing_size) {
> memset(buf_old, 0, n);
> + buf_old_is_zero = true;
Do we still need to spend time on the memset(), or...
> } else {
> if (offset + n > old_backing_size) {
> n = old_backing_size - offset;
> @@ -3458,8 +3461,12 @@ static int img_rebase(int argc, char **argv)
> if (compare_buffers(buf_old + written, buf_new + written,
> n - written, &pnum))
> {
> - ret = blk_pwrite(blk, offset + written,
> - buf_old + written, pnum, 0);
> + if (buf_old_is_zero) {
> + ret = blk_pwrite_zeroes(blk, offset + written, pnum, 0);
...are we able to guarantee that old_buf will not be used when
buf_old_is_zero?
> + } else {
> + ret = blk_pwrite(blk, offset + written,
> + buf_old + written, pnum, 0);
> + }
> if (ret < 0) {
> error_report("Error while writing to COW image: %s",
> strerror(-ret));
>
The series seems reasonable, but is post-3.0 material, so I haven't
reviewed it any closer than this question.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
On 2018-07-20 23:22, Eric Blake wrote:
> On 07/13/2018 06:14 AM, Max Reitz wrote:
>> Past the end of the source backing file, we memset() buf_old to zero, so
>> it is clearly easy to use blk_pwrite_zeroes() instead of blk_pwrite()
>> then.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> qemu-img.c | 11 +++++++++--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index dd684d8bf0..2552e7dad6 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -3403,6 +3403,8 @@ static int img_rebase(int argc, char **argv)
>> }
>> for (offset = 0; offset < size; offset += n) {
>> + bool buf_old_is_zero = false;
>> +
>> /* How many bytes can we handle with the next read? */
>> n = MIN(IO_BUF_SIZE, size - offset);
>> @@ -3423,6 +3425,7 @@ static int img_rebase(int argc, char **argv)
>> */
>> if (offset >= old_backing_size) {
>> memset(buf_old, 0, n);
>> + buf_old_is_zero = true;
>
> Do we still need to spend time on the memset(), or...
>
>> } else {
>> if (offset + n > old_backing_size) {
>> n = old_backing_size - offset;
>> @@ -3458,8 +3461,12 @@ static int img_rebase(int argc, char **argv)
>> if (compare_buffers(buf_old + written, buf_new +
>> written,
>> n - written, &pnum))
>> {
>> - ret = blk_pwrite(blk, offset + written,
>> - buf_old + written, pnum, 0);
>> + if (buf_old_is_zero) {
>> + ret = blk_pwrite_zeroes(blk, offset +
>> written, pnum, 0);
>
> ...are we able to guarantee that old_buf will not be used when
> buf_old_is_zero?
It will certainly be used, as it is used in the compare_buffers() call.
It could be optimized, but I fear that may lead down a small rabbit hole
(we could then also get the block status of the target backing file, see
whether it is zero, and so on). Considering that rebase is probably not
something many people use all the time, I think it’s OK to be slower
than possible for now. (Until someone complains.)
Max
>> + } else {
>> + ret = blk_pwrite(blk, offset + written,
>> + buf_old + written, pnum, 0);
>> + }
>> if (ret < 0) {
>> error_report("Error while writing to COW
>> image: %s",
>> strerror(-ret));
>>
>
> The series seems reasonable, but is post-3.0 material, so I haven't
> reviewed it any closer than this question.
>
On 7/13/18 6:14 AM, Max Reitz wrote:
> Past the end of the source backing file, we memset() buf_old to zero, so
> it is clearly easy to use blk_pwrite_zeroes() instead of blk_pwrite()
> then.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> qemu-img.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> @@ -3458,8 +3461,12 @@ static int img_rebase(int argc, char **argv)
> if (compare_buffers(buf_old + written, buf_new + written,
> n - written, &pnum))
> {
> - ret = blk_pwrite(blk, offset + written,
> - buf_old + written, pnum, 0);
> + if (buf_old_is_zero) {
> + ret = blk_pwrite_zeroes(blk, offset + written, pnum, 0);
Should we allow BDRV_REQ_MAY_UNMAP here, either unconditionally, or
based on a command line knob that told us whether the user is more
interested in a sparse result (that still reads as zero) vs. a
fully-allocated result?
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 18.04.19 20:59, Eric Blake wrote:
> On 7/13/18 6:14 AM, Max Reitz wrote:
>> Past the end of the source backing file, we memset() buf_old to zero, so
>> it is clearly easy to use blk_pwrite_zeroes() instead of blk_pwrite()
>> then.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> qemu-img.c | 11 +++++++++--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>
>> @@ -3458,8 +3461,12 @@ static int img_rebase(int argc, char **argv)
>> if (compare_buffers(buf_old + written, buf_new + written,
>> n - written, &pnum))
>> {
>> - ret = blk_pwrite(blk, offset + written,
>> - buf_old + written, pnum, 0);
>> + if (buf_old_is_zero) {
>> + ret = blk_pwrite_zeroes(blk, offset + written, pnum, 0);
>
> Should we allow BDRV_REQ_MAY_UNMAP here, either unconditionally, or
> based on a command line knob that told us whether the user is more
> interested in a sparse result (that still reads as zero) vs. a
> fully-allocated result?
I wouldn’t trust that. We haven’t yet switched to the new backing file
at this point, so I think a driver would be correct in handling such
requests by punching a hole in the file -- which would lead to the new
backing file poking through once we’ve switched to it.
Max
> Reviewed-by: Eric Blake <eblake@redhat.com>
© 2016 - 2025 Red Hat, Inc.