| Message ID | 20220216194617.126484-5-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make image fleecing more usable | expand |
On 16.02.22 20:46, Vladimir Sementsov-Ogievskiy wrote: > This brings "incremental" mode to copy-before-write filter: user can > specify bitmap so that filter will copy only "dirty" areas. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > qapi/block-core.json | 10 +++++++- > block/copy-before-write.c | 51 ++++++++++++++++++++++++++++++++++++++- > 2 files changed, 59 insertions(+), 2 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 9a5a3641d0..3bab597506 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -4171,11 +4171,19 @@ > # > # @target: The target for copy-before-write operations. > # > +# @bitmap: If specified, copy-before-write filter will do > +# copy-before-write operations only for dirty regions of the > +# bitmap. Bitmap size must be equal to length of file and > +# target child of the filter. Note also, that bitmap is used > +# only to initialize internal bitmap of the process, so further > +# modifications (or removing) of specified bitmap doesn't > +# influence the filter. Sorry, missed this last time: There should be a “since: 7.0” here. > +# > # Since: 6.2 > ## > { 'struct': 'BlockdevOptionsCbw', > 'base': 'BlockdevOptionsGenericFormat', > - 'data': { 'target': 'BlockdevRef' } } > + 'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap' } } > > ## > # @BlockdevOptions: > diff --git a/block/copy-before-write.c b/block/copy-before-write.c > index 799223e3fb..91a2288b66 100644 > --- a/block/copy-before-write.c > +++ b/block/copy-before-write.c > @@ -34,6 +34,8 @@ > > #include "block/copy-before-write.h" > > +#include "qapi/qapi-visit-block-core.h" > + > typedef struct BDRVCopyBeforeWriteState { > BlockCopyState *bcs; > BdrvChild *target; > @@ -145,10 +147,53 @@ static void cbw_child_perm(BlockDriverState *bs, BdrvChild *c, > } > } > > +static bool cbw_parse_bitmap_option(QDict *options, BdrvDirtyBitmap **bitmap, > + Error **errp) > +{ > + QDict *bitmap_qdict = NULL; > + BlockDirtyBitmap *bmp_param = NULL; > + Visitor *v = NULL; > + bool ret = false; > + > + *bitmap = NULL; > + > + qdict_extract_subqdict(options, &bitmap_qdict, "bitmap."); > + if (!qdict_size(bitmap_qdict)) { > + ret = true; > + goto out; > + } > + > + v = qobject_input_visitor_new_flat_confused(bitmap_qdict, errp); > + if (!v) { > + goto out; > + } > + > + visit_type_BlockDirtyBitmap(v, NULL, &bmp_param, errp); > + if (!bmp_param) { > + goto out; > + } > + > + *bitmap = block_dirty_bitmap_lookup(bmp_param->node, bmp_param->name, NULL, > + errp); > + if (!*bitmap) { > + goto out; > + } > + > + ret = true; > + > +out: > + qapi_free_BlockDirtyBitmap(bmp_param); > + visit_free(v); > + qobject_unref(bitmap_qdict); > + > + return ret; > +} > + > static int cbw_open(BlockDriverState *bs, QDict *options, int flags, > Error **errp) > { > BDRVCopyBeforeWriteState *s = bs->opaque; > + BdrvDirtyBitmap *bitmap = NULL; > > bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds, > BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, > @@ -163,6 +208,10 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags, > return -EINVAL; > } > > + if (!cbw_parse_bitmap_option(options, &bitmap, errp)) { > + return -EINVAL; Hm... Just to get a second opinion on this: We don’t need to close s->target here, because the failure paths of bdrv_open_inherit() and bdrv_new_open_driver_opts() both call bdrv_unref(), which will call bdrv_close(), which will close all children including s->target, right? > + } > + > bs->total_sectors = bs->file->bs->total_sectors; > bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED | > (BDRV_REQ_FUA & bs->file->bs->supported_write_flags); > @@ -170,7 +219,7 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags, > ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) & > bs->file->bs->supported_zero_flags); > > - s->bcs = block_copy_state_new(bs->file, s->target, NULL, errp); > + s->bcs = block_copy_state_new(bs->file, s->target, bitmap, errp); > if (!s->bcs) { > error_prepend(errp, "Cannot create block-copy-state: "); > return -EINVAL;
24.02.2022 15:07, Hanna Reitz wrote: > On 16.02.22 20:46, Vladimir Sementsov-Ogievskiy wrote: >> This brings "incremental" mode to copy-before-write filter: user can >> specify bitmap so that filter will copy only "dirty" areas. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> qapi/block-core.json | 10 +++++++- >> block/copy-before-write.c | 51 ++++++++++++++++++++++++++++++++++++++- >> 2 files changed, 59 insertions(+), 2 deletions(-) >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> index 9a5a3641d0..3bab597506 100644 >> --- a/qapi/block-core.json >> +++ b/qapi/block-core.json >> @@ -4171,11 +4171,19 @@ >> # >> # @target: The target for copy-before-write operations. >> # >> +# @bitmap: If specified, copy-before-write filter will do >> +# copy-before-write operations only for dirty regions of the >> +# bitmap. Bitmap size must be equal to length of file and >> +# target child of the filter. Note also, that bitmap is used >> +# only to initialize internal bitmap of the process, so further >> +# modifications (or removing) of specified bitmap doesn't >> +# influence the filter. > > Sorry, missed this last time: There should be a “since: 7.0” here. > >> +# >> # Since: 6.2 >> ## >> { 'struct': 'BlockdevOptionsCbw', >> 'base': 'BlockdevOptionsGenericFormat', >> - 'data': { 'target': 'BlockdevRef' } } >> + 'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap' } } >> ## >> # @BlockdevOptions: >> diff --git a/block/copy-before-write.c b/block/copy-before-write.c >> index 799223e3fb..91a2288b66 100644 >> --- a/block/copy-before-write.c >> +++ b/block/copy-before-write.c >> @@ -34,6 +34,8 @@ >> #include "block/copy-before-write.h" >> +#include "qapi/qapi-visit-block-core.h" >> + >> typedef struct BDRVCopyBeforeWriteState { >> BlockCopyState *bcs; >> BdrvChild *target; >> @@ -145,10 +147,53 @@ static void cbw_child_perm(BlockDriverState *bs, BdrvChild *c, >> } >> } >> +static bool cbw_parse_bitmap_option(QDict *options, BdrvDirtyBitmap **bitmap, >> + Error **errp) >> +{ >> + QDict *bitmap_qdict = NULL; >> + BlockDirtyBitmap *bmp_param = NULL; >> + Visitor *v = NULL; >> + bool ret = false; >> + >> + *bitmap = NULL; >> + >> + qdict_extract_subqdict(options, &bitmap_qdict, "bitmap."); >> + if (!qdict_size(bitmap_qdict)) { >> + ret = true; >> + goto out; >> + } >> + >> + v = qobject_input_visitor_new_flat_confused(bitmap_qdict, errp); >> + if (!v) { >> + goto out; >> + } >> + >> + visit_type_BlockDirtyBitmap(v, NULL, &bmp_param, errp); >> + if (!bmp_param) { >> + goto out; >> + } >> + >> + *bitmap = block_dirty_bitmap_lookup(bmp_param->node, bmp_param->name, NULL, >> + errp); >> + if (!*bitmap) { >> + goto out; >> + } >> + >> + ret = true; >> + >> +out: >> + qapi_free_BlockDirtyBitmap(bmp_param); >> + visit_free(v); >> + qobject_unref(bitmap_qdict); >> + >> + return ret; >> +} >> + >> static int cbw_open(BlockDriverState *bs, QDict *options, int flags, >> Error **errp) >> { >> BDRVCopyBeforeWriteState *s = bs->opaque; >> + BdrvDirtyBitmap *bitmap = NULL; >> bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds, >> BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, >> @@ -163,6 +208,10 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags, >> return -EINVAL; >> } >> + if (!cbw_parse_bitmap_option(options, &bitmap, errp)) { >> + return -EINVAL; > > Hm... Just to get a second opinion on this: We don’t need to close s->target here, because the failure paths of bdrv_open_inherit() and bdrv_new_open_driver_opts() both call bdrv_unref(), which will call bdrv_close(), which will close all children including s->target, right? I think I just followed existing error path in cbw_open() on block_copy_state_new() failure. But I think you are right and bdrv_close() should take care of all bs children. > >> + } >> + >> bs->total_sectors = bs->file->bs->total_sectors; >> bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED | >> (BDRV_REQ_FUA & bs->file->bs->supported_write_flags); >> @@ -170,7 +219,7 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags, >> ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) & >> bs->file->bs->supported_zero_flags); >> - s->bcs = block_copy_state_new(bs->file, s->target, NULL, errp); >> + s->bcs = block_copy_state_new(bs->file, s->target, bitmap, errp); >> if (!s->bcs) { >> error_prepend(errp, "Cannot create block-copy-state: "); >> return -EINVAL; >
diff --git a/qapi/block-core.json b/qapi/block-core.json index 9a5a3641d0..3bab597506 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -4171,11 +4171,19 @@ # # @target: The target for copy-before-write operations. # +# @bitmap: If specified, copy-before-write filter will do +# copy-before-write operations only for dirty regions of the +# bitmap. Bitmap size must be equal to length of file and +# target child of the filter. Note also, that bitmap is used +# only to initialize internal bitmap of the process, so further +# modifications (or removing) of specified bitmap doesn't +# influence the filter. +# # Since: 6.2 ## { 'struct': 'BlockdevOptionsCbw', 'base': 'BlockdevOptionsGenericFormat', - 'data': { 'target': 'BlockdevRef' } } + 'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap' } } ## # @BlockdevOptions: diff --git a/block/copy-before-write.c b/block/copy-before-write.c index 799223e3fb..91a2288b66 100644 --- a/block/copy-before-write.c +++ b/block/copy-before-write.c @@ -34,6 +34,8 @@ #include "block/copy-before-write.h" +#include "qapi/qapi-visit-block-core.h" + typedef struct BDRVCopyBeforeWriteState { BlockCopyState *bcs; BdrvChild *target; @@ -145,10 +147,53 @@ static void cbw_child_perm(BlockDriverState *bs, BdrvChild *c, } } +static bool cbw_parse_bitmap_option(QDict *options, BdrvDirtyBitmap **bitmap, + Error **errp) +{ + QDict *bitmap_qdict = NULL; + BlockDirtyBitmap *bmp_param = NULL; + Visitor *v = NULL; + bool ret = false; + + *bitmap = NULL; + + qdict_extract_subqdict(options, &bitmap_qdict, "bitmap."); + if (!qdict_size(bitmap_qdict)) { + ret = true; + goto out; + } + + v = qobject_input_visitor_new_flat_confused(bitmap_qdict, errp); + if (!v) { + goto out; + } + + visit_type_BlockDirtyBitmap(v, NULL, &bmp_param, errp); + if (!bmp_param) { + goto out; + } + + *bitmap = block_dirty_bitmap_lookup(bmp_param->node, bmp_param->name, NULL, + errp); + if (!*bitmap) { + goto out; + } + + ret = true; + +out: + qapi_free_BlockDirtyBitmap(bmp_param); + visit_free(v); + qobject_unref(bitmap_qdict); + + return ret; +} + static int cbw_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { BDRVCopyBeforeWriteState *s = bs->opaque; + BdrvDirtyBitmap *bitmap = NULL; bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds, BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, @@ -163,6 +208,10 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags, return -EINVAL; } + if (!cbw_parse_bitmap_option(options, &bitmap, errp)) { + return -EINVAL; + } + bs->total_sectors = bs->file->bs->total_sectors; bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED | (BDRV_REQ_FUA & bs->file->bs->supported_write_flags); @@ -170,7 +219,7 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags, ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) & bs->file->bs->supported_zero_flags); - s->bcs = block_copy_state_new(bs->file, s->target, NULL, errp); + s->bcs = block_copy_state_new(bs->file, s->target, bitmap, errp); if (!s->bcs) { error_prepend(errp, "Cannot create block-copy-state: "); return -EINVAL;
This brings "incremental" mode to copy-before-write filter: user can specify bitmap so that filter will copy only "dirty" areas. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- qapi/block-core.json | 10 +++++++- block/copy-before-write.c | 51 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 59 insertions(+), 2 deletions(-)