Re: [Qemu-block] [Qemu-devel] [PATCH 2/4] block/dirty-bitmap: add hide/unhide API

[Vladimir Sementsov-Ogievskiy]

10.06.2019 12:33, Vladimir Sementsov-Ogievskiy wrote:
> 08.06.2019 1:39, John Snow wrote:
>>
>>
>> On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Add functionality to make bitmap temporary anonymous. It will be used
>>> to implement bitmap remove transaction action. We need hide bitmap
>>> persistence too, as there are should not be unnamed persistent bitmaps.
>>>
>>
>> Ah, so this effectively ... "hides" a bitmap from any further
>> transaction actions. It also "hides" it from getting flushed to disk...
>> sort of?
>>
>> The outer loop in store works with bdrv_dirty_bitmap_next, and we'll
>> skip this bitmap because it's anonymous/not persistent.
>>
>> There's a second loop where we iterate bm_list, and we'll skip storing
>> this bitmap because that entry won't have an in-memory bitmap associated
>> with it in bm_list.
>>
>> ...But then we'll call update_ext_header_and_dir with the stale entries
>> in bm_list?
> 
> Hidden is a temprory state, so, we should not go to close() in this state.
> It's a state inside a transaction.
> 
> Stale entries in list will be marked IN_USE anyway, so there is no damage 
> anyway.

I'm wrong, storing inside transaction is exactly what test does.. Hmm, so, 
seems you
are right, we are storing stale IN_USE bitmaps. No serious damage, but bad 
design:
creating inconsistent bitmaps in each snapshot. I need to fix it in v2 somehow.

> 
>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>> ---
>>>   include/block/dirty-bitmap.h |  2 ++
>>>   block/dirty-bitmap.c         | 26 ++++++++++++++++++++++++++
>>>   2 files changed, 28 insertions(+)
>>>
>>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>>> index 8044ace63e..542e437123 100644
>>> --- a/include/block/dirty-bitmap.h
>>> +++ b/include/block/dirty-bitmap.h
>>> @@ -116,5 +116,7 @@ bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap 
>>> *bitmap,
>>>   BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
>>>                                                     BdrvDirtyBitmap *bitmap,
>>>                                                     Error **errp);
>>> +void bdrv_dirty_bitmap_hide(BdrvDirtyBitmap *bitmap);
>>> +void bdrv_dirty_bitmap_unhide(BdrvDirtyBitmap *bitmap);
>>>   #endif
>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>>> index 49646a30e6..592964635e 100644
>>> --- a/block/dirty-bitmap.c
>>> +++ b/block/dirty-bitmap.c
>>> @@ -35,6 +35,10 @@ struct BdrvDirtyBitmap {
>>>       bool busy;                  /* Bitmap is busy, it can't be used via 
>>> QMP */
>>>       BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
>>>       char *name;                 /* Optional non-empty unique ID */
>>> +    char *hidden_name;          /* Backup of @name for removal transaction
>>> +                                   action. Used for hide/unhide API. */
>>> +    bool hidden_persistent;     /* Backup of @persistent for removal 
>>> transaction
>>> +                                   action. */
>>>       int64_t size;               /* Size of the bitmap, in bytes */
>>>       bool disabled;              /* Bitmap is disabled. It ignores all 
>>> writes to
>>>                                      the device */
>>> @@ -849,3 +853,25 @@ out:
>>>           qemu_mutex_unlock(src->mutex);
>>>       }
>>>   }
>>> +
>>> +void bdrv_dirty_bitmap_hide(BdrvDirtyBitmap *bitmap)
>>> +{
>>> +    qemu_mutex_lock(bitmap->mutex);
>>> +    assert(!bitmap->hidden_name);
>>> +    bitmap->hidden_name = bitmap->name;
>>> +    bitmap->hidden_persistent = bitmap->persistent;
>>> +    bitmap->name = NULL;
>>> +    bitmap->persistent = false;
>>> +    qemu_mutex_unlock(bitmap->mutex);
>>> +}
>>> +
>>> +void bdrv_dirty_bitmap_unhide(BdrvDirtyBitmap *bitmap)
>>> +{
>>> +    qemu_mutex_lock(bitmap->mutex);
>>> +    assert(!bitmap->name);
>>> +    bitmap->name = bitmap->hidden_name;
>>> +    bitmap->persistent = bitmap->hidden_persistent;
>>> +    bitmap->hidden_name = NULL;
>>> +    bitmap->hidden_persistent = false;
>>> +    qemu_mutex_unlock(bitmap->mutex);
>>> +}
>>>
> 
> 


-- 
Best regards,
Vladimir