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

[John Snow]

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 <address@hidden>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>>  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);
>>  }
>>  
>>  /**
>