On Fri, Apr 21, 2017 at 1:37 PM, 858585 jemmy <jemmy858...@gmail.com> wrote:
> On Fri, Apr 21, 2017 at 10:58 AM, 858585 jemmy <jemmy858...@gmail.com> wrote:
>> On Thu, Apr 20, 2017 at 6:00 PM, Kevin Wolf <kw...@redhat.com> wrote:
>>> Am 20.04.2017 um 10:38 hat jemmy858...@gmail.com geschrieben:
>>>> From: Lidong Chen <lidongc...@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 <lidongc...@tencent.com>
>>>
>>> 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
