On Tue, Oct 13, 2015 at 05:13:14PM +0800, Wen Congyang wrote: > 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 <we...@cn.fujitsu.com> > >> Signed-off-by: zhanghailiang <zhang.zhanghaili...@huawei.com> > >> Signed-off-by: Gonglei <arei.gong...@huawei.com> > >> Reviewed-by: Jeff Cody <jc...@redhat.com> > >> --- > >> 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.
It's a question of taste. In this case it seems to me that there is really a direct coupling between COLO and the backup block job. This isn't really a generic interface that makes sense in other scenarios. That's why I advocate for direct coupling instead of pretending this is a generic interface. I wish COLO could just stop the existing block job and start a new one for each checkpoint. In reality we probably don't want QMP events and the full block job lifecycle for each checkpoint... But anyway, I like this approach because it does not require a new interface at all. > > 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. Okay, I ask these questions because the information should be part of the doc comment for this new interface. Stefan