[Qemu-devel] [PATCH v3 00/18] fleecing-hook driver for backup

[Qemu-devel] [PATCH v3 00/18] fleecing-hook driver for backup

[Vladimir Sementsov-Ogievskiy]

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

Hi all!

These series introduce fleecing-hook 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 18)

Proposed filter driver is complete and separate: it can be used
standalone, as fleecing provider (instead of backup(sync=none)).
(old-style fleecing based on backup(sync=none) is supported too),
look at patch 16.

There a lot of other ideas and improvements which can be achieved
basing on these series which were discussed on v2 thread, for the
beginning I want to concentrate on the following:

done in these series:
 1. use filter node instead of write notifiers in backup
 2. filter-based fleecing scheme, without a job

near to be done (a test is needed and may be tiny adjustment)
 3. backup scheme for push backup with fleecing, to no disturb
    the guest with long handling of its writes (if target is far
    from remote NBD): just start fleecing to local qcow2 temp
    node and start a backup job (or mirror, why not?) from temp
    node to remote target. These series provide a possibility
    to share dirty bitmap between fleecing-hook and backup job
    to create efficient scenarios.

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 (18):
  block/dirty-bitmap: allow set/reset bits in disabled bitmaps
  block/io: allow BDRV_REQ_SERIALISING for read
  block/backup: simplify backup_incremental_init_copy_bitmap
  block/backup: move from HBitmap to BdrvDirtyBitmap
  util/id: add block-bitmap subsystem
  block/backup: give a name to copy-bitmap
  block/backup: allow use existent copy-bitmap
  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 new filter driver: fleecing-hook
  block/fleecing-hook: internal api
  qapi: add x-drop-fleecing qmp command
  iotests: test new fleecing-hook driver in context of 222 iotest
  block/backup: tiny refactor backup_job_create
  block/backup: use fleecing-hook instead of write notifiers

 qapi/block-core.json          |  57 +++++-
 include/block/block.h         |   9 +
 include/block/block_int.h     |   2 +-
 include/qemu/id.h             |   1 +
 block.c                       |  32 +++-
 block/backup.c                | 320 ++++++++++++++-----------------
 block/dirty-bitmap.c          |   2 -
 block/fleecing-hook.c         | 349 ++++++++++++++++++++++++++++++++++
 block/io.c                    |  16 +-
 block/replication.c           |   2 +-
 blockdev.c                    |  49 ++++-
 util/id.c                     |   1 +
 block/Makefile.objs           |   2 +
 tests/qemu-iotests/055        |  23 ++-
 tests/qemu-iotests/222        |  59 ++++--
 tests/qemu-iotests/222.out    |  66 +++++++
 tests/qemu-iotests/iotests.py |  16 +-
 17 files changed, 778 insertions(+), 228 deletions(-)
 create mode 100644 block/fleecing-hook.c

-- 
2.18.0

[Qemu-devel] [PATCH v3 01/18] block/dirty-bitmap: allow set/reset bits in disabled bitmaps

[Vladimir Sementsov-Ogievskiy]

It is needed for use the bitmaps in backup. "disabled" means that
bitmap is not auto set by writes. Let's allow changing bitmap for other
uses.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/dirty-bitmap.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 89c11111ae..65d2e92ec3 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -532,7 +532,6 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
 void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
                                   int64_t offset, int64_t bytes)
 {
-    assert(bdrv_dirty_bitmap_enabled(bitmap));
     assert(!bdrv_dirty_bitmap_readonly(bitmap));
     hbitmap_set(bitmap->bitmap, offset, bytes);
 }
@@ -549,7 +548,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
 void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
                                     int64_t offset, int64_t bytes)
 {
-    assert(bdrv_dirty_bitmap_enabled(bitmap));
     assert(!bdrv_dirty_bitmap_readonly(bitmap));
     hbitmap_reset(bitmap->bitmap, offset, bytes);
 }
-- 
2.18.0

Re: [Qemu-devel] [PATCH v3 01/18] block/dirty-bitmap: allow set/reset bits in disabled bitmaps

[Eric Blake]

On 10/1/18 5:29 AM, Vladimir Sementsov-Ogievskiy wrote:
> It is needed for use the bitmaps in backup. "disabled" means that
> bitmap is not auto set by writes. Let's allow changing bitmap for other
> uses.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/dirty-bitmap.c | 2 --
>   1 file changed, 2 deletions(-)

Looks like this duplicates John's series:
https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg00406.html

> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 89c11111ae..65d2e92ec3 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -532,7 +532,6 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
>   void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
>                                     int64_t offset, int64_t bytes)
>   {
> -    assert(bdrv_dirty_bitmap_enabled(bitmap));
>       assert(!bdrv_dirty_bitmap_readonly(bitmap));
>       hbitmap_set(bitmap->bitmap, offset, bytes);
>   }
> @@ -549,7 +548,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>   void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
>                                       int64_t offset, int64_t bytes)
>   {
> -    assert(bdrv_dirty_bitmap_enabled(bitmap));
>       assert(!bdrv_dirty_bitmap_readonly(bitmap));
>       hbitmap_reset(bitmap->bitmap, offset, bytes);
>   }
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v3 01/18] block/dirty-bitmap: allow set/reset bits in disabled bitmaps

[Vladimir Sementsov-Ogievskiy]

03.10.2018 17:23, Eric Blake wrote:
> On 10/1/18 5:29 AM, Vladimir Sementsov-Ogievskiy wrote:
>> It is needed for use the bitmaps in backup. "disabled" means that
>> bitmap is not auto set by writes. Let's allow changing bitmap for other
>> uses.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/dirty-bitmap.c | 2 --
>>   1 file changed, 2 deletions(-)
>
> Looks like this duplicates John's series:
> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg00406.html

No. But it relates.

>
>>
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 89c11111ae..65d2e92ec3 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -532,7 +532,6 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter 
>> *iter)
>>   void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
>>                                     int64_t offset, int64_t bytes)
>>   {
>> -    assert(bdrv_dirty_bitmap_enabled(bitmap));
>>       assert(!bdrv_dirty_bitmap_readonly(bitmap));
>>       hbitmap_set(bitmap->bitmap, offset, bytes);
>>   }
>> @@ -549,7 +548,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>>   void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
>>                                       int64_t offset, int64_t bytes)
>>   {
>> -    assert(bdrv_dirty_bitmap_enabled(bitmap));
>>       assert(!bdrv_dirty_bitmap_readonly(bitmap));
>>       hbitmap_reset(bitmap->bitmap, offset, bytes);
>>   }
>>
>


-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH v3 02/18] block/io: allow BDRV_REQ_SERIALISING for read

[Vladimir Sementsov-Ogievskiy]

It will be used in further commit, changing backup architecture.

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

diff --git a/block/io.c b/block/io.c
index bd9d688f8b..b003b4d5bf 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1318,10 +1318,11 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
      * potential fallback support, if we ever implement any read flags
      * to pass through to drivers.  For now, there aren't any
      * passthrough flags.  */
-    assert(!(flags & ~(BDRV_REQ_NO_SERIALISING | BDRV_REQ_COPY_ON_READ)));
+    assert(!(flags & ~(BDRV_REQ_NO_SERIALISING | BDRV_REQ_COPY_ON_READ |
+                       BDRV_REQ_SERIALISING)));
 
     /* Handle Copy on Read and associated serialisation */
-    if (flags & BDRV_REQ_COPY_ON_READ) {
+    if (flags & (BDRV_REQ_COPY_ON_READ | BDRV_REQ_SERIALISING)) {
         /* If we touch the same cluster it counts as an overlap.  This
          * guarantees that allocating writes will be serialized and not race
          * with each other for the same cluster.  For example, in copy-on-read
@@ -1330,9 +1331,6 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
         mark_request_serialising(req, bdrv_get_cluster_size(bs));
     }
 
-    /* BDRV_REQ_SERIALISING is only for write operation */
-    assert(!(flags & BDRV_REQ_SERIALISING));
-
     if (!(flags & BDRV_REQ_NO_SERIALISING)) {
         wait_serialising_requests(req);
     }
@@ -3027,8 +3025,9 @@ static int coroutine_fn bdrv_co_copy_range_internal(
         tracked_request_begin(&req, src->bs, src_offset, bytes,
                               BDRV_TRACKED_READ);
 
-        /* BDRV_REQ_SERIALISING is only for write operation */
-        assert(!(read_flags & BDRV_REQ_SERIALISING));
+        if (read_flags & BDRV_REQ_SERIALISING) {
+            mark_request_serialising(&req, bdrv_get_cluster_size(src->bs));
+        }
         if (!(read_flags & BDRV_REQ_NO_SERIALISING)) {
             wait_serialising_requests(&req);
         }
-- 
2.18.0

[Qemu-devel] [PATCH v3 03/18] 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 v3 04/18] block/backup: move from HBitmap to BdrvDirtyBitmap

[Vladimir Sementsov-Ogievskiy]

This is a necessary step to share copy-bitmap between backup job and
special filter driver in further commit.

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

diff --git a/block/backup.c b/block/backup.c
index fbe7ce19e1..45212d54c9 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -51,7 +51,7 @@ typedef struct BackupBlockJob {
     NotifierWithReturn before_write;
     QLIST_HEAD(, CowRequest) inflight_reqs;
 
-    HBitmap *copy_bitmap;
+    BdrvDirtyBitmap *copy_bitmap;
     bool use_copy_range;
     int64_t copy_range_size;
 
@@ -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(start % job->cluster_size == 0);
+    bdrv_reset_dirty_bitmap(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);
+    bdrv_set_dirty_bitmap(job->copy_bitmap, start, job->cluster_size);
     return ret;
 
 }
@@ -170,16 +171,17 @@ 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);
+    bdrv_reset_dirty_bitmap(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);
+        bdrv_set_dirty_bitmap(job->copy_bitmap, start,
+                              job->cluster_size * nr_clusters);
         return ret;
     }
 
@@ -207,7 +209,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 (!bdrv_get_dirty_locked(NULL, job->copy_bitmap, start)) {
             trace_backup_do_cow_skip(job, start);
             start += job->cluster_size;
             continue; /* already copied */
@@ -303,6 +305,11 @@ static void backup_clean(Job *job)
     assert(s->target);
     blk_unref(s->target);
     s->target = NULL;
+
+    if (s->copy_bitmap) {
+        bdrv_release_dirty_bitmap(blk_bs(s->common.blk), s->copy_bitmap);
+        s->copy_bitmap = NULL;
+    }
 }
 
 static void backup_attached_aio_context(BlockJob *job, AioContext *aio_context)
@@ -315,7 +322,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 +331,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);
+    bdrv_set_dirty_bitmap(backup_job->copy_bitmap, 0, backup_job->len);
 }
 
 static void backup_drain(BlockJob *job)
@@ -379,28 +384,30 @@ static bool coroutine_fn yield_and_check(BackupBlockJob *job)
 
 static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
 {
-    int ret;
+    int ret = 0;
     bool error_is_read;
-    int64_t cluster;
-    HBitmapIter hbi;
+    int64_t offset;
+    BdrvDirtyBitmapIter *dbi = bdrv_dirty_iter_new(job->copy_bitmap);
 
-    hbitmap_iter_init(&hbi, job->copy_bitmap, 0);
-    while ((cluster = hbitmap_iter_next(&hbi)) != -1) {
+    while ((offset = bdrv_dirty_iter_next(dbi)) != -1) {
         do {
             if (yield_and_check(job)) {
-                return 0;
+                goto out;
             }
-            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)
             {
-                return ret;
+                goto out;
             }
         } while (ret < 0);
     }
 
-    return 0;
+out:
+    bdrv_dirty_iter_free(dbi);
+
+    return ret;
 }
 
 /* init copy_bitmap from sync_bitmap */
