Re: [Qemu-block] [Qemu-devel] [RFC PATCH 01/12] qcow2-bitmap: cache bm_list

[John Snow]

On 05/14/2018 08:15 AM, Vladimir Sementsov-Ogievskiy wrote:
> 14.05.2018 14:55, Vladimir Sementsov-Ogievskiy wrote:
>> 12.05.2018 04:25, John Snow wrote:
>>> We don't need to re-read this list every time, exactly. We can keep
>>> it cached
>>> and delete our copy when we flush to disk.
>>
>> Why not simply delete cache only on close (unconditionally)? Why do we
>> need to remove it after flush?
>>
>> Actually, I think we need to remove it only in qcow2_inactive, after
>> storing persistent bitmaps.
>>
>>
>>>
>>> Because we don't try to flush bitmaps on close if there's nothing to
>>> flush,
>>> add a new conditional to delete the state anyway for a clean exit.
>>>
>>> Signed-off-by: John Snow <address@hidden>
>>> ---
>>>   block/qcow2-bitmap.c | 74
>>> ++++++++++++++++++++++++++++++++--------------------
>>>   block/qcow2.c        |  2 ++
>>>   block/qcow2.h        |  2 ++
>>>   3 files changed, 50 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>> index 6e93ec43e1..fb0a4f3ec4 100644
>>> --- a/block/qcow2-bitmap.c
>>> +++ b/block/qcow2-bitmap.c
>>> @@ -536,8 +536,7 @@ static uint32_t bitmap_list_count(Qcow2BitmapList
>>> *bm_list)
>>>    * Get bitmap list from qcow2 image. Actually reads bitmap directory,
>>>    * checks it and convert to bitmap list.
>>>    */
>>> -static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs,
>>> uint64_t offset,
>>> -                                         uint64_t size, Error **errp)
>>> +static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, Error
>>> **errp)
>>>   {
>>>       int ret;
>>>       BDRVQcow2State *s = bs->opaque;
>>> @@ -545,6 +544,8 @@ static Qcow2BitmapList
>>> *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
>>>       Qcow2BitmapDirEntry *e;
>>>       uint32_t nb_dir_entries = 0;
>>>       Qcow2BitmapList *bm_list = NULL;
>>> +    uint64_t offset = s->bitmap_directory_offset;
>>> +    uint64_t size = s->bitmap_directory_size;
>>
>> Worth split this change as a refactoring patch (just remove extra
>> parameters)?
>>
>>>         if (size == 0) {
>>>           error_setg(errp, "Requested bitmap directory size is zero");
>>> @@ -636,6 +637,30 @@ fail:
>>>       return NULL;
>>>   }
>>>   +static Qcow2BitmapList *get_bitmap_list(BlockDriverState *bs,
>>> Error **errp)
>>> +{
>>> +    BDRVQcow2State *s = bs->opaque;
>>> +    Qcow2BitmapList *bm_list;
>>> +
>>> +    if (s->bitmap_list) {
>>> +        return (Qcow2BitmapList *)s->bitmap_list;
>>> +    }
>>> +
>>> +    bm_list = bitmap_list_load(bs, errp);
>>> +    s->bitmap_list = bm_list;
>>> +    return bm_list;
>>> +}
>>> +
>>> +static void del_bitmap_list(BlockDriverState *bs)
>>> +{
>>> +    BDRVQcow2State *s = bs->opaque;
>>> +
>>> +    if (s->bitmap_list) {
>>> +        bitmap_list_free(s->bitmap_list);
>>> +        s->bitmap_list = NULL;
>>> +    }
>>> +}
> 
> so, with this functions, we see, that list is always safely loaded
> through the cache. But we need also guarantee, that list is always saved
> through the cache. There are a lot of functions, which stores an
> abstract bitmap list, given as a parameter, but we want always store our
> cache..
> 

Do you have an example?

These functions return a bitmap list:

bitmap_list_new()
bitmap_list_load(...);
get_bitmap_list(...);


bitmap_list_new creates a new bitmap_list, it's called by
bitmap_list_load and qcow2_store_persistent_dirty_bitmaps.

qcow2_store_persistent_dirty_bitmaps doesn't matter right now because we
drop the cache by the end of that function.

bitmap_list_load is only called by get_bitmap_list now, and
get_bitmap_list caches that bitmap list as s->bitmap_list.


I think any other function that takes a bitmap list must be getting it
from a location where it's cached -- except in store, which trashes it
anyway.

What I can do here is to make get_bitmap_list create a new bitmap list
if s->nb_bitmaps is zero, and remove the new call in store -- then we
can be really very confident that any bitmap list getting passed around
is definitely the cached one.