[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Peter Crosthwaite
Subject: Re: [Qemu-devel] [RFC] Removing brdv_read()s from device init functions
Date: Fri, 13 Jul 2012 19:18:44 +1000

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

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

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]