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