From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35647) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y1Lar-0006Zj-RG for qemu-devel@nongnu.org; Wed, 17 Dec 2014 15:53:20 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y1Lal-0001Ji-GP for qemu-devel@nongnu.org; Wed, 17 Dec 2014 15:53:13 -0500 Received: from mx1.redhat.com ([209.132.183.28]:43055) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y1Lal-0001Jd-7p for qemu-devel@nongnu.org; Wed, 17 Dec 2014 15:53:07 -0500 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sBHKr6ps016868 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 17 Dec 2014 15:53:06 -0500 Message-ID: <5491ED2F.1090607@redhat.com> Date: Wed, 17 Dec 2014 15:53:03 -0500 From: John Snow MIME-Version: 1.0 References: <1418820703-17179-1-git-send-email-famz@redhat.com> <1418820703-17179-3-git-send-email-famz@redhat.com> In-Reply-To: <1418820703-17179-3-git-send-email-famz@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 2/4] qmp: Add command 'blockdev-backup' List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng , qemu-devel@nongnu.org Cc: Kevin Wolf , Markus Armbruster , Stefan Hajnoczi , mreitz@redhat.com On 12/17/2014 07:51 AM, Fam Zheng wrote: > 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 > --- > block/backup.c | 28 +++++++++++++++++++++++++++ > blockdev.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > qapi/block-core.json | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > qmp-commands.hx | 42 ++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 178 insertions(+) > > diff --git a/block/backup.c b/block/backup.c > index 792e655..1c535b1 100644 > --- a/block/backup.c > +++ b/block/backup.c > @@ -360,6 +360,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); > > data = g_malloc(sizeof(*data)); > data->ret = ret; > @@ -379,6 +380,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)) { > @@ -386,6 +392,26 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, > return; > } > > + if (!bdrv_is_inserted(bs)) { > + error_setg(errp, "Device is not inserted: %s", > + bdrv_get_device_name(bs)); > + return; > + } > + > + if (!bdrv_is_inserted(target)) { > + error_setg(errp, "Device is not inserted: %s", > + bdrv_get_device_name(target)); > + 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'", > @@ -399,6 +425,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 5651a8e..dbbab1d 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -2240,6 +2240,8 @@ void qmp_drive_backup(const char *device, const char *target, > aio_context = bdrv_get_aio_context(bs); > aio_context_acquire(aio_context); > > + /* 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); > goto out; > @@ -2256,6 +2258,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)) { > goto out; > } > @@ -2323,6 +2326,57 @@ 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; > + AioContext *aio_context; > + > + 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; > + } > + > + aio_context = bdrv_get_aio_context(bs); > + aio_context_acquire(aio_context); > + > + target_bs = bdrv_find(target); > + if (!target_bs) { > + error_set(errp, QERR_DEVICE_NOT_FOUND, target); > + goto out; > + } > + > + bdrv_ref(target_bs); > + bdrv_set_aio_context(target_bs, bdrv_get_aio_context(bs)); why call bdrv_get_aio_context(bs) again instead of just using aio_context? Will this cause issues? > + 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); > + } > +out: > + aio_context_release(aio_context); > +} > + > #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 6e8db15..d9f0598 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -703,6 +703,41 @@ > '*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. The default is 0, > +# for unlimited. > +# > +# @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.3 > +## > +{ '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. > @@ -822,6 +857,25 @@ > { 'command': 'drive-backup', 'data': 'DriveBackup' } > > ## > +# @blockdev-backup > +# > +# Start a point-in-time copy of a block device to a new destination. The > +# status of ongoing blockdev-backup operations can be checked with > +# query-block-jobs where the BlockJobInfo.type field has the value 'backup'. > +# The operation can be stopped before it has completed using the > +# block-job-cancel command. > +# > +# 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.3 > +## > +{ '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 3348782..a7bb90b 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -1094,6 +1094,48 @@ 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 > +------------ ^ Pad out the dashes to match the line length of the command. Mentioning only because Eric got me on that one last time :) > + > +The device version of drive-backup: this command takes an existing named device > +as backup target. > + > +Arguments: > + > +- "device": the name of the device which should be copied. > + (json-string) > +- "target": the name of the backup target device. (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", > + "sync": "full", > + "target": "tgt-id" } } > +<- { "return": {} } > + > EQMP > > { >