Re: [Qemu-block] [Qemu-stable] [PATCH v7 0/8] block: Mirror discarded sectors
On Thu, 06/11 16:29, Fam Zheng wrote: On Mon, 06/08 14:02, Stefan Hajnoczi wrote: On Mon, Jun 08, 2015 at 01:56:06PM +0800, Fam Zheng wrote: v7: Fix the lost assignment of s-unmap. v6: Fix pnum in bdrv_get_block_status_above. [Paolo] v5: Rewrite patch 1. Address Eric's comments on patch 3. Add Eric's rev-by to patches 2 4. Check BDRV_BLOCK_DATA in patch 3. [Paolo] This fixes the mirror assert failure reported by wangxiaolong: https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg04458.html The direct cause is that hbitmap code couldn't handle unset of bits *after* iterator's current position. We could fix that, but the bdrv_reset_dirty() call is more questionable: Before, if guest discarded some sectors during migration, it could see different data after moving to dest side, depending on block backends of the src and the dest. This is IMO worse than mirroring the actual reading as done in this series, because we don't know what the guest is doing. For example if a guest first issues WRITE SAME to wipe out the area then issues UNMAP to discard it, just to get rid of some sensitive data completely, we may miss both operations and leave stale data on dest image. Fam Zheng (8): block: Add bdrv_get_block_status_above qmp: Add optional bool unmap to drive-mirror mirror: Do zero write on target if sectors not allocated block: Fix dirty bitmap in bdrv_co_discard block: Remove bdrv_reset_dirty qemu-iotests: Make block job methods common qemu-iotests: Add test case for mirror with unmap iotests: Use event_wait in wait_ready block.c | 12 block/io.c| 60 ++- block/mirror.c| 28 +++--- blockdev.c| 5 hmp.c | 2 +- include/block/block.h | 4 +++ include/block/block_int.h | 4 +-- qapi/block-core.json | 8 +- qmp-commands.hx | 3 ++ tests/qemu-iotests/041| 66 ++- tests/qemu-iotests/132| 59 ++ tests/qemu-iotests/132.out| 5 tests/qemu-iotests/group | 1 + tests/qemu-iotests/iotests.py | 23 +++ 14 files changed, 196 insertions(+), 84 deletions(-) create mode 100644 tests/qemu-iotests/132 create mode 100644 tests/qemu-iotests/132.out -- 2.4.2 Thanks, applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan, The only controversial patches are the qmp/drive-mirror ones (1-3), while patches 4-8 are still useful on their own: they fix the mentioned crash and improve iotests. Shall we merge the second half (of course none of them depend on 1-3) now that softfreeze is approaching? Stefan, would you consider applying patches 4-8? Fam
[Qemu-block] [PATCH] virito-blk: drop duplicate check
From: Gonglei arei.gong...@huawei.com
in_num = req-elem.in_num, and req-elem.in_num is
checked in line 489, so the check about in_num variable
is superflous, let's drop it.
Signed-off-by: Gonglei arei.gong...@huawei.com
---
hw/block/virtio-blk.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index cd539aa..cce552a 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -499,8 +499,7 @@ void virtio_blk_handle_request(VirtIOBlockReq *req,
MultiReqBuffer *mrb)
iov_discard_front(iov, out_num, sizeof(req-out));
-if (in_num 1 ||
-in_iov[in_num - 1].iov_len sizeof(struct virtio_blk_inhdr)) {
+if (in_iov[in_num - 1].iov_len sizeof(struct virtio_blk_inhdr)) {
error_report(virtio-blk request inhdr too short);
exit(1);
}
--
1.7.12.4
Re: [Qemu-block] [PATCH v2 02/13] block: Introduce bdrv_lock and bdrv_unlock API
On 24/06/2015 04:47, Fam Zheng wrote: 2. Is this about thread safety? (No, it's about exclusive access to a BDS *within* the AioContext.) As it has to quiesce iothreads as well (for now it's even more urgent than exclusive access within the same AioContext), I'd rather take it as yes. For now it's a no, because there are no races between threads: the main thread has acquired the AioContext and cut away the iothread. However, as we move towards fine-grained AioContext critical sections, it will become a yes. Paolo
Re: [Qemu-block] [PATCH] migration: flush the bdrv before stopping VM
Right now, we don't have an interface to detect that cases and
got back to the iterative stage.
How about go back to the iterative stage when detect that the
pending_size is larger Than max_size, like this:
+/* do flush here is aimed to shorten the VM downtime,
+ * bdrv_flush_all is a time consuming operation
+ * when the guest has done some file writing */
+bdrv_flush_all();
+pending_size = qemu_savevm_state_pending(s-file,
max_size);
+if (pending_size pending_size = max_size) {
+qemu_mutex_unlock_iothread();
+continue;
+}
ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
if (ret = 0) {
qemu_file_set_rate_limit(s-file,
INT64_MAX);
and this is quite simple.
Yes, but it is too simple. If you hold all the locks during
bdrv_flush_all(), your VM will effectively stop as soon as it
performs the next I/O access, so you don't win much. And you
still don't have a timeout for cases where the flush takes really long.
This is probably better than what we had now (basically we are
meassuring
after bdrv_flush_all how much the amount of dirty memory has
changed, and return to iterative stage if it took too much. A
timeout would be better anyways. And an interface te start the
synchronization sooner asynchronously would be also good.
Notice that my understanding is that any proper fix for this is 2.4
material.
Then, how to deal with this issue in 2.3, leave it here? or make an
incomplete fix like I do above?
I think it is better to leave it here for 2.3. With a patch like this
one, we improve in one load and we got worse in a different load
(depens a lot in the ratio of dirtying memory vs disk). I have no
data which load is more common, so I prefer to be conservative so late
in the cycle. What do you think?
I agree, it's too late in the release cycle for such a change.
Kevin
Hi Juan Kevin,
I have not found the related patches to fix the issue which lead to long VM
downtime, how is it going?
Liang
Re: [Qemu-block] [Qemu-devel] [PATCH] virito-blk: drop duplicate check
On Wed, 06/24 17:29, arei.gong...@huawei.com wrote:
From: Gonglei arei.gong...@huawei.com
in_num = req-elem.in_num, and req-elem.in_num is
checked in line 489, so the check about in_num variable
is superflous, let's drop it.
Signed-off-by: Gonglei arei.gong...@huawei.com
Reviewed-by: Fam Zheng f...@redhat.com
---
hw/block/virtio-blk.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index cd539aa..cce552a 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -499,8 +499,7 @@ void virtio_blk_handle_request(VirtIOBlockReq *req,
MultiReqBuffer *mrb)
iov_discard_front(iov, out_num, sizeof(req-out));
-if (in_num 1 ||
-in_iov[in_num - 1].iov_len sizeof(struct virtio_blk_inhdr)) {
+if (in_iov[in_num - 1].iov_len sizeof(struct virtio_blk_inhdr)) {
error_report(virtio-blk request inhdr too short);
exit(1);
}
--
1.7.12.4
Re: [Qemu-block] [Qemu-devel] [PATCH COLO-Block v6 07/16] Add new block driver interface to connect/disconnect the remote target
* Wen Congyang (ghost...@gmail.com) wrote:
At 2015/6/19 18:49, Stefan Hajnoczi Wrote:
On Fri, Jun 19, 2015 at 08:54:56AM +0800, Wen Congyang wrote:
On 06/19/2015 12:06 AM, Stefan Hajnoczi wrote:
On Thu, Jun 18, 2015 at 10:36:39PM +0800, Wen Congyang wrote:
At 2015/6/18 20:55, Stefan Hajnoczi Wrote:
On Thu, Jun 18, 2015 at 04:49:12PM +0800, Wen Congyang wrote:
+void bdrv_connect(BlockDriverState *bs, Error **errp)
+{
+BlockDriver *drv = bs-drv;
+
+if (drv drv-bdrv_connect) {
+drv-bdrv_connect(bs, errp);
+} else if (bs-file) {
+bdrv_connect(bs-file, errp);
+} else {
+error_setg(errp, this feature or command is not currently
supported);
+}
+}
+
+void bdrv_disconnect(BlockDriverState *bs)
+{
+BlockDriver *drv = bs-drv;
+
+if (drv drv-bdrv_disconnect) {
+drv-bdrv_disconnect(bs);
+} else if (bs-file) {
+bdrv_disconnect(bs-file);
+}
+}
Please add doc comments describing the semantics of these commands.
Where should it be documented? In the header file?
block.h doesn't document prototypes in the header file, please document
the function definition in block.c. (QEMU is not consistent here, some
places do it the other way around.)
Why are these operations needed when there is already a bs-drv == NULL
case which means the BDS is not ready for read/write?
The purpos is that: don't connect to nbd server when opening a nbd client.
connect/disconnect
to nbd server when we need to do it.
IIUC, if bs-drv is NULL, it means that the driver is ejected? Here,
connect/disconnect
means that connect/disconnect to remote target(The target may be in
another
host).
Connect/disconnect puts something on the QEMU command-line that isn't
ready at startup time.
How about using monitor commands to add objects when needed instead?
That is cleaner because it doesn't introduce a new state (which is only
implemented for nbd).
The problem is that, nbd client is one child of quorum, and quorum must
have more
than one child. The nbd server is not ready until colo is running.
A monitor command to hot add/remove quorum children solves this problem
and could also be used in other scenarios (e.g. user decides to take a
quorum child offline).
For replication case, we always do checkpoint again and again after
migration. If the disk is not synced before migration, we will use disk
mirgation or mirror
job to sync it.
Can you document the way that you use disk migration or mirror, together
with your COLO setup? I think it would make it easier to understand this
restriction.
At the moment I don't understand how you can switch from doing a disk migration
into COLO mode - there seems to be a gap between the end of disk migration and
the start
of COLO.
So we cannot start block replication when migration is running. We need
that nbd
client is not ready when migration is running, and it is ready between
migration ends
and checkpoint begins. Using a monotir command add the nbd client will cause
larger
downtime. So if the nbd client has been added(only not connect to the nbd
server),
we can connect to nbd server automatically.
Without the disk migration or mirror, I can't see the need for the delayed
bdrv_connect.
I can see that you want to do a disconnect at failover, however you need
to take care because if the network is broken at the point of failover
you need to make sure nothing blocks.
Dave
Thanks
Wen Congyang
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-block] [Qemu-devel] [PATCH 0/9] block: add differential backup support
On Tue, Jun 23, 2015 at 01:00:44PM -0400, John Snow wrote: Stefan: I know you said this wasn't expressly requisite, but I was encouraged by Kevin to send it out as a usability patch for non-libvirt users, so here it is. Final Yeah sure or No, definitely not as 2.4 approaches? Also note that if the answer is Ehhh, maybe not now, nobody's REALLY asked for it we should still probably merge the first patch alone, before our API solidifies -- this will give us the breathing room to add these features in later if we want. I'll take a look at the first patch. pgpM1je6r0lNc.pgp Description: PGP signature
Re: [Qemu-block] [PATCH 3/6] block: Extract blockdev part of qmp_drive_mirror
On Wed, 06/24 18:34, Max Reitz wrote:
On 09.06.2015 04:13, Fam Zheng wrote:
This is the part that will be reused by blockdev-mirror.
Signed-off-by: Fam Zheng f...@redhat.com
---
blockdev.c | 158
+++--
1 file changed, 92 insertions(+), 66 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index b573e56..c32a9a9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2883,29 +2883,22 @@ out:
#define DEFAULT_MIRROR_BUF_SIZE (10 20)
-void qmp_drive_mirror(const char *device, const char *target,
- bool has_format, const char *format,
- bool has_node_name, const char *node_name,
- bool has_replaces, const char *replaces,
- enum MirrorSyncMode sync,
- bool has_mode, enum NewImageMode mode,
- bool has_speed, int64_t speed,
- bool has_granularity, uint32_t granularity,
- bool has_buf_size, int64_t buf_size,
- bool has_on_source_error, BlockdevOnError
on_source_error,
- bool has_on_target_error, BlockdevOnError
on_target_error,
- Error **errp)
+/* Parameter check and block job starting for mirroring.
+ * Caller should hold @device and @target's aio context (They must to be on
the
+ * same aio context). */
+static void blockdev_mirror_common(BlockDriverState *bs,
+ BlockDriverState *target,
+ bool has_replaces, const char *replaces,
+ enum MirrorSyncMode sync,
+ bool has_speed, int64_t speed,
+ bool has_granularity, uint32_t
granularity,
+ bool has_buf_size, int64_t buf_size,
+ bool has_on_source_error,
+ BlockdevOnError on_source_error,
+ bool has_on_target_error,
+ BlockdevOnError on_target_error,
+ Error **errp)
{
-BlockBackend *blk;
-BlockDriverState *bs;
-BlockDriverState *source, *target_bs;
-AioContext *aio_context;
-BlockDriver *drv = NULL;
-Error *local_err = NULL;
-QDict *options = NULL;
-int flags;
-int64_t size;
-int ret;
if (!has_speed) {
speed = 0;
@@ -2916,9 +2909,6 @@ void qmp_drive_mirror(const char *device, const char
*target,
if (!has_on_target_error) {
on_target_error = BLOCKDEV_ON_ERROR_REPORT;
}
-if (!has_mode) {
-mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
-}
if (!has_granularity) {
granularity = 0;
}
@@ -2936,35 +2926,73 @@ void qmp_drive_mirror(const char *device, const char
*target,
return;
}
-blk = blk_by_name(device);
-if (!blk) {
-error_set(errp, QERR_DEVICE_NOT_FOUND, device);
-return;
-}
-
-aio_context = blk_get_aio_context(blk);
-aio_context_acquire(aio_context);
-
-if (!blk_is_available(blk)) {
-error_set(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;
-}
-if (format) {
-drv = bdrv_find_format(format);
-if (!drv) {
-error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
-goto out;
-}
-}
-
if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) {
+return;
+}
+
+if (!bs-backing_hd sync == MIRROR_SYNC_MODE_TOP) {
+sync = MIRROR_SYNC_MODE_FULL;
+}
+
+/* pass the node name to replace to mirror start since it's loose
coupling
+ * and will allow to check whether the node still exist at mirror
completion
+ */
+mirror_start(bs, target,
+ has_replaces ? replaces : NULL,
+ speed, granularity, buf_size, sync,
+ on_source_error, on_target_error,
+ block_job_cb, bs, errp);
+}
+
+void qmp_drive_mirror(const char *device, const char *target,
+ bool has_format, const char *format,
+ bool has_node_name, const char *node_name,
+ bool has_replaces, const char *replaces,
+ enum MirrorSyncMode sync,
+ bool has_mode, enum NewImageMode mode,
+ bool has_speed, int64_t speed,
+ bool has_granularity, uint32_t granularity,
+ bool has_buf_size, int64_t buf_size,
+ bool has_on_source_error, BlockdevOnError
on_source_error,
+
Re: [Qemu-block] [PATCH] virito-blk: drop duplicate check
On Wed, Jun 24, 2015 at 05:29:24PM +0800, arei.gong...@huawei.com wrote: From: Gonglei arei.gong...@huawei.com in_num = req-elem.in_num, and req-elem.in_num is checked in line 489, so the check about in_num variable is superflous, let's drop it. Signed-off-by: Gonglei arei.gong...@huawei.com --- hw/block/virtio-blk.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) Thanks, applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan pgpkE8d0idVhW.pgp Description: PGP signature
[Qemu-block] [PATCH v2 2/6] block: Rename BLOCK_OP_TYPE_MIRROR to BLOCK_OP_TYPE_MIRROR_SOURCE
It's necessary to distinguish source and target before we can add
blockdev-mirror, because we would want a concrete type of operation to
check on target bs before starting.
Signed-off-by: Fam Zheng f...@redhat.com
Reviewed-by: Max Reitz mre...@redhat.com
---
blockdev.c | 2 +-
hw/block/dataplane/virtio-blk.c | 2 +-
include/block/block.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 90f6e77..b573e56 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2962,7 +2962,7 @@ void qmp_drive_mirror(const char *device, const char
*target,
}
}
-if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR, errp)) {
+if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) {
goto out;
}
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 3db139b..dac37de 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -206,7 +206,7 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev,
VirtIOBlkConf *conf,
blk_op_unblock(conf-conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT,
s-blocker);
blk_op_unblock(conf-conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE,
s-blocker);
-blk_op_unblock(conf-conf.blk, BLOCK_OP_TYPE_MIRROR, s-blocker);
+blk_op_unblock(conf-conf.blk, BLOCK_OP_TYPE_MIRROR_SOURCE, s-blocker);
blk_op_unblock(conf-conf.blk, BLOCK_OP_TYPE_STREAM, s-blocker);
blk_op_unblock(conf-conf.blk, BLOCK_OP_TYPE_REPLACE, s-blocker);
diff --git a/include/block/block.h b/include/block/block.h
index f5c0a90..69cb676 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -155,7 +155,7 @@ typedef enum BlockOpType {
BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT,
BLOCK_OP_TYPE_INTERNAL_SNAPSHOT,
BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE,
-BLOCK_OP_TYPE_MIRROR,
+BLOCK_OP_TYPE_MIRROR_SOURCE,
BLOCK_OP_TYPE_RESIZE,
BLOCK_OP_TYPE_STREAM,
BLOCK_OP_TYPE_REPLACE,
--
2.4.3
[Qemu-block] [PATCH v2 5/6] qmp: Add blockdev-mirror command
This will start a mirror job from a named device to another named
device, its relation with drive-mirror is similar with blockdev-backup
to drive-backup.
In blockdev-mirror, the target node should be prepared by blockdev-add,
which will be responsible for assigning a name to the new node, so
'node-name' in drive-mirror is dropped.
Signed-off-by: Fam Zheng f...@redhat.com
---
blockdev.c | 61
qapi/block-core.json | 47
qmp-commands.hx | 48 +
3 files changed, 156 insertions(+)
diff --git a/blockdev.c b/blockdev.c
index de6383f..e0dba07 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2932,6 +2932,10 @@ static void blockdev_mirror_common(BlockDriverState *bs,
if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_MIRROR_TARGET, errp)) {
return;
}
+if (target-blk) {
+error_setg(errp, Cannot mirror to an attached block device);
+return;
+}
if (!bs-backing_hd sync == MIRROR_SYNC_MODE_TOP) {
sync = MIRROR_SYNC_MODE_FULL;
@@ -3107,6 +3111,63 @@ out:
aio_context_release(aio_context);
}
+void qmp_blockdev_mirror(const char *device, const char *target,
+ bool has_replaces, const char *replaces,
+ MirrorSyncMode sync,
+ bool has_speed, int64_t speed,
+ bool has_granularity, uint32_t granularity,
+ bool has_buf_size, int64_t buf_size,
+ bool has_on_source_error,
+ BlockdevOnError on_source_error,
+ bool has_on_target_error,
+ BlockdevOnError on_target_error,
+ Error **errp)
+{
+BlockDriverState *bs;
+BlockBackend *blk;
+BlockDriverState *target_bs;
+AioContext *aio_context;
+Error *local_err = NULL;
+
+blk = blk_by_name(device);
+if (!blk) {
+error_setg(errp, Device '%s' not found, device);
+return;
+}
+bs = blk_bs(blk);
+
+if (!bs) {
+error_setg(errp, Device '%s' has no media, device);
+return;
+}
+
+target_bs = bdrv_lookup_bs(target, target, errp);
+if (!target_bs) {
+return;
+}
+
+aio_context = bdrv_get_aio_context(bs);
+aio_context_acquire(aio_context);
+
+bdrv_ref(target_bs);
+bdrv_set_aio_context(target_bs, aio_context);
+
+blockdev_mirror_common(bs, target_bs,
+ has_replaces, replaces, sync,
+ has_speed, speed,
+ has_granularity, granularity,
+ has_buf_size, buf_size,
+ has_on_source_error, on_source_error,
+ has_on_target_error, on_target_error,
+ local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+bdrv_unref(target_bs);
+}
+
+aio_context_release(aio_context);
+}
+
/* Get the block job for a given device name and acquire its AioContext */
static BlockJob *find_block_job(const char *device, AioContext **aio_context,
Error **errp)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index b5c4559..fe440e1 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1059,6 +1059,53 @@
'data': 'BlockDirtyBitmap' }
##
+# @blockdev-mirror
+#
+# Start mirroring a block device's writes to a new destination.
+#
+# @device: the name of the device whose writes should be mirrored.
+#
+# @target: the name of the device to mirror to.
+#
+# @replaces: #optional with sync=full graph node name to be replaced by the new
+#image when a whole image copy is done. This can be used to repair
+#broken Quorum files.
+#
+# @speed: #optional the maximum speed, in bytes per second
+#
+# @sync: what parts of the disk image should be copied to the destination
+#(all the disk, only the sectors allocated in the topmost image, or
+#only new I/O).
+#
+# @granularity: #optional granularity of the dirty bitmap, default is 64K
+# if the image format doesn't have clusters, 4K if the clusters
+# are smaller than that, else the cluster size. Must be a
+# power of 2 between 512 and 64M
+#
+# @buf-size: #optional maximum amount of data in flight from source to
+#target
+#
+# @on-source-error: #optional the action to take on an error on the source,
+# default 'report'. 'stop' and 'enospc' can only be used
+# if the block device supports io-status (see BlockInfo).
+#
+# @on-target-error: #optional the action to take on an error on the target,
+# default 'report' (no limitations, since this applies to
+# a different block device than @device).
+#
+# Returns: nothing
Re: [Qemu-block] [PATCH 3/6] block: Extract blockdev part of qmp_drive_mirror
On 09.06.2015 04:13, Fam Zheng wrote:
This is the part that will be reused by blockdev-mirror.
Signed-off-by: Fam Zheng f...@redhat.com
---
blockdev.c | 158 +++--
1 file changed, 92 insertions(+), 66 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index b573e56..c32a9a9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2883,29 +2883,22 @@ out:
#define DEFAULT_MIRROR_BUF_SIZE (10 20)
-void qmp_drive_mirror(const char *device, const char *target,
- bool has_format, const char *format,
- bool has_node_name, const char *node_name,
- bool has_replaces, const char *replaces,
- enum MirrorSyncMode sync,
- bool has_mode, enum NewImageMode mode,
- bool has_speed, int64_t speed,
- bool has_granularity, uint32_t granularity,
- bool has_buf_size, int64_t buf_size,
- bool has_on_source_error, BlockdevOnError
on_source_error,
- bool has_on_target_error, BlockdevOnError
on_target_error,
- Error **errp)
+/* Parameter check and block job starting for mirroring.
+ * Caller should hold @device and @target's aio context (They must to be on the
+ * same aio context). */
+static void blockdev_mirror_common(BlockDriverState *bs,
+ BlockDriverState *target,
+ bool has_replaces, const char *replaces,
+ enum MirrorSyncMode sync,
+ bool has_speed, int64_t speed,
+ bool has_granularity, uint32_t granularity,
+ bool has_buf_size, int64_t buf_size,
+ bool has_on_source_error,
+ BlockdevOnError on_source_error,
+ bool has_on_target_error,
+ BlockdevOnError on_target_error,
+ Error **errp)
{
-BlockBackend *blk;
-BlockDriverState *bs;
-BlockDriverState *source, *target_bs;
-AioContext *aio_context;
-BlockDriver *drv = NULL;
-Error *local_err = NULL;
-QDict *options = NULL;
-int flags;
-int64_t size;
-int ret;
if (!has_speed) {
speed = 0;
@@ -2916,9 +2909,6 @@ void qmp_drive_mirror(const char *device, const char
*target,
if (!has_on_target_error) {
on_target_error = BLOCKDEV_ON_ERROR_REPORT;
}
-if (!has_mode) {
-mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
-}
if (!has_granularity) {
granularity = 0;
}
@@ -2936,35 +2926,73 @@ void qmp_drive_mirror(const char *device, const char
*target,
return;
}
-blk = blk_by_name(device);
-if (!blk) {
-error_set(errp, QERR_DEVICE_NOT_FOUND, device);
-return;
-}
-
-aio_context = blk_get_aio_context(blk);
-aio_context_acquire(aio_context);
-
-if (!blk_is_available(blk)) {
-error_set(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;
-}
-if (format) {
-drv = bdrv_find_format(format);
-if (!drv) {
-error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
-goto out;
-}
-}
-
if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) {
+return;
+}
+
+if (!bs-backing_hd sync == MIRROR_SYNC_MODE_TOP) {
+sync = MIRROR_SYNC_MODE_FULL;
+}
+
+/* pass the node name to replace to mirror start since it's loose coupling
+ * and will allow to check whether the node still exist at mirror
completion
+ */
+mirror_start(bs, target,
+ has_replaces ? replaces : NULL,
+ speed, granularity, buf_size, sync,
+ on_source_error, on_target_error,
+ block_job_cb, bs, errp);
+}
+
+void qmp_drive_mirror(const char *device, const char *target,
+ bool has_format, const char *format,
+ bool has_node_name, const char *node_name,
+ bool has_replaces, const char *replaces,
+ enum MirrorSyncMode sync,
+ bool has_mode, enum NewImageMode mode,
+ bool has_speed, int64_t speed,
+ bool has_granularity, uint32_t granularity,
+ bool has_buf_size, int64_t buf_size,
+ bool has_on_source_error, BlockdevOnError
on_source_error,
+ bool has_on_target_error, BlockdevOnError
on_target_error,
+ Error **errp)
+{
+BlockDriverState *bs;
+BlockBackend *blk;
+
Re: [Qemu-block] [PATCH 2/6] block: Rename BLOCK_OP_TYPE_MIRROR to BLOCK_OP_TYPE_MIRROR_SOURCE
On 09.06.2015 04:13, Fam Zheng wrote: It's necessary to distinguish source and target before we can add blockdev-mirror, because we would want a concrete type of operation to check on target bs before starting. Signed-off-by: Fam Zheng f...@redhat.com --- blockdev.c | 2 +- hw/block/dataplane/virtio-blk.c | 2 +- include/block/block.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) Reviewed-by: Max Reitz mre...@redhat.com
Re: [Qemu-block] [PATCH 4/6] block: Add check on mirror target
On 09.06.2015 04:13, Fam Zheng wrote: Signed-off-by: Fam Zheng f...@redhat.com --- blockdev.c| 3 +++ include/block/block.h | 1 + 2 files changed, 4 insertions(+) Reviewed-by: Max Reitz mre...@redhat.com
Re: [Qemu-block] [PATCH 1/6] block: Add blocker on mirror target
On 09.06.2015 04:13, Fam Zheng wrote: In block/backup.c, we already check and add blocker on the target bs, which is necessary so that it won't be intervened with other operations. In block/mirror.c we should also protect the mirror target bs, because it could have a node-name (drive-mirror ... node-name=XXX), and on top of that it's safe to add blockdev-mirror. Signed-off-by: Fam Zheng f...@redhat.com --- block/mirror.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/block/mirror.c b/block/mirror.c index 33c640f..8b23ddd 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -360,6 +360,7 @@ static void mirror_exit(BlockJob *job, void *opaque) aio_context_release(replace_aio_context); } g_free(s-replaces); +bdrv_op_unblock_all(s-target, s-common.blocker); bdrv_unref(s-target); block_job_completed(s-common, data-ret); g_free(data); @@ -682,6 +683,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, return; } +bdrv_op_block_all(target, s-common.blocker); s-replaces = g_strdup(replaces); s-on_source_error = on_source_error; s-on_target_error = on_target_error; Do we need a bdrv_op_unblock_all() if bdrv_create_dirty_bitmap() fails? Max
Re: [Qemu-block] [Qemu-devel] [PATCH] qcow2: Handle EAGAIN returned from update_refcount
On 24.06.2015 07:05, Jindřich Makovička wrote: Fixes a crash during image compression Signed-off-by: Jindřich Makovička makov...@gmail.com --- block/qcow2-refcount.c | 22 -- 1 file changed, 12 insertions(+), 10 deletions(-) Reviewed-by: Max Reitz mre...@redhat.com
Re: [Qemu-block] [PATCH 6/6] iotests: Add test cases for blockdev-mirror
On 09.06.2015 04:13, Fam Zheng wrote:
Signed-off-by: Fam Zheng f...@redhat.com
---
tests/qemu-iotests/041 | 100 +++--
tests/qemu-iotests/041.out | 4 +-
2 files changed, 80 insertions(+), 24 deletions(-)
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 59a8f73..922f53c 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -65,8 +65,23 @@ class ImageMirroringTestCase(iotests.QMPTestCase):
event = self.wait_until_completed(drive=drive)
self.assert_qmp(event, 'data/type', 'mirror')
+def blockdev_add(self, filename, drive_id=None, node_name=None):
+options = {'driver': iotests.imgfmt,
+ 'file': {
+ 'filename': filename,
+ 'driver': 'file'}}
+if drive_id:
+options['id'] = drive_id
+if node_name:
+options['node-name'] = node_name
+result = self.vm.qmp('blockdev-add', options=options)
+self.assert_qmp(result, 'return', {})
+
class TestSingleDrive(ImageMirroringTestCase):
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)
@@ -86,8 +101,8 @@ class TestSingleDrive(ImageMirroringTestCase):
def test_complete(self):
self.assert_no_active_block_jobs()
-result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
- target=target_img)
+result = self.vm.qmp(self.qmp_cmd, device='drive0', sync='full',
+ target=self.qmp_target)
self.assert_qmp(result, 'return', {})
self.complete_and_wait()
@@ -100,8 +115,8 @@ class TestSingleDrive(ImageMirroringTestCase):
def test_cancel(self):
self.assert_no_active_block_jobs()
-result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
- target=target_img)
+result = self.vm.qmp(self.qmp_cmd, device='drive0', sync='full',
+ target=self.qmp_target)
self.assert_qmp(result, 'return', {})
self.cancel_and_wait(force=True)
@@ -112,8 +127,8 @@ class TestSingleDrive(ImageMirroringTestCase):
def test_cancel_after_ready(self):
self.assert_no_active_block_jobs()
-result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
- target=target_img)
+result = self.vm.qmp(self.qmp_cmd, device='drive0', sync='full',
+ target=self.qmp_target)
self.assert_qmp(result, 'return', {})
self.wait_ready_and_cancel()
@@ -126,8 +141,8 @@ class TestSingleDrive(ImageMirroringTestCase):
def test_pause(self):
self.assert_no_active_block_jobs()
-result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
- target=target_img)
+result = self.vm.qmp(self.qmp_cmd, device='drive0', sync='full',
+ target=self.qmp_target)
self.assert_qmp(result, 'return', {})
result = self.vm.qmp('block-job-pause', device='drive0')
@@ -153,8 +168,8 @@ class TestSingleDrive(ImageMirroringTestCase):
self.assert_no_active_block_jobs()
# A small buffer is rounded up automatically
-result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
- buf_size=4096, target=target_img)
+result = self.vm.qmp(self.qmp_cmd, device='drive0', sync='full',
+ buf_size=4096, target=self.qmp_target)
self.assert_qmp(result, 'return', {})
self.complete_and_wait()
@@ -169,8 +184,8 @@ class TestSingleDrive(ImageMirroringTestCase):
qemu_img('create', '-f', iotests.imgfmt, '-o', 'cluster_size=%d,size=%d'
% (self.image_len, self.image_len), target_img)
-result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
- buf_size=65536, mode='existing',
target=target_img)
+result = self.vm.qmp(self.qmp_cmd, device='drive0', sync='full',
+ buf_size=65536, mode='existing',
target=self.qmp_target)
self.assert_qmp(result, 'return', {})
self.complete_and_wait()
@@ -185,8 +200,8 @@ class TestSingleDrive(ImageMirroringTestCase):
qemu_img('create', '-f', iotests.imgfmt, '-o', 'cluster_size=%d,backing_file=%s'
% (self.image_len, backing_img), target_img)
-result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
- mode='existing', target=target_img)
+result = self.vm.qmp(self.qmp_cmd, device='drive0', sync='full',
+
Re: [Qemu-block] [PATCH v6 0/4] Clean unused entries in the qcow2 L2/refcount cache
Ping... https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg01929.html On Fri 05 Jun 2015 06:27:20 PM CEST, Alberto Garcia wrote: v6: - Update documentation to clarify what unused entries mean. v5: https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg00573.html - Fix build in mingw. - Use getpagesize() instead of sysconf(_SC_PAGESIZE). - Clarify that 0 is the default value for 'cache-clean-interval', and that it disables the feature. - Add the patch that documents how to configure the cache to this series, expanded with the explanation of 'cache-clean-interval'. (previous version: https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg02253.html) v4: https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg06120.html - Revert the 'cache-clean-interval' change. This should probably go into a new BlockDeviceInfoSpecific struct (along with other settings), but is out of the scope for this series. v3: https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg05473.html - Add 'cache-clean-interval' field to ImageInfoSpecificQCow2. v2: https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg05316.html - Clarify that the block-commit mentioned in the first patch refers to the HMP commit command. - Check the value of cache_clean_interval and cast it accordingly to prevent it from overflowing. v1: https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg03510.html Regards, Berto Alberto Garcia (4): qcow2: mark the memory as no longer needed after qcow2_cache_empty() qcow2: add option to clean unused cache entries after some time docs: document how to configure the qcow2 L2/refcount caches qcow2: reorder fields in Qcow2CachedTable to reduce padding block/qcow2-cache.c | 63 +++- block/qcow2.c| 64 block/qcow2.h| 4 ++ docs/qcow2-cache.txt | 164 +++ qapi/block-core.json | 7 ++- 5 files changed, 300 insertions(+), 2 deletions(-) create mode 100644 docs/qcow2-cache.txt -- 2.1.4
[Qemu-block] [PATCH 0/2] block: vpc - prevent overflow
This series fixes a bug found by Richard Jones. When we allocate the pagetable based on max_table_entries, we multiply the max table entry value by 4 to accomodate a table of 32-bit integers. However, max_table_entries is a uint32_t, and the VPC driver accepts ranges for that entry over 0x4000. So during this allocation: s-pagetable = qemu_try_blockalign(bs-file, s-max_table_entries * 4); The size arg overflows, allocating significantly less memory than expected. Since qemu_try_blockalign() size argument is size_t, cast the multiplication correctly to prevent overflow. The value of max_table_entries * 4 is used elsewhere in the code as well, so store the correct value for use in all those cases. Jeff Cody (2): block: vpc - prevent overflow if max_table_entries = 0x4000 block: qemu-iotests - add check for multiplication overflow in vpc block/vpc.c | 10 +++-- tests/qemu-iotests/135| 54 ++ tests/qemu-iotests/135.out| 5 +++ tests/qemu-iotests/group | 1 + tests/qemu-iotests/sample_images/afl5.img.bz2 | Bin 0 - 175 bytes 5 files changed, 66 insertions(+), 4 deletions(-) create mode 100755 tests/qemu-iotests/135 create mode 100644 tests/qemu-iotests/135.out create mode 100644 tests/qemu-iotests/sample_images/afl5.img.bz2 -- 1.9.3
[Qemu-block] [PATCH 1/2] block: vpc - prevent overflow if max_table_entries = 0x40000000
When we allocate the pagetable based on max_table_entries, we multiply
the max table entry value by 4 to accomodate a table of 32-bit integers.
However, max_table_entries is a uint32_t, and the VPC driver accepts
ranges for that entry over 0x4000. So during this allocation:
s-pagetable = qemu_try_blockalign(bs-file, s-max_table_entries * 4);
The size arg overflows, allocating significantly less memory than
expected.
Since qemu_try_blockalign() size argument is size_t, cast the
multiplication correctly to prevent overflow.
The value of max_table_entries * 4 is used elsewhere in the code as
well, so store the correct value for use in all those cases.
Reported-by: Richard W.M. Jones rjo...@redhat.com
Signed-off-by: Jeff Cody jc...@redhat.com
---
block/vpc.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/block/vpc.c b/block/vpc.c
index 37572ba..4108b5e 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -168,6 +168,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options,
int flags,
uint8_t buf[HEADER_SIZE];
uint32_t checksum;
uint64_t computed_size;
+uint64_t pagetable_size;
int disk_type = VHD_DYNAMIC;
int ret;
@@ -269,7 +270,9 @@ static int vpc_open(BlockDriverState *bs, QDict *options,
int flags,
goto fail;
}
-s-pagetable = qemu_try_blockalign(bs-file, s-max_table_entries * 4);
+pagetable_size = (size_t) s-max_table_entries * 4;
+
+s-pagetable = qemu_try_blockalign(bs-file, pagetable_size);
if (s-pagetable == NULL) {
ret = -ENOMEM;
goto fail;
@@ -277,14 +280,13 @@ static int vpc_open(BlockDriverState *bs, QDict *options,
int flags,
s-bat_offset = be64_to_cpu(dyndisk_header-table_offset);
-ret = bdrv_pread(bs-file, s-bat_offset, s-pagetable,
- s-max_table_entries * 4);
+ret = bdrv_pread(bs-file, s-bat_offset, s-pagetable,
pagetable_size);
if (ret 0) {
goto fail;
}
s-free_data_block_offset =
-(s-bat_offset + (s-max_table_entries * 4) + 511) ~511;
+(s-bat_offset + pagetable_size + 511) ~511;
for (i = 0; i s-max_table_entries; i++) {
be32_to_cpus(s-pagetable[i]);
--
1.9.3
[Qemu-block] [PATCH 2/2] block: qemu-iotests - add check for multiplication overflow in vpc
This checks that VPC is able to successfully fail (without segfault)
on an image file with a max_table_entries that exceeds 0x4000.
This table entry is within the valid range for VPC (although too large
for this sample image).
Signed-off-by: Jeff Cody jc...@redhat.com
---
tests/qemu-iotests/135| 54 ++
tests/qemu-iotests/135.out| 5 +++
tests/qemu-iotests/group | 1 +
tests/qemu-iotests/sample_images/afl5.img.bz2 | Bin 0 - 175 bytes
4 files changed, 60 insertions(+)
create mode 100755 tests/qemu-iotests/135
create mode 100644 tests/qemu-iotests/135.out
create mode 100644 tests/qemu-iotests/sample_images/afl5.img.bz2
diff --git a/tests/qemu-iotests/135 b/tests/qemu-iotests/135
new file mode 100755
index 000..16bf736
--- /dev/null
+++ b/tests/qemu-iotests/135
@@ -0,0 +1,54 @@
+#!/bin/bash
+#
+# Test VPC open of image with large Max Table Entries value.
+#
+# Copyright (C) 2015 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see http://www.gnu.org/licenses/.
+#
+
+# creator
+owner=jc...@redhat.com
+
+seq=`basename $0`
+echo QA output created by $seq
+
+here=`pwd`
+tmp=/tmp/$$
+status=1 # failure is the default!
+
+_cleanup()
+{
+_cleanup_test_img
+}
+trap _cleanup; exit \$status 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt vpc
+_supported_proto generic
+_supported_os Linux
+
+_use_sample_img afl5.img.bz2
+
+echo
+echo === Verify image open and failure
+$QEMU_IMG info $TEST_IMG 21| _filter_testdir
+
+# success, all done
+echo *** done
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/135.out b/tests/qemu-iotests/135.out
new file mode 100644
index 000..70b305e
--- /dev/null
+++ b/tests/qemu-iotests/135.out
@@ -0,0 +1,5 @@
+QA output created by 135
+
+=== Verify image open and failure
+qemu-img: Could not open 'TEST_DIR/afl5.img': block-vpc:
free_data_block_offset points after the end of file. The image has been
truncated.
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 4597fc1..557e2a2 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -132,3 +132,4 @@
130 rw auto quick
131 rw auto quick
134 rw auto quick
+135 rw auto
diff --git a/tests/qemu-iotests/sample_images/afl5.img.bz2
b/tests/qemu-iotests/sample_images/afl5.img.bz2
new file mode 100644
index
..1614348865e5b2cfcb0340eab9474841717be2c5
GIT binary patch
literal 175
zcmV;g08sxzT4*^jL0KkKSqT!KVgLXwfB*jgAVdfNFaTf(B!Frw|3pDR00;sy03ZSY
z3IG5B1Sp^YbSh$=r=c!nr#TgFeYj0XnKQ9RLxB?V^M@KMwpn4hJ!zOVwhn^RnWP
zlo2h)1BC$~JU|O6a~hBm5oG|a2~t!d6NaCTwor7gVwVQS9W$Kd2?dJH~Ej+J=Q^
dtom#MI_=bg;S5HeF^MqnF64@Ep$|^KE!naKsf*a
literal 0
HcmV?d1
--
1.9.3
