[Qemu-devel] [RFC PATCH 0/4] hmp/qmp: add snapshot option to nbd export

[Qemu-devel] [RFC PATCH 0/4] hmp/qmp: add snapshot option to nbd export

[Fam Zheng]

This implements another way of doing image fleecing:

  (HMP) nbd_server_add -s drive0

or

  (QMP) nbd-server-add device=drive0 snapshot=on

It is one convenient command that starts drive-backup job on device for doing
COW to a newly created temporary image, we don't need manually creating image,
adding drive or overriding backing_hd of the target. The target_bs is internal
to nbd-server-add implementation and the export is deleted when block job is
cancelled or completed.

Fam Zheng (4):
  block/backup: delete target after completion callback
  nbd: call drive_put_ref() only if dinfo exists
  qmp: Add "snapshot=" option to nbd-server-add
  hmp: add -s switch to nbd_server_add

 block/backup.c   |  2 +-
 blockdev-nbd.c   | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
 hmp-commands.hx  |  4 +--
 hmp.c            |  7 +++--
 qapi-schema.json |  3 ++-
 qmp-commands.hx  |  2 +-
 6 files changed, 89 insertions(+), 10 deletions(-)

-- 
1.8.3.4

[Qemu-devel] [RFC PATCH 1/4] block/backup: delete target after completion callback

[Fam Zheng]

Move bdrv_delete(target) one line down to give block job caller a chance
to handle target on completion before deleting it.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/backup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/backup.c b/block/backup.c
index 6ae8a05..d80341c 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -338,9 +338,9 @@ static void coroutine_fn backup_run(void *opaque)
     hbitmap_free(job->bitmap);
 
     bdrv_iostatus_disable(target);
-    bdrv_delete(target);
 
     block_job_completed(&job->common, ret);
+    bdrv_delete(target);
 }
 
 void backup_start(BlockDriverState *bs, BlockDriverState *target,
-- 
1.8.3.4

[Qemu-devel] [RFC PATCH 2/4] nbd: call drive_put_ref() only if dinfo exists

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev-nbd.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 95f10c8..c75df19 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -72,7 +72,10 @@ static void nbd_close_notifier(Notifier *n, void *data)
 static void nbd_server_put_ref(NBDExport *exp)
 {
     BlockDriverState *bs = nbd_export_get_blockdev(exp);
-    drive_put_ref(drive_get_by_blockdev(bs));
+    DriveInfo *dinfo = drive_get_by_blockdev(bs);
+    if (dinfo) {
+        drive_put_ref(dinfo);
+    }
 }
 
 void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
-- 
1.8.3.4

[Qemu-devel] [RFC PATCH 3/4] qmp: Add "snapshot=" option to nbd-server-add

[Fam Zheng]

With drive-backup block job, we can have a point-in-time snapshot of a
device. With snapshot=on, a backup block job is started on the device to
do CoW to a temporary image and this image is exported to nbd. The image
is deleted after nbd server stops.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev-nbd.c   | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 hmp.c            |  5 ++--
 qapi-schema.json |  3 ++-
 qmp-commands.hx  |  2 +-
 4 files changed, 80 insertions(+), 6 deletions(-)

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index c75df19..f12b57c 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -17,6 +17,8 @@
 #include "qmp-commands.h"
 #include "trace.h"
 #include "block/nbd.h"
+#include "block/block_int.h"
+#include "block/block.h"
 #include "qemu/sockets.h"
 
 static int server_fd = -1;
@@ -78,8 +80,62 @@ static void nbd_server_put_ref(NBDExport *exp)
     }
 }
 
