[libvirt PATCH 6/6] qemu: implement vhost-user-blk support

[Pavel Hrdina]

On Wed, Feb 03, 2021 at 11:04:04AM +0100, Peter Krempa wrote:
> On Tue, Feb 02, 2021 at 16:04:12 +0100, Pavel Hrdina wrote:
> > Implements QEMU support for vhost-user-blk together with live
> > hotplug/unplug.
> > 
> > Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> > ---
> >  src/qemu/qemu_block.c                         | 16 ++++
> >  src/qemu/qemu_block.h                         |  5 +
> >  src/qemu/qemu_command.c                       | 91 +++++++++++++++++--
> >  src/qemu/qemu_command.h                       |  8 ++
> >  src/qemu/qemu_domain.c                        |  5 +
> >  src/qemu/qemu_hotplug.c                       |  5 +-
> >  src/qemu/qemu_validate.c                      | 13 +++
> >  .../disk-vhostuser.x86_64-latest.args         | 41 +++++++++
> >  tests/qemuxml2argvtest.c                      |  1 +
> >  9 files changed, 176 insertions(+), 9 deletions(-)
> >  create mode 100644 tests/qemuxml2argvdata/disk-vhostuser.x86_64-latest.args
> 
> [...]
> 
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 3e652e18b7..059126cfeb 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -1712,9 +1712,16 @@ qemuBuildDiskDeviceStr(const virDomainDef *def,
> >          break;
> >  
> >      case VIR_DOMAIN_DISK_BUS_VIRTIO:
> > -        if (qemuBuildVirtioDevStr(&opt, "virtio-blk", qemuCaps,
> > -                                  VIR_DOMAIN_DEVICE_DISK, disk) < 0) {
> > -            return NULL;
> > +        if (virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_VHOST_USER) {
> > +            if (qemuBuildVirtioDevStr(&opt, "vhost-user-blk", qemuCaps,
> > +                                      VIR_DOMAIN_DEVICE_DISK, disk) < 0) {
> > +                return NULL;
> > +            }
> > +        } else {
> > +            if (qemuBuildVirtioDevStr(&opt, "virtio-blk", qemuCaps,
> > +                                      VIR_DOMAIN_DEVICE_DISK, disk) < 0) {
> > +                return NULL;
> > +            }
> 
> Preferably, add a temporary string holding either "vhost-user-blk" or
> "virtio-blk" and call qemuBuildVirtioDevStr just once, since all other
> arguments are the same.

This would require adding one level of nesting because of the temporary
variable or I would have to define the variable at the top of the
function. Not sure if it makes it better.

> 
> >          }
> >  
> >          if (disk->iothread)
> 
> [...]
> 
> 
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index 0c078a9388..aa6d539610 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -10581,6 +10581,11 @@ qemuDomainPrepareDiskSource(virDomainDiskDefPtr disk,
> >              disk->src->format = VIR_STORAGE_FILE_RAW;
> >      }
> >  
> > +    /* Nothing else to prepare as it will use -chardev instead
> > +     * of -blockdev/-drive option. */
> > +    if (disk->src->type == VIR_STORAGE_TYPE_VHOST_USER)
> > +        return 0;
> 
> The code above sets 'format' for storage pool backed disks, but since
> format isn't something we'd be setting for vhost-user this exemption
> should be at the beginning of the function.

Fixed.

> > +
> >      if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV) &&
> >          !qemuDiskBusIsSD(disk->bus)) {
> >          if (qemuDomainPrepareDiskSourceBlockdev(disk, priv, cfg) < 0)
> 
> 
> Missing stuff from this commit:
> 
> - hot-unplug support
>     - see qemuDomainRemoveDiskDevice where the chardev isn't deleted
> - blockjob lockout
>     - qemuDomainDiskBlockJobIsSupported
> - block iotune lockout
> - possible better lockout for qemuDomainBlockPeek
>     - it will barf either that the file is not 'raw' or that it can't be
>       initialized
> - possible better lockout for qemuDomainSetBlockThreshold
>     - "threshold currently can't be set for block device '%s'" <- it
>       will never be possible
> - lockout for qemuDomainGetBlockInfo
> - lockout for qemuDomainBlockResize
> - lockout for qemuDomainBlockStats(Flags)
> - handling in bulk stats (qemuDomainGetStatsBlockExportDisk ?)

Well, that's a lot of missing things, thanks for the pointers, I'll look
into it.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20210203/7d656b9b/attachment-0001.sig>