Re: [PATCH v2 11/20] qapi: backup: add x-max-chunk and x-max-workers parameters

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 7508 bytes --]

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);


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]