+static void snapshot_drive_backup_cb(void *opaque, int ret)
+{
+    BlockDriverState *bs = opaque;
+    bs->backing_hd = NULL;
+}
+
+/* create a point-in-time snapshot BDS from an existing BDS */
+static BlockDriverState *nbd_create_snapshot(BlockDriverState *orig_bs)
+{
+    int ret;
+    char filename[1024];
+    BlockDriver *drv;
+    BlockDriverState *bs;
+    QEMUOptionParameter *options;
+    Error *local_err = NULL;
+
+    bs = bdrv_new("");
+    ret = get_tmp_filename(filename, sizeof(filename));
+    if (ret < 0) {
+        goto err;
+    }
+    drv = bdrv_find_format("qcow2");
+    if (drv < 0) {
+        goto err;
+    }
+    options = parse_option_parameters("", drv->create_options, NULL);
+    set_option_parameter_int(options, BLOCK_OPT_SIZE, bdrv_getlength(orig_bs));
+
+    ret = bdrv_create(drv, filename, options);
+    if (ret < 0) {
+        goto err;
+    }
+    ret = bdrv_open(bs, filename, NULL, BDRV_O_RDWR, drv);
+    if (ret < 0) {
+        goto err;
+    }
+    bs->backing_hd = orig_bs;
+
+    backup_start(orig_bs, bs, 1,
+            MIRROR_SYNC_MODE_NONE,
+            BLOCKDEV_ON_ERROR_REPORT,
+            BLOCKDEV_ON_ERROR_REPORT,
+            snapshot_drive_backup_cb, bs, &local_err);
+    if (error_is_set(&local_err)) {
+        goto err;
+    }
+    return bs;
+
+err:
+    bdrv_delete(bs);
+    unlink(filename);
+    return NULL;
+}
+
 void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
-                        Error **errp)
+                        bool has_snapshot, bool snapshot, Error **errp)
 {
     BlockDriverState *bs;
     NBDExport *exp;
@@ -104,21 +160,37 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
     if (!has_writable) {
         writable = false;
     }
+
+    if (!has_snapshot) {
+        snapshot = false;
+    }
+
     if (bdrv_is_read_only(bs)) {
         writable = false;
     }
 
+    if (snapshot) {
+        bs = nbd_create_snapshot(bs);
+        if (!bs) {
+            error_setg(errp, "Can't create snapshot for device");
+            return;
+        }
+    }
+
     exp = nbd_export_new(bs, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY,
                          nbd_server_put_ref);
 
     nbd_export_set_name(exp, device);
-    drive_get_ref(drive_get_by_blockdev(bs));
+    if (!snapshot) {
+        drive_get_ref(drive_get_by_blockdev(bs));
+    }
 
     n = g_malloc0(sizeof(NBDCloseNotifier));
     n->n.notify = nbd_close_notifier;
     n->exp = exp;
     bdrv_add_close_notifier(bs, &n->n);
     QTAILQ_INSERT_TAIL(&close_notifiers, n, next);
+    return;
 }
 
 void qmp_nbd_server_stop(Error **errp)
diff --git a/hmp.c b/hmp.c
index c45514b..5cc97fe 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1440,7 +1440,8 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
             continue;
         }
 
-        qmp_nbd_server_add(info->value->device, true, writable, &local_err);
+        qmp_nbd_server_add(info->value->device, true, writable, false, false,
+                           &local_err);
 
         if (local_err != NULL) {
             qmp_nbd_server_stop(NULL);
@@ -1460,7 +1461,7 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict)
     int writable = qdict_get_try_bool(qdict, "writable", 0);
     Error *local_err = NULL;
 
-    qmp_nbd_server_add(device, true, writable, &local_err);
+    qmp_nbd_server_add(device, true, writable, false, false, &local_err);
 
     if (local_err != NULL) {
         hmp_handle_error(mon, &local_err);
diff --git a/qapi-schema.json b/qapi-schema.json
index f82d829..bfdbe33 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3225,7 +3225,8 @@
 #
 # Since: 1.3.0
 ##
-{ 'command': 'nbd-server-add', 'data': {'device': 'str', '*writable': 'bool'} }
+{ 'command': 'nbd-server-add', 'data': {'device': 'str', '*writable': 'bool',
+                                        '*snapshot': 'bool'} }
 
 ##
 # @nbd-server-stop:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 2e59b0d..e398d88 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2871,7 +2871,7 @@ EQMP
     },
     {
         .name       = "nbd-server-add",
-        .args_type  = "device:B,writable:b?",
+        .args_type  = "device:B,writable:b?,snapshot:b?",
         .mhandler.cmd_new = qmp_marshal_input_nbd_server_add,
     },
     {
-- 
1.8.3.4

[Qemu-devel] [RFC PATCH 4/4] hmp: add -s switch to nbd_server_add

[Fam Zheng]

The switch is equivalent to "snapshot=on" option in QMP nbd-server-add.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hmp-commands.hx | 4 ++--
 hmp.c           | 8 +++++---
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 8c6b91a..2cbd717 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1382,8 +1382,8 @@ ETEXI
 
     {
         .name       = "nbd_server_add",
-        .args_type  = "writable:-w,device:B",
-        .params     = "nbd_server_add [-w] device",
+        .args_type  = "writable:-w,snapshot:-s,device:B",
+        .params     = "nbd_server_add [-w] [-s] device",
         .help       = "export a block device via NBD",
         .mhandler.cmd = hmp_nbd_server_add,
     },
diff --git a/hmp.c b/hmp.c
index 5cc97fe..03d668b 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1440,8 +1440,8 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
             continue;
         }
 
