On Tue, Apr 18, 2017 at 05:32:56PM +0200, Kevin Wolf wrote:
> Am 18.04.2017 um 16:47 hat Stefan Hajnoczi geschrieben:
> > On Wed, Apr 12, 2017 at 11:18:19AM +0200, Kevin Wolf wrote:
> > > after getting assertion failure reports for block migration in the last
> > > minute, we just hacked around it by commenting out op blocker assertions
> > > for the 2.9 release, but now we need to see how to fix things properly.
> > > Luckily, get_maintainer.pl doesn't report me, but only you. :-)
> > > 
> > > The main problem I see with the block migration code (on the
> > > destination) is that it abuses the BlockBackend that belongs to the
> > > guest device to make its own writes to the image file. If the guest
> > > isn't allowed to write to the image (which it now isn't during incoming
> > > migration since it would conflict with the newer style of block
> > > migration using an NBD server), writing to this BlockBackend doesn't
> > > work any more.
> > > 
> > > So what should really happen is that incoming block migration creates
> > > its own BlockBackend for writing to the image. Now we don't want to do
> > > this anew for every incoming block, but ideally we'd just create all
> > > necessary BlockBackends upfront and then keep using them throughout the
> > > whole migration. Is there a way to get some setup/teardown callbacks
> > > at the start/end of the migration that could initialise and free such
> > > global data?
> > 
> > It can be done in the beginning of block_load() similar to
> > block_mig_state.bmds_list, which is created in init_blk_migration() at
> > save time.
> 
> The difference is that block_load() is the counterpart for
> block_save_iterate(), not for init_blk_migration(). That is, it is
> called for each chunk of block migration data, which is interleaved with
> normal RAM migration chunks.
> 
> So we can either create each BlockBackend the first time we need it in
> block_load(), or create BlockBackends for all existing device BBs and
> BDSes the first time block_load() is called. We still need some place
> to actually free the BlockBackends again when the migration completes.
> 
> Dave suggested migration state notifiers, which looked like an option,
> but at least the existing migration states aren't enough, because the
> BlockBackends need to go away before blk_resume_after_migration() is
> called, but MIGRATION_STATUS_COMPLETED is set only afterwards.
> 
> > We can also move the if (blk != blk_prev) blk_invalidate_cache() code
> > out of the load loop.  It should be done once when setting up
> > BlockBackends.
> 
> Same problem as above, while saving has setup/cleanup callbacks, we only
> have the iterate callback for loading.

I see what you are saying.  setup/cleanups need to be added to
SaveVMHandlers.

Stefan