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

[John Snow]

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 <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)?
> 

Sure, this patch looks a little long.