Re: [Qemu-devel] [PATCH v2 03/10] qcow2/bitmap: cache bm_list

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

20.06.2018 16:04, Vladimir Sementsov-Ogievskiy wrote:
> 13.06.2018 05:06, 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.
>>
>> 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<jsnow@redhat.com>
>> ---
>>   block/qcow2-bitmap.c | 74 ++++++++++++++++++++++++++++++++++------------------
>>   block/qcow2.c        |  2 ++
>>   block/qcow2.h        |  2 ++
>>   3 files changed, 52 insertions(+), 26 deletions(-)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 85c1b5afe3..5ae9b17928 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -636,6 +636,34 @@ 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;
>> +    }
>> +
>> +    if (s->nb_bitmaps) {
>> +        bm_list = bitmap_list_load(bs, errp);
>> +    } else {
>> +        bm_list = bitmap_list_new();
>> +    }
>> +    s->bitmap_list = bm_list;
>
> may be, we shouldn't cache it in inactive mode. However, I think we'll 
> finally will not load bitmaps in inactive mode and drop the on 
> inactivate, so it would not matter..

really, now it would be a problem: we can start in inactive mode, load 
nothing, and then we can't reload bitmaps; my fix in
https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg03254.html
will not work after this patch.

>
> Hm, I've understood the following problem: cache becomes incorrect 
> after failed update_ext_header_and_dir or 
> update_ext_header_and_dir_in_place operations. (after failed 
> qcow2_remove_persistent_dirty_bitmap or qcow2_reopen_bitmaps_rw_hint)
>
> And this comes from incomplete architecture after the patch:
> On the one hand, we work with one singleton bitmap_list, and loading 
> part is refactored to do it safely. On the other hand, storing 
> functions still have old behavior, they work with bitmap list like 
> with their own local variable.
>
> So, we have safe mechanism to read list through the cache. We need 
> also safe mechanism to update list both in cache and in file.
>
> There are two possible variants:
>
> 1. drop cache after failed store
> 2. rollback cache after failed store
>
> 1 looks simpler..
>
> Also, we should drop cache on inactivate (we do this) and should not 
> create cache in inactive mode, because the other process may change 
> the image.
>
> Hm. may be, it is better to work with s->bitmap_list directly? In this 
> case it will be more obvious that it is the cache, not local variable. 
> And we will work with it like with other "parts of extension cache" 
> s->nb_bitmaps, s->bitmap_directory_offset ...
>
> After the patch, functions update_ext_header_and_dir* becomes strange:
>
> 1. before the patch, they take external parameter - bm_list, and by 
> this parameter they updated the file and cached s->nb_bitmaps, 
> s->bitmap_*, ..
> 2. after the patch, they take parameter (actually s->bitmap_list) of 
> same nature like s->nb_bitmap, and update s->nb_bitmap from it.
>
> Sorry for being late and for disordered stream of thoughts. Is this 
> patch really needed for the whole series?
>
>> +    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;
>> +    }
>> +}
>> +
>>   int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>>                                     void **refcount_table,
>>                                     int64_t *refcount_table_size)
>> @@ -656,7 +684,7 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>>           return ret;
>>       }
>>   
>> -    bm_list = bitmap_list_load(bs, NULL);
>> +    bm_list = get_bitmap_list(bs, NULL);
>>       if (bm_list == NULL) {
>>           res->corruptions++;
>>           return -EINVAL;
>> @@ -706,8 +734,6 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>>       }
>>   
>>   out:
>> -    bitmap_list_free(bm_list);
>> -
>>       return ret;
>>   }
>>   
>> @@ -944,7 +970,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>>           return false;
>>       }
>>   
>> -    bm_list = bitmap_list_load(bs, errp);
>> +    bm_list = get_bitmap_list(bs, errp);
>>       if (bm_list == NULL) {
>>           return false;
>>       }
>> @@ -978,8 +1004,6 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>>           }
>>       }
>>   
>> -    bitmap_list_free(bm_list);
>> -
>>       return needs_update;
>>   
>>   fail:
>> @@ -988,8 +1012,7 @@ fail:
>>               bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
>>           }
>>       }
>> -    bitmap_list_free(bm_list);
>> -
>> +    del_bitmap_list(bs);
>>       return false;
>>   }
>>   
>> @@ -1016,7 +1039,7 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>>           return -EINVAL;
>>       }
>>   
>> -    bm_list = bitmap_list_load(bs, errp);
>> +    bm_list = get_bitmap_list(bs, errp);
>>       if (bm_list == NULL) {
>>           return -EINVAL;
>>       }
>> @@ -1056,7 +1079,6 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>>   
>>   out:
>>       g_slist_free(ro_dirty_bitmaps);
>> -    bitmap_list_free(bm_list);
>>   
>>       return ret;
>>   }
>> @@ -1265,7 +1287,7 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>>           return;
>>       }
>>   
>> -    bm_list = bitmap_list_load(bs, errp);
>> +    bm_list = get_bitmap_list(bs, errp);
>>       if (bm_list == NULL) {
>>           return;
>>       }
>> @@ -1287,7 +1309,11 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>>   
>>   fail:
>>       bitmap_free(bm);
>> -    bitmap_list_free(bm_list);
>> +}
>> +
>> +void qcow2_persistent_dirty_bitmaps_cache_destroy(BlockDriverState *bs)
>> +{
>> +    del_bitmap_list(bs);
>>   }
>>   
>>   void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>> @@ -1304,23 +1330,19 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>>   
>>       if (!bdrv_has_changed_persistent_bitmaps(bs)) {
>>           /* nothing to do */
>> -        return;
>> +        goto out;
>>       }
>>   
>>       if (!can_write(bs)) {
>>           error_setg(errp, "No write access");
>> -        return;
>> +        goto out;
>>       }
>>   
>>       QSIMPLEQ_INIT(&drop_tables);
>>   
>> -    if (s->nb_bitmaps == 0) {
>> -        bm_list = bitmap_list_new();
>> -    } else {
>> -        bm_list = bitmap_list_load(bs, errp);
>> -        if (bm_list == NULL) {
>> -            return;
>> -        }
>> +    bm_list = get_bitmap_list(bs, errp);
>> +    if (bm_list == NULL) {
>> +        goto out;
>>       }
>>   
>>       /* check constraints and names */
>> @@ -1400,8 +1422,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>>           g_free(tb);
>>       }
>>   
>> -    bitmap_list_free(bm_list);
>> -    return;
>> +    goto out;
>>   
>>   fail:
>>       QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>> @@ -1416,7 +1437,9 @@ fail:
>>           g_free(tb);
>>       }
>>   
>> -    bitmap_list_free(bm_list);
>> + out:
>> +    del_bitmap_list(bs);
>> +    return;
>>   }
>>   
>>   int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
>> @@ -1481,13 +1504,12 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>>           goto fail;
>>       }
>>   
>> -    bm_list = bitmap_list_load(bs, errp);
>> +    bm_list = get_bitmap_list(bs, errp);
>>       if (bm_list == NULL) {
>>           goto fail;
>>       }
>>   
>>       found = find_bitmap_by_name(bm_list, name);
>> -    bitmap_list_free(bm_list);
>>       if (found) {
>>           error_setg(errp, "Bitmap with the same name is already stored");
>>           goto fail;
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 6fa5e1d71a..dbd334b150 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -2148,6 +2148,8 @@ static void qcow2_close(BlockDriverState *bs)
>>   
>>       if (!(s->flags & BDRV_O_INACTIVE)) {
>>           qcow2_inactivate(bs);
>> +    } else {
>> +        qcow2_persistent_dirty_bitmaps_cache_destroy(bs);
>>       }
>>   
>>       cache_clean_timer_del(bs);
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index 01b5250415..29b637a0ee 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -295,6 +295,7 @@ typedef struct BDRVQcow2State {
>>       uint64_t bitmap_directory_size;
>>       uint64_t bitmap_directory_offset;
>>       bool dirty_bitmaps_loaded;
>> +    void *bitmap_list;
>>   
>>       int flags;
>>       int qcow_version;
>> @@ -671,6 +672,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
>>   int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>>                                    Error **errp);
>>   int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
>> +void qcow2_persistent_dirty_bitmaps_cache_destroy(BlockDriverState *bs);
>>   void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
>>   int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
>>   bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>
>
> -- 
> Best regards,
> Vladimir


-- 
Best regards,
Vladimir