[Qemu-devel] [RFC PATCH v3 0/2] Point-in-time snapshot exporting over NBD

[Qemu-devel] [RFC PATCH v3 0/2] Point-in-time snapshot exporting over NBD

[Fam Zheng]

This series adds for point-in-time snapshot NBD exporting based on
blockdev-backup (variant of drive-backup with existing device as target).

We get a thin point-in-time snapshot by COW mechanism of drive-backup, and
export it through built in NBD server. The steps are as below:

 1. (SHELL) qemu-img create -f qcow2 BACKUP.qcow2 <source size here>

    (Alternatively we can use -o backing_file=RUNNING-VM.img to omit explicitly
    providing the size by ourselves, but it's risky because RUNNING-VM.qcow2 is
    used r/w by guest. Whether or not setting backing file in the image file
    doesn't matter, as we are going to override the backing hd in the next
    step)

 2. (QMP) blockdev-add backing=source-drive file.driver=file file.filename=BACKUP.qcow2 id=target0 if=none driver=qcow2

    (where ide0-hd0 is the running BlockDriverState name for
    RUNNING-VM.img. This patch implements "backing=" option to override
    backing_hd for added drive)

 3. (QMP) blockdev-backup device=source-drive sync=none target=target0

    (this is the QMP command introduced by this series, which use a named
    device as target of drive-backup)

 4. (QMP) nbd-server-add device=target0

When image fleecing done:

 1. (QMP) block-job-complete device=ide0-hd0

 2. (HMP) drive_del target0

 3. (SHELL) rm BACKUP.qcow2

v3: Base on blockdev-add.
    The syntax blockdev-add backing=<id> is new: This will make referenced BDS
    in the middle of a backing chain, which has significant effects over all
    existing block operations in QMP. It needs to be reviewed and tested carefully.

    I'm listing the commands here that can take a device id as parameter (but
    may not be complete).

    These commands do not mutate the backing chain (not directly, at least) and
    should be safe:
        block_passwd
        block_set_io_throttle
        block-job-set-speed
        block-job-cancel
        block-job-pause
        block-job-resume
        block-job-complete
        drive-backup
        blockdev-snapshot-sync
        blockdev-snapshot-internal-sync
        blockdev-snapshot-delete-internal-sync: These should be safe.
        nbd-server-add

    These can mutates the chain (removing, closing or swapping BDS), need more
    work to convert them to safe operations with a BDS in the middle.
        device_del
        eject: it does bdrv_close the device.
        change: internally calls eject.
        block-commit: it deletes intermediate BDSes, which may include other named BDS.
        block-stream: 
        drive-mirror: it swaps active with target when complete.

    Resizing a middle BDS need to be reviewed too:
        block_resize: TBD.

    Adding and backing HD referencing will put other BDS in middle, but should
    not immediately break things:
        blockdev-add

