diff mbox series

[v4,17/18] qapi: backup: add immutable-source parameter

Message ID 20220216194617.126484-18-vsementsov@virtuozzo.com (mailing list archive)
State New, archived
Headers show
Series Make image fleecing more usable | expand

Commit Message

Vladimir Sementsov-Ogievskiy Feb. 16, 2022, 7:46 p.m. UTC
We are on the way to implement internal-backup with fleecing scheme,
which includes backup job copying from fleecing block driver node
(which is target of copy-before-write filter) to final target of
backup. This job doesn't need own filter, as fleecing block driver node
is a kind of snapshot, it's immutable from reader point of view.

Let's add a parameter for backup to not insert filter but instead
unshare writes on source. This way backup job becomes a simple copying
process.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block-core.json      | 11 ++++++-
 include/block/block_int.h |  1 +
 block/backup.c            | 61 +++++++++++++++++++++++++++++++++++----
 block/replication.c       |  2 +-
 blockdev.c                |  1 +
 5 files changed, 69 insertions(+), 7 deletions(-)

Comments

Hanna Czenczek Feb. 24, 2022, 1:05 p.m. UTC | #1
On 16.02.22 20:46, Vladimir Sementsov-Ogievskiy wrote:
> We are on the way to implement internal-backup with fleecing scheme,
> which includes backup job copying from fleecing block driver node
> (which is target of copy-before-write filter) to final target of
> backup. This job doesn't need own filter, as fleecing block driver node
> is a kind of snapshot, it's immutable from reader point of view.
>
> Let's add a parameter for backup to not insert filter but instead
> unshare writes on source. This way backup job becomes a simple copying
> process.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   qapi/block-core.json      | 11 ++++++-
>   include/block/block_int.h |  1 +
>   block/backup.c            | 61 +++++++++++++++++++++++++++++++++++----
>   block/replication.c       |  2 +-
>   blockdev.c                |  1 +
>   5 files changed, 69 insertions(+), 7 deletions(-)

I’m not really technically opposed to this, but I wonder what the actual 
benefit of this is.  It sounds like the only benefit is that we don’t 
need a filter driver, but what’s the problem with such a filter driver?

(And if we just want to copy data off of a immutable node, I personally 
would go for the mirror job instead, but it isn’t like I could give good 
technical reasons for that personal bias.)
Vladimir Sementsov-Ogievskiy Feb. 24, 2022, 2:14 p.m. UTC | #2
24.02.2022 16:05, Hanna Reitz wrote:
> On 16.02.22 20:46, Vladimir Sementsov-Ogievskiy wrote:
>> We are on the way to implement internal-backup with fleecing scheme,
>> which includes backup job copying from fleecing block driver node
>> (which is target of copy-before-write filter) to final target of
>> backup. This job doesn't need own filter, as fleecing block driver node
>> is a kind of snapshot, it's immutable from reader point of view.
>>
>> Let's add a parameter for backup to not insert filter but instead
>> unshare writes on source. This way backup job becomes a simple copying
>> process.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qapi/block-core.json      | 11 ++++++-
>>   include/block/block_int.h |  1 +
>>   block/backup.c            | 61 +++++++++++++++++++++++++++++++++++----
>>   block/replication.c       |  2 +-
>>   blockdev.c                |  1 +
>>   5 files changed, 69 insertions(+), 7 deletions(-)
> 
> I’m not really technically opposed to this, but I wonder what the actual benefit of this is.  It sounds like the only benefit is that we don’t need a filter driver, but what’s the problem with such a filter driver?

Hmm. Yes, that's the only benefit: less extra components -> more stability.

But I doubt now does it really worth extra parameter.. More parameters that actually change nothing for the user -> less stability :)

Ok, I think I at least should postpone it for now, this series is too fat even without this patch.

The only possible problem - will permission conflict happen in the next test without this patch? But if it will, the solution should exist to solve it without user interaction. I'll check and try to avoid this new parameter.

> 
> (And if we just want to copy data off of a immutable node, I personally would go for the mirror job instead, but it isn’t like I could give good technical reasons for that personal bias.)
> 

I still hope that in far good future mirror will work through block/block-copy like backup, and there would be no difference what to use for immutable source copying.


Thanks a lot for reviewing!
diff mbox series

Patch

diff --git a/qapi/block-core.json b/qapi/block-core.json
index a904755e98..30d44683bf 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1436,6 +1436,15 @@ 
 #                    above node specified by @drive. If this option is not given,
 #                    a node name is autogenerated. (Since: 4.2)
 #
+# @immutable-source: If true, assume source is immutable, and don't insert filter
+#                    as no copy-before-write operations are needed. It will
+#                    fail if there are existing writers on source node.
+#                    Any attempt to add writer to source node during backup will
+#                    also fail. @filter-node-name must not be set.
+#                    If false, insert copy-before-write filter above source node
+#                    (see also @filter-node-name parameter).
+#                    Default is false. (Since 6.2)
+#
 # @x-perf: Performance options. (Since 6.0)
 #
 # Features:
@@ -1455,7 +1464,7 @@ 
             '*on-source-error': 'BlockdevOnError',
             '*on-target-error': 'BlockdevOnError',
             '*auto-finalize': 'bool', '*auto-dismiss': 'bool',
-            '*filter-node-name': 'str',
+            '*filter-node-name': 'str', '*immutable-source': 'bool',
             '*x-perf': { 'type': 'BackupPerf',
                          'features': [ 'unstable' ] } } }
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index c43315ae6e..0270af29ae 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1348,6 +1348,7 @@  BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
                             BitmapSyncMode bitmap_mode,
                             bool compress,
                             const char *filter_node_name,
