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