[Qemu-devel] migrate -b problems

[Qemu-devel] migrate -b problems

[Kevin Wolf]

Hi all,

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?

The other problem with block migration is that is uses a BlockBackend
name to identify which device is migrated. However, there can be images
that are not attached to any BlockBackend, or if it is, the BlockBackend
might be anonymous, so this doesn't work. I suppose changing the field
to "device name if available, node-name otherwise" would solve this.

Kevin

Re: [Qemu-devel] migrate -b problems

[858585 jemmy]

it this bug?
https://bugs.launchpad.net/qemu/+bug/1681688

On Wed, Apr 12, 2017 at 5:18 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Hi all,
>
> 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?
>
> The other problem with block migration is that is uses a BlockBackend
> name to identify which device is migrated. However, there can be images
> that are not attached to any BlockBackend, or if it is, the BlockBackend
> might be anonymous, so this doesn't work. I suppose changing the field
> to "device name if available, node-name otherwise" would solve this.
>
> Kevin
>

Re: [Qemu-devel] [Qemu-block] migrate -b problems

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 225 bytes --]

On Wed, Apr 12, 2017 at 05:51:23PM +0800, 858585 jemmy wrote:
> it this bug?
> https://bugs.launchpad.net/qemu/+bug/1681688

Yes.  This discussion is about the long-term fix instead of a short-term
hack for QEMU 2.9.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] migrate -b problems

[Juan Quintela]

Kevin Wolf <kwolf@redhat.com> wrote:
> Hi all,

Hi

> 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. :-)

migration/block.c  is a classic.  Anyone that touches it last is the
maintainer.


> 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?

Two answers:

- Easy one (for me).  look at how spice/qxl use migration notifiers (no,
  it is not pretty, but its what is already done).

- More difficult
  I am trying to get an easier to use way migration notifiers.  What we
  need is to be able to schedule:
     - when we start a migration
        * this thing that you need
     - when we finish a migration (either complete, error or cancel)
        * basically to do a free
        * or spice to handle the screen seamlesly to the new client
     - when we are about to start last stage of migration
        * so devices can write things to memory
          Look at new arm GIC (or PIC) or whatever on the list

And probably something more that I haven't yet think about.

> The other problem with block migration is that is uses a BlockBackend
> name to identify which device is migrated. However, there can be images
> that are not attached to any BlockBackend, or if it is, the BlockBackend
> might be anonymous, so this doesn't work. I suppose changing the field
> to "device name if available, node-name otherwise" would solve this.

That is above my knowledge.

Later, Juan.

Re: [Qemu-devel] [Qemu-block] migrate -b problems

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1979 bytes --]

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.

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.

> The other problem with block migration is that is uses a BlockBackend
> name to identify which device is migrated. However, there can be images
> that are not attached to any BlockBackend, or if it is, the BlockBackend
> might be anonymous, so this doesn't work. I suppose changing the field
> to "device name if available, node-name otherwise" would solve this.

Yes, that sounds good and is backwards compatible.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [Qemu-block] migrate -b problems

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 3032 bytes --]

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.

> > The other problem with block migration is that is uses a BlockBackend
> > name to identify which device is migrated. However, there can be images
> > that are not attached to any BlockBackend, or if it is, the BlockBackend
> > might be anonymous, so this doesn't work. I suppose changing the field
> > to "device name if available, node-name otherwise" would solve this.
> 
> Yes, that sounds good and is backwards compatible.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [Qemu-block] migrate -b problems

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2833 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [Qemu-block] migrate -b problems

[Dr. David Alan Gilbert]

* Kevin Wolf (kwolf@redhat.com) 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.


Yes, and while we have the notifier chain for the source on migration state
changes we don't have the notifier on the destination.

If we just add a load_cleanup  member to SaveVMHandlers and call all of them
at the end of an inbound migration would that be enough?
(And define 'end')

Dave

> > > The other problem with block migration is that is uses a BlockBackend
> > > name to identify which device is migrated. However, there can be images
> > > that are not attached to any BlockBackend, or if it is, the BlockBackend
> > > might be anonymous, so this doesn't work. I suppose changing the field
> > > to "device name if available, node-name otherwise" would solve this.
> > 
> > Yes, that sounds good and is backwards compatible.
> 
> Kevin


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [Qemu-block] migrate -b problems

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Kevin Wolf (kwolf@redhat.com) 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.
>
>
> Yes, and while we have the notifier chain for the source on migration state
> changes we don't have the notifier on the destination.
>
> If we just add a load_cleanup  member to SaveVMHandlers and call all of them
> at the end of an inbound migration would that be enough?
> (And define 'end')

We already have a setup() one, that should be enough, no?
We also need a cleanup() one, that is what I am going to add.

Anything else that is needed for this particular problem?

Thanks, Juan.

Re: [Qemu-devel] [Qemu-block] migrate -b problems

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > * Kevin Wolf (kwolf@redhat.com) 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.
> >
> >
> > Yes, and while we have the notifier chain for the source on migration state
> > changes we don't have the notifier on the destination.
> >
> > If we just add a load_cleanup  member to SaveVMHandlers and call all of them
> > at the end of an inbound migration would that be enough?
> > (And define 'end')
> 
> We already have a setup() one, that should be enough, no?
> We also need a cleanup() one, that is what I am going to add.

We need it on the *destination* there's no setup call on the destination is there?

Dave

> Anything else that is needed for this particular problem?
> 
> Thanks, Juan.
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [Qemu-block] migrate -b problems

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
>> > * Kevin Wolf (kwolf@redhat.com) 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.
>> >
>> >
>> > Yes, and while we have the notifier chain for the source on migration state
>> > changes we don't have the notifier on the destination.
>> >
>> > If we just add a load_cleanup  member to SaveVMHandlers and call all of them
>> > at the end of an inbound migration would that be enough?
>> > (And define 'end')
>> 
>> We already have a setup() one, that should be enough, no?
>> We also need a cleanup() one, that is what I am going to add.
>
> We need it on the *destination* there's no setup call on the destination is there?

Ouch.

Ok.  Looking at that then.

Thanks, Juan.

Re: [Qemu-devel] [Qemu-block] migrate -b problems

[Kevin Wolf]

Am 19.04.2017 um 13:13 hat Dr. David Alan Gilbert geschrieben:
> * Kevin Wolf (kwolf@redhat.com) 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.
> 
> Yes, and while we have the notifier chain for the source on migration state
> changes we don't have the notifier on the destination.
> 
> If we just add a load_cleanup  member to SaveVMHandlers and call all of them
> at the end of an inbound migration would that be enough?
> (And define 'end')

Ideally both setup and cleanup, but yes, cleanup is the one that we
definitely need if we don't want hardcoded calls into block migration
code.

As for the place where to call it, I'd say at the very beginning of
process_incoming_migration_bh(), before bdrv_invalidate_cache_all() (and
at the corresponding place for postcopy, because this is duplicated
code).

Kevin