[v3,2/3] qcow2: handle reopening bitmaps on bdrv_invalidate_cache

Consider migration with shared storage. Persistent bitmaps are stored
on bdrv_inactivate. Then, on destination
process_incoming_migration_bh() calls bdrv_invalidate_cache_all() which
leads to qcow2_load_autoloading_dirty_bitmaps() which fails if bitmaps
are already loaded on destination start. In this case we should call
qcow2_reopen_bitmaps_rw instead.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 block/qcow2.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

On 2018-03-19 10:02, Vladimir Sementsov-Ogievskiy wrote:
> Consider migration with shared storage. Persistent bitmaps are stored
> on bdrv_inactivate. Then, on destination
> process_incoming_migration_bh() calls bdrv_invalidate_cache_all() which
> leads to qcow2_load_autoloading_dirty_bitmaps() which fails if bitmaps
> are already loaded on destination start. In this case we should call
> qcow2_reopen_bitmaps_rw instead.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> ---
>  block/qcow2.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 7472af6931..6219666d4a 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1480,7 +1480,13 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>          s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
>      }
>  
> -    if (qcow2_load_dirty_bitmaps(bs, &local_err)) {
> +    if (bdrv_has_readonly_bitmaps(bs)) {

So if there are any read-only bitmaps, we'll skip
qcow2_load_dirty_bitmaps() altogether.  That doesn't seem so bad because
qcow2_load_dirty_bitmaps() seems to assume no bitmaps have been loaded yet.

But if that's the case, shouldn't we skip that function if any bitmap
has been loaded already, RO or R/W?  (And we can call
qcow2_reopen_bitmaps_rw_hint() even if there aren't any RO bitmaps, it
just won't do anything then.)

This doesn't make this patch really wrong, I'm just wondering whether it
can do better.  (To add to that, I don't even know whether there is a
case where qcow2_do_open() would be called with any R/W bitmaps already
present.)

Max

> +        if (!bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) {
> +            bool header_updated = false;
> +            qcow2_reopen_bitmaps_rw_hint(bs, &header_updated, &local_err);
> +            update_header = update_header && !header_updated;
> +        }
> +    } else if (qcow2_load_dirty_bitmaps(bs, &local_err)) {
>          update_header = false;
>      }
>      if (local_err != NULL) {
>

20.03.2018 16:44, Max Reitz wrote:
> On 2018-03-19 10:02, Vladimir Sementsov-Ogievskiy wrote:
>> Consider migration with shared storage. Persistent bitmaps are stored
>> on bdrv_inactivate. Then, on destination
>> process_incoming_migration_bh() calls bdrv_invalidate_cache_all() which
>> leads to qcow2_load_autoloading_dirty_bitmaps() which fails if bitmaps
>> are already loaded on destination start. In this case we should call
>> qcow2_reopen_bitmaps_rw instead.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: John Snow <jsnow@redhat.com>
>> ---
>>   block/qcow2.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 7472af6931..6219666d4a 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1480,7 +1480,13 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>>           s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
>>       }
>>   
>> -    if (qcow2_load_dirty_bitmaps(bs, &local_err)) {
>> +    if (bdrv_has_readonly_bitmaps(bs)) {
> So if there are any read-only bitmaps, we'll skip
> qcow2_load_dirty_bitmaps() altogether.  That doesn't seem so bad because
> qcow2_load_dirty_bitmaps() seems to assume no bitmaps have been loaded yet.
>
> But if that's the case, shouldn't we skip that function if any bitmap
> has been loaded already, RO or R/W?  (And we can call
> qcow2_reopen_bitmaps_rw_hint() even if there aren't any RO bitmaps, it
> just won't do anything then.)
>
> This doesn't make this patch really wrong, I'm just wondering whether it
> can do better.  (To add to that, I don't even know whether there is a
> case where qcow2_do_open() would be called with any R/W bitmaps already
> present.)
>
> Max

hm reasonable. So, we don't know about such cases, it's better to don't 
allow them.

>
>> +        if (!bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) {
>> +            bool header_updated = false;
>> +            qcow2_reopen_bitmaps_rw_hint(bs, &header_updated, &local_err);
>> +            update_header = update_header && !header_updated;
>> +        }
>> +    } else if (qcow2_load_dirty_bitmaps(bs, &local_err)) {
>>           update_header = false;
>>       }
>>       if (local_err != NULL) {
>>