@@ -412,12 +419,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;
+        bdrv_set_dirty_bitmap(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 +430,29 @@ 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 - bdrv_get_dirty_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);
+    bdrv_disable_dirty_bitmap(s->copy_bitmap);
+
     if (s->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
         backup_incremental_init_copy_bitmap(s);
     } else {
-        hbitmap_set(s->copy_bitmap, 0, nb_clusters);
+        bdrv_set_dirty_bitmap(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 +533,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 +683,13 @@ 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 = bdrv_create_dirty_bitmap(bs, job->cluster_size,
+                                                NULL, errp);
+    if (!job->copy_bitmap) {
+        goto error;
+    }
+
     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 v3 05/18] util/id: add block-bitmap subsystem

[Vladimir Sementsov-Ogievskiy]

Add block-bitmap subsystem to generate bitmap names in the following
commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/qemu/id.h | 1 +
 util/id.c         | 1 +
 2 files changed, 2 insertions(+)

diff --git a/include/qemu/id.h b/include/qemu/id.h
index 40c70103e4..013a560e01 100644
--- a/include/qemu/id.h
+++ b/include/qemu/id.h
@@ -4,6 +4,7 @@
 typedef enum IdSubSystems {
     ID_QDEV,
     ID_BLOCK,
+    ID_BLOCK_BITMAP,
     ID_MAX      /* last element, used as array size */
 } IdSubSystems;
 
diff --git a/util/id.c b/util/id.c
index 6141352955..beb4ee7f0b 100644
--- a/util/id.c
+++ b/util/id.c
@@ -34,6 +34,7 @@ bool id_wellformed(const char *id)
 static const char *const id_subsys_str[ID_MAX] = {
     [ID_QDEV]  = "qdev",
     [ID_BLOCK] = "block",
+    [ID_BLOCK_BITMAP] = "block-bitmap",
 };
 
 /*
-- 
2.18.0

[Qemu-devel] [PATCH v3 06/18] block/backup: give a name to copy-bitmap

[Vladimir Sementsov-Ogievskiy]

The bitmap should be named to be shared with special filter node in
further patches. Make it possible to set this name in qmp command.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block-core.json      | 18 ++++++++++++++++--
 include/block/block_int.h |  2 +-
 block/backup.c            | 10 ++++++++--
 block/replication.c       |  2 +-
 blockdev.c                | 12 ++++++++++--
 5 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index ac3b48ee54..c4774af18e 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1260,6 +1260,12 @@
 #          Must be present if sync is "incremental", must NOT be present
 #          otherwise. (Since 2.4)
 #
+# @x-copy-bitmap: the name for the copy-bitmap - the bitmap which drives the
+#                 backup process. At backup start it corresponds to @bitmap (or
+#                 just all bits set for full backup) and at the finish it
+#                 should be zero. If the bitmap already exists it will be used
+#                 as is. (Since 3.1)
+#
 # @compress: true to compress data, if the target format supports it.
 #            (default: false) (since 2.8)
 #
@@ -1297,7 +1303,8 @@
             '*bitmap': 'str', '*compress': 'bool',
             '*on-source-error': 'BlockdevOnError',
             '*on-target-error': 'BlockdevOnError',
-            '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
+            '*auto-finalize': 'bool', '*auto-dismiss': 'bool',
+            '*x-copy-bitmap': 'str' } }
 
 ##
 # @BlockdevBackup:
@@ -1340,6 +1347,12 @@
 #                list without user intervention.
 #                Defaults to true. (Since 2.12)
 #
+# @x-copy-bitmap: the name for the copy-bitmap - the bitmap which drives the
+#                 backup process. At backup start it corresponds to @bitmap (or
+#                 just all bits set for full backup) and at the finish it
+#                 should be zero. If the bitmap already exists it will be used
+#                 as is. (Since 3.1)
+#
 # Note: @on-source-error and @on-target-error only affect background
 # I/O.  If an error occurs during a guest write request, the device's
 # rerror/werror actions will be used.
@@ -1351,7 +1364,8 @@
             'sync': 'MirrorSyncMode', '*speed': 'int', '*compress': 'bool',
             '*on-source-error': 'BlockdevOnError',
             '*on-target-error': 'BlockdevOnError',
-            '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
+            '*auto-finalize': 'bool', '*auto-dismiss': 'bool',
+            '*x-copy-bitmap': 'str' } }
 
 ##
 # @blockdev-snapshot-sync:
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 92ecbd866e..13f4a0f98e 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1087,7 +1087,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
                             bool compress,
                             BlockdevOnError on_source_error,
                             BlockdevOnError on_target_error,
-                            int creation_flags,
+                            int creation_flags, const char *x_copy_bitmap,
                             BlockCompletionFunc *cb, void *opaque,
                             JobTxn *txn, Error **errp);
 
diff --git a/block/backup.c b/block/backup.c
index 45212d54c9..ad143ea735 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -25,6 +25,7 @@
 #include "sysemu/block-backend.h"
 #include "qemu/bitmap.h"
 #include "qemu/error-report.h"
+#include "qemu/id.h"
 
 #define BACKUP_CLUSTER_SIZE_DEFAULT (1 << 16)
 
@@ -559,7 +560,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
                   bool compress,
                   BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
-                  int creation_flags,
+                  int creation_flags, const char *x_copy_bitmap,
                   BlockCompletionFunc *cb, void *opaque,
                   JobTxn *txn, Error **errp)
 {
@@ -567,6 +568,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     BlockDriverInfo bdi;
     BackupBlockJob *job = NULL;
     int ret;
+    char *gen_bitmap_name = NULL;
 
     assert(bs);
     assert(target);
@@ -684,8 +686,12 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
         job->cluster_size = MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
     }
 
+    if (!x_copy_bitmap) {
+        x_copy_bitmap = gen_bitmap_name = id_generate(ID_BLOCK_BITMAP);
+    }
     job->copy_bitmap = bdrv_create_dirty_bitmap(bs, job->cluster_size,
-                                                NULL, errp);
+                                                x_copy_bitmap, errp);
+    g_free(gen_bitmap_name);
     if (!job->copy_bitmap) {
         goto error;
     }
diff --git a/block/replication.c b/block/replication.c
index 0c2989d2cb..f4ff0bb034 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -548,7 +548,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
         job = backup_job_create(NULL, s->secondary_disk->bs, s->hidden_disk->bs,
                                 0, MIRROR_SYNC_MODE_NONE, NULL, false,
                                 BLOCKDEV_ON_ERROR_REPORT,
-                                BLOCKDEV_ON_ERROR_REPORT, JOB_INTERNAL,
+                                BLOCKDEV_ON_ERROR_REPORT, JOB_INTERNAL, NULL,
                                 backup_job_completed, bs, NULL, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
diff --git a/blockdev.c b/blockdev.c
index a8755bd908..407e03d22a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3407,6 +3407,9 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
     if (!backup->has_compress) {
         backup->compress = false;
     }
+    if (!backup->has_x_copy_bitmap) {
+        backup->x_copy_bitmap = NULL;
+    }
 
     bs = qmp_get_root_bs(backup->device, errp);
     if (!bs) {
@@ -3511,7 +3514,8 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
     job = backup_job_create(backup->job_id, bs, target_bs, backup->speed,
                             backup->sync, bmap, backup->compress,
                             backup->on_source_error, backup->on_target_error,
-                            job_flags, NULL, NULL, txn, &local_err);
+                            job_flags, backup->x_copy_bitmap,
+                            NULL, NULL, txn, &local_err);
     bdrv_unref(target_bs);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
@@ -3569,6 +3573,9 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn,
     if (!backup->has_compress) {
         backup->compress = false;
     }
+    if (!backup->has_x_copy_bitmap) {
+        backup->x_copy_bitmap = NULL;
+    }
 
     bs = bdrv_lookup_bs(backup->device, backup->device, errp);
     if (!bs) {
@@ -3603,7 +3610,8 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn,
     job = backup_job_create(backup->job_id, bs, target_bs, backup->speed,
                             backup->sync, NULL, backup->compress,
                             backup->on_source_error, backup->on_target_error,
-                            job_flags, NULL, NULL, txn, &local_err);
+                            job_flags, backup->x_copy_bitmap,
+                            NULL, NULL, txn, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
     }
-- 
2.18.0

[Qemu-devel] [PATCH v3 07/18] block/backup: allow use existent copy-bitmap

[Vladimir Sementsov-Ogievskiy]

Allow use existent copy-bitmap to make it possible to synchronize with
externally created fleecing-hook filter which will be introduced soon.

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

diff --git a/block/backup.c b/block/backup.c
index ad143ea735..11aa31a323 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -53,6 +53,7 @@ typedef struct BackupBlockJob {
     QLIST_HEAD(, CowRequest) inflight_reqs;
 
     BdrvDirtyBitmap *copy_bitmap;
+    bool copy_bitmap_created;
     bool use_copy_range;
     int64_t copy_range_size;
 
@@ -307,7 +308,7 @@ static void backup_clean(Job *job)
     blk_unref(s->target);
     s->target = NULL;
 
-    if (s->copy_bitmap) {
+    if (s->copy_bitmap_created) {
         bdrv_release_dirty_bitmap(blk_bs(s->common.blk), s->copy_bitmap);
         s->copy_bitmap = NULL;
     }
@@ -686,11 +687,16 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
         job->cluster_size = MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
     }
 
-    if (!x_copy_bitmap) {
+    if (x_copy_bitmap) {
+        job->copy_bitmap = bdrv_find_dirty_bitmap(bs, x_copy_bitmap);
+    } else {
         x_copy_bitmap = gen_bitmap_name = id_generate(ID_BLOCK_BITMAP);
     }
-    job->copy_bitmap = bdrv_create_dirty_bitmap(bs, job->cluster_size,
-                                                x_copy_bitmap, errp);
+    if (!job->copy_bitmap) {
+        job->copy_bitmap = bdrv_create_dirty_bitmap(bs, job->cluster_size,
+                                                    x_copy_bitmap, errp);
+        job->copy_bitmap_created = !!job->copy_bitmap;
+    }
     g_free(gen_bitmap_name);
     if (!job->copy_bitmap) {
         goto error;
-- 
2.18.0

[Qemu-devel] [PATCH v3 08/18] 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 b003b4d5bf..6d611bfe59 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 v3 09/18] 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 v3 10/18] 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 v3 11/18] 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 v3 12/18] 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 v3 13/18] block: introduce new filter driver: fleecing-hook

[Vladimir Sementsov-Ogievskiy]

Fleecing-hook 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   +---------------+
    | Fleecing hook |---------->| target(qcow2) |
    +---+-----------+   CBW     +---+-----------+
        |                           |
backing |r,w                        |
        v                           |
    +---+---------+      backing    |
    | Active disk |<----------------+
    +-------------+        r

Target's backing may point to active disk (should be set up
separately), which gives fleecing-scheme.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block-core.json  |  22 +++-
 block/fleecing-hook.c | 298 ++++++++++++++++++++++++++++++++++++++++++
 block/Makefile.objs   |   2 +
 3 files changed, 320 insertions(+), 2 deletions(-)
 create mode 100644 block/fleecing-hook.c

diff --git a/qapi/block-core.json b/qapi/block-core.json
index c4774af18e..13cf90eab6 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2628,7 +2628,8 @@
             'host_cdrom', 'host_device', 'http', 'https', 'iscsi', 'luks',
             'nbd', 'nfs', 'null-aio', 'null-co', 'nvme', 'parallels', 'qcow',
             'qcow2', 'qed', 'quorum', 'raw', 'rbd', 'replication', 'sheepdog',
-            'ssh', 'throttle', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat', 'vxhs' ] }
+            'ssh', 'throttle', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat', 'vxhs',
+            'fleecing-hook'] }
 
 ##
 # @BlockdevOptionsFile:
@@ -2719,6 +2720,22 @@
 { 'struct': 'BlockdevOptionsGenericFormat',
   'data': { 'file': 'BlockdevRef' } }
 
+##
+# @BlockdevOptionsFleecingHook:
+#
+# Driver specific block device options for image format that have no option
+# besides their data source.
+#
+# @append-to:        reference to or definition of the data source block device
+# @target:        reference to or definition of the data source block device
+# @copy-bitmap:   name for the copy-bitmap of the process. May be shared TODO: normal description here
+#
+# Since: 2.9
+##
+  { 'struct': 'BlockdevOptionsFleecingHook',
+    'data': { 'append-to': 'str', 'target': 'BlockdevRef',
+              '*copy-bitmap': 'str'} }
+
 ##
 # @BlockdevOptionsLUKS:
 #
@@ -3718,7 +3735,8 @@
       'vmdk':       'BlockdevOptionsGenericCOWFormat',
       'vpc':        'BlockdevOptionsGenericFormat',
       'vvfat':      'BlockdevOptionsVVFAT',
-      'vxhs':       'BlockdevOptionsVxHS'
+      'vxhs':       'BlockdevOptionsVxHS',
+      'fleecing-hook': 'BlockdevOptionsFleecingHook'
   } }
 
 ##
diff --git a/block/fleecing-hook.c b/block/fleecing-hook.c
new file mode 100644
index 0000000000..f4e2f3ce83
--- /dev/null
+++ b/block/fleecing-hook.c
@@ -0,0 +1,298 @@
+/*
+ * Fleecing Hook 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"
+
+typedef struct BDRVFleecingHookState {
+    BdrvDirtyBitmap *cbw_bitmap; /* what should be copied to @target
+                                    on guest write. */
+    BdrvChild *target;
+    bool cbw_bitmap_created;
+} BDRVFleecingHookState;
+
+static coroutine_fn int fleecing_hook_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 fleecing_hook_cbw(BlockDriverState *bs, uint64_t offset,
+                                          uint64_t bytes)
+{
+    int ret = 0;
+    BDRVFleecingHookState *s = bs->opaque;
+    uint64_t gran = bdrv_dirty_bitmap_granularity(s->cbw_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]
+     */
+
+    len = end - off;
+    while (bdrv_dirty_bitmap_next_dirty_area(s->cbw_bitmap, &off, &len)) {
+        iov.iov_len = qiov.size = len;
+
+        bdrv_reset_dirty_bitmap(s->cbw_bitmap, off, len);
+
+        ret = bdrv_co_preadv(bs->backing, off, len, &qiov,
+                             BDRV_REQ_NO_SERIALISING);
+        if (ret < 0) {
+            bdrv_set_dirty_bitmap(s->cbw_bitmap, off, len);
+            goto finish;
+        }
+
+        ret = bdrv_co_pwritev(s->target, off, len, &qiov, BDRV_REQ_SERIALISING);
+        if (ret < 0) {
+            bdrv_set_dirty_bitmap(s->cbw_bitmap, off, len);
+            goto finish;
+        }
+
+        off += len;
+        if (off >= end) {
+            break;
+        }
+        len = end - off;
+    }
+
+finish:
+    qemu_vfree(iov.iov_base);
+
+    return ret;
+}
+
+static int coroutine_fn fleecing_hook_co_pdiscard(BlockDriverState *bs,
+                                                  int64_t offset, int bytes)
+{
+    int ret = fleecing_hook_cbw(bs, offset, bytes);
+    if (ret < 0) {
+        return ret;
+    }
+
+    /* Features to be implemented:
+     * F7. 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 fleecing_hook_co_pwrite_zeroes(BlockDriverState *bs,
+        int64_t offset, int bytes, BdrvRequestFlags flags)
+{
+    int ret = fleecing_hook_cbw(bs, offset, bytes);
+    if (ret < 0) {
+        /* F8. Additional option to break fleecing instead of breaking guest
+         * write here */
+        return ret;
+    }
+
+    return bdrv_co_pwrite_zeroes(bs->backing, offset, bytes, flags);
+}
+
+static coroutine_fn int fleecing_hook_co_pwritev(BlockDriverState *bs,
+                                                 uint64_t offset,
+                                                 uint64_t bytes,
+                                                 QEMUIOVector *qiov, int flags)
+{
+    int ret = fleecing_hook_cbw(bs, offset, bytes);
+    if (ret < 0) {
+        return ret;
+    }
+
+    return bdrv_co_pwritev(bs->backing, offset, bytes, qiov, flags);
+}
+
+static int coroutine_fn fleecing_hook_co_flush(BlockDriverState *bs)
+{
+    if (!bs->backing) {
+        return 0;
+    }
+
+    return bdrv_co_flush(bs->backing->bs);
+}
+
+static void fleecing_hook_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 fleecing_hook_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;
+    }
+}
+
+static int fleecing_hook_open(BlockDriverState *bs, QDict *options, int flags,
+                              Error **errp)
+{
+    BDRVFleecingHookState *s = bs->opaque;
+    Error *local_err = NULL;
+    const char *append_to, *copy_bitmap_name;
+    BlockDriverState *backing_bs;
+
+    append_to = qdict_get_str(options, "append-to");
+    qdict_del(options, "append-to");
+    backing_bs = bdrv_lookup_bs(append_to, append_to, errp);
+    if (!backing_bs) {
+        return -EINVAL;
+    }
+
+    bs->total_sectors = backing_bs->total_sectors;
+
+    copy_bitmap_name = qdict_get_try_str(options, "copy-bitmap");
+    if (copy_bitmap_name) {
+        qdict_del(options, "copy-bitmap");
+        s->cbw_bitmap = bdrv_find_dirty_bitmap(backing_bs, copy_bitmap_name);
+    }
+
+    if (!s->cbw_bitmap) {
+        s->cbw_bitmap = bdrv_create_dirty_bitmap(bs, 65536, copy_bitmap_name,
+                                                 errp);
+        if (!s->cbw_bitmap) {
+            return -EINVAL;
+        }
+        s->cbw_bitmap_created = true;
+    }
+
+    bdrv_disable_dirty_bitmap(s->cbw_bitmap);
+    bdrv_set_dirty_bitmap(s->cbw_bitmap, 0, bdrv_getlength(backing_bs));
+
+    s->target = bdrv_open_child(NULL, options, "target", bs, &child_file,
+                               false, errp);
+    if (!s->target) {
+        return -EINVAL;
+    }
+
+    bdrv_set_aio_context(bs, bdrv_get_aio_context(backing_bs));
+    bdrv_set_aio_context(s->target->bs, bdrv_get_aio_context(backing_bs));
+
+    bdrv_drained_begin(backing_bs);
+
+    bdrv_ref(bs);
+    bdrv_append(bs, backing_bs, &local_err);
+
+    if (local_err) {
+        bdrv_unref(bs);
+    }
+
+    bdrv_drained_end(backing_bs);
+
+    if (local_err) {
+        bdrv_unref_child(bs, s->target);
+        error_propagate(errp, local_err);
+        return -EINVAL;
+    }
+
+    return 0;
+}
+
+static void fleecing_hook_close(BlockDriverState *bs)
+{
+    BDRVFleecingHookState *s = bs->opaque;
+
+    if (s->cbw_bitmap && s->cbw_bitmap_created) {
+        bdrv_release_dirty_bitmap(bs, s->cbw_bitmap);
+    }
+
+    if (s->target) {
+        bdrv_unref_child(bs, s->target);
+    }
+}
+
+BlockDriver bdrv_fleecing_hook_filter = {
+    .format_name = "fleecing-hook",
+    .instance_size = sizeof(BDRVFleecingHookState),
+
+    .bdrv_co_preadv             = fleecing_hook_co_preadv,
+    .bdrv_co_pwritev            = fleecing_hook_co_pwritev,
+    .bdrv_co_pwrite_zeroes      = fleecing_hook_co_pwrite_zeroes,
+    .bdrv_co_pdiscard           = fleecing_hook_co_pdiscard,
+    .bdrv_co_flush              = fleecing_hook_co_flush,
+
+    .bdrv_co_block_status       = bdrv_co_block_status_from_backing,
+
+    .bdrv_refresh_filename      = fleecing_hook_refresh_filename,
+
+    .bdrv_open                  = fleecing_hook_open,
+    .bdrv_close                 = fleecing_hook_close,
+
+    .bdrv_child_perm            = fleecing_hook_child_perm,
+
+    .is_filter = true,
+};
+
+static void bdrv_fleecing_hook_init(void)
+{
+    bdrv_register(&bdrv_fleecing_hook_filter);
+}
+
+block_init(bdrv_fleecing_hook_init);
diff --git a/block/Makefile.objs b/block/Makefile.objs
index c8337bf186..081720b14f 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 += fleecing-hook.o
+
 common-obj-y += stream.o
 
 nfs.o-libs         := $(LIBNFS_LIBS)
