[PATCH v3 11/25] qapi: backup: add max-chunk and max-workers to x-perf struct

Vladimir Sementsov-Ogievskiy posted 25 patches 5 years ago
There is a newer version of this series
[PATCH v3 11/25] qapi: backup: add max-chunk and max-workers to x-perf struct
Posted by Vladimir Sementsov-Ogievskiy 5 years ago
Add new parameters to configure future backup features. The patch
doesn't introduce aio backup requests (so we actually have only one
worker) neither requests larger than one cluster. Still, formally we
satisfy these maximums anyway, so add the parameters now, to facilitate
further patch which will really change backup job behavior.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block-core.json | 11 ++++++++++-
 block/backup.c       | 28 +++++++++++++++++++++++-----
 block/replication.c  |  2 +-
 blockdev.c           |  8 +++++++-
 4 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 71e6faa15c..5a21c24c1d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1378,10 +1378,19 @@
 #
 # @use-copy-range: Use copy offloading. Default true.
 #
+# @max-workers: Maximum number of parallel requests for the sustained background
+#               copying process. Doesn't influence copy-before-write operations.
+#               Default 64.
+#
+# @max-chunk: Maximum request length for the sustained background copying
+#             process. Doesn't influence copy-before-write operations.
+#             0 means unlimited. Default 0.
+#
 # Since: 5.2
 ##
 { 'struct': 'BackupPerf',
-  'data': { '*use-copy-range': 'bool' }}
+  'data': { '*use-copy-range': 'bool',
+            '*max-workers': 'int', '*max-chunk': 'int64' } }
 
 ##
 # @BackupCommon:
diff --git a/block/backup.c b/block/backup.c
index 09ff5a92ef..8c67d77504 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -388,6 +388,29 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
         return NULL;
     }
 
