| Message ID | 1375071932-31627-4-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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?
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).
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
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
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
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
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, }, {
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(-)