Re: [PATCH v2] block/file-posix: Optimize for macOS

[Kevin Wolf]

Am 09.03.2021 um 11:26 hat Daniel P. Berrangé geschrieben:
> On Tue, Mar 09, 2021 at 10:37:18AM +0100, Kevin Wolf wrote:
> > Am 09.03.2021 um 05:52 hat Akihiko Odaki geschrieben:
> > > 2021年3月9日(火) 0:37 Akihiko Odaki <akihiko.odaki@gmail.com>:
> > > >
> > > > 2021年3月9日(火) 0:17 Stefan Hajnoczi <stefanha@redhat.com>:
> > > > >
> > > > > The live migration compatibility issue is still present. Migrating to
> > > > > another host might not work if the block limits are different.
> > > > >
> > > > > Here is an idea for solving it:
> > > > >
> > > > > Modify include/hw/block/block.h:DEFINE_BLOCK_PROPERTIES_BASE() to
> > > > > support a new value called "host". The default behavior remains
> > > > > unchanged for live migration compatibility but now you can use "host" 
> > > > > if
> > > > > you know it's okay but don't care about migration compatibility.
> > > > >
> > > > > The downside to this approach is that users must explicitly say
> > > > > something like --drive ...,opt_io_size=host. But it's still better 
> > > > > than
> > > > > the situation we have today where user must manually enter values for
> > > > > their disk.
> > > > >
> > > > > Does this sound okay to everyone?
> > > > >
> > > > > Stefan
> > > >
> > > > I wonder how that change affects other block drivers implementing
> > > > bdrv_probe_blocksizes. As far as I know, the values they report are
> > > > already used by default, which is contrary to the default not being
> > > > "host".
> > > >
> > > > Regards,
> > > > Akihiko Odaki
> > > 
> > > Let me suggest a variant of Stefan's approach:
> > > 
> > > Modify include/hw/block/block.h:DEFINE_BLOCK_PROPERTIES_BASE() to
> > > support a new value called "host". The default values for block size
> > > properties may be "host" or not, but they should be consistent. If
> > > they are "host" by default
> > 
> > I'm not sure if it's a good idea, but maybe we could make it so that the
> > default is "host" only as long as you didn't specify -nodefaults? Then
> > libvirt would automatically keep the old behaviour (because it always
> > sets -nodefaults) and manual invocations would usually get the new one.
> > 
> > Of course, when I start with "I'm not sure if it's a good idea", it's
> > usually not, but I wanted to share the thought anyway...
> 
> Can you elaborate on what the actual live migration problem is, and
> its impact ?  This patch is touching the block backends, so I'm
> wondering how backend data ends up having an impact on the migration
> stream which is all frontend device data ?  I'm especially concerned
> by the mention that some block backends already have this problem,
> and wondering if it already impacts libvirt ?

The part that modifies the backend is the boring part (I haven't even
looked at the code for that one yet).

The interesting part is the change to hw/block/block.c, which modifies
the defaults for some qdev properties of block devices. The reason why
it does so is that it wants to let them default to autodetecting
whatever is optimal on the host.

The potential conflict between autodetecting qdev property defaults in
the backend and live migration should be obvious.

> Using -nodefaults is good practice, but I'm still uncomfortable saying
> that its use is a requirement if you want migration to work, as that
> feels like a change in semantics for non-libvirt users (who can be
> mgmt apps, nor merely human interactive users).

We can make live migration work in a way, by including these properties
in the VM state and then overriding whatever was set on the command line
with the values from the VM state. (This patch doesn't do this yet, nor
does it disable live migration, but it just lets the device magically
change its properties during migration, which is incorrect.)

Of course, this would still mean that the old value is only preserved on
the destination host as long as the QEMU instance runs. On the next
boot, the guest visible disk will change.

So if you want stable guest devices, you can't really have any of this
autodetection. If you set the properties explicitly instead of relying
on the defaults (which is what you should be doing ideally), you don't
have the problem, but I'm not sure if we can expect that users are
actually doing that. Considering -nodefaults would be just another
attempt to cover most users who care about a stable guest view, but
don't specify the value for each qdev property.

Kevin