* [Qemu-devel] [PATCH v6 0/6] block: allow commit active as top
@ 2013-11-26 5:45 Fam Zheng
2013-11-26 5:45 ` [Qemu-devel] [PATCH v6 1/6] mirror: don't close target Fam Zheng
` (7 more replies)
0 siblings, 8 replies; 14+ messages in thread
From: Fam Zheng @ 2013-11-26 5:45 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, pbonzini, jcody, stefanha
Previously live commit of active block device is not supported, this series
implements it and updates corresponding qemu-iotests cases.
This series is based on BlockJobType enum QAPI series.
v6: Address comments from Stefan:
[04/06] commit: support commit active layer
Fix wording.
[05/06] qemu-iotests: update test cases for commit active
Drop is_active.
Experimental for reviewers: the side by side diff against previous series:
http://goo.gl/vgN6mc
v5: Address comments from Eric and Paolo:
Add mirror_start_job and front end wrapper. [Paolo]
Base on BlockJobType enum in QAPI. [Eric]
Drop "common" sync mode. [Eric]
v4: Rewrite to reuse block/mirror.c.
When committing the active layer, the job is internally a mirror job with
type name faked to "commit".
When the job completes, the BDSes are swapped, so the base image become
active and [top, base) dropped.
Fam Zheng (6):
mirror: don't close target
mirror: move base to MirrorBlockJob
block: add commit_active_start()
commit: support commit active layer
qemu-iotests: update test cases for commit active
commit: remove unused check
block/commit.c | 8 +----
block/mirror.c | 77 +++++++++++++++++++++++++++++++++++++++--------
blockdev.c | 9 ++++--
include/block/block_int.h | 22 ++++++++++++--
qapi-schema.json | 5 +--
tests/qemu-iotests/040 | 74 ++++++++++++++++++++-------------------------
6 files changed, 126 insertions(+), 69 deletions(-)
--
1.8.4.2
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH v6 1/6] mirror: don't close target
2013-11-26 5:45 [Qemu-devel] [PATCH v6 0/6] block: allow commit active as top Fam Zheng
@ 2013-11-26 5:45 ` Fam Zheng
2013-11-26 5:45 ` [Qemu-devel] [PATCH v6 2/6] mirror: move base to MirrorBlockJob Fam Zheng
` (6 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Fam Zheng @ 2013-11-26 5:45 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, pbonzini, jcody, stefanha
Let reference count manage target and don't call bdrv_close here.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block/mirror.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/block/mirror.c b/block/mirror.c
index 7b95acf..01a795a 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -479,7 +479,6 @@ immediate_exit:
}
bdrv_swap(s->target, s->common.bs);
}
- bdrv_close(s->target);
bdrv_unref(s->target);
block_job_completed(&s->common, ret);
}
--
1.8.4.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH v6 2/6] mirror: move base to MirrorBlockJob
2013-11-26 5:45 [Qemu-devel] [PATCH v6 0/6] block: allow commit active as top Fam Zheng
2013-11-26 5:45 ` [Qemu-devel] [PATCH v6 1/6] mirror: don't close target Fam Zheng
@ 2013-11-26 5:45 ` Fam Zheng
2013-11-26 5:45 ` [Qemu-devel] [PATCH v6 3/6] block: add commit_active_start() Fam Zheng
` (5 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Fam Zheng @ 2013-11-26 5:45 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, pbonzini, jcody, stefanha
This allows setting the base before entering mirror_run, commit will
make use of it.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block/mirror.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index 01a795a..cbdcc21 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -31,6 +31,7 @@ typedef struct MirrorBlockJob {
BlockJob common;
RateLimit limit;
BlockDriverState *target;
+ BlockDriverState *base;
MirrorSyncMode mode;
BlockdevOnError on_source_error, on_target_error;
bool synced;
@@ -334,8 +335,7 @@ static void coroutine_fn mirror_run(void *opaque)
if (s->mode != MIRROR_SYNC_MODE_NONE) {
/* First part, loop on the sectors and initialize the dirty bitmap. */
- BlockDriverState *base;
- base = s->mode == MIRROR_SYNC_MODE_FULL ? NULL : bs->backing_hd;
+ BlockDriverState *base = s->base;
for (sector_num = 0; sector_num < end; ) {
int64_t next = (sector_num | (sectors_per_chunk - 1)) + 1;
ret = bdrv_is_allocated_above(bs, base,
@@ -540,6 +540,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
void *opaque, Error **errp)
{
MirrorBlockJob *s;
+ BlockDriverState *base = NULL;
if (granularity == 0) {
/* Choose the default granularity based on the target file's cluster
@@ -562,6 +563,12 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
return;
}
+ if (mode == MIRROR_SYNC_MODE_TOP) {
+ base = bs->backing_hd;
+ } else {
+ base = NULL;
+ }
+
s = block_job_create(&mirror_job_driver, bs, speed, cb, opaque, errp);
if (!s) {
return;
@@ -571,6 +578,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
s->on_target_error = on_target_error;
s->target = target;
s->mode = mode;
+ s->base = base;
s->granularity = granularity;
s->buf_size = MAX(buf_size, granularity);
--
1.8.4.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH v6 3/6] block: add commit_active_start()
2013-11-26 5:45 [Qemu-devel] [PATCH v6 0/6] block: allow commit active as top Fam Zheng
2013-11-26 5:45 ` [Qemu-devel] [PATCH v6 1/6] mirror: don't close target Fam Zheng
2013-11-26 5:45 ` [Qemu-devel] [PATCH v6 2/6] mirror: move base to MirrorBlockJob Fam Zheng
@ 2013-11-26 5:45 ` Fam Zheng
2013-11-26 5:45 ` [Qemu-devel] [PATCH v6 4/6] commit: support commit active layer Fam Zheng
` (4 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Fam Zheng @ 2013-11-26 5:45 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, pbonzini, jcody, stefanha
commit_active_start is implemented in block/mirror.c, It will create a
job with "commit" type and designated base in block-commit command. This
will be used for committing active layer of device.
Sync mode is removed from MirrorBlockJob because there's no proper type
for commit. The used information is is_none_mode.
The common part of mirror_start and commit_active_start is moved to
mirror_start_job().
Fix the comment wording for commit_start.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block/mirror.c | 65 +++++++++++++++++++++++++++++++++++------------
include/block/block_int.h | 22 +++++++++++++---
2 files changed, 68 insertions(+), 19 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index cbdcc21..9be741a 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -32,7 +32,7 @@ typedef struct MirrorBlockJob {
RateLimit limit;
BlockDriverState *target;
BlockDriverState *base;
- MirrorSyncMode mode;
+ bool is_none_mode;
BlockdevOnError on_source_error, on_target_error;
bool synced;
bool should_complete;
@@ -333,7 +333,7 @@ static void coroutine_fn mirror_run(void *opaque)
sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
mirror_free_init(s);
- if (s->mode != MIRROR_SYNC_MODE_NONE) {
+ if (!s->is_none_mode) {
/* First part, loop on the sectors and initialize the dirty bitmap. */
BlockDriverState *base = s->base;
for (sector_num = 0; sector_num < end; ) {
@@ -532,15 +532,25 @@ static const BlockJobDriver mirror_job_driver = {
.complete = mirror_complete,
};
-void mirror_start(BlockDriverState *bs, BlockDriverState *target,
- int64_t speed, int64_t granularity, int64_t buf_size,
- MirrorSyncMode mode, BlockdevOnError on_source_error,
- BlockdevOnError on_target_error,
- BlockDriverCompletionFunc *cb,
- void *opaque, Error **errp)
+static const BlockJobDriver commit_active_job_driver = {
+ .instance_size = sizeof(MirrorBlockJob),
+ .job_type = BLOCK_JOB_TYPE_COMMIT,
+ .set_speed = mirror_set_speed,
+ .iostatus_reset= mirror_iostatus_reset,
+ .complete = mirror_complete,
+};
+
+static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
+ int64_t speed, int64_t granularity,
+ int64_t buf_size,
+ BlockdevOnError on_source_error,
+ BlockdevOnError on_target_error,
+ BlockDriverCompletionFunc *cb,
+ void *opaque, Error **errp,
+ const BlockJobDriver *driver,
+ bool is_none_mode, BlockDriverState *base)
{
MirrorBlockJob *s;
- BlockDriverState *base = NULL;
if (granularity == 0) {
/* Choose the default granularity based on the target file's cluster
@@ -563,13 +573,8 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
return;
}
- if (mode == MIRROR_SYNC_MODE_TOP) {
- base = bs->backing_hd;
- } else {
- base = NULL;
- }
- s = block_job_create(&mirror_job_driver, bs, speed, cb, opaque, errp);
+ s = block_job_create(driver, bs, speed, cb, opaque, errp);
if (!s) {
return;
}
@@ -577,7 +582,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
s->on_source_error = on_source_error;
s->on_target_error = on_target_error;
s->target = target;
- s->mode = mode;
+ s->is_none_mode = is_none_mode;
s->base = base;
s->granularity = granularity;
s->buf_size = MAX(buf_size, granularity);
@@ -590,3 +595,31 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
trace_mirror_start(bs, s, s->common.co, opaque);
qemu_coroutine_enter(s->common.co, s);
}
+
+void mirror_start(BlockDriverState *bs, BlockDriverState *target,
+ int64_t speed, int64_t granularity, int64_t buf_size,
+ MirrorSyncMode mode, BlockdevOnError on_source_error,
+ BlockdevOnError on_target_error,
+ BlockDriverCompletionFunc *cb,
+ void *opaque, Error **errp)
+{
+ bool is_none_mode;
+ BlockDriverState *base;
+
+ is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
+ base = mode == MIRROR_SYNC_MODE_TOP ? bs->backing_hd : NULL;
+ mirror_start_job(bs, target, speed, granularity, buf_size,
+ on_source_error, on_target_error, cb, opaque, errp,
+ &mirror_job_driver, is_none_mode, base);
+}
+
+void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
+ int64_t speed,
+ BlockdevOnError on_error,
+ BlockDriverCompletionFunc *cb,
+ void *opaque, Error **errp)
+{
+ mirror_start_job(bs, base, speed, 0, 0,
+ on_error, on_error, cb, opaque, errp,
+ &commit_active_job_driver, false, base);
+}
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 1666066..eeedf20 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -369,8 +369,9 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base,
/**
* commit_start:
- * @bs: Top Block device
- * @base: Block device that will be written into, and become the new top
+ * @bs: Active block device.
+ * @top: Top block device to be committed.
+ * @base: Block device that will be written into, and become the new top.
* @speed: The maximum speed, in bytes per second, or 0 for unlimited.
* @on_error: The action to take upon error.
* @cb: Completion function for the job.
@@ -382,7 +383,22 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
BlockDriverState *top, int64_t speed,
BlockdevOnError on_error, BlockDriverCompletionFunc *cb,
void *opaque, Error **errp);
-
+/**
+ * commit_active_start:
+ * @bs: Active block device to be committed.
+ * @base: Block device that will be written into, and become the new top.
+ * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
+ * @on_error: The action to take upon error.
+ * @cb: Completion function for the job.
+ * @opaque: Opaque pointer value passed to @cb.
+ * @errp: Error object.
+ *
+ */
+void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
+ int64_t speed,
+ BlockdevOnError on_error,
+ BlockDriverCompletionFunc *cb,
+ void *opaque, Error **errp);
/*
* mirror_start:
* @bs: Block device to operate on.
--
1.8.4.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH v6 4/6] commit: support commit active layer
2013-11-26 5:45 [Qemu-devel] [PATCH v6 0/6] block: allow commit active as top Fam Zheng
` (2 preceding siblings ...)
2013-11-26 5:45 ` [Qemu-devel] [PATCH v6 3/6] block: add commit_active_start() Fam Zheng
@ 2013-11-26 5:45 ` Fam Zheng
2013-12-13 18:26 ` Kevin Wolf
2013-12-14 3:10 ` Eric Blake
2013-11-26 5:45 ` [Qemu-devel] [PATCH v6 5/6] qemu-iotests: update test cases for commit active Fam Zheng
` (3 subsequent siblings)
7 siblings, 2 replies; 14+ messages in thread
From: Fam Zheng @ 2013-11-26 5:45 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, pbonzini, jcody, stefanha
If active is top, it will be mirrored to base, (with block/mirror.c
code), then the image is switched when user completes the block job.
QMP documentation is updated.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block/mirror.c | 11 +++++++++++
blockdev.c | 9 +++++++--
qapi-schema.json | 5 +++--
3 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index 9be741a..86ffac8 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -478,6 +478,13 @@ immediate_exit:
bdrv_reopen(s->target, bdrv_get_flags(s->common.bs), NULL);
}
bdrv_swap(s->target, s->common.bs);
+ if (s->common.driver->job_type == BLOCK_JOB_TYPE_COMMIT) {
+ /* drop the bs loop chain formed by the swap: break the loop then
+ * trigger the unref from the top one */
+ BlockDriverState *p = s->base->backing_hd;
+ s->base->backing_hd = NULL;
+ bdrv_unref(p);
+ }
}
bdrv_unref(s->target);
block_job_completed(&s->common, ret);
@@ -619,6 +626,10 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
BlockDriverCompletionFunc *cb,
void *opaque, Error **errp)
{
+ if (bdrv_reopen(base, bs->open_flags, errp)) {
+ return;
+ }
+ bdrv_ref(base);
mirror_start_job(bs, base, speed, 0, 0,
on_error, on_error, cb, opaque, errp,
&commit_active_job_driver, false, base);
diff --git a/blockdev.c b/blockdev.c
index 330aa4a..d6b9005 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1820,8 +1820,13 @@ void qmp_block_commit(const char *device,
return;
}
- commit_start(bs, base_bs, top_bs, speed, on_error, block_job_cb, bs,
- &local_err);
+ if (top_bs == bs) {
+ commit_active_start(bs, base_bs, speed, on_error, block_job_cb,
+ bs, &local_err);
+ } else {
+ commit_start(bs, base_bs, top_bs, speed, on_error, block_job_cb, bs,
+ &local_err);
+ }
if (local_err != NULL) {
error_propagate(errp, local_err);
return;
diff --git a/qapi-schema.json b/qapi-schema.json
index 83fa485..02817d0 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1967,9 +1967,11 @@
#
# @top: The file name of the backing image within the image chain,
# which contains the topmost data to be committed down.
-# Note, the active layer as 'top' is currently unsupported.
#
# If top == base, that is an error.
+# If top == active, the job will not be completed by itself,
+# user needs to complete the job with the block-job-complete
+# command after getting the ready event. (Since 1.8)
#
#
# @speed: #optional the maximum speed, in bytes per second
@@ -1979,7 +1981,6 @@
# If @device does not exist, DeviceNotFound
# If image commit is not supported by this device, NotSupported
# If @base or @top is invalid, a generic error is returned
-# If @top is the active layer, or omitted, a generic error is returned
# If @speed is invalid, InvalidParameter
#
# Since: 1.3
--
1.8.4.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH v6 5/6] qemu-iotests: update test cases for commit active
2013-11-26 5:45 [Qemu-devel] [PATCH v6 0/6] block: allow commit active as top Fam Zheng
` (3 preceding siblings ...)
2013-11-26 5:45 ` [Qemu-devel] [PATCH v6 4/6] commit: support commit active layer Fam Zheng
@ 2013-11-26 5:45 ` Fam Zheng
2013-11-26 5:45 ` [Qemu-devel] [PATCH v6 6/6] commit: remove unused check Fam Zheng
` (2 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Fam Zheng @ 2013-11-26 5:45 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, pbonzini, jcody, stefanha
Factor out commit test common logic into super class, and update test
of committing the active image.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
tests/qemu-iotests/040 | 74 ++++++++++++++++++++++----------------------------
1 file changed, 32 insertions(+), 42 deletions(-)
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index a2e18c5..6d690ce 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -54,6 +54,29 @@ class ImageCommitTestCase(iotests.QMPTestCase):
self.assert_no_active_commit()
+ def run_commit_test(self, top, base):
+ self.assert_no_active_commit()
+ result = self.vm.qmp('block-commit', device='drive0', top=top, base=base)
+ self.assert_qmp(result, 'return', {})
+
+ completed = False
+ while not completed:
+ for event in self.vm.get_qmp_events(wait=True):
+ if event['event'] == 'BLOCK_JOB_COMPLETED':
+ self.assert_qmp(event, 'data/type', 'commit')
+ self.assert_qmp(event, 'data/device', 'drive0')
+ self.assert_qmp(event, 'data/offset', self.image_len)
+ self.assert_qmp(event, 'data/len', self.image_len)
+ completed = True
+ elif event['event'] == 'BLOCK_JOB_READY':
+ self.assert_qmp(event, 'data/type', 'commit')
+ self.assert_qmp(event, 'data/device', 'drive0')
+ self.assert_qmp(event, 'data/len', self.image_len)
+ self.vm.qmp('block-job-complete', device='drive0')
+
+ self.assert_no_active_commit()
+ self.vm.shutdown()
+
class TestSingleDrive(ImageCommitTestCase):
image_len = 1 * 1024 * 1024
test_len = 1 * 1024 * 256
@@ -74,23 +97,7 @@ class TestSingleDrive(ImageCommitTestCase):
os.remove(backing_img)
def test_commit(self):
- self.assert_no_active_commit()
- result = self.vm.qmp('block-commit', device='drive0', top='%s' % mid_img)
- self.assert_qmp(result, 'return', {})
-
- completed = False
- while not completed:
- for event in self.vm.get_qmp_events(wait=True):
- if event['event'] == 'BLOCK_JOB_COMPLETED':
- self.assert_qmp(event, 'data/type', 'commit')
- self.assert_qmp(event, 'data/device', 'drive0')
- self.assert_qmp(event, 'data/offset', self.image_len)
- self.assert_qmp(event, 'data/len', self.image_len)
- completed = True
-
- self.assert_no_active_commit()
- self.vm.shutdown()
-
+ self.run_commit_test(mid_img, backing_img)
self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', backing_img).find("verification failed"))
self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed"))
@@ -117,10 +124,9 @@ class TestSingleDrive(ImageCommitTestCase):
self.assert_qmp(result, 'error/desc', 'Base \'badfile\' not found')
def test_top_is_active(self):
- self.assert_no_active_commit()
- result = self.vm.qmp('block-commit', device='drive0', top='%s' % test_img, base='%s' % backing_img)
- self.assert_qmp(result, 'error/class', 'GenericError')
- self.assert_qmp(result, 'error/desc', 'Top image as the active layer is currently unsupported')
+ self.run_commit_test(test_img, backing_img)
+ self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', backing_img).find("verification failed"))
+ self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed"))
def test_top_and_base_reversed(self):
self.assert_no_active_commit()
@@ -181,23 +187,7 @@ class TestRelativePaths(ImageCommitTestCase):
raise
def test_commit(self):
- self.assert_no_active_commit()
- result = self.vm.qmp('block-commit', device='drive0', top='%s' % self.mid_img)
- self.assert_qmp(result, 'return', {})
-
- completed = False
- while not completed:
- for event in self.vm.get_qmp_events(wait=True):
- if event['event'] == 'BLOCK_JOB_COMPLETED':
- self.assert_qmp(event, 'data/type', 'commit')
- self.assert_qmp(event, 'data/device', 'drive0')
- self.assert_qmp(event, 'data/offset', self.image_len)
- self.assert_qmp(event, 'data/len', self.image_len)
- completed = True
-
- self.assert_no_active_commit()
- self.vm.shutdown()
-
+ self.run_commit_test(self.mid_img, self.backing_img)
self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', self.backing_img_abs).find("verification failed"))
self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', self.backing_img_abs).find("verification failed"))
@@ -224,10 +214,9 @@ class TestRelativePaths(ImageCommitTestCase):
self.assert_qmp(result, 'error/desc', 'Base \'badfile\' not found')
def test_top_is_active(self):
- self.assert_no_active_commit()
- result = self.vm.qmp('block-commit', device='drive0', top='%s' % self.test_img, base='%s' % self.backing_img)
- self.assert_qmp(result, 'error/class', 'GenericError')
- self.assert_qmp(result, 'error/desc', 'Top image as the active layer is currently unsupported')
+ self.run_commit_test(self.test_img, self.backing_img)
+ self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', self.backing_img_abs).find("verification failed"))
+ self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', self.backing_img_abs).find("verification failed"))
def test_top_and_base_reversed(self):
self.assert_no_active_commit()
@@ -243,6 +232,7 @@ class TestSetSpeed(ImageCommitTestCase):
qemu_img('create', backing_img, str(TestSetSpeed.image_len))
qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, mid_img)
qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % mid_img, test_img)
+ qemu_io('-c', 'write -P 0xef 524288 524288', mid_img)
self.vm = iotests.VM().add_drive(test_img)
self.vm.launch()
--
1.8.4.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH v6 6/6] commit: remove unused check
2013-11-26 5:45 [Qemu-devel] [PATCH v6 0/6] block: allow commit active as top Fam Zheng
` (4 preceding siblings ...)
2013-11-26 5:45 ` [Qemu-devel] [PATCH v6 5/6] qemu-iotests: update test cases for commit active Fam Zheng
@ 2013-11-26 5:45 ` Fam Zheng
2013-12-11 2:49 ` [Qemu-devel] [PATCH v6 0/6] block: allow commit active as top Fam Zheng
2013-12-13 18:28 ` Kevin Wolf
7 siblings, 0 replies; 14+ messages in thread
From: Fam Zheng @ 2013-11-26 5:45 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, pbonzini, jcody, stefanha
We support top == active for commit now, remove the check and add an
assertion here.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block/commit.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/block/commit.c b/block/commit.c
index d4090cb..acec4ac 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -198,13 +198,7 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
return;
}
- /* Once we support top == active layer, remove this check */
- if (top == bs) {
- error_setg(errp,
- "Top image as the active layer is currently unsupported");
- return;
- }
-
+ assert(top != bs);
if (top == base) {
error_setg(errp, "Invalid files for merge: top and base are the same");
return;
--
1.8.4.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v6 0/6] block: allow commit active as top
2013-11-26 5:45 [Qemu-devel] [PATCH v6 0/6] block: allow commit active as top Fam Zheng
` (5 preceding siblings ...)
2013-11-26 5:45 ` [Qemu-devel] [PATCH v6 6/6] commit: remove unused check Fam Zheng
@ 2013-12-11 2:49 ` Fam Zheng
2013-12-13 18:28 ` Kevin Wolf
7 siblings, 0 replies; 14+ messages in thread
From: Fam Zheng @ 2013-12-11 2:49 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, pbonzini, jcody, stefanha
On 2013年11月26日 13:45, Fam Zheng wrote:
> Previously live commit of active block device is not supported, this series
> implements it and updates corresponding qemu-iotests cases.
>
> This series is based on BlockJobType enum QAPI series.
>
> v6: Address comments from Stefan:
>
> [04/06] commit: support commit active layer
> Fix wording.
> [05/06] qemu-iotests: update test cases for commit active
> Drop is_active.
>
> Experimental for reviewers: the side by side diff against previous series:
>
> http://goo.gl/vgN6mc
>
Stefan, Kevin,
Are you happy with this version?
Fam
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v6 4/6] commit: support commit active layer
2013-11-26 5:45 ` [Qemu-devel] [PATCH v6 4/6] commit: support commit active layer Fam Zheng
@ 2013-12-13 18:26 ` Kevin Wolf
2013-12-16 6:25 ` Fam Zheng
2013-12-14 3:10 ` Eric Blake
1 sibling, 1 reply; 14+ messages in thread
From: Kevin Wolf @ 2013-12-13 18:26 UTC (permalink / raw)
To: Fam Zheng; +Cc: pbonzini, jcody, qemu-devel, stefanha
Am 26.11.2013 um 06:45 hat Fam Zheng geschrieben:
> If active is top, it will be mirrored to base, (with block/mirror.c
> code), then the image is switched when user completes the block job.
>
> QMP documentation is updated.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> block/mirror.c | 11 +++++++++++
> blockdev.c | 9 +++++++--
> qapi-schema.json | 5 +++--
> 3 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index 9be741a..86ffac8 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -478,6 +478,13 @@ immediate_exit:
> bdrv_reopen(s->target, bdrv_get_flags(s->common.bs), NULL);
> }
> bdrv_swap(s->target, s->common.bs);
> + if (s->common.driver->job_type == BLOCK_JOB_TYPE_COMMIT) {
> + /* drop the bs loop chain formed by the swap: break the loop then
> + * trigger the unref from the top one */
> + BlockDriverState *p = s->base->backing_hd;
> + s->base->backing_hd = NULL;
> + bdrv_unref(p);
I'm not sure about the refcounts...
Let's assume we have the following backing file chain, refcount in
brackets:
+------ NBD server
v
base (1) <-- sn1 (1) <-- sn2 (3) <-- guest
^
+------ something else ;-)
> + }
> }
> bdrv_unref(s->target);
> block_job_completed(&s->common, ret);
> @@ -619,6 +626,10 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
> BlockDriverCompletionFunc *cb,
> void *opaque, Error **errp)
> {
> + if (bdrv_reopen(base, bs->open_flags, errp)) {
> + return;
> + }
> + bdrv_ref(base);
So when we start, the refcount changes as follows:
+--- commit job +------ NBD server
v v
base (2) <-- sn1 (1) <-- sn2 (3) <-- guest
^
+------ something else
Once all the data is copied over, we get o the code above, and first do
a bdrv_swap, which results in the following:
+--- commit job +------ NBD server
v v
sn2 (2) <-- sn1 (1) base (3) <-- guest
| ^ ^
+------------+ +------ something else
Break the loop (but no refcount update!):
+--- commit job +------ NBD server
v v
sn2 (2) <-- sn1 (1) base (3) <-- guest
^
+------ something else
Drop the commit job's reference:
+------ NBD server
v
sn2 (1) <-- sn1 (1) base (3) <-- guest
^
+------ something else
Okay, so writing this down made me actually understand what's wrong with
the refcount logic. You can't set s->base->backing_hd = NULL without
updating the reference count of the backing file.
Easy enough to fix. :-)
Kevin
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v6 0/6] block: allow commit active as top
2013-11-26 5:45 [Qemu-devel] [PATCH v6 0/6] block: allow commit active as top Fam Zheng
` (6 preceding siblings ...)
2013-12-11 2:49 ` [Qemu-devel] [PATCH v6 0/6] block: allow commit active as top Fam Zheng
@ 2013-12-13 18:28 ` Kevin Wolf
7 siblings, 0 replies; 14+ messages in thread
From: Kevin Wolf @ 2013-12-13 18:28 UTC (permalink / raw)
To: Fam Zheng; +Cc: pbonzini, jcody, qemu-devel, stefanha
Am 26.11.2013 um 06:45 hat Fam Zheng geschrieben:
> Previously live commit of active block device is not supported, this series
> implements it and updates corresponding qemu-iotests cases.
>
> This series is based on BlockJobType enum QAPI series.
>
> v6: Address comments from Stefan:
>
> [04/06] commit: support commit active layer
> Fix wording.
> [05/06] qemu-iotests: update test cases for commit active
> Drop is_active.
>
> Experimental for reviewers: the side by side diff against previous series:
>
> http://goo.gl/vgN6mc
>
> v5: Address comments from Eric and Paolo:
> Add mirror_start_job and front end wrapper. [Paolo]
> Base on BlockJobType enum in QAPI. [Eric]
> Drop "common" sync mode. [Eric]
>
> v4: Rewrite to reuse block/mirror.c.
> When committing the active layer, the job is internally a mirror job with
> type name faked to "commit".
> When the job completes, the BDSes are swapped, so the base image become
> active and [top, base) dropped.
>
> Fam Zheng (6):
> mirror: don't close target
> mirror: move base to MirrorBlockJob
> block: add commit_active_start()
> commit: support commit active layer
> qemu-iotests: update test cases for commit active
> commit: remove unused check
>
> block/commit.c | 8 +----
> block/mirror.c | 77 +++++++++++++++++++++++++++++++++++++++--------
> blockdev.c | 9 ++++--
> include/block/block_int.h | 22 ++++++++++++--
> qapi-schema.json | 5 +--
> tests/qemu-iotests/040 | 74 ++++++++++++++++++++-------------------------
> 6 files changed, 126 insertions(+), 69 deletions(-)
Patches 1-3 and 6 are: Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Patch 4 needs a fix, patch 5 didn't apply any more and needs a rebase.
Kevin
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v6 4/6] commit: support commit active layer
2013-11-26 5:45 ` [Qemu-devel] [PATCH v6 4/6] commit: support commit active layer Fam Zheng
2013-12-13 18:26 ` Kevin Wolf
@ 2013-12-14 3:10 ` Eric Blake
2013-12-16 6:28 ` Fam Zheng
1 sibling, 1 reply; 14+ messages in thread
From: Eric Blake @ 2013-12-14 3:10 UTC (permalink / raw)
To: Fam Zheng, qemu-devel; +Cc: kwolf, pbonzini, jcody, stefanha
[-- Attachment #1: Type: text/plain, Size: 1299 bytes --]
On 11/25/2013 10:45 PM, Fam Zheng wrote:
> If active is top, it will be mirrored to base, (with block/mirror.c
> code), then the image is switched when user completes the block job.
>
> QMP documentation is updated.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> +++ b/qapi-schema.json
> @@ -1967,9 +1967,11 @@
> #
> # @top: The file name of the backing image within the image chain,
> # which contains the topmost data to be committed down.
> -# Note, the active layer as 'top' is currently unsupported.
> #
> # If top == base, that is an error.
> +# If top == active, the job will not be completed by itself,
> +# user needs to complete the job with the block-job-complete
> +# command after getting the ready event. (Since 1.8)
s/1.8/2.0/
What happens if the user does block-job-cancel instead of
block-job-complete? With drive-mirror, that leaves the mirrored drive
as a nice copy at the point in time of the cancel. I guess with commit,
a cancel just aborts the commit and does not change which image is top.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v6 4/6] commit: support commit active layer
2013-12-13 18:26 ` Kevin Wolf
@ 2013-12-16 6:25 ` Fam Zheng
0 siblings, 0 replies; 14+ messages in thread
From: Fam Zheng @ 2013-12-16 6:25 UTC (permalink / raw)
To: Kevin Wolf; +Cc: pbonzini, jcody, qemu-devel, stefanha
On 2013年12月14日 02:26, Kevin Wolf wrote:
> Am 26.11.2013 um 06:45 hat Fam Zheng geschrieben:
>> If active is top, it will be mirrored to base, (with block/mirror.c
>> code), then the image is switched when user completes the block job.
>>
>> QMP documentation is updated.
>>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> ---
>> block/mirror.c | 11 +++++++++++
>> blockdev.c | 9 +++++++--
>> qapi-schema.json | 5 +++--
>> 3 files changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index 9be741a..86ffac8 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -478,6 +478,13 @@ immediate_exit:
>> bdrv_reopen(s->target, bdrv_get_flags(s->common.bs), NULL);
>> }
>> bdrv_swap(s->target, s->common.bs);
>> + if (s->common.driver->job_type == BLOCK_JOB_TYPE_COMMIT) {
>> + /* drop the bs loop chain formed by the swap: break the loop then
>> + * trigger the unref from the top one */
>> + BlockDriverState *p = s->base->backing_hd;
>> + s->base->backing_hd = NULL;
>> + bdrv_unref(p);
Dropping "p" here (points to sn1 in your example) recursively results in ...
>
> I'm not sure about the refcounts...
>
> Let's assume we have the following backing file chain, refcount in
> brackets:
>
> +------ NBD server
> v
> base (1) <-- sn1 (1) <-- sn2 (3) <-- guest
> ^
> +------ something else ;-)
>
>> + }
>> }
>> bdrv_unref(s->target);
>> block_job_completed(&s->common, ret);
>> @@ -619,6 +626,10 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
>> BlockDriverCompletionFunc *cb,
>> void *opaque, Error **errp)
>> {
>> + if (bdrv_reopen(base, bs->open_flags, errp)) {
>> + return;
>> + }
>> + bdrv_ref(base);
>
> So when we start, the refcount changes as follows:
>
> +--- commit job +------ NBD server
> v v
> base (2) <-- sn1 (1) <-- sn2 (3) <-- guest
> ^
> +------ something else
>
>
> Once all the data is copied over, we get o the code above, and first do
> a bdrv_swap, which results in the following:
>
> +--- commit job +------ NBD server
> v v
> sn2 (2) <-- sn1 (1) base (3) <-- guest
> | ^ ^
> +------------+ +------ something else
>
>
> Break the loop (but no refcount update!):
>
> +--- commit job +------ NBD server
> v v
> sn2 (2) <-- sn1 (1) base (3) <-- guest
> ^
> +------ something else
>
... sn2(1) left alone. It is also reference by s->target and is
bdrv_unref'ed right before block_job_completed(). So...
>
> Drop the commit job's reference:
>
> +------ NBD server
> v
> sn2 (1) <-- sn1 (1) base (3) <-- guest
> ^
> +------ something else
>
you mean sn2 (1) <-- sn1 (1) is leaked but I double checked it is not.
(Did you overlooked the bdrv_unref(p)?)
Fam
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v6 4/6] commit: support commit active layer
2013-12-14 3:10 ` Eric Blake
@ 2013-12-16 6:28 ` Fam Zheng
2013-12-16 6:32 ` Fam Zheng
0 siblings, 1 reply; 14+ messages in thread
From: Fam Zheng @ 2013-12-16 6:28 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, pbonzini, jcody, stefanha
On 2013年12月14日 11:10, Eric Blake wrote:
> On 11/25/2013 10:45 PM, Fam Zheng wrote:
>> If active is top, it will be mirrored to base, (with block/mirror.c
>> code), then the image is switched when user completes the block job.
>>
>> QMP documentation is updated.
>>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> ---
>
>> +++ b/qapi-schema.json
>> @@ -1967,9 +1967,11 @@
>> #
>> # @top: The file name of the backing image within the image chain,
>> # which contains the topmost data to be committed down.
>> -# Note, the active layer as 'top' is currently unsupported.
>> #
>> # If top == base, that is an error.
>> +# If top == active, the job will not be completed by itself,
>> +# user needs to complete the job with the block-job-complete
>> +# command after getting the ready event. (Since 1.8)
>
> s/1.8/2.0/
OK, thanks.
Kevin, if you agree with my explanation on your comment, would you fix
this while applying?
>
> What happens if the user does block-job-cancel instead of
> block-job-complete? With drive-mirror, that leaves the mirrored drive
> as a nice copy at the point in time of the cancel. I guess with commit,
> a cancel just aborts the commit and does not change which image is top.
>
True.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v6 4/6] commit: support commit active layer
2013-12-16 6:28 ` Fam Zheng
@ 2013-12-16 6:32 ` Fam Zheng
0 siblings, 0 replies; 14+ messages in thread
From: Fam Zheng @ 2013-12-16 6:32 UTC (permalink / raw)
To: qemu-devel, kwolf; +Cc: pbonzini, jcody, stefanha
On 2013年12月16日 14:28, Fam Zheng wrote:
> On 2013年12月14日 11:10, Eric Blake wrote:
>> On 11/25/2013 10:45 PM, Fam Zheng wrote:
>>> If active is top, it will be mirrored to base, (with block/mirror.c
>>> code), then the image is switched when user completes the block job.
>>>
>>> QMP documentation is updated.
>>>
>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>> ---
>>
>>> +++ b/qapi-schema.json
>>> @@ -1967,9 +1967,11 @@
>>> #
>>> # @top: The file name of the backing image within the
>>> image chain,
>>> # which contains the topmost data to be
>>> committed down.
>>> -# Note, the active layer as 'top' is currently
>>> unsupported.
>>> #
>>> # If top == base, that is an error.
>>> +# If top == active, the job will not be completed
>>> by itself,
>>> +# user needs to complete the job with the
>>> block-job-complete
>>> +# command after getting the ready event. (Since 1.8)
>>
>> s/1.8/2.0/
>
> OK, thanks.
>
> Kevin, if you agree with my explanation on your comment, would you fix
> this while applying?
>
Sorry, I forgot I should rebase 05. Never mind, I'll respin.
Fam
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2013-12-16 6:32 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-26 5:45 [Qemu-devel] [PATCH v6 0/6] block: allow commit active as top Fam Zheng
2013-11-26 5:45 ` [Qemu-devel] [PATCH v6 1/6] mirror: don't close target Fam Zheng
2013-11-26 5:45 ` [Qemu-devel] [PATCH v6 2/6] mirror: move base to MirrorBlockJob Fam Zheng
2013-11-26 5:45 ` [Qemu-devel] [PATCH v6 3/6] block: add commit_active_start() Fam Zheng
2013-11-26 5:45 ` [Qemu-devel] [PATCH v6 4/6] commit: support commit active layer Fam Zheng
2013-12-13 18:26 ` Kevin Wolf
2013-12-16 6:25 ` Fam Zheng
2013-12-14 3:10 ` Eric Blake
2013-12-16 6:28 ` Fam Zheng
2013-12-16 6:32 ` Fam Zheng
2013-11-26 5:45 ` [Qemu-devel] [PATCH v6 5/6] qemu-iotests: update test cases for commit active Fam Zheng
2013-11-26 5:45 ` [Qemu-devel] [PATCH v6 6/6] commit: remove unused check Fam Zheng
2013-12-11 2:49 ` [Qemu-devel] [PATCH v6 0/6] block: allow commit active as top Fam Zheng
2013-12-13 18:28 ` Kevin Wolf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.