Re: [Qemu-devel] [PATCH v3 2/3] qapi: implement block-dirty-bitmap-remove transaction action

[John Snow]

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