-- 
2.18.0

Re: [Qemu-devel] [PATCH v3 13/18] block: introduce new filter driver: fleecing-hook

[Kevin Wolf]

Am 01.10.2018 um 12:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Fleecing-hook 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   +---------------+
>     | Fleecing hook |---------->| target(qcow2) |
>     +---+-----------+   CBW     +---+-----------+
>         |                           |
> backing |r,w                        |
>         v                           |
>     +---+---------+      backing    |
>     | Active disk |<----------------+
>     +-------------+        r
> 
> Target's backing may point to active disk (should be set up
> separately), which gives fleecing-scheme.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

This lacks an explanation why we need a specialised fleecing hook driver
rather than just a generic bdrv_backup_top block driver in analogy to
what commit and mirror are already doing.

In fact, if I'm reading the last patch of the series right, backup
doesn't even restrict the use of the fleecing-hook driver to actual
fleecing scenarios.

Maybe what doesn't feel right to me is just that it's a misnomer, and if
you rename it into bdrv_backup_top (and make it internal to the block
job), it is very close to what I actually have in mind?

Kevin

Re: [Qemu-devel] [PATCH v3 13/18] block: introduce new filter driver: fleecing-hook

[Vladimir Sementsov-Ogievskiy]

04.10.2018 15:44, Kevin Wolf wrote:
> Am 01.10.2018 um 12:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Fleecing-hook 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   +---------------+
>>      | Fleecing hook |---------->| target(qcow2) |
>>      +---+-----------+   CBW     +---+-----------+
>>          |                           |
>> backing |r,w                        |
>>          v                           |
>>      +---+---------+      backing    |
>>      | Active disk |<----------------+
>>      +-------------+        r
>>
>> Target's backing may point to active disk (should be set up
>> separately), which gives fleecing-scheme.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> This lacks an explanation why we need a specialised fleecing hook driver
> rather than just a generic bdrv_backup_top block driver in analogy to
> what commit and mirror are already doing.
>
> In fact, if I'm reading the last patch of the series right, backup
> doesn't even restrict the use of the fleecing-hook driver to actual
> fleecing scenarios.
>
> Maybe what doesn't feel right to me is just that it's a misnomer, and if
> you rename it into bdrv_backup_top (and make it internal to the block
> job), it is very close to what I actually have in mind?
>
> Kevin

Hm.
1. assume we move to internal bdrv_backup_top
2. backup(mode=none) becomes just a wrapper for append/drop of the 
bdrv_backup_top node
3. looks interesting to get rid of empty (doing nothing) job and use 
bdrv_backup_top directly.

I want to finally create different backup schemes, based on fleecing 
hook, for example:

     +-------+
     | Guest |
     +-------+
         |r,w
         v
     +---+-----------+  target   +---------------+ +--------+
     | Fleecing hook +---------->+ fleecing-node +---------->+ target |
     +---+-----------+   CBW     +---+-----------+ backup +--------+
         |                           |             (no hook)
backing |r,w                        |
         v                           |
     +---+---------+      backing    |
     | Active disk +<----------------+
     +-------------+        r


This is needed for slow nbd target, if we don't need to slow down guest 
writes.
Here backup(no hook) is a backup job without hook / write notifiers, as 
it actually
do copy from static source.

Or, we can use mirror instead of backup, as mirror is asynchronous and 
is faster than backup. We can even use mirror with write-blocking mode 
(proposed by Max) and use something like null bds (but with backing) 
instead of qcow2 fleecing-node - this will imitate current backup 
approach, but with mirror instead of backup.

Of course, we can use old backup(sync=none) for all such schemes, I just 
think that architecture with filter node is more clean, than with backup 
job, which looks the same but with additional job:
     +-------+
     | Guest |
     +-------+
         |r,w
         v
     +---------------+  target   +---------------+ +--------+
     |bdrv_backup_top+---------->+ fleecing-node +---------->+ target |
     +---------------+   CBW     +---+----------++ backup +--------+
         |                           |          ^  (no hook)
backing |r,w                        |          |
         v                           |          |
     +---+---------+      backing    |          |
     | Active disk +<----------------+          |
     +----------+--+        r                   |
                |                               |
                |           backup(sync=none)   |
                +-------------------------------+



Finally, the first picture looks nicer and has less entities (and I 
didn't draw target blk which backup creates and all the permissions). 
Hmm, it also may be more difficult to setup permissions in the second 
scheme, but I didn't dive into. We just agreed with Max that separate 
building brick which may be reused in different schemes is better than 
internal thing in backup, so, I went this way. However, if you are 
against, it isn't difficult to move it all into backup.

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 13/18] block: introduce new filter driver: fleecing-hook