Fam Zheng (2):
  block: parse "backing" option to reference existing BDS
  qmp: add command 'blockdev-backup'

 block.c          | 27 +++++++++++++++++++-----
 blockdev.c       | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json | 49 +++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx  | 46 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 180 insertions(+), 5 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [RFC PATCH v3 1/2] block: parse "backing" option to reference existing BDS

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index fd05a80..38b3e80 100644
--- a/block.c
+++ b/block.c
@@ -1158,11 +1158,28 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
     /* If there is a backing file, use it */
     if ((flags & BDRV_O_NO_BACKING) == 0) {
         QDict *backing_options;
-
-        qdict_extract_subqdict(options, &backing_options, "backing.");
-        ret = bdrv_open_backing_file(bs, backing_options, &local_err);
-        if (ret < 0) {
-            goto close_and_fail;
+        const char *backing_bs;
+
+        backing_bs = qdict_get_try_str(options, "backing");
+        if (backing_bs) {
+            bs->backing_hd = bdrv_find(backing_bs);
+            bdrv_ref(bs->backing_hd);
+            qdict_del(options, "backing");
+            if (bs->backing_hd) {
+                pstrcpy(bs->backing_file, sizeof(bs->backing_file),
+                        bs->backing_hd->filename);
+                pstrcpy(bs->backing_format, sizeof(bs->backing_format),
+                        bs->backing_hd->drv->format_name);
+            } else {
+                error_setg(errp, "Backing device not found: %s", backing_bs);
+                goto close_and_fail;
+            }
+        } else {
+            qdict_extract_subqdict(options, &backing_options, "backing.");
+            ret = bdrv_open_backing_file(bs, backing_options, &local_err);
+            if (ret < 0) {
+                goto close_and_fail;
+            }
         }
     }
 
-- 
1.8.3.1

Re: [Qemu-devel] [RFC PATCH v3 1/2] block: parse "backing" option to reference existing BDS

[Fam Zheng]

On Thu, 10/17 13:36, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/block.c b/block.c
> index fd05a80..38b3e80 100644
> --- a/block.c
> +++ b/block.c
> @@ -1158,11 +1158,28 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>      /* If there is a backing file, use it */
>      if ((flags & BDRV_O_NO_BACKING) == 0) {
>          QDict *backing_options;
> -
> -        qdict_extract_subqdict(options, &backing_options, "backing.");
> -        ret = bdrv_open_backing_file(bs, backing_options, &local_err);
> -        if (ret < 0) {
> -            goto close_and_fail;
> +        const char *backing_bs;
> +
> +        backing_bs = qdict_get_try_str(options, "backing");
> +        if (backing_bs) {
> +            bs->backing_hd = bdrv_find(backing_bs);
> +            bdrv_ref(bs->backing_hd);

This should be moved down into if.

Fam

> +            qdict_del(options, "backing");
> +            if (bs->backing_hd) {
> +                pstrcpy(bs->backing_file, sizeof(bs->backing_file),
> +                        bs->backing_hd->filename);
> +                pstrcpy(bs->backing_format, sizeof(bs->backing_format),
> +                        bs->backing_hd->drv->format_name);
> +            } else {
> +                error_setg(errp, "Backing device not found: %s", backing_bs);
> +                goto close_and_fail;
> +            }
> +        } else {
> +            qdict_extract_subqdict(options, &backing_options, "backing.");
> +            ret = bdrv_open_backing_file(bs, backing_options, &local_err);
> +            if (ret < 0) {
> +                goto close_and_fail;
> +            }
>          }
>      }
>  
> -- 
> 1.8.3.1
> 
> 

[Qemu-devel] [RFC PATCH v3 2/2] qmp: add command 'blockdev-backup'

[Fam Zheng]

Similar to drive-backup, but this command uses a device id as target
instead of creating/opening an image file.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c       | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json | 49 +++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx  | 46 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 158 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 4f76e28..c53d597 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1937,6 +1937,69 @@ void qmp_drive_backup(const char *device, const char *target,
     }
 }
 
+void qmp_blockdev_backup(const char *device, const char *target,
+                       enum MirrorSyncMode sync,
+                       bool has_speed, int64_t speed,
+                       bool has_on_source_error,
+                       BlockdevOnError on_source_error,
+                       bool has_on_target_error,
+                       BlockdevOnError on_target_error,
+                       Error **errp)
+{
+    BlockDriverState *bs;
+    BlockDriverState *target_bs;
+    Error *local_err = NULL;
+
+    if (!has_speed) {
+        speed = 0;
+    }
+    if (!has_on_source_error) {
+        on_source_error = BLOCKDEV_ON_ERROR_REPORT;
+    }
+    if (!has_on_target_error) {
+        on_target_error = BLOCKDEV_ON_ERROR_REPORT;
+    }
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    if (!bdrv_is_inserted(bs)) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
+        return;
+    }
+
+    if (bdrv_in_use(bs)) {
+        error_set(errp, QERR_DEVICE_IN_USE, device);
+        return;
+    }
+
+    target_bs = bdrv_find(target);
+    if (!target_bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, target);
+        return;
+    }
+
+    if (!bdrv_is_inserted(target_bs)) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, target);
+        return;
+    }
+
+    if (bdrv_in_use(target_bs)) {
+        error_set(errp, QERR_DEVICE_IN_USE, target);
+        return;
+    }
+
+    bdrv_ref(target_bs);
+    backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error,
+                 block_job_cb, bs, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+    }
+}
+
 #define DEFAULT_MIRROR_BUF_SIZE   (10 << 20)
 
 void qmp_drive_mirror(const char *device, const char *target,
diff --git a/qapi-schema.json b/qapi-schema.json
index 60f3fd1..89bc510 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1794,6 +1794,40 @@
             '*on-target-error': 'BlockdevOnError' } }
 
 ##
