Re: [PATCH v2] migration: Handle block device inactivation failures better

[Eric Blake]

On Thu, Apr 20, 2023 at 12:41:25PM +0200, Juan Quintela wrote:
> Eric Blake <eblake@redhat.com> wrote:

...lots of lines...

> > ---
> >  migration/migration.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)

...describing a tiny change ;)

> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index bda47891933..cb0d42c0610 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -3444,13 +3444,11 @@ static void migration_completion(MigrationState *s)
> >                                              MIGRATION_STATUS_DEVICE);
> >              }
> >              if (ret >= 0) {
> > +                s->block_inactive = inactivate;
> >                  qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX);
> >                  ret = qemu_savevm_state_complete_precopy(s->to_dst_file, 
> > false,
> >                                                           inactivate);
> >              }
> > -            if (inactivate && ret >= 0) {
> > -                s->block_inactive = true;
> > -            }
> >          }
> >          qemu_mutex_unlock_iothread();
> 
> And I still have to look at the file to understand this "simple" patch.
> (simple in size, not in what it means).

Indeed - hence the long commit message!

> 
> I will add this to my queue, but if you are in the "mood", I would like
> to remove the declaration of inactivate and change this to something like:
> 
>              if (ret >= 0) {
>                  /* Colo don't stop disks in normal operation */
>                  s->block_inactive = !migrate_colo_enabled();
>                  qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX);
>                  ret = qemu_savevm_state_complete_precopy(s->to_dst_file, 
> false,
>                                                           s->block_inactive);
>              }
> 
> Or something around that lines?

Yes, that looks like a trivial refactoring that preserves the same
semantics.

> 
> > @@ -3522,6 +3520,7 @@ fail_invalidate:
> >          bdrv_activate_all(&local_err);
> >          if (local_err) {
> >              error_report_err(local_err);
> > +            s->block_inactive = true;
> >          } else {
> >              s->block_inactive = false;
> >          }
> > base-commit: 7dbd6f8a27e30fe14adb3d5869097cddf24038d6
> 
> Just wondering, what git magic creates this line?

git send-email --base=COMMIT_ID

or even 'git config format.useAutoBase whenAble' to try and automate
the use of this.  (If my own git habits were cleaner, of always
sticking patches in fresh branches, --base=auto is handy; but in
practice, I tend to send one-off patches like this in the middle of
'git rebase' of a larger series, at which point I'm not on a clean
branch where --base=auto works, so I end up having to manually specify
one at the command line.  Either way, including the base-commit: info
can be very informative for applying a patch at the branch point then
rebasing it locally, when attempting to apply the patch sent through
email hits merge conflicts when applying it directly to changes on
master in the meantime; I believe 'git am -3' is even able to exploit
the comment when present to make smarter decisions about which parent
commit it tries for doing 3-way patch resolution)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org