| Message ID | 1334334198-30899-6-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/13/2012 10:23 AM, Paolo Bonzini wrote: > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > blockdev.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- > hmp-commands.hx | 21 +++++++++++ > hmp.c | 26 ++++++++++++++ > hmp.h | 1 + > qapi-schema.json | 30 ++++++++++++++++ > qmp-commands.hx | 36 +++++++++++++++++++ > trace-events | 2 +- > 7 files changed, 214 insertions(+), 4 deletions(-) > > + if (!has_format) { > + format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : "qcow2"; > + } Do we always want to default to qcow2, or should we be defaulting to the same format as the file we are copying? For example, if I have a raw source, and don't pass in format, wouldn't it make more sense for the destination to also default to raw, instead of qcow2? Same if the source is qed, shouldn't this default to a qed copy? > + case NEW_IMAGE_MODE_ABSOLUTE_PATHS: > + ret = bdrv_img_create(target, format, > + source->filename, > + source->drv->format_name, > + NULL, -1, flags); > + break; > + default: > + abort(); > + } > + } > + > + if (ret) { > + error_set(errp, QERR_OPEN_FILE_FAILED, target); > + return; > + } In this first failure, we've leaked nothing... > + > + /* Grab a reference so hotplug does not delete the BlockDriverState > + * from underneath us. > + */ > + drive_get_ref(drive_get_by_blockdev(bs)); > + ret = mirror_start(bs, target, drv, flags, > + block_job_cb, bs, full); > + if (ret < 0) { > + error_set(errp, QERR_OPEN_FILE_FAILED, target); > + } But here, you have the same error message, and I don't see anything that closed target (unless mirror_start does that). Leak? > +++ b/hmp-commands.hx > @@ -901,6 +901,27 @@ Snapshot device, using snapshot file as target if provided > ETEXI > > { > + .name = "drive_mirror", > + .args_type = "reuse:-n,full:-f,device:B,target:s,format:s?", > + .params = "[-n] [-f] device target [format]", > + .help = "initiates live storage\n\t\t\t" > + "migration for a device. New writes are mirrored to the\n\t\t\t" > + "specified new image file, and the block_stream\n\t\t\t" > + "command can then be started.\n\t\t\t" Outdated help message. > +++ b/qapi-schema.json > @@ -1245,6 +1245,36 @@ > 'returns': 'str' } > > ## > +# @drive-mirror > +# > +# Start mirroring a block device's writes to a new destination. > +# > +# @device: the name of the device whose writes should be mirrored. > +# > +# @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. > +# > +# @format: the format of the new destination. Mark this #optional, and mention how it defaults to probing for 'mode':'existing' and to qcow2 for all other modes. > +# > +# @mode: #optional whether and how QEMU should create a new image, default is > +# 'absolute-paths'. > +# > +# @full: whether the whole disk should be copied to the destination, or > +# only the topmost image. > +# > +# Returns: nothing on success > +# If @device is not a valid block device, DeviceNotFound > +# If @target can't be opened, OpenFileFailed > +# If @format is invalid, InvalidBlockFormat > +# > +# Since 1.1 > +## > +{ 'command': 'drive-mirror', > + 'data': { 'device': 'str', 'target': 'str', '*format': 'str', > + 'full': 'bool', '*mode': 'NewImageMode'} } I'm wondering if we should make 'full' optional (default to false). After all, in the original blkmirror design for adding 'drive-mirror' to 'transaction', there was no 'full' parameter, and a transaction of blockdev-snapshot-sync/drive-mirror on the same device depended on a shallow copy. I can live with it being mandatory, though. Per my comments on 0/8, should we be adding an optional '*base': 'str' (even if we don't implement it yet) so that you can get the full functionality of 'block-stream' in a single 'drive-mirror' command rather than having to do two separate pulls to achieve a partial copy? > +SQMP > +drive-mirror > +------------ > + > +Start mirroring a block device's writes to a new destination. target > +specifies the target of the new image. If the file exists, or if it is > +a device, it will be used as the new destination for writes. If does not > +exist, a new file will be created. format specifies the format of the > +mirror image, default is to probe if mode='existing', else qcow2. > + > +Arguments: > + > +- "device": device name to operate on (json-string) > +- "target": name of new image file (json-string) > +- "format": format of new image (json-string) (json-string, optional)
On 04/13/2012 10:23 AM, Paolo Bonzini wrote: > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > + flags = bs->open_flags | BDRV_O_RDWR; > + source = bs->backing_hd; > + /* create new image w/backing file */ > + if (full && mode != NEW_IMAGE_MODE_EXISTING) { > + assert(format && drv); > + bdrv_get_geometry(bs, &size); > + size *= 512; > + ret = bdrv_img_create(target, format, > + NULL, NULL, NULL, size, flags); This side of the branch forces the size (which makes sense, since there is no backing file to probe it from),... > + } else { > + switch (mode) { > + case NEW_IMAGE_MODE_EXISTING: > + ret = 0; > + break; > + case NEW_IMAGE_MODE_ABSOLUTE_PATHS: > + ret = bdrv_img_create(target, format, > + source->filename, > + source->drv->format_name, > + NULL, -1, flags); ...but this side passes -1 (which I assume means to probe the backing file for its size, and reuse that size). But is this always right, or shouldn't this side of the branch _also_ be calling bdrv_get_geometry and passing in an explicit size? Should we be using BDRV_O_NO_BACKING to avoid potential problems in temporarily reopening a file that we already have open due to source? Am I correct that bdrv_img_create will fail if I attempt to use a driver that doesn't support backing files (think raw) but include a request to set the backing file? (Put another way, I'm wondering whether libvirt can trust qemu to do error detection, or whether libvirt must filter out: virDomainBlockRebase(, _COPY | _SHALLOW | _COPY_RAW) as reasonable on an existing raw image, but as an error if the existing image already uses a backing file. Just to make sure: if I pass in an existing file, then it is expected that the data in that image is either identical to the data of the backing file (if full==false) or is conceptually uninitialized (if fall=true). We document that there might be rare cases of wanting to alter contents - I'm assuming that one such case would be to pass in an existing file with a larger size than the source, such that when we then do disk-reopen, we've achieved a poor-man's block_resize. Does the code react well to the existing destination having a different size than the source?
Il 14/04/2012 01:08, Eric Blake ha scritto: > On 04/13/2012 10:23 AM, Paolo Bonzini wrote: >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- > >> + flags = bs->open_flags | BDRV_O_RDWR; >> + source = bs->backing_hd; > >> + /* create new image w/backing file */ >> + if (full && mode != NEW_IMAGE_MODE_EXISTING) { >> + assert(format && drv); >> + bdrv_get_geometry(bs, &size); >> + size *= 512; >> + ret = bdrv_img_create(target, format, >> + NULL, NULL, NULL, size, flags); > > This side of the branch forces the size (which makes sense, since there > is no backing file to probe it from),... > >> + } else { >> + switch (mode) { >> + case NEW_IMAGE_MODE_EXISTING: >> + ret = 0; >> + break; >> + case NEW_IMAGE_MODE_ABSOLUTE_PATHS: >> + ret = bdrv_img_create(target, format, >> + source->filename, >> + source->drv->format_name, >> + NULL, -1, flags); > > ...but this side passes -1 (which I assume means to probe the backing > file for its size, and reuse that size). But is this always right, or > shouldn't this side of the branch _also_ be calling bdrv_get_geometry > and passing in an explicit size? This could be an idea---and for blockdev_snapshot_sync too, where it would avoid a useless probe and opening the same file twice. It's not a bigger problem than for snapshots, especially since now we open the file for read only. It could be a small problem in case the source backing file is smaller than the source itself. I don't know if that's possible. > Should we be using BDRV_O_NO_BACKING > to avoid potential problems in temporarily reopening a file that we > already have open due to source? Note that bdrv_img_create does not use BDRV_O_NO_BACKING and does not open the backing file other than for probing. mirror_start does use BDRV_O_NO_BACKING. > Am I correct that bdrv_img_create will fail if I attempt to use a driver > that doesn't support backing files (think raw) but include a request to > set the backing file? (Put another way, I'm wondering whether libvirt > can trust qemu to do error detection, or whether libvirt must filter out: > virDomainBlockRebase(, _COPY | _SHALLOW | _COPY_RAW) > as reasonable on an existing raw image, but as an error if the existing > image already uses a backing file. QEMU will fail here, if I understand what you mean: if (base_filename) { if (set_option_parameter(param, BLOCK_OPT_BACKING_FILE, base_filename)) { error_report("Backing file not supported for file format '%s'", fmt); ret = -EINVAL; goto out; } } > Just to make sure: if I pass in an existing file, then it is expected > that the data in that image is either identical to the data of the > backing file (if full==false) or is conceptually uninitialized (if > fall=true). We document that there might be rare cases of wanting to > alter contents - I'm assuming that one such case would be to pass in an > existing file with a larger size than the source, such that when we then > do disk-reopen, we've achieved a poor-man's block_resize. Does the code > react well to the existing destination having a different size than the > source? It doesn't care. It will also allow a smaller destination as long as the "missing" sectors are unallocated in the source. Paolo
diff --git a/blockdev.c b/blockdev.c index 748e192..ce2f2f6 100644 --- a/blockdev.c +++ b/blockdev.c @@ -20,6 +20,8 @@ #include "trace.h" #include "arch_init.h" +static void block_job_cb(void *opaque, int ret); + static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives); static const char *const if_name[IF_COUNT] = { @@ -831,6 +833,100 @@ exit: return; } +void qmp_drive_mirror(const char *device, const char *target, + bool has_format, const char *format, bool full, + bool has_mode, enum NewImageMode mode, Error **errp) +{ + BlockDriverState *bs; + BlockDriverState *source; + BlockDriver *proto_drv; + BlockDriver *drv = NULL; + int flags; + uint64_t size; + int ret; + + if (!has_mode) { + mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS; + } + if (!has_format) { + format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : "qcow2"; + } + if (format) { + drv = bdrv_find_format(format); + if (!drv) { + error_set(errp, QERR_INVALID_BLOCK_FORMAT, format); + return; + } + } + + 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; + } + + flags = bs->open_flags | BDRV_O_RDWR; + source = bs->backing_hd; + if (!source) { + full = true; + } + + proto_drv = bdrv_find_protocol(target); + if (!proto_drv) { + error_set(errp, QERR_INVALID_BLOCK_FORMAT, format); + return; + } + + /* create new image w/backing file */ + if (full && mode != NEW_IMAGE_MODE_EXISTING) { + assert(format && drv); + bdrv_get_geometry(bs, &size); + size *= 512; + ret = bdrv_img_create(target, format, + NULL, NULL, NULL, size, flags); + } else { + switch (mode) { + case NEW_IMAGE_MODE_EXISTING: + ret = 0; + break; + case NEW_IMAGE_MODE_ABSOLUTE_PATHS: + ret = bdrv_img_create(target, format, + source->filename, + source->drv->format_name, + NULL, -1, flags); + break; + default: + abort(); + } + } + + if (ret) { + error_set(errp, QERR_OPEN_FILE_FAILED, target); + return; + } + + /* Grab a reference so hotplug does not delete the BlockDriverState + * from underneath us. + */ + drive_get_ref(drive_get_by_blockdev(bs)); + ret = mirror_start(bs, target, drv, flags, + block_job_cb, bs, full); + if (ret < 0) { + error_set(errp, QERR_OPEN_FILE_FAILED, target); + } +} + + static void eject_device(BlockDriverState *bs, int force, Error **errp) { @@ -1069,12 +1165,12 @@ static QObject *qobject_from_block_job(BlockJob *job) job->speed); } -static void block_stream_cb(void *opaque, int ret) +static void block_job_cb(void *opaque, int ret) { BlockDriverState *bs = opaque; QObject *obj; - trace_block_stream_cb(bs, bs->job, ret); + trace_block_job_cb(bs, bs->job, ret); assert(bs->job); obj = qobject_from_block_job(bs->job); @@ -1114,7 +1210,7 @@ void qmp_block_stream(const char *device, bool has_base, } } - ret = stream_start(bs, base_bs, base, block_stream_cb, bs); + ret = stream_start(bs, base_bs, base, block_job_cb, bs); if (ret < 0) { switch (ret) { case -EBUSY: diff --git a/hmp-commands.hx b/hmp-commands.hx index a6f5a84..36f4ef9 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -901,6 +901,27 @@ Snapshot device, using snapshot file as target if provided ETEXI { + .name = "drive_mirror", + .args_type = "reuse:-n,full:-f,device:B,target:s,format:s?", + .params = "[-n] [-f] device target [format]", + .help = "initiates live storage\n\t\t\t" + "migration for a device. New writes are mirrored to the\n\t\t\t" + "specified new image file, and the block_stream\n\t\t\t" + "command can then be started.\n\t\t\t" + "The -n flag requests QEMU to reuse the image found\n\t\t\t" + "in new-image-file, instead of recreating it from scratch.\n\t\t\t" + "The -f flag requests QEMU to copy the whole disk,\n\t\t\t" + "so that the result does not need a backing file.\n\t\t\t", + .mhandler.cmd = hmp_drive_mirror, + }, +STEXI +@item drive_mirror +@findex drive_mirror +Start mirroring a block device's writes to a new destination, +using the specified target. +ETEXI + + { .name = "drive_add", .args_type = "pci_addr:s,opts:s", .params = "[[<domain>:]<bus>:]<slot>\n" diff --git a/hmp.c b/hmp.c index f3e5163..e7290c9 100644 --- a/hmp.c +++ b/hmp.c @@ -688,6 +688,32 @@ void hmp_block_resize(Monitor *mon, const QDict *qdict) hmp_handle_error(mon, &errp); } +void hmp_drive_mirror(Monitor *mon, const QDict *qdict) +{ + const char *device = qdict_get_str(qdict, "device"); + const char *filename = qdict_get_str(qdict, "target"); + const char *format = qdict_get_str(qdict, "format"); + int reuse = qdict_get_try_bool(qdict, "reuse", 0); + int full = qdict_get_try_bool(qdict, "full", 0); + enum NewImageMode mode; + Error *errp = NULL; + + if (!filename) { + error_set(&errp, QERR_MISSING_PARAMETER, "target"); + hmp_handle_error(mon, &errp); + return; + } + + if (reuse) { + mode = NEW_IMAGE_MODE_EXISTING; + } else { + mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS; + } + + qmp_drive_mirror(device, filename, !!format, format, full, true, mode, &errp); + hmp_handle_error(mon, &errp); +} + void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict) { const char *device = qdict_get_str(qdict, "device"); diff --git a/hmp.h b/hmp.h index 443b812..64bcaa6 100644 --- a/hmp.h +++ b/hmp.h @@ -48,6 +48,7 @@ void hmp_block_passwd(Monitor *mon, const QDict *qdict); void hmp_balloon(Monitor *mon, const QDict *qdict); void hmp_block_resize(Monitor *mon, const QDict *qdict); void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict); +void hmp_drive_mirror(Monitor *mon, const QDict *qdict); void hmp_migrate_cancel(Monitor *mon, const QDict *qdict); void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict); void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict); diff --git a/qapi-schema.json b/qapi-schema.json index ace55f3..c772bc3 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1245,6 +1245,36 @@ 'returns': 'str' } ## +# @drive-mirror +# +# Start mirroring a block device's writes to a new destination. +# +# @device: the name of the device whose writes should be mirrored. +# +# @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. +# +# @format: the format of the new destination. +# +# @mode: #optional whether and how QEMU should create a new image, default is +# 'absolute-paths'. +# +# @full: whether the whole disk should be copied to the destination, or +# only the topmost image. +# +# Returns: nothing on success +# If @device is not a valid block device, DeviceNotFound +# If @target can't be opened, OpenFileFailed +# If @format is invalid, InvalidBlockFormat +# +# Since 1.1 +## +{ 'command': 'drive-mirror', + 'data': { 'device': 'str', 'target': 'str', '*format': 'str', + 'full': 'bool', '*mode': 'NewImageMode'} } + +## # @migrate_cancel # # Cancel the current executing migration process. diff --git a/qmp-commands.hx b/qmp-commands.hx index 4d4c749..a51125e 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -792,6 +792,42 @@ Example: EQMP { + .name = "drive-mirror", + .args_type = "full:-n,device:B,target:s,mode:s?,format:s?", + .mhandler.cmd_new = qmp_marshal_input_drive_mirror, + }, + +SQMP +drive-mirror +------------ + +Start mirroring a block device's writes to a new destination. target +specifies the target of the new image. If the file exists, or if it is +a device, it will be used as the new destination for writes. If does not +exist, a new file will be created. format specifies the format of the +mirror image, default is to probe if mode='existing', else qcow2. + +Arguments: + +- "device": device name to operate on (json-string) +- "target": name of new image file (json-string) +- "format": format of new image (json-string) +- "mode": how an image file should be created into the target + file/device (NewImageMode, optional, default 'absolute-paths') +- "full": whether the whole disk should be copied to the destination, + or only the topmost image (json-bool) + +Example: + +-> { "execute": "drive-mirror", "arguments": { "device": "ide-hd0", + "target": "/some/place/my-image", + "full": false, + "format": "qcow2" } } +<- { "return": {} } + +EQMP + + { .name = "balloon", .args_type = "value:M", .mhandler.cmd_new = qmp_marshal_input_balloon, diff --git a/trace-events b/trace-events index 23aad83..ec268da 100644 --- a/trace-events +++ b/trace-events @@ -81,7 +81,7 @@ stream_start(void *bs, void *base, void *s, void *co, void *opaque) "bs %p base # blockdev.c qmp_block_job_cancel(void *job) "job %p" -block_stream_cb(void *bs, void *job, int ret) "bs %p job %p ret %d" +block_job_cb(void *bs, void *job, int ret) "bs %p job %p ret %d" qmp_block_stream(void *bs, void *job) "bs %p job %p" # hw/virtio-blk.c
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- blockdev.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- hmp-commands.hx | 21 +++++++++++ hmp.c | 26 ++++++++++++++ hmp.h | 1 + qapi-schema.json | 30 ++++++++++++++++ qmp-commands.hx | 36 +++++++++++++++++++ trace-events | 2 +- 7 files changed, 214 insertions(+), 4 deletions(-)