Re: [Qemu-devel] [PATCH 2/5] block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap

[John Snow <jsnow@redhat.com>]

On 6/7/19 10:29 AM, Vladimir Sementsov-Ogievskiy wrote:
> 06.06.2019 21:41, John Snow wrote:
>> Instead of bdrv_can_store_new_bitmap, rework this as
>> bdrv_add_persistent_dirty_bitmap. This makes a more obvious symmetry
>> with bdrv_remove_persistent_dirty_bitmap. Most importantly, we are free
>> to modify the driver state because we know we ARE adding a bitmap
>> instead of simply being asked if we CAN store one.
>>
>> As part of this symmetry, move this function next to
>> bdrv_remove_persistent_bitmap in block/dirty-bitmap.c.
>>
>> To cement this semantic distinction, use a bitmap object instead of the
>> (name, granularity) pair as this allows us to set persistence as a
>> condition of the "add" succeeding.
>>
>> Notably, the qcow2 implementation still does not actually store the new
>> bitmap at add time, but merely ensures that it will be able to at store
>> time.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   block/qcow2.h                |  5 ++---
>>   include/block/block.h        |  2 --
>>   include/block/block_int.h    |  5 ++---
>>   include/block/dirty-bitmap.h |  3 +++
>>   block.c                      | 22 ---------------------
>>   block/dirty-bitmap.c         | 38 ++++++++++++++++++++++++++++++++++++
>>   block/qcow2-bitmap.c         | 24 ++++++++++++++---------
>>   block/qcow2.c                |  2 +-
>>   blockdev.c                   | 19 +++++++-----------
>>   9 files changed, 68 insertions(+), 52 deletions(-)
>>
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index fc1b0d3c1e..95d723d3c0 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -742,9 +742,8 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
>>   int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
>>   void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
>>   int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
>> -bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>> -                                      const char *name,
>> -                                      uint32_t granularity,
>> +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
>> +                                      BdrvDirtyBitmap *bitmap,
>>                                         Error **errp);
>>   void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>>                                             const char *name,
>> diff --git a/include/block/block.h b/include/block/block.h
>> index f9415ed740..6d520f3b59 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -683,8 +683,6 @@ void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child,
>>                       Error **errp);
>>   void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp);
>>   
>> -bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
>> -                                     uint32_t granularity, Error **errp);
>>   /**
>>    *
>>    * bdrv_register_buf/bdrv_unregister_buf:
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index 06df2bda1b..93bbb66cd0 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -537,9 +537,8 @@ struct BlockDriver {
>>        * field of BlockDirtyBitmap's in case of success.
>>        */
>>       int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp);
>> -    bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs,
>> -                                            const char *name,
>> -                                            uint32_t granularity,
>> +    int (*bdrv_add_persistent_dirty_bitmap)(BlockDriverState *bs,
>> +                                            BdrvDirtyBitmap *bitmap,
>>                                               Error **errp);
>>       void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
>>                                                   const char *name,
>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>> index 8044ace63e..c37edbe05f 100644
>> --- a/include/block/dirty-bitmap.h
>> +++ b/include/block/dirty-bitmap.h
>> @@ -38,6 +38,9 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
>>                               Error **errp);
>>   void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
>>   void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
>> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
>> +                                      BdrvDirtyBitmap *bitmap,
>> +                                      Error **errp);
>>   void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>>                                            const char *name,
>>                                            Error **errp);
>> diff --git a/block.c b/block.c
>> index e3e77feee0..6aec36b7c9 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -6379,25 +6379,3 @@ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp)
>>   
>>       parent_bs->drv->bdrv_del_child(parent_bs, child, errp);
>>   }
>> -
>> -bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
>> -                                     uint32_t granularity, Error **errp)
>> -{
>> -    BlockDriver *drv = bs->drv;
>> -
>> -    if (!drv) {
>> -        error_setg_errno(errp, ENOMEDIUM,
>> -                         "Can't store persistent bitmaps to %s",
>> -                         bdrv_get_device_or_node_name(bs));
>> -        return false;
>> -    }
>> -
>> -    if (!drv->bdrv_can_store_new_dirty_bitmap) {
>> -        error_setg_errno(errp, ENOTSUP,
>> -                         "Can't store persistent bitmaps to %s",
>> -                         bdrv_get_device_or_node_name(bs));
>> -        return false;
>> -    }
>> -
>> -    return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
>> -}
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 49646a30e6..615f8480b2 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -466,6 +466,44 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>>       }
>>   }
>>   
>> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
>> +                                     BdrvDirtyBitmap *bitmap,
>> +                                     Error **errp)
> 
> strange thing for me: "bitmap" in function name is _not_ the same
> thing as @bitmap argument.. as if it the same,
> function adds "persistent bitmap", but @bitmap is not persistent yet,
> so may be function, don't add persistent bitmap, but marks bitmap persistent?
> 
> 
> Ok, I can think of it like "add persistent dirty bitmap to driver. if succeed, set
> bitmap.persistent flag"
> 

