Re: [Qemu-devel] [RFC] Removing brdv_read()s from device init functions

[Markus Armbruster]

Peter Crosthwaite <address@hidden> writes:

> Hi All,
>
> This RFC comes from the recent discussion Re coroutines and the block
> layer - the current topic of disucussion there has shifted to
> "bdrv_read() from device init", so rather than continuing the
> discussion as a tangent to the unrelated original topic I'm recreating
> the thread.
>
> So anyways, there are several motivations to remove bdrv_read() from
> init functions. Kevin points out that it breaks migration as the
> reading of machine state at init stage will read out-of-date data. It
> also is the source of my coroutine bug. It also is a worst case
> performance wise.

Peter asked me to comment re encypted images.  In my opinion, image
encryption is a "feeling lucky" feature, and we shouldn't complicate QOM
or qdev to work around its deficiencies, such as keys becoming available
too late.  But here goes anyway: it also breaks with encrypted images.
Details in Message-ID: <address@hidden>
http://lists.nongnu.org/archive/html/qemu-devel/2012-07/msg01572.html

>                   The big question is how for existing models do we
> fix it?
>
> One policy suggested is to ban bdrv_read() from init period and
> require devices to Lazy init. That is, on the first load, do the read
> you were going to do at init time. Peter Maydell points out that this
> has the drawback of late detection of errors, I.E. errors that should
> really be picked up at load time are not picked up until first access
> (like the bdrv file being too small for the device).

Outlawing data read/write in init() doesn't mean we must also outlaw
examining image meta-data, such as size.  We just need to be clear what
exactly is permitted.  Even better: catch violations.

> Kevin propsoses a second init function for devices. I guess the way
> this is implemented is TYPE_DEVICE or TYPE_OBJECT has  ->init_state()
> or some such that is called once the the machine is ready to migrate.
> The bdrv_read is moved from ->init() to ->init_state(). You may or may
> not convert to bdrv_aio_read() but it doesn't matter for that
> approach.

We need to have a clear idea of what the various device methods are
supposed to do.

In the case of init() and init_state(): what's their contract?

In particular, which one completes property initialization?  Right now,
we do create() - set properties - init(), and init() checks property
values, supplies defaults, and so forth.  After init(), property values
shouldn't change.

Example: disk geometry.  If the geometry properties haven't been set,
init() supplies defaults.  Computing defaults requires reading the MBR.
If that's no longer allowed in init(), and has to be done in
init_state(), then property initialization completes only in
init_state().  I doubt that's what we want.

The disk geometry problem can be solved by getting rid of the guessing
from MBR.

I believe the more general problem "encryption keys not available in
init()" can also be solved, but it'll involve hairy rearrangement of the
startup code.

> I came up with the idea of just using AIO to delay the read until the
> appropriate time. The bdrv_read in init is converted to a
> bdrv_aio_read(), and AIO yields the created coroutine when it figures
> out that the machine is not migration ready. When the machine is ready
> the coroutines is re-entered.
>
> I have CCd some of the QOM people as i noticed they were discussing
> some infrastracture for late device init. This may or may not be
> related.