Re: [Qemu-devel] BlockDriverState stack and BlockListeners

[Paolo Bonzini <pbonzini@redhat.com>]

On 02/21/2012 10:49 AM, Kevin Wolf wrote:
> Am 21.02.2012 10:15, schrieb Paolo Bonzini:
>> On 02/21/2012 10:03 AM, Kevin Wolf wrote:
>>> - Old style block migratiom
>>
>> More precisely, dirty bitmap handling.
> 
> Yes, but is it used anywhere else?

No, just nitpicking.

>>> (btw, we should deprecate this)
>>
>> Yeah, but we need blkmirror to provide an alternative.
> 
> Does block migration even work meanwhile without corrupting things left
> and right?

No idea.  Somebody must have used it at some point. :)

>> 1) BlockListener would be by-design coroutine based; I know we disagree
>> on this (you want to change raw to coroutines long term; I would like to
>> reintroduce some AIO fastpaths when there are no active listeners).
> 
> I can't see a technical reason why a BlockListener could not be callback
> based. The only reason might be a "there are only coroutines" policy.

Not a technical reason, more of a sanity reason.  Coroutines were
introduced to allow a number of the things in the list.

>> 2) BlockListener would be entirely an implementation detail, used in the
>> implementation of other commands.
> 
> Depending on what you mean by command (presumably QMP commands?),

QMP commands, command-line (-drive), whatever.

> And on the other hand, protocols like file are entirely implementation
> details as well, and still they are BlockDrivers.

True.  Formats and protocols also do not have perfectly overlapping
functionality (backing images are only a format thing, for example).

>>> The main difference that I see is that the listeners stay always on top.
>>> For example, let's assume that if implemented a blkmirror driver in
>>> today's infrastructure, you would get a BlockDriverState stack like
>>> blkmirror -> qcow2 -> file. If you take a live snapshot now, you don't
>>> want to have the blkmirror applied to the old top-level image, which is
>>> now a read-only backing file. Instead, it should move to the new
>>> top-level image.
>>
>> Yes, that's because a BlockListener always applies to a
>> BlockDriverState, and live snapshots close+reopen the BDS but do not
>> delete/recreate it.
> 
> Hm, that's an interesting angle to look at it. The reasoning makes sense
> to me (though I would reword it as a BlockListener belongs to a
> drive/block device rather than a BDS, which is an implementation detail).

Yes.

> However, live snapshots can't close and reopen the BDS in the future,
> because the reopen could fail and you must not have closed the old image
> yet in this case. So what Jeff and I were looking into recently is to
> change this so that new top-level images are opened first without
> backing file and then the backing file relationship is created with the
> existing BDS.
> 
> Of course, we stumbled across the thing that you're telling me here,
> that devices refer to the same BDS as before. So their content must be
> swapped, but some data like the device name (and now BlockListeners)
> stay with the top-level image instead.
> 
> Which in turn reminds me of a discussion I had with Stefan a while ago,
> where we came to the conclusion that we need to separate the
> representation of an image file and a "view" of it which represents a
> block device (as a whole). [...]
> 
> Isn't it cool how everything is connected with everything? :-)

:)

So we'd have:

- Protocols -- Protocols tell you where to get the raw bits.
- Formats -- Formats transform those raw bits into a block device.
- Views -- Views can move from a format to another.  A format can use a
default view implementation, or provide its own (e.g. to access
different snapshots).
- Listeners -- I think a view can have-a listener?

with the following relationship:

- A format has-a protocol for the raw bits and has-a view for the
backing file.
- A view has-a format, a device has-a view.
- A view can have-a listener?  Or is it formats?

But I think we're digressing...

>>> So maybe we just need to extend the current BlockDriverState stack to
>>> distinguish "normal" and "always on top" BlockDrivers, where the latter
>>> would roughly correspond to BlockListeners?
>>
>> I would prefer having separate objects.  Even if you do not count fields
>> related to throttling or copy-on-read or other tasks in the list above,
>> there are many fields in BDS that do not really apply to BlockListeners.
>>  Backing files, device ops, encryption, even size.  Having extra methods
>> is not a big problem, but unwanted data items smell...
> 
> Most other block drivers use only little of them. We can try to clean up
> some of them (and making the separation described above would probably
> help with it), but BlockListeners aren't really different here from
> existing drivers.

True, the question only matters insofar as having a separate data
structure simplifies the design.  ("Simplify" means "we can actually
understand it and be reasonably sure that it's correct and implementable").

Paolo