Re: [Qemu-block] [PATCH] qemu-img: use blk_co_pwrite_zeroes for zero sectors when compressed

[858585 jemmy]

On Fri, Apr 21, 2017 at 1:37 PM, 858585 jemmy <address@hidden> wrote:
> On Fri, Apr 21, 2017 at 10:58 AM, 858585 jemmy <address@hidden> wrote:
>> On Thu, Apr 20, 2017 at 6:00 PM, Kevin Wolf <address@hidden> wrote:
>>> Am 20.04.2017 um 10:38 hat address@hidden geschrieben:
>>>> From: Lidong Chen <address@hidden>
>>>>
>>>> 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 <address@hidden>
>>>
>>> Good catch, using blk_co_pwrite_zeroes() makes sense even for compressed
>>> images.
>>>
>>>> diff --git a/qemu-img.c b/qemu-img.c
>>>> index b220cf7..0256539 100644
>>>> --- a/qemu-img.c
>>>> +++ b/qemu-img.c
>>>> @@ -1675,13 +1675,20 @@ static int coroutine_fn 
>>>> convert_co_write(ImgConvertState *s, int64_t sector_num,
>>>>               * 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;
>>>> +                if (buffer_is_zero(buf, n * BDRV_SECTOR_SIZE)) {
>>>> +                    if (s->has_zero_init && s->min_sparse) {
>>>> +                        assert(!s->target_has_backing);
>>>> +                        break;
>>>> +                    } else {
>>>> +                        ret = blk_co_pwrite_zeroes(s->target,
>>>> +                                           sector_num << BDRV_SECTOR_BITS,
>>>> +                                           n << BDRV_SECTOR_BITS, 0);
>>>> +                        if (ret < 0) {
>>>> +                            return ret;
>>>> +                        }
>>>> +                        break;
>>>> +                    }
>>>>                  }
>>>
>>> If s->min_sparse == 0, we may neither skip the write not use
>>> blk_co_pwrite_zeroes(), because this requires actual full allocation
>>> with explicit zero sectors.
>>>
>>> Of course, if you fix this, what you end up with here is a duplicate of
>>> the code path for non-compressed images. The remaining difference seems
>>> to be the BDRV_REQ_WRITE_COMPRESSED flag and buffer_is_zero() vs.
>>> is_allocated_sectors_min() (because uncompressed clusters can be written
>>> partially, but compressed clusters can't).
>>
>> I have a try to unify the code.
>>
>> I don't understand why use  is_allocated_sectors_min when don't compressed.
>> the s->min_sparse is 8 default, which is smaller than cluster_sectors.
>>
>> if a cluster which data is  8 sector zero and 8 sector non-zero
>> repeated, it will call
>> blk_co_pwritev and blk_co_pwrite_zeroes many times for a cluster.
>>
>> why not compare the zero by cluster_sectors size?
>
> I write this code, run in guest os.
>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
>
> int main()
> {
>     char *zero;
>     char *nonzero;
>     FILE* fp = fopen("./test.dat", "ab");
>
>     zero = malloc(sizeof(char)*512*8);
>     nonzero = malloc(sizeof(char)*512*8);
>
>     memset(zero, 0, sizeof(char)*512*8);
>     memset(nonzero, 1, sizeof(char)*512*8);
>
>     while (1) {
>         fwrite(zero, sizeof(char)*512*8, 1, fp);
>         fwrite(nonzero, sizeof(char)*512*8, 1, fp);
>     }
>     fclose(fp);
> }
>
> qemu-img info /mnt/img2016111016860868.qcow2
> image: /mnt/img2016111016860868.qcow2
> file format: qcow2
> virtual size: 20G (21474836480 bytes)
> disk size: 19G (20061552640 bytes)
> cluster_size: 65536
> backing file: /baseimage/img2016042213665396/img2016042213665396.qcow2
>
> use -S 65536 option.
>
> time /root/kvm/bin/qemu-img convert -p -B
> /baseimage/img2016042213665396/img2016042213665396.qcow2 -O qcow2
> /mnt/img2016111016860868.qcow2 /mnt/img2017041611162809_zip_new.qcow2
> -S 65536
>     (100.00/100%)
>
> real    0m32.203s
> user    0m5.165s
> sys     0m27.887s
>
> time /root/kvm/bin/qemu-img convert -p -B
> /baseimage/img2016042213665396/img2016042213665396.qcow2 -O qcow2
> /mnt/img2016111016860868.qcow2 /mnt/img2017041611162809_zip_new.qcow2
>     (100.00/100%)
>
> real    1m38.665s
> user    0m45.418s
> sys     1m7.518s
>
> should we set cluster_sectors as the default value of s->min_sparse?

change the default value of s->min_sparse will break the API.
qemu-img --help describe that the default value is 4k.

  '-S' indicates the consecutive number of bytes (defaults to 4k) that must
       contain only zeros for qemu-img to create a sparse image during
       conversion. If the number of bytes is 0, the source will not be
scanned for
       unallocated or zero sectors, and the destination image will always be
       fully allocated

>
>>
>>>
>>> So I suppose that instead of just fixing the above bug, we could actually
>>> mostly unify the two code paths, if you want to have a try at it.
>>>
>>> Kevin