+# @BlockdevBackup
+#
+# @device: the name of the device which should be copied.
+#
+# @target: the name of the backup target device
+#
+# @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).
+#
+# @speed: #optional the maximum speed, in bytes per second
+#
+# @on-source-error: #optional the action to take on an error on the source,
+#                   default 'report'.  'stop' and 'enospc' can only be used
+#                   if the block device supports io-status (see BlockInfo).
+#
+# @on-target-error: #optional the action to take on an error on the target,
+#                   default 'report' (no limitations, since this applies to
+#                   a different block device than @device).
+#
+# Note that @on-source-error and @on-target-error only affect background I/O.
+# If an error occurs during a guest write request, the device's rerror/werror
+# actions will be used.
+#
+# Since: 1.7
+##
+{ 'type': 'BlockdevBackup',
+  'data': { 'device': 'str', 'target': 'str',
+            'sync': 'MirrorSyncMode',
+            '*speed': 'int',
+            '*on-source-error': 'BlockdevOnError',
+            '*on-target-error': 'BlockdevOnError' } }
+
+##
 # @Abort
 #
 # This action can be used to test transaction failure.
@@ -1983,6 +2017,21 @@
 { 'command': 'drive-backup', 'data': 'DriveBackup' }
 
 ##
+# @blockdev-backup
+#
+# Block device version for drive-backup command. Use existing device as target
+# of backup.
+#
+# For the arguments, see the documentation of BlockdevBackup.
+#
+# Returns: nothing on success
+#          If @device or @target is not a valid block device, DeviceNotFound
+#
+# Since 1.7
+##
+{ 'command': 'blockdev-backup', 'data': 'BlockdevBackup' }
+
+##
 # @drive-mirror
 #
 # Start mirroring a block device's writes to a new destination.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index fba15cd..4116a8d 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -963,6 +963,52 @@ Example:
                                                "sync": "full",
                                                "target": "backup.img" } }
 <- { "return": {} }
+
+EQMP
+
+    {
+        .name       = "blockdev-backup",
+        .args_type  = "sync:s,device:B,target:B,speed:i?,"
+                      "on-source-error:s?,on-target-error:s?",
+        .mhandler.cmd_new = qmp_marshal_input_blockdev_backup,
+    },
+
+SQMP
+blockdev-backup
+------------
+
+The device version of drive-backup: this command takes a existing named device
+as backup target.
+
+Arguments:
+
+- "device": the name of the device which should be copied.
+            (json-string)
+- "target": the target of the new image. If the file exists, or if it is a
+            device, the existing file/device will be used as the new
+            destination.  If it does not exist, a new file will be created.
+            (json-string)
+- "sync": what parts of the disk image should be copied to the destination;
+  possibilities include "full" for all the disk, "top" for only the sectors
+  allocated in the topmost image, or "none" to only replicate new I/O
+  (MirrorSyncMode).
+- "mode": whether and how QEMU should create a new image
+          (NewImageMode, optional, default 'absolute-paths')
+- "speed": the maximum speed, in bytes per second (json-int, optional)
+- "on-source-error": the action to take on an error on the source, default
+                     'report'.  'stop' and 'enospc' can only be used
+                     if the block device supports io-status.
+                     (BlockdevOnError, optional)
+- "on-target-error": the action to take on an error on the target, default
+                     'report' (no limitations, since this applies to
+                     a different block device than device).
+                     (BlockdevOnError, optional)
+
+Example:
+-> { "execute": "blockdev-backup", "arguments": { "device": "src-id",
+                                                  "target": "tgt-id" } }
+<- { "return": {} }
+
 EQMP
 
     {
-- 
1.8.3.1

Re: [Qemu-devel] [RFC PATCH v3 2/2] qmp: add command 'blockdev-backup'

[Paolo Bonzini]

Il 17/10/2013 07:36, Fam Zheng ha scritto:
> Similar to drive-backup, but this command uses a device id as target
> instead of creating/opening an image file.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockdev.c       | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json | 49 +++++++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx  | 46 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 158 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 4f76e28..c53d597 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1937,6 +1937,69 @@ void qmp_drive_backup(const char *device, const char *target,
>      }
>  }
>  
> +void qmp_blockdev_backup(const char *device, const char *target,
> +                       enum MirrorSyncMode sync,
> +                       bool has_speed, int64_t speed,
> +                       bool has_on_source_error,
> +                       BlockdevOnError on_source_error,
> +                       bool has_on_target_error,
> +                       BlockdevOnError on_target_error,
> +                       Error **errp)
> +{
> +    BlockDriverState *bs;
> +    BlockDriverState *target_bs;
> +    Error *local_err = NULL;
> +
> +    if (!has_speed) {
> +        speed = 0;
> +    }
> +    if (!has_on_source_error) {
> +        on_source_error = BLOCKDEV_ON_ERROR_REPORT;
> +    }
> +    if (!has_on_target_error) {
> +        on_target_error = BLOCKDEV_ON_ERROR_REPORT;
> +    }
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
> +    }
> +
> +    if (!bdrv_is_inserted(bs)) {
> +        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
> +        return;
> +    }
> +
> +    if (bdrv_in_use(bs)) {
> +        error_set(errp, QERR_DEVICE_IN_USE, device);
> +        return;
> +    }

