Re: [Qemu-devel] [PATCH v4 11/11] virtio-blk: add x-data-plane=on|off performance feature

[Stefan Hajnoczi]

On Thu, Nov 29, 2012 at 03:12:35PM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 22, 2012 at 04:16:52PM +0100, Stefan Hajnoczi wrote:
> > The virtio-blk-data-plane feature is easy to integrate into
> > hw/virtio-blk.c.  The data plane can be started and stopped similar to
> > vhost-net.
> > 
> > Users can take advantage of the virtio-blk-data-plane feature using the
> > new -device virtio-blk-pci,x-data-plane=on property.
> > 
> > The x-data-plane name was chosen because at this stage the feature is
> > experimental and likely to see changes in the future.
> > 
> > If the VM configuration does not support virtio-blk-data-plane an error
> > message is printed.  Although we could fall back to regular virtio-blk,
> > I prefer the explicit approach since it prompts the user to fix their
> > configuration if they want the performance benefit of
> > virtio-blk-data-plane.
> 
> Not only that, this affects features exposed to guest so it really can't be
> trasparent.
> 
> Which reminds me - shouldn't some features be turned off?
> For example, VIRTIO_BLK_F_SCSI?

Yes, virtio-blk-data-plane only starts when you give -device
virtio-blk-pci,scsi=off,x-data-plane=on.  If you use scsi=on an error
message is printed.

> > Limitations:
> >  * Only format=raw is supported
> >  * Live migration is not supported
> 
> This is probably fixable long term?

Absolutely.  There are two parts:

1. Marking written memory dirty so live RAM migration can work.  Missing
   today, easy cheat is to switch off virtio-blk-data-plane and silently
   switch to regular virtio-blk emulation while memory dirty logging is
   enabled.  The more long-term solution is to actually communicate the
   dirty information back to the memory API.

2. Synchronizing virtio-blk-data-plane vring state with virtio-blk so
   save/load works.  This should be relatively straightforward.

I don't want to gate this patch series on live migration support but it
is on my TODO list for virtio-blk-data-plane after this initial series
has been merged.

> >  * Block jobs, hot unplug, and other operations fail with -EBUSY
> 
> Hmm I don't see code to disable PCU unplug in this patch.
> I expected no_hotplug to be set.
> Where is it?

It uses the bdrv_in_use() mechanism.

> >  * I/O throttling limits are ignored
> 
> And this?
> Meanwhile can we have attempts to set them fail?

This limitation exists because virtio-blk-data-plane today bypasses the
QEMU block layer.  The next step is to get the block layer working
inside the data plane thread.  At that point I/O limits work again.

Adding an error would be a layering violation because I/O throttling
happens in the QEMU block layer and is unaware of the emulated storage
controller (virtio-blk, IDE, SCSI, etc).

I think it's better to document the limitation and continue working on
AioContext so that we can soon support I/O throttling with
virtio-blk-data-plane.  It would be quite ugly to add checks.

> > @@ -33,6 +35,8 @@ typedef struct VirtIOBlock
> >      VirtIOBlkConf *blk;
> >      unsigned short sector_mask;
> >      DeviceState *qdev;
> > +    VirtIOBlockDataPlane *dataplane;
> > +    Error *migration_blocker;
> 
> Would be nice to move the migration disabling
> checking supported formats
> and all the rest of it out to dataplane code.

The reason to do it in virtio-blk.c is that we already have access to
the device configuration.  If we move it to hw/dataplane/virtio-blk.c
then that code needs to reach inside and check data that it doesn't
otherwise access.

IMO it's nice to keep data plane "dumb" and perform these checks where
we already have to deal with the relationship between VirtIOBlkConf and
friends.

Stefan