Yeah, I meant "Add bitmap to file", where the persistence is implied
because it's part of the file. The bitmap is indeed not YET persistent,
but becomes so as a condition of this call succeeding.

Do you have a suggestion for a better name? I liked the symmetry with
'remove' so that there was an obvious "add bitmap" and "remove bitmap".

Of course, when you remove, it is indeed persistent, so
"remove_persistent_dirty_bitmap" makes sense there.

> 
>> +{
>> +    BlockDriver *drv = bs->drv;
>> +    int ret;
>> +
>> +    if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
>> +        error_setg(errp, "Bitmap '%s' is already persistent, "
>> +                   "and cannot be re-added to node '%s'",
>> +                   bdrv_dirty_bitmap_name(bitmap),
>> +                   bdrv_get_device_or_node_name(bs));
>> +        return -EINVAL;
>> +    }
>> +
>> +    if (!drv) {
>> +        error_setg_errno(errp, ENOMEDIUM,
>> +                         "Can't store persistent bitmaps to %s",
>> +                         bdrv_get_device_or_node_name(bs));
>> +        return -ENOMEDIUM;
>> +    }
>> +
>> +    if (!drv->bdrv_add_persistent_dirty_bitmap) {
>> +        error_setg_errno(errp, ENOTSUP,
>> +                         "Can't store persistent bitmaps to %s",
>> +                         bdrv_get_device_or_node_name(bs));
>> +        return -ENOTSUP;
>> +    }
>> +
>> +    ret = drv->bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp);
>> +    if (ret == 0) {
>> +        bdrv_dirty_bitmap_set_persistence(bitmap, true);
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +
>>   void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>>   {
>>       bdrv_dirty_bitmap_lock(bitmap);
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 83aee1a42a..c3a72ca782 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -1607,14 +1607,16 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
>>       return 0;
>>   }
>>   
>> -bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>> -                                      const char *name,
>> -                                      uint32_t granularity,
>> +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
>> +                                      BdrvDirtyBitmap *bitmap,
>>                                         Error **errp)
>>   {
>>       BDRVQcow2State *s = bs->opaque;
>>       bool found;
>>       Qcow2BitmapList *bm_list;
>> +    const char *name = bdrv_dirty_bitmap_name(bitmap);
>> +    uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
>> +    int ret = 0;
> 
> dead assignment
> 

Will take care of.

>>   
>>       if (s->qcow_version < 3) {
>>           /* Without autoclear_features, we would always have to assume
>> @@ -1623,20 +1625,23 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>>            * have to drop all dirty bitmaps (defeating their purpose).
>>            */
>>           error_setg(errp, "Cannot store dirty bitmaps in qcow2 v2 files");
>> +        ret = -ENOTSUP;
>>           goto fail;
>>       }
>>   
>> -    if (check_constraints_on_bitmap(bs, name, granularity, errp) != 0) {
>> +    ret = check_constraints_on_bitmap(bs, name, granularity, errp);
>> +    if (ret) {
> 
> hmm, in other places you prefere
> if ((ret = ...)) {
> syntax
> 

I have to get rid of those anyway, they violate our coding style. I'll
be replacing them all with this full style.

>>           goto fail;
>>       }
>>   
>>       if (s->nb_bitmaps == 0) {
>> -        return true;
>> +        return 0;
>>       }
>>   
>>       if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) {
>>           error_setg(errp,
>>                      "Maximum number of persistent bitmaps is already reached");
>> +        ret = -EOVERFLOW;
>>           goto fail;
>>       }
>>   
>> @@ -1644,24 +1649,25 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>>           QCOW2_MAX_BITMAP_DIRECTORY_SIZE)
>>       {
>>           error_setg(errp, "Not enough space in the bitmap directory");
>> +        ret = -EOVERFLOW;
>>           goto fail;
>>       }
>>   
>> -    if (bitmap_list_load(bs, &bm_list, errp)) {
>> +    if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
> 
> interesting, now we load all bitmaps, so, may be, we don't need to load list every time,
> but may use bs.dirty_bitmaps to check such things.. But it seems unsafe
> 

Yeah, I would like to cache this list eventually, but I ran into a lot
of hassle with it last time I tried and put it off for now.

I think we should definitely be able to.

And in fact, if we did, we could even add these bitmaps to the bm_list
as soon as _add_ is called and drop the need for the queued counters.

>>           goto fail;
>>       }
>>   
>>       found = find_bitmap_by_name(bm_list, name);
>>       bitmap_list_free(bm_list);
>>       if (found) {
>> +        ret = -EEXIST;
>>           error_setg(errp, "Bitmap with the same name is already stored");
>>           goto fail;
>>       }
>>   
>> -    return true;
>> -
>> +    return 0;
>>   fail:
>>       error_prepend(errp, "Can't make bitmap '%s' persistent in '%s': ",
>>                     name, bdrv_get_device_or_node_name(bs));
> 
> didn't you consider to move this error_prepend to caller and drop all these gotos?
> 

Nope! But I'll do that.

>> -    return false;
>> +    return ret;
>>   }
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 9396d490d5..1c78eba71b 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -5229,7 +5229,7 @@ BlockDriver bdrv_qcow2 = {
>>       .bdrv_attach_aio_context  = qcow2_attach_aio_context,
>>   
>>       .bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw,
>> -    .bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap,
>> +    .bdrv_add_persistent_dirty_bitmap = qcow2_add_persistent_dirty_bitmap,
>>       .bdrv_remove_persistent_dirty_bitmap = qcow2_remove_persistent_dirty_bitmap,
>>   };
>>   
>> diff --git a/blockdev.c b/blockdev.c
>> index 3f44b891eb..169a8da831 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -2851,26 +2851,21 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
>>           disabled = false;
>>       }
>>   
>> -    if (persistent) {
>> -        aio_context = bdrv_get_aio_context(bs);
>> -        aio_context_acquire(aio_context);
>> -        if (!bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp)) {
>> -            goto out;
>> -        }
>> -    }
>> -
>>       bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
>>       if (bitmap == NULL) {
>> -        goto out;
>> +        return;
>>       }
>>   
>>       if (disabled) {
>>           bdrv_disable_dirty_bitmap(bitmap);
>>       }
>>   
>> -    bdrv_dirty_bitmap_set_persistence(bitmap, persistent);
>> - out:
>> -    if (aio_context) {
>> +    if (persistent) {
> 
> Good thing. I suggest to define aio_context in this block too.
> 

Oh, sure.

>> +        aio_context = bdrv_get_aio_context(bs);
>> +        aio_context_acquire(aio_context);
>> +        if (bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp)) {
>> +            bdrv_release_dirty_bitmap(bs, bitmap);
>> +        }
>>           aio_context_release(aio_context);
>>       }
>>   }
>>
> 
> With some my suggestions or without:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 

I will respin anyway, so I will take your suggestions.

--js