* [PATCH v2 0/2] backup: allow specifying minimum cluster size @ 2024-05-28 12:01 Fiona Ebner 2024-05-28 12:01 ` [PATCH v2 1/2] copy-before-write: " Fiona Ebner 2024-05-28 12:01 ` [PATCH v2 2/2] backup: add minimum cluster size to performance options Fiona Ebner 0 siblings, 2 replies; 8+ messages in thread From: Fiona Ebner @ 2024-05-28 12:01 UTC (permalink / raw) To: qemu-devel; +Cc: qemu-block, armbru, eblake, hreitz, kwolf, vsementsov, jsnow Based-on: https://lore.kernel.org/qemu-devel/20240429115157.2260885-1-vsementsov@yandex-team.ru/ Discussion for v1: https://lore.kernel.org/qemu-devel/20240308155158.830258-1-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. * Reword commit messages hoping to describe the issue in a more straight-forward way. 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. 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 | 22 ++++++++++++++++++---- block/copy-before-write.c | 18 +++++++++++++++++- block/copy-before-write.h | 1 + blockdev.c | 3 +++ include/block/block-copy.h | 1 + qapi/block-core.json | 17 ++++++++++++++--- 7 files changed, 55 insertions(+), 9 deletions(-) -- 2.39.2 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/2] copy-before-write: allow specifying minimum cluster size 2024-05-28 12:01 [PATCH v2 0/2] backup: allow specifying minimum cluster size Fiona Ebner @ 2024-05-28 12:01 ` Fiona Ebner 2024-06-03 11:16 ` Markus Armbruster ` (2 more replies) 2024-05-28 12:01 ` [PATCH v2 2/2] backup: add minimum cluster size to performance options Fiona Ebner 1 sibling, 3 replies; 8+ messages in thread From: Fiona Ebner @ 2024-05-28 12:01 UTC (permalink / raw) To: qemu-devel; +Cc: qemu-block, armbru, eblake, hreitz, kwolf, vsementsov, jsnow 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.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] copy-before-write: allow specifying minimum cluster size 2024-05-28 12:01 ` [PATCH v2 1/2] copy-before-write: " Fiona Ebner @ 2024-06-03 11:16 ` Markus Armbruster 2024-06-25 11:29 ` Vladimir Sementsov-Ogievskiy 2024-06-25 11:40 ` Vladimir Sementsov-Ogievskiy 2 siblings, 0 replies; 8+ messages in thread From: Markus Armbruster @ 2024-06-03 11:16 UTC (permalink / raw) To: Fiona Ebner Cc: qemu-devel, qemu-block, eblake, hreitz, kwolf, vsementsov, jsnow 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> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] copy-before-write: allow specifying minimum cluster size 2024-05-28 12:01 ` [PATCH v2 1/2] copy-before-write: " Fiona Ebner 2024-06-03 11:16 ` Markus Armbruster @ 2024-06-25 11:29 ` Vladimir Sementsov-Ogievskiy 2024-06-25 11:40 ` Vladimir Sementsov-Ogievskiy 2 siblings, 0 replies; 8+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2024-06-25 11:29 UTC (permalink / raw) To: Fiona Ebner, qemu-devel; +Cc: qemu-block, armbru, eblake, hreitz, kwolf, jsnow 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] copy-before-write: allow specifying minimum cluster size 2024-05-28 12:01 ` [PATCH v2 1/2] copy-before-write: " Fiona Ebner 2024-06-03 11:16 ` Markus Armbruster 2024-06-25 11:29 ` Vladimir Sementsov-Ogievskiy @ 2024-06-25 11:40 ` Vladimir Sementsov-Ogievskiy 2 siblings, 0 replies; 8+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2024-06-25 11:40 UTC (permalink / raw) To: Fiona Ebner, qemu-devel; +Cc: qemu-block, armbru, eblake, hreitz, kwolf, jsnow 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 -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 2/2] backup: add minimum cluster size to performance options 2024-05-28 12:01 [PATCH v2 0/2] backup: allow specifying minimum cluster size Fiona Ebner 2024-05-28 12:01 ` [PATCH v2 1/2] copy-before-write: " Fiona Ebner @ 2024-05-28 12:01 ` Fiona Ebner 2024-06-03 11:17 ` Markus Armbruster 2024-06-25 11:38 ` Vladimir Sementsov-Ogievskiy 1 sibling, 2 replies; 8+ messages in thread From: Fiona Ebner @ 2024-05-28 12:01 UTC (permalink / raw) To: qemu-devel; +Cc: qemu-block, armbru, eblake, hreitz, kwolf, vsementsov, jsnow 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. 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. block/backup.c | 2 +- block/copy-before-write.c | 8 ++++++++ block/copy-before-write.h | 1 + blockdev.c | 3 +++ qapi/block-core.json | 9 +++++++-- 5 files changed, 20 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 ef0bc4dcfe..183eed42e5 100644 --- a/block/copy-before-write.c +++ b/block/copy-before-write.c @@ -553,6 +553,7 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source, BlockDriverState *target, const char *filter_node_name, bool discard_source, + uint64_t min_cluster_size, BlockCopyState **bcs, Error **errp) { @@ -572,6 +573,13 @@ 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)); + if (min_cluster_size > INT64_MAX) { + error_setg(errp, "min-cluster-size too large: %lu > %ld", + min_cluster_size, INT64_MAX); + return NULL; + } + qdict_put_int(opts, "min-cluster-size", (int64_t)min_cluster_size); + top = bdrv_insert_node(source, opts, flags, errp); if (!top) { return NULL; diff --git a/block/copy-before-write.h b/block/copy-before-write.h index 01af0cd3c4..2a5d4ba693 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, + uint64_t min_cluster_size, BlockCopyState **bcs, Error **errp); void bdrv_cbw_drop(BlockDriverState *bs); diff --git a/blockdev.c b/blockdev.c index 835064ed03..6740663fda 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2655,6 +2655,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 8fc0a4b234..f1219a9dfb 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.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': 'size' } } ## # @BackupCommon: -- 2.39.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] backup: add minimum cluster size to performance options 2024-05-28 12:01 ` [PATCH v2 2/2] backup: add minimum cluster size to performance options Fiona Ebner @ 2024-06-03 11:17 ` Markus Armbruster 2024-06-25 11:38 ` Vladimir Sementsov-Ogievskiy 1 sibling, 0 replies; 8+ messages in thread From: Markus Armbruster @ 2024-06-03 11:17 UTC (permalink / raw) To: Fiona Ebner Cc: qemu-devel, qemu-block, armbru, eblake, hreitz, kwolf, vsementsov, jsnow 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. > > Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> [...] > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 8fc0a4b234..f1219a9dfb 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.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': 'size' } } > > ## > # @BackupCommon: QAPI schema Acked-by: Markus Armbruster <armbru@redhat.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] backup: add minimum cluster size to performance options 2024-05-28 12:01 ` [PATCH v2 2/2] backup: add minimum cluster size to performance options Fiona Ebner 2024-06-03 11:17 ` Markus Armbruster @ 2024-06-25 11:38 ` Vladimir Sementsov-Ogievskiy 1 sibling, 0 replies; 8+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2024-06-25 11:38 UTC (permalink / raw) To: Fiona Ebner, qemu-devel; +Cc: qemu-block, armbru, eblake, hreitz, kwolf, jsnow 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. > > 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. > > block/backup.c | 2 +- > block/copy-before-write.c | 8 ++++++++ > block/copy-before-write.h | 1 + > blockdev.c | 3 +++ > qapi/block-core.json | 9 +++++++-- > 5 files changed, 20 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 ef0bc4dcfe..183eed42e5 100644 > --- a/block/copy-before-write.c > +++ b/block/copy-before-write.c > @@ -553,6 +553,7 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState *source, > BlockDriverState *target, > const char *filter_node_name, > bool discard_source, > + uint64_t min_cluster_size, > BlockCopyState **bcs, > Error **errp) > { > @@ -572,6 +573,13 @@ 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)); > > + if (min_cluster_size > INT64_MAX) { > + error_setg(errp, "min-cluster-size too large: %lu > %ld", > + min_cluster_size, INT64_MAX); opts leaked here. with that fixed: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > + return NULL; > + } > + qdict_put_int(opts, "min-cluster-size", (int64_t)min_cluster_size); > + > top = bdrv_insert_node(source, opts, flags, errp); > if (!top) { > return NULL; > diff --git a/block/copy-before-write.h b/block/copy-before-write.h > index 01af0cd3c4..2a5d4ba693 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, > + uint64_t min_cluster_size, > BlockCopyState **bcs, > Error **errp); > void bdrv_cbw_drop(BlockDriverState *bs); > diff --git a/blockdev.c b/blockdev.c > index 835064ed03..6740663fda 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -2655,6 +2655,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 8fc0a4b234..f1219a9dfb 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.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': 'size' } } > > ## > # @BackupCommon: -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-06-25 11:41 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-05-28 12:01 [PATCH v2 0/2] backup: allow specifying minimum cluster size Fiona Ebner 2024-05-28 12:01 ` [PATCH v2 1/2] copy-before-write: " Fiona Ebner 2024-06-03 11:16 ` Markus Armbruster 2024-06-25 11:29 ` Vladimir Sementsov-Ogievskiy 2024-06-25 11:40 ` Vladimir Sementsov-Ogievskiy 2024-05-28 12:01 ` [PATCH v2 2/2] backup: add minimum cluster size to performance options Fiona Ebner 2024-06-03 11:17 ` Markus Armbruster 2024-06-25 11:38 ` Vladimir Sementsov-Ogievskiy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).