[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 2/3] qapi: implement block-dirty-bitmap-remov
From: |
John Snow |
Subject: |
Re: [Qemu-devel] [PATCH v3 2/3] qapi: implement block-dirty-bitmap-remove transaction action |
Date: |
Wed, 24 Jul 2019 12:13:50 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.2 |
On 7/24/19 9:58 AM, Vladimir Sementsov-Ogievskiy wrote:
> 09.07.2019 1:05, 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 <address@hidden>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>> block.c | 2 +-
>> block/dirty-bitmap.c | 15 +++----
>> blockdev.c | 79 +++++++++++++++++++++++++++++++---
>> include/block/dirty-bitmap.h | 2 +-
>> migration/block-dirty-bitmap.c | 2 +-
>> qapi/transaction.json | 2 +
>> 6 files changed, 85 insertions(+), 17 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index c139540f2b..5195d4b910 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_skip_store(bm, false);
>> }
>>
>> ret = refresh_total_sectors(bs, bs->total_sectors);
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 95a9c2a5d8..a308e1f84b 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 skip_store; /* We are either migrating or deleting this
>> + * bitmap; it should not be stored on the
>> next
>> + * inactivation. */
>> QLIST_ENTRY(BdrvDirtyBitmap) list;
>> };
>>
>> @@ -757,16 +756,16 @@ 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_skip_store(BdrvDirtyBitmap *bitmap, bool skip)
>> {
>> qemu_mutex_lock(bitmap->mutex);
>> - bitmap->migration = migration;
>> + bitmap->skip_store = skip;
>> qemu_mutex_unlock(bitmap->mutex);
>> }
>>
>> bool bdrv_dirty_bitmap_get_persistence(BdrvDirtyBitmap *bitmap)
>> {
>> - return bitmap->persistent && !bitmap->migration;
>> + return bitmap->persistent && !bitmap->skip_store;
>> }
>>
>> bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap)
>> @@ -778,7 +777,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->skip_store) {
>> return true;
>> }
>> }
>> diff --git a/blockdev.c b/blockdev.c
>> index 01248252ca..800b3dcb42 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_skip_store(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) {
>
> Hmm, interesting, I thought, abort should not be called, if prepare failed,
> so the
> following may be done unconditionally?
>
I don't think so:
qmp_transaction:
...
state->ops->prepare(state, &local_err);
if (local_err) {
error_propagate(errp, local_err);
goto delete_and_fail;
}
...
delete_and_fail:
...
QTAILQ_FOREACH_REVERSE(state, &snap_bdrv_states, entry) {
if (state->ops->abort) {
state->ops->abort(state);
}
}
>> + bdrv_dirty_bitmap_skip_store(state->bitmap, false);
>> + bdrv_dirty_bitmap_set_busy(state->bitmap, false);
>> + }
>> +}
>> +
>
> [..]
>
> OK, I agree, good idea to reuse BUSY and migration field here and avoid new
> API.
> Now release-prepare is less "release", but I don't see any real problems with
> it.
> Maybe, we need something to be noted in docs.
>
> Hmm, as we are not in a hurry now, we may wait for Nikolay to check this on
> his
> scenarios.
>
OK, you have time. These patches are all sitting in my bitmaps branch
and I haven't sent the PR for them yet.
--js