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

[Stefan Hajnoczi]

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

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);
    ...
}

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
this function returns an error?