| Message ID | 1410411902-7104-2-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
Fam Zheng <famz@redhat.com> writes: > Similar to drive-backup, but this command uses a device id as target > instead of creating/opening an image file. > > Also add blocker on target bs, since the target is also a named device > now. > > Add check and report error for bs == target which became possible but is > an illegal case with introduction of blockdev-backup. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block/backup.c | 26 ++++++++++++++++++++++++++ > blockdev.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ > qapi/block-core.json | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ > qmp-commands.hx | 44 ++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 167 insertions(+) > > diff --git a/block/backup.c b/block/backup.c > index d0b0225..24e8ffc 100644 > --- a/block/backup.c > +++ b/block/backup.c > @@ -344,6 +344,7 @@ static void coroutine_fn backup_run(void *opaque) > hbitmap_free(job->bitmap); > > bdrv_iostatus_disable(target); > + bdrv_op_unblock_all(target, job->common.blocker); > bdrv_unref(target); > > block_job_completed(&job->common, ret); > @@ -362,6 +363,11 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, > assert(target); > assert(cb); > > + if (bs == target) { > + error_setg(errp, "Source and target cannot be the same"); > + return; > + } > + > if ((on_source_error == BLOCKDEV_ON_ERROR_STOP || > on_source_error == BLOCKDEV_ON_ERROR_ENOSPC) && > !bdrv_iostatus_is_enabled(bs)) { > @@ -369,6 +375,24 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, > return; > } > > + if (!bdrv_is_inserted(bs)) { > + error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bs->device_name); > + return; > + } > + > + if (!bdrv_is_inserted(target)) { > + error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, target->device_name); > + return; > + } > + > + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) { > + return; > + } > + > + if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_BACKUP_TARGET, errp)) { > + return; > + } > + > len = bdrv_getlength(bs); > if (len < 0) { > error_setg_errno(errp, -len, "unable to get length for '%s'", > @@ -382,6 +406,8 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, > return; > } > > + bdrv_op_block_all(target, job->common.blocker); > + > job->on_source_error = on_source_error; > job->on_target_error = on_target_error; > job->target = target; > diff --git a/blockdev.c b/blockdev.c > index e919566..516de7f 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -2046,6 +2046,8 @@ void qmp_drive_backup(const char *device, const char *target, > return; > } > > + /* Although backup_run has this check too, we need to use bs->drv below, so > + * do an early check redundantly. */ > if (!bdrv_is_inserted(bs)) { > error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device); > return; > @@ -2062,6 +2064,7 @@ void qmp_drive_backup(const char *device, const char *target, > } > } > > + /* Early check to avoid creating target */ > if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) { > return; > } > @@ -2124,6 +2127,50 @@ BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp) > return bdrv_named_nodes_list(); > } > > +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; > + } > + > + target_bs = bdrv_find(target); > + if (!target_bs) { > + error_set(errp, QERR_DEVICE_NOT_FOUND, 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) { > + bdrv_unref(target_bs); > + 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/block-core.json b/qapi/block-core.json > index a685d02..b953c7b 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -669,6 +669,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. Changes from DriveBackup: * @target is a device name, i.e. the name stored in BDS member device_name[]. * Therfore, @format and @mode don't apply, and are dropped. Good. > +# > +# Since: 2.2 > +## > +{ 'type': 'BlockdevBackup', > + 'data': { 'device': 'str', 'target': 'str', > + 'sync': 'MirrorSyncMode', > + '*speed': 'int', > + '*on-source-error': 'BlockdevOnError', > + '*on-target-error': 'BlockdevOnError' } } > + > +## > # @blockdev-snapshot-sync > # > # Generates a synchronous snapshot of a block device. > @@ -788,6 +822,22 @@ > { '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. blockdev-backup is the modern command we want people to use, drive-backup is the old one we provide for compatibility, right? If yes, blockdev-backup should be documented without referring to drive-backup. It's okay for drive-backup's documentation to refer to block-backup, though. > +# > +# Returns: Nothing on success. > +# If @device or @target is not a valid block device, DeviceNotFound. Do we really want error classes other than GenericError in new code without a clear need? > +# > +# Since 2.2 > +## > +{ 'command': 'blockdev-backup', 'data': 'BlockdevBackup' } > + > + > +## > # @query-named-block-nodes > # > # Get the named block driver list > diff --git a/qmp-commands.hx b/qmp-commands.hx > index 7658d4b..fa40260 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -1094,6 +1094,50 @@ 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) Copied from drive-backup without change. Please fix. > +- "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). > +- "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 > > {
Am 11.09.2014 um 07:05 hat Fam Zheng geschrieben: > Similar to drive-backup, but this command uses a device id as target > instead of creating/opening an image file. > > Also add blocker on target bs, since the target is also a named device > now. > > Add check and report error for bs == target which became possible but is > an illegal case with introduction of blockdev-backup. > > Signed-off-by: Fam Zheng <famz@redhat.com> > diff --git a/qapi/block-core.json b/qapi/block-core.json > index a685d02..b953c7b 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -669,6 +669,40 @@ > '*on-target-error': 'BlockdevOnError' } } > > ## > +# @BlockdevBackup > +# > +# @device: the name of the device which should be copied. > +# > +# @target: the name of the backup target device. Both of these are either a BlockBackend ID or a BDS node-name, right? Do we have a standard way of expressing this? "name of the device" isn't quite clear. > +# @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: 2.2 > +## > +{ 'type': 'BlockdevBackup', > + 'data': { 'device': 'str', 'target': 'str', > + 'sync': 'MirrorSyncMode', > + '*speed': 'int', > + '*on-source-error': 'BlockdevOnError', > + '*on-target-error': 'BlockdevOnError' } } Kevin
On Fri, 10/31 10:01, Kevin Wolf wrote: > Am 11.09.2014 um 07:05 hat Fam Zheng geschrieben: > > Similar to drive-backup, but this command uses a device id as target > > instead of creating/opening an image file. > > > > Also add blocker on target bs, since the target is also a named device > > now. > > > > Add check and report error for bs == target which became possible but is > > an illegal case with introduction of blockdev-backup. > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > > diff --git a/qapi/block-core.json b/qapi/block-core.json > > index a685d02..b953c7b 100644 > > --- a/qapi/block-core.json > > +++ b/qapi/block-core.json > > @@ -669,6 +669,40 @@ > > '*on-target-error': 'BlockdevOnError' } } > > > > ## > > +# @BlockdevBackup > > +# > > +# @device: the name of the device which should be copied. > > +# > > +# @target: the name of the backup target device. > > Both of these are either a BlockBackend ID or a BDS node-name, right? Do > we have a standard way of expressing this? "name of the device" isn't > quite clear. "name of the device" is used everywhere to document the "device" parameters in our json schema. Since we have BlockBackend now, device-name and node-name could be better distinguished. All we have to do is giving a beautiful name to both. [This patch is only a copy&paste and is consistent with the rest part of the file. So I'll leave it for now :] Fam
Am 03.11.2014 um 02:46 hat Fam Zheng geschrieben: > On Fri, 10/31 10:01, Kevin Wolf wrote: > > Am 11.09.2014 um 07:05 hat Fam Zheng geschrieben: > > > Similar to drive-backup, but this command uses a device id as target > > > instead of creating/opening an image file. > > > > > > Also add blocker on target bs, since the target is also a named device > > > now. > > > > > > Add check and report error for bs == target which became possible but is > > > an illegal case with introduction of blockdev-backup. > > > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > > > > diff --git a/qapi/block-core.json b/qapi/block-core.json > > > index a685d02..b953c7b 100644 > > > --- a/qapi/block-core.json > > > +++ b/qapi/block-core.json > > > @@ -669,6 +669,40 @@ > > > '*on-target-error': 'BlockdevOnError' } } > > > > > > ## > > > +# @BlockdevBackup > > > +# > > > +# @device: the name of the device which should be copied. > > > +# > > > +# @target: the name of the backup target device. > > > > Both of these are either a BlockBackend ID or a BDS node-name, right? Do > > we have a standard way of expressing this? "name of the device" isn't > > quite clear. > > "name of the device" is used everywhere to document the "device" parameters in > our json schema. Since we have BlockBackend now, device-name and node-name > could be better distinguished. All we have to do is giving a beautiful name to > both. > > [This patch is only a copy&paste and is consistent with the rest part of the > file. So I'll leave it for now :] The rest of the file doesn't accept node names. But looking at your actual code, it seems that you are doing the same (by usign bdrv_find() instead of bdrv_lookup_bs()). Shouldn't a proper blockdev-* command accept node names as well? Kevin
On Mon, 11/03 15:32, Kevin Wolf wrote: > Am 03.11.2014 um 02:46 hat Fam Zheng geschrieben: > > On Fri, 10/31 10:01, Kevin Wolf wrote: > > > Am 11.09.2014 um 07:05 hat Fam Zheng geschrieben: > > > > Similar to drive-backup, but this command uses a device id as target > > > > instead of creating/opening an image file. > > > > > > > > Also add blocker on target bs, since the target is also a named device > > > > now. > > > > > > > > Add check and report error for bs == target which became possible but is > > > > an illegal case with introduction of blockdev-backup. > > > > > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > > > > > > diff --git a/qapi/block-core.json b/qapi/block-core.json > > > > index a685d02..b953c7b 100644 > > > > --- a/qapi/block-core.json > > > > +++ b/qapi/block-core.json > > > > @@ -669,6 +669,40 @@ > > > > '*on-target-error': 'BlockdevOnError' } } > > > > > > > > ## > > > > +# @BlockdevBackup > > > > +# > > > > +# @device: the name of the device which should be copied. > > > > +# > > > > +# @target: the name of the backup target device. > > > > > > Both of these are either a BlockBackend ID or a BDS node-name, right? Do > > > we have a standard way of expressing this? "name of the device" isn't > > > quite clear. > > > > "name of the device" is used everywhere to document the "device" parameters in > > our json schema. Since we have BlockBackend now, device-name and node-name > > could be better distinguished. All we have to do is giving a beautiful name to > > both. > > > > [This patch is only a copy&paste and is consistent with the rest part of the > > file. So I'll leave it for now :] > > The rest of the file doesn't accept node names. But looking at your > actual code, it seems that you are doing the same (by usign bdrv_find() > instead of bdrv_lookup_bs()). Yes, to be consistent with drive-backup. > > Shouldn't a proper blockdev-* command accept node names as well? > I am not sure, it's still blockdev-backup, not blocknode-backup. I think that may be another thing, to changed drive-*'s @device parameter, and blockdev-*'s @device and @target to accept node names, altogether. Fam
Fam Zheng <famz@redhat.com> writes: > On Mon, 11/03 15:32, Kevin Wolf wrote: >> Am 03.11.2014 um 02:46 hat Fam Zheng geschrieben: >> > On Fri, 10/31 10:01, Kevin Wolf wrote: >> > > Am 11.09.2014 um 07:05 hat Fam Zheng geschrieben: >> > > > Similar to drive-backup, but this command uses a device id as target >> > > > instead of creating/opening an image file. >> > > > >> > > > Also add blocker on target bs, since the target is also a named device >> > > > now. >> > > > >> > > > Add check and report error for bs == target which became possible but is >> > > > an illegal case with introduction of blockdev-backup. >> > > > >> > > > Signed-off-by: Fam Zheng <famz@redhat.com> >> > > >> > > > diff --git a/qapi/block-core.json b/qapi/block-core.json >> > > > index a685d02..b953c7b 100644 >> > > > --- a/qapi/block-core.json >> > > > +++ b/qapi/block-core.json >> > > > @@ -669,6 +669,40 @@ >> > > > '*on-target-error': 'BlockdevOnError' } } >> > > > >> > > > ## >> > > > +# @BlockdevBackup >> > > > +# >> > > > +# @device: the name of the device which should be copied. >> > > > +# >> > > > +# @target: the name of the backup target device. >> > > >> > > Both of these are either a BlockBackend ID or a BDS node-name, right? Do >> > > we have a standard way of expressing this? "name of the device" isn't >> > > quite clear. >> > >> > "name of the device" is used everywhere to document the "device" >> > parameters in >> > our json schema. Since we have BlockBackend now, device-name and node-name >> > could be better distinguished. All we have to do is giving a >> > beautiful name to >> > both. >> > >> > [This patch is only a copy&paste and is consistent with the rest part of the >> > file. So I'll leave it for now :] >> >> The rest of the file doesn't accept node names. But looking at your >> actual code, it seems that you are doing the same (by usign bdrv_find() >> instead of bdrv_lookup_bs()). > > Yes, to be consistent with drive-backup. > >> >> Shouldn't a proper blockdev-* command accept node names as well? >> > > I am not sure, it's still blockdev-backup, not blocknode-backup. > > I think that may be another thing, to changed drive-*'s @device parameter, and > blockdev-*'s @device and @target to accept node names, altogether. We have many commands identifying nodes by some name: root nodes by BDS device_name[] (now BB name), inner nodes by "file name" (ugh), and arbitrary nodes by BDS name_name[]. An obvious task is deprecating "file name" in favor of node names. Less obvious is where to accept a node name instead of / in addition to a device name. Want me to start a thread on this?
On Tue, 11/04 07:47, Markus Armbruster wrote: > Fam Zheng <famz@redhat.com> writes: > > > On Mon, 11/03 15:32, Kevin Wolf wrote: > >> Am 03.11.2014 um 02:46 hat Fam Zheng geschrieben: > >> > On Fri, 10/31 10:01, Kevin Wolf wrote: > >> > > Am 11.09.2014 um 07:05 hat Fam Zheng geschrieben: > >> > > > Similar to drive-backup, but this command uses a device id as target > >> > > > instead of creating/opening an image file. > >> > > > > >> > > > Also add blocker on target bs, since the target is also a named device > >> > > > now. > >> > > > > >> > > > Add check and report error for bs == target which became possible but is > >> > > > an illegal case with introduction of blockdev-backup. > >> > > > > >> > > > Signed-off-by: Fam Zheng <famz@redhat.com> > >> > > > >> > > > diff --git a/qapi/block-core.json b/qapi/block-core.json > >> > > > index a685d02..b953c7b 100644 > >> > > > --- a/qapi/block-core.json > >> > > > +++ b/qapi/block-core.json > >> > > > @@ -669,6 +669,40 @@ > >> > > > '*on-target-error': 'BlockdevOnError' } } > >> > > > > >> > > > ## > >> > > > +# @BlockdevBackup > >> > > > +# > >> > > > +# @device: the name of the device which should be copied. > >> > > > +# > >> > > > +# @target: the name of the backup target device. > >> > > > >> > > Both of these are either a BlockBackend ID or a BDS node-name, right? Do > >> > > we have a standard way of expressing this? "name of the device" isn't > >> > > quite clear. > >> > > >> > "name of the device" is used everywhere to document the "device" > >> > parameters in > >> > our json schema. Since we have BlockBackend now, device-name and node-name > >> > could be better distinguished. All we have to do is giving a > >> > beautiful name to > >> > both. > >> > > >> > [This patch is only a copy&paste and is consistent with the rest part of the > >> > file. So I'll leave it for now :] > >> > >> The rest of the file doesn't accept node names. But looking at your > >> actual code, it seems that you are doing the same (by usign bdrv_find() > >> instead of bdrv_lookup_bs()). > > > > Yes, to be consistent with drive-backup. > > > >> > >> Shouldn't a proper blockdev-* command accept node names as well? > >> > > > > I am not sure, it's still blockdev-backup, not blocknode-backup. > > > > I think that may be another thing, to changed drive-*'s @device parameter, and > > blockdev-*'s @device and @target to accept node names, altogether. > > We have many commands identifying nodes by some name: root nodes by BDS > device_name[] (now BB name), inner nodes by "file name" (ugh), and > arbitrary nodes by BDS name_name[]. > > An obvious task is deprecating "file name" in favor of node names. > > Less obvious is where to accept a node name instead of / in addition to > a device name. > > Want me to start a thread on this? Yes please, it's good to review the situation and make some concrete convention and plan for the transition. Fam
Fam Zheng <famz@redhat.com> writes: > On Tue, 11/04 07:47, Markus Armbruster wrote: >> Fam Zheng <famz@redhat.com> writes: >> >> > On Mon, 11/03 15:32, Kevin Wolf wrote: >> >> Am 03.11.2014 um 02:46 hat Fam Zheng geschrieben: >> >> > On Fri, 10/31 10:01, Kevin Wolf wrote: >> >> > > Am 11.09.2014 um 07:05 hat Fam Zheng geschrieben: >> >> > > > Similar to drive-backup, but this command uses a device id as target >> >> > > > instead of creating/opening an image file. >> >> > > > >> >> > > > Also add blocker on target bs, since the target is also a >> >> > > > named device >> >> > > > now. >> >> > > > >> >> > > > Add check and report error for bs == target which became >> >> > > > possible but is >> >> > > > an illegal case with introduction of blockdev-backup. >> >> > > > >> >> > > > Signed-off-by: Fam Zheng <famz@redhat.com> >> >> > > >> >> > > > diff --git a/qapi/block-core.json b/qapi/block-core.json >> >> > > > index a685d02..b953c7b 100644 >> >> > > > --- a/qapi/block-core.json >> >> > > > +++ b/qapi/block-core.json >> >> > > > @@ -669,6 +669,40 @@ >> >> > > > '*on-target-error': 'BlockdevOnError' } } >> >> > > > >> >> > > > ## >> >> > > > +# @BlockdevBackup >> >> > > > +# >> >> > > > +# @device: the name of the device which should be copied. >> >> > > > +# >> >> > > > +# @target: the name of the backup target device. >> >> > > >> >> > > Both of these are either a BlockBackend ID or a BDS >> >> > > node-name, right? Do >> >> > > we have a standard way of expressing this? "name of the device" isn't >> >> > > quite clear. >> >> > >> >> > "name of the device" is used everywhere to document the "device" >> >> > parameters in >> >> > our json schema. Since we have BlockBackend now, device-name >> >> > and node-name >> >> > could be better distinguished. All we have to do is giving a >> >> > beautiful name to >> >> > both. >> >> > >> >> > [This patch is only a copy&paste and is consistent with the >> >> > rest part of the >> >> > file. So I'll leave it for now :] >> >> >> >> The rest of the file doesn't accept node names. But looking at your >> >> actual code, it seems that you are doing the same (by usign bdrv_find() >> >> instead of bdrv_lookup_bs()). >> > >> > Yes, to be consistent with drive-backup. >> > >> >> >> >> Shouldn't a proper blockdev-* command accept node names as well? >> >> >> > >> > I am not sure, it's still blockdev-backup, not blocknode-backup. >> > >> > I think that may be another thing, to changed drive-*'s @device >> > parameter, and >> > blockdev-*'s @device and @target to accept node names, altogether. >> >> We have many commands identifying nodes by some name: root nodes by BDS >> device_name[] (now BB name), inner nodes by "file name" (ugh), and >> arbitrary nodes by BDS name_name[]. >> >> An obvious task is deprecating "file name" in favor of node names. >> >> Less obvious is where to accept a node name instead of / in addition to >> a device name. >> >> Want me to start a thread on this? > > Yes please, it's good to review the situation and make some concrete > convention and plan for the transition. Done: Review of monitor commands identifying BDS / BB by name
diff --git a/block/backup.c b/block/backup.c index d0b0225..24e8ffc 100644 --- a/block/backup.c +++ b/block/backup.c @@ -344,6 +344,7 @@ static void coroutine_fn backup_run(void *opaque) hbitmap_free(job->bitmap); bdrv_iostatus_disable(target); + bdrv_op_unblock_all(target, job->common.blocker); bdrv_unref(target); block_job_completed(&job->common, ret); @@ -362,6 +363,11 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, assert(target); assert(cb); + if (bs == target) { + error_setg(errp, "Source and target cannot be the same"); + return; + } + if ((on_source_error == BLOCKDEV_ON_ERROR_STOP || on_source_error == BLOCKDEV_ON_ERROR_ENOSPC) && !bdrv_iostatus_is_enabled(bs)) { @@ -369,6 +375,24 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, return; } + if (!bdrv_is_inserted(bs)) { + error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bs->device_name); + return; + } + + if (!bdrv_is_inserted(target)) { + error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, target->device_name); + return; + } + + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) { + return; + } + + if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_BACKUP_TARGET, errp)) { + return; + } + len = bdrv_getlength(bs); if (len < 0) { error_setg_errno(errp, -len, "unable to get length for '%s'", @@ -382,6 +406,8 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, return; } + bdrv_op_block_all(target, job->common.blocker); + job->on_source_error = on_source_error; job->on_target_error = on_target_error; job->target = target; diff --git a/blockdev.c b/blockdev.c index e919566..516de7f 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2046,6 +2046,8 @@ void qmp_drive_backup(const char *device, const char *target, return; } + /* Although backup_run has this check too, we need to use bs->drv below, so + * do an early check redundantly. */ if (!bdrv_is_inserted(bs)) { error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device); return; @@ -2062,6 +2064,7 @@ void qmp_drive_backup(const char *device, const char *target, } } + /* Early check to avoid creating target */ if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) { return; } @@ -2124,6 +2127,50 @@ BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp) return bdrv_named_nodes_list(); } +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; + } + + target_bs = bdrv_find(target); + if (!target_bs) { + error_set(errp, QERR_DEVICE_NOT_FOUND, 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) { + bdrv_unref(target_bs); + 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/block-core.json b/qapi/block-core.json index a685d02..b953c7b 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -669,6 +669,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: 2.2 +## +{ 'type': 'BlockdevBackup', + 'data': { 'device': 'str', 'target': 'str', + 'sync': 'MirrorSyncMode', + '*speed': 'int', + '*on-source-error': 'BlockdevOnError', + '*on-target-error': 'BlockdevOnError' } } + +## # @blockdev-snapshot-sync # # Generates a synchronous snapshot of a block device. @@ -788,6 +822,22 @@ { '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 2.2 +## +{ 'command': 'blockdev-backup', 'data': 'BlockdevBackup' } + + +## # @query-named-block-nodes # # Get the named block driver list diff --git a/qmp-commands.hx b/qmp-commands.hx index 7658d4b..fa40260 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1094,6 +1094,50 @@ 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). +- "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 {
Similar to drive-backup, but this command uses a device id as target instead of creating/opening an image file. Also add blocker on target bs, since the target is also a named device now. Add check and report error for bs == target which became possible but is an illegal case with introduction of blockdev-backup. Signed-off-by: Fam Zheng <famz@redhat.com> --- block/backup.c | 26 ++++++++++++++++++++++++++ blockdev.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ qapi/block-core.json | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ qmp-commands.hx | 44 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 167 insertions(+)