From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:46305)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <wency@cn.fujitsu.com>) id 1Yaw63-00037R-I5
	for qemu-devel@nongnu.org; Wed, 25 Mar 2015 20:56:32 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <wency@cn.fujitsu.com>) id 1Yaw5y-0007ko-J0
	for qemu-devel@nongnu.org; Wed, 25 Mar 2015 20:56:31 -0400
Message-ID: <551359EE.9000504@cn.fujitsu.com>
Date: Thu, 26 Mar 2015 08:59:26 +0800
From: Wen Congyang <wency@cn.fujitsu.com>
MIME-Version: 1.0
References: <1427276174-9130-1-git-send-email-wency@cn.fujitsu.com>
	<1427276174-9130-11-git-send-email-wency@cn.fujitsu.com>
	<5512B03B.5040502@redhat.com>
In-Reply-To: <5512B03B.5040502@redhat.com>
Content-Type: text/plain; charset="windows-1252"
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [RFC PATCH COLO v2 10/13] Backup: clear all bitmap
 when doing block checkpoint
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Paolo Bonzini <pbonzini@redhat.com>, qemu devel <qemu-devel@nongnu.org>, Fam Zheng <famz@redhat.com>, Max Reitz <mreitz@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>, Lai Jiangshan <laijs@cn.fujitsu.com>, qemu block <qemu-block@nongnu.org>, Jeff Cody <jcody@redhat.com>, Jiang Yunhong <yunhong.jiang@intel.com>, Dong Eddie <eddie.dong@intel.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Gonglei <arei.gonglei@huawei.com>, Stefan Hajnoczi <stefanha@redhat.com>, Yang Hongyang <yanghy@cn.fujitsu.com>, zhanghailiang <zhang.zhanghailiang@huawei.com>

On 03/25/2015 08:55 PM, Paolo Bonzini wrote:
> 
> 
> On 25/03/2015 10:36, Wen Congyang wrote:
>>
>> +void backup_do_checkpoint(BlockJob *job, Error **errp)
>> +{
>> +    BackupBlockJob *backup_job = container_of(job, BackupBlockJob, common);
>> +
>> +    if (job->driver != &backup_job_driver) {
>> +        error_setg(errp, "It is not backup job");
>> +        return;
>> +    }
>> +
>> +    hbitmap_reset_all(backup_job->bitmap);
>> +}
> 
> Please add instead a block_job_do_checkpoint API, and a do_checkpoint
> function pointer to BlockJobDriver.
> 
>> +{
>> +#if 0
>> +    hbitmap_reset(hb, 0, hb->size << hb->granularity);
>> +#else
>> +    uint64_t size = hb->size;
>> +    unsigned int i;
>> +
>> +    /* Same as hbitmap_alloc() except memset() */
>> +    for (i = HBITMAP_LEVELS; i-- > 0; ) {
> 
> This can be "--i >= 1"...
> 
>> +        size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
>> +        memset(hb->levels[i], 0, size * sizeof(unsigned long));
>> +    }
>> +
>> +    assert(size == 1);
>> +    hb->levels[0][0] |= 1UL << (BITS_PER_LONG - 1);
> 
> ... if you use "=" instead of "|=" here.
> 
>> +#endif
>> +}
> 
> Please pick one implementation (no #if), and also add a testcase to
> tests/test-hbitmap.c.

OK

Thanks
Wen Congyang

> 
> Paolo
> .
>