| Message ID | 20190701201330.29718-3-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: block-dirty-bitmap-remove transaction action | expand |
On 01.07.19 22:13, John Snow wrote: > It is used to do transactional movement of the bitmap (which is > possible in conjunction with merge command). Transactional bitmap > movement is needed in scenarios with external snapshot, when we don't > want to leave copy of the bitmap in the base image. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Signed-off-by: John Snow <jsnow@redhat.com> > --- > qapi/transaction.json | 2 + > include/block/dirty-bitmap.h | 3 +- > block.c | 2 +- > block/dirty-bitmap.c | 16 +++---- > blockdev.c | 79 +++++++++++++++++++++++++++++++--- > migration/block-dirty-bitmap.c | 2 +- > 6 files changed, 87 insertions(+), 17 deletions(-) [...] > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index 95a9c2a5d8..8551f8219e 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -48,10 +48,9 @@ struct BdrvDirtyBitmap { > bool inconsistent; /* bitmap is persistent, but inconsistent. > It cannot be used at all in any way, except > a QMP user can remove it. */ > - bool migration; /* Bitmap is selected for migration, it should > - not be stored on the next inactivation > - (persistent flag doesn't matter until next > - invalidation).*/ > + bool squelch_persistence; /* We are either migrating or deleting this > + * bitmap; it should not be stored on the next > + * inactivation. */ I like the English lessons you give me, but why not just dont_store? Or skip_storing? > QLIST_ENTRY(BdrvDirtyBitmap) list; > }; > [...] > diff --git a/blockdev.c b/blockdev.c > index 01248252ca..4143ab7bbb 100644 > --- a/blockdev.c > +++ b/blockdev.c [...] > +static void block_dirty_bitmap_remove_abort(BlkActionState *common) > +{ > + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, > + common, common); > + > + if (state->bitmap) { > + bdrv_dirty_bitmap_squelch_persistence(state->bitmap, false); > + bdrv_dirty_bitmap_set_busy(state->bitmap, false); Don’t you need to undo the removal? Like, re-add it to state->bs, and re-store it? [...] > @@ -2892,13 +2944,28 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name, > aio_context_acquire(aio_context); > bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err); > aio_context_release(aio_context); > + > if (local_err != NULL) { > error_propagate(errp, local_err); > - return; > + return NULL; > } > } > > - bdrv_release_dirty_bitmap(bs, bitmap); > + if (release) { > + bdrv_release_dirty_bitmap(bs, bitmap); > + } > + > + if (bitmap_bs) { > + *bitmap_bs = bs; > + } > + > + return bitmap; I’d prefer “release ? NULL : bitmap”. Max > +} > + > +void qmp_block_dirty_bitmap_remove(const char *node, const char *name, > + Error **errp) > +{ > + do_block_dirty_bitmap_remove(node, name, true, NULL, errp); > } > > /**
On 03.07.19 21:30, Max Reitz wrote: > On 01.07.19 22:13, John Snow wrote: >> It is used to do transactional movement of the bitmap (which is >> possible in conjunction with merge command). Transactional bitmap >> movement is needed in scenarios with external snapshot, when we don't >> want to leave copy of the bitmap in the base image. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> qapi/transaction.json | 2 + >> include/block/dirty-bitmap.h | 3 +- >> block.c | 2 +- >> block/dirty-bitmap.c | 16 +++---- >> blockdev.c | 79 +++++++++++++++++++++++++++++++--- >> migration/block-dirty-bitmap.c | 2 +- >> 6 files changed, 87 insertions(+), 17 deletions(-) > > [...] > >> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >> index 95a9c2a5d8..8551f8219e 100644 >> --- a/block/dirty-bitmap.c >> +++ b/block/dirty-bitmap.c >> @@ -48,10 +48,9 @@ struct BdrvDirtyBitmap { >> bool inconsistent; /* bitmap is persistent, but inconsistent. >> It cannot be used at all in any way, except >> a QMP user can remove it. */ >> - bool migration; /* Bitmap is selected for migration, it should >> - not be stored on the next inactivation >> - (persistent flag doesn't matter until next >> - invalidation).*/ >> + bool squelch_persistence; /* We are either migrating or deleting this >> + * bitmap; it should not be stored on the next >> + * inactivation. */ > > I like the English lessons you give me, but why not just dont_store? Or > skip_storing? > >> QLIST_ENTRY(BdrvDirtyBitmap) list; >> }; >> > > [...] > >> diff --git a/blockdev.c b/blockdev.c >> index 01248252ca..4143ab7bbb 100644 >> --- a/blockdev.c >> +++ b/blockdev.c > > [...] > >> +static void block_dirty_bitmap_remove_abort(BlkActionState *common) >> +{ >> + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, >> + common, common); >> + >> + if (state->bitmap) { >> + bdrv_dirty_bitmap_squelch_persistence(state->bitmap, false); >> + bdrv_dirty_bitmap_set_busy(state->bitmap, false); > > Don’t you need to undo the removal? Like, re-add it to state->bs, and > re-store it? Ah, right, no need to re-add it because without *release(), it was never removed from state->bs. Having removed the persistent bitmap makes without doing anything here to re-add it to the qcow2 file makes me a bit uneasy, though... (It should be stored automatically when the qcow2 file is closed or anything, but, you know. Feel free to say “Yes, it will be stored automatically, don’t worry.”) Max > [...] > >> @@ -2892,13 +2944,28 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name, >> aio_context_acquire(aio_context); >> bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err); >> aio_context_release(aio_context); >> + >> if (local_err != NULL) { >> error_propagate(errp, local_err); >> - return; >> + return NULL; >> } >> } >> >> - bdrv_release_dirty_bitmap(bs, bitmap); >> + if (release) { >> + bdrv_release_dirty_bitmap(bs, bitmap); >> + } >> + >> + if (bitmap_bs) { >> + *bitmap_bs = bs; >> + } >> + >> + return bitmap; > > I’d prefer “release ? NULL : bitmap”. > > Max > >> +} >> + >> +void qmp_block_dirty_bitmap_remove(const char *node, const char *name, >> + Error **errp) >> +{ >> + do_block_dirty_bitmap_remove(node, name, true, NULL, errp); >> } >> >> /** >
On 7/3/19 3:30 PM, Max Reitz wrote: > On 01.07.19 22:13, John Snow wrote: >> It is used to do transactional movement of the bitmap (which is >> possible in conjunction with merge command). Transactional bitmap >> movement is needed in scenarios with external snapshot, when we don't >> want to leave copy of the bitmap in the base image. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> qapi/transaction.json | 2 + >> include/block/dirty-bitmap.h | 3 +- >> block.c | 2 +- >> block/dirty-bitmap.c | 16 +++---- >> blockdev.c | 79 +++++++++++++++++++++++++++++++--- >> migration/block-dirty-bitmap.c | 2 +- >> 6 files changed, 87 insertions(+), 17 deletions(-) > > [...] > >> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >> index 95a9c2a5d8..8551f8219e 100644 >> --- a/block/dirty-bitmap.c >> +++ b/block/dirty-bitmap.c >> @@ -48,10 +48,9 @@ struct BdrvDirtyBitmap { >> bool inconsistent; /* bitmap is persistent, but inconsistent. >> It cannot be used at all in any way, except >> a QMP user can remove it. */ >> - bool migration; /* Bitmap is selected for migration, it should >> - not be stored on the next inactivation >> - (persistent flag doesn't matter until next >> - invalidation).*/ >> + bool squelch_persistence; /* We are either migrating or deleting this >> + * bitmap; it should not be stored on the next >> + * inactivation. */ > > I like the English lessons you give me, but why not just dont_store? Or > skip_storing? > I have to give you something to look forward to in my patches, don't I? I also like "suppress_persistence" but I'll settle for "skip_store". >> QLIST_ENTRY(BdrvDirtyBitmap) list; >> }; >> > > [...] > >> diff --git a/blockdev.c b/blockdev.c >> index 01248252ca..4143ab7bbb 100644 >> --- a/blockdev.c >> +++ b/blockdev.c > > [...] > >> +static void block_dirty_bitmap_remove_abort(BlkActionState *common) >> +{ >> + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, >> + common, common); >> + >> + if (state->bitmap) { >> + bdrv_dirty_bitmap_squelch_persistence(state->bitmap, false); >> + bdrv_dirty_bitmap_set_busy(state->bitmap, false); > > Don’t you need to undo the removal? Like, re-add it to state->bs, and > re-store it? > > [...] > Not right away, anyway; undoing the suppression will allow it to flush out the next time that happens. Why not flush it RIGHT NOW? Well, we actually never flush bitmaps until final close right now. There has been some contention about how it's pointless to do so as we'd have to re-open the bitmap immediately anyway, and any results you'd get from querying the bitmap outside of QEMU would be meaningless. It does feel risky, though, and seems to frequently violate the principal of least surprise. >> @@ -2892,13 +2944,28 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name, >> aio_context_acquire(aio_context); >> bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err); >> aio_context_release(aio_context); >> + >> if (local_err != NULL) { >> error_propagate(errp, local_err); >> - return; >> + return NULL; >> } >> } >> >> - bdrv_release_dirty_bitmap(bs, bitmap); >> + if (release) { >> + bdrv_release_dirty_bitmap(bs, bitmap); >> + } >> + >> + if (bitmap_bs) { >> + *bitmap_bs = bs; >> + } >> + >> + return bitmap; > > I’d prefer “release ? NULL : bitmap”. > > Max > Sure. >> +} >> + >> +void qmp_block_dirty_bitmap_remove(const char *node, const char *name, >> + Error **errp) >> +{ >> + do_block_dirty_bitmap_remove(node, name, true, NULL, errp); >> } >> >> /** >
diff --git a/qapi/transaction.json b/qapi/transaction.json index 95edb78227..da95b804aa 100644 --- a/qapi/transaction.json +++ b/qapi/transaction.json @@ -45,6 +45,7 @@ # # - @abort: since 1.6 # - @block-dirty-bitmap-add: since 2.5 +# - @block-dirty-bitmap-remove: since 4.1 # - @block-dirty-bitmap-clear: since 2.5 # - @block-dirty-bitmap-enable: since 4.0 # - @block-dirty-bitmap-disable: since 4.0 @@ -61,6 +62,7 @@ 'data': { 'abort': 'Abort', 'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd', + 'block-dirty-bitmap-remove': 'BlockDirtyBitmap', 'block-dirty-bitmap-clear': 'BlockDirtyBitmap', 'block-dirty-bitmap-enable': 'BlockDirtyBitmap', 'block-dirty-bitmap-disable': 'BlockDirtyBitmap', diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h index 62682eb865..aa2737f41e 100644 --- a/include/block/dirty-bitmap.h +++ b/include/block/dirty-bitmap.h @@ -83,7 +83,8 @@ void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap); void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy); void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, HBitmap **backup, Error **errp); -void bdrv_dirty_bitmap_set_migration(BdrvDirtyBitmap *bitmap, bool migration); +void bdrv_dirty_bitmap_squelch_persistence(BdrvDirtyBitmap *bitmap, + bool squelch); /* Functions that require manual locking. */ void bdrv_dirty_bitmap_lock(BdrvDirtyBitmap *bitmap); diff --git a/block.c b/block.c index c139540f2b..b934c522eb 100644 --- a/block.c +++ b/block.c @@ -5316,7 +5316,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)) { - bdrv_dirty_bitmap_set_migration(bm, false); + bdrv_dirty_bitmap_squelch_persistence(bm, false); } ret = refresh_total_sectors(bs, bs->total_sectors); diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 95a9c2a5d8..8551f8219e 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -48,10 +48,9 @@ struct BdrvDirtyBitmap { bool inconsistent; /* bitmap is persistent, but inconsistent. It cannot be used at all in any way, except a QMP user can remove it. */ - bool migration; /* Bitmap is selected for migration, it should - not be stored on the next inactivation - (persistent flag doesn't matter until next - invalidation).*/ + bool squelch_persistence; /* We are either migrating or deleting this + * bitmap; it should not be stored on the next + * inactivation. */ QLIST_ENTRY(BdrvDirtyBitmap) list; }; @@ -757,16 +756,17 @@ void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap) } /* Called with BQL taken. */ -void bdrv_dirty_bitmap_set_migration(BdrvDirtyBitmap *bitmap, bool migration) +void bdrv_dirty_bitmap_squelch_persistence(BdrvDirtyBitmap *bitmap, + bool squelch) { qemu_mutex_lock(bitmap->mutex); - bitmap->migration = migration; + bitmap->squelch_persistence = squelch; qemu_mutex_unlock(bitmap->mutex); } bool bdrv_dirty_bitmap_get_persistence(BdrvDirtyBitmap *bitmap) { - return bitmap->persistent && !bitmap->migration; + return bitmap->persistent && !bitmap->squelch_persistence; } bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap) @@ -778,7 +778,7 @@ bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs) { BdrvDirtyBitmap *bm; QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) { - if (bm->persistent && !bm->readonly && !bm->migration) { + if (bm->persistent && !bm->readonly && !bm->squelch_persistence) { return true; } } diff --git a/blockdev.c b/blockdev.c index 01248252ca..4143ab7bbb 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2134,6 +2134,51 @@ static void block_dirty_bitmap_merge_prepare(BlkActionState *common, errp); } +static BdrvDirtyBitmap *do_block_dirty_bitmap_remove( + const char *node, const char *name, bool release, + BlockDriverState **bitmap_bs, Error **errp); + +static void block_dirty_bitmap_remove_prepare(BlkActionState *common, + Error **errp) +{ + BlockDirtyBitmap *action; + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, + common, common); + + if (action_check_completion_mode(common, errp) < 0) { + return; + } + + action = common->action->u.block_dirty_bitmap_remove.data; + + state->bitmap = do_block_dirty_bitmap_remove(action->node, action->name, + false, &state->bs, errp); + if (state->bitmap) { + bdrv_dirty_bitmap_squelch_persistence(state->bitmap, true); + bdrv_dirty_bitmap_set_busy(state->bitmap, true); + } +} + +static void block_dirty_bitmap_remove_abort(BlkActionState *common) +{ + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, + common, common); + + if (state->bitmap) { + bdrv_dirty_bitmap_squelch_persistence(state->bitmap, false); + bdrv_dirty_bitmap_set_busy(state->bitmap, false); + } +} + +static void block_dirty_bitmap_remove_commit(BlkActionState *common) +{ + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, + common, common); + + bdrv_dirty_bitmap_set_busy(state->bitmap, false); + bdrv_release_dirty_bitmap(state->bs, state->bitmap); +} + static void abort_prepare(BlkActionState *common, Error **errp) { error_setg(errp, "Transaction aborted using Abort action"); @@ -2211,6 +2256,12 @@ static const BlkActionOps actions[] = { .commit = block_dirty_bitmap_free_backup, .abort = block_dirty_bitmap_restore, }, + [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_REMOVE] = { + .instance_size = sizeof(BlockDirtyBitmapState), + .prepare = block_dirty_bitmap_remove_prepare, + .commit = block_dirty_bitmap_remove_commit, + .abort = block_dirty_bitmap_remove_abort, + }, /* Where are transactions for MIRROR, COMMIT and STREAM? * Although these blockjobs use transaction callbacks like the backup job, * these jobs do not necessarily adhere to transaction semantics. @@ -2869,20 +2920,21 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name, bdrv_dirty_bitmap_set_persistence(bitmap, persistent); } -void qmp_block_dirty_bitmap_remove(const char *node, const char *name, - Error **errp) +static BdrvDirtyBitmap *do_block_dirty_bitmap_remove( + const char *node, const char *name, bool release, + BlockDriverState **bitmap_bs, Error **errp) { BlockDriverState *bs; BdrvDirtyBitmap *bitmap; bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp); if (!bitmap || !bs) { - return; + return NULL; } if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_BUSY | BDRV_BITMAP_RO, errp)) { - return; + return NULL; } if (bdrv_dirty_bitmap_get_persistence(bitmap)) { @@ -2892,13 +2944,28 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name, aio_context_acquire(aio_context); bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err); aio_context_release(aio_context); + if (local_err != NULL) { error_propagate(errp, local_err); - return; + return NULL; } } - bdrv_release_dirty_bitmap(bs, bitmap); + if (release) { + bdrv_release_dirty_bitmap(bs, bitmap); + } + + if (bitmap_bs) { + *bitmap_bs = bs; + } + + return bitmap; +} + +void qmp_block_dirty_bitmap_remove(const char *node, const char *name, + Error **errp) +{ + do_block_dirty_bitmap_remove(node, name, true, NULL, errp); } /** diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c index 4a896a09eb..c742b706a9 100644 --- a/migration/block-dirty-bitmap.c +++ b/migration/block-dirty-bitmap.c @@ -326,7 +326,7 @@ static int init_dirty_bitmap_migration(void) /* unset migration flags here, to not roll back it */ QSIMPLEQ_FOREACH(dbms, &dirty_bitmap_mig_state.dbms_list, entry) { - bdrv_dirty_bitmap_set_migration(dbms->bitmap, true); + bdrv_dirty_bitmap_squelch_persistence(dbms->bitmap, true); } if (QSIMPLEQ_EMPTY(&dirty_bitmap_mig_state.dbms_list)) {