[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: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [Qemu-block] [PATCH v10 02/10] Backup: clear all bitmap when doing block checkpoint |
Date: |
Wed, 14 Oct 2015 16:12:17 +0200 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
On Tue, Oct 13, 2015 at 05:13:14PM +0800, Wen Congyang wrote:
> 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.
It's a question of taste. In this case it seems to me that there is
really a direct coupling between COLO and the backup block job. This
isn't really a generic interface that makes sense in other scenarios.
That's why I advocate for direct coupling instead of pretending this is
a generic interface.
I wish COLO could just stop the existing block job and start a new one
for each checkpoint. In reality we probably don't want QMP events and
the full block job lifecycle for each checkpoint... But anyway, I like
this approach because it does not require a new interface at all.
> > 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.
Okay, I ask these questions because the information should be part of
the doc comment for this new interface.
Stefan