Re: [Qemu-devel] BlockDriverState stack and BlockListeners

[Paolo Bonzini]

On 02/21/2012 10:03 AM, Kevin Wolf wrote:
> So let's check which features could make use of it:
> 
> - Copy on read
> - I/O throttling
> - blkmirror for precopy storage migration
> - replication agent
> - Old style block migratiom

More precisely, dirty bitmap handling.

> (btw, we should deprecate this)

Yeah, but we need blkmirror to provide an alternative.

> - Maybe even bdrv_check_request and high watermark? However, they are
>   not optional, so probably makes less sense.
> 
> I think these are enough cases to justify it. Now, which operations do
> we need to intercept?
> 
> - bdrv_co_read
> - bdrv_co_write
> - bdrv_drain (btw, we need a version for only one BDS)
> - Probably bdrv_co_discard as well

Yes.

> Anything I missed?

bdrv_co_flush, I think.

> Now the interesting question that comes to mind is:
> What is really the difference between the proposed BlockListener and a
> BlockDriver? Sure, a listener would implement much less functionality,
> but we also have BlockDrivers today that implement very few of the
> callbacks.

The two differences that come to mind are:

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

2) BlockListener would be entirely an implementation detail, used in the
implementation of other commands.

Third, perhaps the interface to BlockListener could be
bdrv_before/after_read.  Cannot really say without writing one or two
BlockListeners whether this would be useful or a limitation.

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

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

Paolo