Re: [Qemu-devel] [PATCH 2/3] qemu-img: Use zero writes after source backing EOF

[Max Reitz]

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 <address@hidden>
>> ---
>>   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.
>

signature.asc
Description: OpenPGP digital signature