Dirty bitmaps fixing and refactoring
v2 cover Dirty bitmaps fixing and refactoring
v2:
05: drop bdrv_undo_clear_dirty_bitmap(), which becomes unused.
Vladimir Sementsov-Ogievskiy (7):
block/dirty-bitmap: add lock to bdrv_enable/disable_dirty_bitmap
dirty-bitmaps: fix comment about dirty_bitmap_mutex
dirty-bitmap: remove missed bdrv_dirty_bitmap_get_autoload header
dirty-bitmap: separate unused meta-bitmap related functions
blockdev: refactor block-dirty-bitmap-clear transaction block/dirty-bitmap: bdrv_clear_dirty_bitmap: drop unused parameter
blockdev: unify block-dirty-bitmap-clear command and transaction
action
include/block/block_int.h | 15 ++++++---- include/block/dirty-bitmap.h | 15 ++++++---- block/dirty-bitmap.c | 32 +++++++++----------- blockdev.c | 69 ++++++++++++++------------------------------ 4 files changed, 54 insertions(+), 77 deletions(-)
Signature · 1 line
2.11.1
PATCH v2 1/7 block/dirty-bitmap: add lock to bdrv_enable/disable_dirty_bitmap
Functions write to BdrvDirtyBitmap field, so the should take the lock.
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>
--- block/dirty-bitmap.c | 4 ++++ 1 file changed, 4 insertions(+)
block/dirty-bitmap.c +4 -0
@@ -445,15 +445,19 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,/* Called with BQL taken. */ void bdrv_disable_dirty_bitmap (BdrvDirtyBitmap *bitmap) { +bdrv_dirty_bitmap_lock (bitmap);assert (!bdrv_dirty_bitmap_frozen (bitmap)); bitmap->disabled = true; +bdrv_dirty_bitmap_unlock (bitmap); }/* Called with BQL taken. */ void bdrv_enable_dirty_bitmap (BdrvDirtyBitmap *bitmap) { +bdrv_dirty_bitmap_lock (bitmap);assert (!bdrv_dirty_bitmap_frozen (bitmap)); bitmap->disabled = false; +bdrv_dirty_bitmap_unlock (bitmap); }BlockDirtyInfoList *bdrv_query_dirty_bitmaps (BlockDriverState *bs)
Signature · 1 line
2.11.1
Re: [Qemu-devel] [PATCH v2 1/7] block/dirty-bitmap: add lock to bdrv_enable/disable_dirty_bitmap
Quoted 3 lines · Vladimir Sementsov-Ogievskiy
Functions write to BdrvDirtyBitmap field, so the should take the lock.
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>
Looks like this won't introduce any recursive locking that I can spot, so this looks correct.
Reviewed-by John Snow <[email protected]>
PATCH v2 2/7 dirty-bitmaps: fix comment about dirty_bitmap_mutex
In reply to Vladimir Sementsov-OgievskiyClarify first two cases and fix Modify -> Any access in third case. Also, drop 'only' from third case, as it a bit confuses, when thinking about case where we modify BdrvDirtyBitmap and access HBitmap.
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>
--- include/block/block_int.h | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
include/block/block_int.h +8 -4
@@ -709,10 +709,14 @@ struct BlockDriverState {uint64_t write_threshold_offset;NotifierWithReturn write_threshold_notifier; -/* Writing to the list requires the BQL _and_ the dirty_bitmap_mutex. -* Reading from the list can be done with either the BQL or the -* dirty_bitmap_mutex. Modifying a bitmap only requires -* dirty_bitmap_mutex. */ +/* Writing to the list (i.e. to any field of BdrvDirtyBitmap or to the +* list-head) requires both the BQL _and_ the dirty_bitmap_mutex. +* +* Reading from the list (from any field of BdrvDirtyBitmap or from the +* list-head) can be done with either the BQL or the dirty_bitmap_mutex. +* +* Any access to underlying HBitmap requires dirty_bitmap_mutex. +*/ QemuMutex dirty_bitmap_mutex; QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps;
Signature · 1 line
2.11.1
Re: [Qemu-devel] [PATCH v2 2/7] dirty-bitmaps: fix comment about dirty_bitmap_mutex
Quoted 18 lines · Vladimir Sementsov-Ogievskiy
Clarify first two cases and fix Modify -> Any access in third case. Also, drop 'only' from third case, as it a bit confuses, when thinking about case where we modify BdrvDirtyBitmap and access HBitmap.
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- include/block/block_int.h | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)include/block/block_int.h +0
-1@@ -709,10 +709,14 @@ struct BlockDriverState {uint64_t write_threshold_offset;NotifierWithReturn write_threshold_notifier; -/* Writing to the list requires the BQL _and_ the dirty_bitmap_mutex.
include/block/block_int.h struct BlockDriverState { · line 718
Quoted 10 lines
include/block/block_int.h +7
-3-* Reading from the list can be done with either the BQL or the -* dirty_bitmap_mutex. Modifying a bitmap only requires -* dirty_bitmap_mutex. */ +/* Writing to the list (i.e. to any field of BdrvDirtyBitmap or to the +* list-head) requires both the BQL _and_ the dirty_bitmap_mutex. +* +* Reading from the list (from any field of BdrvDirtyBitmap or from the +* list-head) can be done with either the BQL or the dirty_bitmap_mutex. +* +* Any access to underlying HBitmap requires dirty_bitmap_mutex.
"to the underlying HBitmap," probably.
include/block/block_int.h struct BlockDriverState { · line 722
Quoted 5 lines
include/block/block_int.h +1
-0+*/ QemuMutex dirty_bitmap_mutex; QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps;
Reviewed-by John Snow <[email protected]>
PATCH v2 3/7 dirty-bitmap: remove missed bdrv_dirty_bitmap_get_autoload header
In reply to Vladimir Sementsov-OgievskiySigned-off-by Vladimir Sementsov-Ogievskiy <[email protected]>
--- include/block/dirty-bitmap.h | 1 - 1 file changed, 1 deletion(-)
include/block/dirty-bitmap.h +0 -1
@@ -88,7 +88,6 @@ int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);void bdrv_dirty_bitmap_truncate (BlockDriverState *bs,int64_t bytes);bool bdrv_dirty_bitmap_readonly (const BdrvDirtyBitmap *bitmap);bool bdrv_has_readonly_bitmaps (BlockDriverState *bs); -bool bdrv_dirty_bitmap_get_autoload (const BdrvDirtyBitmap *bitmap);bool bdrv_dirty_bitmap_get_persistance (BdrvDirtyBitmap *bitmap);bool bdrv_dirty_bitmap_qmp_locked (BdrvDirtyBitmap *bitmap);bool bdrv_has_changed_persistent_bitmaps (BlockDriverState *bs);
Signature · 1 line
2.11.1
Re: [Qemu-devel] [PATCH v2 3/7] dirty-bitmap: remove missed bdrv_dirty_bitmap_get_autoload header
Quoted 11 lines · Vladimir Sementsov-Ogievskiy
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- include/block/dirty-bitmap.h | 1 - 1 file changed, 1 deletion(-)include/block/dirty-bitmap.h +0
-0
include/block/dirty-bitmap.h int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap); · line 93
Quoted 7 lines
include/block/dirty-bitmap.h +0
-1@@ -88,7 +88,6 @@ int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);void bdrv_dirty_bitmap_truncate (BlockDriverState *bs,int64_t bytes);bool bdrv_dirty_bitmap_readonly (const BdrvDirtyBitmap *bitmap);bool bdrv_has_readonly_bitmaps (BlockDriverState *bs); -bool bdrv_dirty_bitmap_get_autoload (const BdrvDirtyBitmap *bitmap);bool bdrv_dirty_bitmap_get_persistance (BdrvDirtyBitmap *bitmap);bool bdrv_dirty_bitmap_qmp_locked (BdrvDirtyBitmap *bitmap);bool bdrv_has_changed_persistent_bitmaps (BlockDriverState *bs);
Assuming you checked for others as well.
Reviewed-by John Snow <[email protected]>
PATCH v2 4/7 dirty-bitmap: separate unused meta-bitmap related functions
In reply to Vladimir Sementsov-OgievskiySeparate them in the header and clarify needed locking in comments.
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>
--- include/block/dirty-bitmap.h | 14 +++++++++----- block/dirty-bitmap.c | 5 +++++ 2 files changed, 14 insertions(+), 5 deletions(-)
include/block/dirty-bitmap.h +9 -5
@@ -9,9 +9,6 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,uint32_t granularity, const char *name, Error **errp); -void bdrv_create_meta_dirty_bitmap (BdrvDirtyBitmap *bitmap, -int chunk_size); -void bdrv_release_meta_dirty_bitmap (BdrvDirtyBitmap *bitmap);int bdrv_dirty_bitmap_create_successor (BlockDriverState *bs,BdrvDirtyBitmap *bitmap,Error **errp);
@@ -45,7 +42,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap, int64_t offset, int64_t bytes);void bdrv_reset_dirty_bitmap (BdrvDirtyBitmap *bitmap,int64_t offset,int64_t bytes); -BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new (BdrvDirtyBitmap *bitmap);BdrvDirtyBitmapIter *bdrv_dirty_iter_new (BdrvDirtyBitmap *bitmap);void bdrv_dirty_iter_free (BdrvDirtyBitmapIter *iter);
@@ -84,7 +80,6 @@ void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,int64_t bdrv_dirty_iter_next (BdrvDirtyBitmapIter *iter);void bdrv_set_dirty_iter (BdrvDirtyBitmapIter *hbi,int64_t offset);int64_t bdrv_get_dirty_count (BdrvDirtyBitmap *bitmap); -int64_t bdrv_get_meta_dirty_count (BdrvDirtyBitmap *bitmap);void bdrv_dirty_bitmap_truncate (BlockDriverState *bs,int64_t bytes);bool bdrv_dirty_bitmap_readonly (const BdrvDirtyBitmap *bitmap);bool bdrv_has_readonly_bitmaps (BlockDriverState *bs);
@@ -99,4 +94,13 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, Error **errp); +/* +* Unused for now meta-bitmaps related functions +*/ + +void bdrv_create_meta_dirty_bitmap (BdrvDirtyBitmap *bitmap,int chunk_size); +void bdrv_release_meta_dirty_bitmap (BdrvDirtyBitmap *bitmap); +BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new (BdrvDirtyBitmap *bitmap); +int64_t bdrv_get_meta_dirty_count (BdrvDirtyBitmap *bitmap); +#endif
block/dirty-bitmap.c +5 -0
@@ -149,6 +149,8 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,* @bitmap: the block dirty bitmap for which to create a meta dirty bitmap. * @chunk_size: how many bytes of bitmap data does each bit in the meta bitmap * track. +* +* Called with BQL taken. */ void bdrv_create_meta_dirty_bitmap (BdrvDirtyBitmap *bitmap,int chunk_size)
@@ -160,6 +162,7 @@ void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,qemu_mutex_unlock (bitmap->mutex); } +/* Called with BQL taken. */ void bdrv_release_meta_dirty_bitmap (BdrvDirtyBitmap *bitmap) {assert (bitmap->meta);
@@ -529,6 +532,7 @@ BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap)return iter; } +/* Called with BQL and dirty_bitmap_mutex locked. */ BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new (BdrvDirtyBitmap *bitmap) {BdrvDirtyBitmapIter *iter =g_new (BdrvDirtyBitmapIter,1 );
@@ -688,6 +692,7 @@ int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)return hbitmap_count (bitmap->bitmap); } +/* Called with BQL or dirty_bitmap_mutex locked */ int64_t bdrv_get_meta_dirty_count (BdrvDirtyBitmap *bitmap) {return hbitmap_count (bitmap->meta);
Signature · 1 line
2.11.1
Re: [Qemu-devel] [PATCH v2 4/7] dirty-bitmap: separate unused meta-bitmap related functions
Quoted 39 lines · Vladimir Sementsov-Ogievskiy
Separate them in the header and clarify needed locking in comments.
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- include/block/dirty-bitmap.h | 14 +++++++++----- block/dirty-bitmap.c | 5 +++++ 2 files changed, 14 insertions(+), 5 deletions(-)include/block/dirty-bitmap.h +0
-5@@ -9,9 +9,6 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,uint32_t granularity, const char *name, Error **errp); -void bdrv_create_meta_dirty_bitmap (BdrvDirtyBitmap *bitmap, -int chunk_size); -void bdrv_release_meta_dirty_bitmap (BdrvDirtyBitmap *bitmap);int bdrv_dirty_bitmap_create_successor (BlockDriverState *bs,BdrvDirtyBitmap *bitmap,Error **errp);@@ -45,7 +42,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap, int64_t offset, int64_t bytes);void bdrv_reset_dirty_bitmap (BdrvDirtyBitmap *bitmap,int64_t offset,int64_t bytes); -BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new (BdrvDirtyBitmap *bitmap);BdrvDirtyBitmapIter *bdrv_dirty_iter_new (BdrvDirtyBitmap *bitmap);void bdrv_dirty_iter_free (BdrvDirtyBitmapIter *iter);@@ -84,7 +80,6 @@ void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,int64_t bdrv_dirty_iter_next (BdrvDirtyBitmapIter *iter);void bdrv_set_dirty_iter (BdrvDirtyBitmapIter *hbi,int64_t offset);int64_t bdrv_get_dirty_count (BdrvDirtyBitmap *bitmap); -int64_t bdrv_get_meta_dirty_count (BdrvDirtyBitmap *bitmap);void bdrv_dirty_bitmap_truncate (BlockDriverState *bs,int64_t bytes);bool bdrv_dirty_bitmap_readonly (const BdrvDirtyBitmap *bitmap);bool bdrv_has_readonly_bitmaps (BlockDriverState *bs);
include/block/dirty-bitmap.h BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs, · line 100
Quoted 7 lines
include/block/dirty-bitmap.h +4
-0@@ -99,4 +94,13 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, Error **errp); +/* +* Unused for now meta-bitmaps related functions +*/ +
I assume you have plans to use them still? I thought these were useful for storage migration -- but I guess we don't use these for the postcopy mechanism?
Quoted 44 lines
include/block/dirty-bitmap.h +5
-0+void bdrv_create_meta_dirty_bitmap (BdrvDirtyBitmap *bitmap,int chunk_size); +void bdrv_release_meta_dirty_bitmap (BdrvDirtyBitmap *bitmap); +BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new (BdrvDirtyBitmap *bitmap); +int64_t bdrv_get_meta_dirty_count (BdrvDirtyBitmap *bitmap); +#endif block/dirty-bitmap.c +5
-0@@ -149,6 +149,8 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,* @bitmap: the block dirty bitmap for which to create a meta dirty bitmap. * @chunk_size: how many bytes of bitmap data does each bit in the meta bitmap * track. +* +* Called with BQL taken. */ void bdrv_create_meta_dirty_bitmap (BdrvDirtyBitmap *bitmap,int chunk_size)@@ -160,6 +162,7 @@ void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,qemu_mutex_unlock (bitmap->mutex); } +/* Called with BQL taken. */ void bdrv_release_meta_dirty_bitmap (BdrvDirtyBitmap *bitmap) {assert (bitmap->meta);@@ -529,6 +532,7 @@ BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap)return iter; } +/* Called with BQL and dirty_bitmap_mutex locked. */ BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new (BdrvDirtyBitmap *bitmap) {BdrvDirtyBitmapIter *iter =g_new (BdrvDirtyBitmapIter,1 );@@ -688,6 +692,7 @@ int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)return hbitmap_count (bitmap->bitmap); } +/* Called with BQL or dirty_bitmap_mutex locked */ int64_t bdrv_get_meta_dirty_count (BdrvDirtyBitmap *bitmap) {return hbitmap_count (bitmap->meta);
Re: [Qemu-devel] [PATCH v2 4/7] dirty-bitmap: separate unused meta-bitmap related functions
Quoted 43 lines · John Snow
Quoted 42 lines · Vladimir Sementsov-Ogievskiy
Separate them in the header and clarify needed locking in comments.
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- include/block/dirty-bitmap.h | 14 +++++++++----- block/dirty-bitmap.c | 5 +++++ 2 files changed, 14 insertions(+), 5 deletions(-)include/block/dirty-bitmap.h +0
-5@@ -9,9 +9,6 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,uint32_t granularity, const char *name, Error **errp); -void bdrv_create_meta_dirty_bitmap (BdrvDirtyBitmap *bitmap, -int chunk_size); -void bdrv_release_meta_dirty_bitmap (BdrvDirtyBitmap *bitmap);int bdrv_dirty_bitmap_create_successor (BlockDriverState *bs,BdrvDirtyBitmap *bitmap,Error **errp);@@ -45,7 +42,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap, int64_t offset, int64_t bytes);void bdrv_reset_dirty_bitmap (BdrvDirtyBitmap *bitmap,int64_t offset,int64_t bytes); -BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new (BdrvDirtyBitmap *bitmap);BdrvDirtyBitmapIter *bdrv_dirty_iter_new (BdrvDirtyBitmap *bitmap);void bdrv_dirty_iter_free (BdrvDirtyBitmapIter *iter);@@ -84,7 +80,6 @@ void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,int64_t bdrv_dirty_iter_next (BdrvDirtyBitmapIter *iter);void bdrv_set_dirty_iter (BdrvDirtyBitmapIter *hbi,int64_t offset);int64_t bdrv_get_dirty_count (BdrvDirtyBitmap *bitmap); -int64_t bdrv_get_meta_dirty_count (BdrvDirtyBitmap *bitmap);void bdrv_dirty_bitmap_truncate (BlockDriverState *bs,int64_t bytes);bool bdrv_dirty_bitmap_readonly (const BdrvDirtyBitmap *bitmap);bool bdrv_has_readonly_bitmaps (BlockDriverState *bs);@@ -99,4 +94,13 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, Error **errp);
Quoted 7 lines
include/block/dirty-bitmap.h BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs, · line 100
Quoted 4 lines
include/block/dirty-bitmap.h +4
-0+/* +* Unused for now meta-bitmaps related functions +*/ +I assume you have plans to use them still? I thought these were useful for storage migration -- but I guess we don't use these for the postcopy mechanism?
I don't have plans for near future, so for me it's ok to drop them.
Theoretical usage is partial storing (if we know, which portions of bitmaps are changed, we can save to file only these portions, not the whole bitmaps), but it don't look like near future. May be, Fam has some plans?
Quoted 45 lines
Quoted 45 lines
include/block/dirty-bitmap.h +5
-0+void bdrv_create_meta_dirty_bitmap (BdrvDirtyBitmap *bitmap,int chunk_size); +void bdrv_release_meta_dirty_bitmap (BdrvDirtyBitmap *bitmap); +BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new (BdrvDirtyBitmap *bitmap); +int64_t bdrv_get_meta_dirty_count (BdrvDirtyBitmap *bitmap); +#endif block/dirty-bitmap.c +5
-0@@ -149,6 +149,8 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,* @bitmap: the block dirty bitmap for which to create a meta dirty bitmap. * @chunk_size: how many bytes of bitmap data does each bit in the meta bitmap * track. +* +* Called with BQL taken. */ void bdrv_create_meta_dirty_bitmap (BdrvDirtyBitmap *bitmap,int chunk_size)@@ -160,6 +162,7 @@ void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,qemu_mutex_unlock (bitmap->mutex); } +/* Called with BQL taken. */ void bdrv_release_meta_dirty_bitmap (BdrvDirtyBitmap *bitmap) {assert (bitmap->meta);@@ -529,6 +532,7 @@ BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap)return iter; } +/* Called with BQL and dirty_bitmap_mutex locked. */ BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new (BdrvDirtyBitmap *bitmap) {BdrvDirtyBitmapIter *iter =g_new (BdrvDirtyBitmapIter,1 );@@ -688,6 +692,7 @@ int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)return hbitmap_count (bitmap->bitmap); } +/* Called with BQL or dirty_bitmap_mutex locked */ int64_t bdrv_get_meta_dirty_count (BdrvDirtyBitmap *bitmap) {return hbitmap_count (bitmap->meta);
Signature · 2 lines
Best regards, Vladimir
PATCH v2 5/7 blockdev: refactor block-dirty-bitmap-clear transaction
In reply to Vladimir Sementsov-Ogievskiybdrv_clear_dirty_bitmap do not fail, so we can call it in transaction commit, avoiding any rollback.
After this, bdrv_undo_clear_dirty_bitmap() becomes unused, so, drop it.
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>
--- include/block/block_int.h | 1 - block/dirty-bitmap.c | 9 --------- blockdev.c | 16 +--------------- 3 files changed, 1 insertion(+), 25 deletions(-)
include/block/block_int.h +0 -1
@@ -1087,7 +1087,6 @@ bool blk_dev_is_medium_locked(BlockBackend *blk);void bdrv_set_dirty (BlockDriverState *bs,int64_t offset,int64_t bytes);void bdrv_clear_dirty_bitmap (BdrvDirtyBitmap *bitmap,HBitmap **out); -void bdrv_undo_clear_dirty_bitmap (BdrvDirtyBitmap *bitmap,HBitmap *in);void bdrv_inc_in_flight (BlockDriverState *bs);void bdrv_dec_in_flight (BlockDriverState *bs);
block/dirty-bitmap.c +0 -9
@@ -607,15 +607,6 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)bdrv_dirty_bitmap_unlock (bitmap); } -void bdrv_undo_clear_dirty_bitmap (BdrvDirtyBitmap *bitmap,HBitmap *in) -{ -HBitmap *tmp = bitmap->bitmap; -assert (bdrv_dirty_bitmap_enabled (bitmap)); -assert (!bdrv_dirty_bitmap_readonly (bitmap)); - bitmap->bitmap = in; -hbitmap_free (tmp); -} -uint64_t bdrv_dirty_bitmap_serialization_size (const BdrvDirtyBitmap *bitmap,uint64_t offset,uint64_t bytes) {
blockdev.c +1 -15
@@ -2050,7 +2050,6 @@ typedef struct BlockDirtyBitmapState {BlkActionState common;BdrvDirtyBitmap *bitmap;BlockDriverState *bs; -HBitmap *backup;bool prepared; } BlockDirtyBitmapState;
@@ -2129,18 +2128,6 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common,error_setg (errp,"Cannot clear a readonly bitmap" );return ; } - -bdrv_clear_dirty_bitmap (state->bitmap, &state->backup); -} - -static void block_dirty_bitmap_clear_abort (BlkActionState *common) -{ -BlockDirtyBitmapState *state =DO_UPCAST (BlockDirtyBitmapState, - common, common); - -if (state->backup) { -bdrv_undo_clear_dirty_bitmap (state->bitmap, state->backup); - } }static void block_dirty_bitmap_clear_commit (BlkActionState *common)
@@ -2148,7 +2135,7 @@ static void block_dirty_bitmap_clear_commit(BlkActionState *common)BlockDirtyBitmapState *state =DO_UPCAST (BlockDirtyBitmapState, common, common); -hbitmap_free (state->backup); +bdrv_clear_dirty_bitmap (state->bitmap,NULL ); }static void abort_prepare (BlkActionState *common,Error **errp)
@@ -2210,7 +2197,6 @@ static const BlkActionOps actions[] = { .instance_size = sizeof(BlockDirtyBitmapState), .prepare = block_dirty_bitmap_clear_prepare, .commit = block_dirty_bitmap_clear_commit, - .abort = block_dirty_bitmap_clear_abort, } };
Signature · 1 line
2.11.1
Re: [Qemu-devel] [PATCH v2 5/7] blockdev: refactor block-dirty-bitmap-clear transaction
Quoted 5 lines · Vladimir Sementsov-Ogievskiy
bdrv_clear_dirty_bitmapdo not fail, so we can call it in transaction commit, avoiding any rollback.After this,
bdrv_undo_clear_dirty_bitmap()becomes unused, so, drop it.
I'm trying to remember why we ever bothered doing it the other way. Is it because of timings with respect to other operations and we wanted the "clear" to take effect sooner rather than later to co-operate with other commands?
(You and Nikolay hinted about similar problems in the Libvirt PULL api thread, too. It continues to be an issue.)
Hopping in my time machine:
https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg01229.html
(Oh, I wrote it...)
Looks like it was a conscious decision to avoid ordering conflicts with other block jobs.
Quoted 88 lines
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- include/block/block_int.h | 1 - block/dirty-bitmap.c | 9 --------- blockdev.c | 16 +--------------- 3 files changed, 1 insertion(+), 25 deletions(-)include/block/block_int.h +0
-1@@ -1087,7 +1087,6 @@ bool blk_dev_is_medium_locked(BlockBackend *blk);void bdrv_set_dirty (BlockDriverState *bs,int64_t offset,int64_t bytes);void bdrv_clear_dirty_bitmap (BdrvDirtyBitmap *bitmap,HBitmap **out); -void bdrv_undo_clear_dirty_bitmap (BdrvDirtyBitmap *bitmap,HBitmap *in);void bdrv_inc_in_flight (BlockDriverState *bs);void bdrv_dec_in_flight (BlockDriverState *bs);block/dirty-bitmap.c +0
-9@@ -607,15 +607,6 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)bdrv_dirty_bitmap_unlock (bitmap); } -void bdrv_undo_clear_dirty_bitmap (BdrvDirtyBitmap *bitmap,HBitmap *in) -{ -HBitmap *tmp = bitmap->bitmap; -assert (bdrv_dirty_bitmap_enabled (bitmap)); -assert (!bdrv_dirty_bitmap_readonly (bitmap)); - bitmap->bitmap = in; -hbitmap_free (tmp); -} -uint64_t bdrv_dirty_bitmap_serialization_size (const BdrvDirtyBitmap *bitmap,uint64_t offset,uint64_t bytes) {blockdev.c +1
-15@@ -2050,7 +2050,6 @@ typedef struct BlockDirtyBitmapState {BlkActionState common;BdrvDirtyBitmap *bitmap;BlockDriverState *bs; -HBitmap *backup;bool prepared; } BlockDirtyBitmapState;@@ -2129,18 +2128,6 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common,error_setg (errp,"Cannot clear a readonly bitmap" );return ; } - -bdrv_clear_dirty_bitmap (state->bitmap, &state->backup); -} - -static void block_dirty_bitmap_clear_abort (BlkActionState *common) -{ -BlockDirtyBitmapState *state =DO_UPCAST (BlockDirtyBitmapState, - common, common); - -if (state->backup) { -bdrv_undo_clear_dirty_bitmap (state->bitmap, state->backup); - } }static void block_dirty_bitmap_clear_commit (BlkActionState *common)@@ -2148,7 +2135,7 @@ static void block_dirty_bitmap_clear_commit(BlkActionState *common)BlockDirtyBitmapState *state =DO_UPCAST (BlockDirtyBitmapState, common, common); -hbitmap_free (state->backup); +bdrv_clear_dirty_bitmap (state->bitmap,NULL ); }static void abort_prepare (BlkActionState *common,Error **errp)@@ -2210,7 +2197,6 @@ static const BlkActionOps actions[] = { .instance_size = sizeof(BlockDirtyBitmapState), .prepare = block_dirty_bitmap_clear_prepare, .commit = block_dirty_bitmap_clear_commit, - .abort = block_dirty_bitmap_clear_abort, } };
Re: [Qemu-devel] [PATCH v2 5/7] blockdev: refactor block-dirty-bitmap-clear transaction
Quoted 15 lines · John Snow
Quoted 5 lines · Vladimir Sementsov-Ogievskiy
bdrv_clear_dirty_bitmapdo not fail, so we can call it in transaction commit, avoiding any rollback.After this,
bdrv_undo_clear_dirty_bitmap()becomes unused, so, drop it.I'm trying to remember why we ever bothered doing it the other way. Is it because of timings with respect to other operations and we wanted the "clear" to take effect sooner rather than later to co-operate with other commands?
(You and Nikolay hinted about similar problems in the Libvirt PULL api thread, too. It continues to be an issue.)
Quoted 8 lines
Hopping in my time machine:
https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg01229.html
(Oh, I wrote it...)
Looks like it was a conscious decision to avoid ordering conflicts with other block jobs.
ohh, interesting.. so, transactions are tricky.
Don't you think, that this letter shows, that backup transaction is wrong, rather than clear?
I think, when we write a transaction, we should understand, that full effect of the operation will be achieved only after corresponding .commit. So, backup should not do in .prepare something dependent on other operations.
Quoted 89 lines
Quoted 89 lines
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- include/block/block_int.h | 1 - block/dirty-bitmap.c | 9 --------- blockdev.c | 16 +--------------- 3 files changed, 1 insertion(+), 25 deletions(-)include/block/block_int.h +0
-1@@ -1087,7 +1087,6 @@ bool blk_dev_is_medium_locked(BlockBackend *blk);void bdrv_set_dirty (BlockDriverState *bs,int64_t offset,int64_t bytes);void bdrv_clear_dirty_bitmap (BdrvDirtyBitmap *bitmap,HBitmap **out); -void bdrv_undo_clear_dirty_bitmap (BdrvDirtyBitmap *bitmap,HBitmap *in);void bdrv_inc_in_flight (BlockDriverState *bs);void bdrv_dec_in_flight (BlockDriverState *bs);block/dirty-bitmap.c +0
-9@@ -607,15 +607,6 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)bdrv_dirty_bitmap_unlock (bitmap); } -void bdrv_undo_clear_dirty_bitmap (BdrvDirtyBitmap *bitmap,HBitmap *in) -{ -HBitmap *tmp = bitmap->bitmap; -assert (bdrv_dirty_bitmap_enabled (bitmap)); -assert (!bdrv_dirty_bitmap_readonly (bitmap)); - bitmap->bitmap = in; -hbitmap_free (tmp); -} -uint64_t bdrv_dirty_bitmap_serialization_size (const BdrvDirtyBitmap *bitmap,uint64_t offset,uint64_t bytes) {blockdev.c +1
-15@@ -2050,7 +2050,6 @@ typedef struct BlockDirtyBitmapState {BlkActionState common;BdrvDirtyBitmap *bitmap;BlockDriverState *bs; -HBitmap *backup;bool prepared; } BlockDirtyBitmapState;@@ -2129,18 +2128,6 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common,error_setg (errp,"Cannot clear a readonly bitmap" );return ; } - -bdrv_clear_dirty_bitmap (state->bitmap, &state->backup); -} - -static void block_dirty_bitmap_clear_abort (BlkActionState *common) -{ -BlockDirtyBitmapState *state =DO_UPCAST (BlockDirtyBitmapState, - common, common); - -if (state->backup) { -bdrv_undo_clear_dirty_bitmap (state->bitmap, state->backup); - } }static void block_dirty_bitmap_clear_commit (BlkActionState *common)@@ -2148,7 +2135,7 @@ static void block_dirty_bitmap_clear_commit(BlkActionState *common)BlockDirtyBitmapState *state =DO_UPCAST (BlockDirtyBitmapState, common, common); -hbitmap_free (state->backup); +bdrv_clear_dirty_bitmap (state->bitmap,NULL ); }static void abort_prepare (BlkActionState *common,Error **errp)@@ -2210,7 +2197,6 @@ static const BlkActionOps actions[] = { .instance_size = sizeof(BlockDirtyBitmapState), .prepare = block_dirty_bitmap_clear_prepare, .commit = block_dirty_bitmap_clear_commit, - .abort = block_dirty_bitmap_clear_abort, } };
Signature · 2 lines
Best regards, Vladimir
Re: [Qemu-devel] [PATCH v2 5/7] blockdev: refactor block-dirty-bitmap-clear transaction
In reply to Vladimir Sementsov-OgievskiyQuoted 7 lines · Vladimir Sementsov-Ogievskiy
bdrv_clear_dirty_bitmapdo not fail, so we can call it in transaction commit, avoiding any rollback.After this,
bdrv_undo_clear_dirty_bitmap()becomes unused, so, drop it.Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>---
Sorry if it was my idea entirely that led you on this wild goose chase, I had forgotten :(
PATCH v2 6/7 block/dirty-bitmap: bdrv_clear_dirty_bitmap: drop unused parameter
In reply to Vladimir Sementsov-OgievskiyDrop parameter "HBitmap **out" which is unused now, all callers set it to NULL.
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>
--- include/block/block_int.h | 2 +- block/dirty-bitmap.c | 14 +++++--------- blockdev.c | 4 ++-- 3 files changed, 8 insertions(+), 12 deletions(-)
include/block/block_int.h +1 -1
@@ -1086,7 +1086,7 @@ bool blk_dev_is_medium_locked(BlockBackend *blk);void bdrv_set_dirty (BlockDriverState *bs,int64_t offset,int64_t bytes); -void bdrv_clear_dirty_bitmap (BdrvDirtyBitmap *bitmap,HBitmap **out); +void bdrv_clear_dirty_bitmap (BdrvDirtyBitmap *bitmap);void bdrv_inc_in_flight (BlockDriverState *bs);void bdrv_dec_in_flight (BlockDriverState *bs);
block/dirty-bitmap.c +5 -9
@@ -591,19 +591,15 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,bdrv_dirty_bitmap_unlock (bitmap); } -void bdrv_clear_dirty_bitmap (BdrvDirtyBitmap *bitmap,HBitmap **out) +void bdrv_clear_dirty_bitmap (BdrvDirtyBitmap *bitmap) {assert (bdrv_dirty_bitmap_enabled (bitmap));assert (!bdrv_dirty_bitmap_readonly (bitmap)); +bdrv_dirty_bitmap_lock (bitmap); -if (!out) { -hbitmap_reset_all (bitmap->bitmap); - }else { -HBitmap *backup = bitmap->bitmap; - bitmap->bitmap =hbitmap_alloc (bitmap->size, -hbitmap_granularity (backup)); - *out = backup; - } + +hbitmap_reset_all (bitmap->bitmap); +bdrv_dirty_bitmap_unlock (bitmap); }
blockdev.c +2 -2
@@ -2135,7 +2135,7 @@ static void block_dirty_bitmap_clear_commit(BlkActionState *common)BlockDirtyBitmapState *state =DO_UPCAST (BlockDirtyBitmapState, common, common); -bdrv_clear_dirty_bitmap (state->bitmap,NULL ); +bdrv_clear_dirty_bitmap (state->bitmap); }static void abort_prepare (BlkActionState *common,Error **errp)
@@ -2906,7 +2906,7 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,return ; } -bdrv_clear_dirty_bitmap (bitmap,NULL ); +bdrv_clear_dirty_bitmap (bitmap); } BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
Signature · 1 line
2.11.1
Re: [Qemu-devel] [PATCH v2 6/7] block/dirty-bitmap: bdrv_clear_dirty_bitmap: drop unused parameter
Quoted 4 lines · Vladimir Sementsov-Ogievskiy
Drop parameter "HBitmap **out" which is unused now, all callers set it to NULL.
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>
And now this patch is in question too.
PATCH v2 7/7 blockdev: unify block-dirty-bitmap-clear command and transaction action
In reply to Vladimir Sementsov-Ogievskiy - use error messages from qmp command, as they are more descriptive
- we need not check bs, as block_dirty_bitmap_lookup never returns bs = NULL on success (and if user set bs to be not NULL pointer), so, let's just assert it.
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>
--- blockdev.c | 53 +++++++++++++++++++++-------------------------------- 1 file changed, 21 insertions(+), 32 deletions(-)
blockdev.c +21 -32
@@ -2114,18 +2114,26 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common,if (!state->bitmap) {return ; } +assert (state->bs !=NULL );if (bdrv_dirty_bitmap_frozen (state->bitmap)) { -error_setg (errp,"Cannot modify a frozen bitmap" ); +error_setg (errp, +"Bitmap '%s' is currently frozen and cannot be modified" , + action->name);return ; }else if (bdrv_dirty_bitmap_qmp_locked (state->bitmap)) { -error_setg (errp,"Cannot modify a locked bitmap" ); +error_setg (errp, +"Bitmap '%s' is currently locked and cannot be modified" , + action->name);return ; }else if (!bdrv_dirty_bitmap_enabled (state->bitmap)) { -error_setg (errp,"Cannot clear a disabled bitmap" ); +error_setg (errp, +"Bitmap '%s' is currently disabled and cannot be cleared" , + action->name);return ; }else if (bdrv_dirty_bitmap_readonly (state->bitmap)) { -error_setg (errp,"Cannot clear a readonly bitmap" ); +error_setg (errp,"Bitmap '%s' is readonly and cannot be cleared" , + action->name);return ; } }
@@ -2878,35 +2886,16 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,void qmp_block_dirty_bitmap_clear (const char *node,const char *name,Error **errp) { -BdrvDirtyBitmap *bitmap; -BlockDriverState *bs; - - bitmap =block_dirty_bitmap_lookup (node, name, &bs, errp); -if (!bitmap || !bs) { -return ; - } - -if (bdrv_dirty_bitmap_frozen (bitmap)) { -error_setg (errp, -"Bitmap '%s' is currently frozen and cannot be modified" , - name); -return ; - }else if (bdrv_dirty_bitmap_qmp_locked (bitmap)) { -error_setg (errp, -"Bitmap '%s' is currently locked and cannot be modified" , - name); -return ; - }else if (!bdrv_dirty_bitmap_enabled (bitmap)) { -error_setg (errp, -"Bitmap '%s' is currently disabled and cannot be cleared" , - name); -return ; - }else if (bdrv_dirty_bitmap_readonly (bitmap)) { -error_setg (errp,"Bitmap '%s' is readonly and cannot be cleared" , name); -return ; - } +BlockDirtyBitmap data = { + .node = (char *)node, + .name = (char *)name + }; +TransactionAction action = { + .type =TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_CLEAR , + .u.block_dirty_bitmap_clear.data = &data, + }; -bdrv_clear_dirty_bitmap (bitmap); +blockdev_do_action (&action, errp); } BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
Signature · 1 line
2.11.1