+                            bool immutable_source,
                             BackupPerf *perf,
                             BlockdevOnError on_source_error,
                             BlockdevOnError on_target_error,
diff --git a/block/backup.c b/block/backup.c
index 21d5983779..104f8fd835 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -34,6 +34,14 @@  typedef struct BackupBlockJob {
     BlockDriverState *cbw;
     BlockDriverState *source_bs;
     BlockDriverState *target_bs;
+    BlockBackend *source_blk;
+    BlockBackend *target_blk;
+    /*
+     * Note that if backup runs with filter (immutable-source parameter is
+     * false), @cbw is set but @source_blk and @target_blk are NULL.
+     * Otherwise if backup runs without filter (immutable-source paramter is
+     * true), @cbw is NULL but @source_blk and @target_blk are set.
+     */
 
     BdrvDirtyBitmap *sync_bitmap;
 
@@ -102,7 +110,17 @@  static void backup_clean(Job *job)
 {
     BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
     block_job_remove_all_bdrv(&s->common);
-    bdrv_cbw_drop(s->cbw);
+    if (s->cbw) {
+        assert(!s->source_blk && !s->target_blk);
+        bdrv_cbw_drop(s->cbw);
+    } else {
+        block_copy_state_free(s->bcs);
+        s->bcs = NULL;
+        blk_unref(s->source_blk);
+        s->source_blk = NULL;
+        blk_unref(s->target_blk);
+        s->target_blk = NULL;
+    }
 }
 
 void backup_do_checkpoint(BlockJob *job, Error **errp)
@@ -357,6 +375,7 @@  BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
                   BitmapSyncMode bitmap_mode,
                   bool compress,
                   const char *filter_node_name,
+                  bool immutable_source,
                   BackupPerf *perf,
                   BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
@@ -369,6 +388,7 @@  BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     int64_t cluster_size;
     BlockDriverState *cbw = NULL;
     BlockCopyState *bcs = NULL;
+    BlockBackend *source_blk = NULL, *target_blk = NULL;
 
     assert(bs);
     assert(target);
@@ -377,6 +397,12 @@  BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     assert(sync_mode != MIRROR_SYNC_MODE_INCREMENTAL);
     assert(sync_bitmap || sync_mode != MIRROR_SYNC_MODE_BITMAP);
 
+    if (immutable_source && filter_node_name) {
+        error_setg(errp, "immutable-source and filter-node-name should not "
+                   "be set simultaneously");
+        return NULL;
+    }
+
     if (bs == target) {
         error_setg(errp, "Source and target cannot be the same");
         return NULL;
@@ -451,9 +477,30 @@  BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
         goto error;
     }
 
-    cbw = bdrv_cbw_append(bs, target, filter_node_name, &bcs, errp);
-    if (!cbw) {
-        goto error;
+    if (immutable_source) {
+        source_blk = blk_new_with_bs(bs, BLK_PERM_CONSISTENT_READ,
+                                        BLK_PERM_WRITE_UNCHANGED |
+                                        BLK_PERM_CONSISTENT_READ, errp);
+        if (!source_blk) {
+            goto error;
+        }
+
+        target_blk  = blk_new_with_bs(target, BLK_PERM_WRITE,
+                                      BLK_PERM_CONSISTENT_READ, errp);
+        if (!target_blk) {
+            goto error;
+        }
+
+        bcs = block_copy_state_new(blk_root(source_blk), blk_root(target_blk),
+                                   NULL, errp);
+        if (!bcs) {
+            goto error;
+        }
+    } else {
+        cbw = bdrv_cbw_append(bs, target, filter_node_name, &bcs, errp);
+        if (!cbw) {
+            goto error;
+        }
     }
 
     cluster_size = block_copy_cluster_size(bcs);
@@ -465,7 +512,7 @@  BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     }
 
     /* job->len is fixed, so we can't allow resize */
-    job = block_job_create(job_id, &backup_job_driver, txn, cbw,
+    job = block_job_create(job_id, &backup_job_driver, txn, cbw ?: bs,
                            0, BLK_PERM_ALL,
                            speed, creation_flags, cb, opaque, errp);
     if (!job) {
@@ -475,6 +522,8 @@  BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     job->cbw = cbw;
     job->source_bs = bs;
     job->target_bs = target;
+    job->source_blk = source_blk;
+    job->target_blk = target_blk;
     job->on_source_error = on_source_error;
     job->on_target_error = on_target_error;
     job->sync_mode = sync_mode;
@@ -502,6 +551,8 @@  BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     if (cbw) {
         bdrv_cbw_drop(cbw);
     }
+    blk_unref(source_blk);
+    blk_unref(target_blk);
 
     return NULL;
 }
diff --git a/block/replication.c b/block/replication.c
index 55c8f894aa..c6c4d3af85 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -590,7 +590,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, NULL,
-                                &perf,
+                                false, &perf,
                                 BLOCKDEV_ON_ERROR_REPORT,
                                 BLOCKDEV_ON_ERROR_REPORT, JOB_INTERNAL,
                                 backup_job_completed, bs, NULL, &local_err);
diff --git a/blockdev.c b/blockdev.c
index 42e098b458..6997eccb4d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2878,6 +2878,7 @@  static BlockJob *do_backup_common(BackupCommon *backup,
                             backup->sync, bmap, backup->bitmap_mode,
                             backup->compress,
                             backup->filter_node_name,
+                            backup->immutable_source,
                             &perf,
                             backup->on_source_error,
                             backup->on_target_error,