From: Vladimir Sementsov-Ogievskiy <vsementsov(a)virtuozzo.com&gt; block/dirty-bitmap.c seems to be more appropriate for it and bdrv_remove_persistent_dirty_bitmap already in it. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)virtuozzo.com&gt; Reviewed-by: John Snow <jsnow(a)redhat.com&gt; Message-id: 20190920082543.23444-2-vsementsov(a)virtuozzo.com Signed-off-by: John Snow <jsnow(a)redhat.com&gt; --- block.c | 22 ---------------------- block/dirty-bitmap.c | 22 ++++++++++++++++++++++ 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/block.c b/block.c index 1946fc6f57..d19a4781a3 100644 --- a/block.c +++ b/block.c @@ -6582,25 +6582,3 @@ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp) parent_bs->drv->bdrv_del_child(parent_bs, child, errp); } - -bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, - uint32_t granularity, Error **errp) -{ - BlockDriver *drv = bs->drv; - - if (!drv) { - error_setg_errno(errp, ENOMEDIUM, - "Can't store persistent bitmaps to %s", - bdrv_get_device_or_node_name(bs)); - return false; - } - - if (!drv->bdrv_can_store_new_dirty_bitmap) { - error_setg_errno(errp, ENOTSUP, - "Can't store persistent bitmaps to %s", - bdrv_get_device_or_node_name(bs)); - return false; - } - - return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp); -} diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 134e0c9a0c..8f42015db9 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -464,6 +464,28 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, } } +bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, + uint32_t granularity, Error **errp) +{ + BlockDriver *drv = bs->drv; + + if (!drv) { + error_setg_errno(errp, ENOMEDIUM, + "Can't store persistent bitmaps to %s", + bdrv_get_device_or_node_name(bs)); + return false; + } + + if (!drv->bdrv_can_store_new_dirty_bitmap) { + error_setg_errno(errp, ENOTSUP, + "Can't store persistent bitmaps to %s", + bdrv_get_device_or_node_name(bs)); + return false; + } + + return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp); +} + void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) { bdrv_dirty_bitmap_lock(bitmap); -- 2.21.0

From: Vladimir Sementsov-Ogievskiy <vsementsov(a)virtuozzo.com&gt; qmp_block_dirty_bitmap_add and do_block_dirty_bitmap_remove do acquire aio context since 0a6c86d024c52b. But this is not enough: we also must lock qcow2 mutex when access in-image metadata. Especially it concerns freeing qcow2 clusters. To achieve this, move qcow2_can_store_new_dirty_bitmap and qcow2_remove_persistent_dirty_bitmap to coroutine context. Since we work in coroutines in correct aio context, we don't need context acquiring in blockdev.c anymore, drop it. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)virtuozzo.com&gt; Reviewed-by: John Snow <jsnow(a)redhat.com&gt; Message-id: 20190920082543.23444-4-vsementsov(a)virtuozzo.com Signed-off-by: John Snow <jsnow(a)redhat.com&gt; --- block/qcow2.h | 11 ++-- include/block/block_int.h | 10 ++-- block/dirty-bitmap.c | 102 +++++++++++++++++++++++++++++++++++--- block/qcow2-bitmap.c | 24 ++++++--- block/qcow2.c | 5 +- blockdev.c | 27 +++------- 6 files changed, 131 insertions(+), 48 deletions(-) diff --git a/block/qcow2.h b/block/qcow2.h index 08b4c15dc4..0f3d9b088e 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -746,12 +746,13 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp); void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp); int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); -bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, - const char *name, - uint32_t granularity, - Error **errp); -int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name, +bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs, + const char *name, + uint32_t granularity, Error **errp); +int qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs, + const char *name, + Error **errp); ssize_t coroutine_fn qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size, diff --git a/include/block/block_int.h b/include/block/block_int.h index 6b511dd889..32fb493cbb 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -553,13 +553,13 @@ struct BlockDriver { * field of BlockDirtyBitmap's in case of success. */ int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp); - bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs, - const char *name, - uint32_t granularity, - Error **errp); - int (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs, + bool (*bdrv_co_can_store_new_dirty_bitmap)(BlockDriverState *bs, const char *name, + uint32_t granularity, Error **errp); + int (*bdrv_co_remove_persistent_dirty_bitmap)(BlockDriverState *bs, + const char *name, + Error **errp); /** * Register/unregister a buffer for I/O. For example, when the driver is diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index d1ae2e1922..03e0872b97 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -26,6 +26,7 @@ #include "trace.h" #include "block/block_int.h" #include "block/blockjob.h" +#include "qemu/main-loop.h" struct BdrvDirtyBitmap { QemuMutex *mutex; @@ -455,18 +456,59 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs) * not fail. * This function doesn't release corresponding BdrvDirtyBitmap. */ +static int coroutine_fn +bdrv_co_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name, + Error **errp) +{ + if (bs->drv && bs->drv->bdrv_co_remove_persistent_dirty_bitmap) { + return bs->drv->bdrv_co_remove_persistent_dirty_bitmap(bs, name, errp); + } + + return 0; +} + +typedef struct BdrvRemovePersistentDirtyBitmapCo { + BlockDriverState *bs; + const char *name; + Error **errp; + int ret; +} BdrvRemovePersistentDirtyBitmapCo; + +static void coroutine_fn +bdrv_co_remove_persistent_dirty_bitmap_entry(void *opaque) +{ + BdrvRemovePersistentDirtyBitmapCo *s = opaque; + + s->ret = bdrv_co_remove_persistent_dirty_bitmap(s->bs, s->name, s->errp); + aio_wait_kick(); +} + int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name, Error **errp) { - if (bs->drv && bs->drv->bdrv_remove_persistent_dirty_bitmap) { - return bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp); + if (qemu_in_coroutine()) { + return bdrv_co_remove_persistent_dirty_bitmap(bs, name, errp); + } else { + Coroutine *co; + BdrvRemovePersistentDirtyBitmapCo s = { + .bs = bs, + .name = name, + .errp = errp, + .ret = -EINPROGRESS, + }; + + co = qemu_coroutine_create(bdrv_co_remove_persistent_dirty_bitmap_entry, + &s); + bdrv_coroutine_enter(bs, co); + BDRV_POLL_WHILE(bs, s.ret == -EINPROGRESS); + + return s.ret; } - - return 0; } -bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, - uint32_t granularity, Error **errp) +static bool coroutine_fn +bdrv_co_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, + uint32_t granularity, Error **errp) { BlockDriver *drv = bs->drv; @@ -477,14 +519,58 @@ bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, return false; } - if (!drv->bdrv_can_store_new_dirty_bitmap) { + if (!drv->bdrv_co_can_store_new_dirty_bitmap) { error_setg_errno(errp, ENOTSUP, "Can't store persistent bitmaps to %s", bdrv_get_device_or_node_name(bs)); return false; } - return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp); + return drv->bdrv_co_can_store_new_dirty_bitmap(bs, name, granularity, errp); +} + +typedef struct BdrvCanStoreNewDirtyBitmapCo { + BlockDriverState *bs; + const char *name; + uint32_t granularity; + Error **errp; + bool ret; + + bool in_progress; +} BdrvCanStoreNewDirtyBitmapCo; + +static void coroutine_fn bdrv_co_can_store_new_dirty_bitmap_entry(void *opaque) +{ + BdrvCanStoreNewDirtyBitmapCo *s = opaque; + + s->ret = bdrv_co_can_store_new_dirty_bitmap(s->bs, s->name, s->granularity, + s->errp); + s->in_progress = false; + aio_wait_kick(); +} + +bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, + uint32_t granularity, Error **errp) +{ + if (qemu_in_coroutine()) { + return bdrv_co_can_store_new_dirty_bitmap(bs, name, granularity, errp); + } else { + Coroutine *co; + BdrvCanStoreNewDirtyBitmapCo s = { + .bs = bs, + .name = name, + .granularity = granularity, + .errp = errp, + .in_progress = true, + }; + + co = qemu_coroutine_create(bdrv_co_can_store_new_dirty_bitmap_entry, + &s); + bdrv_coroutine_enter(bs, co); + BDRV_POLL_WHILE(bs, s.in_progress); + + return s.ret; + } } void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index 9821c1628f..644837eb03 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -1404,12 +1404,13 @@ static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList *bm_list, return NULL; } -int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name, - Error **errp) +int coroutine_fn qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs, + const char *name, + Error **errp) { int ret; BDRVQcow2State *s = bs->opaque; - Qcow2Bitmap *bm; + Qcow2Bitmap *bm = NULL; Qcow2BitmapList *bm_list; if (s->nb_bitmaps == 0) { @@ -1418,10 +1419,13 @@ int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name, return 0; } + qemu_co_mutex_lock(&s->lock); + bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, s->bitmap_directory_size, errp); if (bm_list == NULL) { - return -EIO; + ret = -EIO; + goto out; } bm = find_bitmap_by_name(bm_list, name); @@ -1441,6 +1445,8 @@ int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name, free_bitmap_clusters(bs, &bm->table); out: + qemu_co_mutex_unlock(&s->lock); + bitmap_free(bm); bitmap_list_free(bm_list); @@ -1615,10 +1621,10 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp) return 0; } -bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, - const char *name, - uint32_t granularity, - Error **errp) +bool coroutine_fn qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs, + const char *name, + uint32_t granularity, + Error **errp) { BDRVQcow2State *s = bs->opaque; bool found; @@ -1655,8 +1661,10 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, goto fail; } + qemu_co_mutex_lock(&s->lock); bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, s->bitmap_directory_size, errp); + qemu_co_mutex_unlock(&s->lock); if (bm_list == NULL) { goto fail; } diff --git a/block/qcow2.c b/block/qcow2.c index 7961c05783..7062eccaee 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -5407,8 +5407,9 @@ BlockDriver bdrv_qcow2 = { .bdrv_attach_aio_context = qcow2_attach_aio_context, .bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw, - .bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap, - .bdrv_remove_persistent_dirty_bitmap = qcow2_remove_persistent_dirty_bitmap, + .bdrv_co_can_store_new_dirty_bitmap = qcow2_co_can_store_new_dirty_bitmap, + .bdrv_co_remove_persistent_dirty_bitmap = + qcow2_co_remove_persistent_dirty_bitmap, }; static void bdrv_qcow2_init(void) diff --git a/blockdev.c b/blockdev.c index a45458a60a..7e12919724 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2898,16 +2898,10 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name, disabled = false; } - if (persistent) { - AioContext *aio_context = bdrv_get_aio_context(bs); - bool ok; - - aio_context_acquire(aio_context); - ok = bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp); - aio_context_release(aio_context); - if (!ok) { - return; - } + if (persistent && + !bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp)) + { + return; } bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp); @@ -2939,17 +2933,10 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_remove( return NULL; } - if (bdrv_dirty_bitmap_get_persistence(bitmap)) { - int ret; - AioContext *aio_context = bdrv_get_aio_context(bs); - - aio_context_acquire(aio_context); - ret = bdrv_remove_persistent_dirty_bitmap(bs, name, errp); - aio_context_release(aio_context); - - if (ret < 0) { + if (bdrv_dirty_bitmap_get_persistence(bitmap) && + bdrv_remove_persistent_dirty_bitmap(bs, name, errp) < 0) + { return NULL; - } } if (release) { -- 2.21.0

From: Vladimir Sementsov-Ogievskiy <vsementsov(a)virtuozzo.com&gt; Add bs field to BdrvDirtyBitmap structure. Drop BlockDriverState parameter from bitmap APIs where possible. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)virtuozzo.com&gt; Reviewed-by: John Snow <jsnow(a)redhat.com&gt; Message-id: 20190916141911.5255-3-vsementsov(a)virtuozzo.com [Rebased on top of block-copy. --js] Signed-off-by: John Snow <jsnow(a)redhat.com&gt; --- include/block/dirty-bitmap.h | 14 +++++--------- block/backup.c | 8 ++++---- block/block-copy.c | 2 +- block/dirty-bitmap.c | 24 ++++++++++++------------ block/mirror.c | 4 ++-- block/qcow2-bitmap.c | 6 +++--- blockdev.c | 6 +++--- migration/block-dirty-bitmap.c | 7 +++---- migration/block.c | 4 ++-- 9 files changed, 35 insertions(+), 40 deletions(-) diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h index 973056778a..2f9b088e11 100644 --- a/include/block/dirty-bitmap.h +++ b/include/block/dirty-bitmap.h @@ -18,21 +18,18 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, uint32_t granularity, const char *name, Error **errp); -int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, - BdrvDirtyBitmap *bitmap, +int bdrv_dirty_bitmap_create_successor(BdrvDirtyBitmap *bitmap, Error **errp); -BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, - BdrvDirtyBitmap *bitmap, +BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BdrvDirtyBitmap *bitmap, Error **errp); -BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, - BdrvDirtyBitmap *bitmap, +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BdrvDirtyBitmap *bitmap, Error **errp); void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap); BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name); int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags, Error **errp); -void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap); +void bdrv_release_dirty_bitmap(BdrvDirtyBitmap *bitmap); void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs); int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name, Error **errp); @@ -106,8 +103,7 @@ int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, uint64_t offset, uint64_t bytes); bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap, uint64_t *offset, uint64_t *bytes); -BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs, - BdrvDirtyBitmap *bitmap, +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap, Error **errp); #endif diff --git a/block/backup.c b/block/backup.c index 46978c1785..dddcf77f53 100644 --- a/block/backup.c +++ b/block/backup.c @@ -98,13 +98,13 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret) * We succeeded, or we always intended to sync the bitmap. * Delete this bitmap and install the child. */ - bm = bdrv_dirty_bitmap_abdicate(job->source_bs, job->sync_bitmap, NULL); + bm = bdrv_dirty_bitmap_abdicate(job->sync_bitmap, NULL); } else { /* * We failed, or we never intended to sync the bitmap anyway. * Merge the successor back into the parent, keeping all data. */ - bm = bdrv_reclaim_dirty_bitmap(job->source_bs, job->sync_bitmap, NULL); + bm = bdrv_reclaim_dirty_bitmap(job->sync_bitmap, NULL); } assert(bm); @@ -402,7 +402,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, } /* Create a new bitmap, and freeze/disable this one. */ - if (bdrv_dirty_bitmap_create_successor(bs, sync_bitmap, errp) < 0) { + if (bdrv_dirty_bitmap_create_successor(sync_bitmap, errp) < 0) { return NULL; } } @@ -472,7 +472,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, error: if (sync_bitmap) { - bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL); + bdrv_reclaim_dirty_bitmap(sync_bitmap, NULL); } if (job) { backup_clean(&job->common.job); diff --git a/block/block-copy.c b/block/block-copy.c index 0f76ea1e63..066e3a7274 100644 --- a/block/block-copy.c +++ b/block/block-copy.c @@ -60,7 +60,7 @@ void block_copy_state_free(BlockCopyState *s) return; } - bdrv_release_dirty_bitmap(s->source->bs, s->copy_bitmap); + bdrv_release_dirty_bitmap(s->copy_bitmap); g_free(s); } diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 4ecf18d5df..44453ff824 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -30,6 +30,7 @@ struct BdrvDirtyBitmap { QemuMutex *mutex; + BlockDriverState *bs; HBitmap *bitmap; /* Dirty bitmap implementation */ bool busy; /* Bitmap is busy, it can't be used via QMP */ BdrvDirtyBitmap *successor; /* Anonymous child, if any. */ @@ -115,6 +116,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, return NULL; } bitmap = g_new0(BdrvDirtyBitmap, 1); + bitmap->bs = bs; bitmap->mutex = &bs->dirty_bitmap_mutex; bitmap->bitmap = hbitmap_alloc(bitmap_size, ctz32(granularity)); bitmap->size = bitmap_size; @@ -237,8 +239,7 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags, * The successor will be enabled if the parent bitmap was. * Called with BQL taken. */ -int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, - BdrvDirtyBitmap *bitmap, Error **errp) +int bdrv_dirty_bitmap_create_successor(BdrvDirtyBitmap *bitmap, Error **errp) { uint64_t granularity; BdrvDirtyBitmap *child; @@ -254,7 +255,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, /* Create an anonymous successor */ granularity = bdrv_dirty_bitmap_granularity(bitmap); - child = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp); + child = bdrv_create_dirty_bitmap(bitmap->bs, granularity, NULL, errp); if (!child) { return -1; } @@ -300,8 +301,7 @@ static void bdrv_release_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap) * delete the old bitmap, and return a handle to the new bitmap. * Called with BQL taken. */ -BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, - BdrvDirtyBitmap *bitmap, +BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BdrvDirtyBitmap *bitmap, Error **errp) { char *name; @@ -320,7 +320,7 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, successor->persistent = bitmap->persistent; bitmap->persistent = false; bitmap->busy = false; - bdrv_release_dirty_bitmap(bs, bitmap); + bdrv_release_dirty_bitmap(bitmap); return successor; } @@ -332,8 +332,7 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, * The marged parent will be enabled if and only if the successor was enabled. * Called within bdrv_dirty_bitmap_lock..unlock and with BQL taken. */ -BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs, - BdrvDirtyBitmap *parent, +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BdrvDirtyBitmap *parent, Error **errp) { BdrvDirtyBitmap *successor = parent->successor; @@ -357,14 +356,13 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs, } /* Called with BQL taken. */ -BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, - BdrvDirtyBitmap *parent, +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BdrvDirtyBitmap *parent, Error **errp) { BdrvDirtyBitmap *ret; qemu_mutex_lock(parent->mutex); - ret = bdrv_reclaim_dirty_bitmap_locked(bs, parent, errp); + ret = bdrv_reclaim_dirty_bitmap_locked(parent, errp); qemu_mutex_unlock(parent->mutex); return ret; @@ -390,8 +388,10 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes) } /* Called with BQL taken. */ -void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) +void bdrv_release_dirty_bitmap(BdrvDirtyBitmap *bitmap) { + BlockDriverState *bs = bitmap->bs; + bdrv_dirty_bitmaps_lock(bs); bdrv_release_dirty_bitmap_locked(bitmap); bdrv_dirty_bitmaps_unlock(bs); diff --git a/block/mirror.c b/block/mirror.c index fe984efb90..a6c50caea4 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -638,7 +638,7 @@ static int mirror_exit_common(Job *job) bdrv_unfreeze_backing_chain(mirror_top_bs, target_bs); } - bdrv_release_dirty_bitmap(src, s->dirty_bitmap); + bdrv_release_dirty_bitmap(s->dirty_bitmap); /* Make sure that the source BDS doesn't go away during bdrv_replace_node, * before we can call bdrv_drained_end */ @@ -1709,7 +1709,7 @@ fail: blk_unref(s->target); bs_opaque->job = NULL; if (s->dirty_bitmap) { - bdrv_release_dirty_bitmap(bs, s->dirty_bitmap); + bdrv_release_dirty_bitmap(s->dirty_bitmap); } job_early_fail(&s->common.job); } diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index 644837eb03..687087d2bc 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -374,7 +374,7 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs, fail: g_free(bitmap_table); if (bitmap != NULL) { - bdrv_release_dirty_bitmap(bs, bitmap); + bdrv_release_dirty_bitmap(bitmap); } return NULL; @@ -941,7 +941,7 @@ fail: static void release_dirty_bitmap_helper(gpointer bitmap, gpointer bs) { - bdrv_release_dirty_bitmap(bs, bitmap); + bdrv_release_dirty_bitmap(bitmap); } /* for g_slist_foreach for GSList of BdrvDirtyBitmap* elements */ @@ -1577,7 +1577,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) continue; } - bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap); + bdrv_release_dirty_bitmap(bm->dirty_bitmap); } bitmap_list_free(bm_list); diff --git a/blockdev.c b/blockdev.c index 7e12919724..d77e809623 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2178,7 +2178,7 @@ static void block_dirty_bitmap_remove_commit(BlkActionState *common) common, common); bdrv_dirty_bitmap_set_busy(state->bitmap, false); - bdrv_release_dirty_bitmap(state->bs, state->bitmap); + bdrv_release_dirty_bitmap(state->bitmap); } static void abort_prepare(BlkActionState *common, Error **errp) @@ -2940,7 +2940,7 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_remove( } if (release) { - bdrv_release_dirty_bitmap(bs, bitmap); + bdrv_release_dirty_bitmap(bitmap); } if (bitmap_bs) { @@ -3072,7 +3072,7 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_merge( bdrv_merge_dirty_bitmap(dst, anon, backup, errp); out: - bdrv_release_dirty_bitmap(bs, anon); + bdrv_release_dirty_bitmap(anon); return dst; } diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c index 5121f86d73..793f249aa5 100644 --- a/migration/block-dirty-bitmap.c +++ b/migration/block-dirty-bitmap.c @@ -474,7 +474,7 @@ static int dirty_bitmap_load_start(QEMUFile *f, DirtyBitmapLoadState *s) if (flags & DIRTY_BITMAP_MIG_START_FLAG_ENABLED) { DirtyBitmapLoadBitmapState *b; - bdrv_dirty_bitmap_create_successor(s->bs, s->bitmap, &local_err); + bdrv_dirty_bitmap_create_successor(s->bitmap, &local_err); if (local_err) { error_report_err(local_err); return -EINVAL; @@ -535,13 +535,12 @@ static void dirty_bitmap_load_complete(QEMUFile *f, DirtyBitmapLoadState *s) bdrv_dirty_bitmap_lock(s->bitmap); if (enabled_bitmaps == NULL) { /* in postcopy */ - bdrv_reclaim_dirty_bitmap_locked(s->bs, s->bitmap, &error_abort); + bdrv_reclaim_dirty_bitmap_locked(s->bitmap, &error_abort); bdrv_enable_dirty_bitmap_locked(s->bitmap); } else { /* target not started, successor must be empty */ int64_t count = bdrv_get_dirty_count(s->bitmap); - BdrvDirtyBitmap *ret = bdrv_reclaim_dirty_bitmap_locked(s->bs, - s->bitmap, + BdrvDirtyBitmap *ret = bdrv_reclaim_dirty_bitmap_locked(s->bitmap, NULL); /* bdrv_reclaim_dirty_bitmap can fail only on no successor (it * must be) or on merge fail, but merge can't fail when second diff --git a/migration/block.c b/migration/block.c index 8e49382070..c90288ed29 100644 --- a/migration/block.c +++ b/migration/block.c @@ -361,7 +361,7 @@ static int set_dirty_tracking(void) fail: QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) { if (bmds->dirty_bitmap) { - bdrv_release_dirty_bitmap(blk_bs(bmds->blk), bmds->dirty_bitmap); + bdrv_release_dirty_bitmap(bmds->dirty_bitmap); } } return ret; @@ -374,7 +374,7 @@ static void unset_dirty_tracking(void) BlkMigDevState *bmds; QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) { - bdrv_release_dirty_bitmap(blk_bs(bmds->blk), bmds->dirty_bitmap); + bdrv_release_dirty_bitmap(bmds->dirty_bitmap); } } -- 2.21.0

