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

[Peter Crosthwaite]

On Fri, Jul 13, 2012 at 6:33 PM, Markus Armbruster <address@hidden> wrote:
> 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?
>

So the concept is the Machine itself (devices and their properties)
are created by init(), and machine state (memory and disk contents,
vmsd type stuff) is created by init_state(). By extension, any
migratable data must not be populated or manipulated until
state_init().

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

Well another policy is to allow init to bdrv_read, just not allow to
set any device state. Getting geo from the MBR seems OK to me, because
your not actually loading any device state, your acquiring a best
guess at the unspecified device properties. Theres only one corner
case left, and that is changing the MBR in the migration window. But
you can adopt something of a "better than nothin" approach here, as I
dont a see a solution here without changing the Migration process
(that Kevin summarised). This corner case exists even if the code
remains unchanged.

Regards,
Peter
>
> 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.