[Qemu-devel] [PATCH v6 0/6] block: allow commit active as top

[Qemu-devel] [PATCH v6 0/6] 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.

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

[Qemu-devel] [PATCH v6 1/6] 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 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

[Qemu-devel] [PATCH v6 2/6] mirror: move base to MirrorBlockJob

[Fam Zheng]

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

[Qemu-devel] [PATCH v6 3/6] block: add commit_active_start()

[Fam Zheng]

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

[Qemu-devel] [PATCH v6 4/6] 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.

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

[Qemu-devel] [PATCH v6 5/6] 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 | 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

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

[Fam Zheng]

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

Re: [Qemu-devel] [PATCH v6 0/6] block: allow commit active as top

[Fam Zheng]

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

Re: [Qemu-devel] [PATCH v6 4/6] commit: support commit active layer

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH v6 0/6] block: allow commit active as top

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH v6 4/6] commit: support commit active layer

[Eric Blake]

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

Re: [Qemu-devel] [PATCH v6 4/6] commit: support commit active layer

[Fam Zheng]

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

Re: [Qemu-devel] [PATCH v6 4/6] commit: support commit active layer

[Fam Zheng]

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.

Re: [Qemu-devel] [PATCH v6 4/6] commit: support commit active layer

[Fam Zheng]

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