Should these two checks be moved to backup_start?

> +    target_bs = bdrv_find(target);
> +    if (!target_bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, target);
> +        return;
> +    }
> +
> +    if (!bdrv_is_inserted(target_bs)) {
> +        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, target);
> +        return;
> +    }
> +
> +    if (bdrv_in_use(target_bs)) {
> +        error_set(errp, QERR_DEVICE_IN_USE, target);
> +        return;
> +    }

Same for these two.

> +    bdrv_ref(target_bs);
> +    backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error,
> +                 block_job_cb, bs, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);

Missing bdrv_unref.

Otherwise looks good!

Paolo

> +    }
> +}
> +
>  #define DEFAULT_MIRROR_BUF_SIZE   (10 << 20)
>  
>  void qmp_drive_mirror(const char *device, const char *target,
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 60f3fd1..89bc510 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1794,6 +1794,40 @@
>              '*on-target-error': 'BlockdevOnError' } }
>  
>  ##
> +# @BlockdevBackup
> +#
> +# @device: the name of the device which should be copied.
> +#
> +# @target: the name of the backup target device
> +#
> +# @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).
> +#
> +# @speed: #optional the maximum speed, in bytes per second
> +#
> +# @on-source-error: #optional the action to take on an error on the source,
> +#                   default 'report'.  'stop' and 'enospc' can only be used
> +#                   if the block device supports io-status (see BlockInfo).
> +#
> +# @on-target-error: #optional the action to take on an error on the target,
> +#                   default 'report' (no limitations, since this applies to
> +#                   a different block device than @device).
> +#
> +# Note that @on-source-error and @on-target-error only affect background I/O.
> +# If an error occurs during a guest write request, the device's rerror/werror
> +# actions will be used.
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'BlockdevBackup',
> +  'data': { 'device': 'str', 'target': 'str',
> +            'sync': 'MirrorSyncMode',
> +            '*speed': 'int',
> +            '*on-source-error': 'BlockdevOnError',
> +            '*on-target-error': 'BlockdevOnError' } }
> +
> +##
>  # @Abort
>  #
>  # This action can be used to test transaction failure.
> @@ -1983,6 +2017,21 @@
>  { 'command': 'drive-backup', 'data': 'DriveBackup' }
>  
>  ##
> +# @blockdev-backup
> +#
> +# Block device version for drive-backup command. Use existing device as target
> +# of backup.
> +#
> +# For the arguments, see the documentation of BlockdevBackup.
> +#
> +# Returns: nothing on success
> +#          If @device or @target is not a valid block device, DeviceNotFound
> +#
> +# Since 1.7
> +##
> +{ 'command': 'blockdev-backup', 'data': 'BlockdevBackup' }
> +
> +##
>  # @drive-mirror
>  #
>  # Start mirroring a block device's writes to a new destination.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index fba15cd..4116a8d 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -963,6 +963,52 @@ Example:
>                                                 "sync": "full",
>                                                 "target": "backup.img" } }
>  <- { "return": {} }
> +
> +EQMP
> +
> +    {
> +        .name       = "blockdev-backup",
> +        .args_type  = "sync:s,device:B,target:B,speed:i?,"
> +                      "on-source-error:s?,on-target-error:s?",
> +        .mhandler.cmd_new = qmp_marshal_input_blockdev_backup,
> +    },
> +
> +SQMP
> +blockdev-backup
> +------------
> +
> +The device version of drive-backup: this command takes a existing named device
> +as backup target.
> +
> +Arguments:
> +
> +- "device": the name of the device which should be copied.
> +            (json-string)
> +- "target": the target of the new image. If the file exists, or if it is a
> +            device, the existing file/device will be used as the new
> +            destination.  If it does not exist, a new file will be created.
> +            (json-string)
> +- "sync": what parts of the disk image should be copied to the destination;
> +  possibilities include "full" for all the disk, "top" for only the sectors
> +  allocated in the topmost image, or "none" to only replicate new I/O
> +  (MirrorSyncMode).
> +- "mode": whether and how QEMU should create a new image
> +          (NewImageMode, optional, default 'absolute-paths')
> +- "speed": the maximum speed, in bytes per second (json-int, optional)
> +- "on-source-error": the action to take on an error on the source, default
> +                     'report'.  'stop' and 'enospc' can only be used
> +                     if the block device supports io-status.
> +                     (BlockdevOnError, optional)
> +- "on-target-error": the action to take on an error on the target, default
> +                     'report' (no limitations, since this applies to
> +                     a different block device than device).
> +                     (BlockdevOnError, optional)
> +
> +Example:
> +-> { "execute": "blockdev-backup", "arguments": { "device": "src-id",
> +                                                  "target": "tgt-id" } }
> +<- { "return": {} }
> +
>  EQMP
>  
>      {
> 

Re: [Qemu-devel] [RFC PATCH v3 0/2] Point-in-time snapshot exporting over NBD

[Ian Main]

On Thu, Oct 17, 2013 at 01:36:41PM +0800, Fam Zheng wrote:
> This series adds for point-in-time snapshot NBD exporting based on
> blockdev-backup (variant of drive-backup with existing device as target).

In general this seems to work great.  I did a bunch of testing and was
able to mount filesystems over NBD, do many writes (on backed up
image)/reads (on target), intense IO etc and all held up fine.

There are definitely some issues with some of the commands allowing you
to blow things up.  I'm interested to hear opinions on whether this is a
showstopper at this time or not.

> We get a thin point-in-time snapshot by COW mechanism of drive-backup, and
> export it through built in NBD server. The steps are as below:
> 
>  1. (SHELL) qemu-img create -f qcow2 BACKUP.qcow2 <source size here>
> 
>     (Alternatively we can use -o backing_file=RUNNING-VM.img to omit explicitly
>     providing the size by ourselves, but it's risky because RUNNING-VM.qcow2 is
>     used r/w by guest. Whether or not setting backing file in the image file
>     doesn't matter, as we are going to override the backing hd in the next
>     step)
> 
>  2. (QMP) blockdev-add backing=source-drive file.driver=file file.filename=BACKUP.qcow2 id=target0 if=none driver=qcow2

I had to create a custom python script to make these commands work as
they require nested dicts.  Is that normal or did I miss something
entirely?
 
>     (where ide0-hd0 is the running BlockDriverState name for
>     RUNNING-VM.img. This patch implements "backing=" option to override
>     backing_hd for added drive)
> 
>  3. (QMP) blockdev-backup device=source-drive sync=none target=target0
> 
>     (this is the QMP command introduced by this series, which use a named
>     device as target of drive-backup)
> 
>  4. (QMP) nbd-server-add device=target0
> 
> When image fleecing done:
> 
>  1. (QMP) block-job-complete device=ide0-hd0

To be clear it is block-job-cancel that is used.  There is no 'complete'
handler for the backup block job so this just returns an error.

>  2. (HMP) drive_del target0

In order for this to work with libvirt I think we are going to need a QMP
interface for this.
 
>  3. (SHELL) rm BACKUP.qcow2
> 
> v3: Base on blockdev-add.
>     The syntax blockdev-add backing=<id> is new: This will make referenced BDS
>     in the middle of a backing chain, which has significant effects over all
>     existing block operations in QMP. It needs to be reviewed and tested carefully.
> 
>     I'm listing the commands here that can take a device id as parameter (but
>     may not be complete).
> 
>     These commands do not mutate the backing chain (not directly, at least) and
>     should be safe:
>         block_passwd
>         block_set_io_throttle
>         block-job-set-speed
>         block-job-cancel
>         block-job-pause
>         block-job-resume
>         block-job-complete
>         drive-backup
>         blockdev-snapshot-sync
>         blockdev-snapshot-internal-sync
>         blockdev-snapshot-delete-internal-sync: These should be safe.
>         nbd-server-add
> 
>     These can mutates the chain (removing, closing or swapping BDS), need more
>     work to convert them to safe operations with a BDS in the middle.
>         device_del