From: Vladimir Sementsov-Ogievskiy <vsementsov(a)virtuozzo.com&gt; bdrv_dirty_bitmap_next is always used in same pattern. So, split it into _next and _first, instead of combining two functions into one and add FOR_EACH_DIRTY_BITMAP macro. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)virtuozzo.com&gt; Reviewed-by: John Snow <jsnow(a)redhat.com&gt; Message-id: 20190916141911.5255-5-vsementsov(a)virtuozzo.com Signed-off-by: John Snow <jsnow(a)redhat.com&gt; --- include/block/dirty-bitmap.h | 9 +++++++-- block.c | 4 +--- block/dirty-bitmap.c | 11 +++++++---- block/qcow2-bitmap.c | 8 ++------ migration/block-dirty-bitmap.c | 4 +--- 5 files changed, 18 insertions(+), 18 deletions(-) diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h index 2f9b088e11..257f0f6704 100644 --- a/include/block/dirty-bitmap.h +++ b/include/block/dirty-bitmap.h @@ -96,8 +96,13 @@ bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap); bool bdrv_dirty_bitmap_get_persistence(BdrvDirtyBitmap *bitmap); bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap); bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs); -BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs, - BdrvDirtyBitmap *bitmap); + +BdrvDirtyBitmap *bdrv_dirty_bitmap_first(BlockDriverState *bs); +BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BdrvDirtyBitmap *bitmap); +#define FOR_EACH_DIRTY_BITMAP(bs, bitmap) \ +for (bitmap = bdrv_dirty_bitmap_first(bs); bitmap; \ + bitmap = bdrv_dirty_bitmap_next(bitmap)) + char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp); int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, uint64_t offset, uint64_t bytes); diff --git a/block.c b/block.c index d19a4781a3..5721441697 100644 --- a/block.c +++ b/block.c @@ -5390,9 +5390,7 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, } } - for (bm = bdrv_dirty_bitmap_next(bs, NULL); bm; - bm = bdrv_dirty_bitmap_next(bs, bm)) - { + FOR_EACH_DIRTY_BITMAP(bs, bm) { bdrv_dirty_bitmap_skip_store(bm, false); } diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 4e5c87a907..6065db8094 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -851,11 +851,14 @@ bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs) return false; } -BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs, - BdrvDirtyBitmap *bitmap) +BdrvDirtyBitmap *bdrv_dirty_bitmap_first(BlockDriverState *bs) { - return bitmap == NULL ? QLIST_FIRST(&bs->dirty_bitmaps) : - QLIST_NEXT(bitmap, list); + return QLIST_FIRST(&bs->dirty_bitmaps); +} + +BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BdrvDirtyBitmap *bitmap) +{ + return QLIST_NEXT(bitmap, list); } char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp) diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index 687087d2bc..99812b418b 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -1488,9 +1488,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) } /* check constraints and names */ - for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL; - bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) - { + FOR_EACH_DIRTY_BITMAP(bs, bitmap) { const char *name = bdrv_dirty_bitmap_name(bitmap); uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap); Qcow2Bitmap *bm; @@ -1610,9 +1608,7 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp) return -EINVAL; } - for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL; - bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) - { + FOR_EACH_DIRTY_BITMAP(bs, bitmap) { if (bdrv_dirty_bitmap_get_persistence(bitmap)) { bdrv_dirty_bitmap_set_readonly(bitmap, true); } diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c index 793f249aa5..7eafface61 100644 --- a/migration/block-dirty-bitmap.c +++ b/migration/block-dirty-bitmap.c @@ -283,9 +283,7 @@ static int init_dirty_bitmap_migration(void) for (bs = bdrv_next_all_states(NULL); bs; bs = bdrv_next_all_states(bs)) { const char *name = bdrv_get_device_or_node_name(bs); - for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap; - bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) - { + FOR_EACH_DIRTY_BITMAP(bs, bitmap) { if (!bdrv_dirty_bitmap_name(bitmap)) { continue; } -- 2.21.0

