block/backup: use backup-top instead of write notifiers

Drop write notifiers and use filter node instead.

= Changes =

1. Add filter-node-name argument for backup qmp api. We have to do it
in this commit, as 257 needs to be fixed.

2. There are no more write notifiers here, so is_write_notifier
parameter is dropped from block-copy paths.

3. To sync with in-flight requests at job finish we now have drained
removing of the filter, we don't need rw-lock.

4. Block-copy is now using BdrvChildren instead of BlockBackends

5. As backup-top owns these children, we also move block-copy state
into backup-top's ownership.

= Iotest changes =

56: op-blocker doesn't shoot now, as we set it on source, but then
check on filter, when trying to start second backup.
To keep the test we instead can catch another collision: both jobs will
get 'drive0' job-id, as job-id parameter is unspecified. To prevent
interleaving with file-posix locks (as they are dependent on config)
let's use another target for second backup.

Also, it's obvious now that we'd like to drop this op-blocker at all
and add a test-case for two backups from one node (to different
destinations) actually works. But not in these series.

141: Output changed: prepatch, "Node is in use" comes from bdrv_has_blk
check inside qmp_blockdev_del. But we've dropped block-copy blk
objects, so no more blk objects on source bs (job blk is on backup-top
filter bs). New message is from op-blocker, which is the next check in
qmp_blockdev_add.

257: The test wants to emulate guest write during backup. They should
go to filter node, not to original source node, of course. Therefore we
need to specify filter node name and use it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-id: 20191001131409.14202-6-vsementsov@virtuozzo.com
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>

4 files changed, 56 insertions, 121 deletions

