Re: [Qemu-block] [PATCH v10 02/10] Backup: clear all bitmap when doing block checkpoint

[Stefan Hajnoczi]

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 <address@hidden>
> >> Signed-off-by: zhanghailiang <address@hidden>
> >> Signed-off-by: Gonglei <address@hidden>
> >> Reviewed-by: Jeff Cody <address@hidden>
> >> ---
> >>  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