[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 11/20] qapi: backup: add x-max-chunk and x-max-workers par
From: |
Max Reitz |
Subject: |
Re: [PATCH v2 11/20] qapi: backup: add x-max-chunk and x-max-workers parameters |
Date: |
Wed, 22 Jul 2020 14:22:35 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 |
On 01.06.20 20:11, 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.
>
> Options are added with x- prefixes, as the only use for them are some
> very conservative iotests which will be updated soon.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> qapi/block-core.json | 9 ++++++++-
> include/block/block_int.h | 7 +++++++
> block/backup.c | 21 +++++++++++++++++++++
> block/replication.c | 2 +-
> blockdev.c | 5 +++++
> 5 files changed, 42 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 0c7600e4ec..f4abcde34e 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1407,6 +1407,12 @@
> #
> # @x-use-copy-range: use copy offloading if possible. Default true. (Since
> 5.1)
> #
> +# @x-max-workers: maximum of parallel requests for static data backup. This
> +# doesn't influence copy-before-write operations. (Since:
> 5.1)
I think I’d prefer something with “background” rather than or in
addition to “static”, like “maximum number of parallel requests for the
sustained background backup operation”.
> +#
> +# @x-max-chunk: maximum chunk length for static data backup. This doesn't
> +# influence copy-before-write operations. (Since: 5.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.
> @@ -1421,7 +1427,8 @@
> '*on-source-error': 'BlockdevOnError',
> '*on-target-error': 'BlockdevOnError',
> '*auto-finalize': 'bool', '*auto-dismiss': 'bool',
> - '*filter-node-name': 'str', '*x-use-copy-range': 'bool' } }
> + '*filter-node-name': 'str', '*x-use-copy-range': 'bool',
> + '*x-max-workers': 'int', '*x-max-chunk': 'int64' } }
>
> ##
> # @DriveBackup:
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 93b9b3bdc0..d93a170d37 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -1207,6 +1207,11 @@ void mirror_start(const char *job_id, BlockDriverState
> *bs,
> * @sync_mode: What parts of the disk image should be copied to the
> destination.
> * @sync_bitmap: The dirty bitmap if sync_mode is 'bitmap' or 'incremental'
> * @bitmap_mode: The bitmap synchronization policy to use.
> + * @max_workers: The limit for parallel requests for main backup loop.
> + * Must be >= 1.
> + * @max_chunk: The limit for one IO operation length in main backup loop.
> + * Must be not less than job cluster size or zero. Zero means no
> + * specific limit.
> * @on_source_error: The action to take upon error reading from the source.
> * @on_target_error: The action to take upon error writing to the target.
> * @creation_flags: Flags that control the behavior of the Job lifetime.
> @@ -1226,6 +1231,8 @@ BlockJob *backup_job_create(const char *job_id,
> BlockDriverState *bs,
> bool compress,
> const char *filter_node_name,
> bool use_copy_range,
> + int max_workers,
> + int64_t max_chunk,
> BlockdevOnError on_source_error,
> BlockdevOnError on_target_error,
> int creation_flags,
> diff --git a/block/backup.c b/block/backup.c
> index 76847b4daf..ec2676abc2 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -46,6 +46,8 @@ typedef struct BackupBlockJob {
> uint64_t len;
> uint64_t bytes_read;
> int64_t cluster_size;
> + int max_workers;
> + int64_t max_chunk;
>
> BlockCopyState *bcs;
> } BackupBlockJob;
> @@ -335,6 +337,8 @@ BlockJob *backup_job_create(const char *job_id,
> BlockDriverState *bs,
> bool compress,
> const char *filter_node_name,
> bool use_copy_range,
> + int max_workers,
> + int64_t max_chunk,
> BlockdevOnError on_source_error,
> BlockdevOnError on_target_error,
> int creation_flags,
> @@ -355,6 +359,16 @@ 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 (max_workers < 1) {
> + error_setg(errp, "At least one worker needed");
> + return NULL;
> + }
> +
> + if (max_chunk < 0) {
> + error_setg(errp, "max-chunk is negative");
Perhaps “must be positive or 0” instead? I think most error messages
try to specify what is allowed instead of what isn’t.
> + return NULL;
> + }
> +
> if (bs == target) {
> error_setg(errp, "Source and target cannot be the same");
> return NULL;
> @@ -422,6 +436,11 @@ BlockJob *backup_job_create(const char *job_id,
> BlockDriverState *bs,
> if (cluster_size < 0) {
> goto error;
> }
> + if (max_chunk && max_chunk < cluster_size) {
> + error_setg(errp, "Required max-chunk (%" PRIi64") is less than
> backup "
(missing a space after PRIi64)
> + "cluster size (%" PRIi64 ")", max_chunk, cluster_size);
Should this be noted in the QAPI documentation? (And perhaps the fact
that without copy offloading, we’ll never copy anything bigger than one
cluster at a time anyway?)
> + return NULL;
> + }
>
> /*
> * If source is in backing chain of target assume that target is going
> to be
> @@ -465,6 +484,8 @@ BlockJob *backup_job_create(const char *job_id,
> BlockDriverState *bs,
> job->bcs = bcs;
> job->cluster_size = cluster_size;
> job->len = len;
> + job->max_workers = max_workers;
> + job->max_chunk = max_chunk;
>
> block_copy_set_progress_callback(bcs, backup_progress_bytes_callback,
> job);
> block_copy_set_progress_meter(bcs, &job->common.job.progress);
> diff --git a/block/replication.c b/block/replication.c
> index 25987eab0f..a9ee82db80 100644
> --- a/block/replication.c
> +++ b/block/replication.c
> @@ -563,7 +563,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,
> - true,
> + true, 0, 0,
Passing 0 for max_workers will error out immediately, won’t it?
> BLOCKDEV_ON_ERROR_REPORT,
> BLOCKDEV_ON_ERROR_REPORT, JOB_INTERNAL,
> backup_job_completed, bs, NULL, &local_err);
signature.asc
Description: OpenPGP digital signature
- Re: [PATCH v2 11/20] qapi: backup: add x-max-chunk and x-max-workers parameters,
Max Reitz <=