From: Vladimir Sementsov-Ogievskiy <vsementsov(a)virtuozzo.com&gt; It's needed to fix reopening qcow2 with bitmaps to RW. Currently it can't work, as qcow2 needs write access to file child, to mark bitmaps in-image with IN_USE flag. But usually children goes after parents in reopen queue and file child is still RO on qcow2 reopen commit. Reverse reopen order to fix it. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)virtuozzo.com&gt; Acked-by: Max Reitz <mreitz(a)redhat.com&gt; Acked-by: John Snow <jsnow(a)redhat.com&gt; Message-id: 20190927122355.7344-3-vsementsov(a)virtuozzo.com Signed-off-by: John Snow <jsnow(a)redhat.com&gt; --- block.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/block.c b/block.c index 0347632c6c..cf312258a9 100644 --- a/block.c +++ b/block.c @@ -3486,10 +3486,16 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp) bs_entry->perms_checked = true; } - /* If we reach this point, we have success and just need to apply the - * changes + /* + * If we reach this point, we have success and just need to apply the + * changes. + * + * Reverse order is used to comfort qcow2 driver: on commit it need to write + * IN_USE flag to the image, to mark bitmaps in the image as invalid. But + * children are usually goes after parents in reopen-queue, so go from last + * to first element. */ - QTAILQ_FOREACH(bs_entry, bs_queue, entry) { + QTAILQ_FOREACH_REVERSE(bs_entry, bs_queue, entry) { bdrv_reopen_commit(&bs_entry->state); } -- 2.21.0

From: Vladimir Sementsov-Ogievskiy <vsementsov(a)virtuozzo.com&gt; Firstly, no reason to optimize failure path. Then, function name is ambiguous: it checks for readonly and similar things, but someone may think that it will ignore normal bitmaps which was just unchanged, and this is in bad relation with the fact that we should drop IN_USE flag for unchanged bitmaps in the image. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)virtuozzo.com&gt; Reviewed-by: John Snow <jsnow(a)redhat.com&gt; Message-id: 20190927122355.7344-5-vsementsov(a)virtuozzo.com Signed-off-by: John Snow <jsnow(a)redhat.com&gt; --- include/block/dirty-bitmap.h | 1 - block/dirty-bitmap.c | 12 ------------ block/qcow2-bitmap.c | 23 +++++++++++++---------- 3 files changed, 13 insertions(+), 23 deletions(-) diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h index 257f0f6704..958e7474fb 100644 --- a/include/block/dirty-bitmap.h +++ b/include/block/dirty-bitmap.h @@ -95,7 +95,6 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs); bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap); bool bdrv_dirty_bitmap_get_persistence(BdrvDirtyBitmap *bitmap); bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap); -bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs); BdrvDirtyBitmap *bdrv_dirty_bitmap_first(BlockDriverState *bs); BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BdrvDirtyBitmap *bitmap); diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 6065db8094..4bbb251b2c 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -839,18 +839,6 @@ bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap) return bitmap->inconsistent; } -bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs) -{ - BdrvDirtyBitmap *bm; - QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) { - if (bm->persistent && !bm->readonly && !bm->skip_store) { - return true; - } - } - - return false; -} - BdrvDirtyBitmap *bdrv_dirty_bitmap_first(BlockDriverState *bs) { return QLIST_FIRST(&bs->dirty_bitmaps); diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index 99812b418b..6dfc083548 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -1464,16 +1464,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) Qcow2Bitmap *bm; QSIMPLEQ_HEAD(, Qcow2BitmapTable) drop_tables; Qcow2BitmapTable *tb, *tb_next; - - if (!bdrv_has_changed_persistent_bitmaps(bs)) { - /* nothing to do */ - return; - } - - if (!can_write(bs)) { - error_setg(errp, "No write access"); - return; - } + bool need_write = false; QSIMPLEQ_INIT(&drop_tables); @@ -1499,6 +1490,8 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) continue; } + need_write = true; + if (check_constraints_on_bitmap(bs, name, granularity, errp) < 0) { error_prepend(errp, "Bitmap '%s' doesn't satisfy the constraints: ", name); @@ -1537,6 +1530,15 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) bm->dirty_bitmap = bitmap; } + if (!need_write) { + goto success; + } + + if (!can_write(bs)) { + error_setg(errp, "No write access"); + goto fail; + } + /* allocate clusters and store bitmaps */ QSIMPLEQ_FOREACH(bm, bm_list, entry) { if (bm->dirty_bitmap == NULL) { @@ -1578,6 +1580,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) bdrv_release_dirty_bitmap(bm->dirty_bitmap); } +success: bitmap_list_free(bm_list); return; -- 2.21.0

