Re: [Qemu-devel] [PATCH v4 for 2.12 0/3] fix bitmaps migration through shared storage

[Max Reitz]

On 2018-03-29 10:08, Vladimir Sementsov-Ogievskiy wrote:
> 28.03.2018 17:53, Max Reitz wrote:
>> On 2018-03-27 12:11, Vladimir Sementsov-Ogievskiy wrote:

[...]

>>> isn't it because a lot of cat processes? will check, update loop to
>>> i=0; while check -qcow2 169; do ((i++)); echo $i OK; killall -9 cat;
>>> done
>> Hmm...  I know I tried to kill all of the cats, but for some reason that
>> didn't really help yesterday.  Seems to help now, for 2.12.0-rc0 at
>> least (that is, before this series).
> 
> reproduced with killing... (without these series, just on master)
> 
>>
>> After the whole series, I still get a lot of failures in 169
>> (mismatching bitmap hash, mostly).
>>
>> And interestingly, if I add an abort():
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 486f3e83b7..9204c1c0ac 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1481,6 +1481,7 @@ static int coroutine_fn
>> qcow2_do_open(BlockDriverState *bs, QDict *options,     }
>>
>>       if (bdrv_dirty_bitmap_next(bs, NULL)) {
>> +        abort();
>>           /* It's some kind of reopen with already existing dirty
>> bitmaps. There
>>            * are no known cases where we need loading bitmaps in such
>> situation,
>>            * so it's safer don't load them.
>>
>> Then this fires for a couple of test cases of 169 even without the third
>> patch of this series.
>>
>> I guess bdrv_dirty_bitmap_next() reacts to some bitmaps that migration
>> adds or something?  Then this would be the wrong condition, because I
>> guess we still want to load the bitmaps that are in the qcow2 file.
>>
>> I'm not sure whether bdrv_has_readonly_bitmaps() is the correct
>> condition then, either, though.  Maybe let's take a step back: We want
>> to load all the bitmaps from the file exactly once, and that is when it
>> is opened the first time.  Or that's what I would have thought...  Is
>> that even correct?
>>
>> Why do we load the bitmaps when the device is inactive anyway?
>> Shouldn't we load them only once the device is activated?
> 
> Hmm, not sure. May be, we don't need. But we anyway need to load them,
> when opening read-only, and we should correspondingly reopen in this case.

Yeah, well, yeah, but the current state seems just wrong.  Apparently
there are cases where a qcow2 node may have bitmaps before we try to
load them from the file, so the current condition doesn't work.

Furthermore, it seems like the current "state machine" is too complex so
we don't know which cases are possible anymore and what to do when.

So the first thing we could do is add a field to the BDRVQCow2State that
tells us whether the bitmaps have been loaded already or not.  If not,
we invoke qcow2_load_dirty_bitmaps() and set the value to true.  If the
value was true already and the BDS is active and R/W now, we call
qcow2_reopen_bitmaps_rw_hint().  That should solve one problem.

The other problem of course is the question whether we should call
qcow2_load_dirty_bitmaps() at all while the drive is still inactive.
You know the migration model better than me, so I'm asking this question
to you.  We can phrase it differently: Do we need to load the bitmaps
before the drive is activated?

Max

signature.asc
Description: OpenPGP digital signature