-        qmp_nbd_server_add(info->value->device, true, writable, false, false,
-                           &local_err);
+        qmp_nbd_server_add(info->value->device, true, writable,
+                           true, false, &local_err);
 
         if (local_err != NULL) {
             qmp_nbd_server_stop(NULL);
@@ -1459,9 +1459,11 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict)
 {
     const char *device = qdict_get_str(qdict, "device");
     int writable = qdict_get_try_bool(qdict, "writable", 0);
+    int snapshot = qdict_get_try_bool(qdict, "snapshot", 0);
     Error *local_err = NULL;
 
-    qmp_nbd_server_add(device, true, writable, false, false, &local_err);
+    qmp_nbd_server_add(device, true, writable,
+                       true, snapshot, &local_err);
 
     if (local_err != NULL) {
         hmp_handle_error(mon, &local_err);
-- 
1.8.3.4

Re: [Qemu-devel] [RFC PATCH 2/4] nbd: call drive_put_ref() only if dinfo exists

[Stefan Hajnoczi]

On Mon, Jul 29, 2013 at 12:25:30PM +0800, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockdev-nbd.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index 95f10c8..c75df19 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -72,7 +72,10 @@ static void nbd_close_notifier(Notifier *n, void *data)
>  static void nbd_server_put_ref(NBDExport *exp)
>  {
>      BlockDriverState *bs = nbd_export_get_blockdev(exp);
> -    drive_put_ref(drive_get_by_blockdev(bs));
> +    DriveInfo *dinfo = drive_get_by_blockdev(bs);
> +    if (dinfo) {
> +        drive_put_ref(dinfo);
> +    }

This doesn't make sense to me.  If we hold a reference, we must release
it.

Once your refcount series is applied, NBD should be able to hold just
the BDS refcount.  Then we don't care about DriveInfo at all anymore
besides looking up the BDS when the user creates an export.

Stefan

Re: [Qemu-devel] [RFC PATCH 3/4] qmp: Add "snapshot=" option to nbd-server-add

[Stefan Hajnoczi]

On Mon, Jul 29, 2013 at 12:25:31PM +0800, Fam Zheng wrote:
> +/* create a point-in-time snapshot BDS from an existing BDS */
> +static BlockDriverState *nbd_create_snapshot(BlockDriverState *orig_bs)
> +{
> +    int ret;
> +    char filename[1024];
> +    BlockDriver *drv;
> +    BlockDriverState *bs;
> +    QEMUOptionParameter *options;
> +    Error *local_err = NULL;
> +
> +    bs = bdrv_new("");
> +    ret = get_tmp_filename(filename, sizeof(filename));
> +    if (ret < 0) {
> +        goto err;

unlink(filename) is not safe if this function returns an error.

It is simpler if you get the temporary filename first and then do
bdrv_new().

> +    }
> +    drv = bdrv_find_format("qcow2");
> +    if (drv < 0) {
> +        goto err;
> +    }
> +    options = parse_option_parameters("", drv->create_options, NULL);
> +    set_option_parameter_int(options, BLOCK_OPT_SIZE, bdrv_getlength(orig_bs));
> +
> +    ret = bdrv_create(drv, filename, options);

This is handy but only works if the QEMU process has permission to
create temporary files.

>      n = g_malloc0(sizeof(NBDCloseNotifier));
>      n->n.notify = nbd_close_notifier;
>      n->exp = exp;
>      bdrv_add_close_notifier(bs, &n->n);
>      QTAILQ_INSERT_TAIL(&close_notifiers, n, next);
> +    return;

Please drop void return.

> diff --git a/qapi-schema.json b/qapi-schema.json
> index f82d829..bfdbe33 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3225,7 +3225,8 @@
>  #
>  # Since: 1.3.0
>  ##
> -{ 'command': 'nbd-server-add', 'data': {'device': 'str', '*writable': 'bool'} }
> +{ 'command': 'nbd-server-add', 'data': {'device': 'str', '*writable': 'bool',
> +                                        '*snapshot': 'bool'} }
>  
>  ##
>  # @nbd-server-stop:
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 2e59b0d..e398d88 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2871,7 +2871,7 @@ EQMP
>      },
>      {
>          .name       = "nbd-server-add",
> -        .args_type  = "device:B,writable:b?",
> +        .args_type  = "device:B,writable:b?,snapshot:b?",
>          .mhandler.cmd_new = qmp_marshal_input_nbd_server_add,
>      },

Missing documentation for the new option.

Re: [Qemu-devel] [RFC PATCH 1/4] block/backup: delete target after completion callback

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 463 bytes --]

On 07/28/2013 10:25 PM, Fam Zheng wrote:
> Move bdrv_delete(target) one line down to give block job caller a chance
> to handle target on completion before deleting it.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/backup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [RFC PATCH 3/4] qmp: Add "snapshot=" option to nbd-server-add

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 535 bytes --]

On 08/21/2013 07:02 AM, Stefan Hajnoczi wrote:

>> +
>> +    ret = bdrv_create(drv, filename, options);
> 
> This is handy but only works if the QEMU process has permission to
> create temporary files.

And in the case of libvirt driving qemu under sVirt, qemu does NOT have
permission to create temp files.  Does that mean that libvirt still has
to use the long-hand pre-creation rather than this new shorthand?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [RFC PATCH 3/4] qmp: Add "snapshot=" option to nbd-server-add

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1306 bytes --]

