[Qemu-devel] [PATCH v4 00/11] backup-top filter driver for backup

[Qemu-devel] [PATCH v4 00/11] backup-top filter driver for backup

[Vladimir Sementsov-Ogievskiy]

Hi all!

These series introduce backup-top driver. It's a filter-node, which
do copy-before-write operation. Mirror uses filter-node for handling
guest writes, let's move to filter-node (from write-notifiers) for
backup too (patch 16)

v4:
fixes, rewrite driver to be implicit, drop new interfaces and
don't move to BdrvDirtyBitmap for now, as it's not obvious will
it be really needed and don't relate to these series more.

v3 was "[PATCH v3 00/18] fleecing-hook driver for backup"

v2 was "[RFC v2] new, node-graph-based fleecing and backup"

These series are based on
 [PATCH v4 0/8] dirty-bitmap: rewrite bdrv_dirty_iter_next_area
and
 [PATCH 0/2] replication: drop extra sync

Based-on: <20180919124343.28206-1-vsementsov@virtuozzo.com>
Based-on: <20180917145732.48590-1-vsementsov@virtuozzo.com>

Vladimir Sementsov-Ogievskiy (11):
  block/backup: simplify backup_incremental_init_copy_bitmap
  block/backup: move to copy_bitmap with granularity
  block: allow serialized reads to intersect
  block: improve should_update_child
  iotests: handle -f argument correctly for qemu_io_silent
  iotests: allow resume_drive by node name
  iotests: prepare 055 to graph changes during backup job
  block: introduce backup-top filter driver
  block: add lock/unlock range functions
  block/backup: tiny refactor backup_job_create
  block/backup: use backup-top instead of write notifiers

 block/backup-top.h            |  44 ++++
 include/block/block_int.h     |   3 +
 block.c                       |  32 ++-
 block/backup-top.c            | 298 ++++++++++++++++++++++++
 block/backup.c                | 415 +++++++++++++++++-----------------
 block/io.c                    |  38 +++-
 block/Makefile.objs           |   2 +
 tests/qemu-iotests/055        |  23 +-
 tests/qemu-iotests/iotests.py |  16 +-
 9 files changed, 641 insertions(+), 230 deletions(-)
 create mode 100644 block/backup-top.h
 create mode 100644 block/backup-top.c

-- 
2.18.0

[Qemu-devel] [PATCH v4 01/11] block/backup: simplify backup_incremental_init_copy_bitmap

[Vladimir Sementsov-Ogievskiy]

Simplify backup_incremental_init_copy_bitmap using the function
bdrv_dirty_bitmap_next_dirty_area.