I couldn't figure out how to make this operate on block devices or how
to make it break things.. :)

>         eject: it does bdrv_close the device.
>         change: internally calls eject.

Calling eject or change on the target causes a 'failed command: WRITE DMA'
backtrace in the VM which causes the root fs to be remounted read only.
I'm guessing from the CoW failing..

>         block-commit: it deletes intermediate BDSes, which may include other named BDS.
>         block-stream: 
>         drive-mirror: it swaps active with target when complete.

Doing a mirror of the target causes a segfault when completed.. :)  I
gather you are not allowed to perform multiple block jobs on 1 BDS at a
time as I get 'in use' errors when trying to do the same BDS as the one
you are backing up.  Doing a drive-mirror on the bottom-most image of
3 seemed to work ok.
 
>     Resizing a middle BDS need to be reviewed too:
>         block_resize: TBD.

I tried resizing a middle BDS and got a 'middle0 is in use' error.  The
topmost BDS got a 'feature/command not supported' error.  I was able to
resize the bottom BDS and then got I/O errors, eg:

{u'timestamp': {u'seconds': 1384913492, u'microseconds': 993397},
u'data': {u'device': u'ide0-hd0', u'action': u'report', u'operation':
u'read'}, u'event': u'BLOCK_IO_ERROR'}

