[Qemu-devel] [PATCH v4 0/7] block: allow commit active as top

[Qemu-devel] [PATCH v4 0/7] block: allow commit active as top

[Fam Zheng]

Previously live commit of active block device is not supported, this series
implements it and updates corresponding qemu-iotests cases.

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 (7):
  block: add bdrv_common_ancestor()
  qmp: add internal sync mode "common" to mirror_start
  mirror: don't close target
  mirror: Add commit_job_type to perform commit with mirror code
  commit: support commit active layer
  commit: remove unused check
  qemu-iotests: update test cases for commit active

 block.c                    | 15 ++++++++++
 block/commit.c             |  7 -----
 block/mirror.c             | 29 +++++++++++++++---
 blockdev.c                 | 49 +++++++++++++++++++++++++++++--
 include/block/block.h      |  2 ++
 include/block/block_int.h  |  2 ++
 qapi-schema.json           |  2 +-
 tests/qemu-iotests/040     | 73 ++++++++++++++++++++--------------------------
 tests/qemu-iotests/041     |  5 ++++
 tests/qemu-iotests/041.out |  4 +--
 10 files changed, 129 insertions(+), 59 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v4 1/7] block: add bdrv_common_ancestor()

[Fam Zheng]

This function finds the common ancestor in backing chain of two BDSes.
If there's no common ancestor, NULL is returned.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c               | 15 +++++++++++++++
 include/block/block.h |  2 ++
 2 files changed, 17 insertions(+)

diff --git a/block.c b/block.c
index ea4956d..db1381d 100644
--- a/block.c
+++ b/block.c
@@ -3036,6 +3036,21 @@ BlockDriverState *bdrv_find(const char *name)
     return NULL;
 }
 
+BlockDriverState *bdrv_common_ancestor(BlockDriverState *bs1,
+                                       BlockDriverState *bs2)
+{
+    /* TODO: higher efficiency with reverse linked backing chain */
+    BlockDriverState *base1, *base2;
+    for (base1 = bs1; base1; base1 = base1->backing_hd) {
+        for (base2 = bs2; base2; base2 = base2->backing_hd) {
+            if (base1 == base2) {
+                return base1;
+            }
+        }
+    }
+    return NULL;
+}
+
 BlockDriverState *bdrv_next(BlockDriverState *bs)
 {
     if (!bs) {
diff --git a/include/block/block.h b/include/block/block.h
index f808550..e095f16 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -321,6 +321,8 @@ void bdrv_lock_medium(BlockDriverState *bs, bool locked);
 void bdrv_eject(BlockDriverState *bs, bool eject_flag);
 const char *bdrv_get_format_name(BlockDriverState *bs);
 BlockDriverState *bdrv_find(const char *name);
+BlockDriverState *bdrv_common_ancestor(BlockDriverState *bs1,
+                                       BlockDriverState *bs2);
 BlockDriverState *bdrv_next(BlockDriverState *bs);
 void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs),
                   void *opaque);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v4 1/7] block: add bdrv_common_ancestor()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 502 bytes --]

On 09/30/2013 06:02 AM, Fam Zheng wrote:
> This function finds the common ancestor in backing chain of two BDSes.
> If there's no common ancestor, NULL is returned.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c               | 15 +++++++++++++++
>  include/block/block.h |  2 ++
>  2 files changed, 17 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 --]

[Qemu-devel] [PATCH v4 2/7] qmp: add internal sync mode "common" to mirror_start

[Fam Zheng]

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(-)

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
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v4 2/7] qmp: add internal sync mode "common" to mirror_start

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1631 bytes --]

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?

-- 
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 --]

Re: [Qemu-devel] [PATCH v4 2/7] qmp: add internal sync mode "common" to mirror_start

[Fam Zheng]

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

[Qemu-devel] [PATCH v4 3/7] mirror: don't close target

[Fam Zheng]

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 fdc7fa8..af6851f 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.3.1

Re: [Qemu-devel] [PATCH v4 3/7] mirror: don't close target

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 749 bytes --]

On 09/30/2013 06:02 AM, Fam Zheng wrote:
> 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(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/mirror.c b/block/mirror.c
> index fdc7fa8..af6851f 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);
>  }
> 

-- 
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 --]

[Qemu-devel] [PATCH v4 4/7] mirror: Add commit_job_type to perform commit with mirror code

[Fam Zheng]

Commit active layer will be implemented in block/mirror.c, prepare a new
job type to let it have a right type name for the user.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/mirror.c            | 12 +++++++++++-
 blockdev.c                |  2 +-
 include/block/block_int.h |  2 ++
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index af6851f..20dcfb6 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -532,10 +532,19 @@ static const BlockJobType mirror_job_type = {
     .complete      = mirror_complete,
 };
 
