On 05/14/2018 07:55 AM, 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?
>
I was being imprecise with my language again -- I'm doing it on
qcow2_inactivate, through qcow2_store_persistent_dirty_bitmaps.
I technically don't even need to destroy the cache *then*, I could just
do it unconditionally in qcow2_close.
I just felt it was "safer" to drop the cache after a store as a natural
point of synchronization, but it *should* be fine without.
> Actually, I think we need to remove it only in qcow2_inactive, after
> storing persistent bitmaps.
>
So would you prefer the unconditional drop on close, or a drop for every
inactivate? You still need a drop on close in that case, because we may
not call inactivate on close, and I am still trying to build the cache
for read only images, so to prevent leaks I need to clean that up.
>
>>
>> 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)?
>
Sure, this patch looks a little long.
