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

[858585 jemmy]

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?

>
>>
>> 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