From: Vladimir Sementsov-Ogievskiy <vsementsov(a)virtuozzo.com&gt; qcow2_reopen_bitmaps_ro wants to store bitmaps and then mark them all readonly. But the latter don't work, as qcow2_store_persistent_dirty_bitmaps removes bitmaps after storing. It's OK for inactivation but bad idea for reopen-ro. And this leads to the following bug: Assume we have persistent bitmap 'bitmap0'. Create external snapshot bitmap0 is stored and therefore removed Commit snapshot now we have no bitmaps Do some writes from guest (*) they are not marked in bitmap Shutdown Start bitmap0 is loaded as valid, but it is actually broken! It misses writes (*) Incremental backup it will be inconsistent So, let's stop removing bitmaps on reopen-ro. But don't rejoice: reopening bitmaps to rw is broken too, so the whole scenario will not work after this patch and we can't enable corresponding test cases in 260 iotests still. Reopening bitmaps rw will be fixed in the following patches. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)virtuozzo.com&gt; Reviewed-by: John Snow <jsnow(a)redhat.com&gt; Message-id: 20190927122355.7344-7-vsementsov(a)virtuozzo.com Signed-off-by: John Snow <jsnow(a)redhat.com&gt; --- block/qcow2.h | 3 ++- block/qcow2-bitmap.c | 49 ++++++++++++++++++++++++++++++-------------- block/qcow2.c | 2 +- 3 files changed, 37 insertions(+), 17 deletions(-) diff --git a/block/qcow2.h b/block/qcow2.h index 23a9898a54..5cccd87162 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -742,7 +742,8 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs, Error **errp); int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp); -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp); +void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, + bool release_stored, Error **errp); int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index ebc1afccd3..f7dfb40256 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -1440,7 +1440,32 @@ out: return ret; } -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) +/* + * qcow2_store_persistent_dirty_bitmaps + * + * Stores persistent BdrvDirtyBitmap objects. + * + * @release_stored: if true, release BdrvDirtyBitmap's after storing to the + * image. This is used in two cases, both via qcow2_inactivate: + * 1. bdrv_close: It's correct to remove bitmaps on close. + * 2. migration: If bitmaps are migrated through migration channel via + * 'dirty-bitmaps' migration capability they are not handled by this code. + * Otherwise, it's OK to drop BdrvDirtyBitmap's and reload them on + * invalidation. + * + * Anyway, it's correct to remove BdrvDirtyBitmap's on inactivation, as + * inactivation means that we lose control on disk, and therefore on bitmaps, + * we should sync them and do not touch more. + * + * Contrariwise, we don't want to release any bitmaps on just reopen-to-ro, + * when we need to store them, as image is still under our control, and it's + * good to keep all the bitmaps in read-only mode. Moreover, keeping them + * read-only is correct because this is what would happen if we opened the node + * readonly to begin with, and whether we opened directly or reopened to that + * state shouldn't matter for the state we get afterward. + */ +void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, + bool release_stored, Error **errp) { BdrvDirtyBitmap *bitmap; BDRVQcow2State *s = bs->opaque; @@ -1551,20 +1576,14 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) g_free(tb); } - QSIMPLEQ_FOREACH(bm, bm_list, entry) { - /* For safety, we remove bitmap after storing. - * We may be here in two cases: - * 1. bdrv_close. It's ok to drop bitmap. - * 2. inactivation. It means migration without 'dirty-bitmaps' - * capability, so bitmaps are not marked with - * BdrvDirtyBitmap.migration flags. It's not bad to drop them too, - * and reload on invalidation. - */ - if (bm->dirty_bitmap == NULL) { - continue; - } + if (release_stored) { + QSIMPLEQ_FOREACH(bm, bm_list, entry) { + if (bm->dirty_bitmap == NULL) { + continue; + } - bdrv_release_dirty_bitmap(bm->dirty_bitmap); + bdrv_release_dirty_bitmap(bm->dirty_bitmap); + } } success: @@ -1592,7 +1611,7 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp) BdrvDirtyBitmap *bitmap; Error *local_err = NULL; - qcow2_store_persistent_dirty_bitmaps(bs, &local_err); + qcow2_store_persistent_dirty_bitmaps(bs, false, &local_err); if (local_err != NULL) { error_propagate(errp, local_err); return -EINVAL; diff --git a/block/qcow2.c b/block/qcow2.c index 7062eccaee..53a025703e 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2503,7 +2503,7 @@ static int qcow2_inactivate(BlockDriverState *bs) int ret, result = 0; Error *local_err = NULL; - qcow2_store_persistent_dirty_bitmaps(bs, &local_err); + qcow2_store_persistent_dirty_bitmaps(bs, true, &local_err); if (local_err != NULL) { result = -EINVAL; error_reportf_err(local_err, "Lost persistent bitmaps during " -- 2.21.0

From: Vladimir Sementsov-Ogievskiy <vsementsov(a)virtuozzo.com&gt; - Correct check for write access to file child, and in correct place (only if we want to write). - Support reopen rw -> rw (which will be used in following commit), for example, !bdrv_dirty_bitmap_readonly() is not a corruption if bitmap is marked IN_USE in the image. - Consider unexpected bitmap as a corruption and check other combinations of in-image and in-RAM bitmaps. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)virtuozzo.com&gt; Message-id: 20190927122355.7344-9-vsementsov(a)virtuozzo.com Signed-off-by: John Snow <jsnow(a)redhat.com&gt; --- block/qcow2-bitmap.c | 77 +++++++++++++++++++++++++++++++++----------- 1 file changed, 58 insertions(+), 19 deletions(-) diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index f7dfb40256..98294a7696 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -1108,18 +1108,14 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp) Qcow2BitmapList *bm_list; Qcow2Bitmap *bm; GSList *ro_dirty_bitmaps = NULL; - int ret = 0; + int ret = -EINVAL; + bool need_header_update = false; if (s->nb_bitmaps == 0) { /* No bitmaps - nothing to do */ return 0; } - if (!can_write(bs)) { - error_setg(errp, "Can't write to the image on reopening bitmaps rw"); - return -EINVAL; - } - bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, s->bitmap_directory_size, errp); if (bm_list == NULL) { @@ -1128,32 +1124,75 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp) QSIMPLEQ_FOREACH(bm, bm_list, entry) { BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name); - if (bitmap == NULL) { - continue; - } - if (!bdrv_dirty_bitmap_readonly(bitmap)) { - error_setg(errp, "Bitmap %s was loaded prior to rw-reopen, but was " - "not marked as readonly. This is a bug, something went " - "wrong. All of the bitmaps may be corrupted", bm->name); - ret = -EINVAL; + if (!bitmap) { + error_setg(errp, "Unexpected bitmap '%s' in image '%s'", + bm->name, bs->filename); goto out; } - bm->flags |= BME_FLAG_IN_USE; - ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap); + if (!(bm->flags & BME_FLAG_IN_USE)) { + if (!bdrv_dirty_bitmap_readonly(bitmap)) { + error_setg(errp, "Corruption: bitmap '%s' is not marked IN_USE " + "in the image '%s' and not marked readonly in RAM", + bm->name, bs->filename); + goto out; + } + if (bdrv_dirty_bitmap_inconsistent(bitmap)) { + error_setg(errp, "Corruption: bitmap '%s' is inconsistent but " + "is not marked IN_USE in the image '%s'", bm->name, + bs->filename); + goto out; + } + + bm->flags |= BME_FLAG_IN_USE; + need_header_update = true; + } else { + /* + * What if flags already has BME_FLAG_IN_USE ? + * + * 1. if we are reopening RW -> RW it's OK, of course. + * 2. if we are reopening RO -> RW: + * 2.1 if @bitmap is inconsistent, it's OK. It means that it was + * inconsistent (IN_USE) when we loaded it + * 2.2 if @bitmap is not inconsistent. This seems to be impossible + * and implies third party interaction. Let's error-out for + * safety. + */ + if (bdrv_dirty_bitmap_readonly(bitmap) && + !bdrv_dirty_bitmap_inconsistent(bitmap)) + { + error_setg(errp, "Corruption: bitmap '%s' is marked IN_USE " + "in the image '%s' but it is readonly and " + "consistent in RAM", + bm->name, bs->filename); + goto out; + } + } + + if (bdrv_dirty_bitmap_readonly(bitmap)) { + ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap); + } } - if (ro_dirty_bitmaps != NULL) { + if (need_header_update) { + if (!can_write(bs->file->bs) || !(bs->file->perm & BLK_PERM_WRITE)) { + error_setg(errp, "Failed to reopen bitmaps rw: no write access " + "the protocol file"); + goto out; + } + /* in_use flags must be updated */ ret = update_ext_header_and_dir_in_place(bs, bm_list); if (ret < 0) { - error_setg_errno(errp, -ret, "Can't update bitmap directory"); + error_setg_errno(errp, -ret, "Cannot update bitmap directory"); goto out; } - g_slist_foreach(ro_dirty_bitmaps, set_readonly_helper, false); } + g_slist_foreach(ro_dirty_bitmaps, set_readonly_helper, false); + ret = 0; + out: g_slist_free(ro_dirty_bitmaps); bitmap_list_free(bm_list); -- 2.21.0

I already try to make sure all bitmaps patches have been reviewed by both Red Hat and Virtuozzo anyway, so this formalizes the arrangement. Fam meanwhile is no longer as active, so I am removing him as a co-maintainer simply to reflect the current practice. Signed-off-by: John Snow <jsnow(a)redhat.com&gt; Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)virtuozzo.com&gt; Message-id: 20191005194448.16629-2-jsnow(a)redhat.com --- MAINTAINERS | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index fe4dc51b08..250ce8e7a1 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1816,8 +1816,8 @@ F: qapi/transaction.json T: git https://repo.or.cz/qemu/armbru.git block-next Dirty Bitmaps -M: Fam Zheng <fam(a)euphon.net&gt; M: John Snow <jsnow(a)redhat.com&gt; +R: Vladimir Sementsov-Ogievskiy <vsementsov(a)virtuozzo.com&gt; L: qemu-block(a)nongnu.org S: Supported F: util/hbitmap.c @@ -1826,7 +1826,6 @@ F: include/qemu/hbitmap.h F: include/block/dirty-bitmap.h F: tests/test-hbitmap.c F: docs/interop/bitmaps.rst -T: git https://github.com/famz/qemu.git bitmaps T: git https://github.com/jnsnow/qemu.git bitmaps Character device backends -- 2.21.0