+    cluster_size = backup_calculate_cluster_size(target, errp);
+    if (cluster_size < 0) {
+        goto error;
+    }
+
+    if (perf->max_workers < 1) {
+        error_setg(errp, "max-worker must be greater than zero");
+        return NULL;
+    }
+
+    if (perf->max_chunk < 0) {
+        error_setg(errp, "max-chunk must be zero (which means no limit) or "
+                   "positive");
+        return NULL;
+    }
+
+    if (perf->max_chunk && perf->max_chunk < cluster_size) {
+        error_setg(errp, "Required max-chunk (%" PRIi64 ") is less than backup "
+                   "cluster size (%" PRIi64 ")", perf->max_chunk, cluster_size);
+        return NULL;
+    }
+
+
     if (sync_bitmap) {
         /* If we need to write to this bitmap, check that we can: */
         if (bitmap_mode != BITMAP_SYNC_MODE_NEVER &&
@@ -420,11 +443,6 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
         goto error;
     }
 
-    cluster_size = backup_calculate_cluster_size(target, errp);
-    if (cluster_size < 0) {
-        goto error;
-    }
-
     /*
      * If source is in backing chain of target assume that target is going to be
      * used for "image fleecing", i.e. it should represent a kind of snapshot of
diff --git a/block/replication.c b/block/replication.c
index 22ffc811ee..97be7ef4de 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -454,7 +454,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
     int64_t active_length, hidden_length, disk_length;
     AioContext *aio_context;
     Error *local_err = NULL;
-    BackupPerf perf = { .use_copy_range = true };
+    BackupPerf perf = { .use_copy_range = true, .max_workers = 1 };
 
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
diff --git a/blockdev.c b/blockdev.c
index b71ed08a3b..0ed390abe0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2788,7 +2788,7 @@ static BlockJob *do_backup_common(BackupCommon *backup,
 {
     BlockJob *job = NULL;
     BdrvDirtyBitmap *bmap = NULL;
-    BackupPerf perf = { .use_copy_range = true };
+    BackupPerf perf = { .use_copy_range = true, .max_workers = 64 };
     int job_flags = JOB_DEFAULT;
 
     if (!backup->has_speed) {
@@ -2817,6 +2817,12 @@ static BlockJob *do_backup_common(BackupCommon *backup,
         if (backup->x_perf->has_use_copy_range) {
             perf.use_copy_range = backup->x_perf->use_copy_range;
         }
+        if (backup->x_perf->has_max_workers) {
+            perf.max_workers = backup->x_perf->max_workers;
+        }
+        if (backup->x_perf->has_max_chunk) {
+            perf.max_chunk = backup->x_perf->max_chunk;
+        }
     }
 
     if ((backup->sync == MIRROR_SYNC_MODE_BITMAP) ||
-- 
2.21.3


Re: [PATCH v3 11/25] qapi: backup: add max-chunk and max-workers to x-perf struct
Posted by Vladimir Sementsov-Ogievskiy 5 years ago
26.10.2020 20:18, Vladimir Sementsov-Ogievskiy wrote:
> Add new parameters to configure future backup features. The patch
> doesn't introduce aio backup requests (so we actually have only one
> worker) neither requests larger than one cluster. Still, formally we
> satisfy these maximums anyway, so add the parameters now, to facilitate
> further patch which will really change backup job behavior.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   qapi/block-core.json | 11 ++++++++++-
>   block/backup.c       | 28 +++++++++++++++++++++++-----
>   block/replication.c  |  2 +-
>   blockdev.c           |  8 +++++++-
>   4 files changed, 41 insertions(+), 8 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 71e6faa15c..5a21c24c1d 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1378,10 +1378,19 @@
>   #
>   # @use-copy-range: Use copy offloading. Default true.
>   #
> +# @max-workers: Maximum number of parallel requests for the sustained background
> +#               copying process. Doesn't influence copy-before-write operations.
> +#               Default 64.
> +#
> +# @max-chunk: Maximum request length for the sustained background copying
> +#             process. Doesn't influence copy-before-write operations.
> +#             0 means unlimited. Default 0.
> +#
>   # Since: 5.2
>   ##
>   { 'struct': 'BackupPerf',
> -  'data': { '*use-copy-range': 'bool' }}
> +  'data': { '*use-copy-range': 'bool',
> +            '*max-workers': 'int', '*max-chunk': 'int64' } }
>   
>   ##
>   # @BackupCommon:

squash-in:

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5591189b75..276ad5e6a8 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1406,7 +1406,9 @@
  #
  # @max-chunk: Maximum request length for the sustained background copying
  #             process. Doesn't influence copy-before-write operations.
-#             0 means unlimited. Default 0.
+#             0 means unlimited. If max-chunk is non-zero then it should not be
+#             less than job cluster size which is calculated as maximum of
+#             target image cluster size and 64k. Default 0.
  #
  # Since: 5.2
  ##


-- 
Best regards,
Vladimir

Re: [PATCH v3 11/25] qapi: backup: add max-chunk and max-workers to x-perf struct
Posted by Max Reitz 4 years, 10 months ago
On 26.10.20 18:18, Vladimir Sementsov-Ogievskiy wrote:
> Add new parameters to configure future backup features. The patch
> doesn't introduce aio backup requests (so we actually have only one
> worker) neither requests larger than one cluster. Still, formally we
> satisfy these maximums anyway, so add the parameters now, to facilitate
> further patch which will really change backup job behavior.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   qapi/block-core.json | 11 ++++++++++-
>   block/backup.c       | 28 +++++++++++++++++++++++-----
>   block/replication.c  |  2 +-
>   blockdev.c           |  8 +++++++-
>   4 files changed, 41 insertions(+), 8 deletions(-)

[...]

> diff --git a/block/backup.c b/block/backup.c
> index 09ff5a92ef..8c67d77504 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -388,6 +388,29 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
>           return NULL;
>       }
>   
> +    cluster_size = backup_calculate_cluster_size(target, errp);
> +    if (cluster_size < 0) {
> +        goto error;
> +    }
> +
> +    if (perf->max_workers < 1) {
> +        error_setg(errp, "max-worker must be greater than zero");

*max-workers

With that fixed:

Reviewed-by: Max Reitz <mreitz@redhat.com>

> +        return NULL;
> +    }