Allow bitmap successors to carry reference counts. We can in a later patch use this ability to clean up the dirty bitmap according to both the individual job's success and the success of all jobs in the transaction group.
As a consequence of moving the bitmap successor cleanup actions behind the frozen bitmap decref operation, the previous operations are made static, their errp parameter deleted, and some user-facing errors are replaced by assertions. "Also," The code for cleaning up a bitmap is also moved from backup_run to backup_complete. Signed-off-by: John Snow <js...@redhat.com> --- block.c | 78 +++++++++++++++++++++++++++++++++++++++------------ block/backup.c | 20 +++++-------- include/block/block.h | 10 +++---- 3 files changed, 71 insertions(+), 37 deletions(-) diff --git a/block.c b/block.c index a481654..1eb81ac 100644 --- a/block.c +++ b/block.c @@ -51,6 +51,12 @@ #include <windows.h> #endif +typedef enum BitmapSuccessorAction { + SUCCESSOR_ACTION_UNDEFINED = 0, + SUCCESSOR_ACTION_ABDICATE, + SUCCESSOR_ACTION_RECLAIM +} BitmapSuccessorAction; + /** * A BdrvDirtyBitmap can be in three possible states: * (1) successor is NULL and disabled is false: full r/w mode @@ -65,6 +71,8 @@ struct BdrvDirtyBitmap { char *name; /* Optional non-empty unique ID */ int64_t size; /* Size of the bitmap (Number of sectors) */ bool disabled; /* Bitmap is read-only */ + int successor_refcount; /* Number of active handles to the successor */ + BitmapSuccessorAction act; /* Action to take on successor upon release */ QLIST_ENTRY(BdrvDirtyBitmap) list; }; @@ -3156,6 +3164,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, /* Install the successor and freeze the parent */ bitmap->successor = child; + bitmap->successor_refcount = 1; return 0; } @@ -3163,19 +3172,13 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, * For a bitmap with a successor, yield our name to the successor, * delete the old bitmap, and return a handle to the new bitmap. */ -BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, - BdrvDirtyBitmap *bitmap, - Error **errp) +static BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, + BdrvDirtyBitmap *bitmap) { char *name; BdrvDirtyBitmap *successor = bitmap->successor; - if (successor == NULL) { - error_setg(errp, "Cannot relinquish control if " - "there's no successor present"); - return NULL; - } - + assert(successor); name = bitmap->name; bitmap->name = NULL; successor->name = name; @@ -3190,19 +3193,13 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, * we may wish to re-join the parent and child/successor. * The merged parent will be un-frozen, but not explicitly re-enabled. */ -BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, - BdrvDirtyBitmap *parent, - Error **errp) +static BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, + BdrvDirtyBitmap *parent) { BdrvDirtyBitmap *successor = parent->successor; - if (!successor) { - error_setg(errp, "Cannot reclaim a successor when none is present"); - return NULL; - } - + assert(successor); if (!hbitmap_merge(parent->bitmap, successor->bitmap)) { - error_setg(errp, "Merging of parent and successor bitmap failed"); return NULL; } bdrv_release_dirty_bitmap(bs, successor); @@ -3211,6 +3208,51 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, return parent; } +static BdrvDirtyBitmap *bdrv_free_bitmap_successor(BlockDriverState *bs, + BdrvDirtyBitmap *parent) +{ + assert(!parent->successor_refcount); + assert(parent->successor); + + switch (parent->act) { + case SUCCESSOR_ACTION_RECLAIM: + return bdrv_reclaim_dirty_bitmap(bs, parent); + case SUCCESSOR_ACTION_ABDICATE: + return bdrv_dirty_bitmap_abdicate(bs, parent); + case SUCCESSOR_ACTION_UNDEFINED: + default: + g_assert_not_reached(); + } +} + +BdrvDirtyBitmap *bdrv_frozen_bitmap_decref(BlockDriverState *bs, + BdrvDirtyBitmap *parent, + int ret) +{ + assert(bdrv_dirty_bitmap_frozen(parent)); + assert(parent->successor); + + if (ret) { + parent->act = SUCCESSOR_ACTION_RECLAIM; + } else if (parent->act != SUCCESSOR_ACTION_RECLAIM) { + parent->act = SUCCESSOR_ACTION_ABDICATE; + } + + parent->successor_refcount--; + if (parent->successor_refcount == 0) { + return bdrv_free_bitmap_successor(bs, parent); + } + return parent; +} + +void bdrv_frozen_bitmap_incref(BdrvDirtyBitmap *parent) +{ + assert(bdrv_dirty_bitmap_frozen(parent)); + assert(parent->successor); + + parent->successor_refcount++; +} + /** * Truncates _all_ bitmaps attached to a BDS. */ diff --git a/block/backup.c b/block/backup.c index d3f648d..4ac0be8 100644 --- a/block/backup.c +++ b/block/backup.c @@ -240,6 +240,12 @@ static void backup_complete(BlockJob *job, void *opaque) bdrv_unref(s->target); + if (s->sync_bitmap) { + BdrvDirtyBitmap *bm; + bm = bdrv_frozen_bitmap_decref(job->bs, s->sync_bitmap, data->ret); + assert(bm); + } + block_job_completed(job, data->ret); g_free(data); } @@ -428,18 +434,6 @@ static void coroutine_fn backup_run(void *opaque) qemu_co_rwlock_wrlock(&job->flush_rwlock); qemu_co_rwlock_unlock(&job->flush_rwlock); - if (job->sync_bitmap) { - BdrvDirtyBitmap *bm; - if (ret < 0) { - /* Merge the successor back into the parent, delete nothing. */ - bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL); - assert(bm); - } else { - /* Everything is fine, delete this bitmap and install the backup. */ - bm = bdrv_dirty_bitmap_abdicate(bs, job->sync_bitmap, NULL); - assert(bm); - } - } hbitmap_free(job->bitmap); bdrv_iostatus_disable(target); @@ -543,6 +537,6 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, error: if (sync_bitmap) { - bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL); + bdrv_frozen_bitmap_decref(bs, sync_bitmap, -ECANCELED); } } diff --git a/include/block/block.h b/include/block/block.h index 3d62d3e..b7892d2 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -459,12 +459,10 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, Error **errp); -BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, - BdrvDirtyBitmap *bitmap, - Error **errp); -BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, - BdrvDirtyBitmap *bitmap, - Error **errp); +BdrvDirtyBitmap *bdrv_frozen_bitmap_decref(BlockDriverState *bs, + BdrvDirtyBitmap *parent, + int ret); +void bdrv_frozen_bitmap_incref(BdrvDirtyBitmap *parent); BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name); void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap); -- 2.1.0