Note: move to job->len instead of bitmap size: it should not matter but
less code.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/backup.c | 40 ++++++++++++----------------------------
 1 file changed, 12 insertions(+), 28 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 435414e964..fbe7ce19e1 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -406,43 +406,27 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
 /* init copy_bitmap from sync_bitmap */
 static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)
 {
-    BdrvDirtyBitmapIter *dbi;
-    int64_t offset;
-    int64_t end = DIV_ROUND_UP(bdrv_dirty_bitmap_size(job->sync_bitmap),
-                               job->cluster_size);
-
-    dbi = bdrv_dirty_iter_new(job->sync_bitmap);
-    while ((offset = bdrv_dirty_iter_next(dbi)) != -1) {
-        int64_t cluster = offset / job->cluster_size;
-        int64_t next_cluster;
-
-        offset += bdrv_dirty_bitmap_granularity(job->sync_bitmap);
-        if (offset >= bdrv_dirty_bitmap_size(job->sync_bitmap)) {
-            hbitmap_set(job->copy_bitmap, cluster, end - cluster);
-            break;
-        }
+    uint64_t offset = 0;
+    uint64_t bytes = job->len;
 
-        offset = bdrv_dirty_bitmap_next_zero(job->sync_bitmap, offset,
-                                             UINT64_MAX);
-        if (offset == -1) {
-            hbitmap_set(job->copy_bitmap, cluster, end - cluster);
-            break;
-        }
+    while (bdrv_dirty_bitmap_next_dirty_area(job->sync_bitmap,
+                                             &offset, &bytes))
+    {
+        uint64_t cluster = offset / job->cluster_size;
+        uint64_t last_cluster = (offset + bytes) / job->cluster_size;
 
-        next_cluster = DIV_ROUND_UP(offset, job->cluster_size);
-        hbitmap_set(job->copy_bitmap, cluster, next_cluster - cluster);
-        if (next_cluster >= end) {
+        hbitmap_set(job->copy_bitmap, cluster, last_cluster - cluster + 1);
+
+        offset = (last_cluster + 1) * job->cluster_size;
+        if (offset >= job->len) {
             break;
         }
-
-        bdrv_set_dirty_iter(dbi, next_cluster * job->cluster_size);
+        bytes = job->len - offset;
     }
 
     /* TODO job_progress_set_remaining() would make more sense */
     job_progress_update(&job->common.job,
         job->len - hbitmap_count(job->copy_bitmap) * job->cluster_size);
-
-    bdrv_dirty_iter_free(dbi);
 }
 
 static int coroutine_fn backup_run(Job *job, Error **errp)
-- 
2.18.0

[Qemu-devel] [PATCH v4 02/11] block/backup: move to copy_bitmap with granularity

[Vladimir Sementsov-Ogievskiy]

We are going to share this bitmap between backup and backup-top filter
driver, so let's share something more meaningful. It also simplifies
some calculations.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/backup.c | 48 +++++++++++++++++++++++-------------------------
 1 file changed, 23 insertions(+), 25 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index fbe7ce19e1..0b3fddeb6c 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -114,7 +114,8 @@ static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,
     int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
     int write_flags = job->serialize_target_writes ? BDRV_REQ_SERIALISING : 0;
 
-    hbitmap_reset(job->copy_bitmap, start / job->cluster_size, 1);
+    assert(QEMU_IS_ALIGNED(start, job->cluster_size));
+    hbitmap_reset(job->copy_bitmap, start, job->cluster_size);
     nbytes = MIN(job->cluster_size, job->len - start);
     if (!*bounce_buffer) {
         *bounce_buffer = blk_blockalign(blk, job->cluster_size);
@@ -150,7 +151,7 @@ static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,
 
     return nbytes;
 fail:
-    hbitmap_set(job->copy_bitmap, start / job->cluster_size, 1);
+    hbitmap_set(job->copy_bitmap, start, job->cluster_size);
     return ret;
 
 }
@@ -170,16 +171,15 @@ static int coroutine_fn backup_cow_with_offload(BackupBlockJob *job,
     int write_flags = job->serialize_target_writes ? BDRV_REQ_SERIALISING : 0;
 
     assert(QEMU_IS_ALIGNED(job->copy_range_size, job->cluster_size));
+    assert(QEMU_IS_ALIGNED(start, job->cluster_size));
     nbytes = MIN(job->copy_range_size, end - start);
     nr_clusters = DIV_ROUND_UP(nbytes, job->cluster_size);
-    hbitmap_reset(job->copy_bitmap, start / job->cluster_size,
-                  nr_clusters);
+    hbitmap_reset(job->copy_bitmap, start, job->cluster_size * nr_clusters);
     ret = blk_co_copy_range(blk, start, job->target, start, nbytes,
                             read_flags, write_flags);
     if (ret < 0) {
         trace_backup_do_cow_copy_range_fail(job, start, ret);
-        hbitmap_set(job->copy_bitmap, start / job->cluster_size,
-                    nr_clusters);
+        hbitmap_set(job->copy_bitmap, start, job->cluster_size * nr_clusters);
         return ret;
     }
 
@@ -207,7 +207,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
     cow_request_begin(&cow_request, job, start, end);
 
     while (start < end) {
-        if (!hbitmap_get(job->copy_bitmap, start / job->cluster_size)) {
+        if (!hbitmap_get(job->copy_bitmap, start)) {
             trace_backup_do_cow_skip(job, start);
             start += job->cluster_size;
             continue; /* already copied */
@@ -303,6 +303,11 @@ static void backup_clean(Job *job)
     assert(s->target);
     blk_unref(s->target);
     s->target = NULL;
+
+    if (s->copy_bitmap) {
+        hbitmap_free(s->copy_bitmap);
+        s->copy_bitmap = NULL;
+    }
 }
 
 static void backup_attached_aio_context(BlockJob *job, AioContext *aio_context)
@@ -315,7 +320,6 @@ static void backup_attached_aio_context(BlockJob *job, AioContext *aio_context)
 void backup_do_checkpoint(BlockJob *job, Error **errp)
 {
     BackupBlockJob *backup_job = container_of(job, BackupBlockJob, common);
-    int64_t len;
 
     assert(block_job_driver(job) == &backup_job_driver);
 
@@ -325,8 +329,7 @@ void backup_do_checkpoint(BlockJob *job, Error **errp)
         return;
     }
 
-    len = DIV_ROUND_UP(backup_job->len, backup_job->cluster_size);
-    hbitmap_set(backup_job->copy_bitmap, 0, len);
+    hbitmap_set(backup_job->copy_bitmap, 0, backup_job->len);
 }
 
 static void backup_drain(BlockJob *job)
@@ -381,16 +384,16 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
 {
     int ret;
     bool error_is_read;
-    int64_t cluster;
+    int64_t offset;
     HBitmapIter hbi;
 
     hbitmap_iter_init(&hbi, job->copy_bitmap, 0);
-    while ((cluster = hbitmap_iter_next(&hbi)) != -1) {
+    while ((offset = hbitmap_iter_next(&hbi)) != -1) {
         do {
             if (yield_and_check(job)) {
                 return 0;
             }
-            ret = backup_do_cow(job, cluster * job->cluster_size,
+            ret = backup_do_cow(job, offset,
                                 job->cluster_size, &error_is_read, false);
             if (ret < 0 && backup_error_action(job, error_is_read, -ret) ==
                            BLOCK_ERROR_ACTION_REPORT)
@@ -412,12 +415,9 @@ static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)
     while (bdrv_dirty_bitmap_next_dirty_area(job->sync_bitmap,
                                              &offset, &bytes))
     {
-        uint64_t cluster = offset / job->cluster_size;
-        uint64_t last_cluster = (offset + bytes) / job->cluster_size;
+        hbitmap_set(job->copy_bitmap, offset, bytes);
 
-        hbitmap_set(job->copy_bitmap, cluster, last_cluster - cluster + 1);
-
-        offset = (last_cluster + 1) * job->cluster_size;
+        offset += bytes;
         if (offset >= job->len) {
             break;
         }
@@ -426,30 +426,27 @@ static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)
 
     /* TODO job_progress_set_remaining() would make more sense */
     job_progress_update(&job->common.job,
-        job->len - hbitmap_count(job->copy_bitmap) * job->cluster_size);
+        job->len - hbitmap_count(job->copy_bitmap));
 }
 
 static int coroutine_fn backup_run(Job *job, Error **errp)
 {
     BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
     BlockDriverState *bs = blk_bs(s->common.blk);
-    int64_t offset, nb_clusters;
+    int64_t offset;
     int ret = 0;
 
     QLIST_INIT(&s->inflight_reqs);
     qemu_co_rwlock_init(&s->flush_rwlock);
 
-    nb_clusters = DIV_ROUND_UP(s->len, s->cluster_size);
     job_progress_set_remaining(job, s->len);
 
-    s->copy_bitmap = hbitmap_alloc(nb_clusters, 0);
     if (s->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
         backup_incremental_init_copy_bitmap(s);
     } else {
-        hbitmap_set(s->copy_bitmap, 0, nb_clusters);
+        hbitmap_set(s->copy_bitmap, 0, s->len);
     }
 
-
     s->before_write.notify = backup_before_write_notify;
     bdrv_add_before_write_notifier(bs, &s->before_write);
 
@@ -530,7 +527,6 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
     /* wait until pending backup_do_cow() calls have completed */
     qemu_co_rwlock_wrlock(&s->flush_rwlock);
     qemu_co_rwlock_unlock(&s->flush_rwlock);
-    hbitmap_free(s->copy_bitmap);
 
     return ret;
 }
@@ -681,6 +677,8 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     } else {
         job->cluster_size = MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
     }
+
+    job->copy_bitmap = hbitmap_alloc(len, ctz32(job->cluster_size));
     job->use_copy_range = true;
     job->copy_range_size = MIN_NON_ZERO(blk_get_max_transfer(job->common.blk),
                                         blk_get_max_transfer(job->target));
-- 
2.18.0

[Qemu-devel] [PATCH v4 03/11] block: allow serialized reads to intersect

[Vladimir Sementsov-Ogievskiy]

Otherwise, if we have serialized read-part in copy_range from backing
file to its parent if CoW take place, this CoW's sub-reads will
intersect with firstly created serialized read request.

Anyway, reads should not influence on disk view, let's allow them to
intersect.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/io.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/io.c b/block/io.c
index bd9d688f8b..d4e46cb3dc 100644
--- a/block/io.c
+++ b/block/io.c
@@ -735,7 +735,8 @@ static bool coroutine_fn wait_serialising_requests(BdrvTrackedRequest *self)
         retry = false;
         qemu_co_mutex_lock(&bs->reqs_lock);
         QLIST_FOREACH(req, &bs->tracked_requests, list) {
-            if (req == self || (!req->serialising && !self->serialising)) {
+            if (req == self || (!req->serialising && !self->serialising) ||
+                (self->type == BDRV_TRACKED_READ && req->type == self->type)) {
                 continue;
             }
             if (tracked_request_overlaps(req, self->overlap_offset,
-- 
2.18.0

[Qemu-devel] [PATCH v4 04/11] block: improve should_update_child

[Vladimir Sementsov-Ogievskiy]

As it already said in the comment, we don't want to create loops in
parent->child relations. So, when we try to append @to to @c, we should
check that @c is not in @to children subtree, and we should check it
recursively, not only the first level. The patch provides BFS-based
search, to check the relations.

This is needed for further fleecing-hook filter usage: we need to
append it to source, when the hook is already a parent of target, and
source may be in a backing chain of target (fleecing-scheme). So, on
appending, the hook should not became a child (direct or through
children subtree) of the target.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index c298ca6a19..7f605b0bf0 100644
--- a/block.c
+++ b/block.c
@@ -3432,7 +3432,7 @@ void bdrv_close_all(void)
 
 static bool should_update_child(BdrvChild *c, BlockDriverState *to)
 {
-    BdrvChild *to_c;
+    GList *queue = NULL, *pos;
 
     if (c->role->stay_at_node) {
         return false;
@@ -3468,13 +3468,35 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to)
      * if A is a child of B, that means we cannot replace A by B there
      * because that would create a loop.  Silently detaching A from B
      * is also not really an option.  So overall just leaving A in
-     * place there is the most sensible choice. */
-    QLIST_FOREACH(to_c, &to->children, next) {
-        if (to_c == c) {
-            return false;
+     * place there is the most sensible choice.
+     *
+     * upd: If the child @c belongs to the @to's children, or children of it's
+     * children and so on - this would create a loop to. To prevent it let's do
+     * a BFS search on @to children subtree.
+     */
+
+    pos = queue = g_list_append(queue, to);
+    while (pos) {
+        BlockDriverState *v = pos->data;
+        BdrvChild *c2;
+
+        QLIST_FOREACH(c2, &v->children, next) {
+            if (c2 == c) {
+                g_list_free(queue);
+                return false;
+            }
+
+            if (g_list_find(queue, c2->bs)) {
+                continue;
+            }
+
+            queue = g_list_append(queue, c2->bs);
         }
+
+        pos = pos->next;
     }
 
+    g_list_free(queue);
     return true;
 }
 
-- 
2.18.0

[Qemu-devel] [PATCH v4 05/11] iotests: handle -f argument correctly for qemu_io_silent

[Vladimir Sementsov-Ogievskiy]

Correctly rewrite default argument. After the patch, the function can
be used for other (not only default test-chosen) image format.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/iotests.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 4e67fbbe96..bc0b8851bd 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -136,7 +136,12 @@ def qemu_io(*args):
 
 def qemu_io_silent(*args):
     '''Run qemu-io and return the exit code, suppressing stdout'''
-    args = qemu_io_args + list(args)
+    if '-f' in qemu_io_args and '-f' in args:
+        ind = qemu_io_args.index('-f')
+        args = qemu_io_args[:ind] + qemu_io_args[ind+2:] + list(args)
+    else:
+        args = qemu_io_args + list(args)
+
     exitcode = subprocess.call(args, stdout=open('/dev/null', 'w'))
     if exitcode < 0:
         sys.stderr.write('qemu-io received signal %i: %s\n' %
-- 
2.18.0

[Qemu-devel] [PATCH v4 06/11] iotests: allow resume_drive by node name

[Vladimir Sementsov-Ogievskiy]

After node graph changes, we may not be able to resume_drive by device
name (backing files are not recursively searched). So, lets allow to
resume by node-name. Set constant name for breakpoints, to avoid
introducing extra parameters.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/iotests.py | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index bc0b8851bd..5cc47c85de 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -404,11 +404,11 @@ class VM(qtest.QEMUQtestMachine):
             self.pause_drive(drive, "write_aio")
             return
         self.qmp('human-monitor-command',
-                    command_line='qemu-io %s "break %s bp_%s"' % (drive, event, drive))
+                    command_line='qemu-io %s "break %s bp_0"' % (drive, event))
 
     def resume_drive(self, drive):
         self.qmp('human-monitor-command',
-                    command_line='qemu-io %s "remove_break bp_%s"' % (drive, drive))
+                    command_line='qemu-io %s "remove_break bp_0"' % (drive))
 
     def hmp_qemu_io(self, drive, cmd):
         '''Write to a given drive using an HMP command'''
@@ -531,13 +531,14 @@ class QMPTestCase(unittest.TestCase):
         self.assertEqual(self.vm.flatten_qmp_object(json.loads(json_filename[5:])),
                          self.vm.flatten_qmp_object(reference))
 
-    def cancel_and_wait(self, drive='drive0', force=False, resume=False):
+    def cancel_and_wait(self, drive='drive0', force=False, resume=False,
+                        resume_node=None):
         '''Cancel a block job and wait for it to finish, returning the event'''
         result = self.vm.qmp('block-job-cancel', device=drive, force=force)
         self.assert_qmp(result, 'return', {})
 
         if resume:
-            self.vm.resume_drive(drive)
+            self.vm.resume_drive(resume_node or drive)
 
         cancelled = False
         result = None
-- 
2.18.0

[Qemu-devel] [PATCH v4 07/11] iotests: prepare 055 to graph changes during backup job

[Vladimir Sementsov-Ogievskiy]

Backup will append fleecing-hook node above source node, so, we can't
resume by device name (because resume don't search recursively through
backing chain).

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/055 | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index 3437c11507..be5451e1c5 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -48,7 +48,8 @@ class TestSingleDrive(iotests.QMPTestCase):
     def setUp(self):
         qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, str(image_len))
 
-        self.vm = iotests.VM().add_drive('blkdebug::' + test_img)
+        self.vm = iotests.VM().add_drive('blkdebug::' + test_img,
+                                         'node-name=source')
         self.vm.add_drive(blockdev_target_img, interface="none")
         if iotests.qemu_default_machine == 'pc':
             self.vm.add_drive(None, 'media=cdrom', 'ide')
@@ -69,7 +70,7 @@ class TestSingleDrive(iotests.QMPTestCase):
         result = self.vm.qmp(cmd, device='drive0', target=target, sync='full')
         self.assert_qmp(result, 'return', {})
 
-        event = self.cancel_and_wait(resume=True)
+        event = self.cancel_and_wait(resume=True, resume_node='source')
         self.assert_qmp(event, 'data/type', 'backup')
 
     def test_cancel_drive_backup(self):
@@ -87,7 +88,7 @@ class TestSingleDrive(iotests.QMPTestCase):
         self.assert_qmp(result, 'return', {})
 
         self.pause_job('drive0', wait=False)
-        self.vm.resume_drive('drive0')
+        self.vm.resume_drive('source')
         self.pause_wait('drive0')
 
         result = self.vm.qmp('query-block-jobs')
@@ -165,7 +166,8 @@ class TestSetSpeed(iotests.QMPTestCase):
     def setUp(self):
         qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, str(image_len))
 
-        self.vm = iotests.VM().add_drive('blkdebug::' + test_img)
+        self.vm = iotests.VM().add_drive('blkdebug::' + test_img,
+                                         'node-name=source')
         self.vm.add_drive(blockdev_target_img, interface="none")
         self.vm.launch()
 
@@ -197,7 +199,7 @@ class TestSetSpeed(iotests.QMPTestCase):
         self.assert_qmp(result, 'return[0]/device', 'drive0')
         self.assert_qmp(result, 'return[0]/speed', 8 * 1024 * 1024)
 
-        event = self.cancel_and_wait(resume=True)
+        event = self.cancel_and_wait(resume=True, resume_node='source')
         self.assert_qmp(event, 'data/type', 'backup')
 
         # Check setting speed option works
@@ -210,7 +212,7 @@ class TestSetSpeed(iotests.QMPTestCase):
         self.assert_qmp(result, 'return[0]/device', 'drive0')
         self.assert_qmp(result, 'return[0]/speed', 4 * 1024 * 1024)
 
-        event = self.cancel_and_wait(resume=True)
+        event = self.cancel_and_wait(resume=True, resume_node='source')
         self.assert_qmp(event, 'data/type', 'backup')
 
     def test_set_speed_drive_backup(self):
@@ -236,7 +238,7 @@ class TestSetSpeed(iotests.QMPTestCase):
         result = self.vm.qmp('block-job-set-speed', device='drive0', speed=-1)
         self.assert_qmp(result, 'error/class', 'GenericError')
 
-        event = self.cancel_and_wait(resume=True)
+        event = self.cancel_and_wait(resume=True, resume_node='source')
         self.assert_qmp(event, 'data/type', 'backup')
 
     def test_set_speed_invalid_drive_backup(self):
@@ -464,7 +466,8 @@ class TestDriveCompression(iotests.QMPTestCase):
             pass
 
     def do_prepare_drives(self, fmt, args, attach_target):
-        self.vm = iotests.VM().add_drive('blkdebug::' + test_img)
+        self.vm = iotests.VM().add_drive('blkdebug::' + test_img,
+                                         'node-name=source')
 
         qemu_img('create', '-f', fmt, blockdev_target_img,
                  str(TestDriveCompression.image_len), *args)
@@ -507,7 +510,7 @@ class TestDriveCompression(iotests.QMPTestCase):
         result = self.vm.qmp(cmd, device='drive0', sync='full', compress=True, **args)
         self.assert_qmp(result, 'return', {})
 
-        event = self.cancel_and_wait(resume=True)
+        event = self.cancel_and_wait(resume=True, resume_node='source')
         self.assert_qmp(event, 'data/type', 'backup')
 
         self.vm.shutdown()
@@ -532,7 +535,7 @@ class TestDriveCompression(iotests.QMPTestCase):
         self.assert_qmp(result, 'return', {})
 
         self.pause_job('drive0', wait=False)
-        self.vm.resume_drive('drive0')
+        self.vm.resume_drive('source')
         self.pause_wait('drive0')
 
         result = self.vm.qmp('query-block-jobs')
-- 
2.18.0

[Qemu-devel] [PATCH v4 08/11] block: introduce backup-top filter driver

[Vladimir Sementsov-Ogievskiy]

Backup-top filter does copy-before-write operation. It should be
inserted above active disk and has a target node for CBW, like the
following:

    +-------+
    | Guest |
    +---+---+
        |r,w
        v
    +---+-----------+  target   +---------------+
    | backup_top    |---------->| target(qcow2) |
    +---+-----------+   CBW     +---+-----------+
        |
backing |r,w
        v
    +---+---------+
    | Active disk |
    +-------------+

The driver will be used in backup instead of write-notifiers.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/backup-top.h  |  44 +++++++
 block/backup-top.c  | 298 ++++++++++++++++++++++++++++++++++++++++++++
 block/Makefile.objs |   2 +
 3 files changed, 344 insertions(+)
 create mode 100644 block/backup-top.h
 create mode 100644 block/backup-top.c

diff --git a/block/backup-top.h b/block/backup-top.h
new file mode 100644
index 0000000000..c26af9fb78
--- /dev/null
+++ b/block/backup-top.h
@@ -0,0 +1,44 @@
+/*
+ * backup-top filter driver
+ *
+ * The driver performs Copy-Before-Write (CBW) operation: it is injected above
+ * some node, and before each write it copies _old_ data to the target node.
+ *
+ * Copyright (c) 2018 Virtuozzo International GmbH. All rights reserved.
+ *
+ * Author:
+ *  Sementsov-Ogievskiy Vladimir <vsementsov@virtuozzo.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+
+#include "block/block_int.h"
+
+typedef struct BDRVBackupTopState {
+    HBitmap *copy_bitmap; /* what should be copied to @target
+                             on guest write. */
+    BdrvChild *target;
+
+    uint64_t bytes_copied;
+} BDRVBackupTopState;
+
+void bdrv_backup_top_drop(BlockDriverState *bs);
+uint64_t bdrv_backup_top_progress(BlockDriverState *bs);
+
+BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
+                                         BlockDriverState *target,
+                                         HBitmap *copy_bitmap,
+                                         Error **errp);
diff --git a/block/backup-top.c b/block/backup-top.c
new file mode 100644
index 0000000000..8cb081f6f3
--- /dev/null
+++ b/block/backup-top.c
@@ -0,0 +1,298 @@
+/*
+ * backup-top filter driver
+ *
+ * The driver performs Copy-Before-Write (CBW) operation: it is injected above
+ * some node, and before each write it copies _old_ data to the target node.
+ *
+ * Copyright (c) 2018 Virtuozzo International GmbH. All rights reserved.
+ *
+ * Author:
+ *  Sementsov-Ogievskiy Vladimir <vsementsov@virtuozzo.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+
+#include "qemu/cutils.h"
+#include "qapi/error.h"
+#include "block/block_int.h"
+#include "block/qdict.h"
+
+#include "block/backup-top.h"
+
+static coroutine_fn int backup_top_co_preadv(
+        BlockDriverState *bs, uint64_t offset, uint64_t bytes,
+        QEMUIOVector *qiov, int flags)
+{
+    /* Features to be implemented:
+     * F1. COR. save read data to fleecing target for fast access
+     *     (to reduce reads). This possibly may be done with use of copy-on-read
+     *     filter, but we need an ability to make COR requests optional: for
+     *     example, if target is a ram-cache, and if it is full now, we should
+     *     skip doing COR request, as it is actually not necessary.
+     *
+     * F2. Feature for guest: read from fleecing target if data is in ram-cache
+     *     and is unchanged
+     */
+
+    return bdrv_co_preadv(bs->backing, offset, bytes, qiov, flags);
+}
+
+static coroutine_fn int backup_top_cbw(BlockDriverState *bs, uint64_t offset,
+                                          uint64_t bytes)
+{
+    int ret = 0;
+    BDRVBackupTopState *s = bs->opaque;
+    uint64_t gran = 1UL << hbitmap_granularity(s->copy_bitmap);
+    uint64_t end = QEMU_ALIGN_UP(offset + bytes, gran);
+    uint64_t off = QEMU_ALIGN_DOWN(offset, gran), len;
+    size_t align = MAX(bdrv_opt_mem_align(bs->backing->bs),
+                       bdrv_opt_mem_align(s->target->bs));
+    struct iovec iov = {
+        .iov_base = qemu_memalign(align, end - off),
+        .iov_len = end - off
+    };
+    QEMUIOVector qiov;
+
+    qemu_iovec_init_external(&qiov, &iov, 1);
+
+    /* Features to be implemented:
+     * F3. parallelize copying loop
+     * F4. detect zeros
+     * F5. use block_status ?
+     * F6. don't copy clusters which are already cached by COR [see F1]
+     * F7. if target is ram-cache and it is full, there should be a possibility
+     *     to drop not necessary data (cached by COR [see F1]) to handle CBW
+     *     fast.
+     */
+
+    len = end - off;
+    while (hbitmap_next_dirty_area(s->copy_bitmap, &off, &len)) {
+        iov.iov_len = qiov.size = len;
+
+        hbitmap_reset(s->copy_bitmap, off, len);
+
+        ret = bdrv_co_preadv(bs->backing, off, len, &qiov,
+                             BDRV_REQ_NO_SERIALISING);
+        if (ret < 0) {
+            hbitmap_set(s->copy_bitmap, off, len);
+            goto finish;
+        }
+
+        ret = bdrv_co_pwritev(s->target, off, len, &qiov, BDRV_REQ_SERIALISING);
+        if (ret < 0) {
+            hbitmap_set(s->copy_bitmap, off, len);
+            goto finish;
+        }
+
+        s->bytes_copied += len;
+        off += len;
+        if (off >= end) {
+            break;
+        }
+        len = end - off;
+    }
+
+finish:
+    qemu_vfree(iov.iov_base);
+
+    /* F8. we fail guest request in case of error. We can alter it by
+     * possibility to fail copying process instead, or retry several times, or
+     * may be guest pause, etc.
+     */
+    return ret;
+}
+
+static int coroutine_fn backup_top_co_pdiscard(BlockDriverState *bs,
+                                                  int64_t offset, int bytes)
+{
+    int ret = backup_top_cbw(bs, offset, bytes);
+    if (ret < 0) {
+        return ret;
+    }
+
+    /* Features to be implemented:
+     * F9. possibility of lazy discard: just defer the discard after fleecing
+     *     completion. If write (or new discard) occurs to the same area, just
+     *     drop deferred discard.
+     */
+
+    return bdrv_co_pdiscard(bs->backing, offset, bytes);
+}
+
+static int coroutine_fn backup_top_co_pwrite_zeroes(BlockDriverState *bs,
+        int64_t offset, int bytes, BdrvRequestFlags flags)
+{
+    int ret = backup_top_cbw(bs, offset, bytes);
+    if (ret < 0) {
+        return ret;
+    }
+
+    return bdrv_co_pwrite_zeroes(bs->backing, offset, bytes, flags);
+}
+
+static coroutine_fn int backup_top_co_pwritev(BlockDriverState *bs,
+                                                 uint64_t offset,
+                                                 uint64_t bytes,
+                                                 QEMUIOVector *qiov, int flags)
+{
+    int ret = backup_top_cbw(bs, offset, bytes);
+    if (ret < 0) {
+        return ret;
+    }
+
+    return bdrv_co_pwritev(bs->backing, offset, bytes, qiov, flags);
+}
+
+static int coroutine_fn backup_top_co_flush(BlockDriverState *bs)
+{
+    if (!bs->backing) {
+        return 0;
+    }
+
+    return bdrv_co_flush(bs->backing->bs);
+}
+
+static void backup_top_refresh_filename(BlockDriverState *bs, QDict *opts)
+{
+    if (bs->backing == NULL) {
+        /* we can be here after failed bdrv_attach_child in
+         * bdrv_set_backing_hd */
+        return;
+    }
+    bdrv_refresh_filename(bs->backing->bs);
+    pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),
+            bs->backing->bs->filename);
+}
+
+static void backup_top_child_perm(BlockDriverState *bs, BdrvChild *c,
+                                       const BdrvChildRole *role,
+                                       BlockReopenQueue *reopen_queue,
+                                       uint64_t perm, uint64_t shared,
+                                       uint64_t *nperm, uint64_t *nshared)
+{
+    bdrv_filter_default_perms(bs, c, role, reopen_queue, perm, shared, nperm,
+                              nshared);
+
+    if (role == &child_file) {
+        /* share write to target, to not interfere guest writes to it's disk
+         * which will be in target backing chain */
+        *nshared = *nshared | BLK_PERM_WRITE;
+        *nperm = *nperm | BLK_PERM_WRITE;
+    } else {
+        *nperm = *nperm | BLK_PERM_CONSISTENT_READ;
+    }
+}
+
+BlockDriver bdrv_backup_top_filter = {
+    .format_name = "backup-top",
+    .instance_size = sizeof(BDRVBackupTopState),
+
+    .bdrv_co_preadv             = backup_top_co_preadv,
+    .bdrv_co_pwritev            = backup_top_co_pwritev,
+    .bdrv_co_pwrite_zeroes      = backup_top_co_pwrite_zeroes,
+    .bdrv_co_pdiscard           = backup_top_co_pdiscard,
+    .bdrv_co_flush              = backup_top_co_flush,
+
+    .bdrv_co_block_status       = bdrv_co_block_status_from_backing,
+
+    .bdrv_refresh_filename      = backup_top_refresh_filename,
+
+    .bdrv_child_perm            = backup_top_child_perm,
+
+    .is_filter = true,
+};
+
+BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
+                                         BlockDriverState *target,
+                                         HBitmap *copy_bitmap,
+                                         Error **errp)
+{
+    Error *local_err = NULL;
+    BDRVBackupTopState *state;
+    BlockDriverState *top = bdrv_new_open_driver(&bdrv_backup_top_filter,
+                                                 NULL, BDRV_O_RDWR, errp);
+
+    if (!top) {
+        return NULL;
+    }
+
+    top->implicit = true;
+    top->total_sectors = source->total_sectors;
+    top->opaque = state = g_new0(BDRVBackupTopState, 1);
+    state->copy_bitmap = copy_bitmap;
+
+    bdrv_ref(target);
+    state->target = bdrv_attach_child(top, target, "target", &child_file, errp);
+    if (!state->target) {
+        bdrv_unref(target);
+        bdrv_unref(top);
+        return NULL;
+    }
+
+    bdrv_set_aio_context(top, bdrv_get_aio_context(source));
+    bdrv_set_aio_context(target, bdrv_get_aio_context(source));
+
+    bdrv_drained_begin(source);
+
+    bdrv_ref(top);
+    bdrv_append(top, source, &local_err);
+
+    if (local_err) {
+        bdrv_unref(top);
+    }
+
+    bdrv_drained_end(source);
+
+    if (local_err) {
+        bdrv_unref_child(top, state->target);
+        bdrv_unref(top);
+        error_propagate(errp, local_err);
+        return NULL;
+    }
+
+    return top;
+}
+
+void bdrv_backup_top_drop(BlockDriverState *bs)
+{
+    BDRVBackupTopState *s = bs->opaque;
+
+    AioContext *aio_context = bdrv_get_aio_context(bs);
+
+    aio_context_acquire(aio_context);
+
+    bdrv_drained_begin(bs);
+
+    bdrv_child_try_set_perm(bs->backing, 0, BLK_PERM_ALL, &error_abort);
+    bdrv_replace_node(bs, backing_bs(bs), &error_abort);
+    bdrv_set_backing_hd(bs, NULL, &error_abort);
+
+    bdrv_drained_end(bs);
+
+    if (s->target) {
+        bdrv_unref_child(bs, s->target);
+    }
+    bdrv_unref(bs);
+
+    aio_context_release(aio_context);
+}
+
+uint64_t bdrv_backup_top_progress(BlockDriverState *bs)
+{
+    BDRVBackupTopState *s = bs->opaque;
+
+    return s->bytes_copied;
+}
diff --git a/block/Makefile.objs b/block/Makefile.objs
index c8337bf186..7f71263be0 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -31,6 +31,8 @@ block-obj-y += throttle.o copy-on-read.o
 
 block-obj-y += crypto.o
 
+block-obj-y += backup-top.o
+
 common-obj-y += stream.o
 
 nfs.o-libs         := $(LIBNFS_LIBS)
-- 
2.18.0

[Qemu-devel] [PATCH v4 09/11] block: add lock/unlock range functions

[Vladimir Sementsov-Ogievskiy]

From: Vladimir Sementsov-Ogievskiy <etendren@gmail.com>

Introduce lock/unlock range functionality, based on serialized
requests. This is needed to refactor backup, dropping local
tracked-request-like synchronization.

Signed-off-by: Vladimir Sementsov-Ogievskiy <etendren@gmail.com>
---
 include/block/block_int.h |  3 +++
 block/io.c                | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 92ecbd866e..20617e5853 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -856,6 +856,9 @@ int coroutine_fn bdrv_co_preadv(BdrvChild *child,
 int coroutine_fn bdrv_co_pwritev(BdrvChild *child,
     int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
     BdrvRequestFlags flags);
+void *coroutine_fn bdrv_co_try_lock(BdrvChild *child,
+                                    int64_t offset, unsigned int bytes);
+void coroutine_fn bdrv_co_unlock(void *opaque);
 
 extern unsigned int bdrv_drain_all_count;
 void bdrv_apply_subtree_drain(BdrvChild *child, BlockDriverState *new_parent);
diff --git a/block/io.c b/block/io.c
index d4e46cb3dc..fd4ec53167 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3243,3 +3243,38 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, PreallocMode prealloc,
 
     return tco.ret;
 }
+
+void *coroutine_fn bdrv_co_try_lock(BdrvChild *child,
+                                    int64_t offset, unsigned int bytes)
+{
+    BlockDriverState *bs = child->bs;
+    BdrvTrackedRequest *req;
+
+    qemu_co_mutex_lock(&bs->reqs_lock);
+
+    QLIST_FOREACH(req, &bs->tracked_requests, list) {
+        if (req->type == BDRV_TRACKED_READ) {
+            continue;
+        }
+        if (tracked_request_overlaps(req, offset, bytes)) {
+            qemu_co_mutex_unlock(&bs->reqs_lock);
+            return NULL;
+        }
+    }
+
+    qemu_co_mutex_unlock(&bs->reqs_lock);
+
+    req = g_new(BdrvTrackedRequest, 1);
+    tracked_request_begin(req, bs, offset, bytes, BDRV_TRACKED_READ);
+    mark_request_serialising(req, bdrv_get_cluster_size(bs));
+
+    return req;
+}
+
+void coroutine_fn bdrv_co_unlock(void *opaque)
+{
+    BdrvTrackedRequest *req = opaque;
+
+    tracked_request_end(req);
+    g_free(req);
+}
-- 
2.18.0

[Qemu-devel] [PATCH v4 10/11] block/backup: tiny refactor backup_job_create

[Vladimir Sementsov-Ogievskiy]

Move copy-bitmap find/create code. It's needed for the following
commit, as we'll need copy_bitmap before actual block job creation. Do
it in a separate commit to simplify review.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/backup.c | 67 ++++++++++++++++++++++++++++----------------------
 1 file changed, 38 insertions(+), 29 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 0b3fddeb6c..eda6f22318 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -561,6 +561,8 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     BlockDriverInfo bdi;
     BackupBlockJob *job = NULL;
     int ret;
+    int64_t cluster_size;
+    HBitmap *copy_bitmap = NULL;
 
     assert(bs);
     assert(target);
@@ -615,6 +617,33 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
         return NULL;
     }
 
+    /* If there is no backing file on the target, we cannot rely on COW if our
+     * backup cluster size is smaller than the target cluster size. Even for
+     * targets with a backing file, try to avoid COW if possible. */
+    ret = bdrv_get_info(target, &bdi);
+    if (ret == -ENOTSUP && !target->backing) {
+        /* Cluster size is not defined */
+        warn_report("The target block device doesn't provide "
+                    "information about the block size and it doesn't have a "
+                    "backing file. The default block size of %u bytes is "
+                    "used. If the actual block size of the target exceeds "
+                    "this default, the backup may be unusable",
+                    BACKUP_CLUSTER_SIZE_DEFAULT);
+        cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT;
+    } else if (ret < 0 && !target->backing) {
+        error_setg_errno(errp, -ret,
+            "Couldn't determine the cluster size of the target image, "
+            "which has no backing file");
+        error_append_hint(errp,
+            "Aborting, since this may create an unusable destination image\n");
+        return NULL;
+    } else if (ret < 0 && target->backing) {
+        /* Not fatal; just trudge on ahead. */
+        cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT;
+    } else {
+        cluster_size = MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
+    }
+
     len = bdrv_getlength(bs);
     if (len < 0) {
         error_setg_errno(errp, -len, "unable to get length for '%s'",
@@ -622,6 +651,8 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
         goto error;
     }
 
+    copy_bitmap = hbitmap_alloc(len, ctz32(cluster_size));
+
     /* job->len is fixed, so we can't allow resize */
     job = block_job_create(job_id, &backup_job_driver, txn, bs,
                            BLK_PERM_CONSISTENT_READ,
@@ -650,35 +681,9 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
 
     /* Detect image-fleecing (and similar) schemes */
     job->serialize_target_writes = bdrv_chain_contains(target, bs);
-
-    /* If there is no backing file on the target, we cannot rely on COW if our
-     * backup cluster size is smaller than the target cluster size. Even for
-     * targets with a backing file, try to avoid COW if possible. */
-    ret = bdrv_get_info(target, &bdi);
-    if (ret == -ENOTSUP && !target->backing) {
-        /* Cluster size is not defined */
-        warn_report("The target block device doesn't provide "
-                    "information about the block size and it doesn't have a "
-                    "backing file. The default block size of %u bytes is "
-                    "used. If the actual block size of the target exceeds "
-                    "this default, the backup may be unusable",
-                    BACKUP_CLUSTER_SIZE_DEFAULT);
-        job->cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT;
-    } else if (ret < 0 && !target->backing) {
-        error_setg_errno(errp, -ret,
-            "Couldn't determine the cluster size of the target image, "
-            "which has no backing file");
-        error_append_hint(errp,
-            "Aborting, since this may create an unusable destination image\n");
-        goto error;
-    } else if (ret < 0 && target->backing) {
-        /* Not fatal; just trudge on ahead. */
-        job->cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT;
-    } else {
-        job->cluster_size = MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
-    }
-
-    job->copy_bitmap = hbitmap_alloc(len, ctz32(job->cluster_size));
+    job->cluster_size = cluster_size;
+    job->copy_bitmap = copy_bitmap;
+    copy_bitmap = NULL;
     job->use_copy_range = true;
     job->copy_range_size = MIN_NON_ZERO(blk_get_max_transfer(job->common.blk),
                                         blk_get_max_transfer(job->target));
@@ -694,6 +699,10 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     return &job->common;
 
  error:
+    if (copy_bitmap) {
+        assert(!job || !job->copy_bitmap);
+        hbitmap_free(copy_bitmap);
+    }
     if (sync_bitmap) {
         bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
     }
-- 
2.18.0

[Qemu-devel] [PATCH v4 11/11] block/backup: use backup-top instead of write notifiers

[Vladimir Sementsov-Ogievskiy]

Drop write notifiers and use filter node instead. Changes:

1. copy-before-writes now handled by filter node, so, drop all
   is_write_notifier arguments.

2. we don't have intersecting requests, so their handling is dropped.
Instead, synchronization works as follows:
when backup or backup-top starts copying of some area it firstly
clears copy-bitmap bits, and nobody touches areas, not marked with
dirty bits in copy-bitmap, so there is no intersection. Also, backup
job copy operations are surrounded by bdrv region lock, which is
actually serializing request, to not interfer with guest writes and
not read changed data from source (before reading we clear
corresponding bit in copy-bitmap, so, this area is not more handled by
backup-top).

3. To sync with in-flight requests we now just drain hook node, we
don't need rw-lock.

4. After the whole backup loop (top, full, incremental modes), we need
to check for not copied clusters, which were under backup-top operation
and we skipped them, but backup-top operation failed, error returned to
the guest and dirty bits set back.

5. Don't create additional blk, use backup-top children for copy
operations.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/backup.c | 278 +++++++++++++++++++++++++------------------------
 1 file changed, 142 insertions(+), 136 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index eda6f22318..6a2f520b28 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -26,105 +26,61 @@
 #include "qemu/bitmap.h"
 #include "qemu/error-report.h"
 
-#define BACKUP_CLUSTER_SIZE_DEFAULT (1 << 16)
+#include "block/backup-top.h"
 
-typedef struct CowRequest {
-    int64_t start_byte;
-    int64_t end_byte;
-    QLIST_ENTRY(CowRequest) list;
-    CoQueue wait_queue; /* coroutines blocked on this request */
-} CowRequest;
+#define BACKUP_CLUSTER_SIZE_DEFAULT (1 << 16)
 
 typedef struct BackupBlockJob {
     BlockJob common;
-    BlockBackend *target;
+    BdrvChild *source;
+    BdrvChild *target;
     /* bitmap for sync=incremental */
     BdrvDirtyBitmap *sync_bitmap;
     MirrorSyncMode sync_mode;
     BlockdevOnError on_source_error;
     BlockdevOnError on_target_error;
-    CoRwlock flush_rwlock;
     uint64_t len;
     uint64_t bytes_read;
     int64_t cluster_size;
     bool compress;
-    NotifierWithReturn before_write;
-    QLIST_HEAD(, CowRequest) inflight_reqs;
 
     HBitmap *copy_bitmap;
     bool use_copy_range;
     int64_t copy_range_size;
 
     bool serialize_target_writes;
+
+    BlockDriverState *backup_top;
+    uint64_t backup_top_progress;
 } BackupBlockJob;
 
 static const BlockJobDriver backup_job_driver;
 
-/* See if in-flight requests overlap and wait for them to complete */
-static void coroutine_fn wait_for_overlapping_requests(BackupBlockJob *job,
-                                                       int64_t start,
-                                                       int64_t end)
-{
-    CowRequest *req;
-    bool retry;
-
-    do {
-        retry = false;
-        QLIST_FOREACH(req, &job->inflight_reqs, list) {
-            if (end > req->start_byte && start < req->end_byte) {
-                qemu_co_queue_wait(&req->wait_queue, NULL);
-                retry = true;
-                break;
-            }
-        }
-    } while (retry);
-}
-
-/* Keep track of an in-flight request */
-static void cow_request_begin(CowRequest *req, BackupBlockJob *job,
-                              int64_t start, int64_t end)
-{
-    req->start_byte = start;
-    req->end_byte = end;
-    qemu_co_queue_init(&req->wait_queue);
-    QLIST_INSERT_HEAD(&job->inflight_reqs, req, list);
-}
-
-/* Forget about a completed request */
-static void cow_request_end(CowRequest *req)
-{
-    QLIST_REMOVE(req, list);
-    qemu_co_queue_restart_all(&req->wait_queue);
-}
-
 /* Copy range to target with a bounce buffer and return the bytes copied. If
  * error occurred, return a negative error number */
 static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,
                                                       int64_t start,
                                                       int64_t end,
-                                                      bool is_write_notifier,
                                                       bool *error_is_read,
                                                       void **bounce_buffer)
 {
     int ret;
     struct iovec iov;
     QEMUIOVector qiov;
-    BlockBackend *blk = job->common.blk;
     int nbytes;
-    int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
     int write_flags = job->serialize_target_writes ? BDRV_REQ_SERIALISING : 0;
 
     assert(QEMU_IS_ALIGNED(start, job->cluster_size));
     hbitmap_reset(job->copy_bitmap, start, job->cluster_size);
     nbytes = MIN(job->cluster_size, job->len - start);
     if (!*bounce_buffer) {
-        *bounce_buffer = blk_blockalign(blk, job->cluster_size);
+        *bounce_buffer = qemu_blockalign(job->source->bs, job->cluster_size);
     }
     iov.iov_base = *bounce_buffer;
     iov.iov_len = nbytes;
     qemu_iovec_init_external(&qiov, &iov, 1);
 
-    ret = blk_co_preadv(blk, start, qiov.size, &qiov, read_flags);
+    ret = bdrv_co_preadv(job->source, start, qiov.size, &qiov, 0);
     if (ret < 0) {
         trace_backup_do_cow_read_fail(job, start, ret);
         if (error_is_read) {
@@ -134,12 +90,12 @@ static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,
     }
 
     if (qemu_iovec_is_zero(&qiov)) {
-        ret = blk_co_pwrite_zeroes(job->target, start,
-                                   qiov.size, write_flags | BDRV_REQ_MAY_UNMAP);
+        ret = bdrv_co_pwrite_zeroes(job->target, start, qiov.size,
+                                    write_flags | BDRV_REQ_MAY_UNMAP);
     } else {
-        ret = blk_co_pwritev(job->target, start,
-                             qiov.size, &qiov, write_flags |
-                             (job->compress ? BDRV_REQ_WRITE_COMPRESSED : 0));
+        ret = bdrv_co_pwritev(job->target, start,
+                              qiov.size, &qiov, write_flags |
+                              (job->compress ? BDRV_REQ_WRITE_COMPRESSED : 0));
     }
     if (ret < 0) {
         trace_backup_do_cow_write_fail(job, start, ret);
@@ -159,15 +115,11 @@ fail:
 /* Copy range to target and return the bytes copied. If error occurred, return a
  * negative error number. */
 static int coroutine_fn backup_cow_with_offload(BackupBlockJob *job,
-                                                int64_t start,
-                                                int64_t end,
-                                                bool is_write_notifier)
+                                                int64_t start, int64_t end)
 {
     int ret;
     int nr_clusters;
-    BlockBackend *blk = job->common.blk;
     int nbytes;
-    int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
     int write_flags = job->serialize_target_writes ? BDRV_REQ_SERIALISING : 0;
 
     assert(QEMU_IS_ALIGNED(job->copy_range_size, job->cluster_size));
@@ -175,8 +127,8 @@ static int coroutine_fn backup_cow_with_offload(BackupBlockJob *job,
     nbytes = MIN(job->copy_range_size, end - start);
     nr_clusters = DIV_ROUND_UP(nbytes, job->cluster_size);
     hbitmap_reset(job->copy_bitmap, start, job->cluster_size * nr_clusters);
-    ret = blk_co_copy_range(blk, start, job->target, start, nbytes,
-                            read_flags, write_flags);
+    ret = bdrv_co_copy_range(job->source, start, job->target, start, nbytes,
+                             0, write_flags);
     if (ret < 0) {
         trace_backup_do_cow_copy_range_fail(job, start, ret);
         hbitmap_set(job->copy_bitmap, start, job->cluster_size * nr_clusters);
@@ -188,24 +140,18 @@ static int coroutine_fn backup_cow_with_offload(BackupBlockJob *job,
 
 static int coroutine_fn backup_do_cow(BackupBlockJob *job,
                                       int64_t offset, uint64_t bytes,
-                                      bool *error_is_read,
-                                      bool is_write_notifier)
+                                      bool *error_is_read)
 {
-    CowRequest cow_request;
     int ret = 0;
     int64_t start, end; /* bytes */
     void *bounce_buffer = NULL;
-
-    qemu_co_rwlock_rdlock(&job->flush_rwlock);
+    uint64_t backup_top_progress;
 
     start = QEMU_ALIGN_DOWN(offset, job->cluster_size);
     end = QEMU_ALIGN_UP(bytes + offset, job->cluster_size);
 
     trace_backup_do_cow_enter(job, start, offset, bytes);
 
-    wait_for_overlapping_requests(job, start, end);
-    cow_request_begin(&cow_request, job, start, end);
-
     while (start < end) {
         if (!hbitmap_get(job->copy_bitmap, start)) {
             trace_backup_do_cow_skip(job, start);
@@ -216,13 +162,13 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
         trace_backup_do_cow_process(job, start);
 
         if (job->use_copy_range) {
-            ret = backup_cow_with_offload(job, start, end, is_write_notifier);
+            ret = backup_cow_with_offload(job, start, end);
             if (ret < 0) {
                 job->use_copy_range = false;
             }
         }
         if (!job->use_copy_range) {
-            ret = backup_cow_with_bounce_buffer(job, start, end, is_write_notifier,
+            ret = backup_cow_with_bounce_buffer(job, start, end,
                                                 error_is_read, &bounce_buffer);
         }
         if (ret < 0) {
@@ -234,7 +180,10 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
          */
         start += ret;
         job->bytes_read += ret;
-        job_progress_update(&job->common.job, ret);
+        backup_top_progress = bdrv_backup_top_progress(job->backup_top);
+        job_progress_update(&job->common.job, ret + backup_top_progress -
+                            job->backup_top_progress);
+        job->backup_top_progress = backup_top_progress;
         ret = 0;
     }
 
@@ -242,33 +191,15 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
         qemu_vfree(bounce_buffer);
     }
 
-    cow_request_end(&cow_request);
-
     trace_backup_do_cow_return(job, offset, bytes, ret);
 
-    qemu_co_rwlock_unlock(&job->flush_rwlock);
-
     return ret;
 }
 
-static int coroutine_fn backup_before_write_notify(
-        NotifierWithReturn *notifier,
-        void *opaque)
-{
-    BackupBlockJob *job = container_of(notifier, BackupBlockJob, before_write);
-    BdrvTrackedRequest *req = opaque;
-
-    assert(req->bs == blk_bs(job->common.blk));
-    assert(QEMU_IS_ALIGNED(req->offset, BDRV_SECTOR_SIZE));
-    assert(QEMU_IS_ALIGNED(req->bytes, BDRV_SECTOR_SIZE));
-
-    return backup_do_cow(job, req->offset, req->bytes, NULL, true);
-}
-
 static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
 {
     BdrvDirtyBitmap *bm;
-    BlockDriverState *bs = blk_bs(job->common.blk);
+    BlockDriverState *bs = job->source->bs;
 
     if (ret < 0) {
         /* Merge the successor back into the parent, delete nothing. */
@@ -300,21 +231,23 @@ static void backup_abort(Job *job)
 static void backup_clean(Job *job)
 {
     BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
-    assert(s->target);
-    blk_unref(s->target);
+
+    /* We must clean it to not crash in backup_drain. */
     s->target = NULL;
 
     if (s->copy_bitmap) {
         hbitmap_free(s->copy_bitmap);
         s->copy_bitmap = NULL;
     }
+
+    bdrv_backup_top_drop(s->backup_top);
 }
 
 static void backup_attached_aio_context(BlockJob *job, AioContext *aio_context)
 {
     BackupBlockJob *s = container_of(job, BackupBlockJob, common);
 
-    blk_set_aio_context(s->target, aio_context);
+    bdrv_set_aio_context(s->target->bs, aio_context);
 }
 
 void backup_do_checkpoint(BlockJob *job, Error **errp)
@@ -340,10 +273,10 @@ static void backup_drain(BlockJob *job)
      * of backup_complete...
      */
     if (s->target) {
-        BlockBackend *target = s->target;
-        blk_ref(target);
-        blk_drain(target);
-        blk_unref(target);
+        BlockDriverState *target = s->target->bs;
+        bdrv_ref(target);
+        bdrv_drain(target);
+        bdrv_unref(target);
     }
 }
 
@@ -386,21 +319,43 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
     bool error_is_read;
     int64_t offset;
     HBitmapIter hbi;
+    void *lock = NULL;
 
     hbitmap_iter_init(&hbi, job->copy_bitmap, 0);
-    while ((offset = hbitmap_iter_next(&hbi)) != -1) {
+    while (hbitmap_count(job->copy_bitmap)) {
+        offset = hbitmap_iter_next(&hbi);
+        if (offset == -1) {
+            /* we may have skipped some clusters, which were handled by
+             * backup-top, but failed and finished by returning error to
+             * the guest and set dirty bit back.
+             */
+            hbitmap_iter_init(&hbi, job->copy_bitmap, 0);
+            offset = hbitmap_iter_next(&hbi);
+            assert(offset);
+        }
+
+        lock = bdrv_co_try_lock(job->source, offset, job->cluster_size);
+        /* Dirty bit is set, which means that there are no in-flight
+         * write requests on this area. We must succeed.
+         */
+        assert(lock);
+
         do {
             if (yield_and_check(job)) {
+                bdrv_co_unlock(lock);
                 return 0;
             }
-            ret = backup_do_cow(job, offset,
-                                job->cluster_size, &error_is_read, false);
+            ret = backup_do_cow(job, offset, job->cluster_size, &error_is_read);
             if (ret < 0 && backup_error_action(job, error_is_read, -ret) ==
                            BLOCK_ERROR_ACTION_REPORT)
             {
+                bdrv_co_unlock(lock);
                 return ret;
             }
         } while (ret < 0);
+
+        bdrv_co_unlock(lock);
+        lock = NULL;
     }
 
     return 0;
@@ -432,12 +387,10 @@ static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)
 static int coroutine_fn backup_run(Job *job, Error **errp)
 {
     BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
-    BlockDriverState *bs = blk_bs(s->common.blk);
+    BlockDriverState *bs = s->source->bs;
     int64_t offset;
     int ret = 0;
-
-    QLIST_INIT(&s->inflight_reqs);
-    qemu_co_rwlock_init(&s->flush_rwlock);
+    uint64_t backup_top_progress;
 
     job_progress_set_remaining(job, s->len);
 
@@ -447,26 +400,37 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
         hbitmap_set(s->copy_bitmap, 0, s->len);
     }
 
-    s->before_write.notify = backup_before_write_notify;
-    bdrv_add_before_write_notifier(bs, &s->before_write);
-
     if (s->sync_mode == MIRROR_SYNC_MODE_NONE) {
         /* All bits are set in copy_bitmap to allow any cluster to be copied.
          * This does not actually require them to be copied. */
         while (!job_is_cancelled(job)) {
-            /* Yield until the job is cancelled.  We just let our before_write
-             * notify callback service CoW requests. */
+            /* Yield until the job is cancelled.  We just let our backup-top
+             * fileter driver service CbW requests. */
             job_yield(job);
         }
     } else if (s->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
         ret = backup_run_incremental(s);
     } else {
+        bool retry;
+        void *lock;
+
+iteration:
+        retry = false;
+        lock = NULL;
+
         /* Both FULL and TOP SYNC_MODE's require copying.. */
         for (offset = 0; offset < s->len;
              offset += s->cluster_size) {
             bool error_is_read;
             int alloced = 0;
 
+            if (retry) {
+                retry = false;
+            } else if (lock) {
+                bdrv_co_unlock(lock);
+                lock = NULL;
+            }
+
             if (yield_and_check(s)) {
                 break;
             }
@@ -498,6 +462,7 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
                 /* If the above loop never found any sectors that are in
                  * the topmost image, skip this backup. */
                 if (alloced == 0) {
+                    hbitmap_reset(s->copy_bitmap, offset, s->cluster_size);
                     continue;
                 }
             }
@@ -505,8 +470,19 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
             if (alloced < 0) {
                 ret = alloced;
             } else {
+                if (!hbitmap_get(s->copy_bitmap, offset)) {
+                    trace_backup_do_cow_skip(job, offset);
+                    continue; /* already copied */
+                }
+                if (!lock) {
+                    lock = bdrv_co_try_lock(s->source, offset, s->cluster_size);
+                    /* Dirty bit is set, which means that there are no in-flight
+                     * write requests on this area. We must succeed.
+                     */
+                    assert(lock);
+                }
                 ret = backup_do_cow(s, offset, s->cluster_size,
-                                    &error_is_read, false);
+                                    &error_is_read);
             }
             if (ret < 0) {
                 /* Depending on error action, fail now or retry cluster */
@@ -516,17 +492,33 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
                     break;
                 } else {
                     offset -= s->cluster_size;
+                    retry = true;
                     continue;
                 }
             }
         }
+        if (lock) {
+            bdrv_co_unlock(lock);
+            lock = NULL;
+        }
+        if (ret == 0 && !job_is_cancelled(job) &&
+            hbitmap_count(s->copy_bitmap))
+        {
+            /* we may have skipped some clusters, which were handled by
+             * backup-top, but failed and finished by returning error to
+             * the guest and set dirty bit back.
+             */
+            goto iteration;
+        }
     }
 
-    notifier_with_return_remove(&s->before_write);
+    /* wait pending CBW operations in backup-top */
+    bdrv_drain(s->backup_top);
 
-    /* wait until pending backup_do_cow() calls have completed */
-    qemu_co_rwlock_wrlock(&s->flush_rwlock);
-    qemu_co_rwlock_unlock(&s->flush_rwlock);
+    backup_top_progress = bdrv_backup_top_progress(s->backup_top);
+    job_progress_update(job, ret + backup_top_progress -
+                        s->backup_top_progress);
+    s->backup_top_progress = backup_top_progress;
 
     return ret;
 }
@@ -563,6 +555,9 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     int ret;
     int64_t cluster_size;
     HBitmap *copy_bitmap = NULL;
+    BlockDriverState *backup_top;
+    uint64_t all_except_resize = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE |
+                                 BLK_PERM_WRITE_UNCHANGED | BLK_PERM_GRAPH_MOD;
 
     assert(bs);
     assert(target);
@@ -653,24 +648,29 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
 
     copy_bitmap = hbitmap_alloc(len, ctz32(cluster_size));
 
+    /* bdrv_get_device_name will not help to find device name starting from
+     * @bs after backup-top append, so let's calculate job_id before. Do
+     * it in the same way like block_job_create
+     */
+    if (job_id == NULL && !(creation_flags & JOB_INTERNAL)) {
+        job_id = bdrv_get_device_name(bs);
+    }
+
+    backup_top = bdrv_backup_top_append(bs, target, copy_bitmap, errp);
+    if (!backup_top) {
+        return NULL;
+    }
+
     /* job->len is fixed, so we can't allow resize */
-    job = block_job_create(job_id, &backup_job_driver, txn, bs,
-                           BLK_PERM_CONSISTENT_READ,
-                           BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE |
-                           BLK_PERM_WRITE_UNCHANGED | BLK_PERM_GRAPH_MOD,
-                           speed, creation_flags, cb, opaque, errp);
+    job = block_job_create(job_id, &backup_job_driver, txn, bs, 0,
+                           all_except_resize, speed, creation_flags,
+                           cb, opaque, errp);
     if (!job) {
         goto error;
     }
 
-    /* The target must match the source in size, so no resize here either */
-    job->target = blk_new(BLK_PERM_WRITE,
-                          BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE |
-                          BLK_PERM_WRITE_UNCHANGED | BLK_PERM_GRAPH_MOD);
-    ret = blk_insert_bs(job->target, target, errp);
-    if (ret < 0) {
-        goto error;
-    }
+    job->source = backup_top->backing;
+    job->target = ((BDRVBackupTopState *)backup_top->opaque)->target;
 
     job->on_source_error = on_source_error;
     job->on_target_error = on_target_error;
@@ -685,16 +685,19 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     job->copy_bitmap = copy_bitmap;
     copy_bitmap = NULL;
     job->use_copy_range = true;
-    job->copy_range_size = MIN_NON_ZERO(blk_get_max_transfer(job->common.blk),
-                                        blk_get_max_transfer(job->target));
+    job->copy_range_size =
+            MIN_NON_ZERO(MIN_NON_ZERO(INT_MAX,
+                                      job->source->bs->bl.max_transfer),
+                         job->target->bs->bl.max_transfer);
     job->copy_range_size = MAX(job->cluster_size,
                                QEMU_ALIGN_UP(job->copy_range_size,
                                              job->cluster_size));
 
-    /* Required permissions are already taken with target's blk_new() */
-    block_job_add_bdrv(&job->common, "target", target, 0, BLK_PERM_ALL,
+    /* The target must match the source in size, so no resize here either */
+    block_job_add_bdrv(&job->common, "target", target, 0, all_except_resize,
                        &error_abort);
     job->len = len;
+    job->backup_top = backup_top;
 
     return &job->common;
 
@@ -710,6 +713,9 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
         backup_clean(&job->common.job);
         job_early_fail(&job->common.job);
     }
+    if (backup_top) {
+        bdrv_backup_top_drop(backup_top);
+    }
 
     return NULL;
 }
-- 
2.18.0

[Qemu-devel] ping Re: [PATCH v4 00/11] backup-top filter driver for backup

[Vladimir Sementsov-Ogievskiy]

ping

15.10.2018 19:06, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> These series introduce backup-top driver. It's a filter-node, which
> do copy-before-write operation. Mirror uses filter-node for handling
> guest writes, let's move to filter-node (from write-notifiers) for
> backup too (patch 16)
>
> v4:
> fixes, rewrite driver to be implicit, drop new interfaces and
> don't move to BdrvDirtyBitmap for now, as it's not obvious will
> it be really needed and don't relate to these series more.
>
> v3 was "[PATCH v3 00/18] fleecing-hook driver for backup"
>
> v2 was "[RFC v2] new, node-graph-based fleecing and backup"
>
> These series are based on
>   [PATCH v4 0/8] dirty-bitmap: rewrite bdrv_dirty_iter_next_area
> and
>   [PATCH 0/2] replication: drop extra sync
>
> Based-on: <20180919124343.28206-1-vsementsov@virtuozzo.com>
> Based-on: <20180917145732.48590-1-vsementsov@virtuozzo.com>
>
> Vladimir Sementsov-Ogievskiy (11):
>    block/backup: simplify backup_incremental_init_copy_bitmap
>    block/backup: move to copy_bitmap with granularity
>    block: allow serialized reads to intersect
>    block: improve should_update_child
>    iotests: handle -f argument correctly for qemu_io_silent
>    iotests: allow resume_drive by node name
>    iotests: prepare 055 to graph changes during backup job
>    block: introduce backup-top filter driver
>    block: add lock/unlock range functions
>    block/backup: tiny refactor backup_job_create
>    block/backup: use backup-top instead of write notifiers
>
>   block/backup-top.h            |  44 ++++
>   include/block/block_int.h     |   3 +
>   block.c                       |  32 ++-
>   block/backup-top.c            | 298 ++++++++++++++++++++++++
>   block/backup.c                | 415 +++++++++++++++++-----------------
>   block/io.c                    |  38 +++-
>   block/Makefile.objs           |   2 +
>   tests/qemu-iotests/055        |  23 +-
>   tests/qemu-iotests/iotests.py |  16 +-
>   9 files changed, 641 insertions(+), 230 deletions(-)
>   create mode 100644 block/backup-top.h
>   create mode 100644 block/backup-top.c
>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] ping Re: [PATCH v4 00/11] backup-top filter driver for backup

[Kevin Wolf]

Am 02.11.2018 um 17:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
> ping
> 
> 15.10.2018 19:06, Vladimir Sementsov-Ogievskiy wrote:
> > Hi all!
> >
> > These series introduce backup-top driver. It's a filter-node, which
> > do copy-before-write operation. Mirror uses filter-node for handling
> > guest writes, let's move to filter-node (from write-notifiers) for
> > backup too (patch 16)
> >
> > v4:
> > fixes, rewrite driver to be implicit, drop new interfaces and
> > don't move to BdrvDirtyBitmap for now, as it's not obvious will
> > it be really needed and don't relate to these series more.
> >
> > v3 was "[PATCH v3 00/18] fleecing-hook driver for backup"
> >
> > v2 was "[RFC v2] new, node-graph-based fleecing and backup"
> >
> > These series are based on
> >   [PATCH v4 0/8] dirty-bitmap: rewrite bdrv_dirty_iter_next_area
> > and
> >   [PATCH 0/2] replication: drop extra sync

Before we can move forward here, we need to deal with the dependencies.
I merged the second one now (because there wasn't any feedback from the
replication maintainers), but the first one looks like it was going
through John's or Eric's tree?

Kevin

Re: [Qemu-devel] [PATCH v4 11/11] block/backup: use backup-top instead of write notifiers

[Kevin Wolf]

Am 15.10.2018 um 18:06 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Drop write notifiers and use filter node instead. Changes:
> 
> 1. copy-before-writes now handled by filter node, so, drop all
>    is_write_notifier arguments.
> 
> 2. we don't have intersecting requests, so their handling is dropped.
> Instead, synchronization works as follows:
> when backup or backup-top starts copying of some area it firstly
> clears copy-bitmap bits, and nobody touches areas, not marked with
> dirty bits in copy-bitmap, so there is no intersection. Also, backup
> job copy operations are surrounded by bdrv region lock, which is
> actually serializing request, to not interfer with guest writes and
> not read changed data from source (before reading we clear
> corresponding bit in copy-bitmap, so, this area is not more handled by
> backup-top).
> 
> 3. To sync with in-flight requests we now just drain hook node, we
> don't need rw-lock.
> 
> 4. After the whole backup loop (top, full, incremental modes), we need
> to check for not copied clusters, which were under backup-top operation
> and we skipped them, but backup-top operation failed, error returned to
> the guest and dirty bits set back.
> 
> 5. Don't create additional blk, use backup-top children for copy
> operations.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Haven't reviewed it yet, but gcc complains correctly here:

block/backup.c: In function 'backup_job_create':
block/backup.c:717:9: error: 'backup_top' may be used uninitialized in this function [-Werror=maybe-uninitialized]
         bdrv_backup_top_drop(backup_top);
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

> @@ -653,24 +648,29 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
>  
>      copy_bitmap = hbitmap_alloc(len, ctz32(cluster_size));
>  
> +    /* bdrv_get_device_name will not help to find device name starting from
> +     * @bs after backup-top append, so let's calculate job_id before. Do
> +     * it in the same way like block_job_create
> +     */
> +    if (job_id == NULL && !(creation_flags & JOB_INTERNAL)) {
> +        job_id = bdrv_get_device_name(bs);
> +    }
> +
> +    backup_top = bdrv_backup_top_append(bs, target, copy_bitmap, errp);
> +    if (!backup_top) {
> +        return NULL;

This needs to be goto error, just like in the bdrv_getlength() failure a
few lines above (which is where the uninitialised backup_top warning
comes from).

Kevin

Re: [Qemu-devel] [PATCH v4 03/11] block: allow serialized reads to intersect

[Kevin Wolf]

Am 15.10.2018 um 18:06 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Otherwise, if we have serialized read-part in copy_range from backing
> file to its parent if CoW take place, this CoW's sub-reads will
> intersect with firstly created serialized read request.
> 
> Anyway, reads should not influence on disk view, let's allow them to
> intersect.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

What about reads that internally trigger writes, such as copy-on-read?

Kevin

Re: [Qemu-devel] [PATCH v4 04/11] block: improve should_update_child

[Kevin Wolf]

Am 15.10.2018 um 18:06 hat Vladimir Sementsov-Ogievskiy geschrieben:
> As it already said in the comment, we don't want to create loops in
> parent->child relations. So, when we try to append @to to @c, we should
> check that @c is not in @to children subtree, and we should check it
> recursively, not only the first level. The patch provides BFS-based
> search, to check the relations.
> 
> This is needed for further fleecing-hook filter usage: we need to
> append it to source, when the hook is already a parent of target, and
> source may be in a backing chain of target (fleecing-scheme). So, on
> appending, the hook should not became a child (direct or through
> children subtree) of the target.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block.c | 32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/block.c b/block.c
> index c298ca6a19..7f605b0bf0 100644
> --- a/block.c
> +++ b/block.c
> @@ -3432,7 +3432,7 @@ void bdrv_close_all(void)
>  
>  static bool should_update_child(BdrvChild *c, BlockDriverState *to)
>  {
> -    BdrvChild *to_c;
> +    GList *queue = NULL, *pos;
>  
>      if (c->role->stay_at_node) {
>          return false;
> @@ -3468,13 +3468,35 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to)
>       * if A is a child of B, that means we cannot replace A by B there
>       * because that would create a loop.  Silently detaching A from B
>       * is also not really an option.  So overall just leaving A in
> -     * place there is the most sensible choice. */
> -    QLIST_FOREACH(to_c, &to->children, next) {
> -        if (to_c == c) {
> -            return false;
> +     * place there is the most sensible choice.
> +     *
> +     * upd: If the child @c belongs to the @to's children, or children of it's
> +     * children and so on - this would create a loop to. To prevent it let's do
> +     * a BFS search on @to children subtree.
> +     */

I don't like the "upd:" thing. Let me try to rephrase the addition:

    We would also create a loop in any cases where @c is only indirectly
    referenced by @to. Prevent this by returning false if @c is found
    (by breadth-first search) anywhere in the whole subtree of @to.

Also, even though the coding style says 80 characters per line, the
existing comment seems to use only 70 characters. Let's try to stay
consistent here, otherwise it looks a bit odd.

> +    pos = queue = g_list_append(queue, to);
> +    while (pos) {
> +        BlockDriverState *v = pos->data;
> +        BdrvChild *c2;
> +
> +        QLIST_FOREACH(c2, &v->children, next) {
> +            if (c2 == c) {
> +                g_list_free(queue);
> +                return false;
> +            }
> +
> +            if (g_list_find(queue, c2->bs)) {
> +                continue;
> +            }
> +
> +            queue = g_list_append(queue, c2->bs);
>          }
> +
> +        pos = pos->next;
>      }
>  
> +    g_list_free(queue);
>      return true;
>  }

The functional change looks good to me.

Kevin

Re: [Qemu-devel] [Qemu-block] ping Re: [PATCH v4 00/11] backup-top filter driver for backup

[John Snow]

On 11/06/2018 12:21 PM, Kevin Wolf wrote:
> Am 02.11.2018 um 17:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> ping
>>
>> 15.10.2018 19:06, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi all!
>>>
>>> These series introduce backup-top driver. It's a filter-node, which
>>> do copy-before-write operation. Mirror uses filter-node for handling
>>> guest writes, let's move to filter-node (from write-notifiers) for
>>> backup too (patch 16)
>>>
>>> v4:
>>> fixes, rewrite driver to be implicit, drop new interfaces and
>>> don't move to BdrvDirtyBitmap for now, as it's not obvious will
>>> it be really needed and don't relate to these series more.
>>>
>>> v3 was "[PATCH v3 00/18] fleecing-hook driver for backup"
>>>
>>> v2 was "[RFC v2] new, node-graph-based fleecing and backup"
>>>
>>> These series are based on
>>>   [PATCH v4 0/8] dirty-bitmap: rewrite bdrv_dirty_iter_next_area
>>> and
>>>   [PATCH 0/2] replication: drop extra sync
> 
> Before we can move forward here, we need to deal with the dependencies.
> I merged the second one now (because there wasn't any feedback from the
> replication maintainers), but the first one looks like it was going
> through John's or Eric's tree?
> 

I was expecting it to go through Eric's because it was predicated on NBD
patches that he was staging.

Is that no longer true?
--js

Re: [Qemu-devel] [PATCH v4 03/11] block: allow serialized reads to intersect

[Vladimir Sementsov-Ogievskiy]

06.11.2018 20:57, Kevin Wolf wrote:
> Am 15.10.2018 um 18:06 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Otherwise, if we have serialized read-part in copy_range from backing
>> file to its parent if CoW take place, this CoW's sub-reads will
>> intersect with firstly created serialized read request.
>>
>> Anyway, reads should not influence on disk view, let's allow them to
>> intersect.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> What about reads that internally trigger writes, such as copy-on-read?

oops :). yes, I've to rethink this thing.

>
> Kevin


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [Qemu-block] ping Re: [PATCH v4 00/11] backup-top filter driver for backup

[Vladimir Sementsov-Ogievskiy]

07.11.2018 01:35, John Snow wrote:
>
> On 11/06/2018 12:21 PM, Kevin Wolf wrote:
>> Am 02.11.2018 um 17:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>> ping
>>>
>>> 15.10.2018 19:06, Vladimir Sementsov-Ogievskiy wrote:
>>>> Hi all!
>>>>
>>>> These series introduce backup-top driver. It's a filter-node, which
>>>> do copy-before-write operation. Mirror uses filter-node for handling
>>>> guest writes, let's move to filter-node (from write-notifiers) for
>>>> backup too (patch 16)
>>>>
>>>> v4:
>>>> fixes, rewrite driver to be implicit, drop new interfaces and
>>>> don't move to BdrvDirtyBitmap for now, as it's not obvious will
>>>> it be really needed and don't relate to these series more.
>>>>
>>>> v3 was "[PATCH v3 00/18] fleecing-hook driver for backup"
>>>>
>>>> v2 was "[RFC v2] new, node-graph-based fleecing and backup"
>>>>
>>>> These series are based on
>>>>    [PATCH v4 0/8] dirty-bitmap: rewrite bdrv_dirty_iter_next_area
>>>> and
>>>>    [PATCH 0/2] replication: drop extra sync
>> Before we can move forward here, we need to deal with the dependencies.
>> I merged the second one now (because there wasn't any feedback from the
>> replication maintainers), but the first one looks like it was going
>> through John's or Eric's tree?
>>
> I was expecting it to go through Eric's because it was predicated on NBD
> patches that he was staging.
>
> Is that no longer true?

yes, it's merged to master.

> --js


-- 
Best regards,
Vladimir