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

[Vladimir Sementsov-Ogievskiy]

07.06.2019 21:10, John Snow wrote:
> 
> 
> 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 <address@hidden>
>>> ---
>>>    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.

Personally I like the old _can_add_ :)

But you want to make this function really do something with driver, so "_can_" 
is not good
ofcourse and _add_ is better. And any kind of _mark_persistent_ or 
_make_persistent_ will
be in a bad relation with simple setter _set_persistence() which we already 
have and which
doesn't imply any complicated logic..

On the other hand I still not sure that we need track "queued" bitmaps in qcow2 
driver, as we
can calculate their number and needed size of directory in any moment, not 
extending the qcow2
state..

> 
>>>            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 <address@hidden>
>>
> 
> I will respin anyway, so I will take your suggestions.
> 
> --js
> 


-- 
Best regards,
Vladimir