+static const BlockJobType commit_job_type = {
+    .instance_size = sizeof(MirrorBlockJob),
+    .job_type      = "commit",
+    .set_speed     = mirror_set_speed,
+    .iostatus_reset= mirror_iostatus_reset,
+    .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,
+                  bool commit_job,
                   BlockDriverCompletionFunc *cb,
                   void *opaque, Error **errp)
 {
@@ -573,7 +582,8 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
         base = bs->backing_hd;
     }
 
-    s = block_job_create(&mirror_job_type, bs, speed, cb, opaque, errp);
+    s = block_job_create(commit_job ? &commit_job_type : &mirror_job_type,
+                         bs, speed, cb, opaque, errp);
     if (!s) {
         return;
     }
diff --git a/blockdev.c b/blockdev.c
index 3acd330..032913e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1963,7 +1963,7 @@ void qmp_drive_mirror(const char *device, const char *target,
     }
 
     mirror_start(bs, target_bs, speed, granularity, buf_size, sync,
-                 on_source_error, on_target_error,
+                 on_source_error, on_target_error, false,
                  block_job_cb, bs, &local_err);
     if (local_err != NULL) {
         bdrv_unref(target_bs);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 3eeb6fe..77a6295 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -377,6 +377,7 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
  * @mode: Whether to collapse all images in the chain to the target.
  * @on_source_error: The action to take upon error reading from the source.
  * @on_target_error: The action to take upon error writing to the target.
+ * @commit_job: Whether the job type string should be "commit".
  * @cb: Completion function for the job.
  * @opaque: Opaque pointer value passed to @cb.
  * @errp: Error object.
@@ -390,6 +391,7 @@ 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,
+                  bool commit_job,
                   BlockDriverCompletionFunc *cb,
                   void *opaque, Error **errp);
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v4 4/7] mirror: Add commit_job_type to perform commit with mirror code

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1778 bytes --]

On 09/30/2013 06:02 AM, Fam Zheng wrote:
> Commit active layer will be implemented in block/mirror.c, prepare a new
> job type to let it have a right type name for the user.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/mirror.c            | 12 +++++++++++-
>  blockdev.c                |  2 +-
>  include/block/block_int.h |  2 ++
>  3 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index af6851f..20dcfb6 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -532,10 +532,19 @@ static const BlockJobType mirror_job_type = {
>      .complete      = mirror_complete,
>  };
>  
> +static const BlockJobType commit_job_type = {
> +    .instance_size = sizeof(MirrorBlockJob),
> +    .job_type      = "commit",

I still wonder if we should complete the conversion over to a QAPI enum
type for all valid job types prior to hard-coding open strings through
yet more of the code base.  As long as we don't have introspection done
yet, we can still make the switch, and in the long run, having an enum
of valid job types seems like it will be better for maintenance, all
with no change to what is sent over the wire in QMP.

> @@ -390,6 +391,7 @@ 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,
> +                  bool commit_job,

If we DO create a QAPI enum for job type, then this parameter would be
the enum type, rather than a bool.

-- 
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 --]

Re: [Qemu-devel] [PATCH v4 4/7] mirror: Add commit_job_type to perform commit with mirror code

[Fam Zheng]

