From 67c75f3dff7fa7776781d93250e1d52f1b009a74 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 14 Jul 2016 15:49:42 +0200 Subject: [PATCH 01/36] ide: ide-cd without drive property for empty drive This allows the creation of an empty ide-cd device without manually creating a BlockBackend. Signed-off-by: Kevin Wolf Acked-by: Eric Blake --- hw/ide/qdev.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c index 67c76bfcd6..2eb055ae70 100644 --- a/hw/ide/qdev.c +++ b/hw/ide/qdev.c @@ -75,10 +75,6 @@ static int ide_qdev_init(DeviceState *qdev) IDEDeviceClass *dc = IDE_DEVICE_GET_CLASS(dev); IDEBus *bus = DO_UPCAST(IDEBus, qbus, qdev->parent_bus); - if (!dev->conf.blk) { - error_report("No drive specified"); - goto err; - } if (dev->unit == -1) { dev->unit = bus->master ? 1 : 0; } @@ -158,6 +154,16 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind) IDEState *s = bus->ifs + dev->unit; Error *err = NULL; + if (!dev->conf.blk) { + if (kind != IDE_CD) { + error_report("No drive specified"); + return -1; + } else { + /* Anonymous BlockBackend for an empty drive */ + dev->conf.blk = blk_new(); + } + } + if (dev->conf.discard_granularity == -1) { dev->conf.discard_granularity = 512; } else if (dev->conf.discard_granularity && @@ -257,7 +263,11 @@ static int ide_cd_initfn(IDEDevice *dev) static int ide_drive_initfn(IDEDevice *dev) { - DriveInfo *dinfo = blk_legacy_dinfo(dev->conf.blk); + DriveInfo *dinfo = NULL; + + if (dev->conf.blk) { + dinfo = blk_legacy_dinfo(dev->conf.blk); + } return ide_dev_initfn(dev, dinfo && dinfo->media_cd ? IDE_CD : IDE_HD); } From 9ef6e505f00aaabfccd8f8b0f0685589a74e76ea Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 14 Jul 2016 15:49:43 +0200 Subject: [PATCH 02/36] scsi: scsi-cd without drive property for empty drive This allows the creation of an empty scsi-cd device without manually creating a BlockBackend. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- hw/scsi/scsi-disk.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index 836a1553ed..99c9d618da 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -2359,6 +2359,11 @@ static void scsi_hd_realize(SCSIDevice *dev, Error **errp) static void scsi_cd_realize(SCSIDevice *dev, Error **errp) { SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev); + + if (!dev->conf.blk) { + dev->conf.blk = blk_new(); + } + s->qdev.blocksize = 2048; s->qdev.type = TYPE_ROM; s->features |= 1 << SCSI_DISK_F_REMOVABLE; From b6c1bae5df8abbed73c4c0bd92e9963df8829c74 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 23 Jun 2016 14:20:24 +0200 Subject: [PATCH 03/36] block: Accept node-name for block-stream In order to remove the necessity to use BlockBackend names in the external API, we want to allow node-names everywhere. This converts block-stream to accept a node-name without lifting the restriction that we're operating at a root node. In case of an invalid device name, the command returns the GenericError error class now instead of DeviceNotFound, because this is what qmp_get_root_bs() returns. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz Reviewed-by: Alberto Garcia --- block/block-backend.c | 16 +++++++++++++++ blockdev.c | 37 +++++++++++++++++++++++----------- include/sysemu/block-backend.h | 1 + qapi/block-core.json | 5 +---- qmp-commands.hx | 2 +- tests/qemu-iotests/030 | 2 +- 6 files changed, 45 insertions(+), 18 deletions(-) diff --git a/block/block-backend.c b/block/block-backend.c index effa038924..dc2bc60b9e 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -409,6 +409,22 @@ bool bdrv_has_blk(BlockDriverState *bs) return bdrv_first_blk(bs) != NULL; } +/* + * Returns true if @bs has only BlockBackends as parents. + */ +bool bdrv_is_root_node(BlockDriverState *bs) +{ + BdrvChild *c; + + QLIST_FOREACH(c, &bs->parents, next_parent) { + if (c->role != &child_root) { + return false; + } + } + + return true; +} + /* * Return @blk's DriveInfo if any, else null. */ diff --git a/blockdev.c b/blockdev.c index 21614004d1..68b6741398 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1174,6 +1174,28 @@ fail: return dinfo; } +static BlockDriverState *qmp_get_root_bs(const char *name, Error **errp) +{ + BlockDriverState *bs; + + bs = bdrv_lookup_bs(name, name, errp); + if (bs == NULL) { + return NULL; + } + + if (!bdrv_is_root_node(bs)) { + error_setg(errp, "Need a root block node"); + return NULL; + } + + if (!bdrv_is_inserted(bs)) { + error_setg(errp, "Device has no medium"); + return NULL; + } + + return bs; +} + void hmp_commit(Monitor *mon, const QDict *qdict) { const char *device = qdict_get_str(qdict, "device"); @@ -2983,7 +3005,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, bool has_on_error, BlockdevOnError on_error, Error **errp) { - BlockBackend *blk; BlockDriverState *bs; BlockDriverState *base_bs = NULL; AioContext *aio_context; @@ -2994,22 +3015,14 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, on_error = BLOCKDEV_ON_ERROR_REPORT; } - blk = blk_by_name(device); - if (!blk) { - error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, - "Device '%s' not found", device); + bs = qmp_get_root_bs(device, errp); + if (!bs) { return; } - aio_context = blk_get_aio_context(blk); + aio_context = bdrv_get_aio_context(bs); aio_context_acquire(aio_context); - if (!blk_is_available(blk)) { - error_setg(errp, "Device '%s' has no medium", device); - goto out; - } - bs = blk_bs(blk); - if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) { goto out; } diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h index 2da4905d18..bb4fa82d1c 100644 --- a/include/sysemu/block-backend.h +++ b/include/sysemu/block-backend.h @@ -98,6 +98,7 @@ BlockDriverState *blk_bs(BlockBackend *blk); void blk_remove_bs(BlockBackend *blk); void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs); bool bdrv_has_blk(BlockDriverState *bs); +bool bdrv_is_root_node(BlockDriverState *bs); void blk_set_allow_write_beyond_eof(BlockBackend *blk, bool allow); void blk_iostatus_enable(BlockBackend *blk); diff --git a/qapi/block-core.json b/qapi/block-core.json index 5e2d7d78d2..a2d18344cf 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1462,7 +1462,7 @@ # @job-id: #optional identifier for the newly-created block job. If # omitted, the device name will be used. (Since 2.7) # -# @device: the device name +# @device: the device name or node-name of a root node # # @base: #optional the common backing file name # @@ -1487,9 +1487,6 @@ # 'stop' and 'enospc' can only be used if the block device # supports io-status (see BlockInfo). Since 1.3. # -# Returns: Nothing on success -# If @device does not exist, DeviceNotFound -# # Since: 1.1 ## { 'command': 'block-stream', diff --git a/qmp-commands.hx b/qmp-commands.hx index 6866264e64..48b7fdcfb8 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1120,7 +1120,7 @@ Arguments: - "job-id": Identifier for the newly-created block job. If omitted, the device name will be used. (json-string, optional) -- "device": The device's ID, must be unique (json-string) +- "device": The device name or node-name of a root node (json-string) - "base": The file name of the backing image above which copying starts (json-string, optional) - "backing-file": The backing file string to write into the active layer. This diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030 index 3ac2443e5b..107049b50f 100755 --- a/tests/qemu-iotests/030 +++ b/tests/qemu-iotests/030 @@ -126,7 +126,7 @@ class TestSingleDrive(iotests.QMPTestCase): def test_device_not_found(self): result = self.vm.qmp('block-stream', device='nonexistent') - self.assert_qmp(result, 'error/class', 'DeviceNotFound') + self.assert_qmp(result, 'error/class', 'GenericError') class TestSmallerBackingFile(iotests.QMPTestCase): From 1d13b167fd40c0ac8dfd779f150d8643afd508dc Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 23 Jun 2016 14:20:24 +0200 Subject: [PATCH 04/36] block: Accept node-name for block-commit In order to remove the necessity to use BlockBackend names in the external API, we want to allow node-names everywhere. This converts block-commit to accept a node-name without lifting the restriction that we're operating at a root node. As libvirt makes use of the DeviceNotFound error class, we must add explicit code to retain this behaviour because qmp_get_root_bs() only returns GenericErrors. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Max Reitz Reviewed-by: Alberto Garcia --- blockdev.c | 23 +++++++++++------------ qapi/block-core.json | 2 +- qmp-commands.hx | 2 +- 3 files changed, 13 insertions(+), 14 deletions(-) diff --git a/blockdev.c b/blockdev.c index 68b6741398..7619ad4fda 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3068,7 +3068,6 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device, bool has_speed, int64_t speed, Error **errp) { - BlockBackend *blk; BlockDriverState *bs; BlockDriverState *base_bs, *top_bs; AioContext *aio_context; @@ -3087,22 +3086,22 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device, * live commit feature versions; for this to work, we must make sure to * perform the device lookup before any generic errors that may occur in a * scenario in which all optional arguments are omitted. */ - blk = blk_by_name(device); - if (!blk) { - error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, - "Device '%s' not found", device); + bs = qmp_get_root_bs(device, &local_err); + if (!bs) { + bs = bdrv_lookup_bs(device, device, NULL); + if (!bs) { + error_free(local_err); + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", device); + } else { + error_propagate(errp, local_err); + } return; } - aio_context = blk_get_aio_context(blk); + aio_context = bdrv_get_aio_context(bs); aio_context_acquire(aio_context); - if (!blk_is_available(blk)) { - error_setg(errp, "Device '%s' has no medium", device); - goto out; - } - bs = blk_bs(blk); - if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT_SOURCE, errp)) { goto out; } diff --git a/qapi/block-core.json b/qapi/block-core.json index a2d18344cf..04076c85c6 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1020,7 +1020,7 @@ # @job-id: #optional identifier for the newly-created block job. If # omitted, the device name will be used. (Since 2.7) # -# @device: the name of the device +# @device: the device name or node-name of a root node # # @base: #optional The file name of the backing image to write data into. # If not specified, this is the deepest backing image diff --git a/qmp-commands.hx b/qmp-commands.hx index 48b7fdcfb8..b53466039a 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1166,7 +1166,7 @@ Arguments: - "job-id": Identifier for the newly-created block job. If omitted, the device name will be used. (json-string, optional) -- "device": The device's ID, must be unique (json-string) +- "device": The device name or node-name of a root node (json-string) - "base": The file name of the backing image to write data into. If not specified, this is the deepest backing image (json-string, optional) From cef34eebf3d0f252a3b3e9a2a459b6c3ecc56f68 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 23 Jun 2016 14:20:24 +0200 Subject: [PATCH 05/36] block: Accept node-name for blockdev-backup In order to remove the necessity to use BlockBackend names in the external API, we want to allow node-names everywhere. This converts blockdev-backup and the corresponding transaction action to accept a node-name without lifting the restriction that we're operating at a root node. In case of an invalid device name, the command returns the GenericError error class now instead of DeviceNotFound, because this is what qmp_get_root_bs() returns. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Max Reitz --- blockdev.c | 31 ++++++++----------------------- qapi/block-core.json | 2 +- qmp-commands.hx | 2 +- 3 files changed, 10 insertions(+), 25 deletions(-) diff --git a/blockdev.c b/blockdev.c index 7619ad4fda..46beafdfad 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1959,21 +1959,14 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp) { BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common); BlockdevBackup *backup; - BlockBackend *blk; - BlockDriverState *target; + BlockDriverState *bs, *target; Error *local_err = NULL; assert(common->action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP); backup = common->action->u.blockdev_backup.data; - blk = blk_by_name(backup->device); - if (!blk) { - error_setg(errp, "Device '%s' not found", backup->device); - return; - } - - if (!blk_is_available(blk)) { - error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, backup->device); + bs = qmp_get_root_bs(backup->device, errp); + if (!bs) { return; } @@ -1983,14 +1976,14 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp) } /* AioContext is released in .clean() */ - state->aio_context = blk_get_aio_context(blk); + state->aio_context = bdrv_get_aio_context(bs); if (state->aio_context != bdrv_get_aio_context(target)) { state->aio_context = NULL; error_setg(errp, "Backup between two IO threads is not implemented"); return; } aio_context_acquire(state->aio_context); - state->bs = blk_bs(blk); + state->bs = bs; bdrv_drained_begin(state->bs); do_blockdev_backup(backup->has_job_id ? backup->job_id : NULL, @@ -3335,7 +3328,6 @@ void do_blockdev_backup(const char *job_id, const char *device, BlockdevOnError on_target_error, BlockJobTxn *txn, Error **errp) { - BlockBackend *blk; BlockDriverState *bs; BlockDriverState *target_bs; Error *local_err = NULL; @@ -3351,21 +3343,14 @@ void do_blockdev_backup(const char *job_id, const char *device, on_target_error = BLOCKDEV_ON_ERROR_REPORT; } - blk = blk_by_name(device); - if (!blk) { - error_setg(errp, "Device '%s' not found", device); + bs = qmp_get_root_bs(device, errp); + if (!bs) { return; } - aio_context = blk_get_aio_context(blk); + aio_context = bdrv_get_aio_context(bs); aio_context_acquire(aio_context); - if (!blk_is_available(blk)) { - error_setg(errp, "Device '%s' has no medium", device); - goto out; - } - bs = blk_bs(blk); - target_bs = bdrv_lookup_bs(target, target, errp); if (!target_bs) { goto out; diff --git a/qapi/block-core.json b/qapi/block-core.json index 04076c85c6..53f46b625c 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -925,7 +925,7 @@ # @job-id: #optional identifier for the newly-created block job. If # omitted, the device name will be used. (Since 2.7) # -# @device: the name of the device which should be copied. +# @device: the device name or node-name of a root node which should be copied. # # @target: the device name or node-name of the backup target node. # diff --git a/qmp-commands.hx b/qmp-commands.hx index b53466039a..2077585dd3 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1288,7 +1288,7 @@ Arguments: - "job-id": Identifier for the newly-created block job. If omitted, the device name will be used. (json-string, optional) -- "device": the name of the device which should be copied. +- "device": the device name or node-name of a root node 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; From 07eec652722f3d12b07c5b28d0671ddfc22fe6a5 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 23 Jun 2016 14:20:24 +0200 Subject: [PATCH 06/36] block: Accept node-name for blockdev-mirror In order to remove the necessity to use BlockBackend names in the external API, we want to allow node-names everywhere. This converts blockdev-mirror to accept a node-name without lifting the restriction that we're operating at a root node. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Alberto Garcia Reviewed-by: Max Reitz --- blockdev.c | 10 +--------- qapi/block-core.json | 3 ++- qmp-commands.hx | 3 ++- 3 files changed, 5 insertions(+), 11 deletions(-) diff --git a/blockdev.c b/blockdev.c index 46beafdfad..ccff1f7d05 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3627,21 +3627,13 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id, Error **errp) { BlockDriverState *bs; - BlockBackend *blk; BlockDriverState *target_bs; AioContext *aio_context; BlockMirrorBackingMode backing_mode = MIRROR_LEAVE_BACKING_CHAIN; Error *local_err = NULL; - blk = blk_by_name(device); - if (!blk) { - error_setg(errp, "Device '%s' not found", device); - return; - } - bs = blk_bs(blk); - + bs = qmp_get_root_bs(device, errp); if (!bs) { - error_setg(errp, "Device '%s' has no media", device); return; } diff --git a/qapi/block-core.json b/qapi/block-core.json index 53f46b625c..bcb37db5af 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1277,7 +1277,8 @@ # @job-id: #optional identifier for the newly-created block job. If # omitted, the device name will be used. (Since 2.7) # -# @device: the name of the device whose writes should be mirrored. +# @device: The device name or node-name of a root node whose writes should be +# mirrored. # # @target: the id or node-name of the block device to mirror to. This mustn't be # attached to guest. diff --git a/qmp-commands.hx b/qmp-commands.hx index 2077585dd3..034d51718c 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1747,7 +1747,8 @@ Arguments: - "job-id": Identifier for the newly-created block job. If omitted, the device name will be used. (json-string, optional) -- "device": device name to operate on (json-string) +- "device": The device name or node-name of a root node whose writes should be + mirrored (json-string) - "target": device name to mirror to (json-string) - "replaces": the block driver node name to replace when finished (json-string, optional) From 2dfb4c033f2f8fbad252bed1ba3e0fed112cbb7c Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 23 Jun 2016 14:20:24 +0200 Subject: [PATCH 07/36] block: Accept node-name for blockdev-snapshot-delete-internal-sync In order to remove the necessity to use BlockBackend names in the external API, we want to allow node-names everywhere. This converts blockdev-snapshot-delete-internal-sync to accept a node-name without lifting the restriction that we're operating at a root node. In case of an invalid device name, the command returns the GenericError error class now instead of DeviceNotFound, because this is what qmp_get_root_bs() returns. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Alberto Garcia Reviewed-by: Max Reitz --- blockdev.c | 16 +++------------- qapi/block.json | 5 +++-- qmp-commands.hx | 2 +- tests/qemu-iotests/057 | 2 +- 4 files changed, 8 insertions(+), 17 deletions(-) diff --git a/blockdev.c b/blockdev.c index ccff1f7d05..9feffcd088 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1306,21 +1306,17 @@ SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device, Error **errp) { BlockDriverState *bs; - BlockBackend *blk; AioContext *aio_context; QEMUSnapshotInfo sn; Error *local_err = NULL; SnapshotInfo *info = NULL; int ret; - blk = blk_by_name(device); - if (!blk) { - error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, - "Device '%s' not found", device); + bs = qmp_get_root_bs(device, errp); + if (!bs) { return NULL; } - - aio_context = blk_get_aio_context(blk); + aio_context = bdrv_get_aio_context(bs); aio_context_acquire(aio_context); if (!has_id) { @@ -1336,12 +1332,6 @@ SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device, goto out_aio_context; } - if (!blk_is_available(blk)) { - error_setg(errp, "Device '%s' has no medium", device); - goto out_aio_context; - } - bs = blk_bs(blk); - if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE, errp)) { goto out_aio_context; } diff --git a/qapi/block.json b/qapi/block.json index 937337dce5..c0e831fd29 100644 --- a/qapi/block.json +++ b/qapi/block.json @@ -99,14 +99,15 @@ # both. One of the name or id is required. Return SnapshotInfo for the # successfully deleted snapshot. # -# @device: the name of the device to delete the snapshot from +# @device: the device name or node-name of a root node to delete the snapshot +# from # # @id: optional the snapshot's ID to be deleted # # @name: optional the snapshot's name to be deleted # # Returns: SnapshotInfo on success -# If @device is not a valid block device, DeviceNotFound +# If @device is not a valid block device, GenericError # If snapshot not found, GenericError # If the format of the image used does not support it, # BlockFormatFeatureNotSupported diff --git a/qmp-commands.hx b/qmp-commands.hx index 034d51718c..6793c8d36b 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1639,7 +1639,7 @@ fail. Arguments: -- "device": device name (json-string) +- "device": the device name or node-name of a root node (json-string) - "id": ID of the snapshot (json-string, optional) - "name": name of the snapshot (json-string, optional) diff --git a/tests/qemu-iotests/057 b/tests/qemu-iotests/057 index 9cdd582e39..bb6c06a6fd 100755 --- a/tests/qemu-iotests/057 +++ b/tests/qemu-iotests/057 @@ -239,7 +239,7 @@ class TestSnapshotDelete(ImageSnapshotTestCase): result = self.vm.qmp('blockdev-snapshot-delete-internal-sync', device = 'drive_error', id = '0') - self.assert_qmp(result, 'error/class', 'DeviceNotFound') + self.assert_qmp(result, 'error/class', 'GenericError') def test_error_no_id_and_name(self): result = self.vm.qmp('blockdev-snapshot-delete-internal-sync', From 75dfd402a734e8080fa77a1bcf60ecc46e0e6158 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 23 Jun 2016 14:20:24 +0200 Subject: [PATCH 08/36] block: Accept node-name for blockdev-snapshot-internal-sync In order to remove the necessity to use BlockBackend names in the external API, we want to allow node-names everywhere. This converts blockdev-snapshot-internal-sync to accept a node-name without lifting the restriction that we're operating at a root node. In case of an invalid device name, the command returns the GenericError error class now instead of DeviceNotFound, because this is what qmp_get_root_bs() returns. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Alberto Garcia Reviewed-by: Max Reitz --- blockdev.c | 15 +++------------ qapi/block.json | 5 +++-- qmp-commands.hx | 6 ++++-- tests/qemu-iotests/057 | 2 +- 4 files changed, 11 insertions(+), 17 deletions(-) diff --git a/blockdev.c b/blockdev.c index 9feffcd088..f775bbdf9d 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1511,7 +1511,6 @@ static void internal_snapshot_prepare(BlkActionState *common, Error *local_err = NULL; const char *device; const char *name; - BlockBackend *blk; BlockDriverState *bs; QEMUSnapshotInfo old_sn, *sn; bool ret; @@ -1534,23 +1533,15 @@ static void internal_snapshot_prepare(BlkActionState *common, return; } - blk = blk_by_name(device); - if (!blk) { - error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, - "Device '%s' not found", device); + bs = qmp_get_root_bs(device, errp); + if (!bs) { return; } /* AioContext is released in .clean() */ - state->aio_context = blk_get_aio_context(blk); + state->aio_context = bdrv_get_aio_context(bs); aio_context_acquire(state->aio_context); - if (!blk_is_available(blk)) { - error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, device); - return; - } - bs = blk_bs(blk); - state->bs = bs; bdrv_drained_begin(bs); diff --git a/qapi/block.json b/qapi/block.json index c0e831fd29..db05169187 100644 --- a/qapi/block.json +++ b/qapi/block.json @@ -58,7 +58,8 @@ ## # @BlockdevSnapshotInternal # -# @device: the name of the device to generate the snapshot from +# @device: the device name or node-name of a root node to generate the snapshot +# from # # @name: the name of the internal snapshot to be created # @@ -80,7 +81,7 @@ # For the arguments, see the documentation of BlockdevSnapshotInternal. # # Returns: nothing on success -# If @device is not a valid block device, DeviceNotFound +# If @device is not a valid block device, GenericError # If any snapshot matching @name exists, or @name is empty, # GenericError # If the format of the image used does not support it, diff --git a/qmp-commands.hx b/qmp-commands.hx index 6793c8d36b..eb2736009f 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1407,7 +1407,8 @@ actions array: - "mode": whether and how QEMU should create the snapshot file (NewImageMode, optional, default "absolute-paths") When "type" is "blockdev-snapshot-internal-sync": - - "device": device name to snapshot (json-string) + - "device": the device name or node-name of a root node to snapshot + (json-string) - "name": name of the new snapshot (json-string) Example: @@ -1608,7 +1609,8 @@ name already exists, the operation will fail. Arguments: -- "device": device name to snapshot (json-string) +- "device": the device name or node-name of a root node to snapshot + (json-string) - "name": name of the new snapshot (json-string) Example: diff --git a/tests/qemu-iotests/057 b/tests/qemu-iotests/057 index bb6c06a6fd..9f0a5a3057 100755 --- a/tests/qemu-iotests/057 +++ b/tests/qemu-iotests/057 @@ -182,7 +182,7 @@ class TestSingleTransaction(ImageSnapshotTestCase): 'name': 'a' }, }] result = self.vm.qmp('transaction', actions = actions) - self.assert_qmp(result, 'error/class', 'DeviceNotFound') + self.assert_qmp(result, 'error/class', 'GenericError') def test_error_exist(self): self.createSnapshotInTransaction(1) From 7b5dca3f0215ec6484473631928a36c3eb8da0ef Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 23 Jun 2016 14:20:24 +0200 Subject: [PATCH 09/36] block: Accept node-name for change-backing-file In order to remove the necessity to use BlockBackend names in the external API, we want to allow node-names everywhere. This converts change-backing-file to accept a node-name without lifting the restriction that we're operating at a root node. In case of an invalid device name, the command returns the GenericError error class now instead of DeviceNotFound, because this is what qmp_get_root_bs() returns. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Alberto Garcia Reviewed-by: Max Reitz --- blockdev.c | 15 +++------------ qapi/block-core.json | 3 ++- qmp-commands.hx | 3 ++- 3 files changed, 7 insertions(+), 14 deletions(-) diff --git a/blockdev.c b/blockdev.c index f775bbdf9d..a42c8028ac 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3755,7 +3755,6 @@ void qmp_change_backing_file(const char *device, const char *backing_file, Error **errp) { - BlockBackend *blk; BlockDriverState *bs = NULL; AioContext *aio_context; BlockDriverState *image_bs = NULL; @@ -3764,22 +3763,14 @@ void qmp_change_backing_file(const char *device, int open_flags; int ret; - blk = blk_by_name(device); - if (!blk) { - error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, - "Device '%s' not found", device); + bs = qmp_get_root_bs(device, errp); + if (!bs) { return; } - aio_context = blk_get_aio_context(blk); + aio_context = bdrv_get_aio_context(bs); aio_context_acquire(aio_context); - if (!blk_is_available(blk)) { - error_setg(errp, "Device '%s' has no medium", device); - goto out; - } - bs = blk_bs(blk); - image_bs = bdrv_lookup_bs(NULL, image_node_name, &local_err); if (local_err) { error_propagate(errp, local_err); diff --git a/qapi/block-core.json b/qapi/block-core.json index bcb37db5af..d25ba46b77 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -998,7 +998,8 @@ # @image-node-name: The name of the block driver state node of the # image to modify. # -# @device: The name of the device that owns image-node-name. +# @device: The device name or node-name of the root node that owns +# image-node-name. # # @backing-file: The string to write as the backing file. This # string is not validated, so care should be taken diff --git a/qmp-commands.hx b/qmp-commands.hx index eb2736009f..c4ca603c5d 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1806,7 +1806,8 @@ Arguments: "device". (json-string, optional) -- "device": The name of the device. +- "device": The device name or node-name of the root node that owns + image-node-name. (json-string) - "backing-file": The string to write as the backing file. This string is From b7e4fa224200ec87b9599a1d72b16ada35a3d113 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 23 Jun 2016 14:20:24 +0200 Subject: [PATCH 10/36] block: Accept node-name for drive-backup In order to remove the necessity to use BlockBackend names in the external API, we want to allow node-names everywhere. This converts drive-backup and the corresponding transaction action to accept a node-name without lifting the restriction that we're operating at a root node. In case of an invalid device name, the command returns the GenericError error class now instead of DeviceNotFound, because this is what qmp_get_root_bs() returns. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Alberto Garcia Reviewed-by: Max Reitz --- blockdev.c | 36 +++++++++--------------------------- qapi/block-core.json | 4 ++-- qmp-commands.hx | 2 +- tests/qemu-iotests/055 | 7 ++----- 4 files changed, 14 insertions(+), 35 deletions(-) diff --git a/blockdev.c b/blockdev.c index a42c8028ac..773336c1c3 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1856,30 +1856,23 @@ static void do_drive_backup(const char *job_id, const char *device, static void drive_backup_prepare(BlkActionState *common, Error **errp) { DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common); - BlockBackend *blk; + BlockDriverState *bs; DriveBackup *backup; Error *local_err = NULL; assert(common->action->type == TRANSACTION_ACTION_KIND_DRIVE_BACKUP); backup = common->action->u.drive_backup.data; - blk = blk_by_name(backup->device); - if (!blk) { - error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, - "Device '%s' not found", backup->device); - return; - } - - if (!blk_is_available(blk)) { - error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, backup->device); + bs = qmp_get_root_bs(backup->device, errp); + if (!bs) { return; } /* AioContext is released in .clean() */ - state->aio_context = blk_get_aio_context(blk); + state->aio_context = bdrv_get_aio_context(bs); aio_context_acquire(state->aio_context); - bdrv_drained_begin(blk_bs(blk)); - state->bs = blk_bs(blk); + bdrv_drained_begin(bs); + state->bs = bs; do_drive_backup(backup->has_job_id ? backup->job_id : NULL, backup->device, backup->target, @@ -3153,7 +3146,6 @@ static void do_drive_backup(const char *job_id, const char *device, BlockdevOnError on_target_error, BlockJobTxn *txn, Error **errp) { - BlockBackend *blk; BlockDriverState *bs; BlockDriverState *target_bs; BlockDriverState *source = NULL; @@ -3177,24 +3169,14 @@ static void do_drive_backup(const char *job_id, const char *device, mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS; } - blk = blk_by_name(device); - if (!blk) { - error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, - "Device '%s' not found", device); + bs = qmp_get_root_bs(device, errp); + if (!bs) { return; } - aio_context = blk_get_aio_context(blk); + 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 (!blk_is_available(blk)) { - error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, device); - goto out; - } - bs = blk_bs(blk); - if (!has_format) { format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : bs->drv->format_name; } diff --git a/qapi/block-core.json b/qapi/block-core.json index d25ba46b77..f081eb8c56 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -876,7 +876,7 @@ # @job-id: #optional identifier for the newly-created block job. If # omitted, the device name will be used. (Since 2.7) # -# @device: the name of the device which should be copied. +# @device: the device name or node-name of a root node which should be copied. # # @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 @@ -1087,7 +1087,7 @@ # For the arguments, see the documentation of DriveBackup. # # Returns: nothing on success -# If @device is not a valid block device, DeviceNotFound +# If @device is not a valid block device, GenericError # # Since 1.6 ## diff --git a/qmp-commands.hx b/qmp-commands.hx index c4ca603c5d..c5e6cd57aa 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1235,7 +1235,7 @@ Arguments: - "job-id": Identifier for the newly-created block job. If omitted, the device name will be used. (json-string, optional) -- "device": the name of the device which should be copied. +- "device": the device name or node-name of a root node 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 diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055 index c8e3578702..8113c61ade 100755 --- a/tests/qemu-iotests/055 +++ b/tests/qemu-iotests/055 @@ -134,10 +134,7 @@ class TestSingleDrive(iotests.QMPTestCase): def do_test_device_not_found(self, cmd, **args): result = self.vm.qmp(cmd, **args) - if cmd == 'drive-backup': - self.assert_qmp(result, 'error/class', 'DeviceNotFound') - else: - self.assert_qmp(result, 'error/class', 'GenericError') + self.assert_qmp(result, 'error/class', 'GenericError') def test_device_not_found(self): self.do_test_device_not_found('drive-backup', device='nonexistent', @@ -371,7 +368,7 @@ class TestSingleTransaction(iotests.QMPTestCase): 'sync': 'full' }, } ]) - self.assert_qmp(result, 'error/class', 'DeviceNotFound') + self.assert_qmp(result, 'error/class', 'GenericError') result = self.vm.qmp('transaction', actions=[{ 'type': 'blockdev-backup', From 0524e93a3fd7bff5bb4a584c372f2632ab7c0e0f Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 23 Jun 2016 14:20:24 +0200 Subject: [PATCH 11/36] block: Accept node-name for drive-mirror In order to remove the necessity to use BlockBackend names in the external API, we want to allow node-names everywhere. This converts drive-mirror to accept a node-name without lifting the restriction that we're operating at a root node. In case of an invalid device name, the command returns the GenericError error class now instead of DeviceNotFound, because this is what qmp_get_root_bs() returns. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Max Reitz --- blockdev.c | 14 +++----------- qapi/block-core.json | 5 +++-- qmp-commands.hx | 3 ++- tests/qemu-iotests/041 | 8 +++----- 4 files changed, 11 insertions(+), 19 deletions(-) diff --git a/blockdev.c b/blockdev.c index 773336c1c3..a392e08ed2 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3429,7 +3429,6 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs, void qmp_drive_mirror(DriveMirror *arg, Error **errp) { BlockDriverState *bs; - BlockBackend *blk; BlockDriverState *source, *target_bs; AioContext *aio_context; BlockMirrorBackingMode backing_mode; @@ -3439,21 +3438,14 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp) int64_t size; const char *format = arg->format; - blk = blk_by_name(arg->device); - if (!blk) { - error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, - "Device '%s' not found", arg->device); + bs = qmp_get_root_bs(arg->device, errp); + if (!bs) { return; } - aio_context = blk_get_aio_context(blk); + aio_context = bdrv_get_aio_context(bs); aio_context_acquire(aio_context); - if (!blk_is_available(blk)) { - error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, arg->device); - goto out; - } - bs = blk_bs(blk); if (!arg->has_mode) { arg->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS; } diff --git a/qapi/block-core.json b/qapi/block-core.json index f081eb8c56..fba1718cef 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1128,7 +1128,7 @@ # See DriveMirror for parameter descriptions # # Returns: nothing on success -# If @device is not a valid block device, DeviceNotFound +# If @device is not a valid block device, GenericError # # Since 1.3 ## @@ -1143,7 +1143,8 @@ # @job-id: #optional identifier for the newly-created block job. If # omitted, the device name will be used. (Since 2.7) # -# @device: the name of the device whose writes should be mirrored. +# @device: the device name or node-name of a root node 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 diff --git a/qmp-commands.hx b/qmp-commands.hx index c5e6cd57aa..956f1b0f47 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1689,7 +1689,8 @@ Arguments: - "job-id": Identifier for the newly-created block job. If omitted, the device name will be used. (json-string, optional) -- "device": device name to operate on (json-string) +- "device": the device name or node-name of a root node whose writes should be + mirrored. (json-string) - "target": name of new image file (json-string) - "format": format of new image (json-string, optional) - "node-name": the name of the new block driver state in the node graph diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041 index cbf5e0ba5c..80939c0d0d 100755 --- a/tests/qemu-iotests/041 +++ b/tests/qemu-iotests/041 @@ -38,7 +38,6 @@ class TestSingleDrive(iotests.QMPTestCase): image_len = 1 * 1024 * 1024 # MB qmp_cmd = 'drive-mirror' qmp_target = target_img - not_found_error = 'DeviceNotFound' def setUp(self): iotests.create_image(backing_img, self.image_len) @@ -176,7 +175,7 @@ class TestSingleDrive(iotests.QMPTestCase): result = self.vm.qmp(self.qmp_cmd, device='ide1-cd0', sync='full', target=self.qmp_target) - self.assert_qmp(result, 'error/class', self.not_found_error) + self.assert_qmp(result, 'error/class', 'GenericError') def test_image_not_found(self): result = self.vm.qmp(self.qmp_cmd, device='drive0', sync='full', @@ -186,12 +185,11 @@ class TestSingleDrive(iotests.QMPTestCase): def test_device_not_found(self): result = self.vm.qmp(self.qmp_cmd, device='nonexistent', sync='full', target=self.qmp_target) - self.assert_qmp(result, 'error/class', self.not_found_error) + self.assert_qmp(result, 'error/class', 'GenericError') class TestSingleBlockdev(TestSingleDrive): qmp_cmd = 'blockdev-mirror' qmp_target = 'node1' - not_found_error = 'GenericError' def setUp(self): TestSingleDrive.setUp(self) @@ -922,7 +920,7 @@ class TestRepairQuorum(iotests.QMPTestCase): node_name='repair0', replaces='img1', target=quorum_repair_img, format=iotests.imgfmt) - self.assert_qmp(result, 'error/class', 'DeviceNotFound') + self.assert_qmp(result, 'error/class', 'GenericError') def test_wrong_sync_mode(self): if not self.has_quorum(): From cd7fca952ce8456955f7f4e11df9ced14204c2f1 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 6 Jul 2016 11:22:39 +0200 Subject: [PATCH 12/36] nbd-server: Use a separate BlockBackend The builtin NBD server uses its own BlockBackend now instead of reusing the monitor/guest device one. This means that it has its own writethrough setting now. The builtin NBD server always uses writeback caching now regardless of whether the guest device has WCE enabled. qemu-nbd respects the cache mode given on the command line. We still need to keep a reference to the monitor BB because we put an eject notifier on it, but we don't use it for any I/O. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Max Reitz --- block.c | 2 ++ blockdev-nbd.c | 4 ++-- include/block/nbd.h | 3 ++- nbd/server.c | 25 ++++++++++++++++++++----- qemu-nbd.c | 4 ++-- 5 files changed, 28 insertions(+), 10 deletions(-) diff --git a/block.c b/block.c index 30d64e6ca5..101f8c628f 100644 --- a/block.c +++ b/block.c @@ -25,6 +25,7 @@ #include "trace.h" #include "block/block_int.h" #include "block/blockjob.h" +#include "block/nbd.h" #include "qemu/error-report.h" #include "qemu/module.h" #include "qapi/qmp/qerror.h" @@ -2206,6 +2207,7 @@ static void bdrv_close(BlockDriverState *bs) void bdrv_close_all(void) { block_job_cancel_sync_all(); + nbd_export_close_all(); /* Drop references from requests still in flight, such as canceled block * jobs whose AIO context has not been polled yet */ diff --git a/blockdev-nbd.c b/blockdev-nbd.c index 12cae0ea72..c437d32573 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -176,8 +176,8 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable, writable = false; } - exp = nbd_export_new(blk, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY, NULL, - errp); + exp = nbd_export_new(blk_bs(blk), 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY, + NULL, false, blk, errp); if (!exp) { return; } diff --git a/include/block/nbd.h b/include/block/nbd.h index 1897557a9b..80610ff31b 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -103,8 +103,9 @@ int nbd_disconnect(int fd); typedef struct NBDExport NBDExport; typedef struct NBDClient NBDClient; -NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size, +NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size, uint16_t nbdflags, void (*close)(NBDExport *), + bool writethrough, BlockBackend *on_eject_blk, Error **errp); void nbd_export_close(NBDExport *exp); void nbd_export_get(NBDExport *exp); diff --git a/nbd/server.c b/nbd/server.c index 80fbb4da1d..472f584c32 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -69,6 +69,7 @@ struct NBDExport { AioContext *ctx; + BlockBackend *eject_notifier_blk; Notifier eject_notifier; }; @@ -807,11 +808,18 @@ static void nbd_eject_notifier(Notifier *n, void *data) nbd_export_close(exp); } -NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size, +NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size, uint16_t nbdflags, void (*close)(NBDExport *), + bool writethrough, BlockBackend *on_eject_blk, Error **errp) { + BlockBackend *blk; NBDExport *exp = g_malloc0(sizeof(NBDExport)); + + blk = blk_new(); + blk_insert_bs(blk, bs); + blk_set_enable_write_cache(blk, !writethrough); + exp->refcount = 1; QTAILQ_INIT(&exp->clients); exp->blk = blk; @@ -827,11 +835,14 @@ NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size, exp->close = close; exp->ctx = blk_get_aio_context(blk); - blk_ref(blk); blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp); - exp->eject_notifier.notify = nbd_eject_notifier; - blk_add_remove_bs_notifier(blk, &exp->eject_notifier); + if (on_eject_blk) { + blk_ref(on_eject_blk); + exp->eject_notifier_blk = on_eject_blk; + exp->eject_notifier.notify = nbd_eject_notifier; + blk_add_remove_bs_notifier(on_eject_blk, &exp->eject_notifier); + } /* * NBD exports are used for non-shared storage migration. Make sure @@ -844,6 +855,7 @@ NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size, return exp; fail: + blk_unref(blk); g_free(exp); return NULL; } @@ -914,7 +926,10 @@ void nbd_export_put(NBDExport *exp) } if (exp->blk) { - notifier_remove(&exp->eject_notifier); + if (exp->eject_notifier_blk) { + notifier_remove(&exp->eject_notifier); + blk_unref(exp->eject_notifier_blk); + } blk_remove_aio_context_notifier(exp->blk, blk_aio_attached, blk_aio_detach, exp); blk_unref(exp->blk); diff --git a/qemu-nbd.c b/qemu-nbd.c index e3571c2025..99297a556f 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -910,8 +910,8 @@ int main(int argc, char **argv) } } - exp = nbd_export_new(blk, dev_offset, fd_size, nbdflags, nbd_export_closed, - &local_err); + exp = nbd_export_new(bs, dev_offset, fd_size, nbdflags, nbd_export_closed, + writethrough, NULL, &local_err); if (!exp) { error_report_err(local_err); exit(EXIT_FAILURE); From 094138d09e78443a4b6ec9532398e0ae98323319 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 6 Jul 2016 14:15:51 +0200 Subject: [PATCH 13/36] nbd-server: Allow node name for nbd-server-add There is no reason why an NBD server couldn't be started for any node, even if it's not on the top level. This converts nbd-server-add to accept a node-name. Note that there is a semantic difference between using a BlockBackend name and the node name of its root: In the former case, the NBD server is closed on eject; in the latter case, the NBD server doesn't drop its reference and keeps the image file open this way. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz --- blockdev-nbd.c | 21 +++++++++------------ qapi/block.json | 4 ++-- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/blockdev-nbd.c b/blockdev-nbd.c index c437d32573..ca41cc6fdd 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -145,7 +145,8 @@ void qmp_nbd_server_start(SocketAddress *addr, void qmp_nbd_server_add(const char *device, bool has_writable, bool writable, Error **errp) { - BlockBackend *blk; + BlockDriverState *bs = NULL; + BlockBackend *on_eject_blk; NBDExport *exp; if (!nbd_server) { @@ -158,26 +159,22 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable, return; } - blk = blk_by_name(device); - if (!blk) { - error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, - "Device '%s' not found", device); - return; - } - if (!blk_is_inserted(blk)) { - error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, device); + on_eject_blk = blk_by_name(device); + + bs = bdrv_lookup_bs(device, device, errp); + if (!bs) { return; } if (!has_writable) { writable = false; } - if (blk_is_read_only(blk)) { + if (bdrv_is_read_only(bs)) { writable = false; } - exp = nbd_export_new(blk_bs(blk), 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY, - NULL, false, blk, errp); + exp = nbd_export_new(bs, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY, + NULL, false, on_eject_blk, errp); if (!exp) { return; } diff --git a/qapi/block.json b/qapi/block.json index db05169187..8b08bd2fd0 100644 --- a/qapi/block.json +++ b/qapi/block.json @@ -161,9 +161,9 @@ ## # @nbd-server-add: # -# Export a device to QEMU's embedded NBD server. +# Export a block node to QEMU's embedded NBD server. # -# @device: Block device to be exported +# @device: The device name or node name of the node to be exported # # @writable: Whether clients should be able to write to the device via the # NBD connection (default false). #optional From fe5c1355e73940dbde9b38dec6e8fab4117ec637 Mon Sep 17 00:00:00 2001 From: Pavel Butsykin Date: Fri, 22 Jul 2016 11:17:40 +0300 Subject: [PATCH 14/36] block: switch blk_write_compressed() to byte-based interface This is a preparatory patch, which continues the general trend of the transition to the byte-based interfaces. bdrv_check_request() and blk_check_request() are no longer used, thus we can remove them. Signed-off-by: Pavel Butsykin Reviewed-by: Stefan Hajnoczi Reviewed-by: Eric Blake Signed-off-by: Denis V. Lunev CC: Jeff Cody CC: Markus Armbruster CC: Eric Blake CC: John Snow CC: Stefan Hajnoczi CC: Kevin Wolf Signed-off-by: Kevin Wolf --- block/block-backend.c | 23 ++++------------------- block/io.c | 22 +++++++--------------- include/block/block.h | 4 ++-- include/sysemu/block-backend.h | 4 ++-- qemu-img.c | 6 ++++-- qemu-io-cmds.c | 2 +- 6 files changed, 20 insertions(+), 41 deletions(-) diff --git a/block/block-backend.c b/block/block-backend.c index dc2bc60b9e..4c704a134f 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -743,21 +743,6 @@ static int blk_check_byte_request(BlockBackend *blk, int64_t offset, return 0; } -static int blk_check_request(BlockBackend *blk, int64_t sector_num, - int nb_sectors) -{ - if (sector_num < 0 || sector_num > INT64_MAX / BDRV_SECTOR_SIZE) { - return -EIO; - } - - if (nb_sectors < 0 || nb_sectors > INT_MAX / BDRV_SECTOR_SIZE) { - return -EIO; - } - - return blk_check_byte_request(blk, sector_num * BDRV_SECTOR_SIZE, - nb_sectors * BDRV_SECTOR_SIZE); -} - int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset, unsigned int bytes, QEMUIOVector *qiov, BdrvRequestFlags flags) @@ -1500,15 +1485,15 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset, flags | BDRV_REQ_ZERO_WRITE); } -int blk_write_compressed(BlockBackend *blk, int64_t sector_num, - const uint8_t *buf, int nb_sectors) +int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf, + int count) { - int ret = blk_check_request(blk, sector_num, nb_sectors); + int ret = blk_check_byte_request(blk, offset, count); if (ret < 0) { return ret; } - return bdrv_write_compressed(blk_bs(blk), sector_num, buf, nb_sectors); + return bdrv_pwrite_compressed(blk_bs(blk), offset, buf, count); } int blk_truncate(BlockBackend *blk, int64_t offset) diff --git a/block/io.c b/block/io.c index 420944d80d..da874d0f19 100644 --- a/block/io.c +++ b/block/io.c @@ -540,17 +540,6 @@ static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset, return 0; } -static int bdrv_check_request(BlockDriverState *bs, int64_t sector_num, - int nb_sectors) -{ - if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) { - return -EIO; - } - - return bdrv_check_byte_request(bs, sector_num * BDRV_SECTOR_SIZE, - nb_sectors * BDRV_SECTOR_SIZE); -} - typedef struct RwCo { BdrvChild *child; int64_t offset; @@ -1879,8 +1868,8 @@ int bdrv_is_allocated_above(BlockDriverState *top, return 0; } -int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num, - const uint8_t *buf, int nb_sectors) +int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset, + const void *buf, int bytes) { BlockDriver *drv = bs->drv; int ret; @@ -1891,14 +1880,17 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num, if (!drv->bdrv_write_compressed) { return -ENOTSUP; } - ret = bdrv_check_request(bs, sector_num, nb_sectors); + ret = bdrv_check_byte_request(bs, offset, bytes); if (ret < 0) { return ret; } assert(QLIST_EMPTY(&bs->dirty_bitmaps)); + assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0); + assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0); - return drv->bdrv_write_compressed(bs, sector_num, buf, nb_sectors); + return drv->bdrv_write_compressed(bs, offset >> BDRV_SECTOR_BITS, buf, + bytes >> BDRV_SECTOR_BITS); } typedef struct BdrvVmstateCo { diff --git a/include/block/block.h b/include/block/block.h index 11c162d594..b4a97f2612 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -399,8 +399,8 @@ const char *bdrv_get_node_name(const BlockDriverState *bs); const char *bdrv_get_device_name(const BlockDriverState *bs); const char *bdrv_get_device_or_node_name(const BlockDriverState *bs); int bdrv_get_flags(BlockDriverState *bs); -int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num, - const uint8_t *buf, int nb_sectors); +int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset, + const void *buf, int bytes); int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi); ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs); void bdrv_round_sectors_to_clusters(BlockDriverState *bs, diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h index bb4fa82d1c..4808a9621a 100644 --- a/include/sysemu/block-backend.h +++ b/include/sysemu/block-backend.h @@ -204,8 +204,8 @@ void *blk_aio_get(const AIOCBInfo *aiocb_info, BlockBackend *blk, BlockCompletionFunc *cb, void *opaque); int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset, int count, BdrvRequestFlags flags); -int blk_write_compressed(BlockBackend *blk, int64_t sector_num, - const uint8_t *buf, int nb_sectors); +int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf, + int count); int blk_truncate(BlockBackend *blk, int64_t offset); int blk_pdiscard(BlockBackend *blk, int64_t offset, int count); int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf, diff --git a/qemu-img.c b/qemu-img.c index f204d04136..ef0157aa8a 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1590,7 +1590,9 @@ static int convert_write(ImgConvertState *s, int64_t sector_num, int nb_sectors, break; } - ret = blk_write_compressed(s->target, sector_num, buf, n); + ret = blk_pwrite_compressed(s->target, + sector_num << BDRV_SECTOR_BITS, + buf, n << BDRV_SECTOR_BITS); if (ret < 0) { return ret; } @@ -1727,7 +1729,7 @@ static int convert_do_copy(ImgConvertState *s) if (s->compressed) { /* signal EOF to align */ - ret = blk_write_compressed(s->target, 0, NULL, 0); + ret = blk_pwrite_compressed(s->target, 0, NULL, 0); if (ret < 0) { goto fail; } diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c index 25954f5634..3a3838a079 100644 --- a/qemu-io-cmds.c +++ b/qemu-io-cmds.c @@ -504,7 +504,7 @@ static int do_write_compressed(BlockBackend *blk, char *buf, int64_t offset, return -ERANGE; } - ret = blk_write_compressed(blk, offset >> 9, (uint8_t *)buf, count >> 9); + ret = blk_pwrite_compressed(blk, offset, buf, count); if (ret < 0) { return ret; } From 751e2f0698c284b4924d83ec63fa4e834bf4c80d Mon Sep 17 00:00:00 2001 From: Pavel Butsykin Date: Fri, 22 Jul 2016 11:17:41 +0300 Subject: [PATCH 15/36] block: Convert bdrv_pwrite_compressed() to BdrvChild Signed-off-by: Pavel Butsykin Signed-off-by: Denis V. Lunev Reviewed-by: Eric Blake CC: Jeff Cody CC: Markus Armbruster CC: Eric Blake CC: John Snow CC: Stefan Hajnoczi CC: Kevin Wolf Signed-off-by: Kevin Wolf --- block/block-backend.c | 2 +- block/io.c | 3 ++- include/block/block.h | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/block/block-backend.c b/block/block-backend.c index 4c704a134f..76ea45955f 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -1493,7 +1493,7 @@ int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf, return ret; } - return bdrv_pwrite_compressed(blk_bs(blk), offset, buf, count); + return bdrv_pwrite_compressed(blk->root, offset, buf, count); } int blk_truncate(BlockBackend *blk, int64_t offset) diff --git a/block/io.c b/block/io.c index da874d0f19..c528fead1b 100644 --- a/block/io.c +++ b/block/io.c @@ -1868,9 +1868,10 @@ int bdrv_is_allocated_above(BlockDriverState *top, return 0; } -int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset, +int bdrv_pwrite_compressed(BdrvChild *child, int64_t offset, const void *buf, int bytes) { + BlockDriverState *bs = child->bs; BlockDriver *drv = bs->drv; int ret; diff --git a/include/block/block.h b/include/block/block.h index b4a97f2612..7bb5ddbf73 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -399,7 +399,7 @@ const char *bdrv_get_node_name(const BlockDriverState *bs); const char *bdrv_get_device_name(const BlockDriverState *bs); const char *bdrv_get_device_or_node_name(const BlockDriverState *bs); int bdrv_get_flags(BlockDriverState *bs); -int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset, +int bdrv_pwrite_compressed(BdrvChild *child, int64_t offset, const void *buf, int bytes); int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi); ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs); From 29a298af9d2479cc230505b18d5a5c2da0ab578e Mon Sep 17 00:00:00 2001 From: Pavel Butsykin Date: Fri, 22 Jul 2016 11:17:42 +0300 Subject: [PATCH 16/36] block/io: reuse bdrv_co_pwritev() for write compressed For bdrv_pwrite_compressed() it looks like most of the code creating coroutine is duplicated in bdrv_prwv_co(). So we can just add a flag (BDRV_REQ_WRITE_COMPRESSED) and use bdrv_prwv_co() as a generic one. In the end we get coroutine oriented function for write compressed by using bdrv_co_pwritev/blk_co_pwritev with BDRV_REQ_WRITE_COMPRESSED flag. Signed-off-by: Pavel Butsykin Reviewed-by: Stefan Hajnoczi Signed-off-by: Denis V. Lunev CC: Jeff Cody CC: Markus Armbruster CC: Eric Blake CC: John Snow CC: Stefan Hajnoczi CC: Kevin Wolf Signed-off-by: Kevin Wolf --- block/io.c | 56 ++++++++++++++++++++++++++++----------- include/block/block.h | 3 ++- include/block/block_int.h | 3 +++ qemu-img.c | 2 +- 4 files changed, 46 insertions(+), 18 deletions(-) diff --git a/block/io.c b/block/io.c index c528fead1b..d402076e95 100644 --- a/block/io.c +++ b/block/io.c @@ -886,6 +886,20 @@ emulate_flags: return ret; } +static int coroutine_fn +bdrv_driver_pwritev_compressed(BlockDriverState *bs, uint64_t offset, + uint64_t bytes, QEMUIOVector *qiov) +{ + BlockDriver *drv = bs->drv; + + if (!drv->bdrv_co_pwritev_compressed) { + return -ENOTSUP; + } + + assert(QLIST_EMPTY(&bs->dirty_bitmaps)); + return drv->bdrv_co_pwritev_compressed(bs, offset, bytes, qiov); +} + static int coroutine_fn bdrv_co_do_copy_on_readv(BlockDriverState *bs, int64_t offset, unsigned int bytes, QEMUIOVector *qiov) { @@ -1555,9 +1569,14 @@ int coroutine_fn bdrv_co_pwritev(BdrvChild *child, bytes = ROUND_UP(bytes, align); } - ret = bdrv_aligned_pwritev(bs, &req, offset, bytes, align, - use_local_qiov ? &local_qiov : qiov, - flags); + if (flags & BDRV_REQ_WRITE_COMPRESSED) { + ret = bdrv_driver_pwritev_compressed( + bs, offset, bytes, use_local_qiov ? &local_qiov : qiov); + } else { + ret = bdrv_aligned_pwritev(bs, &req, offset, bytes, align, + use_local_qiov ? &local_qiov : qiov, + flags); + } fail: @@ -1873,25 +1892,30 @@ int bdrv_pwrite_compressed(BdrvChild *child, int64_t offset, { BlockDriverState *bs = child->bs; BlockDriver *drv = bs->drv; - int ret; + QEMUIOVector qiov; + struct iovec iov; if (!drv) { return -ENOMEDIUM; } - if (!drv->bdrv_write_compressed) { - return -ENOTSUP; - } - ret = bdrv_check_byte_request(bs, offset, bytes); - if (ret < 0) { - return ret; + if (drv->bdrv_write_compressed) { + int ret = bdrv_check_byte_request(bs, offset, bytes); + if (ret < 0) { + return ret; + } + assert(QLIST_EMPTY(&bs->dirty_bitmaps)); + assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0); + assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0); + return drv->bdrv_write_compressed(bs, offset >> BDRV_SECTOR_BITS, buf, + bytes >> BDRV_SECTOR_BITS); } + iov = (struct iovec) { + .iov_base = (void *)buf, + .iov_len = bytes, + }; + qemu_iovec_init_external(&qiov, &iov, 1); - assert(QLIST_EMPTY(&bs->dirty_bitmaps)); - assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0); - assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0); - - return drv->bdrv_write_compressed(bs, offset >> BDRV_SECTOR_BITS, buf, - bytes >> BDRV_SECTOR_BITS); + return bdrv_prwv_co(child, offset, &qiov, true, BDRV_REQ_WRITE_COMPRESSED); } typedef struct BdrvVmstateCo { diff --git a/include/block/block.h b/include/block/block.h index 7bb5ddbf73..d8dacd2ced 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -65,9 +65,10 @@ typedef enum { BDRV_REQ_MAY_UNMAP = 0x4, BDRV_REQ_NO_SERIALISING = 0x8, BDRV_REQ_FUA = 0x10, + BDRV_REQ_WRITE_COMPRESSED = 0x20, /* Mask of valid flags */ - BDRV_REQ_MASK = 0x1f, + BDRV_REQ_MASK = 0x3f, } BdrvRequestFlags; typedef struct BlockSizes { diff --git a/include/block/block_int.h b/include/block/block_int.h index 1e939de4fe..42f8f8443d 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -207,6 +207,9 @@ struct BlockDriver { int (*bdrv_write_compressed)(BlockDriverState *bs, int64_t sector_num, const uint8_t *buf, int nb_sectors); + int coroutine_fn (*bdrv_co_pwritev_compressed)(BlockDriverState *bs, + uint64_t offset, uint64_t bytes, QEMUIOVector *qiov); + int (*bdrv_snapshot_create)(BlockDriverState *bs, QEMUSnapshotInfo *sn_info); int (*bdrv_snapshot_goto)(BlockDriverState *bs, diff --git a/qemu-img.c b/qemu-img.c index ef0157aa8a..c2ea494928 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -2034,7 +2034,7 @@ static int img_convert(int argc, char **argv) const char *preallocation = qemu_opt_get(opts, BLOCK_OPT_PREALLOC); - if (!drv->bdrv_write_compressed) { + if (!drv->bdrv_write_compressed && !drv->bdrv_co_pwritev_compressed) { error_report("Compression not supported for this file format"); ret = -1; goto out; From fcccefc57f235c0928e60bba0b7f6084677ed3df Mon Sep 17 00:00:00 2001 From: Pavel Butsykin Date: Fri, 22 Jul 2016 11:17:43 +0300 Subject: [PATCH 17/36] qcow2: add qcow2_co_pwritev_compressed Added implementation of the qcow2_co_pwritev_compressed function that will allow us to safely use compressed writes for the qcow2 from running VMs. Signed-off-by: Pavel Butsykin Reviewed-by: Stefan Hajnoczi Signed-off-by: Denis V. Lunev CC: Jeff Cody CC: Markus Armbruster CC: Eric Blake CC: John Snow CC: Stefan Hajnoczi CC: Kevin Wolf Signed-off-by: Kevin Wolf --- block/qcow2.c | 128 ++++++++++++++++++++------------------------------ 1 file changed, 52 insertions(+), 76 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index 91ef4dfefc..1cbaa8f030 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2533,84 +2533,49 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset) return 0; } -typedef struct Qcow2WriteCo { - BlockDriverState *bs; - int64_t sector_num; - const uint8_t *buf; - int nb_sectors; - int ret; -} Qcow2WriteCo; - -static void qcow2_write_co_entry(void *opaque) -{ - Qcow2WriteCo *co = opaque; - QEMUIOVector qiov; - uint64_t offset = co->sector_num * BDRV_SECTOR_SIZE; - uint64_t bytes = co->nb_sectors * BDRV_SECTOR_SIZE; - - struct iovec iov = (struct iovec) { - .iov_base = (uint8_t*) co->buf, - .iov_len = bytes, - }; - qemu_iovec_init_external(&qiov, &iov, 1); - - co->ret = qcow2_co_pwritev(co->bs, offset, bytes, &qiov, 0); -} - -/* Wrapper for non-coroutine contexts */ -static int qcow2_write(BlockDriverState *bs, int64_t sector_num, - const uint8_t *buf, int nb_sectors) -{ - Coroutine *co; - AioContext *aio_context = bdrv_get_aio_context(bs); - Qcow2WriteCo data = { - .bs = bs, - .sector_num = sector_num, - .buf = buf, - .nb_sectors = nb_sectors, - .ret = -EINPROGRESS, - }; - co = qemu_coroutine_create(qcow2_write_co_entry, &data); - qemu_coroutine_enter(co); - while (data.ret == -EINPROGRESS) { - aio_poll(aio_context, true); - } - return data.ret; -} - /* XXX: put compressed sectors first, then all the cluster aligned tables to avoid losing bytes in alignment */ -static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num, - const uint8_t *buf, int nb_sectors) +static coroutine_fn int +qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset, + uint64_t bytes, QEMUIOVector *qiov) { BDRVQcow2State *s = bs->opaque; + QEMUIOVector hd_qiov; + struct iovec iov; z_stream strm; int ret, out_len; - uint8_t *out_buf; + uint8_t *buf, *out_buf; uint64_t cluster_offset; - if (nb_sectors == 0) { + if (bytes == 0) { /* align end of file to a sector boundary to ease reading with sector based I/Os */ cluster_offset = bdrv_getlength(bs->file->bs); return bdrv_truncate(bs->file->bs, cluster_offset); } - if (nb_sectors != s->cluster_sectors) { + if (bytes != s->cluster_size) { ret = -EINVAL; /* Zero-pad last write if image size is not cluster aligned */ - if (sector_num + nb_sectors == bs->total_sectors && - nb_sectors < s->cluster_sectors) { + if (offset + bytes == bs->total_sectors << BDRV_SECTOR_BITS && + bytes < s->cluster_size) + { uint8_t *pad_buf = qemu_blockalign(bs, s->cluster_size); memset(pad_buf, 0, s->cluster_size); - memcpy(pad_buf, buf, nb_sectors * BDRV_SECTOR_SIZE); - ret = qcow2_write_compressed(bs, sector_num, - pad_buf, s->cluster_sectors); + qemu_iovec_to_buf(qiov, 0, pad_buf, s->cluster_size); + iov = (struct iovec) { + .iov_base = pad_buf, + .iov_len = s->cluster_size, + }; + qemu_iovec_init_external(&hd_qiov, &iov, 1); + ret = qcow2_co_pwritev_compressed(bs, offset, bytes, &hd_qiov); qemu_vfree(pad_buf); } return ret; } + buf = qemu_blockalign(bs, s->cluster_size); + qemu_iovec_to_buf(qiov, 0, buf, s->cluster_size); out_buf = g_malloc(s->cluster_size); @@ -2641,33 +2606,44 @@ static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num, if (ret != Z_STREAM_END || out_len >= s->cluster_size) { /* could not compress: write normal cluster */ - ret = qcow2_write(bs, sector_num, buf, s->cluster_sectors); - if (ret < 0) { - goto fail; - } - } else { - cluster_offset = qcow2_alloc_compressed_cluster_offset(bs, - sector_num << 9, out_len); - if (!cluster_offset) { - ret = -EIO; - goto fail; - } - cluster_offset &= s->cluster_offset_mask; - - ret = qcow2_pre_write_overlap_check(bs, 0, cluster_offset, out_len); - if (ret < 0) { - goto fail; - } - - BLKDBG_EVENT(bs->file, BLKDBG_WRITE_COMPRESSED); - ret = bdrv_pwrite(bs->file, cluster_offset, out_buf, out_len); + ret = qcow2_co_pwritev(bs, offset, bytes, qiov, 0); if (ret < 0) { goto fail; } + goto success; } + qemu_co_mutex_lock(&s->lock); + cluster_offset = + qcow2_alloc_compressed_cluster_offset(bs, offset, out_len); + if (!cluster_offset) { + qemu_co_mutex_unlock(&s->lock); + ret = -EIO; + goto fail; + } + cluster_offset &= s->cluster_offset_mask; + + ret = qcow2_pre_write_overlap_check(bs, 0, cluster_offset, out_len); + qemu_co_mutex_unlock(&s->lock); + if (ret < 0) { + goto fail; + } + + iov = (struct iovec) { + .iov_base = out_buf, + .iov_len = out_len, + }; + qemu_iovec_init_external(&hd_qiov, &iov, 1); + + BLKDBG_EVENT(bs->file, BLKDBG_WRITE_COMPRESSED); + ret = bdrv_co_pwritev(bs->file, cluster_offset, out_len, &hd_qiov, 0); + if (ret < 0) { + goto fail; + } +success: ret = 0; fail: + qemu_vfree(buf); g_free(out_buf); return ret; } @@ -3412,7 +3388,7 @@ BlockDriver bdrv_qcow2 = { .bdrv_co_pwrite_zeroes = qcow2_co_pwrite_zeroes, .bdrv_co_pdiscard = qcow2_co_pdiscard, .bdrv_truncate = qcow2_truncate, - .bdrv_write_compressed = qcow2_write_compressed, + .bdrv_co_pwritev_compressed = qcow2_co_pwritev_compressed, .bdrv_make_empty = qcow2_make_empty, .bdrv_snapshot_create = qcow2_snapshot_create, From a2c0ca6f55fbf6cbb5540dea9783f22d99aca528 Mon Sep 17 00:00:00 2001 From: Pavel Butsykin Date: Fri, 22 Jul 2016 11:17:44 +0300 Subject: [PATCH 18/36] qcow2: cleanup qcow2_co_pwritev_compressed to avoid the recursion Now that the function uses a vector instead of a buffer, there is no need to use recursive code. Signed-off-by: Pavel Butsykin Reviewed-by: Stefan Hajnoczi Signed-off-by: Denis V. Lunev CC: Jeff Cody CC: Markus Armbruster CC: Eric Blake CC: John Snow CC: Stefan Hajnoczi CC: Kevin Wolf Signed-off-by: Kevin Wolf --- block/qcow2.c | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index 1cbaa8f030..adf451491f 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2554,27 +2554,17 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset, return bdrv_truncate(bs->file->bs, cluster_offset); } - if (bytes != s->cluster_size) { - ret = -EINVAL; - - /* Zero-pad last write if image size is not cluster aligned */ - if (offset + bytes == bs->total_sectors << BDRV_SECTOR_BITS && - bytes < s->cluster_size) - { - uint8_t *pad_buf = qemu_blockalign(bs, s->cluster_size); - memset(pad_buf, 0, s->cluster_size); - qemu_iovec_to_buf(qiov, 0, pad_buf, s->cluster_size); - iov = (struct iovec) { - .iov_base = pad_buf, - .iov_len = s->cluster_size, - }; - qemu_iovec_init_external(&hd_qiov, &iov, 1); - ret = qcow2_co_pwritev_compressed(bs, offset, bytes, &hd_qiov); - qemu_vfree(pad_buf); - } - return ret; - } buf = qemu_blockalign(bs, s->cluster_size); + if (bytes != s->cluster_size) { + if (bytes > s->cluster_size || + offset + bytes != bs->total_sectors << BDRV_SECTOR_BITS) + { + qemu_vfree(buf); + return -EINVAL; + } + /* Zero-pad last write if image size is not cluster aligned */ + memset(buf + bytes, 0, s->cluster_size - bytes); + } qemu_iovec_to_buf(qiov, 0, buf, s->cluster_size); out_buf = g_malloc(s->cluster_size); From b2c622d365e9eea3ddc83d71c8af19e1bca6dbb1 Mon Sep 17 00:00:00 2001 From: Pavel Butsykin Date: Fri, 22 Jul 2016 11:17:45 +0300 Subject: [PATCH 19/36] vmdk: add vmdk_co_pwritev_compressed Added implementation of the vmdk_co_pwritev_compressed function that will allow us to safely use compressed writes for the vmdk from running VMs. Signed-off-by: Pavel Butsykin Reviewed-by: Stefan Hajnoczi Signed-off-by: Denis V. Lunev CC: Jeff Cody CC: Markus Armbruster CC: Eric Blake CC: John Snow CC: Stefan Hajnoczi CC: Kevin Wolf Signed-off-by: Kevin Wolf --- block/vmdk.c | 55 +++++----------------------------------------------- 1 file changed, 5 insertions(+), 50 deletions(-) diff --git a/block/vmdk.c b/block/vmdk.c index 46d474e442..a11c27a1c4 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -1645,56 +1645,11 @@ vmdk_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes, return ret; } -typedef struct VmdkWriteCompressedCo { - BlockDriverState *bs; - int64_t sector_num; - const uint8_t *buf; - int nb_sectors; - int ret; -} VmdkWriteCompressedCo; - -static void vmdk_co_write_compressed(void *opaque) +static int coroutine_fn +vmdk_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset, + uint64_t bytes, QEMUIOVector *qiov) { - VmdkWriteCompressedCo *co = opaque; - QEMUIOVector local_qiov; - uint64_t offset = co->sector_num * BDRV_SECTOR_SIZE; - uint64_t bytes = co->nb_sectors * BDRV_SECTOR_SIZE; - - struct iovec iov = (struct iovec) { - .iov_base = (uint8_t*) co->buf, - .iov_len = bytes, - }; - qemu_iovec_init_external(&local_qiov, &iov, 1); - - co->ret = vmdk_pwritev(co->bs, offset, bytes, &local_qiov, false, false); -} - -static int vmdk_write_compressed(BlockDriverState *bs, - int64_t sector_num, - const uint8_t *buf, - int nb_sectors) -{ - BDRVVmdkState *s = bs->opaque; - - if (s->num_extents == 1 && s->extents[0].compressed) { - Coroutine *co; - AioContext *aio_context = bdrv_get_aio_context(bs); - VmdkWriteCompressedCo data = { - .bs = bs, - .sector_num = sector_num, - .buf = buf, - .nb_sectors = nb_sectors, - .ret = -EINPROGRESS, - }; - co = qemu_coroutine_create(vmdk_co_write_compressed, &data); - qemu_coroutine_enter(co); - while (data.ret == -EINPROGRESS) { - aio_poll(aio_context, true); - } - return data.ret; - } else { - return -ENOTSUP; - } + return vmdk_co_pwritev(bs, offset, bytes, qiov, 0); } static int coroutine_fn vmdk_co_pwrite_zeroes(BlockDriverState *bs, @@ -2393,7 +2348,7 @@ static BlockDriver bdrv_vmdk = { .bdrv_reopen_prepare = vmdk_reopen_prepare, .bdrv_co_preadv = vmdk_co_preadv, .bdrv_co_pwritev = vmdk_co_pwritev, - .bdrv_write_compressed = vmdk_write_compressed, + .bdrv_co_pwritev_compressed = vmdk_co_pwritev_compressed, .bdrv_co_pwrite_zeroes = vmdk_co_pwrite_zeroes, .bdrv_close = vmdk_close, .bdrv_create = vmdk_create, From f2b95a12317bf827f3f734353ffe0e1ec41942ae Mon Sep 17 00:00:00 2001 From: Pavel Butsykin Date: Fri, 22 Jul 2016 11:17:46 +0300 Subject: [PATCH 20/36] qcow: add qcow_co_pwritev_compressed Added implementation of the qcow_co_pwritev_compressed function that will allow us to safely use compressed writes for the qcow from running VMs. Signed-off-by: Pavel Butsykin Reviewed-by: Stefan Hajnoczi Signed-off-by: Denis V. Lunev CC: Jeff Cody CC: Markus Armbruster CC: Eric Blake CC: John Snow CC: Stefan Hajnoczi CC: Kevin Wolf Signed-off-by: Kevin Wolf --- block/qcow.c | 109 ++++++++++++++++++++------------------------------- 1 file changed, 42 insertions(+), 67 deletions(-) diff --git a/block/qcow.c b/block/qcow.c index 6f9b2e2d26..1b9c91128a 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -913,75 +913,42 @@ static int qcow_make_empty(BlockDriverState *bs) return 0; } -typedef struct QcowWriteCo { - BlockDriverState *bs; - int64_t sector_num; - const uint8_t *buf; - int nb_sectors; - int ret; -} QcowWriteCo; - -static void qcow_write_co_entry(void *opaque) -{ - QcowWriteCo *co = opaque; - QEMUIOVector qiov; - - struct iovec iov = (struct iovec) { - .iov_base = (uint8_t*) co->buf, - .iov_len = co->nb_sectors * BDRV_SECTOR_SIZE, - }; - qemu_iovec_init_external(&qiov, &iov, 1); - - co->ret = qcow_co_writev(co->bs, co->sector_num, co->nb_sectors, &qiov); -} - -/* Wrapper for non-coroutine contexts */ -static int qcow_write(BlockDriverState *bs, int64_t sector_num, - const uint8_t *buf, int nb_sectors) -{ - Coroutine *co; - AioContext *aio_context = bdrv_get_aio_context(bs); - QcowWriteCo data = { - .bs = bs, - .sector_num = sector_num, - .buf = buf, - .nb_sectors = nb_sectors, - .ret = -EINPROGRESS, - }; - co = qemu_coroutine_create(qcow_write_co_entry, &data); - qemu_coroutine_enter(co); - while (data.ret == -EINPROGRESS) { - aio_poll(aio_context, true); - } - return data.ret; -} - /* XXX: put compressed sectors first, then all the cluster aligned tables to avoid losing bytes in alignment */ -static int qcow_write_compressed(BlockDriverState *bs, int64_t sector_num, - const uint8_t *buf, int nb_sectors) +static coroutine_fn int +qcow_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset, + uint64_t bytes, QEMUIOVector *qiov) { BDRVQcowState *s = bs->opaque; + QEMUIOVector hd_qiov; + struct iovec iov; z_stream strm; int ret, out_len; - uint8_t *out_buf; + uint8_t *buf, *out_buf; uint64_t cluster_offset; - if (nb_sectors != s->cluster_sectors) { + if (bytes != s->cluster_size) { ret = -EINVAL; /* Zero-pad last write if image size is not cluster aligned */ - if (sector_num + nb_sectors == bs->total_sectors && - nb_sectors < s->cluster_sectors) { + if (offset + bytes == bs->total_sectors << BDRV_SECTOR_BITS && + bytes < s->cluster_size) + { uint8_t *pad_buf = qemu_blockalign(bs, s->cluster_size); memset(pad_buf, 0, s->cluster_size); - memcpy(pad_buf, buf, nb_sectors * BDRV_SECTOR_SIZE); - ret = qcow_write_compressed(bs, sector_num, - pad_buf, s->cluster_sectors); + qemu_iovec_to_buf(qiov, 0, pad_buf, s->cluster_size); + iov = (struct iovec) { + .iov_base = pad_buf, + .iov_len = s->cluster_size, + }; + qemu_iovec_init_external(&hd_qiov, &iov, 1); + ret = qcow_co_pwritev_compressed(bs, offset, bytes, &hd_qiov); qemu_vfree(pad_buf); } return ret; } + buf = qemu_blockalign(bs, s->cluster_size); + qemu_iovec_to_buf(qiov, 0, buf, qiov->size); out_buf = g_malloc(s->cluster_size); @@ -1012,27 +979,35 @@ static int qcow_write_compressed(BlockDriverState *bs, int64_t sector_num, if (ret != Z_STREAM_END || out_len >= s->cluster_size) { /* could not compress: write normal cluster */ - ret = qcow_write(bs, sector_num, buf, s->cluster_sectors); - if (ret < 0) { - goto fail; - } - } else { - cluster_offset = get_cluster_offset(bs, sector_num << 9, 2, - out_len, 0, 0); - if (cluster_offset == 0) { - ret = -EIO; - goto fail; - } - - cluster_offset &= s->cluster_offset_mask; - ret = bdrv_pwrite(bs->file, cluster_offset, out_buf, out_len); + ret = qcow_co_writev(bs, offset >> BDRV_SECTOR_BITS, + bytes >> BDRV_SECTOR_BITS, qiov); if (ret < 0) { goto fail; } + goto success; } + qemu_co_mutex_lock(&s->lock); + cluster_offset = get_cluster_offset(bs, offset, 2, out_len, 0, 0); + qemu_co_mutex_unlock(&s->lock); + if (cluster_offset == 0) { + ret = -EIO; + goto fail; + } + cluster_offset &= s->cluster_offset_mask; + iov = (struct iovec) { + .iov_base = out_buf, + .iov_len = out_len, + }; + qemu_iovec_init_external(&hd_qiov, &iov, 1); + ret = bdrv_co_pwritev(bs->file, cluster_offset, out_len, &hd_qiov, 0); + if (ret < 0) { + goto fail; + } +success: ret = 0; fail: + qemu_vfree(buf); g_free(out_buf); return ret; } @@ -1085,7 +1060,7 @@ static BlockDriver bdrv_qcow = { .bdrv_set_key = qcow_set_key, .bdrv_make_empty = qcow_make_empty, - .bdrv_write_compressed = qcow_write_compressed, + .bdrv_co_pwritev_compressed = qcow_co_pwritev_compressed, .bdrv_get_info = qcow_get_info, .create_opts = &qcow_create_opts, From 655923df4be82ac23efc6862d35f569d05824e42 Mon Sep 17 00:00:00 2001 From: Pavel Butsykin Date: Fri, 22 Jul 2016 11:17:47 +0300 Subject: [PATCH 21/36] qcow: cleanup qcow_co_pwritev_compressed to avoid the recursion Now that the function uses a vector instead of a buffer, there is no need to use recursive code. Signed-off-by: Pavel Butsykin Reviewed-by: Stefan Hajnoczi Signed-off-by: Denis V. Lunev CC: Jeff Cody CC: Markus Armbruster CC: Eric Blake CC: John Snow CC: Stefan Hajnoczi CC: Kevin Wolf Signed-off-by: Kevin Wolf --- block/qcow.c | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/block/qcow.c b/block/qcow.c index 1b9c91128a..94f01b3d0c 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -927,27 +927,17 @@ qcow_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset, uint8_t *buf, *out_buf; uint64_t cluster_offset; - if (bytes != s->cluster_size) { - ret = -EINVAL; - - /* Zero-pad last write if image size is not cluster aligned */ - if (offset + bytes == bs->total_sectors << BDRV_SECTOR_BITS && - bytes < s->cluster_size) - { - uint8_t *pad_buf = qemu_blockalign(bs, s->cluster_size); - memset(pad_buf, 0, s->cluster_size); - qemu_iovec_to_buf(qiov, 0, pad_buf, s->cluster_size); - iov = (struct iovec) { - .iov_base = pad_buf, - .iov_len = s->cluster_size, - }; - qemu_iovec_init_external(&hd_qiov, &iov, 1); - ret = qcow_co_pwritev_compressed(bs, offset, bytes, &hd_qiov); - qemu_vfree(pad_buf); - } - return ret; - } buf = qemu_blockalign(bs, s->cluster_size); + if (bytes != s->cluster_size) { + if (bytes > s->cluster_size || + offset + bytes != bs->total_sectors << BDRV_SECTOR_BITS) + { + qemu_vfree(buf); + return -EINVAL; + } + /* Zero-pad last write if image size is not cluster aligned */ + memset(buf + bytes, 0, s->cluster_size - bytes); + } qemu_iovec_to_buf(qiov, 0, buf, qiov->size); out_buf = g_malloc(s->cluster_size); From 35fadca80e6df2e7a2e57ea162db11f0219c2b2d Mon Sep 17 00:00:00 2001 From: Pavel Butsykin Date: Fri, 22 Jul 2016 11:17:48 +0300 Subject: [PATCH 22/36] block: remove BlockDriver.bdrv_write_compressed There are no block drivers left that implement the old .bdrv_write_compressed interface, so it can be removed. Also now we have no need to use the bdrv_pwrite_compressed function and we can remove it entirely. Signed-off-by: Pavel Butsykin Reviewed-by: Stefan Hajnoczi Signed-off-by: Denis V. Lunev CC: Jeff Cody CC: Markus Armbruster CC: Eric Blake CC: John Snow CC: Stefan Hajnoczi CC: Kevin Wolf Signed-off-by: Kevin Wolf --- block/block-backend.c | 8 ++------ block/io.c | 31 ------------------------------- include/block/block.h | 2 -- include/block/block_int.h | 3 --- qemu-img.c | 2 +- 5 files changed, 3 insertions(+), 43 deletions(-) diff --git a/block/block-backend.c b/block/block-backend.c index 76ea45955f..d1349d90e5 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -1488,12 +1488,8 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset, int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf, int count) { - int ret = blk_check_byte_request(blk, offset, count); - if (ret < 0) { - return ret; - } - - return bdrv_pwrite_compressed(blk->root, offset, buf, count); + return blk_prw(blk, offset, (void *) buf, count, blk_write_entry, + BDRV_REQ_WRITE_COMPRESSED); } int blk_truncate(BlockBackend *blk, int64_t offset) diff --git a/block/io.c b/block/io.c index d402076e95..0339911dbb 100644 --- a/block/io.c +++ b/block/io.c @@ -1887,37 +1887,6 @@ int bdrv_is_allocated_above(BlockDriverState *top, return 0; } -int bdrv_pwrite_compressed(BdrvChild *child, int64_t offset, - const void *buf, int bytes) -{ - BlockDriverState *bs = child->bs; - BlockDriver *drv = bs->drv; - QEMUIOVector qiov; - struct iovec iov; - - if (!drv) { - return -ENOMEDIUM; - } - if (drv->bdrv_write_compressed) { - int ret = bdrv_check_byte_request(bs, offset, bytes); - if (ret < 0) { - return ret; - } - assert(QLIST_EMPTY(&bs->dirty_bitmaps)); - assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0); - assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0); - return drv->bdrv_write_compressed(bs, offset >> BDRV_SECTOR_BITS, buf, - bytes >> BDRV_SECTOR_BITS); - } - iov = (struct iovec) { - .iov_base = (void *)buf, - .iov_len = bytes, - }; - qemu_iovec_init_external(&qiov, &iov, 1); - - return bdrv_prwv_co(child, offset, &qiov, true, BDRV_REQ_WRITE_COMPRESSED); -} - typedef struct BdrvVmstateCo { BlockDriverState *bs; QEMUIOVector *qiov; diff --git a/include/block/block.h b/include/block/block.h index d8dacd2ced..7edce5c35f 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -400,8 +400,6 @@ const char *bdrv_get_node_name(const BlockDriverState *bs); const char *bdrv_get_device_name(const BlockDriverState *bs); const char *bdrv_get_device_or_node_name(const BlockDriverState *bs); int bdrv_get_flags(BlockDriverState *bs); -int bdrv_pwrite_compressed(BdrvChild *child, int64_t offset, - const void *buf, int bytes); int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi); ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs); void bdrv_round_sectors_to_clusters(BlockDriverState *bs, diff --git a/include/block/block_int.h b/include/block/block_int.h index 42f8f8443d..9c61f49f94 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -204,9 +204,6 @@ struct BlockDriver { bool has_variable_length; int64_t (*bdrv_get_allocated_file_size)(BlockDriverState *bs); - int (*bdrv_write_compressed)(BlockDriverState *bs, int64_t sector_num, - const uint8_t *buf, int nb_sectors); - int coroutine_fn (*bdrv_co_pwritev_compressed)(BlockDriverState *bs, uint64_t offset, uint64_t bytes, QEMUIOVector *qiov); diff --git a/qemu-img.c b/qemu-img.c index c2ea494928..1090286a9f 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -2034,7 +2034,7 @@ static int img_convert(int argc, char **argv) const char *preallocation = qemu_opt_get(opts, BLOCK_OPT_PREALLOC); - if (!drv->bdrv_write_compressed && !drv->bdrv_co_pwritev_compressed) { + if (!drv->bdrv_co_pwritev_compressed) { error_report("Compression not supported for this file format"); ret = -1; goto out; From 3ea1a091119793a5e554859cc4dcd1ef060810bd Mon Sep 17 00:00:00 2001 From: Pavel Butsykin Date: Fri, 22 Jul 2016 11:17:49 +0300 Subject: [PATCH 23/36] block/io: turn on dirty_bitmaps for the compressed writes Previously was added the assert: commit 1755da16e32c15b22a521e8a38539e4b5cf367f3 Author: Paolo Bonzini Date: Thu Oct 18 16:49:18 2012 +0200 block: introduce new dirty bitmap functionality Now the compressed write is always in coroutine and setting the bits is done after the write, so that we can return the dirty_bitmaps for the compressed writes. Signed-off-by: Pavel Butsykin Reviewed-by: Stefan Hajnoczi Signed-off-by: Denis V. Lunev CC: Jeff Cody CC: Markus Armbruster CC: Eric Blake CC: John Snow CC: Stefan Hajnoczi CC: Kevin Wolf Signed-off-by: Kevin Wolf --- block/io.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/block/io.c b/block/io.c index 0339911dbb..fdf70807b0 100644 --- a/block/io.c +++ b/block/io.c @@ -896,7 +896,6 @@ bdrv_driver_pwritev_compressed(BlockDriverState *bs, uint64_t offset, return -ENOTSUP; } - assert(QLIST_EMPTY(&bs->dirty_bitmaps)); return drv->bdrv_co_pwritev_compressed(bs, offset, bytes, qiov); } @@ -1318,6 +1317,8 @@ static int coroutine_fn bdrv_aligned_pwritev(BlockDriverState *bs, } else if (flags & BDRV_REQ_ZERO_WRITE) { bdrv_debug_event(bs, BLKDBG_PWRITEV_ZERO); ret = bdrv_co_do_pwrite_zeroes(bs, offset, bytes, flags); + } else if (flags & BDRV_REQ_WRITE_COMPRESSED) { + ret = bdrv_driver_pwritev_compressed(bs, offset, bytes, qiov); } else if (bytes <= max_transfer) { bdrv_debug_event(bs, BLKDBG_PWRITEV); ret = bdrv_driver_pwritev(bs, offset, bytes, qiov, flags); @@ -1569,14 +1570,9 @@ int coroutine_fn bdrv_co_pwritev(BdrvChild *child, bytes = ROUND_UP(bytes, align); } - if (flags & BDRV_REQ_WRITE_COMPRESSED) { - ret = bdrv_driver_pwritev_compressed( - bs, offset, bytes, use_local_qiov ? &local_qiov : qiov); - } else { - ret = bdrv_aligned_pwritev(bs, &req, offset, bytes, align, - use_local_qiov ? &local_qiov : qiov, - flags); - } + ret = bdrv_aligned_pwritev(bs, &req, offset, bytes, align, + use_local_qiov ? &local_qiov : qiov, + flags); fail: From 81206a8987f896adb61d070133c81c47a74b1a38 Mon Sep 17 00:00:00 2001 From: Pavel Butsykin Date: Fri, 22 Jul 2016 11:17:50 +0300 Subject: [PATCH 24/36] block: simplify drive-backup Now that we can support boxed commands, use it to greatly reduce the number of parameters (and likelihood of getting out of sync) when adjusting drive-backup parameters. Signed-off-by: Pavel Butsykin Reviewed-by: Stefan Hajnoczi Reviewed-by: Eric Blake Signed-off-by: Denis V. Lunev CC: Jeff Cody CC: Markus Armbruster CC: Eric Blake CC: John Snow CC: Stefan Hajnoczi CC: Kevin Wolf Signed-off-by: Kevin Wolf --- blockdev.c | 111 ++++++++++++++----------------------------- hmp.c | 21 ++++---- qapi/block-core.json | 3 +- 3 files changed, 48 insertions(+), 87 deletions(-) diff --git a/blockdev.c b/blockdev.c index a392e08ed2..539aa7d1e7 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1841,17 +1841,8 @@ typedef struct DriveBackupState { BlockJob *job; } DriveBackupState; -static void do_drive_backup(const char *job_id, const char *device, - const char *target, bool has_format, - const char *format, enum MirrorSyncMode sync, - bool has_mode, enum NewImageMode mode, - bool has_speed, int64_t speed, - bool has_bitmap, const char *bitmap, - bool has_on_source_error, - BlockdevOnError on_source_error, - bool has_on_target_error, - BlockdevOnError on_target_error, - BlockJobTxn *txn, Error **errp); +static void do_drive_backup(DriveBackup *backup, BlockJobTxn *txn, + Error **errp); static void drive_backup_prepare(BlkActionState *common, Error **errp) { @@ -1874,16 +1865,7 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp) bdrv_drained_begin(bs); state->bs = bs; - do_drive_backup(backup->has_job_id ? backup->job_id : NULL, - backup->device, backup->target, - backup->has_format, backup->format, - backup->sync, - backup->has_mode, backup->mode, - backup->has_speed, backup->speed, - backup->has_bitmap, backup->bitmap, - backup->has_on_source_error, backup->on_source_error, - backup->has_on_target_error, backup->on_target_error, - common->block_job_txn, &local_err); + do_drive_backup(backup, common->block_job_txn, &local_err); if (local_err) { error_propagate(errp, local_err); return; @@ -3134,17 +3116,7 @@ out: aio_context_release(aio_context); } -static void do_drive_backup(const char *job_id, const char *device, - const char *target, bool has_format, - const char *format, enum MirrorSyncMode sync, - bool has_mode, enum NewImageMode mode, - bool has_speed, int64_t speed, - bool has_bitmap, const char *bitmap, - bool has_on_source_error, - BlockdevOnError on_source_error, - bool has_on_target_error, - BlockdevOnError on_target_error, - BlockJobTxn *txn, Error **errp) +static void do_drive_backup(DriveBackup *backup, BlockJobTxn *txn, Error **errp) { BlockDriverState *bs; BlockDriverState *target_bs; @@ -3156,20 +3128,23 @@ static void do_drive_backup(const char *job_id, const char *device, int flags; int64_t size; - if (!has_speed) { - speed = 0; + if (!backup->has_speed) { + backup->speed = 0; } - if (!has_on_source_error) { - on_source_error = BLOCKDEV_ON_ERROR_REPORT; + if (!backup->has_on_source_error) { + backup->on_source_error = BLOCKDEV_ON_ERROR_REPORT; } - if (!has_on_target_error) { - on_target_error = BLOCKDEV_ON_ERROR_REPORT; + if (!backup->has_on_target_error) { + backup->on_target_error = BLOCKDEV_ON_ERROR_REPORT; } - if (!has_mode) { - mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS; + if (!backup->has_mode) { + backup->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS; + } + if (!backup->has_job_id) { + backup->job_id = NULL; } - bs = qmp_get_root_bs(device, errp); + bs = qmp_get_root_bs(backup->device, errp); if (!bs) { return; } @@ -3177,8 +3152,9 @@ static void do_drive_backup(const char *job_id, const char *device, aio_context = bdrv_get_aio_context(bs); aio_context_acquire(aio_context); - if (!has_format) { - format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : bs->drv->format_name; + if (!backup->has_format) { + backup->format = backup->mode == NEW_IMAGE_MODE_EXISTING ? + NULL : (char*) bs->drv->format_name; } /* Early check to avoid creating target */ @@ -3190,13 +3166,13 @@ static void do_drive_backup(const char *job_id, const char *device, /* See if we have a backing HD we can use to create our new image * on top of. */ - if (sync == MIRROR_SYNC_MODE_TOP) { + if (backup->sync == MIRROR_SYNC_MODE_TOP) { source = backing_bs(bs); if (!source) { - sync = MIRROR_SYNC_MODE_FULL; + backup->sync = MIRROR_SYNC_MODE_FULL; } } - if (sync == MIRROR_SYNC_MODE_NONE) { + if (backup->sync == MIRROR_SYNC_MODE_NONE) { source = bs; } @@ -3206,14 +3182,14 @@ static void do_drive_backup(const char *job_id, const char *device, goto out; } - if (mode != NEW_IMAGE_MODE_EXISTING) { - assert(format); + if (backup->mode != NEW_IMAGE_MODE_EXISTING) { + assert(backup->format); if (source) { - bdrv_img_create(target, format, source->filename, + bdrv_img_create(backup->target, backup->format, source->filename, source->drv->format_name, NULL, size, flags, &local_err, false); } else { - bdrv_img_create(target, format, NULL, NULL, NULL, + bdrv_img_create(backup->target, backup->format, NULL, NULL, NULL, size, flags, &local_err, false); } } @@ -3223,29 +3199,29 @@ static void do_drive_backup(const char *job_id, const char *device, goto out; } - if (format) { + if (backup->format) { options = qdict_new(); - qdict_put(options, "driver", qstring_from_str(format)); + qdict_put(options, "driver", qstring_from_str(backup->format)); } - target_bs = bdrv_open(target, NULL, options, flags, errp); + target_bs = bdrv_open(backup->target, NULL, options, flags, errp); if (!target_bs) { goto out; } bdrv_set_aio_context(target_bs, aio_context); - if (has_bitmap) { - bmap = bdrv_find_dirty_bitmap(bs, bitmap); + if (backup->has_bitmap) { + bmap = bdrv_find_dirty_bitmap(bs, backup->bitmap); if (!bmap) { - error_setg(errp, "Bitmap '%s' could not be found", bitmap); + error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap); bdrv_unref(target_bs); goto out; } } - backup_start(job_id, bs, target_bs, speed, sync, bmap, - on_source_error, on_target_error, + backup_start(backup->job_id, bs, target_bs, backup->speed, backup->sync, + bmap, backup->on_source_error, backup->on_target_error, block_job_cb, bs, txn, &local_err); bdrv_unref(target_bs); if (local_err != NULL) { @@ -3257,24 +3233,9 @@ out: aio_context_release(aio_context); } -void qmp_drive_backup(bool has_job_id, const char *job_id, - const char *device, const char *target, - bool has_format, const char *format, - enum MirrorSyncMode sync, - bool has_mode, enum NewImageMode mode, - bool has_speed, int64_t speed, - bool has_bitmap, const char *bitmap, - bool has_on_source_error, BlockdevOnError on_source_error, - bool has_on_target_error, BlockdevOnError on_target_error, - Error **errp) +void qmp_drive_backup(DriveBackup *arg, Error **errp) { - return do_drive_backup(has_job_id ? job_id : NULL, device, target, - has_format, format, sync, - has_mode, mode, has_speed, speed, - has_bitmap, bitmap, - has_on_source_error, on_source_error, - has_on_target_error, on_target_error, - NULL, errp); + return do_drive_backup(arg, NULL, errp); } BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp) diff --git a/hmp.c b/hmp.c index cc2056e9e2..3c06200e48 100644 --- a/hmp.c +++ b/hmp.c @@ -1109,8 +1109,16 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict) const char *format = qdict_get_try_str(qdict, "format"); bool reuse = qdict_get_try_bool(qdict, "reuse", false); bool full = qdict_get_try_bool(qdict, "full", false); - enum NewImageMode mode; Error *err = NULL; + DriveBackup backup = { + .device = (char *)device, + .target = (char *)filename, + .has_format = !!format, + .format = (char *)format, + .sync = full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP, + .has_mode = true, + .mode = reuse ? NEW_IMAGE_MODE_EXISTING : NEW_IMAGE_MODE_ABSOLUTE_PATHS, + }; if (!filename) { error_setg(&err, QERR_MISSING_PARAMETER, "target"); @@ -1118,16 +1126,7 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict) return; } - if (reuse) { - mode = NEW_IMAGE_MODE_EXISTING; - } else { - mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS; - } - - qmp_drive_backup(false, NULL, device, filename, !!format, format, - full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP, - true, mode, false, 0, false, NULL, - false, 0, false, 0, &err); + qmp_drive_backup(&backup, &err); hmp_handle_error(mon, &err); } diff --git a/qapi/block-core.json b/qapi/block-core.json index fba1718cef..2bc51dfe6c 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1091,7 +1091,8 @@ # # Since 1.6 ## -{ 'command': 'drive-backup', 'data': 'DriveBackup' } +{ 'command': 'drive-backup', 'boxed': true, + 'data': 'DriveBackup' } ## # @blockdev-backup From dc7a4a9ed170c19b4036cef2720d3e8fb551cc0f Mon Sep 17 00:00:00 2001 From: Pavel Butsykin Date: Fri, 22 Jul 2016 11:17:51 +0300 Subject: [PATCH 25/36] block: simplify blockdev-backup Now that we can support boxed commands, use it to greatly reduce the number of parameters (and likelihood of getting out of sync) when adjusting blockdev-backup parameters. Signed-off-by: Pavel Butsykin Reviewed-by: Stefan Hajnoczi Signed-off-by: Denis V. Lunev CC: Jeff Cody CC: Markus Armbruster CC: Eric Blake CC: John Snow CC: Stefan Hajnoczi CC: Kevin Wolf Signed-off-by: Kevin Wolf --- blockdev.c | 66 ++++++++++++++------------------------------ qapi/block-core.json | 6 +++- 2 files changed, 25 insertions(+), 47 deletions(-) diff --git a/blockdev.c b/blockdev.c index 539aa7d1e7..5f1b01518d 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1902,14 +1902,8 @@ typedef struct BlockdevBackupState { AioContext *aio_context; } BlockdevBackupState; -static void do_blockdev_backup(const char *job_id, 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, - BlockJobTxn *txn, Error **errp); +static void do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, + Error **errp); static void blockdev_backup_prepare(BlkActionState *common, Error **errp) { @@ -1942,12 +1936,7 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp) state->bs = bs; bdrv_drained_begin(state->bs); - do_blockdev_backup(backup->has_job_id ? backup->job_id : NULL, - backup->device, backup->target, backup->sync, - backup->has_speed, backup->speed, - backup->has_on_source_error, backup->on_source_error, - backup->has_on_target_error, backup->on_target_error, - common->block_job_txn, &local_err); + do_blockdev_backup(backup, common->block_job_txn, &local_err); if (local_err) { error_propagate(errp, local_err); return; @@ -3243,31 +3232,27 @@ BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp) return bdrv_named_nodes_list(errp); } -void do_blockdev_backup(const char *job_id, 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, - BlockJobTxn *txn, Error **errp) +void do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, Error **errp) { BlockDriverState *bs; BlockDriverState *target_bs; Error *local_err = NULL; AioContext *aio_context; - if (!has_speed) { - speed = 0; + if (!backup->has_speed) { + backup->speed = 0; } - if (!has_on_source_error) { - on_source_error = BLOCKDEV_ON_ERROR_REPORT; + if (!backup->has_on_source_error) { + backup->on_source_error = BLOCKDEV_ON_ERROR_REPORT; } - if (!has_on_target_error) { - on_target_error = BLOCKDEV_ON_ERROR_REPORT; + if (!backup->has_on_target_error) { + backup->on_target_error = BLOCKDEV_ON_ERROR_REPORT; + } + if (!backup->has_job_id) { + backup->job_id = NULL; } - bs = qmp_get_root_bs(device, errp); + bs = qmp_get_root_bs(backup->device, errp); if (!bs) { return; } @@ -3275,7 +3260,7 @@ void do_blockdev_backup(const char *job_id, const char *device, aio_context = bdrv_get_aio_context(bs); aio_context_acquire(aio_context); - target_bs = bdrv_lookup_bs(target, target, errp); + target_bs = bdrv_lookup_bs(backup->target, backup->target, errp); if (!target_bs) { goto out; } @@ -3291,8 +3276,9 @@ void do_blockdev_backup(const char *job_id, const char *device, goto out; } } - backup_start(job_id, bs, target_bs, speed, sync, NULL, on_source_error, - on_target_error, block_job_cb, bs, txn, &local_err); + backup_start(backup->job_id, bs, target_bs, backup->speed, backup->sync, + NULL, backup->on_source_error, backup->on_target_error, + block_job_cb, bs, txn, &local_err); if (local_err != NULL) { error_propagate(errp, local_err); } @@ -3300,21 +3286,9 @@ out: aio_context_release(aio_context); } -void qmp_blockdev_backup(bool has_job_id, const char *job_id, - 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) +void qmp_blockdev_backup(BlockdevBackup *arg, Error **errp) { - do_blockdev_backup(has_job_id ? job_id : NULL, device, target, - sync, has_speed, speed, - has_on_source_error, on_source_error, - has_on_target_error, on_target_error, - NULL, errp); + do_blockdev_backup(arg, NULL, errp); } /* Parameter check and block job starting for drive mirroring. diff --git a/qapi/block-core.json b/qapi/block-core.json index 2bc51dfe6c..4c8cf057b5 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1105,9 +1105,13 @@ # # For the arguments, see the documentation of BlockdevBackup. # +# Returns: nothing on success +# If @device is not a valid block device, DeviceNotFound +# # Since 2.3 ## -{ 'command': 'blockdev-backup', 'data': 'BlockdevBackup' } +{ 'command': 'blockdev-backup', 'boxed': true, + 'data': 'BlockdevBackup' } ## From 13b9414b5798539e2dbb87a570d96184fe21edf4 Mon Sep 17 00:00:00 2001 From: Pavel Butsykin Date: Fri, 22 Jul 2016 11:17:52 +0300 Subject: [PATCH 26/36] drive-backup: added support for data compression The idea is simple - backup is "written-once" data. It is written block by block and it is large enough. It would be nice to save storage space and compress it. The patch adds a flag to the qmp/hmp drive-backup command which enables block compression. Compression should be implemented in the format driver to enable this feature. There are some limitations of the format driver to allow compressed writes. We can write data only once. Though for backup this is perfectly fine. These limitations are maintained by the driver and the error will be reported if we are doing something wrong. Signed-off-by: Pavel Butsykin Reviewed-by: Stefan Hajnoczi Signed-off-by: Denis V. Lunev CC: Jeff Cody CC: Markus Armbruster CC: Eric Blake CC: John Snow CC: Stefan Hajnoczi CC: Kevin Wolf Signed-off-by: Kevin Wolf --- block/backup.c | 12 +++++++++++- blockdev.c | 9 ++++++--- hmp-commands.hx | 8 +++++--- hmp.c | 3 +++ include/block/block_int.h | 1 + qapi/block-core.json | 5 ++++- qmp-commands.hx | 5 ++++- 7 files changed, 34 insertions(+), 9 deletions(-) diff --git a/block/backup.c b/block/backup.c index 2c0532314f..bb3bb9a9eb 100644 --- a/block/backup.c +++ b/block/backup.c @@ -47,6 +47,7 @@ typedef struct BackupBlockJob { uint64_t sectors_read; unsigned long *done_bitmap; int64_t cluster_size; + bool compress; NotifierWithReturn before_write; QLIST_HEAD(, CowRequest) inflight_reqs; } BackupBlockJob; @@ -154,7 +155,8 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job, bounce_qiov.size, BDRV_REQ_MAY_UNMAP); } else { ret = blk_co_pwritev(job->target, start * job->cluster_size, - bounce_qiov.size, &bounce_qiov, 0); + bounce_qiov.size, &bounce_qiov, + job->compress ? BDRV_REQ_WRITE_COMPRESSED : 0); } if (ret < 0) { trace_backup_do_cow_write_fail(job, start, ret); @@ -477,6 +479,7 @@ static void coroutine_fn backup_run(void *opaque) void backup_start(const char *job_id, BlockDriverState *bs, BlockDriverState *target, int64_t speed, MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap, + bool compress, BlockdevOnError on_source_error, BlockdevOnError on_target_error, BlockCompletionFunc *cb, void *opaque, @@ -507,6 +510,12 @@ void backup_start(const char *job_id, BlockDriverState *bs, return; } + if (compress && target->drv->bdrv_co_pwritev_compressed == NULL) { + error_setg(errp, "Compression is not supported for this drive %s", + bdrv_get_device_name(target)); + return; + } + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) { return; } @@ -555,6 +564,7 @@ void backup_start(const char *job_id, BlockDriverState *bs, job->sync_mode = sync_mode; job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_INCREMENTAL ? sync_bitmap : NULL; + job->compress = compress; /* If there is no backing file on the target, we cannot rely on COW if our * backup cluster size is smaller than the target cluster size. Even for diff --git a/blockdev.c b/blockdev.c index 5f1b01518d..f5857d0187 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3132,6 +3132,9 @@ static void do_drive_backup(DriveBackup *backup, BlockJobTxn *txn, Error **errp) if (!backup->has_job_id) { backup->job_id = NULL; } + if (!backup->has_compress) { + backup->compress = false; + } bs = qmp_get_root_bs(backup->device, errp); if (!bs) { @@ -3210,8 +3213,8 @@ static void do_drive_backup(DriveBackup *backup, BlockJobTxn *txn, Error **errp) } backup_start(backup->job_id, bs, target_bs, backup->speed, backup->sync, - bmap, backup->on_source_error, backup->on_target_error, - block_job_cb, bs, txn, &local_err); + bmap, backup->compress, backup->on_source_error, + backup->on_target_error, block_job_cb, bs, txn, &local_err); bdrv_unref(target_bs); if (local_err != NULL) { error_propagate(errp, local_err); @@ -3277,7 +3280,7 @@ void do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, Error **errp) } } backup_start(backup->job_id, bs, target_bs, backup->speed, backup->sync, - NULL, backup->on_source_error, backup->on_target_error, + NULL, false, backup->on_source_error, backup->on_target_error, block_job_cb, bs, txn, &local_err); if (local_err != NULL) { error_propagate(errp, local_err); diff --git a/hmp-commands.hx b/hmp-commands.hx index 848efee5d1..74f32e515c 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -1182,8 +1182,8 @@ ETEXI { .name = "drive_backup", - .args_type = "reuse:-n,full:-f,device:B,target:s,format:s?", - .params = "[-n] [-f] device target [format]", + .args_type = "reuse:-n,full:-f,compress:-c,device:B,target:s,format:s?", + .params = "[-n] [-f] [-c] device target [format]", .help = "initiates a point-in-time\n\t\t\t" "copy for a device. The device's contents are\n\t\t\t" "copied to the new image file, excluding data that\n\t\t\t" @@ -1191,7 +1191,9 @@ ETEXI "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", + "so that the result does not need a backing file.\n\t\t\t" + "The -c flag requests QEMU to compress backup data\n\t\t\t" + "(if the target format supports it).\n\t\t\t", .mhandler.cmd = hmp_drive_backup, }, STEXI diff --git a/hmp.c b/hmp.c index 3c06200e48..a7dfe6fa11 100644 --- a/hmp.c +++ b/hmp.c @@ -1109,6 +1109,7 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict) const char *format = qdict_get_try_str(qdict, "format"); bool reuse = qdict_get_try_bool(qdict, "reuse", false); bool full = qdict_get_try_bool(qdict, "full", false); + bool compress = qdict_get_try_bool(qdict, "compress", false); Error *err = NULL; DriveBackup backup = { .device = (char *)device, @@ -1118,6 +1119,8 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict) .sync = full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP, .has_mode = true, .mode = reuse ? NEW_IMAGE_MODE_EXISTING : NEW_IMAGE_MODE_ABSOLUTE_PATHS, + .has_compress = !!compress, + .compress = compress, }; if (!filename) { diff --git a/include/block/block_int.h b/include/block/block_int.h index 9c61f49f94..0ca6a78eb3 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -765,6 +765,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs, void backup_start(const char *job_id, BlockDriverState *bs, BlockDriverState *target, int64_t speed, MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap, + bool compress, BlockdevOnError on_source_error, BlockdevOnError on_target_error, BlockCompletionFunc *cb, void *opaque, diff --git a/qapi/block-core.json b/qapi/block-core.json index 4c8cf057b5..300a68b63a 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -898,6 +898,9 @@ # Must be present if sync is "incremental", must NOT be present # otherwise. (Since 2.4) # +# @compress: #optional true to compress data, if the target format supports it. +# (default: false) (since 2.7) +# # @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). @@ -915,7 +918,7 @@ { 'struct': 'DriveBackup', 'data': { '*job-id': 'str', 'device': 'str', 'target': 'str', '*format': 'str', 'sync': 'MirrorSyncMode', '*mode': 'NewImageMode', - '*speed': 'int', '*bitmap': 'str', + '*speed': 'int', '*bitmap': 'str', '*compress': 'bool', '*on-source-error': 'BlockdevOnError', '*on-target-error': 'BlockdevOnError' } } diff --git a/qmp-commands.hx b/qmp-commands.hx index 956f1b0f47..d1593c91ff 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1217,7 +1217,8 @@ EQMP { .name = "drive-backup", .args_type = "job-id:s?,sync:s,device:B,target:s,speed:i?,mode:s?," - "format:s?,bitmap:s?,on-source-error:s?,on-target-error:s?", + "format:s?,bitmap:s?,compress:b?," + "on-source-error:s?,on-target-error:s?", .mhandler.cmd_new = qmp_marshal_drive_backup, }, @@ -1253,6 +1254,8 @@ Arguments: - "mode": whether and how QEMU should create a new image (NewImageMode, optional, default 'absolute-paths') - "speed": the maximum speed, in bytes per second (json-int, optional) +- "compress": true to compress data, if the target format supports it. + (json-bool, optional, default false) - "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. From 3b7b12365953c507d50b52fbd3f1b46574c6946f Mon Sep 17 00:00:00 2001 From: Pavel Butsykin Date: Fri, 22 Jul 2016 11:17:53 +0300 Subject: [PATCH 27/36] blockdev-backup: added support for data compression The idea is simple - backup is "written-once" data. It is written block by block and it is large enough. It would be nice to save storage space and compress it. Signed-off-by: Pavel Butsykin Reviewed-by: Stefan Hajnoczi Signed-off-by: Denis V. Lunev CC: Jeff Cody CC: Markus Armbruster CC: Eric Blake CC: John Snow CC: Stefan Hajnoczi CC: Kevin Wolf Signed-off-by: Kevin Wolf --- blockdev.c | 7 +++++-- qapi/block-core.json | 4 ++++ qmp-commands.hx | 4 +++- 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/blockdev.c b/blockdev.c index f5857d0187..97062e33c3 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3254,6 +3254,9 @@ void do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, Error **errp) if (!backup->has_job_id) { backup->job_id = NULL; } + if (!backup->has_compress) { + backup->compress = false; + } bs = qmp_get_root_bs(backup->device, errp); if (!bs) { @@ -3280,8 +3283,8 @@ void do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, Error **errp) } } backup_start(backup->job_id, bs, target_bs, backup->speed, backup->sync, - NULL, false, backup->on_source_error, backup->on_target_error, - block_job_cb, bs, txn, &local_err); + NULL, backup->compress, backup->on_source_error, + backup->on_target_error, block_job_cb, bs, txn, &local_err); if (local_err != NULL) { error_propagate(errp, local_err); } diff --git a/qapi/block-core.json b/qapi/block-core.json index 300a68b63a..31f9990754 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -939,6 +939,9 @@ # @speed: #optional the maximum speed, in bytes per second. The default is 0, # for unlimited. # +# @compress: #optional true to compress data, if the target format supports it. +# (default: false) (since 2.7) +# # @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). @@ -957,6 +960,7 @@ 'data': { '*job-id': 'str', 'device': 'str', 'target': 'str', 'sync': 'MirrorSyncMode', '*speed': 'int', + '*compress': 'bool', '*on-source-error': 'BlockdevOnError', '*on-target-error': 'BlockdevOnError' } } diff --git a/qmp-commands.hx b/qmp-commands.hx index d1593c91ff..ba2a916f69 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1275,7 +1275,7 @@ EQMP { .name = "blockdev-backup", - .args_type = "job-id:s?,sync:s,device:B,target:B,speed:i?," + .args_type = "job-id:s?,sync:s,device:B,target:B,speed:i?,compress:b?," "on-source-error:s?,on-target-error:s?", .mhandler.cmd_new = qmp_marshal_blockdev_backup, }, @@ -1299,6 +1299,8 @@ Arguments: 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) +- "compress": true to compress data, if the target format supports it. + (json-bool, optional, default false) - "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. From e1b5c51f4c0c5ecbf158148e2333ee87080bc66f Mon Sep 17 00:00:00 2001 From: Pavel Butsykin Date: Fri, 22 Jul 2016 11:17:54 +0300 Subject: [PATCH 28/36] qemu-iotests: test backup compression in 055 Added cases to check the backup compression out of qcow2, raw in qcow2 on drive-backup and blockdev-backup. Signed-off-by: Pavel Butsykin Reviewed-by: Stefan Hajnoczi Signed-off-by: Denis V. Lunev CC: Jeff Cody CC: Markus Armbruster CC: Eric Blake CC: John Snow CC: Stefan Hajnoczi CC: Kevin Wolf Signed-off-by: Kevin Wolf --- tests/qemu-iotests/055 | 97 +++++++++++++++++++++++++++++++++++ tests/qemu-iotests/055.out | 4 +- tests/qemu-iotests/iotests.py | 10 ++-- 3 files changed, 104 insertions(+), 7 deletions(-) diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055 index 8113c61ade..0e1326cba3 100755 --- a/tests/qemu-iotests/055 +++ b/tests/qemu-iotests/055 @@ -448,5 +448,102 @@ class TestSingleTransaction(iotests.QMPTestCase): self.assert_qmp(result, 'error/class', 'GenericError') self.assert_no_active_block_jobs() + +class TestDriveCompression(iotests.QMPTestCase): + image_len = 64 * 1024 * 1024 # MB + outfmt = 'qcow2' + + def setUp(self): + # Write data to the image so we can compare later + qemu_img('create', '-f', iotests.imgfmt, test_img, str(TestDriveCompression.image_len)) + qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x11 0 64k', test_img) + qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x00 64k 128k', test_img) + qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x22 162k 32k', test_img) + qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x33 67043328 64k', test_img) + + qemu_img('create', '-f', TestDriveCompression.outfmt, blockdev_target_img, + str(TestDriveCompression.image_len)) + self.vm = iotests.VM().add_drive(test_img).add_drive(blockdev_target_img, + format=TestDriveCompression.outfmt) + self.vm.launch() + + def tearDown(self): + self.vm.shutdown() + os.remove(test_img) + os.remove(blockdev_target_img) + try: + os.remove(target_img) + except OSError: + pass + + def do_test_compress_complete(self, cmd, **args): + self.assert_no_active_block_jobs() + + result = self.vm.qmp(cmd, device='drive0', sync='full', compress=True, **args) + self.assert_qmp(result, 'return', {}) + + self.wait_until_completed() + + self.vm.shutdown() + self.assertTrue(iotests.compare_images(test_img, blockdev_target_img, + iotests.imgfmt, TestDriveCompression.outfmt), + 'target image does not match source after backup') + + def test_complete_compress_drive_backup(self): + self.do_test_compress_complete('drive-backup', target=blockdev_target_img, mode='existing') + + def test_complete_compress_blockdev_backup(self): + self.do_test_compress_complete('blockdev-backup', target='drive1') + + def do_test_compress_cancel(self, cmd, **args): + self.assert_no_active_block_jobs() + + result = self.vm.qmp(cmd, device='drive0', sync='full', compress=True, **args) + self.assert_qmp(result, 'return', {}) + + event = self.cancel_and_wait() + self.assert_qmp(event, 'data/type', 'backup') + + def test_compress_cancel_drive_backup(self): + self.do_test_compress_cancel('drive-backup', target=blockdev_target_img, mode='existing') + + def test_compress_cancel_blockdev_backup(self): + self.do_test_compress_cancel('blockdev-backup', target='drive1') + + def do_test_compress_pause(self, cmd, **args): + self.assert_no_active_block_jobs() + + self.vm.pause_drive('drive0') + result = self.vm.qmp(cmd, device='drive0', sync='full', compress=True, **args) + self.assert_qmp(result, 'return', {}) + + result = self.vm.qmp('block-job-pause', device='drive0') + self.assert_qmp(result, 'return', {}) + + self.vm.resume_drive('drive0') + time.sleep(1) + result = self.vm.qmp('query-block-jobs') + offset = self.dictpath(result, 'return[0]/offset') + + time.sleep(1) + result = self.vm.qmp('query-block-jobs') + self.assert_qmp(result, 'return[0]/offset', offset) + + result = self.vm.qmp('block-job-resume', device='drive0') + self.assert_qmp(result, 'return', {}) + + self.wait_until_completed() + + self.vm.shutdown() + self.assertTrue(iotests.compare_images(test_img, blockdev_target_img, + iotests.imgfmt, TestDriveCompression.outfmt), + 'target image does not match source after backup') + + def test_compress_pause_drive_backup(self): + self.do_test_compress_pause('drive-backup', target=blockdev_target_img, mode='existing') + + def test_compress_pause_blockdev_backup(self): + self.do_test_compress_pause('blockdev-backup', target='drive1') + if __name__ == '__main__': iotests.main(supported_fmts=['raw', 'qcow2']) diff --git a/tests/qemu-iotests/055.out b/tests/qemu-iotests/055.out index 42314e9c00..5ce2f9a2ed 100644 --- a/tests/qemu-iotests/055.out +++ b/tests/qemu-iotests/055.out @@ -1,5 +1,5 @@ -........................ +.............................. ---------------------------------------------------------------------- -Ran 24 tests +Ran 30 tests OK diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index dbe0ee548a..69aa41e514 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -86,10 +86,10 @@ def qemu_io(*args): sys.stderr.write('qemu-io received signal %i: %s\n' % (-exitcode, ' '.join(args))) return subp.communicate()[0] -def compare_images(img1, img2): +def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt): '''Return True if two image files are identical''' - return qemu_img('compare', '-f', imgfmt, - '-F', imgfmt, img1, img2) == 0 + return qemu_img('compare', '-f', fmt1, + '-F', fmt2, img1, img2) == 0 def create_image(name, size): '''Create a fully-allocated raw image with sector markers''' @@ -141,14 +141,14 @@ class VM(qtest.QEMUQtestMachine): self._args.append(opts) return self - def add_drive(self, path, opts='', interface='virtio'): + def add_drive(self, path, opts='', interface='virtio', format=imgfmt): '''Add a virtio-blk drive to the VM''' options = ['if=%s' % interface, 'id=drive%d' % self._num_drives] if path is not None: options.append('file=%s' % path) - options.append('format=%s' % imgfmt) + options.append('format=%s' % format) options.append('cache=%s' % cachemode) if opts: From 00198ecc776d2d6b9f65703f209f4e2c8b2ab844 Mon Sep 17 00:00:00 2001 From: Pavel Butsykin Date: Fri, 22 Jul 2016 11:17:55 +0300 Subject: [PATCH 29/36] qemu-iotests: add vmdk for test backup compression in 055 The vmdk format has support for compression, it would be fine to add it for the test backup compression Signed-off-by: Pavel Butsykin Reviewed-by: Stefan Hajnoczi Signed-off-by: Denis V. Lunev CC: Jeff Cody CC: Markus Armbruster CC: Eric Blake CC: John Snow CC: Stefan Hajnoczi CC: Kevin Wolf Signed-off-by: Kevin Wolf --- tests/qemu-iotests/055 | 57 +++++++++++++++++++++++++++++------------- 1 file changed, 39 insertions(+), 18 deletions(-) diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055 index 0e1326cba3..ff4535e3ea 100755 --- a/tests/qemu-iotests/055 +++ b/tests/qemu-iotests/055 @@ -451,7 +451,8 @@ class TestSingleTransaction(iotests.QMPTestCase): class TestDriveCompression(iotests.QMPTestCase): image_len = 64 * 1024 * 1024 # MB - outfmt = 'qcow2' + fmt_supports_compression = [{'type': 'qcow2', 'args': ()}, + {'type': 'vmdk', 'args': ('-o', 'subformat=streamOptimized')}] def setUp(self): # Write data to the image so we can compare later @@ -461,12 +462,6 @@ class TestDriveCompression(iotests.QMPTestCase): qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x22 162k 32k', test_img) qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x33 67043328 64k', test_img) - qemu_img('create', '-f', TestDriveCompression.outfmt, blockdev_target_img, - str(TestDriveCompression.image_len)) - self.vm = iotests.VM().add_drive(test_img).add_drive(blockdev_target_img, - format=TestDriveCompression.outfmt) - self.vm.launch() - def tearDown(self): self.vm.shutdown() os.remove(test_img) @@ -476,7 +471,18 @@ class TestDriveCompression(iotests.QMPTestCase): except OSError: pass - def do_test_compress_complete(self, cmd, **args): + def do_prepare_drives(self, fmt, args): + self.vm = iotests.VM().add_drive(test_img) + + qemu_img('create', '-f', fmt, blockdev_target_img, + str(TestDriveCompression.image_len), *args) + self.vm.add_drive(blockdev_target_img, format=fmt) + + self.vm.launch() + + def do_test_compress_complete(self, cmd, format, **args): + self.do_prepare_drives(format['type'], format['args']) + self.assert_no_active_block_jobs() result = self.vm.qmp(cmd, device='drive0', sync='full', compress=True, **args) @@ -486,16 +492,21 @@ class TestDriveCompression(iotests.QMPTestCase): self.vm.shutdown() self.assertTrue(iotests.compare_images(test_img, blockdev_target_img, - iotests.imgfmt, TestDriveCompression.outfmt), + iotests.imgfmt, format['type']), 'target image does not match source after backup') def test_complete_compress_drive_backup(self): - self.do_test_compress_complete('drive-backup', target=blockdev_target_img, mode='existing') + for format in TestDriveCompression.fmt_supports_compression: + self.do_test_compress_complete('drive-backup', format, + target=blockdev_target_img, mode='existing') def test_complete_compress_blockdev_backup(self): - self.do_test_compress_complete('blockdev-backup', target='drive1') + for format in TestDriveCompression.fmt_supports_compression: + self.do_test_compress_complete('blockdev-backup', format, target='drive1') + + def do_test_compress_cancel(self, cmd, format, **args): + self.do_prepare_drives(format['type'], format['args']) - def do_test_compress_cancel(self, cmd, **args): self.assert_no_active_block_jobs() result = self.vm.qmp(cmd, device='drive0', sync='full', compress=True, **args) @@ -504,13 +515,20 @@ class TestDriveCompression(iotests.QMPTestCase): event = self.cancel_and_wait() self.assert_qmp(event, 'data/type', 'backup') + self.vm.shutdown() + def test_compress_cancel_drive_backup(self): - self.do_test_compress_cancel('drive-backup', target=blockdev_target_img, mode='existing') + for format in TestDriveCompression.fmt_supports_compression: + self.do_test_compress_cancel('drive-backup', format, + target=blockdev_target_img, mode='existing') def test_compress_cancel_blockdev_backup(self): - self.do_test_compress_cancel('blockdev-backup', target='drive1') + for format in TestDriveCompression.fmt_supports_compression: + self.do_test_compress_cancel('blockdev-backup', format, target='drive1') + + def do_test_compress_pause(self, cmd, format, **args): + self.do_prepare_drives(format['type'], format['args']) - def do_test_compress_pause(self, cmd, **args): self.assert_no_active_block_jobs() self.vm.pause_drive('drive0') @@ -536,14 +554,17 @@ class TestDriveCompression(iotests.QMPTestCase): self.vm.shutdown() self.assertTrue(iotests.compare_images(test_img, blockdev_target_img, - iotests.imgfmt, TestDriveCompression.outfmt), + iotests.imgfmt, format['type']), 'target image does not match source after backup') def test_compress_pause_drive_backup(self): - self.do_test_compress_pause('drive-backup', target=blockdev_target_img, mode='existing') + for format in TestDriveCompression.fmt_supports_compression: + self.do_test_compress_pause('drive-backup', format, + target=blockdev_target_img, mode='existing') def test_compress_pause_blockdev_backup(self): - self.do_test_compress_pause('blockdev-backup', target='drive1') + for format in TestDriveCompression.fmt_supports_compression: + self.do_test_compress_pause('blockdev-backup', format, target='drive1') if __name__ == '__main__': iotests.main(supported_fmts=['raw', 'qcow2']) From 980e66216ffc3e37903f979e02c5f63152b518c3 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 10 Aug 2016 13:06:55 +0200 Subject: [PATCH 30/36] test-coroutine: Fix coroutine pool corruption The test case overwrites the Coroutine object with 0xff as a way to assert that the coroutine isn't used any more. However, this means that the coroutine pool now contains a corrupted object and later test cases may get this corrupted object and crash. This patch saves the real content of the object and restores it after completing the test. The only use of the coroutine pool between those two points is the deletion of co2. As this only means an insertion at the head of an SLIST (release_pool or alloc_pool), it doesn't access the invalid list pointers that co1 has during this period. Signed-off-by: Kevin Wolf Reviewed-by: Stefan Hajnoczi --- tests/test-coroutine.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/test-coroutine.c b/tests/test-coroutine.c index ee5e06d327..6431dd6d7c 100644 --- a/tests/test-coroutine.c +++ b/tests/test-coroutine.c @@ -139,13 +139,20 @@ static void test_co_queue(void) { Coroutine *c1; Coroutine *c2; + Coroutine tmp; c2 = qemu_coroutine_create(c2_fn, NULL); c1 = qemu_coroutine_create(c1_fn, c2); qemu_coroutine_enter(c1); + + /* c1 shouldn't be used any more now; make sure we segfault if it is */ + tmp = *c1; memset(c1, 0xff, sizeof(Coroutine)); qemu_coroutine_enter(c2); + + /* Must restore the coroutine now to avoid corrupted pool */ + *c1 = tmp; } /* From 8b2bd09338ae44e35a46e324cda566dbf17fc21c Mon Sep 17 00:00:00 2001 From: Pavel Butsykin Date: Mon, 15 Aug 2016 12:39:22 +0300 Subject: [PATCH 31/36] qcow2: fix iovec size at qcow2_co_pwritev_compressed Use bytes as the size would be more exact than s->cluster_size. Although qemu_iovec_to_buf() will not allow to go beyond the qiov. Signed-off-by: Pavel Butsykin Signed-off-by: Kevin Wolf --- block/qcow2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/qcow2.c b/block/qcow2.c index adf451491f..c079aa83b6 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2565,7 +2565,7 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset, /* Zero-pad last write if image size is not cluster aligned */ memset(buf + bytes, 0, s->cluster_size - bytes); } - qemu_iovec_to_buf(qiov, 0, buf, s->cluster_size); + qemu_iovec_to_buf(qiov, 0, buf, bytes); out_buf = g_malloc(s->cluster_size); From 0e438cdc932a785de72166af4641aafa103a6670 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 11 Aug 2016 17:45:06 +0200 Subject: [PATCH 32/36] coroutine: Let CoMutex remember who holds it In cases of deadlocks, knowing who holds a given CoMutex is really helpful for debugging. Keeping the information around doesn't cost much and allows us to add another assertion to keep the code correct, so let's just add it. Signed-off-by: Kevin Wolf Reviewed-by: Paolo Bonzini Reviewed-by: Stefan Hajnoczi --- include/qemu/coroutine.h | 1 + util/qemu-coroutine-lock.c | 3 +++ 2 files changed, 4 insertions(+) diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h index ac8d4c9cc8..29a20782f0 100644 --- a/include/qemu/coroutine.h +++ b/include/qemu/coroutine.h @@ -143,6 +143,7 @@ bool qemu_co_queue_empty(CoQueue *queue); */ typedef struct CoMutex { bool locked; + Coroutine *holder; CoQueue queue; } CoMutex; diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c index 22aa9abb30..f30ee8184d 100644 --- a/util/qemu-coroutine-lock.c +++ b/util/qemu-coroutine-lock.c @@ -129,6 +129,7 @@ void coroutine_fn qemu_co_mutex_lock(CoMutex *mutex) } mutex->locked = true; + mutex->holder = self; trace_qemu_co_mutex_lock_return(mutex, self); } @@ -140,9 +141,11 @@ void coroutine_fn qemu_co_mutex_unlock(CoMutex *mutex) trace_qemu_co_mutex_unlock_entry(mutex, self); assert(mutex->locked == true); + assert(mutex->holder == self); assert(qemu_in_coroutine()); mutex->locked = false; + mutex->holder = NULL; qemu_co_queue_next(&mutex->queue); trace_qemu_co_mutex_unlock_return(mutex, self); From 1b7f01d966f97b7820f3cdd471461cf0799a93cc Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 11 Aug 2016 17:51:59 +0200 Subject: [PATCH 33/36] coroutine: Assert that no locks are held on termination A coroutine that takes a lock must also release it again. If the coroutine terminates without having released all its locks, it's buggy and we'll probably run into a deadlock sooner or later. Make sure that we don't get such cases. Signed-off-by: Kevin Wolf Reviewed-by: Paolo Bonzini Reviewed-by: Stefan Hajnoczi --- include/qemu/coroutine_int.h | 1 + util/qemu-coroutine-lock.c | 11 +++++++++++ util/qemu-coroutine.c | 1 + 3 files changed, 13 insertions(+) diff --git a/include/qemu/coroutine_int.h b/include/qemu/coroutine_int.h index 581a7f5140..6df9d33352 100644 --- a/include/qemu/coroutine_int.h +++ b/include/qemu/coroutine_int.h @@ -39,6 +39,7 @@ struct Coroutine { void *entry_arg; Coroutine *caller; QSLIST_ENTRY(Coroutine) pool_next; + size_t locks_held; /* Coroutines that should be woken up when we yield or terminate */ QSIMPLEQ_HEAD(, Coroutine) co_queue_wakeup; diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c index f30ee8184d..14cf9ce458 100644 --- a/util/qemu-coroutine-lock.c +++ b/util/qemu-coroutine-lock.c @@ -130,6 +130,7 @@ void coroutine_fn qemu_co_mutex_lock(CoMutex *mutex) mutex->locked = true; mutex->holder = self; + self->locks_held++; trace_qemu_co_mutex_lock_return(mutex, self); } @@ -146,6 +147,7 @@ void coroutine_fn qemu_co_mutex_unlock(CoMutex *mutex) mutex->locked = false; mutex->holder = NULL; + self->locks_held--; qemu_co_queue_next(&mutex->queue); trace_qemu_co_mutex_unlock_return(mutex, self); @@ -159,14 +161,19 @@ void qemu_co_rwlock_init(CoRwlock *lock) void qemu_co_rwlock_rdlock(CoRwlock *lock) { + Coroutine *self = qemu_coroutine_self(); + while (lock->writer) { qemu_co_queue_wait(&lock->queue); } lock->reader++; + self->locks_held++; } void qemu_co_rwlock_unlock(CoRwlock *lock) { + Coroutine *self = qemu_coroutine_self(); + assert(qemu_in_coroutine()); if (lock->writer) { lock->writer = false; @@ -179,12 +186,16 @@ void qemu_co_rwlock_unlock(CoRwlock *lock) qemu_co_queue_next(&lock->queue); } } + self->locks_held--; } void qemu_co_rwlock_wrlock(CoRwlock *lock) { + Coroutine *self = qemu_coroutine_self(); + while (lock->writer || lock->reader) { qemu_co_queue_wait(&lock->queue); } lock->writer = true; + self->locks_held++; } diff --git a/util/qemu-coroutine.c b/util/qemu-coroutine.c index 89f21a9cec..3cbf225487 100644 --- a/util/qemu-coroutine.c +++ b/util/qemu-coroutine.c @@ -122,6 +122,7 @@ void qemu_coroutine_enter(Coroutine *co) case COROUTINE_YIELD: return; case COROUTINE_TERMINATE: + assert(!co->locks_held); trace_qemu_coroutine_terminate(co); coroutine_delete(co); return; From 1562047c899ccecdf8af9645ce7fd35fcd2fdb31 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 12 Jul 2016 18:15:49 +0200 Subject: [PATCH 34/36] block jobs: Improve error message for missing job ID If a block job is started with a node name rather than a device name and no explicit job ID is passed, it was reported that '' isn't a well-formed ID. Which is correct, but we can make the message a little bit nicer. Signed-off-by: Kevin Wolf Reviewed-by: Jeff Cody Reviewed-by: Alberto Garcia Reviewed-by: Stefan Hajnoczi --- blockjob.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/blockjob.c b/blockjob.c index a5ba3bee52..a167f96fd4 100644 --- a/blockjob.c +++ b/blockjob.c @@ -132,6 +132,10 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver, if (job_id == NULL) { job_id = bdrv_get_device_name(bs); + if (!*job_id) { + error_setg(errp, "An explicit job ID is required for this node"); + return NULL; + } } if (!id_wellformed(job_id)) { From c0088d79a7d0d9070a46331b60ec8aa871fe2085 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 23 Aug 2016 16:44:03 +0200 Subject: [PATCH 35/36] qemu-iotests: Log QMP traffic in debug mode Python tests are already annoying enough to debug. With QMP traffic available it's a little bit easier at least. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- tests/qemu-iotests/iotests.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index 69aa41e514..f1f36d7fc7 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -50,6 +50,7 @@ cachemode = os.environ.get('CACHEMODE') qemu_default_machine = os.environ.get('QEMU_DEFAULT_MACHINE') socket_scm_helper = os.environ.get('SOCKET_SCM_HELPER', 'socket_scm_helper') +debug = False def qemu_img(*args): '''Run qemu-img and return the exit code''' @@ -134,6 +135,8 @@ class VM(qtest.QEMUQtestMachine): def __init__(self): super(VM, self).__init__(qemu_prog, qemu_opts, test_dir=test_dir, socket_scm_helper=socket_scm_helper) + if debug: + self._debug = True self._num_drives = 0 def add_drive_raw(self, opts): @@ -318,6 +321,8 @@ def verify_quorum(): def main(supported_fmts=[], supported_oses=['linux']): '''Run tests''' + global debug + # We are using TEST_DIR and QEMU_DEFAULT_MACHINE as proxies to # indicate that we're not being run via "check". There may be # other things set up by "check" that individual test cases rely From e7f98f2f92827df9944402d1613a4e32fe50215b Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 13 Jul 2016 17:27:51 +0200 Subject: [PATCH 36/36] block: Allow node name for 'qemu-io' HMP command When using a node name, create a temporary BlockBackend that is used to run the qemu-io command. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- hmp.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/hmp.c b/hmp.c index a7dfe6fa11..ad33b442d2 100644 --- a/hmp.c +++ b/hmp.c @@ -1923,11 +1923,22 @@ void hmp_chardev_remove(Monitor *mon, const QDict *qdict) void hmp_qemu_io(Monitor *mon, const QDict *qdict) { BlockBackend *blk; + BlockBackend *local_blk = NULL; const char* device = qdict_get_str(qdict, "device"); const char* command = qdict_get_str(qdict, "command"); Error *err = NULL; blk = blk_by_name(device); + if (!blk) { + BlockDriverState *bs = bdrv_lookup_bs(NULL, device, &err); + if (bs) { + blk = local_blk = blk_new(); + blk_insert_bs(blk, bs); + } else { + goto fail; + } + } + if (blk) { AioContext *aio_context = blk_get_aio_context(blk); aio_context_acquire(aio_context); @@ -1940,6 +1951,8 @@ void hmp_qemu_io(Monitor *mon, const QDict *qdict) "Device '%s' not found", device); } +fail: + blk_unref(local_blk); hmp_handle_error(mon, &err); }