Re: [Qemu-block] [Qemu-devel] [PATCH v2 1/4] block/dirty-bitmaps: add inconsistent bit

[Vladimir Sementsov-Ogievskiy]

27.02.2019 21:45, John Snow wrote:
> 
> 
> On 2/25/19 10:30 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 25.02.2019 17:18, Vladimir Sementsov-Ogievskiy wrote:
>>> 23.02.2019 3:22, John Snow wrote:
>>>> Add an inconsistent bit to dirty-bitmaps that allows us to report a bitmap 
>>>> as
>>>> persistent but potentially inconsistent, i.e. if we find bitmaps on a qcow2
>>>> that have been marked as "in use".
>>>>
>>>> Signed-off-by: John Snow <address@hidden>
>>>> ---
>>>>    block/dirty-bitmap.c         | 19 +++++++++++++++++++
>>>>    include/block/dirty-bitmap.h |  2 ++
>>>>    qapi/block-core.json         |  8 ++++++--
>>>>    3 files changed, 27 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>>>> index 86c3b87ab9..9042c04788 100644
>>>> --- a/block/dirty-bitmap.c
>>>> +++ b/block/dirty-bitmap.c
>>>> @@ -46,6 +46,9 @@ struct BdrvDirtyBitmap {
>>>>                                       and this bitmap must remain 
>>>> unchanged while
>>>>                                       this flag is set. */
>>>>        bool persistent;            /* bitmap must be saved to owner disk 
>>>> image */
>>>> +    bool inconsistent;          /* bitmap is persistent, but not owned by 
>>>> QEMU.
>>>> +                                 * 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
>>>> @@ -462,6 +465,8 @@ BlockDirtyInfoList 
>>>> *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
>>>>            info->recording = bdrv_dirty_bitmap_recording(bm);
>>>>            info->busy = bdrv_dirty_bitmap_busy(bm);
>>>>            info->persistent = bm->persistent;
>>>> +        info->has_inconsistent = bm->inconsistent;
>>>> +        info->inconsistent = bm->inconsistent;
>>>>            entry->value = info;
>>>>            *plist = entry;
>>>>            plist = &entry->next;
>>>> @@ -709,6 +714,15 @@ void 
>>>> bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, bool persistent)
>>>>        qemu_mutex_unlock(bitmap->mutex);
>>>>    }
>>>> +/* Called with BQL taken. */
>>>> +void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap)
>>>> +{
>>>> +    qemu_mutex_lock(bitmap->mutex);
>>>> +    bitmap->inconsistent = true;
>>>> +    bitmap->disabled = true;
>>>> +    qemu_mutex_unlock(bitmap->mutex);
>>>> +}
>>>
>>> Didn't you consider separate bdrv_create_inconsistent_bitmap, which will 
>>> not allocate HBitmap?
>>>
>>> It may be done separately ofcourse..
>>>
>>>> +
>>>>    /* Called with BQL taken. */
>>>>    void bdrv_dirty_bitmap_set_migration(BdrvDirtyBitmap *bitmap, bool 
>>>> migration)
>>>>    {
>>>> @@ -722,6 +736,11 @@ bool 
>>>> bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap)
>>>>        return bitmap->persistent && !bitmap->migration;
>>>>    }
>>>> +bool bdrv_dirty_bitmap_inconsistent(BdrvDirtyBitmap *bitmap)
>>>> +{
>>>> +    return bitmap->inconsistent;
>>>> +}
>>>> +
>>>>    bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
>>>>    {
>>>>        BdrvDirtyBitmap *bm;
>>>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>>>> index ba8477b73f..b270773f7e 100644
>>>> --- a/include/block/dirty-bitmap.h
>>>> +++ b/include/block/dirty-bitmap.h
>>>> @@ -68,6 +68,7 @@ void 
>>>> bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
>>>>    void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool 
>>>> value);
>>>>    void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
>>>>                                           bool persistent);
>>>> +void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap);
>>>>    void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy);
>>>>    void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const 
>>>> BdrvDirtyBitmap *src,
>>>>                                 HBitmap **backup, Error **errp);
>>>> @@ -91,6 +92,7 @@ bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap 
>>>> *bitmap);
>>>>    bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
>>>>    bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
>>>>    bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
>>>> +bool bdrv_dirty_bitmap_inconsistent(BdrvDirtyBitmap *bitmap);
>>>>    bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap);
>>>>    bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
>>>>    BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>> index 6e543594b3..a7209fce22 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -470,12 +470,16 @@
>>>>    # @persistent: true if the bitmap will eventually be flushed to 
>>>> persistent
>>>>    #              storage (since 4.0)
>>
>> so, bitmap can't be inconsistent and persistent, as we don't want to flush
>> inconsistent bitmaps...
>>
> 
> I think ideally I'd just change this phrasing to say something like
> "true if the bitmap is stored on-disk, or is scheduled to be flushed to
> disk."

