Re: [PATCH v2 11/20] qapi: backup: add x-max-chunk and x-max-workers parameters
04.11.2020 20:19, Max Reitz wrote:
On 22.10.20 22:35, Vladimir Sementsov-Ogievskiy wrote:
22.07.2020 15:22, Max Reitz wrote:
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
---
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(-)
[..]
@@ -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?
Hmm.. It makes sense, but I don't know what to write: should be >= job
cluster_size? But then I'll have to describe the logic of
backup_calculate_cluster_size()...
Isn’t the logic basically just “cluster size of the target image file
(min 64k)”? The other cases just cover error cases, and they always
return 64k, which would effectively be documented by stating that 64k is
the minimum.
But in the common cases (qcow2 or raw), we’ll never get an error anyway.
I think it’d be good to describe the cluster size somewhere, because
otherwise I find it a bit malicious to throw an error at the user that
they could not have anticipated because they have no idea what valid
values for the parameter are (until they try).
OK
(And perhaps the fact
that without copy offloading, we’ll never copy anything bigger than one
cluster at a time anyway?)
This is a parameter for background copying. Look at
block_copy_task_create(), if call_state has own max_chunk, it's used
instead of common copy_size (derived from cluster_size). But at a moment
of this patch background process through async block-copy is not yet
implemented, and the parameter doesn't work, which is described in
commit message.
Ah, OK. Right.
Max
--
Best regards,
Vladimir
Re: [PATCH v2 11/20] qapi: backup: add x-max-chunk and x-max-workers parameters
On 22.10.20 22:35, Vladimir Sementsov-Ogievskiy wrote:
> 22.07.2020 15:22, Max Reitz wrote:
>> 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
>>> ---
>>> 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(-)
>>>
>
> [..]
>
>>> @@ -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?
>
> Hmm.. It makes sense, but I don't know what to write: should be >= job
> cluster_size? But then I'll have to describe the logic of
> backup_calculate_cluster_size()...
Isn’t the logic basically just “cluster size of the target image file
(min 64k)”? The other cases just cover error cases, and they always
return 64k, which would effectively be documented by stating that 64k is
the minimum.
But in the common cases (qcow2 or raw), we’ll never get an error anyway.
I think it’d be good to describe the cluster size somewhere, because
otherwise I find it a bit malicious to throw an error at the user that
they could not have anticipated because they have no idea what valid
values for the parameter are (until they try).
>> (And perhaps the fact
>> that without copy offloading, we’ll never copy anything bigger than one
>> cluster at a time anyway?)
>
> This is a parameter for background copying. Look at
> block_copy_task_create(), if call_state has own max_chunk, it's used
> instead of common copy_size (derived from cluster_size). But at a moment
> of this patch background process through async block-copy is not yet
> implemented, and the parameter doesn't work, which is described in
> commit message.
Ah, OK. Right.
Max
Re: [PATCH v2 11/20] qapi: backup: add x-max-chunk and x-max-workers parameters
22.07.2020 15:22, Max Reitz wrote:
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
---
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(-)
[..]
@@ -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?
Hmm.. It makes sense, but I don't know what to write: should be >= job
cluster_size? But then I'll have to describe the logic of
backup_calculate_cluster_size()...
(And perhaps the fact
that without copy offloading, we’ll never copy anything bigger than one
cluster at a time anyway?)
This is a parameter for background copying. Look at block_copy_task_create(),
if call_state has own max_chunk, it's used instead of common copy_size (derived
from cluster_size). But at a moment of this patch background process through
async block-copy is not yet implemented, and the parameter doesn't work, which
is described in commit message.
+return NULL;
+}
/*
[..]
--
Best regards,
Vladimir
Re: [PATCH v2 11/20] qapi: backup: add x-max-chunk and x-max-workers parameters
On 22.07.20 14:22, Max Reitz wrote: > 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 >> --- >> 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/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? Ah, oops. Saw your own reply only now. Yep, 1 worker would be nice. :) signature.asc Description: OpenPGP digital signature
Re: [PATCH v2 11/20] qapi: backup: add x-max-chunk and x-max-workers parameters
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
> ---
> 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;
> +}
Re: [PATCH v2 11/20] qapi: backup: add x-max-chunk and x-max-workers parameters
01.06.2020 21: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 --- 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(-) [..] --- 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, must be true, 1, 0, to setup 1 worker. 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 28145afe7d..cf068d20fa 100644 -- Best regards, Vladimir
[PATCH v2 11/20] qapi: backup: add x-max-chunk and x-max-workers parameters
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
---
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)
+#
+# @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");
+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,
