In the context of backup fleecing, discarding the source will not work
when the fleecing image has a larger granularity than the one used for
block-copy operations (can happen if the backup target has smaller
cluster size), because cbw_co_pdiscard_snapshot() will align down the
discard requests and thus effectively ignore then.
To make @discard-source work in such a scenario, allow specifying the
minimum cluster size used for block-copy operations and thus in
particular also the granularity for discard requests to the source.
The type 'size' (corresponding to uint64_t in C) is used in QAPI to
rule out negative inputs and for consistency with already existing
@cluster-size parameters. Since block_copy_calculate_cluster_size()
uses int64_t for its result, a check that the input is not too large
is added in block_copy_state_new() before calling it. The calculation
in block_copy_calculate_cluster_size() is done in the target int64_t
type.
Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
Changes in v2:
* Use 'size' type in QAPI.
* Remove option in cbw_parse_options(), i.e. before parsing generic
blockdev options.
block/block-copy.c | 22 ++++++++++++++++++----
block/copy-before-write.c | 10 +++++++++-
include/block/block-copy.h | 1 +
qapi/block-core.json | 8 +++++++-
4 files changed, 35 insertions(+), 6 deletions(-)
diff --git a/block/block-copy.c b/block/block-copy.c
index 7e3b378528..36eaecaaf4 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, (int64_t)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, (int64_t)BLOCK_COPY_CLUSTER_SIZE_DEFAULT);
}
- return MAX(BLOCK_COPY_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
+ return MAX(min_cluster_size,
+ (int64_t)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,
+ uint64_t min_cluster_size,
Error **errp)
{
ERRP_GUARD();
@@ -365,7 +368,18 @@ 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 > INT64_MAX) {
+ error_setg(errp, "min-cluster-size too large: %lu > %ld",
+ min_cluster_size, INT64_MAX);
+ return NULL;
+ } else 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,
+ (int64_t)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 cd65524e26..ef0bc4dcfe 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -417,6 +417,7 @@ static BlockdevOptions *cbw_parse_options(QDict *options, Error **errp)
qdict_extract_subqdict(options, NULL, "bitmap");
qdict_del(options, "on-cbw-error");
qdict_del(options, "cbw-timeout");
+ qdict_del(options, "min-cluster-size");
out:
visit_free(v);
@@ -432,6 +433,7 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
BDRVCopyBeforeWriteState *s = bs->opaque;
BdrvDirtyBitmap *bitmap = NULL;
int64_t cluster_size;
+ uint64_t min_cluster_size = 0;
g_autoptr(BlockdevOptions) full_opts = NULL;
BlockdevOptionsCbw *opts;
int ret;
@@ -476,8 +478,14 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
bs->file->bs->supported_zero_flags);
s->discard_source = flags & BDRV_O_CBW_DISCARD_SOURCE;
+
+ if (opts->has_min_cluster_size) {
+ min_cluster_size = opts->min_cluster_size;
+ }
+
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,
+ 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..dd5cc82f3b 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,
+ uint64_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 df5e07debd..8fc0a4b234 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4642,12 +4642,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.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': 'size' } }
##
# @BlockdevOptions:
--
2.39.2On 28.05.24 15:01, Fiona Ebner wrote:
> + if (min_cluster_size > INT64_MAX) {
> + error_setg(errp, "min-cluster-size too large: %lu > %ld",
> + min_cluster_size, INT64_MAX);
Better use PRIu64 and PRIi64 macros
--
Best regards,
Vladimir
On 28.05.24 15:01, Fiona Ebner wrote:
> In the context of backup fleecing, discarding the source will not work
> when the fleecing image has a larger granularity than the one used for
> block-copy operations (can happen if the backup target has smaller
> cluster size), because cbw_co_pdiscard_snapshot() will align down the
> discard requests and thus effectively ignore then.
>
> To make @discard-source work in such a scenario, allow specifying the
> minimum cluster size used for block-copy operations and thus in
> particular also the granularity for discard requests to the source.
>
> The type 'size' (corresponding to uint64_t in C) is used in QAPI to
> rule out negative inputs and for consistency with already existing
> @cluster-size parameters. Since block_copy_calculate_cluster_size()
> uses int64_t for its result, a check that the input is not too large
> is added in block_copy_state_new() before calling it. The calculation
> in block_copy_calculate_cluster_size() is done in the target int64_t
> type.
>
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>
> Changes in v2:
> * Use 'size' type in QAPI.
> * Remove option in cbw_parse_options(), i.e. before parsing generic
> blockdev options.
>
> block/block-copy.c | 22 ++++++++++++++++++----
> block/copy-before-write.c | 10 +++++++++-
> include/block/block-copy.h | 1 +
> qapi/block-core.json | 8 +++++++-
> 4 files changed, 35 insertions(+), 6 deletions(-)
>
> diff --git a/block/block-copy.c b/block/block-copy.c
> index 7e3b378528..36eaecaaf4 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, (int64_t)BLOCK_COPY_CLUSTER_SIZE_DEFAULT);
instead of triple use MAX(min_..., let's just do
min_cluster_size = MAX(min_cluster_size, (int64_t)BLOCK_COPY_CLUSTER_SIZE_DEFAULT);
at start of function, and then use min_cluster_size
anyway:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> } 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, (int64_t)BLOCK_COPY_CLUSTER_SIZE_DEFAULT);
> }
>
> - return MAX(BLOCK_COPY_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
> + return MAX(min_cluster_size,
> + (int64_t)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,
> + uint64_t min_cluster_size,
> Error **errp)
> {
> ERRP_GUARD();
> @@ -365,7 +368,18 @@ 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 > INT64_MAX) {
> + error_setg(errp, "min-cluster-size too large: %lu > %ld",
> + min_cluster_size, INT64_MAX);
> + return NULL;
> + } else 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,
> + (int64_t)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 cd65524e26..ef0bc4dcfe 100644
> --- a/block/copy-before-write.c
> +++ b/block/copy-before-write.c
> @@ -417,6 +417,7 @@ static BlockdevOptions *cbw_parse_options(QDict *options, Error **errp)
> qdict_extract_subqdict(options, NULL, "bitmap");
> qdict_del(options, "on-cbw-error");
> qdict_del(options, "cbw-timeout");
> + qdict_del(options, "min-cluster-size");
>
> out:
> visit_free(v);
> @@ -432,6 +433,7 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
> BDRVCopyBeforeWriteState *s = bs->opaque;
> BdrvDirtyBitmap *bitmap = NULL;
> int64_t cluster_size;
> + uint64_t min_cluster_size = 0;
> g_autoptr(BlockdevOptions) full_opts = NULL;
> BlockdevOptionsCbw *opts;
> int ret;
> @@ -476,8 +478,14 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
> bs->file->bs->supported_zero_flags);
>
> s->discard_source = flags & BDRV_O_CBW_DISCARD_SOURCE;
> +
> + if (opts->has_min_cluster_size) {
> + min_cluster_size = opts->min_cluster_size;
> + }
> +
> 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,
> + 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..dd5cc82f3b 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,
> + uint64_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 df5e07debd..8fc0a4b234 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4642,12 +4642,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.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': 'size' } }
>
> ##
> # @BlockdevOptions:
--
Best regards,
Vladimir
Fiona Ebner <f.ebner@proxmox.com> writes:
> In the context of backup fleecing, discarding the source will not work
> when the fleecing image has a larger granularity than the one used for
> block-copy operations (can happen if the backup target has smaller
> cluster size), because cbw_co_pdiscard_snapshot() will align down the
> discard requests and thus effectively ignore then.
>
> To make @discard-source work in such a scenario, allow specifying the
> minimum cluster size used for block-copy operations and thus in
> particular also the granularity for discard requests to the source.
>
> The type 'size' (corresponding to uint64_t in C) is used in QAPI to
> rule out negative inputs and for consistency with already existing
> @cluster-size parameters. Since block_copy_calculate_cluster_size()
> uses int64_t for its result, a check that the input is not too large
> is added in block_copy_state_new() before calling it. The calculation
> in block_copy_calculate_cluster_size() is done in the target int64_t
> type.
>
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>
> Changes in v2:
> * Use 'size' type in QAPI.
> * Remove option in cbw_parse_options(), i.e. before parsing generic
> blockdev options.
>
> block/block-copy.c | 22 ++++++++++++++++++----
> block/copy-before-write.c | 10 +++++++++-
> include/block/block-copy.h | 1 +
> qapi/block-core.json | 8 +++++++-
> 4 files changed, 35 insertions(+), 6 deletions(-)
>
> diff --git a/block/block-copy.c b/block/block-copy.c
> index 7e3b378528..36eaecaaf4 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, (int64_t)BLOCK_COPY_CLUSTER_SIZE_DEFAULT);
BLOCK_COPY_CLUSTER_SIZE_DEFAULT gets promoted from int to
min_cluster_size's type int64_t even without the cast. The cast makes
it explicit. Matter of taste, and I'm not the maintainer here :)
> } 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, (int64_t)BLOCK_COPY_CLUSTER_SIZE_DEFAULT);
Likewise.
> }
>
> - return MAX(BLOCK_COPY_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
> + return MAX(min_cluster_size,
> + (int64_t)MAX(BLOCK_COPY_CLUSTER_SIZE_DEFAULT, bdi.cluster_size));
Likewise.
> }
>
> BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
> BlockDriverState *copy_bitmap_bs,
> const BdrvDirtyBitmap *bitmap,
> bool discard_source,
> + uint64_t min_cluster_size,
> Error **errp)
> {
> ERRP_GUARD();
> @@ -365,7 +368,18 @@ 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 > INT64_MAX) {
> + error_setg(errp, "min-cluster-size too large: %lu > %ld",
> + min_cluster_size, INT64_MAX);
> + return NULL;
> + } else 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,
> + (int64_t)min_cluster_size,
> + errp);
This is now more obviously safe. Thanks!
> if (cluster_size < 0) {
> return NULL;
> }
> diff --git a/block/copy-before-write.c b/block/copy-before-write.c
> index cd65524e26..ef0bc4dcfe 100644
> --- a/block/copy-before-write.c
> +++ b/block/copy-before-write.c
> @@ -417,6 +417,7 @@ static BlockdevOptions *cbw_parse_options(QDict *options, Error **errp)
> qdict_extract_subqdict(options, NULL, "bitmap");
> qdict_del(options, "on-cbw-error");
> qdict_del(options, "cbw-timeout");
> + qdict_del(options, "min-cluster-size");
>
> out:
> visit_free(v);
> @@ -432,6 +433,7 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
> BDRVCopyBeforeWriteState *s = bs->opaque;
> BdrvDirtyBitmap *bitmap = NULL;
> int64_t cluster_size;
> + uint64_t min_cluster_size = 0;
> g_autoptr(BlockdevOptions) full_opts = NULL;
> BlockdevOptionsCbw *opts;
> int ret;
> @@ -476,8 +478,14 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
> bs->file->bs->supported_zero_flags);
>
> s->discard_source = flags & BDRV_O_CBW_DISCARD_SOURCE;
> +
> + if (opts->has_min_cluster_size) {
> + min_cluster_size = opts->min_cluster_size;
> + }
> +
> 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,
> + min_cluster_size, errp);
> if (!s->bcs) {
> error_prepend(errp, "Cannot create block-copy-state: ");
> return -EINVAL;
You can pass opts->min_cluster_size directly, as in v1. When
!opts->has_min_cluster_size, then opts->min_cluster_size is zero.
> diff --git a/include/block/block-copy.h b/include/block/block-copy.h
> index bdc703bacd..dd5cc82f3b 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,
> + uint64_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 df5e07debd..8fc0a4b234 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4642,12 +4642,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.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': 'size' } }
>
> ##
> # @BlockdevOptions:
QAPI schema
Acked-by: Markus Armbruster <armbru@redhat.com>
© 2016 - 2026 Red Hat, Inc.