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

[Michael S. Tsirkin]

On Thu, Nov 29, 2012 at 03:45:55PM +0100, Stefan Hajnoczi wrote:
> 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.

Hmm but PCI device can still go away if
guest ejects it. Does this work fine?

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

Not really, just pass it all necessary data.

> 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

Yes but then it's not contained.