| Message ID | 1380542578-2387-3-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 09/30/2013 06:02 AM, Fam Zheng wrote: > This adds a new sync mode "common" which only copies data that is above > the common ancestor of source and target. In general, this could be useful > in cases like: > > base_bs ---> common_ancestor ---> foo ---> bar --->source > \ > \---> target > > Where data in foo, bar and source will be copied to target, once such > common backing_hd sharing is introduced. For now, we could use a special > case: If target is the ancestor of source, like, > > base_bs ---> target ---> foo ---> bar --->source > > The data in foo, bar and source will be copied to target, like > drive-commit, and when they are synced, the source bs replaces target > bs. This is specifically useful for block commit of active layer. > > This mode is not available (-ENOTSUP) from QMP interface, it is only > used internally by block commit code. > > +++ b/qapi-schema.json > @@ -1363,7 +1363,7 @@ > # Since: 1.3 > ## > { 'enum': 'MirrorSyncMode', > - 'data': ['top', 'full', 'none'] } > + 'data': ['top', 'full', 'none', 'common'] } Is it worth documenting the mode, in order to include a '(since 1.7)' notation, as well as a mention that this mode is not supported via QMP but only exists so that the code generator will support the mode needed internally? Is there any way to refactor things so that you don't have to munge the QAPI just to provide this internal-only mode?
On Mon, 09/30 08:49, Eric Blake wrote: > On 09/30/2013 06:02 AM, Fam Zheng wrote: > > This adds a new sync mode "common" which only copies data that is above > > the common ancestor of source and target. In general, this could be useful > > in cases like: > > > > base_bs ---> common_ancestor ---> foo ---> bar --->source > > \ > > \---> target > > > > Where data in foo, bar and source will be copied to target, once such > > common backing_hd sharing is introduced. For now, we could use a special > > case: If target is the ancestor of source, like, > > > > base_bs ---> target ---> foo ---> bar --->source > > > > The data in foo, bar and source will be copied to target, like > > drive-commit, and when they are synced, the source bs replaces target > > bs. This is specifically useful for block commit of active layer. > > > > This mode is not available (-ENOTSUP) from QMP interface, it is only > > used internally by block commit code. > > > > > +++ b/qapi-schema.json > > @@ -1363,7 +1363,7 @@ > > # Since: 1.3 > > ## > > { 'enum': 'MirrorSyncMode', > > - 'data': ['top', 'full', 'none'] } > > + 'data': ['top', 'full', 'none', 'common'] } > > Is it worth documenting the mode, in order to include a '(since 1.7)' > notation, as well as a mention that this mode is not supported via QMP > but only exists so that the code generator will support the mode needed > internally? Is there any way to refactor things so that you don't have > to munge the QAPI just to provide this internal-only mode? > As described in commit message, this mode could be useful once blockdev-add has device referencing (backing_hd sharing). For now, even with the same backing file, they don't share BDS, so it's not working as expected and should be disabled. So do you think it OK to document as "not implemented" for now, and wait for backing_hd sharing to enable it? Thanks, Fam
diff --git a/block/mirror.c b/block/mirror.c index 6e7a274..fdc7fa8 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, @@ -541,6 +541,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 @@ -563,6 +564,16 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, return; } + if (mode == MIRROR_SYNC_MODE_COMMON) { + base = bdrv_common_ancestor(bs, target); + if (!base) { + error_setg(errp, "source and target has no common ancestor"); + return; + } + } else if (mode == MIRROR_SYNC_MODE_TOP) { + base = bs->backing_hd; + } + s = block_job_create(&mirror_job_type, bs, speed, cb, opaque, errp); if (!s) { return; @@ -572,6 +583,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); diff --git a/blockdev.c b/blockdev.c index 8aa66a9..3acd330 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1906,6 +1906,11 @@ void qmp_drive_mirror(const char *device, const char *target, return; } + if (sync == MIRROR_SYNC_MODE_COMMON) { + error_set(errp, QERR_UNSUPPORTED); + return; + } + flags = bs->open_flags | BDRV_O_RDWR; source = bs->backing_hd; if (!source && sync == MIRROR_SYNC_MODE_TOP) { diff --git a/qapi-schema.json b/qapi-schema.json index 145eca8..6addf49 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1363,7 +1363,7 @@ # Since: 1.3 ## { 'enum': 'MirrorSyncMode', - 'data': ['top', 'full', 'none'] } + 'data': ['top', 'full', 'none', 'common'] } ## # @BlockJobInfo: diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041 index 6661c03..86b3251 100755 --- a/tests/qemu-iotests/041 +++ b/tests/qemu-iotests/041 @@ -206,6 +206,11 @@ class TestSingleDrive(ImageMirroringTestCase): target=target_img) self.assert_qmp(result, 'error/class', 'DeviceNotFound') + def test_common(self): + result = self.vm.qmp('drive-mirror', device='drive0', sync='common', + target=target_img) + self.assert_qmp(result, 'error/class', 'GenericError') + class TestMirrorNoBacking(ImageMirroringTestCase): image_len = 2 * 1024 * 1024 # MB diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out index 42314e9..4fd1c2d 100644 --- a/tests/qemu-iotests/041.out +++ b/tests/qemu-iotests/041.out @@ -1,5 +1,5 @@ -........................ +......................... ---------------------------------------------------------------------- -Ran 24 tests +Ran 25 tests OK
This adds a new sync mode "common" which only copies data that is above the common ancestor of source and target. In general, this could be useful in cases like: base_bs ---> common_ancestor ---> foo ---> bar --->source \ \---> target Where data in foo, bar and source will be copied to target, once such common backing_hd sharing is introduced. For now, we could use a special case: If target is the ancestor of source, like, base_bs ---> target ---> foo ---> bar --->source The data in foo, bar and source will be copied to target, like drive-commit, and when they are synced, the source bs replaces target bs. This is specifically useful for block commit of active layer. This mode is not available (-ENOTSUP) from QMP interface, it is only used internally by block commit code. Signed-off-by: Fam Zheng <famz@redhat.com> --- block/mirror.c | 16 ++++++++++++++-- blockdev.c | 5 +++++ qapi-schema.json | 2 +- tests/qemu-iotests/041 | 5 +++++ tests/qemu-iotests/041.out | 4 ++-- 5 files changed, 27 insertions(+), 5 deletions(-)