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

[Hailiang Zhang]

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