[Kevin Wolf]

Am 04.10.2018 um 15:59 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 04.10.2018 15:44, Kevin Wolf wrote:
> > Am 01.10.2018 um 12:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >> Fleecing-hook 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   +---------------+
> >>      | Fleecing hook |---------->| target(qcow2) |
> >>      +---+-----------+   CBW     +---+-----------+
> >>          |                           |
> >> backing |r,w                        |
> >>          v                           |
> >>      +---+---------+      backing    |
> >>      | Active disk |<----------------+
> >>      +-------------+        r
> >>
> >> Target's backing may point to active disk (should be set up
> >> separately), which gives fleecing-scheme.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > This lacks an explanation why we need a specialised fleecing hook driver
> > rather than just a generic bdrv_backup_top block driver in analogy to
> > what commit and mirror are already doing.
> >
> > In fact, if I'm reading the last patch of the series right, backup
> > doesn't even restrict the use of the fleecing-hook driver to actual
> > fleecing scenarios.
> >
> > Maybe what doesn't feel right to me is just that it's a misnomer, and if
> > you rename it into bdrv_backup_top (and make it internal to the block
> > job), it is very close to what I actually have in mind?
> >
> > Kevin
> 
> Hm.
> 1. assume we move to internal bdrv_backup_top
> 2. backup(mode=none) becomes just a wrapper for append/drop of the 
> bdrv_backup_top node

I think you mean sync=none?

Yes, this is true. There is no actual background job taking place there,
so the job infrastructure doesn't add much. As you say, it's just
inserting the node at the start and dropping it again at the end.

> 3. looks interesting to get rid of empty (doing nothing) job and use 
> bdrv_backup_top directly.

We could directly make the filter node available for the user, like this
series does. Should we do that? I'm not sure, but I'm not necessarily
opposed either.

But looking at the big picture, I have some more thoughts on this:

1. Is backup with sync=none only useful for fleecing? My understanding
   was that "fleecing" specifically means a setup where the target of
   the backup node is an overlay of the active layer of the guest
   device.

   I can imagine other use cases that would use sync=none (e.g. if you
   don't access arbitrary blocks like from the NBD server in the
   fleecing setup, but directly write to a backup file that can be
   commited back later to revert things).

   So I think 'fleecing-hook' is too narrow as a name. Maybe just
   'backup' would be better?

2. mirror has a sync=none mode, too. And like backup, it doesn't
   actually have any background job running then (at least in active
   mirror mode), but only changes the graph at the end of the job.
   Some consistency would be nice there, so is the goal to eventually
   let the user create filter nodes for all jobs that don't have a
   real background job?

3. We have been thinking about unifying backup, commit and mirror
   into a single copy block job because they are doing quite similar
   things. Of course, there are differences whether the old data or the
   new data should be copied on a write, and which graph changes to make
   at the end of the job, but many of the other differences are actually
   features that would make sense in all of them, but are only
   implemented in one job driver.

   Maybe having a single 'copy' filter driver that provides options to
   select backup-like behaviour or mirror-like behaviour, and that can
   then internally be used by all three block jobs would be an
   interesting first step towards this?

   We can start with supporting only what backup needs, but design
   everything with the idea that mirror and commit could use it, too.

I honestly feel that at first this wouldn't be very different from what
you have, so with a few renames and cleanups we might be good. But it
would give us a design in the grand scheme to work towards instead of
doing one-off things for every special case like fleecing and ending up
with even more similar things that are implemented separately even
though they do mostly the same thing.

> I want to finally create different backup schemes, based on fleecing 
> hook, for example:
> 
>      +-------+
>      | Guest |
>      +-------+
>          |r,w
>          v
>      +---+-----------+  target   +---------------+ +--------+
>      | Fleecing hook +---------->+ fleecing-node +---------->+ target |
>      +---+-----------+   CBW     +---+-----------+ backup +--------+
>          |                           |             (no hook)
> backing |r,w                        |
>          v                           |
>      +---+---------+      backing    |
>      | Active disk +<----------------+
>      +-------------+        r
> 
> 
> This is needed for slow nbd target, if we don't need to slow down
> guest writes.  Here backup(no hook) is a backup job without hook /
> write notifiers, as it actually do copy from static source.

Right.

We don't actually have a backup without a hook yet (which would be the
same as the equally missing mirror for read-only nodes), but we do have
commit without a hook - it doesn't share the WRITE permission for the
source.  This is an example for a mode that a unified 'copy' driver
would automatically support.

> Or, we can use mirror instead of backup, as mirror is asynchronous and 
> is faster than backup. We can even use mirror with write-blocking mode 
> (proposed by Max) and use something like null bds (but with backing) 
> instead of qcow2 fleecing-node - this will imitate current backup 
> approach, but with mirror instead of backup.

To be honest, I don't understand the null BDS part. null throws away
whatever data is written to it, so that's certainly not what you want?

> Of course, we can use old backup(sync=none) for all such schemes, I just 
> think that architecture with filter node is more clean, than with backup 
> job, which looks the same but with additional job:
>      +-------+
>      | Guest |
>      +-------+
>          |r,w
>          v
>      +---------------+  target   +---------------+ +--------+
>      |bdrv_backup_top+---------->+ fleecing-node +---------->+ target |
>      +---------------+   CBW     +---+----------++ backup +--------+
>          |                           |          ^  (no hook)
> backing |r,w                        |          |
>          v                           |          |
>      +---+---------+      backing    |          |
>      | Active disk +<----------------+          |
>      +----------+--+        r                   |
>                 |                               |
>                 |           backup(sync=none)   |
>                 +-------------------------------+

This looks only more complex because you decided to draw the block job
into the graph, as an edge connecting source and target. In reality,
this is not an edge that would be existing because bdrv_backup_top
already has both nodes as children. The job wouldn't have an additional
reference, but just use the BdrvChild that is owned by bdrv_backup_top.

Maybe this is an interesting point for the decision between an
integrated filter driver in the jobs and completely separate filter
driver. The jobs probably need access to the internal data structure
(bs->opaque) of the filter node at least, so that they can issue
requests on the child nodes.

Of course, if it isn't an internal filter driver, but a proper
standalone driver, letting jobs use those child nodes might be
considered a bit ugly...

> Finally, the first picture looks nicer and has less entities (and I 
> didn't draw target blk which backup creates and all the permissions). 
> Hmm, it also may be more difficult to setup permissions in the second 
> scheme, but I didn't dive into. We just agreed with Max that separate 
> building brick which may be reused in different schemes is better than 
> internal thing in backup, so, I went this way. However, if you are 
> against, it isn't difficult to move it all into backup.

The idea with bdrv_backup_top would obviously be to get rid of the
additional BlockBackend and BdrvChild instances and only access source
and target as children of the filter node.

Kevin

Re: [Qemu-devel] [PATCH v3 13/18] block: introduce new filter driver: fleecing-hook

[Vladimir Sementsov-Ogievskiy]

On 10/04/2018 05:52 PM, Kevin Wolf wrote:
> Am 04.10.2018 um 15:59 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 04.10.2018 15:44, Kevin Wolf wrote:
>>> Am 01.10.2018 um 12:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>> Fleecing-hook 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   +---------------+
>>>>       | Fleecing hook |---------->| target(qcow2) |
>>>>       +---+-----------+   CBW     +---+-----------+
>>>>           |                           |
>>>> backing |r,w                        |
>>>>           v                           |
>>>>       +---+---------+      backing    |
>>>>       | Active disk |<----------------+
>>>>       +-------------+        r
>>>>
>>>> Target's backing may point to active disk (should be set up
>>>> separately), which gives fleecing-scheme.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> This lacks an explanation why we need a specialised fleecing hook driver
>>> rather than just a generic bdrv_backup_top block driver in analogy to
>>> what commit and mirror are already doing.
>>>
>>> In fact, if I'm reading the last patch of the series right, backup
>>> doesn't even restrict the use of the fleecing-hook driver to actual
>>> fleecing scenarios.
>>>
>>> Maybe what doesn't feel right to me is just that it's a misnomer, and if
>>> you rename it into bdrv_backup_top (and make it internal to the block
>>> job), it is very close to what I actually have in mind?
>>>
>>> Kevin
>> Hm.
>> 1. assume we move to internal bdrv_backup_top
>> 2. backup(mode=none) becomes just a wrapper for append/drop of the
>> bdrv_backup_top node
> I think you mean sync=none?
>
> Yes, this is true. There is no actual background job taking place there,
> so the job infrastructure doesn't add much. As you say, it's just
> inserting the node at the start and dropping it again at the end.
>
>> 3. looks interesting to get rid of empty (doing nothing) job and use
>> bdrv_backup_top directly.
> We could directly make the filter node available for the user, like this
> series does. Should we do that? I'm not sure, but I'm not necessarily
> opposed either.
>
> But looking at the big picture, I have some more thoughts on this:
>
> 1. Is backup with sync=none only useful for fleecing? My understanding
>     was that "fleecing" specifically means a setup where the target of
>     the backup node is an overlay of the active layer of the guest
>     device.
>
>     I can imagine other use cases that would use sync=none (e.g. if you
>     don't access arbitrary blocks like from the NBD server in the
>     fleecing setup, but directly write to a backup file that can be
>     commited back later to revert things).
>
>     So I think 'fleecing-hook' is too narrow as a name. Maybe just
>     'backup' would be better?

may be copy-before-write?

>
> 2. mirror has a sync=none mode, too. And like backup, it doesn't
>     actually have any background job running then (at least in active
>     mirror mode), but only changes the graph at the end of the job.
>     Some consistency would be nice there, so is the goal to eventually
>     let the user create filter nodes for all jobs that don't have a
>     real background job?
>
> 3. We have been thinking about unifying backup, commit and mirror
>     into a single copy block job because they are doing quite similar
>     things. Of course, there are differences whether the old data or the
>     new data should be copied on a write, and which graph changes to make
>     at the end of the job, but many of the other differences are actually
>     features that would make sense in all of them, but are only
>     implemented in one job driver.
>
>     Maybe having a single 'copy' filter driver that provides options to
>     select backup-like behaviour or mirror-like behaviour, and that can
>     then internally be used by all three block jobs would be an
>     interesting first step towards this?


Isn't it a question about having several simple things against one 
complicated?)
All these jobs are similar only in the fact that they are copying blocks 
from one point to another.. So, instead of creating one big job with a 
lot of options, we can separate copying code to some kind of internal 
copying api, to then share it between jobs (and qemi-img convert). 
Didn't you considered this way? Intuitively, I'm not a fan of idea to 
create one job, but I don't insist. Of course, we can create one job 
carefully split the code to different objects files, with (again) 
separate copying api, shared with qemu-img, so, difference between 
one-job vs several-jobs will be mostly in qapi/ , not in the real code...

>
>     We can start with supporting only what backup needs, but design
>     everything with the idea that mirror and commit could use it, too.
>
> I honestly feel that at first this wouldn't be very different from what
> you have, so with a few renames and cleanups we might be good. But it
> would give us a design in the grand scheme to work towards instead of
> doing one-off things for every special case like fleecing and ending up
> with even more similar things that are implemented separately even
> though they do mostly the same thing.
>
>> I want to finally create different backup schemes, based on fleecing
>> hook, for example:
>>
>>       +-------+
>>       | Guest |
>>       +-------+
>>           |r,w
>>           v
>>       +---+-----------+  target   +---------------+ +--------+
>>       | Fleecing hook +---------->+ fleecing-node +---------->+ target |
>>       +---+-----------+   CBW     +---+-----------+ backup +--------+
>>           |                           |             (no hook)
>> backing |r,w                        |
>>           v                           |
>>       +---+---------+      backing    |
>>       | Active disk +<----------------+
>>       +-------------+        r
>>
>>
>> This is needed for slow nbd target, if we don't need to slow down
>> guest writes.  Here backup(no hook) is a backup job without hook /
>> write notifiers, as it actually do copy from static source.
> Right.
>
> We don't actually have a backup without a hook yet (which would be the
> same as the equally missing mirror for read-only nodes), but we do have
> commit without a hook - it doesn't share the WRITE permission for the
> source.  This is an example for a mode that a unified 'copy' driver
> would automatically support.