However the NBD export *seemed* to be ok and everything kept running..
 
>     Adding and backing HD referencing will put other BDS in middle, but should
>     not immediately break things:
>         blockdev-add

Not quite sure what you mean here.  I can add other BDS's but could I
put one in the middle while in a backup?  That would require being able
to change the 'backing' on the fly which I don't think we can do?  (or
should be able to do.. heh).
 
> Fam Zheng (2):
>   block: parse "backing" option to reference existing BDS
>   qmp: add command 'blockdev-backup'
> 
>  block.c          | 27 +++++++++++++++++++-----
>  blockdev.c       | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json | 49 +++++++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx  | 46 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 180 insertions(+), 5 deletions(-)
> 
> -- 
> 1.8.3.1
> 

Re: [Qemu-devel] [RFC PATCH v3 0/2] Point-in-time snapshot exporting over NBD

[Fam Zheng]

On 2013年11月20日 10:32, Ian Main wrote:
> On Thu, Oct 17, 2013 at 01:36:41PM +0800, Fam Zheng wrote:
>> This series adds for point-in-time snapshot NBD exporting based on
>> blockdev-backup (variant of drive-backup with existing device as target).
>
> In general this seems to work great.  I did a bunch of testing and was
> able to mount filesystems over NBD, do many writes (on backed up
> image)/reads (on target), intense IO etc and all held up fine.
>
> There are definitely some issues with some of the commands allowing you
> to blow things up.  I'm interested to hear opinions on whether this is a
> showstopper at this time or not.
>

