Re: [Qemu-devel] [PATCH RFC 4/7] replication: Split out backup_do_checkpoint() from secondary_do_checkpoint()

[Hailiang Zhang <zhang.zhanghailiang@huawei.com>]

On 2016/10/26 9:40, Changlong Xie wrote:
> On 10/20/2016 09:57 PM, zhanghailiang wrote:
>> The helper backup_do_checkpoint() will be used for primary related
>> codes. Here we split it out from secondary_do_checkpoint().
>>
>> Besides, it is unnecessary to call backup_do_checkpoint() in
>> replication starting and normally stop replication path.
>
> This patch is unnecessary. We *really* need clean
> backup_job->done_bitmap in replication_start/stop path.
>

After we support internal job ('BLOCK_JOB_INTERNAL'), do we still need
to call backup_do?
IMHO, we don't have to clean 'done_bitmap', because
its initial value is zero, and we don't have to call it in
stop path either, the backup job will be cleaned.


>> We only need call it while do real checkpointing.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> ---
>>    block/replication.c | 36 +++++++++++++++++++-----------------
>>    1 file changed, 19 insertions(+), 17 deletions(-)
>>
>> diff --git a/block/replication.c b/block/replication.c
>> index 2a2fdb2..d687ffc 100644
>> --- a/block/replication.c
>> +++ b/block/replication.c
>> @@ -320,20 +320,8 @@ static bool replication_recurse_is_first_non_filter(BlockDriverState *bs,
>>
>>    static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp)
>>    {
>> -    Error *local_err = NULL;
>>        int ret;
>>
>> -    if (!s->secondary_disk->bs->job) {
>> -        error_setg(errp, "Backup job was cancelled unexpectedly");
>> -        return;
>> -    }
>> -
>> -    backup_do_checkpoint(s->secondary_disk->bs->job, &local_err);
>> -    if (local_err) {
>> -        error_propagate(errp, local_err);
>> -        return;
>> -    }
>> -
>>        ret = s->active_disk->bs->drv->bdrv_make_empty(s->active_disk->bs);
>>        if (ret < 0) {
>>            error_setg(errp, "Cannot make active disk empty");
>> @@ -539,6 +527,8 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
>>                aio_context_release(aio_context);
>>                return;
>>            }
>> +
>> +        secondary_do_checkpoint(s, errp);
>>            break;
>>        default:
>>            aio_context_release(aio_context);
>> @@ -547,10 +537,6 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
>>
>>        s->replication_state = BLOCK_REPLICATION_RUNNING;
>>
>> -    if (s->mode == REPLICATION_MODE_SECONDARY) {
>> -        secondary_do_checkpoint(s, errp);
>> -    }
>> -
>>        s->error = 0;
>>        aio_context_release(aio_context);
>>    }
>> @@ -560,13 +546,29 @@ static void replication_do_checkpoint(ReplicationState *rs, Error **errp)
>>        BlockDriverState *bs = rs->opaque;
>>        BDRVReplicationState *s;
>>        AioContext *aio_context;
>> +    Error *local_err = NULL;
>>
>>        aio_context = bdrv_get_aio_context(bs);
>>        aio_context_acquire(aio_context);
>>        s = bs->opaque;
>>
>> -    if (s->mode == REPLICATION_MODE_SECONDARY) {
>> +    switch (s->mode) {
>> +    case REPLICATION_MODE_PRIMARY:
>> +        break;
>> +    case REPLICATION_MODE_SECONDARY:
>> +        if (!s->secondary_disk->bs->job) {
>> +            error_setg(errp, "Backup job was cancelled unexpectedly");
>> +            break;
>> +        }
>> +        backup_do_checkpoint(s->secondary_disk->bs->job, &local_err);
>> +        if (local_err) {
>> +            error_propagate(errp, local_err);
>> +            break;
>> +        }
>>            secondary_do_checkpoint(s, errp);
>> +        break;
>> +    default:
>> +        abort();
>>        }
>>        aio_context_release(aio_context);
>>    }
>>
>
>
>
> .
>