Re: [Qemu-devel] [PATCH V1 1/2] Implement sync modes for drive-backup.

[Fam Zheng]

On Mon, 07/01 14:16, Paolo Bonzini wrote:
> Il 28/06/2013 04:28, Ian Main ha scritto:
> > diff --git a/blockdev.c b/blockdev.c
> > index c5abd65..000dea6 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -1430,17 +1430,13 @@ void qmp_drive_backup(const char *device, const 
> > char *target,
> >                        Error **errp)
> >  {
> >      BlockDriverState *bs;
> > -    BlockDriverState *target_bs;
> > +    BlockDriverState *target_bs, *source;
> >      BlockDriver *drv = NULL;
> >      Error *local_err = NULL;
> >      int flags;
> >      int64_t size;
> >      int ret;
> >  
> > -    if (sync != MIRROR_SYNC_MODE_FULL) {
> > -        error_setg(errp, "only sync mode 'full' is currently supported");
> > -        return;
> > -    }
> >      if (!has_speed) {
> >          speed = 0;
> >      }
> > @@ -1483,6 +1479,13 @@ void qmp_drive_backup(const char *device, const char 
> > *target,
> >  
> >      flags = bs->open_flags | BDRV_O_RDWR;
> >  
> > +    /* See if we have a backing HD we can use to create our new image
> > +     * on top of. */
> > +    source = bs->backing_hd;
> 
> Should the source be "bs" for MIRROR_SYNC_MODE_NONE?  Also in this case
> you may want to default the format to "qcow2" instead of bs's format.
> 
> Paolo
> 
Maybe not. "source" only affects when sync=top below here. For reading
the uncopied for target from source, we can't simply assign it to "bs"
for sync=none and create the new image with with bs->filename as backing
file, because that way we don't know how to open it with what we have
now: the backing file is already opened RW (as "bs").

Fam
> > +    if (!source && sync == MIRROR_SYNC_MODE_TOP) {
> > +        sync = MIRROR_SYNC_MODE_FULL;
> > +    }
> > +
> >      size = bdrv_getlength(bs);
> >      if (size < 0) {
> >          error_setg_errno(errp, -size, "bdrv_getlength failed");
> > @@ -1491,8 +1494,14 @@ void qmp_drive_backup(const char *device, const char 
> > *target,
> >  
> >      if (mode != NEW_IMAGE_MODE_EXISTING) {
> >          assert(format && drv);
> > -        bdrv_img_create(target, format,
> > -                        NULL, NULL, NULL, size, flags, &local_err, false);
> > +        if (sync == MIRROR_SYNC_MODE_TOP) {
> > +            bdrv_img_create(target, format, source->filename,
> > +                            source->drv->format_name, NULL,
> > +                            size, flags, &local_err, false);
> > +        } else {
> > +            bdrv_img_create(target, format, NULL, NULL, NULL,
> > +                            size, flags, &local_err, false);
> > +        }
> >      }
> >  
> >      if (error_is_set(&local_err)) {
> > @@ -1508,7 +1517,7 @@ void qmp_drive_backup(const char *device, const char 
> > *target,
> >          return;
> >      }
> >  
> > -    backup_start(bs, target_bs, speed, on_source_error, on_target_error,
> > +    backup_start(bs, target_bs, speed, sync, on_source_error, 
> > on_target_error,
> >                   block_job_cb, bs, &local_err);
> >      if (local_err != NULL) {
> >          bdrv_delete(target_bs);
> > diff --git a/include/block/block_int.h b/include/block/block_int.h
> > index c6ac871..e45f2a0 100644
> > --- a/include/block/block_int.h
> > +++ b/include/block/block_int.h
> > @@ -404,6 +404,7 @@ void mirror_start(BlockDriverState *bs, 
> > BlockDriverState *target,
> >   * @bs: Block device to operate on.
> >   * @target: Block device to write to.
> >   * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
> > + * @sync_mode: What parts of the disk image should be copied to the 
> > destination.
> >   * @on_source_error: The action to take upon error reading from the source.
> >   * @on_target_error: The action to take upon error writing to the target.
> >   * @cb: Completion function for the job.
> > @@ -413,7 +414,8 @@ void mirror_start(BlockDriverState *bs, 
> > BlockDriverState *target,
> >   * until the job is cancelled or manually completed.
> >   */
> >  void backup_start(BlockDriverState *bs, BlockDriverState *target,
> > -                  int64_t speed, BlockdevOnError on_source_error,
> > +                  int64_t speed, MirrorSyncMode sync_mode,
> > +                  BlockdevOnError on_source_error,
> >                    BlockdevOnError on_target_error,
> >                    BlockDriverCompletionFunc *cb, void *opaque,
> >                    Error **errp);
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index cdd2d6b..b3f6c2a 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -1807,6 +1807,10 @@
> >  # @format: #optional the format of the new destination, default is to
> >  #          probe if @mode is 'existing', else the format of the source
> >  #
> > +# @sync: what parts of the disk image should be copied to the destination
> > +#        (all the disk, only the sectors allocated in the topmost image, or
> > +#        only new I/O).
> > +#
> >  # @mode: #optional whether and how QEMU should create a new image, default 
> > is
> >  #        'absolute-paths'.
> >  #
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index e075df4..f3f6b3d 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -957,6 +957,7 @@ Arguments:
> >  
> >  Example:
> >  -> { "execute": "drive-backup", "arguments": { "device": "drive0",
> > +                                               "sync": "full",
> >                                                 "target": "backup.img" } }
> >  <- { "return": {} }
> >  EQMP
> > 
> 
>