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 <js...@redhat.com>
>>> ---
>>> 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.
