From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58682) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZlveG-0002dU-1i for qemu-devel@nongnu.org; Tue, 13 Oct 2015 05:13:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZlveF-0007e7-26 for qemu-devel@nongnu.org; Tue, 13 Oct 2015 05:13:31 -0400 References: <1443161858-20533-1-git-send-email-wency@cn.fujitsu.com> <1443161858-20533-3-git-send-email-wency@cn.fujitsu.com> <20151012134543.GA4053@stefanha-thinkpad.redhat.com> From: Wen Congyang Message-ID: <561CCB2A.3080501@cn.fujitsu.com> Date: Tue, 13 Oct 2015 17:13:14 +0800 MIME-Version: 1.0 In-Reply-To: <20151012134543.GA4053@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Qemu-block] [PATCH v10 02/10] Backup: clear all bitmap when doing block checkpoint List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi , Paolo Bonzini Cc: Kevin Wolf , Fam Zheng , zhanghailiang , qemu block , Jiang Yunhong , Dong Eddie , qemu devel , "Michael R. Hines" , Max Reitz , Gonglei , Stefan Hajnoczi , Yang Hongyang , "Dr. David Alan Gilbert" On 10/12/2015 09:45 PM, Stefan Hajnoczi wrote: > On Fri, Sep 25, 2015 at 02:17:30PM +0800, Wen Congyang wrote: >> Signed-off-by: Wen Congyang >> Signed-off-by: zhanghailiang >> Signed-off-by: Gonglei >> Reviewed-by: Jeff Cody >> --- >> block/backup.c | 14 ++++++++++++++ >> blockjob.c | 11 +++++++++++ >> include/block/blockjob.h | 12 ++++++++++++ >> 3 files changed, 37 insertions(+) >> >> diff --git a/block/backup.c b/block/backup.c >> index c61e4c3..5e5995e 100644 >> --- a/block/backup.c >> +++ b/block/backup.c >> @@ -214,11 +214,25 @@ static void backup_iostatus_reset(BlockJob *job) >> } >> } >> >> +static void backup_do_checkpoint(BlockJob *job, Error **errp) >> +{ >> + BackupBlockJob *backup_job = container_of(job, BackupBlockJob, common); >> + >> + if (backup_job->sync_mode != MIRROR_SYNC_MODE_NONE) { >> + error_setg(errp, "The backup job only supports block checkpoint in" >> + " sync=none mode"); >> + return; >> + } >> + >> + hbitmap_reset_all(backup_job->bitmap); >> +} > > Is this a fast way to stop and then start a new backup blockjob without > emitting block job lifecycle events? > > Not sure the blockjob_do_checkpoint() interface is appropriate. Is > there any other block job type that will implement .do_checkpoint()? Currently, the answer is no. > > COLO block replication could call a public backup_do_checkpoint() > function. That way the direct coupling between COLO and the backup > block job is obvious. I'm not convinced a generic interface like > blockjob_do_checkpoint() makes sense since it's really not a generic > operation that makes sense for other block job types. > > void backup_do_checkpoint(BlockJob *job, Error **errp) > { > BackupBlockJob *s; > > if (job->driver != backup_job_driver) { > error_setg(errp, "expected backup block job type for " > "checkpoint, got %d", job->driver->job_type); > return; > } > > s = container_of(job, BackupBlockJob, common); > ... > } In a older version, I implement it like this, but Paolo didn't like it. > > Please also make the function name and documentation more specific. > Instead of "do" maybe this should be "pre" or "post" to indicate whether > this happens before or after the checkpoint commit. What happens if OK > this function returns an error? We just return this error to COLO, and COLO will do failover. Thanks Wen Congyang > . >