On 07/28/2013 10:25 PM, Fam Zheng wrote:
> With drive-backup block job, we can have a point-in-time snapshot of a
> device. With snapshot=on, a backup block job is started on the device to
> do CoW to a temporary image and this image is exported to nbd. The image
> is deleted after nbd server stops.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockdev-nbd.c   | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  hmp.c            |  5 ++--
>  qapi-schema.json |  3 ++-
>  qmp-commands.hx  |  2 +-
>  4 files changed, 80 insertions(+), 6 deletions(-)

In addition to Stefan's comments about missing docs...

> +++ b/qapi-schema.json
> @@ -3225,7 +3225,8 @@
>  #
>  # Since: 1.3.0
>  ##
> -{ 'command': 'nbd-server-add', 'data': {'device': 'str', '*writable': 'bool'} }
> +{ 'command': 'nbd-server-add', 'data': {'device': 'str', '*writable': 'bool',
> +                                        '*snapshot': 'bool'} }

When documenting the new option, be sure to use a (since 1.7) tag.
Also, it would be nice to get introspection in (otherwise, libvirt
cannot learn whether '*snapshot' is supported without trying and failing
on older qemu).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [RFC PATCH 3/4] qmp: Add "snapshot=" option to nbd-server-add

[Fam Zheng]

