Message ID | 1421080265-2228-8-git-send-email-jsnow@redhat.com |
---|---|
State | New |
Headers | show |
On 2015-01-12 at 11:30, John Snow wrote: > From: Fam Zheng <famz@redhat.com> > > This allows to put the dirty bitmap into a disabled state where no more > writes will be tracked. > > It will be used before backup or writing to persistent file. > > Signed-off-by: Fam Zheng <famz@redhat.com> > Signed-off-by: John Snow <jsnow@redhat.com> > --- > block.c | 24 +++++++++++++++++++++++- > blockdev.c | 40 ++++++++++++++++++++++++++++++++++++++++ > include/block/block.h | 3 +++ > qapi/block-core.json | 28 ++++++++++++++++++++++++++++ > qmp-commands.hx | 10 ++++++++++ > 5 files changed, 104 insertions(+), 1 deletion(-) > > diff --git a/block.c b/block.c > index 7bf7079..bd4b449 100644 > --- a/block.c > +++ b/block.c > @@ -56,6 +56,7 @@ struct BdrvDirtyBitmap { > int64_t size; > int64_t granularity; > char *name; > + bool enabled; > QLIST_ENTRY(BdrvDirtyBitmap) list; > }; > > @@ -5373,10 +5374,16 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, > bitmap->granularity = granularity; > bitmap->bitmap = hbitmap_alloc(bitmap->size, ffs(sector_granularity) - 1); > bitmap->name = g_strdup(name); > + bitmap->enabled = true; > QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list); > return bitmap; > } > > +bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap) > +{ > + return bitmap->enabled; > +} > + > void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) > { > BdrvDirtyBitmap *bm, *next; > @@ -5391,6 +5398,16 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) > } > } > > +void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) > +{ > + bitmap->enabled = false; > +} > + > +void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap) > +{ > + bitmap->enabled = true; > +} > + > BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs) > { > BdrvDirtyBitmap *bm; > @@ -5458,7 +5475,9 @@ void bdrv_dirty_iter_init(BlockDriverState *bs, > void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, > int64_t cur_sector, int nr_sectors) > { > - hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); > + if (bdrv_dirty_bitmap_enabled(bitmap)) { > + hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); > + } Why are you checking whether the bitmap is enabled here in bdrv_set_dirty_bitmap(), but neither in bdrv_reset_dirty_bitmap() nor bdrv_clear_dirty_bitmap()? It seems consistent with the commit message which only states that disabled state means that no more writes (i.e. bdrv_set_dirty_bitmap()) will be tracked, but it still seems strange to me. > } > > void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, > @@ -5481,6 +5500,9 @@ static void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, > { > BdrvDirtyBitmap *bitmap; > QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { > + if (!bdrv_dirty_bitmap_enabled(bitmap)) { > + continue; > + } Same question as above, only this time it's about bdrv_reset_dirty(). In case the answer to the question is "that's intentional": Reviewed-by: Max Reitz <mreitz@redhat.com>
On 01/16/2015 11:28 AM, Max Reitz wrote: > On 2015-01-12 at 11:30, John Snow wrote: >> From: Fam Zheng <famz@redhat.com> >> >> This allows to put the dirty bitmap into a disabled state where no more >> writes will be tracked. >> >> It will be used before backup or writing to persistent file. >> >> Signed-off-by: Fam Zheng <famz@redhat.com> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> block.c | 24 +++++++++++++++++++++++- >> blockdev.c | 40 ++++++++++++++++++++++++++++++++++++++++ >> include/block/block.h | 3 +++ >> qapi/block-core.json | 28 ++++++++++++++++++++++++++++ >> qmp-commands.hx | 10 ++++++++++ >> 5 files changed, 104 insertions(+), 1 deletion(-) >> >> diff --git a/block.c b/block.c >> index 7bf7079..bd4b449 100644 >> --- a/block.c >> +++ b/block.c >> @@ -56,6 +56,7 @@ struct BdrvDirtyBitmap { >> int64_t size; >> int64_t granularity; >> char *name; >> + bool enabled; >> QLIST_ENTRY(BdrvDirtyBitmap) list; >> }; >> @@ -5373,10 +5374,16 @@ BdrvDirtyBitmap >> *bdrv_create_dirty_bitmap(BlockDriverState *bs, >> bitmap->granularity = granularity; >> bitmap->bitmap = hbitmap_alloc(bitmap->size, >> ffs(sector_granularity) - 1); >> bitmap->name = g_strdup(name); >> + bitmap->enabled = true; >> QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list); >> return bitmap; >> } >> +bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap) >> +{ >> + return bitmap->enabled; >> +} >> + >> void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap >> *bitmap) >> { >> BdrvDirtyBitmap *bm, *next; >> @@ -5391,6 +5398,16 @@ void bdrv_release_dirty_bitmap(BlockDriverState >> *bs, BdrvDirtyBitmap *bitmap) >> } >> } >> +void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) >> +{ >> + bitmap->enabled = false; >> +} >> + >> +void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap) >> +{ >> + bitmap->enabled = true; >> +} >> + >> BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs) >> { >> BdrvDirtyBitmap *bm; >> @@ -5458,7 +5475,9 @@ void bdrv_dirty_iter_init(BlockDriverState *bs, >> void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap >> *bitmap, >> int64_t cur_sector, int nr_sectors) >> { >> - hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); >> + if (bdrv_dirty_bitmap_enabled(bitmap)) { >> + hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); >> + } > > Why are you checking whether the bitmap is enabled here in > bdrv_set_dirty_bitmap(), but neither in bdrv_reset_dirty_bitmap() nor > bdrv_clear_dirty_bitmap()? > > It seems consistent with the commit message which only states that > disabled state means that no more writes (i.e. bdrv_set_dirty_bitmap()) > will be tracked, but it still seems strange to me. > >> } >> void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap >> *bitmap, >> @@ -5481,6 +5500,9 @@ static void bdrv_set_dirty(BlockDriverState *bs, >> int64_t cur_sector, >> { >> BdrvDirtyBitmap *bitmap; >> QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { >> + if (!bdrv_dirty_bitmap_enabled(bitmap)) { >> + continue; >> + } > > Same question as above, only this time it's about bdrv_reset_dirty(). > > In case the answer to the question is "that's intentional": > > Reviewed-by: Max Reitz <mreitz@redhat.com> > Just because it's intentional doesn't make it a good idea. Fam is currently suggesting we might just scrap the "enable/disable" and "frozen/normal" modes altogether in favor of a single unified status, but the original way he proposed enabled/disabled is that it simply, and exclusively enabled/disabled new writes to the bitmap -- it never was intended to block clears/resets. So the bitmap is still able to be consumed while disabled, for instance. How useful is this? Perhaps not very useful. Probably more confusing than useful. I will *probably* try to unify these parameters as Fam suggested, but I am really eager to hear from Markus on the QMP interface side before I bother sending out another minor iteration. I'll skip the R-b for now, since Fam had some ideas on how to make it nicer. Thanks! --js
diff --git a/block.c b/block.c index 7bf7079..bd4b449 100644 --- a/block.c +++ b/block.c @@ -56,6 +56,7 @@ struct BdrvDirtyBitmap { int64_t size; int64_t granularity; char *name; + bool enabled; QLIST_ENTRY(BdrvDirtyBitmap) list; }; @@ -5373,10 +5374,16 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, bitmap->granularity = granularity; bitmap->bitmap = hbitmap_alloc(bitmap->size, ffs(sector_granularity) - 1); bitmap->name = g_strdup(name); + bitmap->enabled = true; QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list); return bitmap; } +bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap) +{ + return bitmap->enabled; +} + void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) { BdrvDirtyBitmap *bm, *next; @@ -5391,6 +5398,16 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) } } +void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) +{ + bitmap->enabled = false; +} + +void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap) +{ + bitmap->enabled = true; +} + BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs) { BdrvDirtyBitmap *bm; @@ -5458,7 +5475,9 @@ void bdrv_dirty_iter_init(BlockDriverState *bs, void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t cur_sector, int nr_sectors) { - hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); + if (bdrv_dirty_bitmap_enabled(bitmap)) { + hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); + } } void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, @@ -5481,6 +5500,9 @@ static void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, { BdrvDirtyBitmap *bitmap; QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { + if (!bdrv_dirty_bitmap_enabled(bitmap)) { + continue; + } hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); } } diff --git a/blockdev.c b/blockdev.c index 95251c7..118cb6c 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1994,6 +1994,46 @@ void qmp_block_dirty_bitmap_remove(const char *node_ref, const char *name, aio_context_release(aio_context); } +void qmp_block_dirty_bitmap_enable(const char *node_ref, const char *name, + Error **errp) +{ + AioContext *aio_context; + BdrvDirtyBitmap *bitmap; + BlockDriverState *bs; + + bitmap = block_dirty_bitmap_lookup(node_ref, name, &bs, errp); + if (!bitmap) { + return; + } + + aio_context = bdrv_get_aio_context(bs); + aio_context_acquire(aio_context); + + bdrv_enable_dirty_bitmap(bitmap); + + aio_context_release(aio_context); +} + +void qmp_block_dirty_bitmap_disable(const char *node_ref, const char *name, + Error **errp) +{ + AioContext *aio_context; + BdrvDirtyBitmap *bitmap; + BlockDriverState *bs; + + bitmap = block_dirty_bitmap_lookup(node_ref, name, &bs, errp); + if (!bitmap) { + return; + } + + aio_context = bdrv_get_aio_context(bs); + aio_context_acquire(aio_context); + + bdrv_disable_dirty_bitmap(bitmap); + + aio_context_release(aio_context); +} + int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data) { const char *id = qdict_get_str(qdict, "id"); diff --git a/include/block/block.h b/include/block/block.h index 66f092a..a5bc249 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -437,10 +437,13 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name); void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap); void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap); +void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap); +void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap); BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs); uint64_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs); uint64_t bdrv_dirty_bitmap_granularity(BlockDriverState *bs, BdrvDirtyBitmap *bitmap); +bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap); int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector); void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t cur_sector, int nr_sectors); diff --git a/qapi/block-core.json b/qapi/block-core.json index f79d165..3e863b9 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -949,6 +949,34 @@ 'data': 'BlockDirtyBitmap' } ## +# @block-dirty-bitmap-enable +# +# Enable a dirty bitmap on the device +# +# Returns: nothing on success +# If @node-ref is not a valid block device, DeviceNotFound +# If @name is not found, GenericError with an explaining message +# +# Since 2.3 +## +{'command': 'block-dirty-bitmap-enable', + 'data': 'BlockDirtyBitmap' } + +## +# @block-dirty-bitmap-disable +# +# Disable a dirty bitmap on the device +# +# Returns: nothing on success +# If @node-ref is not a valid block device, DeviceNotFound +# If @name is not found, GenericError with an explaining message +# +# Since 2.3 +## +{'command': 'block-dirty-bitmap-disable', + 'data': 'BlockDirtyBitmap' } + +## # @block_set_io_throttle: # # Change I/O throttle limits for a block drive. diff --git a/qmp-commands.hx b/qmp-commands.hx index 4ffca8a..59be8eb 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1211,6 +1211,16 @@ EQMP .args_type = "node-ref:B,name:s", .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_remove, }, + { + .name = "block-dirty-bitmap-enable", + .args_type = "node-ref:B,name:s", + .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_enable, + }, + { + .name = "block-dirty-bitmap-disable", + .args_type = "node-ref:B,name:s", + .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_disable, + }, SQMP