[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-block] [PATCH v10 02/10] Backup: clear all bitmap
From: |
Wen Congyang |
Subject: |
Re: [Qemu-devel] [Qemu-block] [PATCH v10 02/10] Backup: clear all bitmap when doing block checkpoint |
Date: |
Tue, 13 Oct 2015 17:13:14 +0800 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 |
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.
>
> 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
> .
>