I'm just afraid that copy driver will be even more complicated than 
mirror already is.
Mirror needs several iterations through the whole disk, other jobs - don't..

>
>> Or, we can use mirror instead of backup, as mirror is asynchronous and
>> is faster than backup. We can even use mirror with write-blocking mode
>> (proposed by Max) and use something like null bds (but with backing)
>> instead of qcow2 fleecing-node - this will imitate current backup
>> approach, but with mirror instead of backup.
> To be honest, I don't understand the null BDS part. null throws away
> whatever data is written to it, so that's certainly not what you want?

Exactly that. We don't need this data in fleecing node, as active sync 
mirror will copy it in-flight.

>
>> Of course, we can use old backup(sync=none) for all such schemes, I just
>> think that architecture with filter node is more clean, than with backup
>> job, which looks the same but with additional job:
>>       +-------+
>>       | Guest |
>>       +-------+
>>           |r,w
>>           v
>>       +---------------+  target   +---------------+ +--------+
>>       |bdrv_backup_top+---------->+ fleecing-node +---------->+ target |
>>       +---------------+   CBW     +---+----------++ backup +--------+
>>           |                           |          ^  (no hook)
>> backing |r,w                        |          |
>>           v                           |          |
>>       +---+---------+      backing    |          |
>>       | Active disk +<----------------+          |
>>       +----------+--+        r                   |
>>                  |                               |
>>                  |           backup(sync=none)   |
>>                  +-------------------------------+
> This looks only more complex because you decided to draw the block job
> into the graph, as an edge connecting source and target. In reality,
> this is not an edge that would be existing because bdrv_backup_top
> already has both nodes as children. The job wouldn't have an additional
> reference, but just use the BdrvChild that is owned by bdrv_backup_top.
>
> Maybe this is an interesting point for the decision between an
> integrated filter driver in the jobs and completely separate filter
> driver. The jobs probably need access to the internal data structure
> (bs->opaque) of the filter node at least, so that they can issue
> requests on the child nodes.
>
> Of course, if it isn't an internal filter driver, but a proper
> standalone driver, letting jobs use those child nodes might be
> considered a bit ugly...

Yes, in this case, there should be different children, sharing the same 
target BDS..
Hmm, sharing a BdrvChild is a point in favor of internal filter.

>
>> Finally, the first picture looks nicer and has less entities (and I
>> didn't draw target blk which backup creates and all the permissions).
>> Hmm, it also may be more difficult to setup permissions in the second
>> scheme, but I didn't dive into. We just agreed with Max that separate
>> building brick which may be reused in different schemes is better than
>> internal thing in backup, so, I went this way. However, if you are
>> against, it isn't difficult to move it all into backup.
> The idea with bdrv_backup_top would obviously be to get rid of the
> additional BlockBackend and BdrvChild instances and only access source
> and target as children of the filter node.
>
> Kevin

Ok. Let's start from internal driver, anyway, it is a lot easier to turn 
from internal to external if it is needed than vice versa. I'll resend. 
Most of prerequisites will not change. Also, anyway, I'll need to share 
copy-bitmap between two backup jobs, to avoid extra copying in the 
picture above. And anyway, I want to try to get rid of backup 
intersecting requests, using serializing requests and copy-bitmap instead.

Re: [Qemu-devel] [PATCH v3 13/18] block: introduce new filter driver: fleecing-hook

[Vladimir Sementsov-Ogievskiy]

05.10.2018 00:19, Vladimir Sementsov-Ogievskiy wrote:
On 10/04/2018 05:52 PM, Kevin Wolf wrote:
Am 04.10.2018 um 15:59 hat Vladimir Sementsov-Ogievskiy geschrieben:
04.10.2018 15:44, Kevin Wolf wrote:
Am 01.10.2018 um 12:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
Fleecing-hook 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   +---------------+
      | Fleecing hook |---------->| target(qcow2) |
      +---+-----------+   CBW     +---+-----------+
          |                           |
backing |r,w                        |
          v                           |
      +---+---------+      backing    |
      | Active disk |<----------------+
      +-------------+        r

Target's backing may point to active disk (should be set up
separately), which gives fleecing-scheme.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com><mailto:vsementsov@virtuozzo.com>
This lacks an explanation why we need a specialised fleecing hook driver
rather than just a generic bdrv_backup_top block driver in analogy to
what commit and mirror are already doing.

In fact, if I'm reading the last patch of the series right, backup
doesn't even restrict the use of the fleecing-hook driver to actual
fleecing scenarios.

Maybe what doesn't feel right to me is just that it's a misnomer, and if
you rename it into bdrv_backup_top (and make it internal to the block
job), it is very close to what I actually have in mind?

Kevin
Hm.
1. assume we move to internal bdrv_backup_top
2. backup(mode=none) becomes just a wrapper for append/drop of the
bdrv_backup_top node
I think you mean sync=none?

Yes, this is true. There is no actual background job taking place there,
so the job infrastructure doesn't add much. As you say, it's just
inserting the node at the start and dropping it again at the end.

3. looks interesting to get rid of empty (doing nothing) job and use
bdrv_backup_top directly.
We could directly make the filter node available for the user, like this
series does. Should we do that? I'm not sure, but I'm not necessarily
opposed either.

But looking at the big picture, I have some more thoughts on this:

1. Is backup with sync=none only useful for fleecing? My understanding
    was that "fleecing" specifically means a setup where the target of
    the backup node is an overlay of the active layer of the guest
    device.

    I can imagine other use cases that would use sync=none (e.g. if you
    don't access arbitrary blocks like from the NBD server in the
    fleecing setup, but directly write to a backup file that can be
    commited back later to revert things).

    So I think 'fleecing-hook' is too narrow as a name. Maybe just
    'backup' would be better?

may be copy-before-write?


2. mirror has a sync=none mode, too. And like backup, it doesn't
    actually have any background job running then (at least in active
    mirror mode), but only changes the graph at the end of the job.
    Some consistency would be nice there, so is the goal to eventually
    let the user create filter nodes for all jobs that don't have a
    real background job?

3. We have been thinking about unifying backup, commit and mirror
    into a single copy block job because they are doing quite similar
    things. Of course, there are differences whether the old data or the
    new data should be copied on a write, and which graph changes to make
    at the end of the job, but many of the other differences are actually
    features that would make sense in all of them, but are only
    implemented in one job driver.

    Maybe having a single 'copy' filter driver that provides options to
    select backup-like behaviour or mirror-like behaviour, and that can
    then internally be used by all three block jobs would be an
    interesting first step towards this?


Isn't it a question about having several simple things against one complicated?)
All these jobs are similar only in the fact that they are copying blocks from one point to another.. So, instead of creating one big job with a lot of options, we can separate copying code to some kind of internal copying api, to then share it between jobs (and qemi-img convert). Didn't you considered this way? Intuitively, I'm not a fan of idea to create one job, but I don't insist. Of course, we can create one job carefully split the code to different objects files, with (again) separate copying api, shared with qemu-img, so, difference between one-job vs several-jobs will be mostly in qapi/ , not in the real code...


    We can start with supporting only what backup needs, but design
    everything with the idea that mirror and commit could use it, too.

I honestly feel that at first this wouldn't be very different from what
you have, so with a few renames and cleanups we might be good. But it
would give us a design in the grand scheme to work towards instead of
doing one-off things for every special case like fleecing and ending up
with even more similar things that are implemented separately even
though they do mostly the same thing.

I want to finally create different backup schemes, based on fleecing
hook, for example:

      +-------+
      | Guest |
      +-------+
          |r,w
          v
      +---+-----------+  target   +---------------+ +--------+
      | Fleecing hook +---------->+ fleecing-node +---------->+ target |
      +---+-----------+   CBW     +---+-----------+ backup +--------+
          |                           |             (no hook)
backing |r,w                        |
          v                           |
      +---+---------+      backing    |
      | Active disk +<----------------+
      +-------------+        r


This is needed for slow nbd target, if we don't need to slow down
guest writes.  Here backup(no hook) is a backup job without hook /
write notifiers, as it actually do copy from static source.
Right.

We don't actually have a backup without a hook yet (which would be the
same as the equally missing mirror for read-only nodes), but we do have
commit without a hook - it doesn't share the WRITE permission for the
source.  This is an example for a mode that a unified 'copy' driver
would automatically support.

I'm just afraid that copy driver will be even more complicated than mirror already is.
Mirror needs several iterations through the whole disk, other jobs - don't..


Or, we can use mirror instead of backup, as mirror is asynchronous and
is faster than backup. We can even use mirror with write-blocking mode
(proposed by Max) and use something like null bds (but with backing)
instead of qcow2 fleecing-node - this will imitate current backup
approach, but with mirror instead of backup.
To be honest, I don't understand the null BDS part. null throws away
whatever data is written to it, so that's certainly not what you want?

Exactly that. We don't need this data in fleecing node, as active sync mirror will copy it in-flight.


Of course, we can use old backup(sync=none) for all such schemes, I just
think that architecture with filter node is more clean, than with backup
job, which looks the same but with additional job:
      +-------+
      | Guest |
      +-------+
          |r,w
          v
      +---------------+  target   +---------------+ +--------+
      |bdrv_backup_top+---------->+ fleecing-node +---------->+ target |
      +---------------+   CBW     +---+----------++ backup +--------+
          |                           |          ^  (no hook)
backing |r,w                        |          |
          v                           |          |
      +---+---------+      backing    |          |
      | Active disk +<----------------+          |
      +----------+--+        r                   |
                 |                               |
                 |           backup(sync=none)   |
                 +-------------------------------+
This looks only more complex because you decided to draw the block job
into the graph, as an edge connecting source and target. In reality,
this is not an edge that would be existing because bdrv_backup_top
already has both nodes as children. The job wouldn't have an additional
reference, but just use the BdrvChild that is owned by bdrv_backup_top.

Maybe this is an interesting point for the decision between an
integrated filter driver in the jobs and completely separate filter
driver. The jobs probably need access to the internal data structure
(bs->opaque) of the filter node at least, so that they can issue
requests on the child nodes.

Of course, if it isn't an internal filter driver, but a proper
standalone driver, letting jobs use those child nodes might be
considered a bit ugly...

Yes, in this case, there should be different children, sharing the same target BDS..
Hmm, sharing a BdrvChild is a point in favor of internal filter.

Hmm, how to share children?

backup job has two source BdrvChild'ren - child_job and child_root of job blk and two target BdrvChild'ren - again, child_job and child_root.

backup_top has source child - child_backing and second - child_file (named "target")..

Which BdrvChild'ren you suggest to remove? They are all different.

I don't know, why job needs both unnamed blk's and child_job's, and I don't know is it necessary for backup to use blk's not BdrvChild'ren..

And with internal way in none-mode we'll have two unused blk's  and four unused BdrvChild'ren.. Or we want to rewrite backup to use BdrvChild'ren for io operations and drop child_job BdrvChild'ren? So I'm lost. What did you mean?



Finally, the first picture looks nicer and has less entities (and I
didn't draw target blk which backup creates and all the permissions).
Hmm, it also may be more difficult to setup permissions in the second
scheme, but I didn't dive into. We just agreed with Max that separate
building brick which may be reused in different schemes is better than
internal thing in backup, so, I went this way. However, if you are
against, it isn't difficult to move it all into backup.
The idea with bdrv_backup_top would obviously be to get rid of the
additional BlockBackend and BdrvChild instances and only access source
and target as children of the filter node.

Kevin

Ok. Let's start from internal driver, anyway, it is a lot easier to turn from internal to external if it is needed than vice versa. I'll resend. Most of prerequisites will not change. Also, anyway, I'll need to share copy-bitmap between two backup jobs, to avoid extra copying in the picture above. And anyway, I want to try to get rid of backup intersecting requests, using serializing requests and copy-bitmap instead.




--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 13/18] block: introduce new filter driver: fleecing-hook

[Kevin Wolf]

Hi Vladimir,

can you please check your mailer settings? The plain text version of the
emails is hardly legible because it mixes quotes text and replies. I
had to manually open the HTML part to figure out what you really wrote.

Am 05.10.2018 um 17:00 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Hmm, how to share children?
> 
> backup job has two source BdrvChild'ren - child_job and child_root of
> job blk and two target BdrvChild'ren - again, child_job and
> child_root.
> 
> backup_top has source child - child_backing and second - child_file
> (named "target")..