On Wed, 08/21 15:02, Stefan Hajnoczi wrote:
> On Mon, Jul 29, 2013 at 12:25:31PM +0800, Fam Zheng wrote:
> > +/* create a point-in-time snapshot BDS from an existing BDS */
> > +static BlockDriverState *nbd_create_snapshot(BlockDriverState *orig_bs)
> > +{
> > +    int ret;
> > +    char filename[1024];
> > +    BlockDriver *drv;
> > +    BlockDriverState *bs;
> > +    QEMUOptionParameter *options;
> > +    Error *local_err = NULL;
> > +
> > +    bs = bdrv_new("");
> > +    ret = get_tmp_filename(filename, sizeof(filename));
> > +    if (ret < 0) {
> > +        goto err;
> 
> unlink(filename) is not safe if this function returns an error.
> 
> It is simpler if you get the temporary filename first and then do
> bdrv_new().
> 
> > +    }
> > +    drv = bdrv_find_format("qcow2");
> > +    if (drv < 0) {
> > +        goto err;
> > +    }
> > +    options = parse_option_parameters("", drv->create_options, NULL);
> > +    set_option_parameter_int(options, BLOCK_OPT_SIZE, bdrv_getlength(orig_bs));
> > +
> > +    ret = bdrv_create(drv, filename, options);
> 
> This is handy but only works if the QEMU process has permission to
> create temporary files.
> 
Yes, this is a shortcut, it has this limitation, but the good side is
it's easy to get and no other dependency.

To avoid creating file, we'll need blockdev-add to override backing_hd,
and blockdev-backup to use an existing BDS as target, then we simply use
current nbd-server-add to export it.

This series is still RFC, with above said, we still need to decide which
is the way we (QEMU and libvirt) want for 1.7.  any thoughts?

Thanks
Fam

Re: [Qemu-devel] [RFC PATCH 3/4] qmp: Add "snapshot=" option to nbd-server-add

[Paolo Bonzini]

Il 22/08/2013 10:53, Fam Zheng ha scritto:
>> > This is handy but only works if the QEMU process has permission to
>> > create temporary files.
>> > 
> Yes, this is a shortcut, it has this limitation, but the good side is
> it's easy to get and no other dependency.
> 
> To avoid creating file, we'll need blockdev-add to override backing_hd,
> and blockdev-backup to use an existing BDS as target, then we simply use
> current nbd-server-add to export it.
> 
> This series is still RFC, with above said, we still need to decide which
> is the way we (QEMU and libvirt) want for 1.7.  any thoughts?

I think this was the initial design, but Kevin already said he didn't
like the idea.

(Also, if you do this you have to add nbd-server-add to qmp-transaction,
to atomically start fleecing of multiple devices).

Paolo

Re: [Qemu-devel] [RFC PATCH 3/4] qmp: Add "snapshot=" option to nbd-server-add

[Fam Zheng]

On Thu, 08/22 11:22, Paolo Bonzini wrote:
> Il 22/08/2013 10:53, Fam Zheng ha scritto:
> >> > This is handy but only works if the QEMU process has permission to
> >> > create temporary files.
> >> > 
> > Yes, this is a shortcut, it has this limitation, but the good side is
> > it's easy to get and no other dependency.
> > 
> > To avoid creating file, we'll need blockdev-add to override backing_hd,
> > and blockdev-backup to use an existing BDS as target, then we simply use
> > current nbd-server-add to export it.
> > 
> > This series is still RFC, with above said, we still need to decide which
> > is the way we (QEMU and libvirt) want for 1.7.  any thoughts?
> 
> I think this was the initial design, but Kevin already said he didn't
> like the idea.
> 
> (Also, if you do this you have to add nbd-server-add to qmp-transaction,
> to atomically start fleecing of multiple devices).
> 
Rigth. And this stands the same as for blockdev-backup.

Fam

Re: [Qemu-devel] [RFC PATCH 3/4] qmp: Add "snapshot=" option to nbd-server-add

[Fam Zheng]

On Thu, 08/22 11:22, Paolo Bonzini wrote:
> Il 22/08/2013 10:53, Fam Zheng ha scritto:
> >> > This is handy but only works if the QEMU process has permission to
> >> > create temporary files.
> >> > 
> > Yes, this is a shortcut, it has this limitation, but the good side is
> > it's easy to get and no other dependency.
> > 
> > To avoid creating file, we'll need blockdev-add to override backing_hd,
> > and blockdev-backup to use an existing BDS as target, then we simply use
> > current nbd-server-add to export it.
> > 
> > This series is still RFC, with above said, we still need to decide which
> > is the way we (QEMU and libvirt) want for 1.7.  any thoughts?
> 
> I think this was the initial design, but Kevin already said he didn't
> like the idea.
> 
> (Also, if you do this you have to add nbd-server-add to qmp-transaction,
> to atomically start fleecing of multiple devices).
> 
Kevin,

The downside of this is unresolvable (create file in QEMU), so I am
giving up this and back to blockdev-add & blockdev-backup interface.

So I guess I'm depending on your blockdev-add command patches, do you
have a solution for overriding backing_hd there yet? If yes, do you have
a working branch that I can rebase my blockdev-backup onto?

Thanks.
Fam