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

[Vladimir Sementsov-Ogievskiy]

On 13.02.2015 20:32, John Snow wrote:
> 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.
Ok. Today I've refactored these things, there would be separate start 
and complete frames of bitmap, the first with additional granularity 
field (without any dirty bitmap data) and the second with additional 
enabled field (also without data).

--
Best regards,
Vladimir