* [PATCH 0/2] backup: allow specifying minimum cluster size
@ 2024-03-08 15:51 Fiona Ebner
2024-03-08 15:51 ` [PATCH 1/2] copy-before-write: " Fiona Ebner
2024-03-08 15:51 ` [PATCH 2/2] backup: add minimum cluster size to performance options Fiona Ebner
0 siblings, 2 replies; 8+ messages in thread
From: Fiona Ebner @ 2024-03-08 15:51 UTC (permalink / raw
To: qemu-devel; +Cc: qemu-block, armbru, eblake, hreitz, kwolf, vsementsov, jsnow
Based-on: https://lore.kernel.org/qemu-devel/20240228141501.455989-1-vsementsov@yandex-team.ru/
Useful to make discard-source work in the context of backup fleecing
when the fleecing image has a larger granularity than the backup
target.
Backup/block-copy will use at least this granularity for copy operations
and in particular, discard requests to the backup source will too. If
the granularity is too small, they will just be aligned down in
cbw_co_pdiscard_snapshot() and thus effectively ignored.
Fiona Ebner (2):
copy-before-write: allow specifying minimum cluster size
backup: add minimum cluster size to performance options
block/backup.c | 2 +-
block/block-copy.c | 17 +++++++++++++----
block/copy-before-write.c | 5 ++++-
block/copy-before-write.h | 1 +
blockdev.c | 3 +++
include/block/block-copy.h | 1 +
qapi/block-core.json | 17 ++++++++++++++---
7 files changed, 37 insertions(+), 9 deletions(-)
--
2.39.2
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] copy-before-write: allow specifying minimum cluster size
2024-03-08 15:51 [PATCH 0/2] backup: allow specifying minimum cluster size Fiona Ebner
@ 2024-03-08 15:51 ` Fiona Ebner
2024-03-26 9:06 ` Markus Armbruster
2024-03-29 10:10 ` Vladimir Sementsov-Ogievskiy
2024-03-08 15:51 ` [PATCH 2/2] backup: add minimum cluster size to performance options Fiona Ebner
1 sibling, 2 replies; 8+ messages in thread
From: Fiona Ebner @ 2024-03-08 15:51 UTC (permalink / raw
To: qemu-devel; +Cc: qemu-block, armbru, eblake, hreitz, kwolf, vsementsov, jsnow
Useful to make discard-source work in the context of backup fleecing
when the fleecing image has a larger granularity than the backup
target.
Copy-before-write operations will use at least this granularity and in
particular, discard requests to the source node will too. If the
granularity is too small, they will just be aligned down in
cbw_co_pdiscard_snapshot() and thus effectively ignored.
The QAPI uses uint32 so the value will be non-negative, but still fit
into a uint64_t.
Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
block/block-copy.c | 17 +++++++++++++----
block/copy-before-write.c | 3 ++-
include/block/block-copy.h | 1 +
qapi/block-core.json | 8 +++++++-
4 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/block/block-copy.c b/block/block-copy.c
index 7e3b378528..adb1cbb440 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -310,6 +310,7 @@ void block_copy_set_copy_opts(BlockCopyState *s, bool use_copy_range,
}
static int64_t block_copy_calculate_cluster_size(BlockDriverState *target,
+ int64_t min_cluster_size,
Error **errp)
{
int ret;
@@ -335,7 +336,7 @@ static int64_t block_copy_calculate_cluster_size(BlockDriverState *target,
"used. If the actual block size of the target exceeds "
"this default, the backup may be unusable",
BLOCK_COPY_CLUSTER_SIZE_DEFAULT);
- return BLOCK_COPY_CLUSTER_SIZE_DEFAULT;
+ return MAX(min_cluster_size, BLOCK_COPY_CLUSTER_SIZE_DEFAULT);
} else if (ret < 0 && !target_does_cow) {
error_setg_errno(errp, -ret,
"Couldn't determine the cluster size of the target image, "
@@ -345,16 +346,18 @@ static int64_t block_copy_calculate_cluster_size(BlockDriverState *target,
return ret;
} else if (ret < 0 && target_does_cow) {
/* Not fatal; just trudge on ahead. */
- return BLOCK_COPY_CLUSTER_SIZE_DEFAULT;
+ return MAX(min_cluster_size, BLOCK_COPY_CLUSTER_SIZE_DEFAULT);
}
- return MAX(BLOCK_COPY_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
+ return MAX(min_cluster_size,
+ MAX(BLOCK_COPY_CLUSTER_SIZE_DEFAULT, bdi.cluster_size));
}
BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
BlockDriverState *copy_bitmap_bs,
const BdrvDirtyBitmap *bitmap,
bool discard_source,
+ int64_t min_cluster_size,
Error **errp)
{
ERRP_GUARD();
@@ -365,7 +368,13 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
GLOBAL_STATE_CODE();
- cluster_size = block_copy_calculate_cluster_size(target->bs, errp);
+ if (min_cluster_size && !is_power_of_2(min_cluster_size)) {
+ error_setg(errp, "min-cluster-size needs to be a power of 2");
+ return NULL;
+ }
+
+ cluster_size = block_copy_calculate_cluster_size(target->bs,
+ min_cluster_size, errp);
if (cluster_size < 0) {
return NULL;
}
diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index dac57481c5..f9896c6c1e 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -476,7 +476,8 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
s->discard_source = flags & BDRV_O_CBW_DISCARD_SOURCE;
s->bcs = block_copy_state_new(bs->file, s->target, bs, bitmap,
- flags & BDRV_O_CBW_DISCARD_SOURCE, errp);
+ flags & BDRV_O_CBW_DISCARD_SOURCE,
+ opts->min_cluster_size, errp);
if (!s->bcs) {
error_prepend(errp, "Cannot create block-copy-state: ");
return -EINVAL;
diff --git a/include/block/block-copy.h b/include/block/block-copy.h
index bdc703bacd..77857c6c68 100644
--- a/include/block/block-copy.h
+++ b/include/block/block-copy.h
@@ -28,6 +28,7 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
BlockDriverState *copy_bitmap_bs,
const BdrvDirtyBitmap *bitmap,
bool discard_source,
+ int64_t min_cluster_size,
Error **errp);
/* Function should be called prior any actual copy request */
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0a72c590a8..85c8f88f6e 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4625,12 +4625,18 @@
# @on-cbw-error parameter will decide how this failure is handled.
# Default 0. (Since 7.1)
#
+# @min-cluster-size: Minimum size of blocks used by copy-before-write
+# operations. Has to be a power of 2. No effect if smaller than
+# the maximum of the target's cluster size and 64 KiB. Default 0.
+# (Since 9.0)
+#
# Since: 6.2
##
{ 'struct': 'BlockdevOptionsCbw',
'base': 'BlockdevOptionsGenericFormat',
'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap',
- '*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32' } }
+ '*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32',
+ '*min-cluster-size': 'uint32' } }
##
# @BlockdevOptions:
--
2.39.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] backup: add minimum cluster size to performance options
2024-03-08 15:51 [PATCH 0/2] backup: allow specifying minimum cluster size Fiona Ebner
2024-03-08 15:51 ` [PATCH 1/2] copy-before-write: " Fiona Ebner
@ 2024-03-08 15:51 ` Fiona Ebner
2024-03-29 10:15 ` Vladimir Sementsov-Ogievskiy
1 sibling, 1 reply; 8+ messages in thread
From: Fiona Ebner @ 2024-03-08 15:51 UTC (permalink / raw
To: qemu-devel; +Cc: qemu-block, armbru, eblake, hreitz, kwolf, vsementsov, jsnow
Useful to make discard-source work in the context of backup fleecing
when the fleecing image has a larger granularity than the backup
target.
Backup/block-copy will use at least this granularity for copy operations
and in particular, discard requests to the backup source will too. If
the granularity is too small, they will just be aligned down in
cbw_co_pdiscard_snapshot() and thus effectively ignored.
Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
block/backup.c | 2 +-
block/copy-before-write.c | 2 ++
block/copy-before-write.h | 1 +
blockdev.c | 3 +++
qapi/block-core.json | 9 +++++++--
5 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 3dd2e229d2..a1292c01ec 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -458,7 +458,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
}
cbw = bdrv_cbw_append(bs, target, filter_node_name, discard_source,
- &bcs, errp);
+ perf->min_cluster_size, &bcs, errp);
if (!cbw) {
goto error;
}
diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index f9896c6c1e..55a9272485 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -545,6 +545,7 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source,
BlockDriverState *target,
const char *filter_node_name,
bool discard_source,
+ int64_t min_cluster_size,
BlockCopyState **bcs,
Error **errp)
{
@@ -563,6 +564,7 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source,
}
qdict_put_str(opts, "file", bdrv_get_node_name(source));
qdict_put_str(opts, "target", bdrv_get_node_name(target));
+ qdict_put_int(opts, "min-cluster-size", min_cluster_size);
top = bdrv_insert_node(source, opts, flags, errp);
if (!top) {
diff --git a/block/copy-before-write.h b/block/copy-before-write.h
index 01af0cd3c4..dc6cafe7fa 100644
--- a/block/copy-before-write.h
+++ b/block/copy-before-write.h
@@ -40,6 +40,7 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source,
BlockDriverState *target,
const char *filter_node_name,
bool discard_source,
+ int64_t min_cluster_size,
BlockCopyState **bcs,
Error **errp);
void bdrv_cbw_drop(BlockDriverState *bs);
diff --git a/blockdev.c b/blockdev.c
index daceb50460..8e6bdbc94a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2653,6 +2653,9 @@ static BlockJob *do_backup_common(BackupCommon *backup,
if (backup->x_perf->has_max_chunk) {
perf.max_chunk = backup->x_perf->max_chunk;
}
+ if (backup->x_perf->has_min_cluster_size) {
+ perf.min_cluster_size = backup->x_perf->min_cluster_size;
+ }
}
if ((backup->sync == MIRROR_SYNC_MODE_BITMAP) ||
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 85c8f88f6e..ba0836892f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1551,11 +1551,16 @@
# it should not be less than job cluster size which is calculated
# as maximum of target image cluster size and 64k. Default 0.
#
+# @min-cluster-size: Minimum size of blocks used by copy-before-write
+# and background copy operations. Has to be a power of 2. No
+# effect if smaller than the maximum of the target's cluster size
+# and 64 KiB. Default 0. (Since 9.0)
+#
# Since: 6.0
##
{ 'struct': 'BackupPerf',
- 'data': { '*use-copy-range': 'bool',
- '*max-workers': 'int', '*max-chunk': 'int64' } }
+ 'data': { '*use-copy-range': 'bool', '*max-workers': 'int',
+ '*max-chunk': 'int64', '*min-cluster-size': 'uint32' } }
##
# @BackupCommon:
--
2.39.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] copy-before-write: allow specifying minimum cluster size
2024-03-08 15:51 ` [PATCH 1/2] copy-before-write: " Fiona Ebner
@ 2024-03-26 9:06 ` Markus Armbruster
2024-05-13 13:24 ` Fiona Ebner
2024-03-29 10:10 ` Vladimir Sementsov-Ogievskiy
1 sibling, 1 reply; 8+ messages in thread
From: Markus Armbruster @ 2024-03-26 9:06 UTC (permalink / raw
To: Fiona Ebner
Cc: qemu-devel, qemu-block, eblake, hreitz, kwolf, vsementsov, jsnow
Fiona Ebner <f.ebner@proxmox.com> writes:
> Useful to make discard-source work in the context of backup fleecing
> when the fleecing image has a larger granularity than the backup
> target.
>
> Copy-before-write operations will use at least this granularity and in
> particular, discard requests to the source node will too. If the
> granularity is too small, they will just be aligned down in
> cbw_co_pdiscard_snapshot() and thus effectively ignored.
>
> The QAPI uses uint32 so the value will be non-negative, but still fit
> into a uint64_t.
>
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> block/block-copy.c | 17 +++++++++++++----
> block/copy-before-write.c | 3 ++-
> include/block/block-copy.h | 1 +
> qapi/block-core.json | 8 +++++++-
> 4 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/block/block-copy.c b/block/block-copy.c
> index 7e3b378528..adb1cbb440 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -310,6 +310,7 @@ void block_copy_set_copy_opts(BlockCopyState *s, bool use_copy_range,
> }
>
> static int64_t block_copy_calculate_cluster_size(BlockDriverState *target,
> + int64_t min_cluster_size,
> Error **errp)
> {
> int ret;
> @@ -335,7 +336,7 @@ static int64_t block_copy_calculate_cluster_size(BlockDriverState *target,
> "used. If the actual block size of the target exceeds "
> "this default, the backup may be unusable",
> BLOCK_COPY_CLUSTER_SIZE_DEFAULT);
> - return BLOCK_COPY_CLUSTER_SIZE_DEFAULT;
> + return MAX(min_cluster_size, BLOCK_COPY_CLUSTER_SIZE_DEFAULT);
min_cluster_size is int64_t, BLOCK_COPY_CLUSTER_SIZE_DEFAULT is int, and
gets converted to int64_t. The return type is int64_t. Okay.
> } else if (ret < 0 && !target_does_cow) {
> error_setg_errno(errp, -ret,
> "Couldn't determine the cluster size of the target image, "
> @@ -345,16 +346,18 @@ static int64_t block_copy_calculate_cluster_size(BlockDriverState *target,
> return ret;
> } else if (ret < 0 && target_does_cow) {
> /* Not fatal; just trudge on ahead. */
> - return BLOCK_COPY_CLUSTER_SIZE_DEFAULT;
> + return MAX(min_cluster_size, BLOCK_COPY_CLUSTER_SIZE_DEFAULT);
Same.
> }
>
> - return MAX(BLOCK_COPY_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
> + return MAX(min_cluster_size,
> + MAX(BLOCK_COPY_CLUSTER_SIZE_DEFAULT, bdi.cluster_size));
Similar: bdi.cluster_size is int.
> }
>
> BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
> BlockDriverState *copy_bitmap_bs,
> const BdrvDirtyBitmap *bitmap,
> bool discard_source,
> + int64_t min_cluster_size,
> Error **errp)
> {
> ERRP_GUARD();
> @@ -365,7 +368,13 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
>
> GLOBAL_STATE_CODE();
>
> - cluster_size = block_copy_calculate_cluster_size(target->bs, errp);
> + if (min_cluster_size && !is_power_of_2(min_cluster_size)) {
min_cluster_size is int64_t, is_power_of_2() takes uint64_t. Bad if
min_cluster_size is negative. Could this happen?
> + error_setg(errp, "min-cluster-size needs to be a power of 2");
> + return NULL;
> + }
> +
> + cluster_size = block_copy_calculate_cluster_size(target->bs,
> + min_cluster_size, errp);
min_cluster_size is int64_t, block_copy_calculate_cluster_size() takes
int64_t, returns int64_t, and cluster_size is int64_t. Good.
> if (cluster_size < 0) {
> return NULL;
> }
> diff --git a/block/copy-before-write.c b/block/copy-before-write.c
> index dac57481c5..f9896c6c1e 100644
> --- a/block/copy-before-write.c
> +++ b/block/copy-before-write.c
> @@ -476,7 +476,8 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
>
> s->discard_source = flags & BDRV_O_CBW_DISCARD_SOURCE;
> s->bcs = block_copy_state_new(bs->file, s->target, bs, bitmap,
> - flags & BDRV_O_CBW_DISCARD_SOURCE, errp);
> + flags & BDRV_O_CBW_DISCARD_SOURCE,
> + opts->min_cluster_size, errp);
@opts is BlockdevOptionsCbw *, opts->min_cluster_size is uint32_t (see
last hunk), block_copy_state_new() takes int64_t: opts->min_cluster_size
gets zero-extended. Okay.
> if (!s->bcs) {
> error_prepend(errp, "Cannot create block-copy-state: ");
> return -EINVAL;
> diff --git a/include/block/block-copy.h b/include/block/block-copy.h
> index bdc703bacd..77857c6c68 100644
> --- a/include/block/block-copy.h
> +++ b/include/block/block-copy.h
> @@ -28,6 +28,7 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
> BlockDriverState *copy_bitmap_bs,
> const BdrvDirtyBitmap *bitmap,
> bool discard_source,
> + int64_t min_cluster_size,
> Error **errp);
>
> /* Function should be called prior any actual copy request */
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 0a72c590a8..85c8f88f6e 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4625,12 +4625,18 @@
> # @on-cbw-error parameter will decide how this failure is handled.
> # Default 0. (Since 7.1)
> #
> +# @min-cluster-size: Minimum size of blocks used by copy-before-write
> +# operations. Has to be a power of 2. No effect if smaller than
> +# the maximum of the target's cluster size and 64 KiB. Default 0.
> +# (Since 9.0)
> +#
> # Since: 6.2
> ##
> { 'struct': 'BlockdevOptionsCbw',
> 'base': 'BlockdevOptionsGenericFormat',
> 'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap',
> - '*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32' } }
> + '*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32',
> + '*min-cluster-size': 'uint32' } }
Elsewhere in the schema, we use either 'int' or 'size' for cluster-size.
Why the difference?
>
> ##
> # @BlockdevOptions:
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] copy-before-write: allow specifying minimum cluster size
2024-03-08 15:51 ` [PATCH 1/2] copy-before-write: " Fiona Ebner
2024-03-26 9:06 ` Markus Armbruster
@ 2024-03-29 10:10 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 8+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2024-03-29 10:10 UTC (permalink / raw
To: Fiona Ebner, qemu-devel; +Cc: qemu-block, armbru, eblake, hreitz, kwolf, jsnow
On 08.03.24 18:51, Fiona Ebner wrote:
> Useful to make discard-source work in the context of backup fleecing
> when the fleecing image has a larger granularity than the backup
> target.
>
> Copy-before-write operations will use at least this granularity and in
> particular, discard requests to the source node will too. If the
> granularity is too small, they will just be aligned down in
> cbw_co_pdiscard_snapshot() and thus effectively ignored.
>
> The QAPI uses uint32 so the value will be non-negative, but still fit
> into a uint64_t.
>
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> block/block-copy.c | 17 +++++++++++++----
> block/copy-before-write.c | 3 ++-
> include/block/block-copy.h | 1 +
> qapi/block-core.json | 8 +++++++-
> 4 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/block/block-copy.c b/block/block-copy.c
> index 7e3b378528..adb1cbb440 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -310,6 +310,7 @@ void block_copy_set_copy_opts(BlockCopyState *s, bool use_copy_range,
> }
>
> static int64_t block_copy_calculate_cluster_size(BlockDriverState *target,
> + int64_t min_cluster_size,
Maybe better use uint32_t here as well.
> Error **errp)
> {
> int ret;
> @@ -335,7 +336,7 @@ static int64_t block_copy_calculate_cluster_size(BlockDriverState *target,
> "used. If the actual block size of the target exceeds "
> "this default, the backup may be unusable",
> BLOCK_COPY_CLUSTER_SIZE_DEFAULT);
> - return BLOCK_COPY_CLUSTER_SIZE_DEFAULT;
> + return MAX(min_cluster_size, BLOCK_COPY_CLUSTER_SIZE_DEFAULT);
> } else if (ret < 0 && !target_does_cow) {
> error_setg_errno(errp, -ret,
> "Couldn't determine the cluster size of the target image, "
> @@ -345,16 +346,18 @@ static int64_t block_copy_calculate_cluster_size(BlockDriverState *target,
> return ret;
> } else if (ret < 0 && target_does_cow) {
> /* Not fatal; just trudge on ahead. */
> - return BLOCK_COPY_CLUSTER_SIZE_DEFAULT;
> + return MAX(min_cluster_size, BLOCK_COPY_CLUSTER_SIZE_DEFAULT);
> }
>
> - return MAX(BLOCK_COPY_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
> + return MAX(min_cluster_size,
> + MAX(BLOCK_COPY_CLUSTER_SIZE_DEFAULT, bdi.cluster_size));
> }
>
> BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
> BlockDriverState *copy_bitmap_bs,
> const BdrvDirtyBitmap *bitmap,
> bool discard_source,
> + int64_t min_cluster_size,
and here why not uint32_t
> Error **errp)
> {
> ERRP_GUARD();
> @@ -365,7 +368,13 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
>
> GLOBAL_STATE_CODE();
>
> - cluster_size = block_copy_calculate_cluster_size(target->bs, errp);
> + if (min_cluster_size && !is_power_of_2(min_cluster_size)) {
> + error_setg(errp, "min-cluster-size needs to be a power of 2");
> + return NULL;
> + }
> +
> + cluster_size = block_copy_calculate_cluster_size(target->bs,
> + min_cluster_size, errp);
> if (cluster_size < 0) {
> return NULL;
> }
> diff --git a/block/copy-before-write.c b/block/copy-before-write.c
> index dac57481c5..f9896c6c1e 100644
> --- a/block/copy-before-write.c
> +++ b/block/copy-before-write.c
> @@ -476,7 +476,8 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
>
> s->discard_source = flags & BDRV_O_CBW_DISCARD_SOURCE;
> s->bcs = block_copy_state_new(bs->file, s->target, bs, bitmap,
> - flags & BDRV_O_CBW_DISCARD_SOURCE, errp);
> + flags & BDRV_O_CBW_DISCARD_SOURCE,
> + opts->min_cluster_size, errp);
I assume it is guaranteed to be 0 when not specified by user.
> if (!s->bcs) {
> error_prepend(errp, "Cannot create block-copy-state: ");
> return -EINVAL;
> diff --git a/include/block/block-copy.h b/include/block/block-copy.h
> index bdc703bacd..77857c6c68 100644
> --- a/include/block/block-copy.h
> +++ b/include/block/block-copy.h
> @@ -28,6 +28,7 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
> BlockDriverState *copy_bitmap_bs,
> const BdrvDirtyBitmap *bitmap,
> bool discard_source,
> + int64_t min_cluster_size,
> Error **errp);
>
> /* Function should be called prior any actual copy request */
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 0a72c590a8..85c8f88f6e 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4625,12 +4625,18 @@
> # @on-cbw-error parameter will decide how this failure is handled.
> # Default 0. (Since 7.1)
> #
> +# @min-cluster-size: Minimum size of blocks used by copy-before-write
> +# operations. Has to be a power of 2. No effect if smaller than
> +# the maximum of the target's cluster size and 64 KiB. Default 0.
> +# (Since 9.0)
will have to update to 9.1
> +#
> # Since: 6.2
> ##
> { 'struct': 'BlockdevOptionsCbw',
> 'base': 'BlockdevOptionsGenericFormat',
> 'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap',
> - '*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32' } }
> + '*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32',
> + '*min-cluster-size': 'uint32' } }
>
> ##
> # @BlockdevOptions:
Otherwise, looks good to me.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] backup: add minimum cluster size to performance options
2024-03-08 15:51 ` [PATCH 2/2] backup: add minimum cluster size to performance options Fiona Ebner
@ 2024-03-29 10:15 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 8+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2024-03-29 10:15 UTC (permalink / raw
To: Fiona Ebner, qemu-devel; +Cc: qemu-block, armbru, eblake, hreitz, kwolf, jsnow
On 08.03.24 18:51, Fiona Ebner wrote:
> Useful to make discard-source work in the context of backup fleecing
> when the fleecing image has a larger granularity than the backup
> target.
>
> Backup/block-copy will use at least this granularity for copy operations
> and in particular, discard requests to the backup source will too. If
> the granularity is too small, they will just be aligned down in
> cbw_co_pdiscard_snapshot() and thus effectively ignored.
>
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> block/backup.c | 2 +-
> block/copy-before-write.c | 2 ++
> block/copy-before-write.h | 1 +
> blockdev.c | 3 +++
> qapi/block-core.json | 9 +++++++--
> 5 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index 3dd2e229d2..a1292c01ec 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -458,7 +458,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
> }
>
> cbw = bdrv_cbw_append(bs, target, filter_node_name, discard_source,
> - &bcs, errp);
> + perf->min_cluster_size, &bcs, errp);
> if (!cbw) {
> goto error;
> }
> diff --git a/block/copy-before-write.c b/block/copy-before-write.c
> index f9896c6c1e..55a9272485 100644
> --- a/block/copy-before-write.c
> +++ b/block/copy-before-write.c
> @@ -545,6 +545,7 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source,
> BlockDriverState *target,
> const char *filter_node_name,
> bool discard_source,
> + int64_t min_cluster_size,
same note, suggest uint32_t
> BlockCopyState **bcs,
> Error **errp)
> {
> @@ -563,6 +564,7 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source,
> }
> qdict_put_str(opts, "file", bdrv_get_node_name(source));
> qdict_put_str(opts, "target", bdrv_get_node_name(target));
> + qdict_put_int(opts, "min-cluster-size", min_cluster_size);
>
> top = bdrv_insert_node(source, opts, flags, errp);
> if (!top) {
> diff --git a/block/copy-before-write.h b/block/copy-before-write.h
> index 01af0cd3c4..dc6cafe7fa 100644
> --- a/block/copy-before-write.h
> +++ b/block/copy-before-write.h
> @@ -40,6 +40,7 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source,
> BlockDriverState *target,
> const char *filter_node_name,
> bool discard_source,
> + int64_t min_cluster_size,
> BlockCopyState **bcs,
> Error **errp);
> void bdrv_cbw_drop(BlockDriverState *bs);
> diff --git a/blockdev.c b/blockdev.c
> index daceb50460..8e6bdbc94a 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2653,6 +2653,9 @@ static BlockJob *do_backup_common(BackupCommon *backup,
> if (backup->x_perf->has_max_chunk) {
> perf.max_chunk = backup->x_perf->max_chunk;
> }
> + if (backup->x_perf->has_min_cluster_size) {
> + perf.min_cluster_size = backup->x_perf->min_cluster_size;
> + }
> }
>
> if ((backup->sync == MIRROR_SYNC_MODE_BITMAP) ||
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 85c8f88f6e..ba0836892f 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1551,11 +1551,16 @@
> # it should not be less than job cluster size which is calculated
> # as maximum of target image cluster size and 64k. Default 0.
> #
> +# @min-cluster-size: Minimum size of blocks used by copy-before-write
> +# and background copy operations. Has to be a power of 2. No
> +# effect if smaller than the maximum of the target's cluster size
> +# and 64 KiB. Default 0. (Since 9.0)
9.1
> +#
> # Since: 6.0
> ##
> { 'struct': 'BackupPerf',
> - 'data': { '*use-copy-range': 'bool',
> - '*max-workers': 'int', '*max-chunk': 'int64' } }
> + 'data': { '*use-copy-range': 'bool', '*max-workers': 'int',
> + '*max-chunk': 'int64', '*min-cluster-size': 'uint32' } }
>
> ##
> # @BackupCommon:
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] copy-before-write: allow specifying minimum cluster size
2024-03-26 9:06 ` Markus Armbruster
@ 2024-05-13 13:24 ` Fiona Ebner
2024-05-14 12:09 ` Markus Armbruster
0 siblings, 1 reply; 8+ messages in thread
From: Fiona Ebner @ 2024-05-13 13:24 UTC (permalink / raw
To: Markus Armbruster
Cc: qemu-devel, qemu-block, eblake, hreitz, kwolf, vsementsov, jsnow
Am 26.03.24 um 10:06 schrieb Markus Armbruster:
>> @@ -365,7 +368,13 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
>>
>> GLOBAL_STATE_CODE();
>>
>> - cluster_size = block_copy_calculate_cluster_size(target->bs, errp);
>> + if (min_cluster_size && !is_power_of_2(min_cluster_size)) {
>
> min_cluster_size is int64_t, is_power_of_2() takes uint64_t. Bad if
> min_cluster_size is negative. Could this happen?
>
No, because it comes in as a uint32_t via the QAPI (the internal caller
added by patch 2/2 from the backup code also gets the value via QAPI and
there uint32_t is used too).
---snip---
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 0a72c590a8..85c8f88f6e 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -4625,12 +4625,18 @@
>> # @on-cbw-error parameter will decide how this failure is handled.
>> # Default 0. (Since 7.1)
>> #
>> +# @min-cluster-size: Minimum size of blocks used by copy-before-write
>> +# operations. Has to be a power of 2. No effect if smaller than
>> +# the maximum of the target's cluster size and 64 KiB. Default 0.
>> +# (Since 9.0)
>> +#
>> # Since: 6.2
>> ##
>> { 'struct': 'BlockdevOptionsCbw',
>> 'base': 'BlockdevOptionsGenericFormat',
>> 'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap',
>> - '*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32' } }
>> + '*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32',
>> + '*min-cluster-size': 'uint32' } }
>
> Elsewhere in the schema, we use either 'int' or 'size' for cluster-size.
> Why the difference?
>
The motivation was to disallow negative values up front and have it work
with block_copy_calculate_cluster_size(), whose result is an int64_t. If
I go with 'int', I'll have to add a check to disallow negative values.
If I go with 'size', I'll have to add a check for to disallow too large
values.
Which approach should I go with?
Best Regards,
Fiona
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] copy-before-write: allow specifying minimum cluster size
2024-05-13 13:24 ` Fiona Ebner
@ 2024-05-14 12:09 ` Markus Armbruster
0 siblings, 0 replies; 8+ messages in thread
From: Markus Armbruster @ 2024-05-14 12:09 UTC (permalink / raw
To: Fiona Ebner
Cc: qemu-devel, qemu-block, eblake, hreitz, kwolf, vsementsov, jsnow
Fiona Ebner <f.ebner@proxmox.com> writes:
> Am 26.03.24 um 10:06 schrieb Markus Armbruster:
>>> @@ -365,7 +368,13 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
>>>
>>> GLOBAL_STATE_CODE();
>>>
>>> - cluster_size = block_copy_calculate_cluster_size(target->bs, errp);
>>> + if (min_cluster_size && !is_power_of_2(min_cluster_size)) {
>>
>> min_cluster_size is int64_t, is_power_of_2() takes uint64_t. Bad if
>> min_cluster_size is negative. Could this happen?
>
> No, because it comes in as a uint32_t via the QAPI (the internal caller
> added by patch 2/2 from the backup code also gets the value via QAPI and
> there uint32_t is used too).
Good.
Is there a practical way to tweak the types so it's more obvious?
>>> + error_setg(errp, "min-cluster-size needs to be a power of 2");
>>> + return NULL;
>>> + }
>>> +
>>> + cluster_size = block_copy_calculate_cluster_size(target->bs,
>>> + min_cluster_size, errp);
>>> if (cluster_size < 0) {
>>> return NULL;
>>> }
>
> ---snip---
>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 0a72c590a8..85c8f88f6e 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -4625,12 +4625,18 @@
>>> # @on-cbw-error parameter will decide how this failure is handled.
>>> # Default 0. (Since 7.1)
>>> #
>>> +# @min-cluster-size: Minimum size of blocks used by copy-before-write
>>> +# operations. Has to be a power of 2. No effect if smaller than
>>> +# the maximum of the target's cluster size and 64 KiB. Default 0.
>>> +# (Since 9.0)
>>> +#
>>> # Since: 6.2
>>> ##
>>> { 'struct': 'BlockdevOptionsCbw',
>>> 'base': 'BlockdevOptionsGenericFormat',
>>> 'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap',
>>> - '*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32' } }
>>> + '*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32',
>>> + '*min-cluster-size': 'uint32' } }
>>
>> Elsewhere in the schema, we use either 'int' or 'size' for cluster-size.
>> Why the difference?
>
> The motivation was to disallow negative values up front and have it work
> with block_copy_calculate_cluster_size(), whose result is an int64_t.
Let's see whether I understand.
cbw_open() passes the new member @min-cluster-size to
block_copy_state_new().
block_copy_state_new() checks it, and passes it on to
block_copy_calculate_cluster_size(). This is the C code shown above.
block_copy_calculate_cluster_size() uses it like
return MAX(min_cluster_size, BLOCK_COPY_CLUSTER_SIZE_DEFAULT);
and
return MAX(min_cluster_size,
MAX(BLOCK_COPY_CLUSTER_SIZE_DEFAULT, bdi.cluster_size));
BLOCK_COPY_CLUSTER_SIZE_DEFAULT and bdi.cluster_size are int. The
return type is int64_t.
Correct?
I don't like mixing signed and unsigned in MAX() like this. Figuring
out whether it's safe takes a C language lawyer. I'd rather avoid such
subtle code. Can we please compute these maxima entirely in the
destination type int64_t?
> If
> I go with 'int', I'll have to add a check to disallow negative values.
> If I go with 'size', I'll have to add a check for to disallow too large
> values.
>
> Which approach should I go with?
For me, reducing the need for manual checking is important, but
cleanliness of the external interface trumps it. I'd use 'size'.
Not the first time I see friction between QAPI 'size' or 'uint64' and
the block layer's use of int64_t.
The block layer likes to use int64_t even when the value must not be
negative. There are good reasons for that.
Perhaps a QAPI type for "non-negative int64_t" could be useful. Weird,
but useful.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-05-14 12:10 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-08 15:51 [PATCH 0/2] backup: allow specifying minimum cluster size Fiona Ebner
2024-03-08 15:51 ` [PATCH 1/2] copy-before-write: " Fiona Ebner
2024-03-26 9:06 ` Markus Armbruster
2024-05-13 13:24 ` Fiona Ebner
2024-05-14 12:09 ` Markus Armbruster
2024-03-29 10:10 ` Vladimir Sementsov-Ogievskiy
2024-03-08 15:51 ` [PATCH 2/2] backup: add minimum cluster size to performance options Fiona Ebner
2024-03-29 10:15 ` Vladimir Sementsov-Ogievskiy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.