Re: [PATCH] vfio/migration: Skip log_sync during migration SETUP state

[Avihai Horon]

On 04/04/2023 0:36, Alex Williamson wrote:
> External email: Use caution opening links or attachments
>
>
> On Mon, 3 Apr 2023 22:36:42 +0200
> Cédric Le Goater <clg@redhat.com> wrote:
>
> > On 4/3/23 15:00, Avihai Horon wrote:
> > > Currently, VFIO log_sync can be issued while migration is in SETUP
> > > state. However, doing this log_sync is at best redundant and at worst
> > > can fail.
> > >
> > > Redundant -- all RAM is marked dirty in migration SETUP state and is
> > > transferred only after migration is set to ACTIVE state, so doing
> > > log_sync during migration SETUP is pointless.
> > >
> > > Can fail -- there is a time window, between setting migration state to
> > > SETUP and starting dirty tracking by RAM save_live_setup handler, during
> > > which dirty tracking is still not started. Any VFIO log_sync call that
> > > is issued during this time window will fail. For example, this error can
> > > be triggered by migrating a VM when a GUI is active, which constantly
> > > calls log_sync.
> > >
> > > Fix it by skipping VFIO log_sync while migration is in SETUP state.
> > >
> > > Fixes: 758b96b61d5c ("vfio/migrate: Move switch of dirty tracking into 
> > > vfio_memory_listener")
> > > Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> > migration is still experimental, so this can wait 8.1. Correct me if not.
> Agreed, this doesn't seem nearly catastrophic enough as an experimental
> feature that it can't wait for the 8.1 devel cycle to open.

Sure, so let's wait for 8.1 cycle to open.

Thanks!

> > > ---
> > >    hw/vfio/common.c | 3 ++-
> > >    1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > > index 4d01ea3515..78358ede27 100644
> > > --- a/hw/vfio/common.c
> > > +++ b/hw/vfio/common.c
> > > @@ -478,7 +478,8 @@ static bool vfio_devices_all_dirty_tracking(VFIOContainer 
> > > *container)
> > >        VFIODevice *vbasedev;
> > >        MigrationState *ms = migrate_get_current();
> > >
> > > -    if (!migration_is_setup_or_active(ms->state)) {
> > > +    if (ms->state != MIGRATION_STATUS_ACTIVE &&
> > > +        ms->state != MIGRATION_STATUS_DEVICE) {
> > >            return false;
> > >        }