Right, these are six BdrvChild instances in total. I think we can ignore
the child_job ones, they are internal to the block job infrastructure,
so we have four of them left.

> Which BdrvChild'ren you suggest to remove? They are all different.

Now that you introduced backup_top, I think we don't need any
BlockBackends any more. So I suggest to remove the child_root ones and
to do all I/O through the child_backing and child_file ones of
backup_top.

> I don't know, why job needs both unnamed blk's and child_job's, and I
> don't know is it necessary for backup to use blk's not BdrvChild'ren..

I think we had a case recently where it turned out that it is strictly
speaking even wrong for jobs to use BlockBackends in a function that
intercepts a request on the BDS level (like the copy-before-write of
backup).

So getting rid of the BlockBackends isn't only okay, but actually a good
thing by itself.

> And with internal way in none-mode we'll have two unused blk's  and
> four unused BdrvChild'ren.. Or we want to rewrite backup to use
> BdrvChild'ren for io operations and drop child_job BdrvChild'ren? So
> I'm lost. What did you mean?

child_job isn't actually unused, even though you never use them to make
requests. The child_job BdrvChild is important because of the
BdrvChildRole callbacks it provides to the block job infrastructure.

Kevin

Re: [Qemu-devel] [PATCH v3 13/18] block: introduce new filter driver: fleecing-hook

[Vladimir Sementsov-Ogievskiy]

05.10.2018 18:52, Kevin Wolf wrote:
> Hi Vladimir,
>
> can you please check your mailer settings? The plain text version of the
> emails is hardly legible because it mixes quotes text and replies. I
> had to manually open the HTML part to figure out what you really wrote.

I've sent it from other thunderbird instance from home, I hope 
thunderbird at work (where I'm composing now) is ok..

>
> Am 05.10.2018 um 17:00 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Hmm, how to share children?
>>
>> backup job has two source BdrvChild'ren - child_job and child_root of
>> job blk and two target BdrvChild'ren - again, child_job and
>> child_root.
>>
>> backup_top has source child - child_backing and second - child_file
>> (named "target")..
> Right, these are six BdrvChild instances in total. I think we can ignore
> the child_job ones, they are internal to the block job infrastructure,
> so we have four of them left.
>
>> Which BdrvChild'ren you suggest to remove? They are all different.
> Now that you introduced backup_top, I think we don't need any
> BlockBackends any more. So I suggest to remove the child_root ones and
> to do all I/O through the child_backing and child_file ones of
> backup_top.
>
>> I don't know, why job needs both unnamed blk's and child_job's, and I
>> don't know is it necessary for backup to use blk's not BdrvChild'ren..
> I think we had a case recently where it turned out that it is strictly
> speaking even wrong for jobs to use BlockBackends in a function that
> intercepts a request on the BDS level (like the copy-before-write of
> backup).
>
> So getting rid of the BlockBackends isn't only okay, but actually a good
> thing by itself.
>
>> And with internal way in none-mode we'll have two unused blk's  and
>> four unused BdrvChild'ren.. Or we want to rewrite backup to use
>> BdrvChild'ren for io operations and drop child_job BdrvChild'ren? So
>> I'm lost. What did you mean?
> child_job isn't actually unused, even though you never use them to make
> requests. The child_job BdrvChild is important because of the
> BdrvChildRole callbacks it provides to the block job infrastructure.
>
> Kevin

Ok, understand, thank you for the explanation!

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 13/18] block: introduce new filter driver: fleecing-hook

[Eric Blake]

On 10/5/18 11:40 AM, Vladimir Sementsov-Ogievskiy wrote:
> 05.10.2018 18:52, Kevin Wolf wrote:
>> Hi Vladimir,
>>
>> can you please check your mailer settings? The plain text version of the
>> emails is hardly legible because it mixes quotes text and replies. I
>> had to manually open the HTML part to figure out what you really wrote.
> 
> I've sent it from other thunderbird instance from home, I hope
> thunderbird at work (where I'm composing now) is ok..

Comparing the two:

Home:
Message-ID: <46e224bd-8c1f-4565-944e-52440e85e2f0@virtuozzo.com>
...
Content-Type: multipart/alternative;
	boundary="_000_46e224bd8c1f4565944e52440e85e2f0virtuozzocom_"

Work:
Message-ID: <05adf79a-4ae1-0ba1-aa7f-7696aa043594@virtuozzo.com>
...
Content-Type: text/plain; charset="utf-8"
Content-ID: <DF06DD561001084699A8EB982D66721B@eurprd08.prod.outlook.com>
Content-Transfer-Encoding: base64

So, the difference is that at home, you haven't told thunderbird to send 
plain-text only emails to specific recipients (setting up the list as 
one of those recipients that wants plain-text only), and something else 
in your local configurations then results in a multipart email where the 
html portion looks fine but the plain-text portion has horrendous 
quoting. But at work, you are configured for plain-text-only output, 
html is not even available, and the quoting is decent from the start.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v3 13/18] block: introduce new filter driver: fleecing-hook

[Vladimir Sementsov-Ogievskiy]

Thank you, hope that's fixed now)

On 10/05/2018 07:47 PM, Eric Blake wrote:
> On 10/5/18 11:40 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 05.10.2018 18:52, Kevin Wolf wrote:
>>> Hi Vladimir,
>>>
>>> can you please check your mailer settings? The plain text version of the
>>> emails is hardly legible because it mixes quotes text and replies. I
>>> had to manually open the HTML part to figure out what you really wrote.
>>
>> I've sent it from other thunderbird instance from home, I hope
>> thunderbird at work (where I'm composing now) is ok..
> 
> Comparing the two:
> 
> Home:
> Message-ID: <46e224bd-8c1f-4565-944e-52440e85e2f0@virtuozzo.com>
> ...
> Content-Type: multipart/alternative;
>      boundary="_000_46e224bd8c1f4565944e52440e85e2f0virtuozzocom_"
> 
> Work:
> Message-ID: <05adf79a-4ae1-0ba1-aa7f-7696aa043594@virtuozzo.com>
> ...
> Content-Type: text/plain; charset="utf-8"
> Content-ID: <DF06DD561001084699A8EB982D66721B@eurprd08.prod.outlook.com>
> Content-Transfer-Encoding: base64
> 
> So, the difference is that at home, you haven't told thunderbird to send 
> plain-text only emails to specific recipients (setting up the list as 
> one of those recipients that wants plain-text only), and something else 
> in your local configurations then results in a multipart email where the 
> html portion looks fine but the plain-text portion has horrendous 
> quoting. But at work, you are configured for plain-text-only output, 
> html is not even available, and the quoting is decent from the start.
> 

[Qemu-devel] [PATCH v3 14/18] block/fleecing-hook: internal api

[Vladimir Sementsov-Ogievskiy]

Add some functions to use fleecing-hook internally from backup job in
further commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/block.h |  9 ++++++++
 block/fleecing-hook.c | 51 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+)

diff --git a/include/block/block.h b/include/block/block.h
index 4edc1e8afa..018751b6ea 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -710,4 +710,13 @@ int coroutine_fn bdrv_co_copy_range(BdrvChild *src, uint64_t src_offset,
                                     BdrvChild *dst, uint64_t dst_offset,
                                     uint64_t bytes, BdrvRequestFlags read_flags,
                                     BdrvRequestFlags write_flags);
+
+
+BlockDriverState *bdrv_fleecing_hook_append(BlockDriverState *source,
+                                            BlockDriverState *target,
+                                            const char *copy_bitmap_name,
+                                            Error **errp);
+void bdrv_fleecing_hook_drop(BlockDriverState *hook);
+uint64_t bdrv_fleecing_hook_progress(BlockDriverState *hook);
+
 #endif
