Patchwork [RFC,3/4] qmp: Add "snapshot=" option to nbd-server-add

login
register
mail settings
Submitter Fam Zheng
Date July 29, 2013, 4:25 a.m.
Message ID <1375071932-31627-4-git-send-email-famz@redhat.com>
Download mbox | patch
Permalink /patch/262665/
State New
Headers show

Comments

Fam Zheng - July 29, 2013, 4:25 a.m.
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(-)
Stefan Hajnoczi - Aug. 21, 2013, 1:02 p.m.
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.
Eric Blake - Aug. 21, 2013, 6:40 p.m.
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 - Aug. 21, 2013, 6:42 p.m.
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).
Fam Zheng - Aug. 22, 2013, 8:53 a.m.
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
Paolo Bonzini - Aug. 22, 2013, 9:22 a.m.
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
Fam Zheng - Aug. 22, 2013, 9:42 a.m.
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
Fam Zheng - Aug. 26, 2013, 8:07 a.m.
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

Patch

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,
     },
     {