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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4063 bytes --]

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);
>>  }
>>  
>>  /**
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]