And such wording leads to immediate question: why it could be stored on disk but
_not_ scheduled to be flushed..

So if you want, more honest is something like "true if bitmap will be flushed to
storage or if it is @inconsistent (read ahead)." but it's not looking nice...

May be something like this?

true if bitmap is marked to be flushed to persistent storage. Bitmap may or may 
not
already persist in the storage. Also true if bitmap persist in the storage but
considered inconsistent, in which case it will not be flushed and only may be 
removed,
look at @inconsistent field description.

> 
>>>>    #
>>>> +# @inconsistent: true if this is a persistent bitmap that QEMU does not 
>>>> own.
>>>> +#                Implies @recording and @busy to be false. To reclaim
>>>> +#                ownership, use @block-dirty-bitmap-remove. (since 4.0)
>>>
>>> If we opened an image for rw with in-use bitmaps, the main thing about them 
>>> not
>>> that QEMU doesn't own them, but that they are truly inconsistent. Nobody 
>>> owns them.
>>>
>>> Also, "QEMU does not own" sound like somebody other may own. Then removing 
>>> it don't
>>> seem a correct thing to do.
>>>
>>> And removing don't reclaim ownership, but just remove (looks like it was 
>>> more about
>>> previous version with -clear).
>>>
>>> So for me something like: "Bitmap was not correctly saved after last usage, 
>>> so it
>>> may be inconsistent. It's useless and only take place in a list. The only 
>>> possible
>>> operation on it is remove." seems better.
>>>
> 
> You're right. I was going by memory, but the spec says rather plainly:
> "The bitmap was not saved correctly." Pretty unambiguous.
> 
> I *was* leaving open the window that this bitmap was just simply...
> something QEMU didn't understand or know about, but the spec doesn't
> allow for that, so I will tighten the phrasing.
> 
> I've looked at the counter-proposal now, but I'm not convinced that I
> want to have to worry about the 97 usages of the bitmap field now that
> it can be NULL... Really, we shouldn't be using the allocated bitmap at
> all in these cases -- I agree -- but I worry that this is a premature
> optimization that makes determining the correctness of the code by a
> human reader a little more difficult.
> 
> Ideally we have no bugs, but if we did accidentally use a blank bitmap,
> that's much less of a problem than an assertion or SIGSEGV.

using wrong bitmap for some operation is a potential data corruption, not much
worse than SIGSEGV..

So, we must be sure that we are not using inconsistent bitmap for something 
except
remove. Then, with any way we implement them, we should add assertions into all
low-level bitmap APIs, and accurate error-checking in top-level, to never crash 
on
added asserts.. And SIGSEGV is not worse than assert-failure I think.

Hmm, may be the best way is to keep inconsistent bitmaps in separate list? I'll 
look,
how much is it.

> 
> Am I being too conservative?
> 
>>>> +#
>>>>    # Since: 1.3
>>>>    ##
>>>>    { 'struct': 'BlockDirtyInfo',
>>>>      'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
>>>> -           'recording': 'bool', 'busy': 'bool',
>>>> -           'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }
>>>> +           'recording': 'bool', 'busy': 'bool', 'status': 
>>>> 'DirtyBitmapStatus',
>>>> +           'persistent': 'bool', '*inconsistent': 'bool' } }
>>>>    ##
>>>>    # @Qcow2BitmapInfoFlags:
>>>>
>>>
>>>
>>
>>
> 

Another thing: what about migration? I don't think we are going to teach 
migration protocol
to migrate them. So, migration is a way to get rid of inconsistent bitmaps? Or 
what? Or
we should restrict migration, if there are any inconsistent bitmap, to force 
user to remove
them first?


-- 
Best regards,
Vladimir