| Message ID | 20240528120114.344416-2-f.ebner@proxmox.com |
|---|---|
| State | New |
| Headers | show |
| Series | backup: allow specifying minimum cluster size | expand |
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>
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:
On 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
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:
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(-)