diff --git a/block/fleecing-hook.c b/block/fleecing-hook.c
index f4e2f3ce83..1471b985b2 100644
--- a/block/fleecing-hook.c
+++ b/block/fleecing-hook.c
@@ -34,6 +34,8 @@ typedef struct BDRVFleecingHookState {
                                     on guest write. */
     BdrvChild *target;
     bool cbw_bitmap_created;
+
+    uint64_t bytes_copied;
 } BDRVFleecingHookState;
 
 static coroutine_fn int fleecing_hook_co_preadv(
@@ -98,6 +100,7 @@ static coroutine_fn int fleecing_hook_cbw(BlockDriverState *bs, uint64_t offset,
             goto finish;
         }
 
+        s->bytes_copied += len;
         off += len;
         if (off >= end) {
             break;
@@ -296,3 +299,51 @@ static void bdrv_fleecing_hook_init(void)
 }
 
 block_init(bdrv_fleecing_hook_init);
+
+BlockDriverState *bdrv_fleecing_hook_append(BlockDriverState *source,
+                                            BlockDriverState *target,
+                                            const char *copy_bitmap_name,
+                                            Error **errp)
+{
+    QDict *opts = qdict_new();
+
+    qdict_put_str(opts, "driver", "fleecing-hook");
+    qdict_put_str(opts, "append-to", bdrv_get_node_name(source));
+    qdict_put_str(opts, "target", bdrv_get_node_name(target));
+    if (copy_bitmap_name) {
+        qdict_put_str(opts, "copy-bitmap", copy_bitmap_name);
+    }
+
+    /* set defaults like qmp_blockdev_add */
+    qdict_put_str(opts, BDRV_OPT_CACHE_DIRECT, "off");
+    qdict_put_str(opts, BDRV_OPT_CACHE_NO_FLUSH, "off");
+    qdict_put_str(opts, BDRV_OPT_READ_ONLY, "off");
+
+    return bdrv_open(NULL, NULL, opts, 0, errp);
+}
+
+void bdrv_fleecing_hook_drop(BlockDriverState *hook)
+{
+    AioContext *aio_context = bdrv_get_aio_context(hook);
+
+    aio_context_acquire(aio_context);
+
+    bdrv_drained_begin(hook);
+
+    bdrv_child_try_set_perm(hook->backing, 0, BLK_PERM_ALL, &error_abort);
+    bdrv_replace_node(hook, backing_bs(hook), &error_abort);
+    bdrv_set_backing_hd(hook, NULL, &error_abort);
+
+    bdrv_drained_end(hook);
+
+    bdrv_unref(hook);
+
+    aio_context_release(aio_context);
+}
+
+uint64_t bdrv_fleecing_hook_progress(BlockDriverState *hook)
+{
+    BDRVFleecingHookState *s = hook->opaque;
+
+    return s->bytes_copied;
+}
-- 
2.18.0

Re: [Qemu-devel] [PATCH v3 14/18] block/fleecing-hook: internal api

[Kevin Wolf]

Am 01.10.2018 um 12:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Add some functions to use fleecing-hook internally from backup job in
> further commit.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block.h |  9 ++++++++
>  block/fleecing-hook.c | 51 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 60 insertions(+)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 4edc1e8afa..018751b6ea 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -710,4 +710,13 @@ int coroutine_fn bdrv_co_copy_range(BdrvChild *src, uint64_t src_offset,
>                                      BdrvChild *dst, uint64_t dst_offset,
>                                      uint64_t bytes, BdrvRequestFlags read_flags,
>                                      BdrvRequestFlags write_flags);
> +
> +
> +BlockDriverState *bdrv_fleecing_hook_append(BlockDriverState *source,
> +                                            BlockDriverState *target,
> +                                            const char *copy_bitmap_name,
> +                                            Error **errp);
> +void bdrv_fleecing_hook_drop(BlockDriverState *hook);
> +uint64_t bdrv_fleecing_hook_progress(BlockDriverState *hook);

Public block.h interfaces that are specific to a single driver are bad
design. If at all possible, the generic block layer shouldn't know
anything about specific drivers.

Luckily, if you make the driver more consistent with what mirror and
commit are doing, you won't need public interfaces any more and the
functions will just become part of the backup job driver. (The backup
driver can still be split into multiple source files if necessary, and
then use a separate internal header; I'm not sure if this is the case
here, though.)

Kevin

[Qemu-devel] [PATCH v3 15/18] qapi: add x-drop-fleecing qmp command

[Vladimir Sementsov-Ogievskiy]

To insert fleecing-hook, it should be just created (blockdev-add) with
corresponding parameters. But it can't be properly removed by
blockdev-del (we can't restore backing chain in fleecing-hook .close),
So, let's add this expiremental api to drop the hook.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block-core.json | 17 +++++++++++++++++
 blockdev.c           | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 13cf90eab6..eda6fb6b7e 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3856,6 +3856,23 @@
 ##
 { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
 
+##
+# @x-drop-fleecing:
+#
+# Deletes fleecing-hook filter from the top of the backing chain.
+#
+# @node-name: Name of the fleecing-hook node name.
+#
+# Since: 3.1
+#
+# -> { "execute": "x-drop-fleecing",
+#      "arguments": { "node-name": "fleece0" }
+#    }
+# <- { "return": {} }
+#
+##
+{ 'command': 'x-drop-fleecing', 'data': { 'node-name': 'str' } }
+
 ##
 # @BlockdevCreateOptionsFile:
 #
diff --git a/blockdev.c b/blockdev.c
index 407e03d22a..90c09d5cee 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4250,6 +4250,43 @@ out:
     aio_context_release(aio_context);
 }
 
+void qmp_x_drop_fleecing(const char *node_name, Error **errp)
+{
+    AioContext *aio_context;
+    BlockDriverState *bs;
+
+    bs = bdrv_find_node(node_name);
+    if (!bs) {
+        error_setg(errp, "Cannot find node %s", node_name);
+        return;
+    }
+
+    if (!bdrv_has_blk(bs)) {
+        error_setg(errp, "Node %s is not inserted", node_name);
+        return;
+    }
+
+    if (!bs->backing) {
+        error_setg(errp, "'%s' has no backing", node_name);
+        return;
+    }
+
+    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);
+
+    qmp_blockdev_del(node_name, &error_abort);
+
+    aio_context_release(aio_context);
+}
+
 static BdrvChild *bdrv_find_child(BlockDriverState *parent_bs,
                                   const char *child_name)
 {
-- 
2.18.0

[Qemu-devel] [PATCH v3 16/18] iotests: test new fleecing-hook driver in context of 222 iotest

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/222     | 59 +++++++++++++++++++++++++++-------
 tests/qemu-iotests/222.out | 66 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 114 insertions(+), 11 deletions(-)

diff --git a/tests/qemu-iotests/222 b/tests/qemu-iotests/222
index 0ead56d574..3063dd0705 100644
--- a/tests/qemu-iotests/222
+++ b/tests/qemu-iotests/222
@@ -4,7 +4,9 @@
 # point-in-time snapshot of a node that can be queried over NBD.
 #
 # Copyright (C) 2018 Red Hat, Inc.
+# Copyright (c) 2018 Virtuozzo International GmbH. All rights reserved.
 # John helped, too.
+# And Vladimir, a bit.
 #
 # 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
@@ -46,11 +48,40 @@ remainder = [("0xd5", "0x108000",  "32k"), # Right-end of partial-left [1]
              ("0xdc", "32M",       "32k"), # Left-end of partial-right [2]
              ("0xcd", "0x3ff0000", "64k")] # patterns[3]
 
-with iotests.FilePath('base.img') as base_img_path, \
-     iotests.FilePath('fleece.img') as fleece_img_path, \
-     iotests.FilePath('nbd.sock') as nbd_sock_path, \
-     iotests.VM() as vm:
+class Fleecing():
+    def __init__(self, vm, src_node, tgt_node):
+        self.vm = vm
+        self.src_node = src_node
+        self.tgt_node = tgt_node
 
+class OldBackupFleecing(Fleecing):
+    def start(self):
+        log(self.vm.qmp("blockdev-backup",
+                        device=self.src_node,
+                        target=self.tgt_node,
+                        sync="none"))
+
+    def stop(self):
+        log(self.vm.qmp('block-job-cancel', device=self.src_node))
+        log(self.vm.event_wait('BLOCK_JOB_CANCELLED'),
+                filters=[iotests.filter_qmp_event])
+
+
+class FleecingHookFleecing(Fleecing):
+    def start(self):
+        log(self.vm.qmp("blockdev-add", **{
+            "driver": "fleecing-hook",
+            "node-name": "hook",
+            "append-to": self.src_node,
+            "target": self.tgt_node,
+        }))
+
+    def stop(self):
+        log(self.vm.qmp('x-drop-fleecing', node_name="hook"))
+
+
+def do_test(base_img_path, fleece_img_path, nbd_sock_path, fleecing):
+    vm = iotests.VM()
     log('--- Setting up images ---')
     log('')
 
@@ -77,6 +108,7 @@ with iotests.FilePath('base.img') as base_img_path, \
 
     src_node = "drive0"
     tgt_node = "fleeceNode"
+    fl = fleecing(vm, src_node, tgt_node)
 
     # create tgt_node backed by src_node
     log(vm.qmp("blockdev-add", **{
@@ -90,10 +122,7 @@ with iotests.FilePath('base.img') as base_img_path, \
     }))
 
     # Establish COW from source to fleecing node
-    log(vm.qmp("blockdev-backup",
-               device=src_node,
-               target=tgt_node,
-               sync="none"))
+    fl.start()
 
     log('')
     log('--- Setting up NBD Export ---')
@@ -137,10 +166,8 @@ with iotests.FilePath('base.img') as base_img_path, \
     log('--- Cleanup ---')
     log('')
 
-    log(vm.qmp('block-job-cancel', device=src_node))
-    log(vm.event_wait('BLOCK_JOB_CANCELLED'),
-        filters=[iotests.filter_qmp_event])
     log(vm.qmp('nbd-server-stop'))
+    fl.stop()
     log(vm.qmp('blockdev-del', node_name=tgt_node))
     vm.shutdown()
 
@@ -155,3 +182,13 @@ with iotests.FilePath('base.img') as base_img_path, \
 
     log('')
     log('Done')
+
+def test(fleecing):
+    with iotests.FilePath('base.img') as base_img_path, \
+         iotests.FilePath('fleece.img') as fleece_img_path, \
+         iotests.FilePath('nbd.sock') as nbd_sock_path:
+
+         do_test(base_img_path, fleece_img_path, nbd_sock_path, fleecing)
+
+test(OldBackupFleecing)
+test(FleecingHookFleecing)
diff --git a/tests/qemu-iotests/222.out b/tests/qemu-iotests/222.out
index 48f336a02b..28c3623ee8 100644
--- a/tests/qemu-iotests/222.out
+++ b/tests/qemu-iotests/222.out
@@ -49,9 +49,75 @@ read -P0 0x3fe0000 64k
 
 --- Cleanup ---
 
+{u'return': {}}
 {u'return': {}}
 {u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data': {u'device': u'drive0', u'type': u'backup', u'speed': 0, u'len': 67108864, u'offset': 393216}, u'event': u'BLOCK_JOB_CANCELLED'}
 {u'return': {}}
+
+--- Confirming writes ---
+
+read -P0xab 0 64k
+read -P0xad 0x00f8000 64k
+read -P0x1d 0x2008000 64k
+read -P0xea 0x3fe0000 64k
+read -P0xd5 0x108000 32k
+read -P0xdc 32M 32k
+read -P0xcd 0x3ff0000 64k
+
+Done
+--- Setting up images ---
+
+Done
+
+--- Launching VM ---
+
+Done
+
+--- Setting up Fleecing Graph ---
+
+{u'return': {}}
+{u'return': {}}
+
+--- Setting up NBD Export ---
+
+{u'return': {}}
+{u'return': {}}
+
+--- Sanity Check ---
+
+read -P0x5d 0 64k
+read -P0xd5 1M 64k
+read -P0xdc 32M 64k
+read -P0xcd 0x3ff0000 64k
+read -P0 0x00f8000 32k
+read -P0 0x2010000 32k
+read -P0 0x3fe0000 64k
+
+--- Testing COW ---
+
+write -P0xab 0 64k
+{u'return': u''}
+write -P0xad 0x00f8000 64k
+{u'return': u''}
+write -P0x1d 0x2008000 64k
+{u'return': u''}
+write -P0xea 0x3fe0000 64k
+{u'return': u''}
+
+--- Verifying Data ---
+
+read -P0x5d 0 64k
+read -P0xd5 1M 64k
+read -P0xdc 32M 64k
+read -P0xcd 0x3ff0000 64k
+read -P0 0x00f8000 32k
+read -P0 0x2010000 32k
+read -P0 0x3fe0000 64k
+
+--- Cleanup ---
+
+{u'return': {}}
+{u'return': {}}
 {u'return': {}}
 
 --- Confirming writes ---
-- 
2.18.0

[Qemu-devel] [PATCH v3 17/18] 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 | 95 +++++++++++++++++++++++++++-----------------------
 1 file changed, 52 insertions(+), 43 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 11aa31a323..6cab54dea4 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -570,6 +570,9 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     BackupBlockJob *job = NULL;
     int ret;
     char *gen_bitmap_name = NULL;
+    int64_t cluster_size;
+    BdrvDirtyBitmap *copy_bitmap = NULL;
+    bool copy_bitmap_created = false;
 
     assert(bs);
     assert(target);
@@ -624,6 +627,48 @@ 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);
+    }
+
+    if (x_copy_bitmap) {
+        copy_bitmap = bdrv_find_dirty_bitmap(bs, x_copy_bitmap);
+    } else {
+        x_copy_bitmap = gen_bitmap_name = id_generate(ID_BLOCK_BITMAP);
+    }
+    if (!copy_bitmap) {
+        copy_bitmap = bdrv_create_dirty_bitmap(bs, cluster_size,
+                                               x_copy_bitmap, errp);
+        copy_bitmap_created = !!copy_bitmap;
+    }
+    g_free(gen_bitmap_name);
+    if (!copy_bitmap) {
+        return NULL;
+    }
+
     len = bdrv_getlength(bs);
     if (len < 0) {
         error_setg_errno(errp, -len, "unable to get length for '%s'",
@@ -659,49 +704,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);
-    }
-
-    if (x_copy_bitmap) {
-        job->copy_bitmap = bdrv_find_dirty_bitmap(bs, x_copy_bitmap);
-    } else {
-        x_copy_bitmap = gen_bitmap_name = id_generate(ID_BLOCK_BITMAP);
-    }
-    if (!job->copy_bitmap) {
-        job->copy_bitmap = bdrv_create_dirty_bitmap(bs, job->cluster_size,
-                                                    x_copy_bitmap, errp);
-        job->copy_bitmap_created = !!job->copy_bitmap;
-    }
-    g_free(gen_bitmap_name);
-    if (!job->copy_bitmap) {
-        goto error;
-    }
-
+    job->cluster_size = cluster_size;
+    job->copy_bitmap = copy_bitmap;
+    job->copy_bitmap_created = copy_bitmap_created;
     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));
@@ -717,6 +722,10 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     return &job->common;
 
  error:
+    if (copy_bitmap_created) {
+        assert(!job || !job->copy_bitmap_created);
+        bdrv_release_dirty_bitmap(bs, copy_bitmap);
+    }
     if (sync_bitmap) {
         bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
     }
-- 
2.18.0

[Qemu-devel] [PATCH v3 18/18] block/backup: use fleecing-hook 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 fleecing-hook 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 no intersection. Also, read
requests are marked serializing, 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
fleecing-hook).

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

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

diff --git a/block/backup.c b/block/backup.c
index 6cab54dea4..9c85b23d68 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -29,13 +29,6 @@
 
 #define BACKUP_CLUSTER_SIZE_DEFAULT (1 << 16)
 