On Tue, 10/01 11:13, Eric Blake wrote:
> On 09/30/2013 06:02 AM, Fam Zheng wrote:
> > Commit active layer will be implemented in block/mirror.c, prepare a new
> > job type to let it have a right type name for the user.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/mirror.c            | 12 +++++++++++-
> >  blockdev.c                |  2 +-
> >  include/block/block_int.h |  2 ++
> >  3 files changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/mirror.c b/block/mirror.c
> > index af6851f..20dcfb6 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -532,10 +532,19 @@ static const BlockJobType mirror_job_type = {
> >      .complete      = mirror_complete,
> >  };
> >  
> > +static const BlockJobType commit_job_type = {
> > +    .instance_size = sizeof(MirrorBlockJob),
> > +    .job_type      = "commit",
> 
> I still wonder if we should complete the conversion over to a QAPI enum
> type for all valid job types prior to hard-coding open strings through
> yet more of the code base.  As long as we don't have introspection done
> yet, we can still make the switch, and in the long run, having an enum
> of valid job types seems like it will be better for maintenance, all
> with no change to what is sent over the wire in QMP.
> 
> > @@ -390,6 +391,7 @@ 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,
> > +                  bool commit_job,
> 
> If we DO create a QAPI enum for job type, then this parameter would be
> the enum type, rather than a bool.
> 

Good point, I'll work on a QAPI enum and base this on it.

Fam

[Qemu-devel] [PATCH v4 5/7] commit: support commit active layer

[Fam Zheng]

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.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c | 42 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 032913e..f730e6f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1663,6 +1663,30 @@ void qmp_block_stream(const char *device, bool has_base,
     trace_qmp_block_stream(bs, bs->job);
 }
 
+typedef struct {
+    BlockDriverState *bs;
+    BlockDriverState *base;
+} commit_active_info_t;
+
+static void commit_active_cb(void *_info, int ret)
+{
+    commit_active_info_t *info = _info;
+    BlockDriverState *p;
+    assert(info->bs);
+    assert(info->base);
+    assert(info->base->backing_hd);
+    if (!ret) {
+        /* Mirror code already swapped bs and base, we drop the bs loop chain
+         * formed by the swap: break the loop chain, trigger the chain unref.
+         */
+        p = info->base->backing_hd;
+        info->base->backing_hd = NULL;
+        bdrv_unref(p);
+    }
+    block_job_cb(info->bs, ret);
+    g_free(info);
+}
+
 void qmp_block_commit(const char *device,
                       bool has_base, const char *base, const char *top,
                       bool has_speed, int64_t speed,
@@ -1710,8 +1734,22 @@ 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_info_t *info = g_malloc(sizeof(commit_active_info_t));
+        info->bs = bs;
+        info->base = base_bs;
+        if (bdrv_reopen(base_bs, bs->open_flags, &local_err)) {
+            error_propagate(errp, local_err);
+            return;
+        }
+        bdrv_ref(base_bs);
+        mirror_start(bs, base_bs, speed, 0, 0, MIRROR_SYNC_MODE_COMMON,
+                     on_error, on_error, true,
+                     commit_active_cb, info, &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;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v4 5/7] commit: support commit active layer

[Paolo Bonzini]

Il 30/09/2013 14:02, Fam Zheng ha scritto:
> +        /* Mirror code already swapped bs and base, we drop the bs loop chain
> +         * formed by the swap: break the loop chain, trigger the chain unref.
> +         */
> +        p = info->base->backing_hd;
> +        info->base->backing_hd = NULL;
> +        bdrv_unref(p);

Should this code be in mirror_run? Perhaps extracting it into a new
function together with the preceding lines:

        if (bdrv_get_flags(s->target) != bdrv_get_flags(s->common.bs)) {
            bdrv_reopen(s->target, bdrv_get_flags(s->common.bs), NULL);
        }
        bdrv_swap(s->target, s->common.bs);

Same for this:

> +        if (bdrv_reopen(base_bs, bs->open_flags, &local_err)) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +        bdrv_ref(base_bs);

Perhaps you can have an internal function mirror_start_job and two
front-ends mirror_start + commit_active_start that wrap it.  Similarly,
mirror_start_job can take the base BDS and a bool instead of
MirrorSyncMode (removing the need for MIRROR_SYNC_MODE_COMMON).

But apart from these details, the series looks very nice.

Paolo

[Qemu-devel] [PATCH v4 6/7] commit: remove unused check

[Fam Zheng]

We support top == active for commit now, remove the check which is dead
code now.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/commit.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index ac4b7cc..086f8c9 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -198,13 +198,6 @@ 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;
-    }
-
     if (top == base) {
         error_setg(errp, "Invalid files for merge: top and base are the same");
         return;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v4 6/7] commit: remove unused check

[Paolo Bonzini]

Il 30/09/2013 14:02, Fam Zheng ha scritto:
> We support top == active for commit now, remove the check which is dead
> code now.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/commit.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/block/commit.c b/block/commit.c
> index ac4b7cc..086f8c9 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -198,13 +198,6 @@ 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;
> -    }
> -
>      if (top == base) {
>          error_setg(errp, "Invalid files for merge: top and base are the same");
>          return;
> 

Perhaps this could even become an assertion, or it could take care of
calling commit_active_start?

Paolo

Re: [Qemu-devel] [PATCH v4 6/7] commit: remove unused check

[Fam Zheng]

On Mon, 09/30 14:17, Paolo Bonzini wrote:
> Il 30/09/2013 14:02, Fam Zheng ha scritto:
> > We support top == active for commit now, remove the check which is dead
> > code now.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/commit.c | 7 -------
> >  1 file changed, 7 deletions(-)
> > 
> > diff --git a/block/commit.c b/block/commit.c
> > index ac4b7cc..086f8c9 100644
> > --- a/block/commit.c
> > +++ b/block/commit.c
> > @@ -198,13 +198,6 @@ 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;
> > -    }
> > -
> >      if (top == base) {
> >          error_setg(errp, "Invalid files for merge: top and base are the same");
> >          return;
> > 
> 
> Perhaps this could even become an assertion, or it could take care of
> calling commit_active_start?
> 

Good idea. I'll make it an assertion.

Thanks,
Fam

[Qemu-devel] [PATCH v4 7/7] qemu-iotests: update test cases for commit active

[Fam Zheng]

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 | 73 +++++++++++++++++++++-----------------------------
 1 file changed, 31 insertions(+), 42 deletions(-)

diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index aad535a..902df0d 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -63,6 +63,28 @@ class ImageCommitTestCase(iotests.QMPTestCase):
             i = i + 512
         file.close()
 
+    def run_commit_test(self, top, base, is_active=False):
+        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
@@ -84,23 +106,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"))
 
@@ -127,10 +133,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, True)
+        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()
@@ -191,23 +196,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"))
 
@@ -234,10 +223,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, True)
+        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()
@@ -253,6 +241,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.3.1