Re: [Qemu-block] [PATCH COLO-BLOCK v8 09/18] Backup: clear all bitmap when doing block checkpoint

[Dr. David Alan Gilbert]

* Wen Congyang (address@hidden) wrote:
> Signed-off-by: Wen Congyang <address@hidden>
> Signed-off-by: zhanghailiang <address@hidden>
> Signed-off-by: Gonglei <address@hidden>
> Cc: Jeff Cody <address@hidden>
> ---
>  block/backup.c           | 13 +++++++++++++
>  blockjob.c               | 10 ++++++++++
>  include/block/blockjob.h | 12 ++++++++++++
>  3 files changed, 35 insertions(+)
> 
> diff --git a/block/backup.c b/block/backup.c
> index d3c7d9f..ebb8a88 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -211,11 +211,24 @@ static void backup_iostatus_reset(BlockJob *job)
>      bdrv_iostatus_reset(s->target);
>  }
>  
> +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, "this feature or command is not currently 
> supported");

You use this text in a few different errors in the block code; we've currently
got one test machine which is producing it and we haven't yet figured out
which one of the exit paths is doing it.  Please make each one unique and state
an identifier; e.g. "checkpoint on block backup in sync ... for blockjob ..."
(I'm not sure what exaclty makes sense for block jobs - but something like 
that).

Dave
  
> +        return;
> +    }
> +
> +    hbitmap_reset_all(backup_job->bitmap);
> +}
> +
>  static const BlockJobDriver backup_job_driver = {
>      .instance_size  = sizeof(BackupBlockJob),
>      .job_type       = BLOCK_JOB_TYPE_BACKUP,
>      .set_speed      = backup_set_speed,
>      .iostatus_reset = backup_iostatus_reset,
> +    .do_checkpoint  = backup_do_checkpoint,
>  };
>  
>  static BlockErrorAction backup_error_action(BackupBlockJob *job,
> diff --git a/blockjob.c b/blockjob.c
> index ec46fad..cb412d1 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -400,3 +400,13 @@ void block_job_defer_to_main_loop(BlockJob *job,
>  
>      qemu_bh_schedule(data->bh);
>  }
> +
> +void block_job_do_checkpoint(BlockJob *job, Error **errp)
> +{
> +    if (!job->driver->do_checkpoint) {
> +        error_setg(errp, "this feature or command is not currently 
> supported");
> +        return;
> +    }
> +
> +    job->driver->do_checkpoint(job, errp);
> +}
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index 57d8ef1..b832dc3 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -50,6 +50,9 @@ typedef struct BlockJobDriver {
>       * manually.
>       */
>      void (*complete)(BlockJob *job, Error **errp);
> +
> +    /** Optional callback for job types that support checkpoint. */
> +    void (*do_checkpoint)(BlockJob *job, Error **errp);
>  } BlockJobDriver;
>  
>  /**
> @@ -348,4 +351,13 @@ void block_job_defer_to_main_loop(BlockJob *job,
>                                    BlockJobDeferToMainLoopFn *fn,
>                                    void *opaque);
>  
> +/**
> + * block_job_do_checkpoint:
> + * @job: The job.
> + * @errp: Error object.
> + *
> + * Do block checkpoint on the specified job.
> + */
> +void block_job_do_checkpoint(BlockJob *job, Error **errp);
> +
>  #endif
> -- 
> 2.4.3
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK