| Message ID | 20190516122725.132334-2-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | bitmaps: merge bitmaps from different nodes | expand |
On 5/16/19 8:27 AM, Vladimir Sementsov-Ogievskiy wrote: > Add new optional parameter making possible to merge bitmaps from > different nodes. It is needed to maintain external snapshots during > incremental backup chain history. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > qapi/block-core.json | 13 ++++++++++--- > block/dirty-bitmap.c | 9 ++++++--- > blockdev.c | 46 ++++++++++++++++++++++++++++++-------------- > 3 files changed, 48 insertions(+), 20 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 7ccbfff9d0..933b50771a 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -2006,16 +2006,23 @@ > ## > # @BlockDirtyBitmapMerge: > # > -# @node: name of device/node which the bitmap is tracking > +# @node: name of device/node which the @target and @bitmaps bitmaps are > +# tracking > # > # @target: name of the destination dirty bitmap > # > -# @bitmaps: name(s) of the source dirty bitmap(s) > +# @bitmaps: name(s) of the source dirty bitmap(s). The field is optional > +# since 4.1. > +# > +# @external-bitmaps: additional list of source dirty bitmaps with specified > +# nodes, which allows merging bitmaps between different > +# nodes. (Since: 4.1) > # > # Since: 4.0 > ## > { 'struct': 'BlockDirtyBitmapMerge', > - 'data': { 'node': 'str', 'target': 'str', 'bitmaps': ['str'] } } > + 'data': { 'node': 'str', 'target': 'str', '*bitmaps': ['str'], > + '*external-bitmaps': ['BlockDirtyBitmap'] } } > I guess you can specify one, or both, or maybe neither! Seems fine. > ## > # @block-dirty-bitmap-add: > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index 59e6ebb861..49646a30e6 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -816,10 +816,10 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, > { > bool ret; > > - /* only bitmaps from one bds are supported */ > - assert(dest->mutex == src->mutex); > - > qemu_mutex_lock(dest->mutex); > + if (src->mutex != dest->mutex) { > + qemu_mutex_lock(src->mutex); > + } > > if (bdrv_dirty_bitmap_check(dest, BDRV_BITMAP_DEFAULT, errp)) { > goto out; > @@ -845,4 +845,7 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, > > out: > qemu_mutex_unlock(dest->mutex); > + if (src->mutex != dest->mutex) { > + qemu_mutex_unlock(src->mutex); > + } > } > diff --git a/blockdev.c b/blockdev.c > index 79fbac8450..8d37ce5943 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -2112,11 +2112,9 @@ static void block_dirty_bitmap_disable_abort(BlkActionState *common) > } > } > > -static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(const char *node, > - const char *target, > - strList *bitmaps, > - HBitmap **backup, > - Error **errp); > +static BdrvDirtyBitmap *do_block_dirty_bitmap_merge( > + const char *node, const char *target, strList *bitmaps, > + BlockDirtyBitmapList *external_bitmaps, HBitmap **backup, Error **errp); > You know, that's actually a much smarter way to reflow these ... > static void block_dirty_bitmap_merge_prepare(BlkActionState *common, > Error **errp) > @@ -2132,8 +2130,9 @@ static void block_dirty_bitmap_merge_prepare(BlkActionState *common, > action = common->action->u.block_dirty_bitmap_merge.data; > > state->bitmap = do_block_dirty_bitmap_merge(action->node, action->target, > - action->bitmaps, &state->backup, > - errp); > + action->bitmaps, > + action->external_bitmaps, > + &state->backup, errp); > } > > static void abort_prepare(BlkActionState *common, Error **errp) > @@ -2965,15 +2964,14 @@ void qmp_block_dirty_bitmap_disable(const char *node, const char *name, > bdrv_disable_dirty_bitmap(bitmap); > } > > -static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(const char *node, > - const char *target, > - strList *bitmaps, > - HBitmap **backup, > - Error **errp) > +static BdrvDirtyBitmap *do_block_dirty_bitmap_merge( > + const char *node, const char *target, strList *bitmaps, > + BlockDirtyBitmapList *external_bitmaps, HBitmap **backup, Error **errp) > { > BlockDriverState *bs; > BdrvDirtyBitmap *dst, *src, *anon; > strList *lst; > + BlockDirtyBitmapList *ext_lst; > Error *local_err = NULL; > > dst = block_dirty_bitmap_lookup(node, target, &bs, errp); > @@ -3003,6 +3001,22 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(const char *node, > } > } > > + for (ext_lst = external_bitmaps; ext_lst; ext_lst = ext_lst->next) { > + src = block_dirty_bitmap_lookup(ext_lst->value->node, > + ext_lst->value->name, NULL, errp); > + if (!src) { > + dst = NULL; > + goto out; > + } > + > + bdrv_merge_dirty_bitmap(anon, src, NULL, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + dst = NULL; > + goto out; > + } > + } > + > /* Merge into dst; dst is unchanged on failure. */ > bdrv_merge_dirty_bitmap(dst, anon, backup, errp); > > @@ -3012,9 +3026,13 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(const char *node, > } > > void qmp_block_dirty_bitmap_merge(const char *node, const char *target, > - strList *bitmaps, Error **errp) > + bool has_bitmaps, strList *bitmaps, > + bool has_external_bitmaps, > + BlockDirtyBitmapList *external_bitmaps, > + Error **errp) > { > - do_block_dirty_bitmap_merge(node, target, bitmaps, NULL, errp); > + do_block_dirty_bitmap_merge(node, target, bitmaps, external_bitmaps, NULL, > + errp); > } > > BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node, > I don't think I like the name "external-bitmaps" but I guess I don't really have a better suggestion. Technically you can use this for node-local source bitmaps too, it's just that it's a more verbose way to specify them. (They're like "fully-qualified" bitmap identifiers instead of the local/relative references we've been using.) Well, I can't hold you up if I have no better ideas. Reviewed-by: John Snow <jsnow@redhat.com>
On 5/16/19 7:32 PM, John Snow wrote: > > > On 5/16/19 8:27 AM, Vladimir Sementsov-Ogievskiy wrote: >> Add new optional parameter making possible to merge bitmaps from >> different nodes. It is needed to maintain external snapshots during >> incremental backup chain history. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> qapi/block-core.json | 13 ++++++++++--- >> block/dirty-bitmap.c | 9 ++++++--- >> blockdev.c | 46 ++++++++++++++++++++++++++++++-------------- >> 3 files changed, 48 insertions(+), 20 deletions(-) >> -# @bitmaps: name(s) of the source dirty bitmap(s) >> +# @bitmaps: name(s) of the source dirty bitmap(s). The field is optional >> +# since 4.1. >> +# >> +# @external-bitmaps: additional list of source dirty bitmaps with specified >> +# nodes, which allows merging bitmaps between different >> +# nodes. (Since: 4.1) >> # >> # Since: 4.0 >> ## >> { 'struct': 'BlockDirtyBitmapMerge', >> - 'data': { 'node': 'str', 'target': 'str', 'bitmaps': ['str'] } } >> + 'data': { 'node': 'str', 'target': 'str', '*bitmaps': ['str'], >> + '*external-bitmaps': ['BlockDirtyBitmap'] } } >> > > I guess you can specify one, or both, or maybe neither! Seems fine. > > I don't think I like the name "external-bitmaps" but I guess I don't > really have a better suggestion. I do - we could use an alternate type instead: { 'alternate': 'BitmapSource', 'data': { 'local': 'str', 'external': 'BlockDirtyBitmap' } } then use 'bitmaps': ['BitmapSource'] so that the caller can pass: "bitmaps": [ "bitmap1", { "node": "other", "name", "bitmap2" } ] and we only have to deal with one array at all times, and not have the name 'external-bitmaps' to worry about.
17.05.2019 16:50, Eric Blake wrote: > On 5/16/19 7:32 PM, John Snow wrote: >> >> >> On 5/16/19 8:27 AM, Vladimir Sementsov-Ogievskiy wrote: >>> Add new optional parameter making possible to merge bitmaps from >>> different nodes. It is needed to maintain external snapshots during >>> incremental backup chain history. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> --- >>> qapi/block-core.json | 13 ++++++++++--- >>> block/dirty-bitmap.c | 9 ++++++--- >>> blockdev.c | 46 ++++++++++++++++++++++++++++++-------------- >>> 3 files changed, 48 insertions(+), 20 deletions(-) > >>> -# @bitmaps: name(s) of the source dirty bitmap(s) >>> +# @bitmaps: name(s) of the source dirty bitmap(s). The field is optional >>> +# since 4.1. >>> +# >>> +# @external-bitmaps: additional list of source dirty bitmaps with specified >>> +# nodes, which allows merging bitmaps between different >>> +# nodes. (Since: 4.1) >>> # >>> # Since: 4.0 >>> ## >>> { 'struct': 'BlockDirtyBitmapMerge', >>> - 'data': { 'node': 'str', 'target': 'str', 'bitmaps': ['str'] } } >>> + 'data': { 'node': 'str', 'target': 'str', '*bitmaps': ['str'], >>> + '*external-bitmaps': ['BlockDirtyBitmap'] } } >>> >> >> I guess you can specify one, or both, or maybe neither! Seems fine. > > >> >> I don't think I like the name "external-bitmaps" but I guess I don't >> really have a better suggestion. > > I do - we could use an alternate type instead: > > { 'alternate': 'BitmapSource', > 'data': { 'local': 'str', > 'external': 'BlockDirtyBitmap' } } > > then use 'bitmaps': ['BitmapSource'] > > so that the caller can pass: > > "bitmaps": [ "bitmap1", { "node": "other", "name", "bitmap2" } ] > > and we only have to deal with one array at all times, and not have the > name 'external-bitmaps' to worry about. > Oh, I wanted to do something like this, but looked at union type, which also needs some discriminator field, and decided that it's impossible to make it backward-compatible. Will resend.
On 5/17/19 10:09 AM, Vladimir Sementsov-Ogievskiy wrote: > 17.05.2019 16:50, Eric Blake wrote: >> On 5/16/19 7:32 PM, John Snow wrote: >>> >>> >>> On 5/16/19 8:27 AM, Vladimir Sementsov-Ogievskiy wrote: >>>> Add new optional parameter making possible to merge bitmaps from >>>> different nodes. It is needed to maintain external snapshots during >>>> incremental backup chain history. >>>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>> --- >>>> qapi/block-core.json | 13 ++++++++++--- >>>> block/dirty-bitmap.c | 9 ++++++--- >>>> blockdev.c | 46 ++++++++++++++++++++++++++++++-------------- >>>> 3 files changed, 48 insertions(+), 20 deletions(-) >> >>>> -# @bitmaps: name(s) of the source dirty bitmap(s) >>>> +# @bitmaps: name(s) of the source dirty bitmap(s). The field is optional >>>> +# since 4.1. >>>> +# >>>> +# @external-bitmaps: additional list of source dirty bitmaps with specified >>>> +# nodes, which allows merging bitmaps between different >>>> +# nodes. (Since: 4.1) >>>> # >>>> # Since: 4.0 >>>> ## >>>> { 'struct': 'BlockDirtyBitmapMerge', >>>> - 'data': { 'node': 'str', 'target': 'str', 'bitmaps': ['str'] } } >>>> + 'data': { 'node': 'str', 'target': 'str', '*bitmaps': ['str'], >>>> + '*external-bitmaps': ['BlockDirtyBitmap'] } } >>>> >>> >>> I guess you can specify one, or both, or maybe neither! Seems fine. >> >> >>> >>> I don't think I like the name "external-bitmaps" but I guess I don't >>> really have a better suggestion. >> >> I do - we could use an alternate type instead: >> >> { 'alternate': 'BitmapSource', >> 'data': { 'local': 'str', >> 'external': 'BlockDirtyBitmap' } } >> >> then use 'bitmaps': ['BitmapSource'] >> >> so that the caller can pass: >> >> "bitmaps": [ "bitmap1", { "node": "other", "name", "bitmap2" } ] >> >> and we only have to deal with one array at all times, and not have the >> name 'external-bitmaps' to worry about. >> > > Oh, I wanted to do something like this, but looked at union type, which also needs some > discriminator field, and decided that it's impossible to make it backward-compatible. > > Will resend. > Excellent! Thanks Eric! --js
diff --git a/qapi/block-core.json b/qapi/block-core.json index 7ccbfff9d0..933b50771a 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -2006,16 +2006,23 @@ ## # @BlockDirtyBitmapMerge: # -# @node: name of device/node which the bitmap is tracking +# @node: name of device/node which the @target and @bitmaps bitmaps are +# tracking # # @target: name of the destination dirty bitmap # -# @bitmaps: name(s) of the source dirty bitmap(s) +# @bitmaps: name(s) of the source dirty bitmap(s). The field is optional +# since 4.1. +# +# @external-bitmaps: additional list of source dirty bitmaps with specified +# nodes, which allows merging bitmaps between different +# nodes. (Since: 4.1) # # Since: 4.0 ## { 'struct': 'BlockDirtyBitmapMerge', - 'data': { 'node': 'str', 'target': 'str', 'bitmaps': ['str'] } } + 'data': { 'node': 'str', 'target': 'str', '*bitmaps': ['str'], + '*external-bitmaps': ['BlockDirtyBitmap'] } } ## # @block-dirty-bitmap-add: diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 59e6ebb861..49646a30e6 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -816,10 +816,10 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, { bool ret; - /* only bitmaps from one bds are supported */ - assert(dest->mutex == src->mutex); - qemu_mutex_lock(dest->mutex); + if (src->mutex != dest->mutex) { + qemu_mutex_lock(src->mutex); + } if (bdrv_dirty_bitmap_check(dest, BDRV_BITMAP_DEFAULT, errp)) { goto out; @@ -845,4 +845,7 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, out: qemu_mutex_unlock(dest->mutex); + if (src->mutex != dest->mutex) { + qemu_mutex_unlock(src->mutex); + } } diff --git a/blockdev.c b/blockdev.c index 79fbac8450..8d37ce5943 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2112,11 +2112,9 @@ static void block_dirty_bitmap_disable_abort(BlkActionState *common) } } -static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(const char *node, - const char *target, - strList *bitmaps, - HBitmap **backup, - Error **errp); +static BdrvDirtyBitmap *do_block_dirty_bitmap_merge( + const char *node, const char *target, strList *bitmaps, + BlockDirtyBitmapList *external_bitmaps, HBitmap **backup, Error **errp); static void block_dirty_bitmap_merge_prepare(BlkActionState *common, Error **errp) @@ -2132,8 +2130,9 @@ static void block_dirty_bitmap_merge_prepare(BlkActionState *common, action = common->action->u.block_dirty_bitmap_merge.data; state->bitmap = do_block_dirty_bitmap_merge(action->node, action->target, - action->bitmaps, &state->backup, - errp); + action->bitmaps, + action->external_bitmaps, + &state->backup, errp); } static void abort_prepare(BlkActionState *common, Error **errp) @@ -2965,15 +2964,14 @@ void qmp_block_dirty_bitmap_disable(const char *node, const char *name, bdrv_disable_dirty_bitmap(bitmap); } -static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(const char *node, - const char *target, - strList *bitmaps, - HBitmap **backup, - Error **errp) +static BdrvDirtyBitmap *do_block_dirty_bitmap_merge( + const char *node, const char *target, strList *bitmaps, + BlockDirtyBitmapList *external_bitmaps, HBitmap **backup, Error **errp) { BlockDriverState *bs; BdrvDirtyBitmap *dst, *src, *anon; strList *lst; + BlockDirtyBitmapList *ext_lst; Error *local_err = NULL; dst = block_dirty_bitmap_lookup(node, target, &bs, errp); @@ -3003,6 +3001,22 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(const char *node, } } + for (ext_lst = external_bitmaps; ext_lst; ext_lst = ext_lst->next) { + src = block_dirty_bitmap_lookup(ext_lst->value->node, + ext_lst->value->name, NULL, errp); + if (!src) { + dst = NULL; + goto out; + } + + bdrv_merge_dirty_bitmap(anon, src, NULL, &local_err); + if (local_err) { + error_propagate(errp, local_err); + dst = NULL; + goto out; + } + } + /* Merge into dst; dst is unchanged on failure. */ bdrv_merge_dirty_bitmap(dst, anon, backup, errp); @@ -3012,9 +3026,13 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(const char *node, } void qmp_block_dirty_bitmap_merge(const char *node, const char *target, - strList *bitmaps, Error **errp) + bool has_bitmaps, strList *bitmaps, + bool has_external_bitmaps, + BlockDirtyBitmapList *external_bitmaps, + Error **errp) { - do_block_dirty_bitmap_merge(node, target, bitmaps, NULL, errp); + do_block_dirty_bitmap_merge(node, target, bitmaps, external_bitmaps, NULL, + errp); } BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
Add new optional parameter making possible to merge bitmaps from different nodes. It is needed to maintain external snapshots during incremental backup chain history. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- qapi/block-core.json | 13 ++++++++++--- block/dirty-bitmap.c | 9 ++++++--- blockdev.c | 46 ++++++++++++++++++++++++++++++-------------- 3 files changed, 48 insertions(+), 20 deletions(-)