diff --git a/block/backup-top.c b/block/backup-top.c
index 75a3157..7cdb1f8 100644
--- a/block/backup-top.c
+++ b/block/backup-top.c
@@ -50,12 +50,11 @@ static coroutine_fn int backup_top_co_preadv(
 static coroutine_fn int backup_top_cbw(BlockDriverState *bs, uint64_t offset,
                                        uint64_t bytes)
 {
-    /*
-     * Here we'd like to use block_copy(), but block-copy need to be moved to
-     * use BdrvChildren to correctly use it in backup-top filter. It's a TODO.
-     */
+    BDRVBackupTopState *s = bs->opaque;
+    uint64_t end = QEMU_ALIGN_UP(offset + bytes, s->bcs->cluster_size);
+    uint64_t off = QEMU_ALIGN_DOWN(offset, s->bcs->cluster_size);
 
-    abort();
+    return block_copy(s->bcs, off, end - off, NULL);
 }
 
 static int coroutine_fn backup_top_co_pdiscard(BlockDriverState *bs,
@@ -228,14 +227,10 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
         goto failed_after_append;
     }
 
-    /*
-     * TODO: Create block-copy-state here (which will utilize @cluster_size and
-     * @write_flags parameters which are unused now). For this, block-copy
-     * should be refactored to use BdrvChildren.
-     */
-    state->bcs = NULL;
-    if (!state->bcs) {
-        error_setg(&local_err, "Cannot create block-copy-state");
+    state->bcs = block_copy_state_new(top->backing, state->target,
+                                      cluster_size, write_flags, &local_err);
+    if (local_err) {
+        error_prepend(&local_err, "Cannot create block-copy-state: ");
         goto failed_after_append;
     }
     *bcs = state->bcs;
diff --git a/block/backup.c b/block/backup.c
index 1057ed0..46978c1 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -2,6 +2,7 @@
  * QEMU backup
  *
  * Copyright (C) 2013 Proxmox Server Solutions
+ * Copyright (c) 2019 Virtuozzo International GmbH.
  *
  * Authors:
  *  Dietmar Maurer (dietmar@proxmox.com)
@@ -27,10 +28,13 @@
 #include "qemu/bitmap.h"
 #include "qemu/error-report.h"
 
+#include "block/backup-top.h"
+
 #define BACKUP_CLUSTER_SIZE_DEFAULT (1 << 16)
 
 typedef struct BackupBlockJob {
     BlockJob common;
+    BlockDriverState *backup_top;
     BlockDriverState *source_bs;
 
     BdrvDirtyBitmap *sync_bitmap;
@@ -39,11 +43,9 @@ typedef struct BackupBlockJob {
     BitmapSyncMode bitmap_mode;
     BlockdevOnError on_source_error;
     BlockdevOnError on_target_error;
-    CoRwlock flush_rwlock;
     uint64_t len;
     uint64_t bytes_read;
     int64_t cluster_size;
-    NotifierWithReturn before_write;
 
     BlockCopyState *bcs;
 } BackupBlockJob;
@@ -68,43 +70,23 @@ static void backup_progress_reset_callback(void *opaque)
 
 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)
 {
     int ret = 0;
     int64_t start, end; /* bytes */
 
-    qemu_co_rwlock_rdlock(&job->flush_rwlock);
-
     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);
 
-    ret = block_copy(job->bcs, start, end - start, error_is_read,
-                     is_write_notifier);
+    ret = block_copy(job->bcs, start, end - start, error_is_read);
 
     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 == job->source_bs);
-    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;
@@ -154,7 +136,7 @@ static void backup_clean(Job *job)
 {
     BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
 
-    block_copy_state_free(s->bcs);
+    bdrv_backup_top_drop(s->backup_top);
 }
 
 void backup_do_checkpoint(BlockJob *job, Error **errp)
@@ -220,8 +202,7 @@ static int coroutine_fn backup_loop(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)
             {
@@ -265,13 +246,8 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
     BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
     int ret = 0;
 
-    qemu_co_rwlock_init(&s->flush_rwlock);
-
     backup_init_copy_bitmap(s);
 
-    s->before_write.notify = backup_before_write_notify;
-    bdrv_add_before_write_notifier(s->source_bs, &s->before_write);
-
     if (s->sync_mode == MIRROR_SYNC_MODE_TOP) {
         int64_t offset = 0;
         int64_t count;
@@ -309,12 +285,6 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
     }
 
  out:
-    notifier_with_return_remove(&s->before_write);
-
-    /* wait until pending backup_do_cow() calls have completed */
-    qemu_co_rwlock_wrlock(&s->flush_rwlock);
-    qemu_co_rwlock_unlock(&s->flush_rwlock);
-
     return ret;
 }
 
@@ -372,6 +342,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
                   MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap,
                   BitmapSyncMode bitmap_mode,
                   bool compress,
+                  const char *filter_node_name,
                   BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
                   int creation_flags,
@@ -382,6 +353,8 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     BackupBlockJob *job = NULL;
     int64_t cluster_size;
     BdrvRequestFlags write_flags;
+    BlockDriverState *backup_top = NULL;
+    BlockCopyState *bcs = NULL;
 
     assert(bs);
     assert(target);
@@ -463,33 +436,35 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     write_flags = (bdrv_chain_contains(target, bs) ? BDRV_REQ_SERIALISING : 0) |
                   (compress ? BDRV_REQ_WRITE_COMPRESSED : 0),
 
+    backup_top = bdrv_backup_top_append(bs, target, filter_node_name,
+                                        cluster_size, write_flags, &bcs, errp);
+    if (!backup_top) {
+        goto error;
+    }
+
     /* job->len is fixed, so we can't allow resize */
-    job = block_job_create(job_id, &backup_job_driver, txn, bs, 0, BLK_PERM_ALL,
+    job = block_job_create(job_id, &backup_job_driver, txn, backup_top,
+                           0, BLK_PERM_ALL,
                            speed, creation_flags, cb, opaque, errp);
     if (!job) {
         goto error;
     }
 
+    job->backup_top = backup_top;
     job->source_bs = bs;
     job->on_source_error = on_source_error;
     job->on_target_error = on_target_error;
     job->sync_mode = sync_mode;
     job->sync_bitmap = sync_bitmap;
     job->bitmap_mode = bitmap_mode;
-
-    job->bcs = block_copy_state_new(bs, target, cluster_size, write_flags,
-                                    errp);
-    if (!job->bcs) {
-        goto error;
-    }
-
+    job->bcs = bcs;
     job->cluster_size = cluster_size;
     job->len = len;
 
-    block_copy_set_callbacks(job->bcs, backup_progress_bytes_callback,
+    block_copy_set_callbacks(bcs, backup_progress_bytes_callback,
                              backup_progress_reset_callback, job);
 
-    /* Required permissions are already taken by block-copy-state target */
+    /* Required permissions are already taken by backup-top target */
     block_job_add_bdrv(&job->common, "target", target, 0, BLK_PERM_ALL,
                        &error_abort);
 
@@ -502,6 +477,8 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     if (job) {
         backup_clean(&job->common.job);
         job_early_fail(&job->common.job);
+    } else if (backup_top) {
+        bdrv_backup_top_drop(backup_top);
     }
 
     return NULL;
diff --git a/block/block-copy.c b/block/block-copy.c
index fcb112d..0f76ea1 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -60,24 +60,22 @@ void block_copy_state_free(BlockCopyState *s)
         return;
     }
 
-    bdrv_release_dirty_bitmap(blk_bs(s->source), s->copy_bitmap);
-    blk_unref(s->source);
-    blk_unref(s->target);
+    bdrv_release_dirty_bitmap(s->source->bs, s->copy_bitmap);
     g_free(s);
 }
 
-BlockCopyState *block_copy_state_new(BlockDriverState *source,
-                                     BlockDriverState *target,
+BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
                                      int64_t cluster_size,
                                      BdrvRequestFlags write_flags, Error **errp)
 {
     BlockCopyState *s;
-    int ret;
-    uint64_t no_resize = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE |
-                         BLK_PERM_WRITE_UNCHANGED | BLK_PERM_GRAPH_MOD;
     BdrvDirtyBitmap *copy_bitmap;
+    uint32_t max_transfer =
+            MIN_NON_ZERO(INT_MAX, MIN_NON_ZERO(source->bs->bl.max_transfer,
+                                               target->bs->bl.max_transfer));
 
-    copy_bitmap = bdrv_create_dirty_bitmap(source, cluster_size, NULL, errp);
+    copy_bitmap = bdrv_create_dirty_bitmap(source->bs, cluster_size, NULL,
+                                           errp);
     if (!copy_bitmap) {
         return NULL;
     }
@@ -85,19 +83,15 @@ BlockCopyState *block_copy_state_new(BlockDriverState *source,
 
     s = g_new(BlockCopyState, 1);
     *s = (BlockCopyState) {
-        .source = blk_new(bdrv_get_aio_context(source),
-                          BLK_PERM_CONSISTENT_READ, no_resize),
-        .target = blk_new(bdrv_get_aio_context(target),
-                          BLK_PERM_WRITE, no_resize),
+        .source = source,
+        .target = target,
         .copy_bitmap = copy_bitmap,
         .cluster_size = cluster_size,
         .len = bdrv_dirty_bitmap_size(copy_bitmap),
         .write_flags = write_flags,
     };
 
-    s->copy_range_size = QEMU_ALIGN_DOWN(MIN(blk_get_max_transfer(s->source),
-                                             blk_get_max_transfer(s->target)),
-                                         s->cluster_size);
+    s->copy_range_size = QEMU_ALIGN_DOWN(max_transfer, cluster_size),
     /*
      * Set use_copy_range, consider the following:
      * 1. Compression is not supported for copy_range.
@@ -111,32 +105,7 @@ BlockCopyState *block_copy_state_new(BlockDriverState *source,
 
     QLIST_INIT(&s->inflight_reqs);
 
-    /*
-     * We just allow aio context change on our block backends. block_copy() user
-     * (now it's only backup) is responsible for source and target being in same
-     * aio context.
-     */
-    blk_set_disable_request_queuing(s->source, true);
-    blk_set_allow_aio_context_change(s->source, true);
-    blk_set_disable_request_queuing(s->target, true);
-    blk_set_allow_aio_context_change(s->target, true);
-
-    ret = blk_insert_bs(s->source, source, errp);
-    if (ret < 0) {
-        goto fail;
-    }
-
-    ret = blk_insert_bs(s->target, target, errp);
-    if (ret < 0) {
-        goto fail;
-    }
-
     return s;
-
-fail:
-    block_copy_state_free(s);
-
-    return NULL;
 }
 
 void block_copy_set_callbacks(
@@ -157,22 +126,20 @@ void block_copy_set_callbacks(
 static int coroutine_fn block_copy_with_bounce_buffer(BlockCopyState *s,
                                                       int64_t start,
                                                       int64_t end,
-                                                      bool is_write_notifier,
                                                       bool *error_is_read,
                                                       void **bounce_buffer)
 {
     int ret;
     int nbytes;
-    int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
 
     assert(QEMU_IS_ALIGNED(start, s->cluster_size));
     bdrv_reset_dirty_bitmap(s->copy_bitmap, start, s->cluster_size);
     nbytes = MIN(s->cluster_size, s->len - start);
     if (!*bounce_buffer) {
-        *bounce_buffer = blk_blockalign(s->source, s->cluster_size);
+        *bounce_buffer = qemu_blockalign(s->source->bs, s->cluster_size);
     }
 
-    ret = blk_co_pread(s->source, start, nbytes, *bounce_buffer, read_flags);
+    ret = bdrv_co_pread(s->source, start, nbytes, *bounce_buffer, 0);
     if (ret < 0) {
         trace_block_copy_with_bounce_buffer_read_fail(s, start, ret);
         if (error_is_read) {
@@ -181,8 +148,8 @@ static int coroutine_fn block_copy_with_bounce_buffer(BlockCopyState *s,
         goto fail;
     }
 
-    ret = blk_co_pwrite(s->target, start, nbytes, *bounce_buffer,
-                        s->write_flags);
+    ret = bdrv_co_pwrite(s->target, start, nbytes, *bounce_buffer,
+                         s->write_flags);
     if (ret < 0) {
         trace_block_copy_with_bounce_buffer_write_fail(s, start, ret);
         if (error_is_read) {
@@ -204,13 +171,11 @@ fail:
  */
 static int coroutine_fn block_copy_with_offload(BlockCopyState *s,
                                                 int64_t start,
-                                                int64_t end,
-                                                bool is_write_notifier)
+                                                int64_t end)
 {
     int ret;
     int nr_clusters;
     int nbytes;
-    int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
 
     assert(QEMU_IS_ALIGNED(s->copy_range_size, s->cluster_size));
     assert(QEMU_IS_ALIGNED(start, s->cluster_size));
@@ -218,8 +183,8 @@ static int coroutine_fn block_copy_with_offload(BlockCopyState *s,
     nr_clusters = DIV_ROUND_UP(nbytes, s->cluster_size);
     bdrv_reset_dirty_bitmap(s->copy_bitmap, start,
                             s->cluster_size * nr_clusters);
-    ret = blk_co_copy_range(s->source, start, s->target, start, nbytes,
-                            read_flags, s->write_flags);
+    ret = bdrv_co_copy_range(s->source, start, s->target, start, nbytes,
+                             0, s->write_flags);
     if (ret < 0) {
         trace_block_copy_with_offload_fail(s, start, ret);
         bdrv_set_dirty_bitmap(s->copy_bitmap, start,
@@ -237,7 +202,7 @@ static int coroutine_fn block_copy_with_offload(BlockCopyState *s,
 static int block_copy_is_cluster_allocated(BlockCopyState *s, int64_t offset,
                                            int64_t *pnum)
 {
-    BlockDriverState *bs = blk_bs(s->source);
+    BlockDriverState *bs = s->source->bs;
     int64_t count, total_count = 0;
     int64_t bytes = s->len - offset;
     int ret;
@@ -302,8 +267,7 @@ int64_t block_copy_reset_unallocated(BlockCopyState *s,
 
 int coroutine_fn block_copy(BlockCopyState *s,
                             int64_t start, uint64_t bytes,
-                            bool *error_is_read,
-                            bool is_write_notifier)
+                            bool *error_is_read)
 {
     int ret = 0;
     int64_t end = bytes + start; /* bytes */
@@ -315,7 +279,8 @@ int coroutine_fn block_copy(BlockCopyState *s,
      * block_copy() user is responsible for keeping source and target in same
      * aio context
      */
-    assert(blk_get_aio_context(s->source) == blk_get_aio_context(s->target));
+    assert(bdrv_get_aio_context(s->source->bs) ==
+           bdrv_get_aio_context(s->target->bs));
 
     assert(QEMU_IS_ALIGNED(start, s->cluster_size));
     assert(QEMU_IS_ALIGNED(end, s->cluster_size));
@@ -352,15 +317,13 @@ int coroutine_fn block_copy(BlockCopyState *s,
         trace_block_copy_process(s, start);
 
         if (s->use_copy_range) {
-            ret = block_copy_with_offload(s, start, dirty_end,
-                                          is_write_notifier);
+            ret = block_copy_with_offload(s, start, dirty_end);
             if (ret < 0) {
                 s->use_copy_range = false;
             }
         }
         if (!s->use_copy_range) {
             ret = block_copy_with_bounce_buffer(s, start, dirty_end,
-                                                is_write_notifier,
                                                 error_is_read, &bounce_buffer);
         }
         if (ret < 0) {
diff --git a/block/replication.c b/block/replication.c
index 936b2f8..99532ce 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -543,7 +543,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
 
         s->backup_job = backup_job_create(
                                 NULL, s->secondary_disk->bs, s->hidden_disk->bs,
-                                0, MIRROR_SYNC_MODE_NONE, NULL, 0, false,
+                                0, MIRROR_SYNC_MODE_NONE, NULL, 0, false, NULL,
                                 BLOCKDEV_ON_ERROR_REPORT,
                                 BLOCKDEV_ON_ERROR_REPORT, JOB_INTERNAL,
                                 backup_job_completed, bs, NULL, &local_err);