-typedef struct CowRequest {
-    int64_t start_byte;
-    int64_t end_byte;
-    QLIST_ENTRY(CowRequest) list;
-    CoQueue wait_queue; /* coroutines blocked on this request */
-} CowRequest;
-
 typedef struct BackupBlockJob {
     BlockJob common;
     BlockBackend *target;
@@ -44,13 +37,10 @@ typedef struct BackupBlockJob {
     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;
 
     BdrvDirtyBitmap *copy_bitmap;
     bool copy_bitmap_created;
@@ -58,53 +48,18 @@ typedef struct BackupBlockJob {
     int64_t copy_range_size;
 
     bool serialize_target_writes;
+
+    BlockDriverState *hook;
+    uint64_t fleecing_hook_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)
 {
@@ -113,7 +68,7 @@ static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,
     QEMUIOVector qiov;
     BlockBackend *blk = job->common.blk;
     int nbytes;
-    int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
+    int read_flags = BDRV_REQ_SERIALISING;
     int write_flags = job->serialize_target_writes ? BDRV_REQ_SERIALISING : 0;
 
     assert(start % job->cluster_size == 0);
@@ -161,15 +116,13 @@ 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 read_flags = BDRV_REQ_SERIALISING;
     int write_flags = job->serialize_target_writes ? BDRV_REQ_SERIALISING : 0;
 
     assert(QEMU_IS_ALIGNED(job->copy_range_size, job->cluster_size));
@@ -192,24 +145,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 fleecing_hook_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 (!bdrv_get_dirty_locked(NULL, job->copy_bitmap, start)) {
             trace_backup_do_cow_skip(job, start);
@@ -220,13 +167,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) {
@@ -238,7 +185,10 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
          */
         start += ret;
         job->bytes_read += ret;
-        job_progress_update(&job->common.job, ret);
+        fleecing_hook_progress = bdrv_fleecing_hook_progress(job->hook);
+        job_progress_update(&job->common.job, ret + fleecing_hook_progress -
+                            job->fleecing_hook_progress);
+        job->fleecing_hook_progress = fleecing_hook_progress;
         ret = 0;
     }
 
@@ -246,29 +196,11 @@ 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;
@@ -312,6 +244,8 @@ static void backup_clean(Job *job)
         bdrv_release_dirty_bitmap(blk_bs(s->common.blk), s->copy_bitmap);
         s->copy_bitmap = NULL;
     }
+
+    bdrv_fleecing_hook_drop(s->hook);
 }
 
 static void backup_attached_aio_context(BlockJob *job, AioContext *aio_context)
@@ -396,8 +330,7 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
             if (yield_and_check(job)) {
                 goto out;
             }
-            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)
             {
@@ -441,9 +374,7 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
     BlockDriverState *bs = blk_bs(s->common.blk);
     int64_t offset;
     int ret = 0;
-
-    QLIST_INIT(&s->inflight_reqs);
-    qemu_co_rwlock_init(&s->flush_rwlock);
+    uint64_t fleecing_hook_progress;
 
     job_progress_set_remaining(job, s->len);
 
@@ -455,15 +386,12 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
         bdrv_set_dirty_bitmap(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 fleecing-hook
+             * fileter driver service CbW requests. */
             job_yield(job);
         }
     } else if (s->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
@@ -514,7 +442,7 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
                 ret = alloced;
             } else {
                 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 */
@@ -530,11 +458,13 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
         }
     }
 
-    notifier_with_return_remove(&s->before_write);
+    /* wait pending CBW operations in fleecing hook */
+    bdrv_drain(s->hook);
 
-    /* wait until pending backup_do_cow() calls have completed */
-    qemu_co_rwlock_wrlock(&s->flush_rwlock);
-    qemu_co_rwlock_unlock(&s->flush_rwlock);
+    fleecing_hook_progress = bdrv_fleecing_hook_progress(s->hook);
+    job_progress_update(job, ret + fleecing_hook_progress -
+                        s->fleecing_hook_progress);
+    s->fleecing_hook_progress = fleecing_hook_progress;
 
     return ret;
 }
@@ -573,6 +503,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     int64_t cluster_size;
     BdrvDirtyBitmap *copy_bitmap = NULL;
     bool copy_bitmap_created = false;
+    BlockDriverState *hook;
 
     assert(bs);
     assert(target);
@@ -669,6 +600,19 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
         return NULL;
     }
 
+    /* bdrv_get_device_name will not help to find device name starting from
+     * @bs after fleecing hook 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);
+    }
+
+    hook = bdrv_fleecing_hook_append(bs, target, x_copy_bitmap, errp);
+    if (!hook) {
+        return NULL;
+    }
+
     len = bdrv_getlength(bs);
     if (len < 0) {
         error_setg_errno(errp, -len, "unable to get length for '%s'",
@@ -718,6 +662,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     block_job_add_bdrv(&job->common, "target", target, 0, BLK_PERM_ALL,
                        &error_abort);
     job->len = len;
+    job->hook = hook;
 
     return &job->common;
 
@@ -733,6 +678,9 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
         backup_clean(&job->common.job);
         job_early_fail(&job->common.job);
     }
+    if (hook) {
+        bdrv_fleecing_hook_drop(hook);
+    }
 
     return NULL;
 }
-- 
2.18.0

Re: [Qemu-devel] [PATCH v3 18/18] block/backup: use fleecing-hook instead of write notifiers

[Vladimir Sementsov-Ogievskiy]

01.10.2018 13:29, Vladimir Sementsov-Ogievskiy wrote:
> 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 fleecing-hook 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 no intersection. Also, read
> requests are marked serializing, 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
> fleecing-hook).
>
> 3. To sync with in-flight requests we no just drain hook node, we don't
> need rw-lock.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>

There is a problem in this patch: to be sure, that if backup job started 
to read from source, guest will not write to the area, I use
1. dirty bitmap: if it is set this means there must not be any in flight 
write requests on source on this region
2. serialized read: we start it, so until it finishes nobody writes

But backup may restart copy operation if it failed. And of course, 
second try of read may appear after guest write.. I see the following 
way to fix:

don't retry the whole operation, but only read. Then all there reads 
will be serialized and no yield point between them, so, guest can't 
write until we finish..

in the same time, retrying of copy_range() operation should be OK as is, 
as it keeps read serialized requests during the whole operation.

so, a kind of refactoring needed here.

Or, is it better to create external interface for creating serialized 
requests? something like bdrv_co_pblockwrite, which will block writes on 
this region, by creating special serialized request, until paired 
bdrv_co_punblockwrite()?

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 00/18] fleecing-hook driver for backup

[Eric Blake]

On 10/1/18 5:29 AM, Vladimir Sementsov-Ogievskiy wrote:
> v2 was "[RFC v2] new, node-graph-based fleecing and backup"
> 
> Hi all!
> 
> These series introduce fleecing-hook 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 18)
> 
> Proposed filter driver is complete and separate: it can be used
> standalone, as fleecing provider (instead of backup(sync=none)).
> (old-style fleecing based on backup(sync=none) is supported too),
> look at patch 16.

I haven't had time to look at this series in any sort of depth yet, but 
it reminds me of a question I just ran into with my libvirt code:

What happens if we want to have two parallel clients both reading off 
different backup/fleece nodes at once?  Right now, 'nbd-server-start' is 
hard-coded to at most one NBD server, and 'nbd-server-add' is hardcoded 
to adding an export to the one-and-only NBD server.  But it would be a 
lot nicer if you could pick different ports for different clients (or 
even mix TCP and Unix sockets), so that independent backup jobs can both 
operate in parallel via different NBD servers both under control of the 
same qemu process, instead of the second client having to wait for the 
first client to disconnect so that the first NBD server can stop.  In 
the meantime, you can be somewhat careful by controlling which export 
names are exposed over NBD, but even with nbd-server-start using 
"tls-creds", all clients can see one another's exports via NBD_OPT_LIST, 
and you are relying on the clients being well-behaved, vs. the nicer 
ability to spawn multiple NBD servers, then control which exports are 
exposed over which servers, and where distinct servers could even have 
different tls-creds.

To get to that point, we'd need to enhance nbd-server-start to return a 
server id, and allow nbd-server-add and friends to take an optional 
parameter of a server id (for back-compat, if the server id is not 
provided, it operates on the first one).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v3 00/18] fleecing-hook driver for backup

[Vladimir Sementsov-Ogievskiy]

02.10.2018 23:19, Eric Blake wrote:
> On 10/1/18 5:29 AM, Vladimir Sementsov-Ogievskiy wrote:
>> v2 was "[RFC v2] new, node-graph-based fleecing and backup"
>>
>> Hi all!
>>
>> These series introduce fleecing-hook 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 18)
>>
>> Proposed filter driver is complete and separate: it can be used
>> standalone, as fleecing provider (instead of backup(sync=none)).
>> (old-style fleecing based on backup(sync=none) is supported too),
>> look at patch 16.
>
> I haven't had time to look at this series in any sort of depth yet, 
> but it reminds me of a question I just ran into with my libvirt code:
>
> What happens if we want to have two parallel clients both reading off 
> different backup/fleece nodes at once?  Right now, 'nbd-server-start' 
> is hard-coded to at most one NBD server, and 'nbd-server-add' is 
> hardcoded to adding an export to the one-and-only NBD server.  But it 
> would be a lot nicer if you could pick different ports for different 
> clients (or even mix TCP and Unix sockets), so that independent backup 
> jobs can both operate in parallel via different NBD servers both under 
> control of the same qemu process, instead of the second client having 
> to wait for the first client to disconnect so that the first NBD 
> server can stop. In the meantime, you can be somewhat careful by 
> controlling which export names are exposed over NBD, but even with 
> nbd-server-start using "tls-creds", all clients can see one another's 
> exports via NBD_OPT_LIST, and you are relying on the clients being 
> well-behaved, vs. the nicer ability to spawn multiple NBD servers, 
> then control which exports are exposed over which servers, and where 
> distinct servers could even have different tls-creds.
>
> To get to that point, we'd need to enhance nbd-server-start to return 
> a server id, and allow nbd-server-add and friends to take an optional 
> parameter of a server id (for back-compat, if the server id is not 
> provided, it operates on the first one).
>

Good thing.
Don't see any problems from block layer: if we want to export the same 
fleecing through several different servers, they all can share the same 
fleecing node.

However, with new approach, we even can setup several fleecing nodes for 
one active disk.. any benefits? For example we can start second external 
backup on the same disk, when the first one is still in progress.. Don't 
sure that's a real case.

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 00/18] fleecing-hook driver for backup

[Vladimir Sementsov-Ogievskiy]

02.10.2018 23:19, Eric Blake wrote:
> On 10/1/18 5:29 AM, Vladimir Sementsov-Ogievskiy wrote:
>> v2 was "[RFC v2] new, node-graph-based fleecing and backup"
>>
>> Hi all!
>>
>> These series introduce fleecing-hook 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 18)
>>
>> Proposed filter driver is complete and separate: it can be used
>> standalone, as fleecing provider (instead of backup(sync=none)).
>> (old-style fleecing based on backup(sync=none) is supported too),
>> look at patch 16.
>
> I haven't had time to look at this series in any sort of depth yet, 
> but it reminds me of a question I just ran into with my libvirt code:
>
> What happens if we want to have two parallel clients both reading off 
> different backup/fleece nodes at once?  Right now, 'nbd-server-start' 
> is hard-coded to at most one NBD server, and 'nbd-server-add' is 
> hardcoded to adding an export to the one-and-only NBD server.  But it 
> would be a lot nicer if you could pick different ports for different 
> clients (or even mix TCP and Unix sockets), so that independent backup 
> jobs can both operate in parallel via different NBD servers both under 
> control of the same qemu process, instead of the second client having 
> to wait for the first client to disconnect so that the first NBD 
> server can stop. In the meantime, you can be somewhat careful by 
> controlling which export names are exposed over NBD, but even with 
> nbd-server-start using "tls-creds", all clients can see one another's 
> exports via NBD_OPT_LIST, and you are relying on the clients being 
> well-behaved, vs. the nicer ability to spawn multiple NBD servers, 
> then control which exports are exposed over which servers, and where 
> distinct servers could even have different tls-creds.
>
> To get to that point, we'd need to enhance nbd-server-start to return 
> a server id, and allow nbd-server-add and friends to take an optional 
> parameter of a server id (for back-compat, if the server id is not 
> provided, it operates on the first one).
>

hmm, about different ports, it's funny, but NBD-spec is directly against 
different ports for new style negotiation:
A client who wants to use the new style negotiation SHOULD connect on 
the IANA-reserved port for NBD, 10809. The server MAY listen on other 
ports as well, but it SHOULD use the old style handshake on those.

also, next sentence is strange too:
The server SHOULD refuse to allow oldstyle negotiations on the newstyle 
port.

refuse? Refuse to whom? It's a server, who choose negotiation type. Or 
this means, that it should refuse to start at all. so refuse to server 
admin, not to NBD client? sounds strange. Should it be "Server SHOULD 
NOT use the old style on port 10809"?

-- 
Best regards,
Vladimir