Re: [Qemu-devel] [PATCH RFC v2 8/8] migration: add migration/dirty-bitmap.c

[John Snow <jsnow@redhat.com>]

On 02/13/2015 04:06 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>
>>>> +
>>>> +    blk_mig_reset_dirty_cursor();
>>>> +    dirty_phase(f, false);
>>>> +
>>>> +    QSIMPLEQ_FOREACH(dbms, &dirty_bitmap_mig_state.dbms_list, entry) {
>>>> +        uint8_t flags = DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME |
>>>> +                        DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME |
>>>> +                        DIRTY_BITMAP_MIG_FLAG_ENABLED;
>>>> +
>>>> +        qemu_put_byte(f, flags);
>>>> +        qemu_put_name(f, bdrv_get_device_name(dbms->bs));
>>>> +        qemu_put_name(f, bdrv_dirty_bitmap_name(dbms->bitmap));
>>>> +        qemu_put_byte(f, bdrv_dirty_bitmap_enabled(dbms->bitmap));
>>>> +    }
>>>> +
>>>> +    qemu_put_byte(f, DIRTY_BITMAP_MIG_FLAG_EOS);
>>>> +
>>>> +    DPRINTF("Dirty bitmaps migration completed\n");
>>>> +
>>>> +    dirty_bitmap_mig_cleanup();
>>>> +    return 0;
>>>> +}
>>>> +
>>>
>>> I suppose we don't need a flag that distinctly SAYS this is the end
>>> section, since we can tell by omission of
>>> DIRTY_BITMAP_MIG_FLAG_NORMAL_CHUNK or ZERO_CHUNK.
>> Hmm. I think it simplifies the logic (to use EOS after each section).
>> And the same approach is in migration/block.c.. It's a question about
>> which format is better:  "Each section for dirty_bitmap_load ends with
>> EOS" or "Each section for dirty_bitmap_load ends with EOS except the
>> last one. The last one may be recognized by absent NORMAL_CHUNK and
>> ZERO_CHUNK"
>
> Oh, sorry, no, it's important EOS. There are several blocks with no
> *_CHUNK! Several bitmaps. And loop in dirty_bitmap_load will read them
> iteratively, and it will finish when find EOS.
>
>

Sorry, I worded that poorly. I was wondering why you didn't have an 
explicit "end of bitmap" flag, and I realized that you are doing this 
check essentially by the absence of the NORMAL_CHUNK/ZERO_CHUNK flags.

This is really just a comment on my part; I was expecting a more 
distinct "It is now safe to rebuild the bitmap" flag and was just 
commenting on why we didn't necessarily need one.

I think in another comment I point out that an "end of bitmap" flag 
might make the loading function simpler, and I still think that might be 
nice.