Thanks very much for your testing, Ian. QEMU should report error instead 
of crashing with invalid operations. I added an "operation blocker" and 
incorporated into the next version.

In the future, we still want to review more on those commands and try to 
enable safe ones, and possibly also allow multiple block jobs on a BDS. 
For now it is good enough to only allow blockdev-backup on the source 
and NBD export on the target (which is safe, and all what we need for 
image fleecing, as this version shows). With that being a working 
implementation, I've posted v4. Please test and free to make comments.

>> We get a thin point-in-time snapshot by COW mechanism of drive-backup, and
>> export it through built in NBD server. The steps are as below:
>>
>>   1. (SHELL) qemu-img create -f qcow2 BACKUP.qcow2 <source size here>
>>
>>      (Alternatively we can use -o backing_file=RUNNING-VM.img to omit explicitly
>>      providing the size by ourselves, but it's risky because RUNNING-VM.qcow2 is
>>      used r/w by guest. Whether or not setting backing file in the image file
>>      doesn't matter, as we are going to override the backing hd in the next
>>      step)
>>
>>   2. (QMP) blockdev-add backing=source-drive file.driver=file file.filename=BACKUP.qcow2 id=target0 if=none driver=qcow2
>
> I had to create a custom python script to make these commands work as
> they require nested dicts.  Is that normal or did I miss something
> entirely?
>

Yes, I use a python script locally to test it too.

Fam

Re: [Qemu-devel] [RFC PATCH v3 0/2] Point-in-time snapshot exporting over NBD

[Ian Main]

On Fri, Nov 22, 2013 at 01:47:26PM +0800, Fam Zheng wrote:
> On 2013年11月20日 10:32, Ian Main wrote:
> >On Thu, Oct 17, 2013 at 01:36:41PM +0800, Fam Zheng wrote:
> >>This series adds for point-in-time snapshot NBD exporting based on
> >>blockdev-backup (variant of drive-backup with existing device as target).
> >
> >In general this seems to work great.  I did a bunch of testing and was
> >able to mount filesystems over NBD, do many writes (on backed up
> >image)/reads (on target), intense IO etc and all held up fine.
> >
> >There are definitely some issues with some of the commands allowing you
> >to blow things up.  I'm interested to hear opinions on whether this is a
> >showstopper at this time or not.
> >
> 
> Thanks very much for your testing, Ian. QEMU should report error
> instead of crashing with invalid operations. I added an "operation
> blocker" and incorporated into the next version.
> 
> In the future, we still want to review more on those commands and
> try to enable safe ones, and possibly also allow multiple block jobs
> on a BDS. For now it is good enough to only allow blockdev-backup on
> the source and NBD export on the target (which is safe, and all what
> we need for image fleecing, as this version shows). With that being
> a working implementation, I've posted v4. Please test and free to
> make comments.

That's great.  Interesting solution you have come up with.  I will give
it a try and let you know how it goes.
 
> >>We get a thin point-in-time snapshot by COW mechanism of drive-backup, and
> >>export it through built in NBD server. The steps are as below:
> >>
> >>  1. (SHELL) qemu-img create -f qcow2 BACKUP.qcow2 <source size here>
> >>
> >>     (Alternatively we can use -o backing_file=RUNNING-VM.img to omit explicitly
> >>     providing the size by ourselves, but it's risky because RUNNING-VM.qcow2 is
> >>     used r/w by guest. Whether or not setting backing file in the image file
> >>     doesn't matter, as we are going to override the backing hd in the next
> >>     step)
> >>
> >>  2. (QMP) blockdev-add backing=source-drive file.driver=file file.filename=BACKUP.qcow2 id=target0 if=none driver=qcow2
> >
> >I had to create a custom python script to make these commands work as
> >they require nested dicts.  Is that normal or did I miss something
> >entirely?
> >
> 
> Yes, I use a python script locally to test it too.

I like your notation.. I might try to make a patch for qmp/qmp-shell to
allow it to create nested dicts using the dot notation.

	Ian